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

Tue Aug 2 05:29:10 CEST 2016

Hi York,

Thanks for your comments!

> -----Original Message-----
> From: york sun
> Sent: 2016年8月2日 0:10
> 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/31/2016 08:20 PM, Zhiqiang Hou wrote:
> > 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?
> 
> Zhiqiang,
> 
> When you send a new version, reviewers are expecting updated version
> number and a change log. This helps us tracking what has been changed.
> All patches in the same set should be updated. If a patch has not change, a
> simple "no change" helps. If any dependency has changed/updated, please
> update the note as well.
> 

I know what do you mean, this 3 patch was sent to u-boot at lists.denx.de the first time, please forget the versions for internal review. So I am confusing if you mean this 3 patches should be merged into the patchset '[PATCHv7 1/6] armv8: fsl-layerscape: add i/d-cache enable function to enable_caches' and then resubmit all the patches? I don't know if that is legal, because the state of that patchset has been applied to u-boot-fsl-qoriq master and awaiting upstream. 

> >
> >>>  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?
> 
> I just guessed without "arm,psci" node, kernel wouldn't look for "cpu_on",
> "cpu_off" node. I could be wrong.

As it is a collective, so we'd better keep the sequence as previous.

Thanks,
Zhiqiang