Re: [PATCH v2] arm64: dts: Hi3660: Fix up psci state id

[Leo Yan <leo.yan@linaro.org>]

Hi Vincent,

[ + John, Kevin Wang ]

On Fri, Dec 22, 2017 at 03:22:51PM +0100, Vincent Guittot wrote:
> Hi Leo,
> 
> Sorry for jumping late in the discussion but should  we also remove
> the NAP state from the property cpu-idle-states of the CPUs because
> this state not supported by the platform at least for now and may be
> not in a near future ?

Thanks for bringing up this.

I don't want to hide anything for patch discussion :) this patch is to
resolve the PSCI parameter mismatching issue between kernel and ARM-TF
and it's not used to resolve the bug for CPU_NAP, so I didn't mention
the CPU_NAP malfunction issue to avoid complex discussion context.

I want to keep CPU_NAP state and track bug for CPU_NAP fixing; if we
remove this state, I suspect we might have no chance to enable it
anymore. Finally this is up to Hisilicon colleague decision and if they
have time to fix this.

I will offline to check with Daniel and Kevin for this; and if we
finally decide to remove it we can commit extra patch for this later,
how about you think?

> Then, I have another question regarding the update of the
> psci-suspend-parameter. These changes implies an update of the psci
> firmawre which means that we will now have 2 different firmware
> version compatible with 2 different dt.
> 
> Is there any way to check that the ATF on the board is the one that
> compatible with the parameter with something like a version ? I
> currently use the previous firmware which works fine with current
> kernel and dt binding once the NAP state is removed from the table.
> When moving on recent kernel, I will have to take care of updating the
> firmware and if i need to go back on a previous kernel, i will have to
> make sure that i have the right ATF version. This make a lot of chance
> of having the wrong configuration

AFAIK, we cannot distinguish the PSCI parameter by PSCI version or
ARM-TF version number; alternatively one simple way for checking ARM-TF
is we can get commit ID (e.g. 83df7ce) from the ARM-TF log; so any
ARM-TF commit ID is newer than the patch fdae60b6ba27: "Hikey960:
Change to use recommended power state id format" should apply this
kernel patch.

NOTICE:  BL1: Booting BL31
NOTICE:  BL31: v1.4(debug):v1.4-441-g83df7ce-dirty
NOTICE:  BL31: Built : 17:31:35, Dec 22 2017

BTW, I hope we can upgrade Linux kernel and ARM-TF to latest code base
to avoid compatible issue; for Android offical releasing it uses the
old PSCI parameters with Hisilicon legacy booting images, so they can
work well, but if someone uses ARM-TF mainline code + Android kernel
4.4/4.9, there must have compatible issue.

I am monitoring the integration ARM-TF/UEFI into Android on Hikey960,
we need backport this patch onto Android kernel 4.4/4.9 ASAP after
integration ARM-TF/UEFI.

Thanks,
Leo Yan

> Regards,
> Vincent
> 
> On 12 December 2017 at 10:12, Leo Yan <leo.yan@linaro.org> wrote:
> > Thanks a lot for Vincent Guittot careful work to find bug for 'CPU_NAP'
> > idle state.  From ftrace log we can observe CA73 CPUs can be easily
> > waken up from 'CPU_NAP' state but the 'waken up' CPUs doesn't handle
> > anything and sleep again; so there have tons of trace events for CA73
> > CPUs entering and exiting idle state.
> >
> > On Hi3660 CA73 has retention state 'CPU_NAP' for CPU idle, this state we
> > set its psci parameter as '0x0000001' and from this parameter it can
> > calculate state id is 1.  Unfortunately ARM trusted firmware (ARM-TF)
> > takes 1 as a invalid value for state id, so the CPU cannot enter idle
> > state and directly bail out to kernel.
> >
> > We want to create good practice for psci parameters platform definition,
> > so review the psci specification. The spec "ARM Power State Coordination
> > Interface - Platform Design Document (ARM DEN 0022D)" recommends state
> > ID in chapter "6.5 Recommended StateID Encoding".  The recommended power
> > state IDs can be presented by below listed values; and it divides into
> > three fields, every field can use 4 bits to present power states
> > corresponding to core level, cluster level and system level:
> >   0: Run
> >   1: Standby
> >   2: Retention
> >   3: Powerdown
> >
> > This commit changes psci parameter to compliance with the suggested
> > state ID in the doc.  Except we change 'CPU_NAP' state psci parameter
> > to '0x0000002', this commit also changes 'CPU_SLEEP' and 'CLUSTER_SLEEP'
> > state parameters to '0x0010003' and '0x1010033' respectively.
> >
> > Credits to Daniel, Sudeep and Soby for suggestion and consolidation.
> >
> > Cc: Vincent Guittot <vincent.guittot@linaro.org>
> > Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> > Cc: Sudeep Holla <sudeep.holla@arm.com>
> > Cc: Soby Mathew <Soby.Mathew@arm.com>
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> >  arch/arm64/boot/dts/hisilicon/hi3660.dtsi | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
> > index ab0b95b..99d5a46 100644
> > --- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
> > +++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
> > @@ -147,7 +147,7 @@
> >
> >                         CPU_NAP: cpu-nap {
> >                                 compatible = "arm,idle-state";
> > -                               arm,psci-suspend-param = <0x0000001>;
> > +                               arm,psci-suspend-param = <0x0000002>;
> >                                 entry-latency-us = <7>;
> >                                 exit-latency-us = <2>;
> >                                 min-residency-us = <15>;
> > @@ -156,7 +156,7 @@
> >                         CPU_SLEEP: cpu-sleep {
> >                                 compatible = "arm,idle-state";
> >                                 local-timer-stop;
> > -                               arm,psci-suspend-param = <0x0010000>;
> > +                               arm,psci-suspend-param = <0x0010003>;
> >                                 entry-latency-us = <40>;
> >                                 exit-latency-us = <70>;
> >                                 min-residency-us = <3000>;
> > @@ -165,7 +165,7 @@
> >                         CLUSTER_SLEEP_0: cluster-sleep-0 {
> >                                 compatible = "arm,idle-state";
> >                                 local-timer-stop;
> > -                               arm,psci-suspend-param = <0x1010000>;
> > +                               arm,psci-suspend-param = <0x1010033>;
> >                                 entry-latency-us = <500>;
> >                                 exit-latency-us = <5000>;
> >                                 min-residency-us = <20000>;
> > @@ -174,7 +174,7 @@
> >                         CLUSTER_SLEEP_1: cluster-sleep-1 {
> >                                 compatible = "arm,idle-state";
> >                                 local-timer-stop;
> > -                               arm,psci-suspend-param = <0x1010000>;
> > +                               arm,psci-suspend-param = <0x1010033>;
> >                                 entry-latency-us = <1000>;
> >                                 exit-latency-us = <5000>;
> >                                 min-residency-us = <20000>;
> > --
> > 2.7.4
> >