[U-Boot] [PATCH v2 1/3] SECURE_BOOT: Enable chain of trust on LS1046A platform

[Sumit Garg]

> -----Original Message-----
> From: york sun
> Sent: Monday, November 14, 2016 11:22 PM
> To: Sumit Garg <sumit.garg at nxp.com>; u-boot at lists.denx.de
> Cc: Ruchika Gupta <ruchika.gupta at nxp.com>; Prabhakar Kushwaha
> <prabhakar.kushwaha at nxp.com>; Vini Pillai <vinitha.pillai at nxp.com>
> Subject: Re: [PATCH v2 1/3] SECURE_BOOT: Enable chain of trust on LS1046A
> platform
> 
> On 10/26/2016 03:47 AM, Sumit Garg wrote:
> > Define bootscript and its header addresses for QSPI target. Also
> > define PPA header address to enable PPA validation.
> >
> > Signed-off-by: Vinitha Pillai <vinitha.pillai at nxp.com>
> > Signed-off-by: Sumit Garg <sumit.garg at nxp.com>
> > ---
> >
> > Changes in v2:
> > Split patches logically from 2 to 3.
> >
> >  arch/arm/include/asm/arch-fsl-layerscape/config.h |  2 +-
> >  arch/arm/include/asm/fsl_secure_boot.h            | 37 ++++++++++++++++----
> ---
> >  2 files changed, 27 insertions(+), 12 deletions(-)
> >
> > diff --git a/arch/arm/include/asm/arch-fsl-layerscape/config.h
> > b/arch/arm/include/asm/arch-fsl-layerscape/config.h
> > index 4201e0f..11a62e8 100644
> > --- a/arch/arm/include/asm/arch-fsl-layerscape/config.h
> > +++ b/arch/arm/include/asm/arch-fsl-layerscape/config.h
> > @@ -196,7 +196,7 @@
> >
> >  #define CONFIG_SYS_FSL_IFC_BE
> >  #define CONFIG_SYS_FSL_SFP_VER_3_2
> > -#define CONFIG_SYS_FSL_SNVS_LE
> > +#define CONFIG_SYS_FSL_SEC_MON_BE
> >  #define CONFIG_SYS_FSL_SFP_BE
> >  #define CONFIG_SYS_FSL_SRK_LE
> >  #define CONFIG_KEY_REVOCATION
> > diff --git a/arch/arm/include/asm/fsl_secure_boot.h
> > b/arch/arm/include/asm/fsl_secure_boot.h
> > index 4525287..933e09c 100644
> > --- a/arch/arm/include/asm/fsl_secure_boot.h
> > +++ b/arch/arm/include/asm/fsl_secure_boot.h
> > @@ -45,7 +45,8 @@
> >  #define CONFIG_CMD_HASH
> >  #define CONFIG_KEY_REVOCATION
> >  #ifndef CONFIG_SYS_RAMBOOT
> > -/* The key used for verification of next level images
> > +/*
> > + * The key used for verification of next level images
> >   * is picked up from an Extension Table which has
> >   * been verified by the ISBC (Internal Secure boot Code)
> >   * in boot ROM of the SoC.
> > @@ -59,9 +60,10 @@
> >
> >  #endif
> >
> > -#if defined(CONFIG_LS1043A) || defined(CONFIG_LS2080A)
> > -/* For LS1043 (ARMv8), ESBC image Address in Header is 64 bit
> > - * Similiarly for LS2080
> > +#if defined(CONFIG_FSL_LAYERSCAPE)
> > +/*
> > + * For fsl layerscape based platforms, ESBC image Address in Header
> > + * is 64 bit.
> >   */
> >  #define CONFIG_ESBC_ADDR_64BIT
> >  #endif
> > @@ -78,13 +80,16 @@
> >  	"setenv hwconfig \'fsl_ddr:ctlr_intlv=null,bank_intlv=null\';"
> >  #endif
> >
> > -/* Copying Bootscript and Header to DDR from NOR for LS2 and for
> > rest, from
> > - * Non-XIP Memory (Nand/SD)*/
> > +/*
> > + * Copying Bootscript and Header to DDR from NOR for LS2 and for
> > +rest, from
> > + * Non-XIP Memory (Nand/SD)
> > + */
> >  #if defined(CONFIG_SYS_RAMBOOT) || defined(CONFIG_LS2080A) || \
> >  	defined(CONFIG_SD_BOOT)
> >  #define CONFIG_BOOTSCRIPT_COPY_RAM
> >  #endif
> > -/* The address needs to be modified according to NOR, NAND, SD and
> > +/*
> > + * The address needs to be modified according to NOR, NAND, SD and
> >   * DDR memory map
> >   */
> >  #ifdef CONFIG_LS2080A
> > @@ -96,19 +101,26 @@
> >  #define CONFIG_BS_SIZE			0x00001000
> >  #else
> >  #ifdef CONFIG_SD_BOOT
> > -/* For SD boot address and size are assigned in terms of sector
> > +/*
> > + * For SD boot address and size are assigned in terms of sector
> >   * offset and no. of sectors respectively.
> >   */
> >  #define CONFIG_BS_HDR_ADDR_DEVICE	0x00000800
> >  #define CONFIG_BS_ADDR_DEVICE		0x00000840
> >  #define CONFIG_BS_HDR_SIZE		0x00000010
> >  #define CONFIG_BS_SIZE			0x00000008
> > -#else
> > +/* ifdef CONFIG_SD_BOOT */
> 
> This comment confuses me. The code below is for QSPI_BOOT obviously.
 
I have put this comment to mark ending of SD_BOOT block as starting is marked by #ifdef CONFIG_SD_BOOT.

> 
> > +#elif defined(CONFIG_QSPI_BOOT)
> > +#define CONFIG_BS_HDR_ADDR_DEVICE	0x40780000
> > +#define CONFIG_BS_ADDR_DEVICE		0x40800000
> > +#define CONFIG_BS_HDR_SIZE		0x00002000
> > +#define CONFIG_BS_SIZE			0x00001000
> > +#else /* elif defined(CONFIG_QSPI_BOOT) */
> 
> The code below is not for QSPI_BOOT. Confusing comment.

 I have put this comment to mark ending of QSPI_BOOT block as starting is marked by "#elif defined(CONFIG_QSPI_BOOT)".

> 
> >  #define CONFIG_BS_HDR_ADDR_DEVICE	0x600a0000
> >  #define CONFIG_BS_ADDR_DEVICE		0x60060000
> >  #define CONFIG_BS_HDR_SIZE		0x00002000
> >  #define CONFIG_BS_SIZE			0x00001000
> > -#endif /* #ifdef CONFIG_SD_BOOT */
> > +#endif /* Default NOR Boot */
> 
> I guess the above is for normal NOR boot. The comment should be moved
> above the block.
 
Here also comment marks ending of NOR boot block. I have just added these comments to avoid confusion.

> 
> >  #define CONFIG_BS_HDR_ADDR_RAM		0x81000000
> >  #define CONFIG_BS_ADDR_RAM		0x81020000
> >  #endif
> > @@ -125,12 +137,15 @@
> >  #ifdef CONFIG_SYS_LS_PPA_FW_IN_XIP
> >  #ifdef CONFIG_LS1043A
> >  #define CONFIG_SYS_LS_PPA_ESBC_ADDR	0x600c0000
> > +#elif defined(CONFIG_LS1046A)
> > +#define CONFIG_SYS_LS_PPA_ESBC_ADDR     0x40740000
> >  #endif
> >  #else
> >  #error "No CONFIG_SYS_LS_PPA_FW_IN_xxx defined"
> >  #endif /* ifdef CONFIG_SYS_LS_PPA_FW_IN_XIP */
> >
> > -/* Define the key hash here if SRK used for signing PPA image is
> > +/*
> > + * Define the key hash here if SRK used for signing PPA image is
> >   * different from SRK hash put in SFP used for U-Boot.
> >   * Example
> >   * #define CONFIG_PPA_KEY_HASH \
> >
> 
> It would be better to separate the cosmetic change from the code change.
> 
> York
 
Sure, I will remove  these changes from this patch.

-Sumit