[U-Boot] [PATCH v2 1/5] imx: cpu: move common chip revision id's

[U-Boot] [PATCH v2 1/5] imx: cpu: move common chip revision id's

[Adrian Alonso]

Move common chip revision id's to main cpu header file
mx25 generic include cpu header for chip revision

Signed-off-by: Adrian Alonso <aalonso@freescale.com>
---
Chages for V2: Resend

 arch/arm/cpu/arm926ejs/mx25/generic.c     |  1 +
 arch/arm/include/asm/arch-imx/cpu.h       | 12 ++++++++++++
 arch/arm/include/asm/arch-mx25/imx-regs.h |  4 ----
 arch/arm/include/asm/arch-mx5/imx-regs.h  | 11 -----------
 arch/arm/include/asm/arch-mx6/imx-regs.h  |  4 ----
 5 files changed, 13 insertions(+), 19 deletions(-)

diff --git a/arch/arm/cpu/arm926ejs/mx25/generic.c b/arch/arm/cpu/arm926ejs/mx25/generic.c
index 8912098..0b1a8f4 100644
--- a/arch/arm/cpu/arm926ejs/mx25/generic.c
+++ b/arch/arm/cpu/arm926ejs/mx25/generic.c
@@ -13,6 +13,7 @@
 #include <div64.h>
 #include <netdev.h>
 #include <asm/io.h>
+#include <asm/arch-imx/cpu.h>
 #include <asm/arch/imx-regs.h>
 #include <asm/arch/clock.h>
 
diff --git a/arch/arm/include/asm/arch-imx/cpu.h b/arch/arm/include/asm/arch-imx/cpu.h
index 7e681e9..8a75902 100644
--- a/arch/arm/include/asm/arch-imx/cpu.h
+++ b/arch/arm/include/asm/arch-imx/cpu.h
@@ -27,6 +27,18 @@
 #define MXC_SOC_MX6		0x60
 #define MXC_SOC_MX7		0x70
 
+#define CHIP_REV_1_0            0x10
+#define CHIP_REV_1_1            0x11
+#define CHIP_REV_1_2            0x12
+#define CHIP_REV_1_5            0x15
+#define CHIP_REV_2_0            0x20
+#define CHIP_REV_2_5            0x25
+#define CHIP_REV_3_0            0x30
+
+#define BOARD_REV_1_0           0x0
+#define BOARD_REV_2_0           0x1
+#define BOARD_VER_OFFSET        0x8
+
 #define CS0_128					0
 #define CS0_64M_CS1_64M				1
 #define CS0_64M_CS1_32M_CS2_32M			2
diff --git a/arch/arm/include/asm/arch-mx25/imx-regs.h b/arch/arm/include/asm/arch-mx25/imx-regs.h
index 78c4e9b..1b00ed7 100644
--- a/arch/arm/include/asm/arch-mx25/imx-regs.h
+++ b/arch/arm/include/asm/arch-mx25/imx-regs.h
@@ -526,8 +526,4 @@ struct cspi_regs {
 	IMX_CSPI2_BASE, \
 	IMX_CSPI3_BASE
 
-#define CHIP_REV_1_0		0x10
-#define CHIP_REV_1_1		0x11
-#define CHIP_REV_1_2		0x12
-
 #endif				/* _IMX_REGS_H */
diff --git a/arch/arm/include/asm/arch-mx5/imx-regs.h b/arch/arm/include/asm/arch-mx5/imx-regs.h
index 5f0e1e6..e73cc07 100644
--- a/arch/arm/include/asm/arch-mx5/imx-regs.h
+++ b/arch/arm/include/asm/arch-mx5/imx-regs.h
@@ -291,17 +291,6 @@
 #define DP_MFD_216	(4 - 1)
 #define DP_MFN_216	3
 
-#define CHIP_REV_1_0            0x10
-#define CHIP_REV_1_1            0x11
-#define CHIP_REV_2_0            0x20
-#define CHIP_REV_2_5		0x25
-#define CHIP_REV_3_0            0x30
-
-#define BOARD_REV_1_0           0x0
-#define BOARD_REV_2_0           0x1
-
-#define BOARD_VER_OFFSET	0x8
-
 #define IMX_IIM_BASE            (IIM_BASE_ADDR)
 
 #if !(defined(__KERNEL_STRICT_NAMES) || defined(__ASSEMBLY__))
diff --git a/arch/arm/include/asm/arch-mx6/imx-regs.h b/arch/arm/include/asm/arch-mx6/imx-regs.h
index 74512ac..6249b33 100644
--- a/arch/arm/include/asm/arch-mx6/imx-regs.h
+++ b/arch/arm/include/asm/arch-mx6/imx-regs.h
@@ -325,10 +325,6 @@
 #define WDOG3_BASE_ADDR (is_cpu_type(MXC_CPU_MX6UL) ?	\
 			 MX6UL_WDOG3_BASE_ADDR :  MX6SX_WDOG3_BASE_ADDR)
 
-#define CHIP_REV_1_0                 0x10
-#define CHIP_REV_1_2                 0x12
-#define CHIP_REV_1_5                 0x15
-#define CHIP_REV_2_0                 0x20
 #if !(defined(CONFIG_MX6SX) || defined(CONFIG_MX6UL))
 #define IRAM_SIZE                    0x00040000
 #else
-- 
2.1.4

[U-Boot] [PATCH v2 2/5] imx: hab: rework support for imx6/imx7

[Adrian Alonso]

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

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
-- 
2.1.4

[U-Boot] [PATCH v2 3/5] imx: hab use unified rom section for mx6sx and mx6ul

[Adrian Alonso]

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

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
-- 
2.1.4

[U-Boot] [PATCH v2 4/5] imx: hab add mx7 secure boot support

[Adrian Alonso]

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
+	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
+
+	if (is_soc_type(MXC_SOC_MX7))
+		return (reg & 0x2000000) == 0x2000000;
+	else if (is_soc_type(MXC_SOC_MX6))
+		return (reg & 0x2) == 0x2;
 
-	return (reg & 0x2) == 0x2;
+	return 0;
 }
 
 static inline uint8_t get_idx(uint8_t *list, uint8_t tgt)
@@ -414,7 +427,7 @@ uint32_t authenticate_image(uint32_t ddr_start, uint32_t image_size)
 			 * crash.
 			 */
 			/* Check MMU enabled */
-			if (get_cr() & CR_M) {
+			if (is_soc_type(MXC_SOC_MX6) && get_cr() & CR_M) {
 				if (is_cpu_type(MXC_CPU_MX6Q) ||
 				    is_cpu_type(MXC_CPU_MX6D)) {
 					/*
-- 
2.1.4

[U-Boot] [PATCH v2 5/5] mx7dsabresd: add secure boot support

[Adrian Alonso]

Add secure boot support for mx7dsabresd target board

Signed-off-by: Adrian Alonso <aalonso@freescale.com>
---
Changes for V2: Resend

 board/freescale/mx7dsabresd/imximage.cfg | 7 +++++++
 include/configs/mx7dsabresd.h            | 4 ++++
 2 files changed, 11 insertions(+)

diff --git a/board/freescale/mx7dsabresd/imximage.cfg b/board/freescale/mx7dsabresd/imximage.cfg
index 91b70ee..76574ff 100644
--- a/board/freescale/mx7dsabresd/imximage.cfg
+++ b/board/freescale/mx7dsabresd/imximage.cfg
@@ -23,6 +23,13 @@ IMAGE_VERSION 2
 BOOT_FROM	sd
 
 /*
+ * Secure boot support
+ */
+#ifdef CONFIG_SECURE_BOOT
+CSF CONFIG_CSF_SIZE
+#endif
+
+/*
  * Device Configuration Data (DCD)
  *
  * Each entry must have the format:
diff --git a/include/configs/mx7dsabresd.h b/include/configs/mx7dsabresd.h
index ada5de8..910ceaa 100644
--- a/include/configs/mx7dsabresd.h
+++ b/include/configs/mx7dsabresd.h
@@ -14,6 +14,10 @@
 #define CONFIG_DBG_MONITOR
 #define PHYS_SDRAM_SIZE			SZ_1G
 
+/* Uncomment to enable secure boot support */
+/* #define CONFIG_SECURE_BOOT */
+#define CONFIG_CSF_SIZE			0x4000
+
 /* Network */
 #define CONFIG_CMD_MII
 #define CONFIG_FEC_MXC
-- 
2.1.4

[U-Boot] [PATCH v2 3/5] imx: hab use unified rom section for mx6sx and mx6ul

[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


-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH v2 2/5] imx: hab: rework support for imx6/imx7

[Stefano Babic]

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

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH v2 4/5] imx: hab add mx7 secure boot support

[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



-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH v2 4/5] imx: hab add mx7 secure boot support

[Alonso Adrian]

Hi Stefano,

> -----Original Message-----
> From: Stefano Babic [mailto:sbabic at denx.de]
> Sent: Friday, October 09, 2015 3:53 AM
> To: Alonso Lazcano Adrian-B38018 <aalonso@freescale.com>; u-
> boot at lists.denx.de; sbabic at denx.de
> Cc: otavio at ossystems.com.br; Estevam Fabio-R49496
> <Fabio.Estevam@freescale.com>; Li Frank-B20596 <Frank.Li@freescale.com>;
> Garg Nitin-B37173 <nitin.garg@freescale.com>
> Subject: Re: [PATCH v2 4/5] imx: hab add mx7 secure boot support
> 
> 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.

[Adrian] Please take a look at the new patch series for hab rework it address all
Your observations.

Thanks a lot :)
> 
> Best regards,
> Stefano Babic
> 
> 
> 
> --
> ============================================================
> =========
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
> ============================================================
> =========