* [PATCH v2] arm64: dts: Hi3660: Fix up psci state id @ 2017-12-12 9:12 Leo Yan 2017-12-22 9:37 ` Wei Xu 2017-12-22 14:22 ` Vincent Guittot 0 siblings, 2 replies; 7+ messages in thread From: Leo Yan @ 2017-12-12 9:12 UTC (permalink / raw) To: Wei Xu, Rob Herring, Mark Rutland, Catalin Marinas, Will Deacon, linux-arm-kernel, devicetree, linux-kernel Cc: Leo Yan, Vincent Guittot, Daniel Lezcano, Sudeep Holla, Soby Mathew 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] arm64: dts: Hi3660: Fix up psci state id 2017-12-12 9:12 [PATCH v2] arm64: dts: Hi3660: Fix up psci state id Leo Yan @ 2017-12-22 9:37 ` Wei Xu 2017-12-28 21:14 ` Wei Xu 2017-12-22 14:22 ` Vincent Guittot 1 sibling, 1 reply; 7+ messages in thread From: Wei Xu @ 2017-12-22 9:37 UTC (permalink / raw) To: Leo Yan, Rob Herring, Mark Rutland, Catalin Marinas, Will Deacon, linux-arm-kernel, devicetree, linux-kernel Cc: Vincent Guittot, Daniel Lezcano, Sudeep Holla, Soby Mathew 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>; > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] arm64: dts: Hi3660: Fix up psci state id 2017-12-22 9:37 ` Wei Xu @ 2017-12-28 21:14 ` Wei Xu 2017-12-28 23:13 ` Leo Yan 0 siblings, 1 reply; 7+ messages in thread From: Wei Xu @ 2017-12-28 21:14 UTC (permalink / raw) To: Leo Yan, Rob Herring, Mark Rutland, Catalin Marinas, Will Deacon, linux-arm-kernel, devicetree, linux-kernel Cc: Vincent Guittot, Daniel Lezcano, Sudeep Holla, Soby Mathew 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>; >> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] arm64: dts: Hi3660: Fix up psci state id 2017-12-28 21:14 ` Wei Xu @ 2017-12-28 23:13 ` Leo Yan 0 siblings, 0 replies; 7+ messages in thread From: Leo Yan @ 2017-12-28 23:13 UTC (permalink / raw) To: Wei Xu Cc: Rob Herring, Mark Rutland, Catalin Marinas, Will Deacon, linux-arm-kernel, devicetree, linux-kernel, Vincent Guittot, Daniel Lezcano, Sudeep Holla, Soby Mathew 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] arm64: dts: Hi3660: Fix up psci state id 2017-12-12 9:12 [PATCH v2] arm64: dts: Hi3660: Fix up psci state id Leo Yan 2017-12-22 9:37 ` Wei Xu @ 2017-12-22 14:22 ` Vincent Guittot 2017-12-25 2:22 ` Leo Yan 1 sibling, 1 reply; 7+ messages in thread From: Vincent Guittot @ 2017-12-22 14:22 UTC (permalink / raw) To: Leo Yan Cc: Wei Xu, Rob Herring, Mark Rutland, Catalin Marinas, Will Deacon, LAK, devicetree, linux-kernel, Daniel Lezcano, Sudeep Holla, Soby Mathew 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] arm64: dts: Hi3660: Fix up psci state id 2017-12-22 14:22 ` Vincent Guittot @ 2017-12-25 2:22 ` Leo Yan 2017-12-27 8:29 ` Vincent Guittot 0 siblings, 1 reply; 7+ messages in thread From: Leo Yan @ 2017-12-25 2:22 UTC (permalink / raw) To: Vincent Guittot Cc: Wei Xu, Rob Herring, Mark Rutland, Catalin Marinas, Will Deacon, LAK, devicetree, linux-kernel, Daniel Lezcano, Sudeep Holla, Soby Mathew, John Stultz, kevin.wangtao 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 > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] arm64: dts: Hi3660: Fix up psci state id 2017-12-25 2:22 ` Leo Yan @ 2017-12-27 8:29 ` Vincent Guittot 0 siblings, 0 replies; 7+ messages in thread From: Vincent Guittot @ 2017-12-27 8:29 UTC (permalink / raw) To: Leo Yan Cc: Wei Xu, Rob Herring, Mark Rutland, Catalin Marinas, Will Deacon, LAK, devicetree, linux-kernel, Daniel Lezcano, Sudeep Holla, Soby Mathew, John Stultz, Wangtao (Kevin, Kirin) Hi Leo On 25 December 2017 at 03:22, Leo Yan <leo.yan@linaro.org> wrote: > 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? I would prefer to remove it right now. Removing NAP from c-state table makes the hikey960 working correctly; I mean even with current ATF and current state id. So it's the best solution to the NAP problem IMO and I don't see the benefit of keeping NAP in the table until this state has been fixed. This will just add uncertainties in the behavior of the board. I don't see why you can't re-add it once it has been fixed. > >> 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 And that's my main concern because this adds a new possible regression factor when switching between different kernel version > 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 >> > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-12-28 23:14 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-12-12 9:12 [PATCH v2] arm64: dts: Hi3660: Fix up psci state id Leo Yan 2017-12-22 9:37 ` Wei Xu 2017-12-28 21:14 ` Wei Xu 2017-12-28 23:13 ` Leo Yan 2017-12-22 14:22 ` Vincent Guittot 2017-12-25 2:22 ` Leo Yan 2017-12-27 8:29 ` Vincent Guittot
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).