Hi Stefano,
-----Original Message----- From: Stefano Babic [mailto:sbabic@denx.de] Sent: Friday, October 09, 2015 3:53 AM To: Alonso Lazcano Adrian-B38018 aalonso@freescale.com; u- boot@lists.denx.de; sbabic@denx.de Cc: otavio@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@denx.de ============================================================ =========