[U-Boot] [PATCHv4 3/7] ARMv8/layerscape: Add FSL PPA support

[Zhiqiang Hou]

Hi York,

Thanks for your comments and suggestion!

> -----Original Message-----
> From: York Sun [mailto:york.sun at nxp.com]
> Sent: 2016年5月28日 2:06
> To: Zhiqiang Hou <zhiqiang.hou at nxp.com>; u-boot at lists.denx.de;
> albert.u.boot at aribaud.net; scottwood at freescale.com;
> Mingkai.hu at freescale.com; yorksun at freescale.com; leoli at freescale.com;
> prabhakar at freescale.com; bhupesh.sharma at freescale.com
> Cc: Hou Zhiqiang <Zhiqiag.Hou at nxp.com>
> Subject: Re: [PATCHv4 3/7] ARMv8/layerscape: Add FSL PPA support
> 
> On 05/23/2016 04:48 AM, Zhiqiang Hou wrote:
> > From: Hou Zhiqiang <Zhiqiang.Hou at nxp.com>
> >
> > The FSL Primary Protected Application (PPA) is a software component
> > loaded during boot which runs in TrustZone and remains resident after
> > boot.
> >
> > Signed-off-by: Hou Zhiqiang <Zhiqiag.Hou at nxp.com>
> > ---
> > V4:
> >  - Moved secure firmware validation API to this patch.
> >  - Moved secure firmware getting supported PSCI version API to this patch.
> >
> > V3:
> >  - Refactor the code.
> >  - Add PPA firmware version info output.
> >
> <snip>
> 
> > +int sec_firmware_validate(void)
> > +{
> > +	void *ppa_addr;
> > +
> > +#ifdef CONFIG_SYS_LS_PPA_FW_IN_NOR
> > +	ppa_addr = (void *)CONFIG_SYS_LS_PPA_FW_ADDR; #else #error "No
> > +CONFIG_SYS_LS_PPA_FW_IN_xxx defined"
> > +#endif
> > +
> > +	return ppa_firmware_validate(ppa_addr); }
> 
> You are returning 0 when the image is valid. This function name is confusing.
> How about declare it as bool, and change the name to is_sec_firmware_valid()?

Yes, will correct the confusing name next version.

> > +
> > +#ifdef CONFIG_ARMV8_PSCI
> > +unsigned int sec_firmware_support_psci_version(void)
> > +{
> > +	unsigned int psci_ver = 0;
> > +
> > +	if (!sec_firmware_validate())
> > +		psci_ver = ppa_support_psci_version();
> > +
> > +	return psci_ver;
> > +}
> > +#endif
> > +
> > +int ppa_init_pre(u64 *entry)
> > +{
> > +	u64 ppa_ram_addr;
> > +	const void *raw_image_addr;
> > +	size_t raw_image_size = 0;
> > +	size_t ppa_ram_size = ppa_get_dram_block_size();
> > +	int ret;
> > +
> > +	debug("fsl-ppa: ppa size(0x%lx)\n", ppa_ram_size);
> > +
> > +	/*
> > +	 * The PPA must be stored in secure memory.
> > +	 * Append PPA to secure mmu table.
> > +	 */
> > +	ppa_ram_addr = (gd->secure_ram &
> MEM_RESERVE_SECURE_ADDR_MASK) +
> > +			gd->arch.tlb_size;
> > +
> 
> Should we check "if (gd->secure_ram & MEM_RESERVE_SECURE_SECURED)"
> before using the secure memory?

Yes, will add the check next version.

Thanks,
Zhiqiang