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

[york sun <york.sun@nxp.com>]

On 08/01/2016 08:29 PM, Zhiqiang Hou wrote:
> Hi York,
>
> Thanks for your comments!
>
>> -----Original Message-----
>> From: york sun
>> Sent: 2016?8?2? 0:10
>> To: Zhiqiang Hou <zhiqiang.hou@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@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@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@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@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@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.
>

I must got confused with your internal version numbers. Let it go.
Regarding your other patch set start with '[PATCHv7 1/6] armv8: 
fsl-layerscape: add i/d-cache enable function to enable_caches', they 
are already merged. No need to update.


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

OK.

York