[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 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.

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