[U-Boot] [PATCH 2/3] arm/PSCI: Fixed the backward compatiblity issue

[Zhiqiang Hou]

Hi York,

Thanks for your comments!

> -----Original Message-----
> From: york sun
> Sent: 2016年7月29日 23:37
> To: Zhiqiang Hou <zhiqiang.hou at nxp.com>; u-boot at lists.denx.de;
> albert.u.boot at aribaud.net; hdegoede at redhat.com; wens at csie.org; Hongbo
> Zhang <hongbo.zhang at nxp.com>; b.galvani at gmail.com
> Subject: Re: [PATCH 2/3] arm/PSCI: Fixed the backward compatiblity issue
> 
> On 07/29/2016 03:37 AM, Zhiqiang Hou wrote:
> > From: Hou Zhiqiang <Zhiqiang.Hou at nxp.com>
> >
> > Appended the compatible strings of old version PSCI to the latest
> > version supported. And there are some psci functions' property must be
> > added to DT only for psci version 0.1, such as 'cpu_on' 'cpu_off' etc.
> >
> > Note:
> > The PSCI version 0.1 isn't supported by ARMv8 Secure Firmware Framework.
> >
> > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou at nxp.com>
> > ---
> 
> You missed version number and change log.

Should the previous Secure Firmware Framework patchset need to be resubmitted? And the version number follow the that patchset?

> >  arch/arm/include/asm/psci.h |  3 +++
> >  arch/arm/lib/psci-dt.c      | 61
> ++++++++++++++++++++++++++-------------------
> >  2 files changed, 38 insertions(+), 26 deletions(-)
> >
> > diff --git a/arch/arm/include/asm/psci.h b/arch/arm/include/asm/psci.h
> > index 8aefaa7..5b8ce4d 100644
> > --- a/arch/arm/include/asm/psci.h
> > +++ b/arch/arm/include/asm/psci.h
> > @@ -18,6 +18,9 @@
> >  #ifndef __ARM_PSCI_H__
> >  #define __ARM_PSCI_H__
> >
> > +#define ARM_PSCI_VER_1_0		(0x00010000)
> > +#define ARM_PSCI_VER_0_2		(0x00000002)
> > +
> >  /* PSCI 0.1 interface */
> >  #define ARM_PSCI_FN_BASE		0x95c1ba5e
> >  #define ARM_PSCI_FN(n)			(ARM_PSCI_FN_BASE + (n))
> > diff --git a/arch/arm/lib/psci-dt.c b/arch/arm/lib/psci-dt.c index
> > bcd92e7..af49c24 100644
> > --- a/arch/arm/lib/psci-dt.c
> > +++ b/arch/arm/lib/psci-dt.c
> > @@ -19,7 +19,6 @@ int fdt_psci(void *fdt)  #if
> > defined(CONFIG_ARMV8_PSCI) || defined(CONFIG_ARMV7_PSCI)
> >  	int nodeoff;
> >  	unsigned int psci_ver = 0;
> > -	char *psci_compt;
> >  	int tmp;
> >
> >  	nodeoff = fdt_path_offset(fdt, "/cpus"); @@ -68,39 +67,49 @@
> > init_psci_node:
> >  	psci_ver = sec_firmware_support_psci_version();
> >  #endif
> >  	switch (psci_ver) {
> > -	case 0x00010000:
> > -		psci_compt = "arm,psci-1.0";
> > -		break;
> > -	case 0x00000002:
> > -		psci_compt = "arm,psci-0.2";
> > -		break;
> > +	case ARM_PSCI_VER_1_0:
> > +		tmp = fdt_setprop_string(fdt, nodeoff,
> > +				"compatible", "arm,psci-1.0");
> > +		if (tmp)
> > +			return tmp;
> 
> Add a comment "fall through".
> 

Yes, will add the comment.
[] 
> > +	case ARM_PSCI_VER_0_2:
> > +		tmp = fdt_appendprop_string(fdt, nodeoff,
> > +				"compatible", "arm,psci-0.2");
> > +		if (tmp)
> > +			return tmp;
> 
> Add a comment "fall through".
> 
> >  	default:
> > -		psci_compt = "arm,psci";
> > +	/*
> > +	 * The Secure firmware framework isn't able to support PSCI version 0.1.
> > +	 */
> > +#ifndef CONFIG_ARMV8_SEC_FIRMWARE_SUPPORT
> > +		tmp = fdt_appendprop_string(fdt, nodeoff,
> > +				"compatible", "arm,psci");
> > +		if (tmp)
> > +			return tmp;
> > +		tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_suspend",
> > +				ARM_PSCI_FN_CPU_SUSPEND);
> > +		if (tmp)
> > +			return tmp;
> > +		tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_off",
> > +				ARM_PSCI_FN_CPU_OFF);
> > +		if (tmp)
> > +			return tmp;
> > +		tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_on",
> > +				ARM_PSCI_FN_CPU_ON);
> > +		if (tmp)
> > +			return tmp;
> > +		tmp = fdt_setprop_u32(fdt, nodeoff, "migrate",
> > +				ARM_PSCI_FN_MIGRATE);
> > +		if (tmp)
> > +			return tmp;
> 
> This may not be a real concern, but I am curious what would happen if one of
> the above fdt_setprop_u32 fails? Would it be better to set "arm,psci" last?

Trend to add the error check to prevent the unexpected issue. Is there benefit to set the "arm,psci" last?

Thanks,
Zhiqiang