Hi Adrian,

On 30/09/2015 23:30, Adrian Alonso wrote:

Rework secure boot support for imx6 and imx7 Move existing hab support for imx6 into imx-common for SoC reuse.

Message is reused from V1 - but patch only moves code into imx-common without doing anything for i.mx7. Please fix the commit message.

Signed-off-by: Adrian Alonso aalonso@freescale.com

Changes for V2: Split patch

• Move existing hab mx6 into common location
• Update include hab header location

arch/arm/cpu/armv7/mx6/Makefile | 1 - arch/arm/imx-common/Makefile | 1 + arch/arm/{cpu/armv7/mx6 => imx-common}/hab.c | 2 +- arch/arm/include/asm/{arch-mx6 => imx-common}/hab.h | 0 4 files changed, 2 insertions(+), 2 deletions(-) rename arch/arm/{cpu/armv7/mx6 => imx-common}/hab.c (99%) rename arch/arm/include/asm/{arch-mx6 => imx-common}/hab.h (100%)

diff --git a/arch/arm/cpu/armv7/mx6/Makefile b/arch/arm/cpu/armv7/mx6/Makefile index bf6effc..8af191d 100644 --- a/arch/arm/cpu/armv7/mx6/Makefile +++ b/arch/arm/cpu/armv7/mx6/Makefile @@ -9,5 +9,4 @@

obj-y := soc.o clock.o obj-$(CONFIG_SPL_BUILD) += ddr.o -obj-$(CONFIG_SECURE_BOOT) += hab.o obj-$(CONFIG_MP) += mp.o diff --git a/arch/arm/imx-common/Makefile b/arch/arm/imx-common/Makefile index 1698d06..4abffee 100644 --- a/arch/arm/imx-common/Makefile +++ b/arch/arm/imx-common/Makefile @@ -27,6 +27,7 @@ ifeq ($(SOC),$(filter $(SOC),mx6 mx7)) obj-y += cache.o init.o obj-$(CONFIG_CMD_SATA) += sata.o obj-$(CONFIG_IMX_VIDEO_SKIP) += video.o +obj-$(CONFIG_SECURE_BOOT) += hab.o endif ifeq ($(SOC),$(filter $(SOC),vf610)) obj-y += ddrmc-vf610.o diff --git a/arch/arm/cpu/armv7/mx6/hab.c b/arch/arm/imx-common/hab.c similarity index 99% rename from arch/arm/cpu/armv7/mx6/hab.c rename to arch/arm/imx-common/hab.c index 27cabe4..9ee0f12 100644 --- a/arch/arm/cpu/armv7/mx6/hab.c +++ b/arch/arm/imx-common/hab.c @@ -7,9 +7,9 @@ #include <common.h> #include <asm/io.h> #include <asm/system.h> -#include <asm/arch/hab.h> #include <asm/arch/clock.h> #include <asm/arch/sys_proto.h> +#include <asm/imx-common/hab.h>

/* -------- start of HAB API updates ------------*/

diff --git a/arch/arm/include/asm/arch-mx6/hab.h b/arch/arm/include/asm/imx-common/hab.h similarity index 100% rename from arch/arm/include/asm/arch-mx6/hab.h rename to arch/arm/include/asm/imx-common/hab.h

Best regards, Stefano Babic

Hi Adrian,

On 30/09/2015 23:30, Adrian Alonso wrote:

Add CONFIG_ROM_UNIFIED_SECTIONS for mx6sx and mx6ul target platforms to resolve corresponding HAB_RVT_BASE

This is the only explanation for the patch. It does not explain the name ROM_UNIFIED_SECTION and what it has to do with HAB_RVT_BASE. I agree that users must read the manual, but even a better and longer description in the commit message helps.

Signed-off-by: Adrian Alonso aalonso@freescale.com

Chages for V2: New patch in series

arch/arm/include/asm/imx-common/hab.h | 2 +- include/configs/mx6_common.h | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/arm/include/asm/imx-common/hab.h b/arch/arm/include/asm/imx-common/hab.h index d0eaa67..af77d45 100644 --- a/arch/arm/include/asm/imx-common/hab.h +++ b/arch/arm/include/asm/imx-common/hab.h @@ -113,7 +113,7 @@ typedef void hapi_clock_init_t(void); #define HAB_ENG_RTL 0x77 /* RTL simulation engine */ #define HAB_ENG_SW 0xff /* Software engine */

-#ifdef CONFIG_MX6SX +#ifdef CONFIG_ROM_UNIFIED_SECTIONS #define HAB_RVT_BASE 0x00000100 #else #define HAB_RVT_BASE 0x00000094 diff --git a/include/configs/mx6_common.h b/include/configs/mx6_common.h index ef4cb68..3c7138a 100644 --- a/include/configs/mx6_common.h +++ b/include/configs/mx6_common.h @@ -117,4 +117,8 @@ #define CONFIG_CMD_FUSE #define CONFIG_MXC_OCOTP

+#if (defined(CONFIG_MX6SX) || defined(CONFIG_MX6UL)) +#define CONFIG_ROM_UNIFIED_SECTIONS +#endif

#endif

This is not ok. We are in the process to move all CONFIG_ from the board configuration file to Kconfig. New CONFIG_ should be added only via Kconfig.

As ROM_UNIFIED_SECTIONS is like a property for the SOC, the selection of the SOC type must add automatically it to the config. You can see as example SYS_L2CACHE_OFF for i.mx6ul.

Best regards, Stefano Babic

Hi Adrian,

On 30/09/2015 23:30, Adrian Alonso wrote:

Add mx7 secure boot support, reuse existing mx6 hab

Signed-off-by: Adrian Alonso aalonso@freescale.com

Changes for V2:

• Split from original patch to track mx7 change set hab: rework support for imx6/imx7

arch/arm/imx-common/hab.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/arch/arm/imx-common/hab.c b/arch/arm/imx-common/hab.c index 9ee0f12..565a838 100644 --- a/arch/arm/imx-common/hab.c +++ b/arch/arm/imx-common/hab.c @@ -261,12 +261,25 @@ uint8_t hab_engines[16] = { bool is_hab_enabled(void) { struct ocotp_regs *ocotp = (struct ocotp_regs *)OCOTP_BASE_ADDR;

• uint32_t reg;

+#if CONFIG_MX7

Usage of SOC's runtime detection is vanished by introducing new #ifdef section. I am aware there some registers defined for a SOC and not for another. This is the point. We should make the access unaware for the driver (this file).

hab.c does not yet contain any nasty #ifdef SOC_TYPE - we should have the same in future.

• struct fuse_bank *bank = &ocotp->bank[1];
• struct fuse_bank1_regs *fuse =

• (struct fuse_bank1_regs *)bank->fuse_regs;
  

• reg = readl(&fuse->cfg0);

+#elif CONFIG_MX6 struct fuse_bank *bank = &ocotp->bank[0]; struct fuse_bank0_regs *fuse = (struct fuse_bank0_regs *)bank->fuse_regs;

• uint32_t reg = readl(&fuse->cfg5);

• reg = readl(&fuse->cfg5);

+#endif

There are some ways to solve it. For example (but it is not the only solution), the offset can be set into the corresponding imx_regs as offsetof(..) and which fuse bank can be picked up from a table here fuse_bank_choosen = {0 ,1 }

and then struct fuse_bank *bank; if (is_cpu_type(MXC_CPU_MX7)) bank = &ocotp->bank[1]; else bank = &ocotp->bank[0];

or you find a better solution, of course - but do not introduce new #ifdef. If you see, hab.c contains at the very beginning a lot of stuff hab_rvt_report_* that makes runtime detection against compiler switches.

• if (is_soc_type(MXC_SOC_MX7))

• return (reg & 0x2000000) == 0x2000000;
  

• else if (is_soc_type(MXC_SOC_MX6))

• return (reg & 0x2) == 0x2;
  

If I did not know that all i.MXes are little endian, it looks like an endianess problem. But I suggest to add a macro for a better readiness:

#define IS_HAB_ENABLED_BIT (is_soc_type(MXC_SOC_MX7) ? 0x2000000 : 0x2)

and then you can use here simply:

return (reg & IS_HAB_ENABLED_BIT) == IS_HAB_ENABLED_BIT

	 * crash.
	 */
	/* Check MMU enabled */

• 	if (get_cr() & CR_M) {
  

• 	if (is_soc_type(MXC_SOC_MX6) && get_cr() & CR_M) {
  

There is a long explanation before this code. As it depends from MMU only, it does not explain why it is not required for MX7. Please extend the comment and put a full explanation here.

Best regards, Stefano Babic