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....@nxp.com>; u-boot@lists.denx.de; >> albert.u.b...@aribaud.net; hdego...@redhat.com; w...@csie.org; Hongbo >> Zhang <hongbo.zh...@nxp.com>; b.galv...@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....@nxp.com>; u-boot@lists.denx.de; >>>> albert.u.b...@aribaud.net; hdego...@redhat.com; w...@csie.org; >> Hongbo >>>> Zhang <hongbo.zh...@nxp.com>; b.galv...@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....@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....@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@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot