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.
> >
> >>> 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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
