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 [off-list ref] wrote:

quoted

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 <redacted>
Cc: Sudeep Holla <redacted>
Cc: Soby Mathew <redacted>
Signed-off-by: Leo Yan <redacted>
---
 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