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

Dear nitin.garg@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@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