* [PATCH] arm64: dts: Hi3660: Fix state id for 'CPU_NAP' state @ 2017-11-23 5:40 Leo Yan 2017-11-23 14:03 ` Sudeep Holla 0 siblings, 1 reply; 5+ messages in thread From: Leo Yan @ 2017-11-23 5:40 UTC (permalink / raw) To: Wei Xu, Mark Rutland, linux-arm-kernel, linux-kernel, Rob Herring, devicetree Cc: Leo Yan, Vincent Guittot, Daniel Lezcano 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] arm64: dts: Hi3660: Fix state id for 'CPU_NAP' state 2017-11-23 5:40 [PATCH] arm64: dts: Hi3660: Fix state id for 'CPU_NAP' state Leo Yan @ 2017-11-23 14:03 ` Sudeep Holla 2017-11-24 6:56 ` Leo Yan 0 siblings, 1 reply; 5+ messages in thread From: Sudeep Holla @ 2017-11-23 14:03 UTC (permalink / raw) To: Leo Yan, Wei Xu, Mark Rutland, linux-arm-kernel, linux-kernel, Rob Herring, devicetree, Daniel Lezcano Cc: Sudeep Holla, Vincent Guittot 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] arm64: dts: Hi3660: Fix state id for 'CPU_NAP' state 2017-11-23 14:03 ` Sudeep Holla @ 2017-11-24 6:56 ` Leo Yan 2017-11-24 14:39 ` Sudeep Holla 0 siblings, 1 reply; 5+ messages in thread From: Leo Yan @ 2017-11-24 6:56 UTC (permalink / raw) To: Sudeep Holla Cc: Wei Xu, Mark Rutland, linux-arm-kernel, linux-kernel, Rob Herring, devicetree, Daniel Lezcano, Vincent Guittot 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] arm64: dts: Hi3660: Fix state id for 'CPU_NAP' state 2017-11-24 6:56 ` Leo Yan @ 2017-11-24 14:39 ` Sudeep Holla 2017-11-27 2:01 ` Leo Yan 0 siblings, 1 reply; 5+ messages in thread From: Sudeep Holla @ 2017-11-24 14:39 UTC (permalink / raw) To: Leo Yan Cc: Sudeep Holla, Wei Xu, Mark Rutland, linux-arm-kernel, linux-kernel, Rob Herring, devicetree, Daniel Lezcano, Vincent Guittot 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] arm64: dts: Hi3660: Fix state id for 'CPU_NAP' state 2017-11-24 14:39 ` Sudeep Holla @ 2017-11-27 2:01 ` Leo Yan 0 siblings, 0 replies; 5+ messages in thread From: Leo Yan @ 2017-11-27 2:01 UTC (permalink / raw) To: Sudeep Holla Cc: Wei Xu, Mark Rutland, linux-arm-kernel, linux-kernel, Rob Herring, devicetree, Daniel Lezcano, Vincent Guittot 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-11-27 2:01 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-11-23 5:40 [PATCH] arm64: dts: Hi3660: Fix state id for 'CPU_NAP' state Leo Yan 2017-11-23 14:03 ` Sudeep Holla 2017-11-24 6:56 ` Leo Yan 2017-11-24 14:39 ` Sudeep Holla 2017-11-27 2:01 ` Leo Yan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).