From mboxrd@z Thu Jan 1 00:00:00 1970 From: york sun Date: Mon, 1 Aug 2016 16:10:23 +0000 Subject: [U-Boot] [PATCH 2/3] arm/PSCI: Fixed the backward compatiblity issue References: <1469787997-38462-1-git-send-email-Zhiqiang.Hou@nxp.com> <1469787997-38462-2-git-send-email-Zhiqiang.Hou@nxp.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 ; u-boot at lists.denx.de; >> albert.u.boot at aribaud.net; hdegoede at redhat.com; wens at csie.org; Hongbo >> Zhang ; 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 >>> >>> 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 >>> --- >> >> 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