Add Atmel SAMA5 platform support #1714

nodeax · 2017-07-24T21:05:34Z

Adding support for Atmel/Microchip SAMA5D2-XULT board

The processor supports:
ARM TrustZone, configuring memory and/or peripherals as secure, secure RTC, Secure boot, On-the-fly encryption/decryption of the DDR bus, Tamper protection etc. Datasheet.

Instructions to build optee_os along with bootlog, test results can be found here.

nodeax · 2017-07-25T13:08:03Z

Realized the memory map in the comment needs to be updated. optee runs in the middle of the DDR (as shown in SAMA5D2 memory map). Long story: As you said on ARM32 linux putting optee in lower 128M means Linux can not use that memory. However I have an additional restriction on SAMA5D2 where memory protection works in chunks of 128MiB. Inside the 128MiB chunk the lower memory has fine resolution for configuring as secure memory but high memory can only be configured as a single 64MiB secure memory chunk. So to avoid unnecessarily setting aside 64MiB for secure memory, I put op-tee at the start of a 128MiB boundary in DDR. End of DDR memory + * +------------------+ + * | Linux memory | + * +------------------+ + * | SHMEM | + * +------------------+ + * | | TA_RAM | + * + TZDRAM +---------+ + * | | TEE_RAM | + * +------------------+ + * | Linux memory | + * +--------+---------+ Start of DDR memory /* * SAMA5D2 memory map * * 0x4000_0000 - * Linux/other | * 0x30C0_0000 - * Shared memory: 4 MiB (CFG_SHMEM_SIZE) | * 0x3080_0000 [CFG_SHMEM_START] - * TA RAM: 7 MiB (remaining memory) | * 0x3010_0000 [CFG_TA_RAM_START] - * TEE RAM: 1 MiB (CFG_TEE_RAM_VA_SIZE) | * 0x3000_0000 [CFG_TEE_RAM_START/LOAD_ADDR] - * Linux/other | * 0x2000_0000 [DRAM0_BASE] - * */

jforissier

For the first commit ("plat-sam: Add support..."), please see my review comments. I suggest you add the platform description above to the commit text, and also add a Link: tag to link to the datasheet.
For the second commit ("arm: pl310: Support processors without SCU"):
Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

jforissier · 2017-07-25T13:06:04Z

core/arch/arm/plat-sam/main.c

+ return AT91C_BASE_MATRIX64;
+}
+
+static int matrix_configure_slave(void)


The function always returns 0, so you should return void instead

jforissier · 2017-07-25T13:16:47Z

core/arch/arm/plat-sam/conf.mk

+
+CFG_WITH_STACK_CANARIES ?= y
+
+CFG_PL310=y


When setting CFG_ values, please use either ?= or $(call force, ...). This commit: dffb004951df explains when to use one or the other.
Same for CFG_AT91_MATRIX below.

jforissier · 2017-07-25T13:36:34Z

core/arch/arm/plat-sam/main.c

+ unsigned int array_size;
+ int ret;
+
+ array_size = sizeof(security_ps_peri_id) / sizeof(unsigned int);


Use ARRAY_SIZE from <util.h>

jforissier · 2017-07-25T13:54:09Z

core/arch/arm/plat-sam/tz_matrix.h

+#define MATRIX_MEIER 0x0150 /* Master Error Interrupt Enable Register */
+#define MATRIX_MEIDR 0x0154 /* Master Error Interrupt Disable Register */
+#define MATRIX_MEIMR 0x0158 /* Master Error Interrupt Mask Register */
+#define MATRIX_MESR 0x015c /* Master Error Statue Register */


s/Statue/Status/

jforissier · 2017-07-25T14:01:27Z

core/arch/arm/plat-sam/matrix.c

+ .matrix = MATRIX_H64MX,
+ .security_type = SECURITY_TYPE_PS,
+ },
+ /* ARM */


This comment (and all the others below) are kind of useless since they simply repeat what's written into .peri_id, they don't really improve readability either IMO. But you can keep them if you prefer.

jforissier · 2017-07-25T14:03:14Z

core/arch/arm/plat-sam/platform_config.h

+ * Treats shared accesses (bit22=0, bit13=0)
+ * Parity disabled (bit21=0)
+ * Event monitor disabled (bit20=0)
+ * Platform fmavor specific way config:


s/fmavor/flavor/

jforissier · 2017-07-25T14:03:35Z

core/arch/arm/plat-sam/platform_config.h

+ *
+ * I/Dcache prefetch enabled (bit29:28=2b11)
+ * NS can access interrupts (bit27=1)
+ * NS can lockown cache lines (bit26=1)


s/lockown/lockdown/

jforissier · 2017-07-25T14:08:42Z

core/arch/arm/plat-sam/tz_matrix.h

+#ifndef __TZ_MATRIX_H__
+#define __TZ_MATRIX_H__
+
+#define MATRIX_MCFG(n) (0x0000 + (n) * 4)/* Master Configuration Register */


Space after ) and before /*

jforissier · 2017-07-25T14:10:30Z

core/include/drivers/atmel_uart.h

+};
+
+void atmel_uart_init(struct atmel_uart_data *pd, paddr_t base);
+void set_mmu_flag(void);


Unused? (remove)

nodeax · 2017-07-25T15:58:52Z

@jforissier : Thanks for all the review comments/suggestions. All of them have been addressed in the updated pull request.
@MrVan: The memory map comment has been updated to reflect the correct layout.

igoropaniuk · 2017-07-25T16:39:00Z

core/arch/arm/plat-sam/tz_matrix.h

+ * NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE,
+ * EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+#ifndef __TZ_MATRIX_H__


minor: different styles of header guards are used through all header files (sometimes wrapped with __ __, sometimes without them)

Thanks, fixed in latest update.

jforissier

Some more comments.

In the commit description, please move the Link: tag next to the Signed-off-by: without any blank line in between, like so:

Commit subject

Commit description
(several lines)

Link: ...
Signed-off-by: ...

jforissier · 2017-07-26T13:06:57Z

core/arch/arm/plat-sam/matrix.c

+
+ for (i = 0; i < ARRAY_SIZE(peri_security_array); i++) {
+ if (peri_id == peri_security_array[i].peri_id)
+ break;


return &peri_security_array[i]; here and return NULL; after the loop.

jforissier · 2017-07-26T13:07:24Z

core/arch/arm/plat-sam/matrix.c

+ matrix_write(matrix_base, MATRIX_SASSR(slave), srsplit_setting);
+}
+
+static struct peri_security *get_peri_security(unsigned int peri_id)


Should probably return const struct peri_security *

jforissier · 2017-07-26T13:11:15Z

core/arch/arm/plat-sam/matrix.h

+ unsigned int size);
+
+extern vaddr_t matrix32_base(void);
+extern vaddr_t matrix64_base(void);


Are those extern needed? I don't think so.

matrix32/64_base() is being used in both matrix.c and main.c, hence it is included in the header.

I have fixed all other review comments in the updated pull request. Thanks for reviewing.

Huh, sorry for not being clear, I meant the extern keyword is not needed, is it?

sorry, misread the original comment. no they are not needed, fixed in latest update.

jforissier · 2017-07-26T13:19:02Z

core/arch/arm/plat-sam/matrix.c

+ struct peri_security *periperal_sec;
+ int ret;
+
+ if ((peri_id_array == NULL) || (size == 0))


if (!peri_id_array || !size)

jforissier · 2017-07-26T13:19:22Z

core/arch/arm/plat-sam/matrix.c

+ peri_id_p = peri_id_array;
+ for (i = 0; i < size; i++) {
+ periperal_sec = get_peri_security(*peri_id_p);
+ if (periperal_sec == NULL)


if (!peripheral_sec)

jforissier · 2017-07-26T13:23:29Z

core/arch/arm/plat-sam/matrix.c

+ },
+};
+
+static inline void matrix_write(unsigned int base,


Drop inline

jforissier · 2017-07-26T13:23:35Z

core/arch/arm/plat-sam/matrix.c

+ write32(value, offset + base);
+}
+
+static inline unsigned int matrix_read(int base, unsigned int offset)


Drop inline

jforissier · 2017-07-26T13:29:28Z

core/arch/arm/plat-sam/main.c

+ AT91C_ID_CHIPID,
+};
+
+static int matrix_config_periheral(void)


Typo, and perhaps plural is better? (matrix_config_peripherals)

jforissier · 2017-07-26T13:34:19Z

core/arch/arm/plat-sam/main.c

+ if (ret)
+ return -1;
+
+ return 0;


Simplify:

static int matrix_config_peripherals(void) { return matrix_configure_peri_security(security_ps_peri_id, ARRAY_SIZE(security_ps_peri_id)); }

jforissier · 2017-07-26T13:35:11Z

core/arch/arm/plat-sam/main.c

+ if (ret)
+ return -1;
+
+ return 0;


Simplify: return matrix_config_peripherals();

jenswi-linaro · 2017-08-15T13:36:51Z

Is this review still active?

jforissier · 2017-08-16T08:52:00Z

LGTM, @nodeax you may add my Ack to the 2nd commit, too.
Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

Not all processors might have a SCU unit. So conditionally include code that configures SCU. Signed-off-by: Akshay Bhat <akshay.bhat@timesys.com> Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

Add basic support to get op-tee to run on SAMA5D2-XULT board. The SoC is based on single core ARM Cortex-A5 and supports: ARM TrustZone with support for configuring memory/peripherals as secure Secure RTC Secure boot On-the-fly encryption/decryption of DDR bus Tamper protection Link: http://www.atmel.com/Images/Atmel-11267-32-bit-Cortex-A5-Microcontroller-SAMA5D2_Datasheet.pdf Signed-off-by: Akshay Bhat <akshay.bhat@timesys.com> Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

nodeax · 2017-08-28T15:33:37Z

@jforissier : Thanks for the review, added you Ack to the 2nd commit.
@jenswi-linaro : Was on vacation. Rebased/Verified everything still works after pulling in changes from master and also added Ack from jforissier.

jforissier · 2017-08-31T08:05:29Z

@nodeax thanks for contributing!

jforissier reviewed Jul 25, 2017

View reviewed changes

igoropaniuk reviewed Jul 25, 2017

View reviewed changes

jforissier reviewed Jul 26, 2017

View reviewed changes

nodeax added 2 commits August 28, 2017 11:11

arm: pl310: Support processors without SCU

de16d35

Not all processors might have a SCU unit. So conditionally include code that configures SCU. Signed-off-by: Akshay Bhat <akshay.bhat@timesys.com> Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

jforissier merged commit e20d1bc into OP-TEE:master Aug 31, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Atmel SAMA5 platform support #1714

Add Atmel SAMA5 platform support #1714

nodeax commented Jul 24, 2017

nodeax commented Jul 25, 2017 via email

jforissier left a comment

jforissier Jul 25, 2017

jforissier Jul 25, 2017

jforissier Jul 25, 2017

jforissier Jul 25, 2017

jforissier Jul 25, 2017

jforissier Jul 25, 2017

jforissier Jul 25, 2017

jforissier Jul 25, 2017

jforissier Jul 25, 2017

nodeax commented Jul 25, 2017

igoropaniuk Jul 25, 2017

nodeax Jul 25, 2017

jforissier left a comment

jforissier Jul 26, 2017

jforissier Jul 26, 2017

jforissier Jul 26, 2017

nodeax Jul 26, 2017

jforissier Jul 26, 2017

nodeax Jul 26, 2017

jforissier Jul 26, 2017

jforissier Jul 26, 2017

jforissier Jul 26, 2017

jforissier Jul 26, 2017

jforissier Jul 26, 2017

jforissier Jul 26, 2017

jforissier Jul 26, 2017

jenswi-linaro commented Aug 15, 2017

jforissier commented Aug 16, 2017

nodeax commented Aug 28, 2017

jforissier commented Aug 31, 2017


		CFG_WITH_STACK_CANARIES ?= y

		CFG_PL310=y

Add Atmel SAMA5 platform support #1714

Add Atmel SAMA5 platform support #1714

Conversation

nodeax commented Jul 24, 2017

nodeax commented Jul 25, 2017 via email

jforissier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nodeax commented Jul 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jforissier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro commented Aug 15, 2017

jforissier commented Aug 16, 2017

nodeax commented Aug 28, 2017

jforissier commented Aug 31, 2017