| Message ID | 1443648649-24599-4-git-send-email-aalonso@freescale.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
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
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 > ============================================================ > =========
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)) { /*
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(-)