[U-Boot] [PATCH] Support i.MX6 High Assurance Boot (HAB) authentication of images

[Wolfgang Denk <wd@denx.de>]

Dear nitin.garg at freescale.com,

In message <1409516179-32553-2-git-send-email-nitin.garg@freescale.com> you wrote:
>
> +	reg = readl(&ccm_regs->CCGR0);
> +	reg |= (MXC_CCM_CCGR0_CAAM_WRAPPER_IPG_MASK |
> +			MXC_CCM_CCGR0_CAAM_WRAPPER_ACLK_MASK |
> +			MXC_CCM_CCGR0_CAAM_SECURE_MEM_MASK);
> +	writel(reg, &ccm_regs->CCGR0);

Is there any specific reason for not using setbits_le32() instead?

> +	/* Enable EMI slow clk */
> +	reg = readl(&ccm_regs->CCGR6);
> +	reg |= MXC_CCM_CCGR6_EMI_SLOW_MASK;
> +	writel(reg, &ccm_regs->CCGR6);

Ditto?

> +	reg = readl(&ccm_regs->CCGR0);
> +	reg &= ~(MXC_CCM_CCGR0_CAAM_WRAPPER_IPG_MASK |
> +			MXC_CCM_CCGR0_CAAM_WRAPPER_ACLK_MASK |
> +			MXC_CCM_CCGR0_CAAM_SECURE_MEM_MASK);
> +	writel(reg, &ccm_regs->CCGR0);

Why not using clrbits_le32() instead?

> +	/* Disable EMI slow clk */
> +	reg = readl(&ccm_regs->CCGR6);
> +	reg &= ~MXC_CCM_CCGR6_EMI_SLOW_MASK;
> +	writel(reg, &ccm_regs->CCGR6);

Ditto?

> +/* enable HAB (High Assurance Boot) debug */
> +/* #define DEBUG_AUTHENTICATE_IMAGE */
> +

Please do not add dead code.


> +#ifdef DEBUG_AUTHENTICATE_IMAGE
> +void dump_mem(uint32_t addr, int size)
> +{
> +	int i;
> +
> +	for (i = 0; i < size; i += 4) {
> +		if (i != 0) {
> +			if (i % 16 == 0)
> +				printf("\n");
> +			else
> +				printf(" ");
> +		}
> +
> +		printf("0x%08x", *(uint32_t *)addr);
> +		addr += 4;
> +	}
> +
> +	printf("\n");
> +
> +	return;
> +}
> +#endif

Is this really needed?  Do we not already have such code, like
print_buffer() ?


> @@ -60,4 +62,8 @@ int enable_i2c_clk(unsigned char enable, unsigned i2c_num);
>  int enable_spi_clk(unsigned char enable, unsigned spi_num);
>  void enable_ipu_clock(void);
>  int enable_fec_anatop_clock(enum enet_freq freq);
> +#ifdef CONFIG_SECURE_BOOT
> +void hab_caam_clock_enable(void);
> +void hab_caam_clock_disable(void);
> +#endif

You can drop the #ifdef for prototypes.

> --- a/arch/arm/include/asm/arch-mx6/sys_proto.h
> +++ b/arch/arm/include/asm/arch-mx6/sys_proto.h
> @@ -2,6 +2,8 @@
>   * (C) Copyright 2009
>   * Stefano Babic, DENX Software Engineering, sbabic at denx.de.
>   *
> + * (C) Copyright 2014 Freescale Semiconductor, Inc.
> + *
>   * SPDX-License-Identifier:	GPL-2.0+
>   */
>  
> @@ -11,7 +13,7 @@
>  #include <asm/imx-common/regs-common.h>
>  #include "../arch-imx/cpu.h"
>  
> -#define soc_rev() (get_cpu_rev() & 0xFF)
> +#define soc_rev() ((int)(get_cpu_rev() & 0xFF))

Claiming copyright for adding a single cast is a bit excessive, isn't
it?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Your csh still thinks true is false. Write to your vendor  today  and
tell them that next year Configure ought to "rm /bin/csh" unless they
fix  their blasted shell. :-)
                 - Larry Wall in Configure from the perl distribution