| Message ID | 1513069954-22827-1-git-send-email-leo.yan@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] arm64: dts: Hi3660: Fix up psci state id | expand |
Hi Leo, On 2017/12/12 9:12, 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. > > 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> > --- Applied into hisilicon dt tree. Thanks! Best Regards, Wei > 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>; > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 ? 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 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 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Leo, On 2017/12/22 9:37, Wei Xu wrote: > Hi Leo, > > On 2017/12/12 9:12, 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. >> >> 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> >> --- > > Applied into hisilicon dt tree. > Thanks! Sorry, since this patch is still under discussion, I will drop it firstly. Thanks! Best Regards, Wei > > Best Regards, > Wei > >> 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>; >> -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 28, 2017 at 09:14:03PM +0000, Wei Xu wrote: > Hi Leo, [...] > Sorry, since this patch is still under discussion, > I will drop it firstly. > Thanks! Thanks, Wei. > Best Regards, > Wei -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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>;
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(-) -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html