[PATCH] arm64: dts: Hi3660: Fix state id for 'CPU_NAP' state

[PATCH] arm64: dts: Hi3660: Fix state id for 'CPU_NAP' state

[Leo Yan]

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.

This commit changes psci parameter to '0x00000000' for state id = 0;
this id is accepted by ARM trusted firmware and finally CPU can stay
properly in 'CPU_NAP' state.

Cc: Vincent Guittot <vincent.guittot@linaro.org>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 arch/arm64/boot/dts/hisilicon/hi3660.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
index ab0b95b..5666d29 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 = <0x0000000>;
 				entry-latency-us = <7>;
 				exit-latency-us = <2>;
 				min-residency-us = <15>;
-- 
2.7.4

[PATCH] arm64: dts: Hi3660: Fix state id for 'CPU_NAP' state

[Leo Yan]

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.

This commit changes psci parameter to '0x00000000' for state id = 0;
this id is accepted by ARM trusted firmware and finally CPU can stay
properly in 'CPU_NAP' state.

Cc: Vincent Guittot <vincent.guittot@linaro.org>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 arch/arm64/boot/dts/hisilicon/hi3660.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
index ab0b95b..5666d29 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 = <0x0000000>;
 				entry-latency-us = <7>;
 				exit-latency-us = <2>;
 				min-residency-us = <15>;
-- 
2.7.4

Re: [PATCH] arm64: dts: Hi3660: Fix state id for 'CPU_NAP' state

[Sudeep Holla]

Hi Daniel,

Thanks a lot for pointing me to this and having some useful discussion
in private. That helped to dig a bit further on this.

On 23/11/17 05:40, Leo Yan 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.
> 
> This commit changes psci parameter to '0x00000000' for state id = 0;
> this id is accepted by ARM trusted firmware and finally CPU can stay
> properly in 'CPU_NAP' state.
> 

I would like to conditionally NACK this patch. If we can't update the
ARM TF at all then, I will agree with this change reluctantly.

This looks like an artifact of copy paste in ARM TF port for this
platform. If you look as PSCI specification, CPU suspend parameter has
some recommendations and it's good to follow then unless you have strong
reasons not to.

As Daniel pointed to me, this patch is required to satisfy TF
particularly [1]. Now that looks like copy pasted from Juno or FVP port
and if you look deeper, it's clearly under !ARM_RECOM_STATE_ID_ENC [2]
which was not copied IIUC :).

Juno's implementation is legacy as these recommendations were added
later in the specification while Juno is 3 year old platform now.

Though strictly speaking it's not violation of the PSCI specification,
but I would rather get this fixed not before it's too late and copied to
the next generation of platforms. Since the firmware can be easily
upgraded that shouldn't be that difficult.

-- 
Regards,
Sudeep

[1]
https://github.com/ARM-software/arm-trusted-firmware/blob/master/plat/hisilicon/hikey960/hikey960_pm.c#L156
[2]
https://github.com/ARM-software/arm-trusted-firmware/blob/master/plat/arm/common/arm_pm.c#L28

Re: [PATCH] arm64: dts: Hi3660: Fix state id for 'CPU_NAP' state

[Sudeep Holla]

Hi Daniel,

Thanks a lot for pointing me to this and having some useful discussion
in private. That helped to dig a bit further on this.

On 23/11/17 05:40, Leo Yan 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.
> 
> This commit changes psci parameter to '0x00000000' for state id = 0;
> this id is accepted by ARM trusted firmware and finally CPU can stay
> properly in 'CPU_NAP' state.
> 

I would like to conditionally NACK this patch. If we can't update the
ARM TF at all then, I will agree with this change reluctantly.

This looks like an artifact of copy paste in ARM TF port for this
platform. If you look as PSCI specification, CPU suspend parameter has
some recommendations and it's good to follow then unless you have strong
reasons not to.

As Daniel pointed to me, this patch is required to satisfy TF
particularly [1]. Now that looks like copy pasted from Juno or FVP port
and if you look deeper, it's clearly under !ARM_RECOM_STATE_ID_ENC [2]
which was not copied IIUC :).

Juno's implementation is legacy as these recommendations were added
later in the specification while Juno is 3 year old platform now.

Though strictly speaking it's not violation of the PSCI specification,
but I would rather get this fixed not before it's too late and copied to
the next generation of platforms. Since the firmware can be easily
upgraded that shouldn't be that difficult.

-- 
Regards,
Sudeep

[1]
https://github.com/ARM-software/arm-trusted-firmware/blob/master/plat/hisilicon/hikey960/hikey960_pm.c#L156
[2]
https://github.com/ARM-software/arm-trusted-firmware/blob/master/plat/arm/common/arm_pm.c#L28
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] arm64: dts: Hi3660: Fix state id for 'CPU_NAP' state

[Sudeep Holla]

Hi Daniel,

Thanks a lot for pointing me to this and having some useful discussion
in private. That helped to dig a bit further on this.

On 23/11/17 05:40, Leo Yan 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.
> 
> This commit changes psci parameter to '0x00000000' for state id = 0;
> this id is accepted by ARM trusted firmware and finally CPU can stay
> properly in 'CPU_NAP' state.
> 

I would like to conditionally NACK this patch. If we can't update the
ARM TF at all then, I will agree with this change reluctantly.

This looks like an artifact of copy paste in ARM TF port for this
platform. If you look as PSCI specification, CPU suspend parameter has
some recommendations and it's good to follow then unless you have strong
reasons not to.

As Daniel pointed to me, this patch is required to satisfy TF
particularly [1]. Now that looks like copy pasted from Juno or FVP port
and if you look deeper, it's clearly under !ARM_RECOM_STATE_ID_ENC [2]
which was not copied IIUC :).

Juno's implementation is legacy as these recommendations were added
later in the specification while Juno is 3 year old platform now.

Though strictly speaking it's not violation of the PSCI specification,
but I would rather get this fixed not before it's too late and copied to
the next generation of platforms. Since the firmware can be easily
upgraded that shouldn't be that difficult.

-- 
Regards,
Sudeep

[1]
https://github.com/ARM-software/arm-trusted-firmware/blob/master/plat/hisilicon/hikey960/hikey960_pm.c#L156
[2]
https://github.com/ARM-software/arm-trusted-firmware/blob/master/plat/arm/common/arm_pm.c#L28

Re: [PATCH] arm64: dts: Hi3660: Fix state id for 'CPU_NAP' state

[Leo Yan]

Hi Sudeep,

On Thu, Nov 23, 2017 at 02:03:51PM +0000, Sudeep Holla wrote:
> Hi Daniel,
> 
> Thanks a lot for pointing me to this and having some useful discussion
> in private. That helped to dig a bit further on this.
> 
> On 23/11/17 05:40, Leo Yan 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.
> > 
> > This commit changes psci parameter to '0x00000000' for state id = 0;
> > this id is accepted by ARM trusted firmware and finally CPU can stay
> > properly in 'CPU_NAP' state.
> > 
> 
> I would like to conditionally NACK this patch. If we can't update the
> ARM TF at all then, I will agree with this change reluctantly.

Thanks for reviewing. Just like Daniel said, we need to figure out the
right method for this. So suggestions are very welcome!

> This looks like an artifact of copy paste in ARM TF port for this
> platform. If you look as PSCI specification, CPU suspend parameter has
> some recommendations and it's good to follow then unless you have strong
> reasons not to.
> 
> As Daniel pointed to me, this patch is required to satisfy TF
> particularly [1]. Now that looks like copy pasted from Juno or FVP port
> and if you look deeper, it's clearly under !ARM_RECOM_STATE_ID_ENC [2]
> which was not copied IIUC :).

Thanks for sharing pointers. It's shame that the copying is not
correct for Hikey960 :)

Come back to recommended state id, I reviewed Juno board defintion and
I found it's not align with PSCI spec defintion, in ARM-TF Juno code
defines state as below [1]:

#define ARM_LOCAL_STATE_RUN     0
#define ARM_LOCAL_STATE_RET     1
#define ARM_LOCAL_STATE_OFF     2

In PSCI spec chapter "6.5 Recommended StateID Encoding" recommends power
state id as below:

0: Run
1: Standby
2: Retention
3: Powerdown

So could you confirm on Hikey960 we should follow PSCI definition for
state id definition?

> Juno's implementation is legacy as these recommendations were added
> later in the specification while Juno is 3 year old platform now.
> 
> Though strictly speaking it's not violation of the PSCI specification,
> but I would rather get this fixed not before it's too late and copied to
> the next generation of platforms. Since the firmware can be easily
> upgraded that shouldn't be that difficult.

If completely compliant with PSCI recommended state id, we need change
both for ARM-TF and kernel for this. In ARM-TF, I have sent PR [2].

For the kernel patch, we should change state id as below. Please let me
know if you have suggestion for this.

diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
index 12544c3..812437a 100644
--- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
@@ -179,7 +179,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>;
@@ -188,7 +188,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>;
@@ -197,7 +197,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>;
@@ -206,7 +206,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>;


[1] https://github.com/ARM-software/arm-trusted-firmware/blob/master/include/plat/arm/common/arm_def.h#L43
[2] https://github.com/ARM-software/arm-trusted-firmware/pull/1171/commits/13d30c1c33609eb6cffadd50954e4301d2cab909

Thanks,
Leo Yan

Re: [PATCH] arm64: dts: Hi3660: Fix state id for 'CPU_NAP' state

[Leo Yan]

Hi Sudeep,

On Thu, Nov 23, 2017 at 02:03:51PM +0000, Sudeep Holla wrote:
> Hi Daniel,
> 
> Thanks a lot for pointing me to this and having some useful discussion
> in private. That helped to dig a bit further on this.
> 
> On 23/11/17 05:40, Leo Yan 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.
> > 
> > This commit changes psci parameter to '0x00000000' for state id = 0;
> > this id is accepted by ARM trusted firmware and finally CPU can stay
> > properly in 'CPU_NAP' state.
> > 
> 
> I would like to conditionally NACK this patch. If we can't update the
> ARM TF at all then, I will agree with this change reluctantly.

Thanks for reviewing. Just like Daniel said, we need to figure out the
right method for this. So suggestions are very welcome!

> This looks like an artifact of copy paste in ARM TF port for this
> platform. If you look as PSCI specification, CPU suspend parameter has
> some recommendations and it's good to follow then unless you have strong
> reasons not to.
> 
> As Daniel pointed to me, this patch is required to satisfy TF
> particularly [1]. Now that looks like copy pasted from Juno or FVP port
> and if you look deeper, it's clearly under !ARM_RECOM_STATE_ID_ENC [2]
> which was not copied IIUC :).

Thanks for sharing pointers. It's shame that the copying is not
correct for Hikey960 :)

Come back to recommended state id, I reviewed Juno board defintion and
I found it's not align with PSCI spec defintion, in ARM-TF Juno code
defines state as below [1]:

#define ARM_LOCAL_STATE_RUN     0
#define ARM_LOCAL_STATE_RET     1
#define ARM_LOCAL_STATE_OFF     2

In PSCI spec chapter "6.5 Recommended StateID Encoding" recommends power
state id as below:

0: Run
1: Standby
2: Retention
3: Powerdown

So could you confirm on Hikey960 we should follow PSCI definition for
state id definition?

> Juno's implementation is legacy as these recommendations were added
> later in the specification while Juno is 3 year old platform now.
> 
> Though strictly speaking it's not violation of the PSCI specification,
> but I would rather get this fixed not before it's too late and copied to
> the next generation of platforms. Since the firmware can be easily
> upgraded that shouldn't be that difficult.

If completely compliant with PSCI recommended state id, we need change
both for ARM-TF and kernel for this. In ARM-TF, I have sent PR [2].

For the kernel patch, we should change state id as below. Please let me
know if you have suggestion for this.

diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
index 12544c3..812437a 100644
--- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
@@ -179,7 +179,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>;
@@ -188,7 +188,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>;
@@ -197,7 +197,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>;
@@ -206,7 +206,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>;


[1] https://github.com/ARM-software/arm-trusted-firmware/blob/master/include/plat/arm/common/arm_def.h#L43
[2] https://github.com/ARM-software/arm-trusted-firmware/pull/1171/commits/13d30c1c33609eb6cffadd50954e4301d2cab909

Thanks,
Leo Yan
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] arm64: dts: Hi3660: Fix state id for 'CPU_NAP' state

[Leo Yan]

Hi Sudeep,

On Thu, Nov 23, 2017 at 02:03:51PM +0000, Sudeep Holla wrote:
> Hi Daniel,
> 
> Thanks a lot for pointing me to this and having some useful discussion
> in private. That helped to dig a bit further on this.
> 
> On 23/11/17 05:40, Leo Yan 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.
> > 
> > This commit changes psci parameter to '0x00000000' for state id = 0;
> > this id is accepted by ARM trusted firmware and finally CPU can stay
> > properly in 'CPU_NAP' state.
> > 
> 
> I would like to conditionally NACK this patch. If we can't update the
> ARM TF at all then, I will agree with this change reluctantly.

Thanks for reviewing. Just like Daniel said, we need to figure out the
right method for this. So suggestions are very welcome!

> This looks like an artifact of copy paste in ARM TF port for this
> platform. If you look as PSCI specification, CPU suspend parameter has
> some recommendations and it's good to follow then unless you have strong
> reasons not to.
> 
> As Daniel pointed to me, this patch is required to satisfy TF
> particularly [1]. Now that looks like copy pasted from Juno or FVP port
> and if you look deeper, it's clearly under !ARM_RECOM_STATE_ID_ENC [2]
> which was not copied IIUC :).

Thanks for sharing pointers. It's shame that the copying is not
correct for Hikey960 :)

Come back to recommended state id, I reviewed Juno board defintion and
I found it's not align with PSCI spec defintion, in ARM-TF Juno code
defines state as below [1]:

#define ARM_LOCAL_STATE_RUN     0
#define ARM_LOCAL_STATE_RET     1
#define ARM_LOCAL_STATE_OFF     2

In PSCI spec chapter "6.5 Recommended StateID Encoding" recommends power
state id as below:

0: Run
1: Standby
2: Retention
3: Powerdown

So could you confirm on Hikey960 we should follow PSCI definition for
state id definition?

> Juno's implementation is legacy as these recommendations were added
> later in the specification while Juno is 3 year old platform now.
> 
> Though strictly speaking it's not violation of the PSCI specification,
> but I would rather get this fixed not before it's too late and copied to
> the next generation of platforms. Since the firmware can be easily
> upgraded that shouldn't be that difficult.

If completely compliant with PSCI recommended state id, we need change
both for ARM-TF and kernel for this. In ARM-TF, I have sent PR [2].

For the kernel patch, we should change state id as below. Please let me
know if you have suggestion for this.

diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
index 12544c3..812437a 100644
--- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
@@ -179,7 +179,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>;
@@ -188,7 +188,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>;
@@ -197,7 +197,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>;
@@ -206,7 +206,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>;


[1] https://github.com/ARM-software/arm-trusted-firmware/blob/master/include/plat/arm/common/arm_def.h#L43
[2] https://github.com/ARM-software/arm-trusted-firmware/pull/1171/commits/13d30c1c33609eb6cffadd50954e4301d2cab909

Thanks,
Leo Yan

Re: [PATCH] arm64: dts: Hi3660: Fix state id for 'CPU_NAP' state

[Sudeep Holla]

On 24/11/17 06:56, Leo Yan wrote:
> Hi Sudeep,
> 
> On Thu, Nov 23, 2017 at 02:03:51PM +0000, Sudeep Holla wrote:
>> Hi Daniel,
>>
>> Thanks a lot for pointing me to this and having some useful discussion
>> in private. That helped to dig a bit further on this.
>>
>> On 23/11/17 05:40, Leo Yan 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.
>>>
>>> This commit changes psci parameter to '0x00000000' for state id = 0;
>>> this id is accepted by ARM trusted firmware and finally CPU can stay
>>> properly in 'CPU_NAP' state.
>>>
>>
>> I would like to conditionally NACK this patch. If we can't update the
>> ARM TF at all then, I will agree with this change reluctantly.
> 
> Thanks for reviewing. Just like Daniel said, we need to figure out the
> right method for this. So suggestions are very welcome!
> 

Sure, thanks for such a quick response and resolution :)

>> This looks like an artifact of copy paste in ARM TF port for this
>> platform. If you look as PSCI specification, CPU suspend parameter has
>> some recommendations and it's good to follow then unless you have strong
>> reasons not to.
>>
>> As Daniel pointed to me, this patch is required to satisfy TF
>> particularly [1]. Now that looks like copy pasted from Juno or FVP port
>> and if you look deeper, it's clearly under !ARM_RECOM_STATE_ID_ENC [2]
>> which was not copied IIUC :).
> 
> Thanks for sharing pointers. It's shame that the copying is not
> correct for Hikey960 :)
> 

Indeed.

> Come back to recommended state id, I reviewed Juno board defintion and
> I found it's not align with PSCI spec defintion, in ARM-TF Juno code
> defines state as below [1]:
> 

Yes Juno is almost 4 years old now, so it may not be too good a
reference platform for latest and greatest platforms like hikey2 ;)
As I said, Juno predates the recommendation in the PSCI spec.

> #define ARM_LOCAL_STATE_RUN     0
> #define ARM_LOCAL_STATE_RET     1
> #define ARM_LOCAL_STATE_OFF     2
> 
> In PSCI spec chapter "6.5 Recommended StateID Encoding" recommends power
> state id as below:
> 
> 0: Run
> 1: Standby
> 2: Retention
> 3: Powerdown
> 
> So could you confirm on Hikey960 we should follow PSCI definition for
> state id definition?
> 

Yes, I don't see any reason not to, as this may become reference to some
other platform, it's good to keep it aligned so that copy paste happens
in a good sense for future platforms. :)

>> Juno's implementation is legacy as these recommendations were added
>> later in the specification while Juno is 3 year old platform now.
>>
>> Though strictly speaking it's not violation of the PSCI specification,
>> but I would rather get this fixed not before it's too late and copied to
>> the next generation of platforms. Since the firmware can be easily
>> upgraded that shouldn't be that difficult.
> 
> If completely compliant with PSCI recommended state id, we need change
> both for ARM-TF and kernel for this. In ARM-TF, I have sent PR [2].
> 

OK

> For the kernel patch, we should change state id as below. Please let me
> know if you have suggestion for this.
> 

I would wait until ATF changes are merged before you patch DT in the kernel.

-- 
Regards,
Sudeep

[PATCH] arm64: dts: Hi3660: Fix state id for 'CPU_NAP' state

[Sudeep Holla]

On 24/11/17 06:56, Leo Yan wrote:
> Hi Sudeep,
> 
> On Thu, Nov 23, 2017 at 02:03:51PM +0000, Sudeep Holla wrote:
>> Hi Daniel,
>>
>> Thanks a lot for pointing me to this and having some useful discussion
>> in private. That helped to dig a bit further on this.
>>
>> On 23/11/17 05:40, Leo Yan 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.
>>>
>>> This commit changes psci parameter to '0x00000000' for state id = 0;
>>> this id is accepted by ARM trusted firmware and finally CPU can stay
>>> properly in 'CPU_NAP' state.
>>>
>>
>> I would like to conditionally NACK this patch. If we can't update the
>> ARM TF at all then, I will agree with this change reluctantly.
> 
> Thanks for reviewing. Just like Daniel said, we need to figure out the
> right method for this. So suggestions are very welcome!
> 

Sure, thanks for such a quick response and resolution :)

>> This looks like an artifact of copy paste in ARM TF port for this
>> platform. If you look as PSCI specification, CPU suspend parameter has
>> some recommendations and it's good to follow then unless you have strong
>> reasons not to.
>>
>> As Daniel pointed to me, this patch is required to satisfy TF
>> particularly [1]. Now that looks like copy pasted from Juno or FVP port
>> and if you look deeper, it's clearly under !ARM_RECOM_STATE_ID_ENC [2]
>> which was not copied IIUC :).
> 
> Thanks for sharing pointers. It's shame that the copying is not
> correct for Hikey960 :)
> 

Indeed.

> Come back to recommended state id, I reviewed Juno board defintion and
> I found it's not align with PSCI spec defintion, in ARM-TF Juno code
> defines state as below [1]:
> 

Yes Juno is almost 4 years old now, so it may not be too good a
reference platform for latest and greatest platforms like hikey2 ;)
As I said, Juno predates the recommendation in the PSCI spec.

> #define ARM_LOCAL_STATE_RUN     0
> #define ARM_LOCAL_STATE_RET     1
> #define ARM_LOCAL_STATE_OFF     2
> 
> In PSCI spec chapter "6.5 Recommended StateID Encoding" recommends power
> state id as below:
> 
> 0: Run
> 1: Standby
> 2: Retention
> 3: Powerdown
> 
> So could you confirm on Hikey960 we should follow PSCI definition for
> state id definition?
> 

Yes, I don't see any reason not to, as this may become reference to some
other platform, it's good to keep it aligned so that copy paste happens
in a good sense for future platforms. :)

>> Juno's implementation is legacy as these recommendations were added
>> later in the specification while Juno is 3 year old platform now.
>>
>> Though strictly speaking it's not violation of the PSCI specification,
>> but I would rather get this fixed not before it's too late and copied to
>> the next generation of platforms. Since the firmware can be easily
>> upgraded that shouldn't be that difficult.
> 
> If completely compliant with PSCI recommended state id, we need change
> both for ARM-TF and kernel for this. In ARM-TF, I have sent PR [2].
> 

OK

> For the kernel patch, we should change state id as below. Please let me
> know if you have suggestion for this.
> 

I would wait until ATF changes are merged before you patch DT in the kernel.

-- 
Regards,
Sudeep

Re: [PATCH] arm64: dts: Hi3660: Fix state id for 'CPU_NAP' state

[Leo Yan]

Hi Sudeep,

On Fri, Nov 24, 2017 at 02:39:47PM +0000, Sudeep Holla wrote:

[...]

> > Come back to recommended state id, I reviewed Juno board defintion and
> > I found it's not align with PSCI spec defintion, in ARM-TF Juno code
> > defines state as below [1]:
> > 
> 
> Yes Juno is almost 4 years old now, so it may not be too good a
> reference platform for latest and greatest platforms like hikey2 ;)
> As I said, Juno predates the recommendation in the PSCI spec.
> 
> > #define ARM_LOCAL_STATE_RUN     0
> > #define ARM_LOCAL_STATE_RET     1
> > #define ARM_LOCAL_STATE_OFF     2
> > 
> > In PSCI spec chapter "6.5 Recommended StateID Encoding" recommends power
> > state id as below:
> > 
> > 0: Run
> > 1: Standby
> > 2: Retention
> > 3: Powerdown
> > 
> > So could you confirm on Hikey960 we should follow PSCI definition for
> > state id definition?
> > 
> 
> Yes, I don't see any reason not to, as this may become reference to some
> other platform, it's good to keep it aligned so that copy paste happens
> in a good sense for future platforms. :)

Thanks for upper confirmation.

> >> Juno's implementation is legacy as these recommendations were added
> >> later in the specification while Juno is 3 year old platform now.
> >>
> >> Though strictly speaking it's not violation of the PSCI specification,
> >> but I would rather get this fixed not before it's too late and copied to
> >> the next generation of platforms. Since the firmware can be easily
> >> upgraded that shouldn't be that difficult.
> > 
> > If completely compliant with PSCI recommended state id, we need change
> > both for ARM-TF and kernel for this. In ARM-TF, I have sent PR [2].
> > 
> 
> OK
> 
> > For the kernel patch, we should change state id as below. Please let me
> > know if you have suggestion for this.
> > 
> 
> I would wait until ATF changes are merged before you patch DT in the kernel.

Agree, will sent new version patch after ATF patch merging ahead.

Thank you for suggestions.
Leo Yan

Re: [PATCH] arm64: dts: Hi3660: Fix state id for 'CPU_NAP' state

[Leo Yan]

Hi Sudeep,

On Fri, Nov 24, 2017 at 02:39:47PM +0000, Sudeep Holla wrote:

[...]

> > Come back to recommended state id, I reviewed Juno board defintion and
> > I found it's not align with PSCI spec defintion, in ARM-TF Juno code
> > defines state as below [1]:
> > 
> 
> Yes Juno is almost 4 years old now, so it may not be too good a
> reference platform for latest and greatest platforms like hikey2 ;)
> As I said, Juno predates the recommendation in the PSCI spec.
> 
> > #define ARM_LOCAL_STATE_RUN     0
> > #define ARM_LOCAL_STATE_RET     1
> > #define ARM_LOCAL_STATE_OFF     2
> > 
> > In PSCI spec chapter "6.5 Recommended StateID Encoding" recommends power
> > state id as below:
> > 
> > 0: Run
> > 1: Standby
> > 2: Retention
> > 3: Powerdown
> > 
> > So could you confirm on Hikey960 we should follow PSCI definition for
> > state id definition?
> > 
> 
> Yes, I don't see any reason not to, as this may become reference to some
> other platform, it's good to keep it aligned so that copy paste happens
> in a good sense for future platforms. :)

Thanks for upper confirmation.

> >> Juno's implementation is legacy as these recommendations were added
> >> later in the specification while Juno is 3 year old platform now.
> >>
> >> Though strictly speaking it's not violation of the PSCI specification,
> >> but I would rather get this fixed not before it's too late and copied to
> >> the next generation of platforms. Since the firmware can be easily
> >> upgraded that shouldn't be that difficult.
> > 
> > If completely compliant with PSCI recommended state id, we need change
> > both for ARM-TF and kernel for this. In ARM-TF, I have sent PR [2].
> > 
> 
> OK
> 
> > For the kernel patch, we should change state id as below. Please let me
> > know if you have suggestion for this.
> > 
> 
> I would wait until ATF changes are merged before you patch DT in the kernel.

Agree, will sent new version patch after ATF patch merging ahead.

Thank you for suggestions.
Leo Yan
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] arm64: dts: Hi3660: Fix state id for 'CPU_NAP' state

[Leo Yan]

Hi Sudeep,

On Fri, Nov 24, 2017 at 02:39:47PM +0000, Sudeep Holla wrote:

[...]

> > Come back to recommended state id, I reviewed Juno board defintion and
> > I found it's not align with PSCI spec defintion, in ARM-TF Juno code
> > defines state as below [1]:
> > 
> 
> Yes Juno is almost 4 years old now, so it may not be too good a
> reference platform for latest and greatest platforms like hikey2 ;)
> As I said, Juno predates the recommendation in the PSCI spec.
> 
> > #define ARM_LOCAL_STATE_RUN     0
> > #define ARM_LOCAL_STATE_RET     1
> > #define ARM_LOCAL_STATE_OFF     2
> > 
> > In PSCI spec chapter "6.5 Recommended StateID Encoding" recommends power
> > state id as below:
> > 
> > 0: Run
> > 1: Standby
> > 2: Retention
> > 3: Powerdown
> > 
> > So could you confirm on Hikey960 we should follow PSCI definition for
> > state id definition?
> > 
> 
> Yes, I don't see any reason not to, as this may become reference to some
> other platform, it's good to keep it aligned so that copy paste happens
> in a good sense for future platforms. :)

Thanks for upper confirmation.

> >> Juno's implementation is legacy as these recommendations were added
> >> later in the specification while Juno is 3 year old platform now.
> >>
> >> Though strictly speaking it's not violation of the PSCI specification,
> >> but I would rather get this fixed not before it's too late and copied to
> >> the next generation of platforms. Since the firmware can be easily
> >> upgraded that shouldn't be that difficult.
> > 
> > If completely compliant with PSCI recommended state id, we need change
> > both for ARM-TF and kernel for this. In ARM-TF, I have sent PR [2].
> > 
> 
> OK
> 
> > For the kernel patch, we should change state id as below. Please let me
> > know if you have suggestion for this.
> > 
> 
> I would wait until ATF changes are merged before you patch DT in the kernel.

Agree, will sent new version patch after ATF patch merging ahead.

Thank you for suggestions.
Leo Yan