| Message ID | 20190513192300.653-19-ulf.hansson@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | ARM/ARM64: Support hierarchical CPU arrangement for PSCI | expand |
On Mon, May 13, 2019 at 09:23:00PM +0200, Ulf Hansson wrote: > To enable the OS to manage last-man standing activities for a CPU, while an > idle state for a group of CPUs is selected, let's convert the Hikey > platform into using the hierarchical CPU topology layout. > > Cc: Wei Xu <xuwei5@hisilicon.com> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > --- > > Changes: > - None. > > --- > arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 87 ++++++++++++++++++++--- > 1 file changed, 76 insertions(+), 11 deletions(-) > > diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi > index 108e2a4227f6..36ff460f428f 100644 > --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi > +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi > cpus { [...] > @@ -70,9 +128,8 @@ > }; > > CLUSTER_SLEEP: cluster-sleep { > - compatible = "arm,idle-state"; > - local-timer-stop; > - arm,psci-suspend-param = <0x1010000>; > + compatible = "domain-idle-state"; > + arm,psci-suspend-param = <0x1000000>; > entry-latency-us = <1000>; > exit-latency-us = <700>; > min-residency-us = <2700>; Again this must be original format and as per PSCI spec, your patch changes this cluster sleep state into cluster retention state which I think is not what you intended. -- Regards, Sudeep
On Tue, 16 Jul 2019 at 16:47, Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Mon, May 13, 2019 at 09:23:00PM +0200, Ulf Hansson wrote: > > To enable the OS to manage last-man standing activities for a CPU, while an > > idle state for a group of CPUs is selected, let's convert the Hikey > > platform into using the hierarchical CPU topology layout. > > > > Cc: Wei Xu <xuwei5@hisilicon.com> > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > --- > > > > Changes: > > - None. > > > > --- > > arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 87 ++++++++++++++++++++--- > > 1 file changed, 76 insertions(+), 11 deletions(-) > > > > diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi > > index 108e2a4227f6..36ff460f428f 100644 > > --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi > > +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi > > cpus { > > [...] > > > @@ -70,9 +128,8 @@ > > }; > > > > CLUSTER_SLEEP: cluster-sleep { > > - compatible = "arm,idle-state"; > > - local-timer-stop; > > - arm,psci-suspend-param = <0x1010000>; > > + compatible = "domain-idle-state"; > > + arm,psci-suspend-param = <0x1000000>; > > entry-latency-us = <1000>; > > exit-latency-us = <700>; > > min-residency-us = <2700>; > > Again this must be original format and as per PSCI spec, your patch > changes this cluster sleep state into cluster retention state which I > think is not what you intended. If the hierarchical topology is used, the parameter for cluster states are ORed with the deepest idle state for the CPU. CPU_SLEEP: 0x0010000 CLUSTER_SLEEP: 0x1000000 After the ORed operation CLUSTER_SLEEP: 0x1010000 So, this indeed works as expected. However, are you saying that ORing the state parameters like above has other problems? I am reading your other replies... Kind regards Uffe
On Thu, Jul 18, 2019 at 12:48:14PM +0200, Ulf Hansson wrote: > On Tue, 16 Jul 2019 at 16:47, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > On Mon, May 13, 2019 at 09:23:00PM +0200, Ulf Hansson wrote: > > > To enable the OS to manage last-man standing activities for a CPU, while an > > > idle state for a group of CPUs is selected, let's convert the Hikey > > > platform into using the hierarchical CPU topology layout. > > > > > > Cc: Wei Xu <xuwei5@hisilicon.com> > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > > --- > > > > > > Changes: > > > - None. > > > > > > --- > > > arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 87 ++++++++++++++++++++--- > > > 1 file changed, 76 insertions(+), 11 deletions(-) > > > > > > diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi > > > index 108e2a4227f6..36ff460f428f 100644 > > > --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi > > > +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi > > > cpus { > > > > [...] > > > > > @@ -70,9 +128,8 @@ > > > }; > > > > > > CLUSTER_SLEEP: cluster-sleep { > > > - compatible = "arm,idle-state"; > > > - local-timer-stop; > > > - arm,psci-suspend-param = <0x1010000>; > > > + compatible = "domain-idle-state"; > > > + arm,psci-suspend-param = <0x1000000>; > > > entry-latency-us = <1000>; > > > exit-latency-us = <700>; > > > min-residency-us = <2700>; > > > > Again this must be original format and as per PSCI spec, your patch > > changes this cluster sleep state into cluster retention state which I > > think is not what you intended. > > If the hierarchical topology is used, the parameter for cluster states > are ORed with the deepest idle state for the CPU. > > CPU_SLEEP: 0x0010000 > CLUSTER_SLEEP: 0x1000000 > > After the ORed operation > CLUSTER_SLEEP: 0x1010000 > > So, this indeed works as expected. > Yes, it works. But we are not XOR-ing so what's wrong in keeping the StateType as required and be compliant to specification. Why do we need to make the state param on it's own non-complaint. What's wrong in retaining CLUSTER_SLEEP as 0x1010000 so that it reflects the state level and type correctly on it's own ? > However, are you saying that ORing the state parameters like above has > other problems? I am reading your other replies... > Yes OR-ing may have other problems but the point I made here was more on PSCI spec compliance for each suspend parameter values independently. -- Regards, Sudeep
diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi index 108e2a4227f6..36ff460f428f 100644 --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi @@ -20,6 +20,64 @@ psci { compatible = "arm,psci-0.2"; method = "smc"; + + CPU_PD0: cpu-pd0 { + #power-domain-cells = <0>; + power-domains = <&CLUSTER_PD0>; + domain-idle-states = <&CPU_SLEEP>; + }; + + CPU_PD1: cpu-pd1 { + #power-domain-cells = <0>; + power-domains = <&CLUSTER_PD0>; + domain-idle-states = <&CPU_SLEEP>; + }; + + CPU_PD2: cpu-pd2 { + #power-domain-cells = <0>; + power-domains = <&CLUSTER_PD0>; + domain-idle-states = <&CPU_SLEEP>; + }; + + CPU_PD3: cpu-pd3 { + #power-domain-cells = <0>; + power-domains = <&CLUSTER_PD0>; + domain-idle-states = <&CPU_SLEEP>; + }; + + CPU_PD4: cpu-pd4 { + #power-domain-cells = <0>; + power-domains = <&CLUSTER_PD1>; + domain-idle-states = <&CPU_SLEEP>; + }; + + CPU_PD5: cpu-pd5 { + #power-domain-cells = <0>; + power-domains = <&CLUSTER_PD1>; + domain-idle-states = <&CPU_SLEEP>; + }; + + CPU_PD6: cpu-pd6 { + #power-domain-cells = <0>; + power-domains = <&CLUSTER_PD1>; + domain-idle-states = <&CPU_SLEEP>; + }; + + CPU_PD7: cpu-pd7 { + #power-domain-cells = <0>; + power-domains = <&CLUSTER_PD1>; + domain-idle-states = <&CPU_SLEEP>; + }; + + CLUSTER_PD0: cluster-pd0 { + #power-domain-cells = <0>; + domain-idle-states = <&CLUSTER_SLEEP>; + }; + + CLUSTER_PD1: cluster-pd1 { + #power-domain-cells = <0>; + domain-idle-states = <&CLUSTER_SLEEP>; + }; }; cpus { @@ -70,9 +128,8 @@ }; CLUSTER_SLEEP: cluster-sleep { - compatible = "arm,idle-state"; - local-timer-stop; - arm,psci-suspend-param = <0x1010000>; + compatible = "domain-idle-state"; + arm,psci-suspend-param = <0x1000000>; entry-latency-us = <1000>; exit-latency-us = <700>; min-residency-us = <2700>; @@ -88,9 +145,10 @@ next-level-cache = <&CLUSTER0_L2>; clocks = <&stub_clock 0>; operating-points-v2 = <&cpu_opp_table>; - cpu-idle-states = <&CPU_SLEEP &CLUSTER_SLEEP>; #cooling-cells = <2>; /* min followed by max */ dynamic-power-coefficient = <311>; + power-domains = <&CPU_PD0>; + power-domain-names = "psci"; }; cpu1: cpu@1 { @@ -101,9 +159,10 @@ next-level-cache = <&CLUSTER0_L2>; clocks = <&stub_clock 0>; operating-points-v2 = <&cpu_opp_table>; - cpu-idle-states = <&CPU_SLEEP &CLUSTER_SLEEP>; #cooling-cells = <2>; /* min followed by max */ dynamic-power-coefficient = <311>; + power-domains = <&CPU_PD1>; + power-domain-names = "psci"; }; cpu2: cpu@2 { @@ -114,9 +173,10 @@ next-level-cache = <&CLUSTER0_L2>; clocks = <&stub_clock 0>; operating-points-v2 = <&cpu_opp_table>; - cpu-idle-states = <&CPU_SLEEP &CLUSTER_SLEEP>; #cooling-cells = <2>; /* min followed by max */ dynamic-power-coefficient = <311>; + power-domains = <&CPU_PD2>; + power-domain-names = "psci"; }; cpu3: cpu@3 { @@ -127,9 +187,10 @@ next-level-cache = <&CLUSTER0_L2>; clocks = <&stub_clock 0>; operating-points-v2 = <&cpu_opp_table>; - cpu-idle-states = <&CPU_SLEEP &CLUSTER_SLEEP>; #cooling-cells = <2>; /* min followed by max */ dynamic-power-coefficient = <311>; + power-domains = <&CPU_PD3>; + power-domain-names = "psci"; }; cpu4: cpu@100 { @@ -140,9 +201,10 @@ next-level-cache = <&CLUSTER1_L2>; clocks = <&stub_clock 0>; operating-points-v2 = <&cpu_opp_table>; - cpu-idle-states = <&CPU_SLEEP &CLUSTER_SLEEP>; #cooling-cells = <2>; /* min followed by max */ dynamic-power-coefficient = <311>; + power-domains = <&CPU_PD4>; + power-domain-names = "psci"; }; cpu5: cpu@101 { @@ -153,9 +215,10 @@ next-level-cache = <&CLUSTER1_L2>; clocks = <&stub_clock 0>; operating-points-v2 = <&cpu_opp_table>; - cpu-idle-states = <&CPU_SLEEP &CLUSTER_SLEEP>; #cooling-cells = <2>; /* min followed by max */ dynamic-power-coefficient = <311>; + power-domains = <&CPU_PD5>; + power-domain-names = "psci"; }; cpu6: cpu@102 { @@ -166,9 +229,10 @@ next-level-cache = <&CLUSTER1_L2>; clocks = <&stub_clock 0>; operating-points-v2 = <&cpu_opp_table>; - cpu-idle-states = <&CPU_SLEEP &CLUSTER_SLEEP>; #cooling-cells = <2>; /* min followed by max */ dynamic-power-coefficient = <311>; + power-domains = <&CPU_PD6>; + power-domain-names = "psci"; }; cpu7: cpu@103 { @@ -179,9 +243,10 @@ next-level-cache = <&CLUSTER1_L2>; clocks = <&stub_clock 0>; operating-points-v2 = <&cpu_opp_table>; - cpu-idle-states = <&CPU_SLEEP &CLUSTER_SLEEP>; #cooling-cells = <2>; /* min followed by max */ dynamic-power-coefficient = <311>; + power-domains = <&CPU_PD7>; + power-domain-names = "psci"; }; CLUSTER0_L2: l2-cache0 {
To enable the OS to manage last-man standing activities for a CPU, while an idle state for a group of CPUs is selected, let's convert the Hikey platform into using the hierarchical CPU topology layout. Cc: Wei Xu <xuwei5@hisilicon.com> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- Changes: - None. --- arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 87 ++++++++++++++++++++--- 1 file changed, 76 insertions(+), 11 deletions(-)