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

[york sun]

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.

>
>>>  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.

York