[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 at freescale.com>; u-
> boot at lists.denx.de; sbabic at denx.de
> Cc: otavio at ossystems.com.br; Estevam Fabio-R49496
> <Fabio.Estevam at freescale.com>; Li Frank-B20596 <Frank.Li at freescale.com>;
> Garg Nitin-B37173 <nitin.garg at 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 at 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
> ============================================================
> =========