* Re: [PATCH v2] arm64: dts: sdm845: Add PSCI cpuidle low power states
2018-10-30 17:23 [PATCH v2] arm64: dts: sdm845: Add PSCI cpuidle low power states Raju P.L.S.S.S.N
@ 2018-10-31 23:57 ` Doug Anderson
2018-11-01 17:21 ` Evan Green
` (2 subsequent siblings)
3 siblings, 0 replies; 10+ messages in thread
From: Doug Anderson @ 2018-10-31 23:57 UTC (permalink / raw)
To: Raju P L S S S N
Cc: Andy Gross, David Brown, linux-arm-msm,
open list:ARM/QUALCOMM SUPPORT, Rajendra Nayak, Bjorn Andersson,
LKML, linux-pm, devicetree, Stephen Boyd, Evan Green,
Matthias Kaehlcke, Lina Iyer
Hi,
On Tue, Oct 30, 2018 at 10:23 AM Raju P.L.S.S.S.N
<rplsssn@codeaurora.org> wrote:
>
> Add device bindings for cpuidle states for cpu devices.
>
> Cc: <devicetree@vger.kernel.org>
> Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
> ---
> Changes in v2
> - Address comments from Doug
> ---
> ---
> arch/arm64/boot/dts/qcom/sdm845.dtsi | 62 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 62 insertions(+)
I haven't been following all the discussion about idle states closely
enough to know if the contents of this patch is correct. Thus I won't
provide my Reviewed-by since I expect Evan and/or Lina will provide
theirs again like they did on v1. However, I'll mention that the nits
I had regarding v1 have been fixed so I'm happy with this patch
landing if others are. ;-)
-Doug
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2] arm64: dts: sdm845: Add PSCI cpuidle low power states
2018-10-30 17:23 [PATCH v2] arm64: dts: sdm845: Add PSCI cpuidle low power states Raju P.L.S.S.S.N
2018-10-31 23:57 ` Doug Anderson
@ 2018-11-01 17:21 ` Evan Green
2018-11-30 15:41 ` Lina Iyer
2019-05-07 22:20 ` Amit Kucheria
3 siblings, 0 replies; 10+ messages in thread
From: Evan Green @ 2018-11-01 17:21 UTC (permalink / raw)
To: rplsssn
Cc: Andy Gross, David Brown, linux-arm-msm, linux-soc,
Rajendra Nayak, Bjorn Andersson, linux-kernel, linux-pm,
devicetree, sboyd, Doug Anderson, mka, Lina Iyer
On Tue, Oct 30, 2018 at 10:23 AM Raju P.L.S.S.S.N
<rplsssn@codeaurora.org> wrote:
>
> Add device bindings for cpuidle states for cpu devices.
>
> Cc: <devicetree@vger.kernel.org>
> Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
> ---
> Changes in v2
> - Address comments from Doug
> ---
> ---
> arch/arm64/boot/dts/qcom/sdm845.dtsi | 62 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 62 insertions(+)
>
Reviewed-by: Evan Green <evgreen@chromium.org>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] arm64: dts: sdm845: Add PSCI cpuidle low power states
2018-10-30 17:23 [PATCH v2] arm64: dts: sdm845: Add PSCI cpuidle low power states Raju P.L.S.S.S.N
2018-10-31 23:57 ` Doug Anderson
2018-11-01 17:21 ` Evan Green
@ 2018-11-30 15:41 ` Lina Iyer
2018-12-20 21:40 ` Doug Anderson
2019-05-07 22:20 ` Amit Kucheria
3 siblings, 1 reply; 10+ messages in thread
From: Lina Iyer @ 2018-11-30 15:41 UTC (permalink / raw)
To: Raju P.L.S.S.S.N
Cc: andy.gross, david.brown, linux-arm-msm, linux-soc, rnayak,
bjorn.andersson, linux-kernel, linux-pm, devicetree, sboyd,
evgreen, dianders, mka
On Tue, Oct 30 2018 at 11:23 -0600, Raju P.L.S.S.S.N wrote:
>Add device bindings for cpuidle states for cpu devices.
>
>Cc: <devicetree@vger.kernel.org>
>Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
>---
>Changes in v2
> - Address comments from Doug
>---
>---
> arch/arm64/boot/dts/qcom/sdm845.dtsi | 62 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 62 insertions(+)
>
>diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>index 0c9a2aa..3a8381e 100644
>--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
>+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>@@ -96,6 +96,7 @@
> reg = <0x0 0x0>;
> enable-method = "psci";
> next-level-cache = <&L2_0>;
>+ cpu-idle-states = <&C0_CPU_PD &C0_CPU_RPD &CLUSTER_PD>;
I think it might be better to use
<&C0_CPU_PD>, <&C0_CPU_RPD>, <&CLUSTER_PD>
> L2_0: l2-cache {
> compatible = "cache";
> next-level-cache = <&L3_0>;
>@@ -111,6 +112,7 @@
> reg = <0x0 0x100>;
> enable-method = "psci";
> next-level-cache = <&L2_100>;
>+ cpu-idle-states = <&C0_CPU_PD &C0_CPU_RPD &CLUSTER_PD>;
> L2_100: l2-cache {
> compatible = "cache";
> next-level-cache = <&L3_0>;
>@@ -123,6 +125,7 @@
> reg = <0x0 0x200>;
> enable-method = "psci";
> next-level-cache = <&L2_200>;
>+ cpu-idle-states = <&C0_CPU_PD &C0_CPU_RPD &CLUSTER_PD>;
> L2_200: l2-cache {
> compatible = "cache";
> next-level-cache = <&L3_0>;
>@@ -135,6 +138,7 @@
> reg = <0x0 0x300>;
> enable-method = "psci";
> next-level-cache = <&L2_300>;
>+ cpu-idle-states = <&C0_CPU_PD &C0_CPU_RPD &CLUSTER_PD>;
> L2_300: l2-cache {
> compatible = "cache";
> next-level-cache = <&L3_0>;
>@@ -147,6 +151,7 @@
> reg = <0x0 0x400>;
> enable-method = "psci";
> next-level-cache = <&L2_400>;
>+ cpu-idle-states = <&C4_CPU_PD &C4_CPU_RPD &CLUSTER_PD>;
> L2_400: l2-cache {
> compatible = "cache";
> next-level-cache = <&L3_0>;
>@@ -159,6 +164,7 @@
> reg = <0x0 0x500>;
> enable-method = "psci";
> next-level-cache = <&L2_500>;
>+ cpu-idle-states = <&C4_CPU_PD &C4_CPU_RPD &CLUSTER_PD>;
> L2_500: l2-cache {
> compatible = "cache";
> next-level-cache = <&L3_0>;
>@@ -171,6 +177,7 @@
> reg = <0x0 0x600>;
> enable-method = "psci";
> next-level-cache = <&L2_600>;
>+ cpu-idle-states = <&C4_CPU_PD &C4_CPU_RPD &CLUSTER_PD>;
> L2_600: l2-cache {
> compatible = "cache";
> next-level-cache = <&L3_0>;
>@@ -183,11 +190,66 @@
> reg = <0x0 0x700>;
> enable-method = "psci";
> next-level-cache = <&L2_700>;
>+ cpu-idle-states = <&C4_CPU_PD &C4_CPU_RPD &CLUSTER_PD>;
> L2_700: l2-cache {
> compatible = "cache";
> next-level-cache = <&L3_0>;
> };
> };
>+
>+ idle-states {
>+ entry-method = "psci";
>+
>+ C0_CPU_PD: c0-power-down {
>+ compatible = "arm,idle-state";
>+ arm,psci-suspend-param = <0x40000003>;
>+ entry-latency-us = <350>;
>+ exit-latency-us = <461>;
>+ min-residency-us = <1890>;
>+ local-timer-stop;
>+ idle-state-name = "power-down";
>+ };
>+
>+ C0_CPU_RPD: c0-rail-power-down {
>+ compatible = "arm,idle-state";
>+ arm,psci-suspend-param = <0x40000004>;
>+ entry-latency-us = <360>;
>+ exit-latency-us = <531>;
>+ min-residency-us = <3934>;
>+ local-timer-stop;
>+ idle-state-name = "rail-power-down";
>+ };
>+
>+ C4_CPU_PD: c4-power-down {
>+ compatible = "arm,idle-state";
>+ arm,psci-suspend-param = <0x40000003>;
>+ entry-latency-us = <264>;
>+ exit-latency-us = <621>;
>+ min-residency-us = <952>;
>+ local-timer-stop;
>+ idle-state-name = "power-down";
>+ };
>+
>+ C4_CPU_RPD: c4-rail-power-down {
>+ compatible = "arm,idle-state";
>+ arm,psci-suspend-param = <0x40000004>;
>+ entry-latency-us = <702>;
>+ exit-latency-us = <1061>;
>+ min-residency-us = <4488>;
>+ local-timer-stop;
>+ idle-state-name = "rail-power-down";
>+ };
>+
>+ CLUSTER_PD: cluster-power-down {
>+ compatible = "arm,idle-state";
>+ arm,psci-suspend-param = <0x400000F4>;
>+ entry-latency-us = <3263>;
>+ exit-latency-us = <6562>;
>+ min-residency-us = <9987>;
>+ local-timer-stop;
>+ idle-state-name = "cluster-power-down";
>+ };
>+ };
> };
>
> pmu {
>--
>QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
>of the Code Aurora Forum, hosted by The Linux Foundation.
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2] arm64: dts: sdm845: Add PSCI cpuidle low power states
2018-11-30 15:41 ` Lina Iyer
@ 2018-12-20 21:40 ` Doug Anderson
2018-12-20 22:55 ` Lina Iyer
0 siblings, 1 reply; 10+ messages in thread
From: Doug Anderson @ 2018-12-20 21:40 UTC (permalink / raw)
To: Lina Iyer
Cc: Raju P.L.S.S.S.N, Andy Gross, David Brown, linux-arm-msm,
open list:ARM/QUALCOMM SUPPORT, Rajendra Nayak, Bjorn Andersson,
LKML, linux-pm, devicetree, Stephen Boyd, Evan Green,
Matthias Kaehlcke
Hi,
On Fri, Nov 30, 2018 at 7:41 AM Lina Iyer <ilina@codeaurora.org> wrote:
>
> On Tue, Oct 30 2018 at 11:23 -0600, Raju P.L.S.S.S.N wrote:
> >Add device bindings for cpuidle states for cpu devices.
> >
> >Cc: <devicetree@vger.kernel.org>
> >Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
> >---
> >Changes in v2
> > - Address comments from Doug
> >---
> >---
> > arch/arm64/boot/dts/qcom/sdm845.dtsi | 62 ++++++++++++++++++++++++++++++++++++
> > 1 file changed, 62 insertions(+)
> >
> >diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> >index 0c9a2aa..3a8381e 100644
> >--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> >+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> >@@ -96,6 +96,7 @@
> > reg = <0x0 0x0>;
> > enable-method = "psci";
> > next-level-cache = <&L2_0>;
> >+ cpu-idle-states = <&C0_CPU_PD &C0_CPU_RPD &CLUSTER_PD>;
> I think it might be better to use
> <&C0_CPU_PD>, <&C0_CPU_RPD>, <&CLUSTER_PD>
I disagree. All existing examples in both bindings and other device
trees use the syntax that Raju uses. It doesn't matter
functionality-wise, but I'd much rather match everyone else. We
should land what Raju has posted and if someone feels super strongly
that the examples in the bindings are wrong then they should attempt
to convince Rob Herring to Ack a change to the examples in the
bindings. Doesn't seem like a good use of time to me, though.
-Doug
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] arm64: dts: sdm845: Add PSCI cpuidle low power states
2018-12-20 21:40 ` Doug Anderson
@ 2018-12-20 22:55 ` Lina Iyer
0 siblings, 0 replies; 10+ messages in thread
From: Lina Iyer @ 2018-12-20 22:55 UTC (permalink / raw)
To: Doug Anderson
Cc: Raju P.L.S.S.S.N, Andy Gross, David Brown, linux-arm-msm,
open list:ARM/QUALCOMM SUPPORT, Rajendra Nayak, Bjorn Andersson,
LKML, linux-pm, devicetree, Stephen Boyd, Evan Green,
Matthias Kaehlcke
On Thu, Dec 20 2018 at 14:42 -0700, Doug Anderson wrote:
>Hi,
>
>On Fri, Nov 30, 2018 at 7:41 AM Lina Iyer <ilina@codeaurora.org> wrote:
>>
>> On Tue, Oct 30 2018 at 11:23 -0600, Raju P.L.S.S.S.N wrote:
>> >Add device bindings for cpuidle states for cpu devices.
>> >
>> >Cc: <devicetree@vger.kernel.org>
>> >Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
>> >---
>> >Changes in v2
>> > - Address comments from Doug
>> >---
>> >---
>> > arch/arm64/boot/dts/qcom/sdm845.dtsi | 62 ++++++++++++++++++++++++++++++++++++
>> > 1 file changed, 62 insertions(+)
>> >
>> >diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> >index 0c9a2aa..3a8381e 100644
>> >--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> >+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> >@@ -96,6 +96,7 @@
>> > reg = <0x0 0x0>;
>> > enable-method = "psci";
>> > next-level-cache = <&L2_0>;
>> >+ cpu-idle-states = <&C0_CPU_PD &C0_CPU_RPD &CLUSTER_PD>;
>> I think it might be better to use
>> <&C0_CPU_PD>, <&C0_CPU_RPD>, <&CLUSTER_PD>
>
>I disagree. All existing examples in both bindings and other device
>trees use the syntax that Raju uses. It doesn't matter
>functionality-wise, but I'd much rather match everyone else. We
>should land what Raju has posted and if someone feels super strongly
>that the examples in the bindings are wrong then they should attempt
>to convince Rob Herring to Ack a change to the examples in the
>bindings. Doesn't seem like a good use of time to me, though.
>
No strong preference from my end.
-- Lina
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] arm64: dts: sdm845: Add PSCI cpuidle low power states
@ 2019-05-07 22:20 ` Amit Kucheria
0 siblings, 0 replies; 10+ messages in thread
From: Amit Kucheria @ 2019-05-07 22:20 UTC (permalink / raw)
To: Raju P.L.S.S.S.N
Cc: Andy Gross, David Brown, linux-arm-msm,
open list:ARM/QUALCOMM SUPPORT, Rajendra Nayak, Bjorn Andersson,
LKML, Linux PM list,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Stephen Boyd, evgreen, Douglas Anderson, Matthias Kaehlcke,
ilina
On Tue, Oct 30, 2018 at 10:53 PM Raju P.L.S.S.S.N
<rplsssn@codeaurora.org> wrote:
>
> Add device bindings for cpuidle states for cpu devices.
Raju: Did this patch fall through the cracks? It would be nice to land
this while Lina works on setting up the infrastructure to do
hierarchical power domains.
> Cc: <devicetree@vger.kernel.org>
> Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
> ---
> Changes in v2
> - Address comments from Doug
> ---
> ---
> arch/arm64/boot/dts/qcom/sdm845.dtsi | 62 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 62 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index 0c9a2aa..3a8381e 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -96,6 +96,7 @@
> reg = <0x0 0x0>;
> enable-method = "psci";
> next-level-cache = <&L2_0>;
> + cpu-idle-states = <&C0_CPU_PD &C0_CPU_RPD &CLUSTER_PD>;
May I suggest using more generic names here instead of C0_CPU_PD and
move the QC-specific description to the idle-state-name property? C0
and C4 isn't easy to understand at a glance. Neither is PD and RPD.
Something like big_cpu_retention, big_cpu_sleep, little_cpu_retention,
little_cpu_sleep, cluster_sleep?
Or big_cpu_idle_0, big_cpu_idle_1, big_cpu_idle_2 for states with
increasing breakeven times.
I've commented below on what it might look like.
> L2_0: l2-cache {
> compatible = "cache";
> next-level-cache = <&L3_0>;
> @@ -111,6 +112,7 @@
> reg = <0x0 0x100>;
> enable-method = "psci";
> next-level-cache = <&L2_100>;
> + cpu-idle-states = <&C0_CPU_PD &C0_CPU_RPD &CLUSTER_PD>;
> L2_100: l2-cache {
> compatible = "cache";
> next-level-cache = <&L3_0>;
> @@ -123,6 +125,7 @@
> reg = <0x0 0x200>;
> enable-method = "psci";
> next-level-cache = <&L2_200>;
> + cpu-idle-states = <&C0_CPU_PD &C0_CPU_RPD &CLUSTER_PD>;
> L2_200: l2-cache {
> compatible = "cache";
> next-level-cache = <&L3_0>;
> @@ -135,6 +138,7 @@
> reg = <0x0 0x300>;
> enable-method = "psci";
> next-level-cache = <&L2_300>;
> + cpu-idle-states = <&C0_CPU_PD &C0_CPU_RPD &CLUSTER_PD>;
> L2_300: l2-cache {
> compatible = "cache";
> next-level-cache = <&L3_0>;
> @@ -147,6 +151,7 @@
> reg = <0x0 0x400>;
> enable-method = "psci";
> next-level-cache = <&L2_400>;
> + cpu-idle-states = <&C4_CPU_PD &C4_CPU_RPD &CLUSTER_PD>;
> L2_400: l2-cache {
> compatible = "cache";
> next-level-cache = <&L3_0>;
> @@ -159,6 +164,7 @@
> reg = <0x0 0x500>;
> enable-method = "psci";
> next-level-cache = <&L2_500>;
> + cpu-idle-states = <&C4_CPU_PD &C4_CPU_RPD &CLUSTER_PD>;
> L2_500: l2-cache {
> compatible = "cache";
> next-level-cache = <&L3_0>;
> @@ -171,6 +177,7 @@
> reg = <0x0 0x600>;
> enable-method = "psci";
> next-level-cache = <&L2_600>;
> + cpu-idle-states = <&C4_CPU_PD &C4_CPU_RPD &CLUSTER_PD>;
> L2_600: l2-cache {
> compatible = "cache";
> next-level-cache = <&L3_0>;
> @@ -183,11 +190,66 @@
> reg = <0x0 0x700>;
> enable-method = "psci";
> next-level-cache = <&L2_700>;
> + cpu-idle-states = <&C4_CPU_PD &C4_CPU_RPD &CLUSTER_PD>;
> L2_700: l2-cache {
> compatible = "cache";
> next-level-cache = <&L3_0>;
> };
> };
> +
> + idle-states {
> + entry-method = "psci";
> +
> + C0_CPU_PD: c0-power-down {
big_cpu_retention: big-cpu-retention
> + compatible = "arm,idle-state";
> + arm,psci-suspend-param = <0x40000003>;
> + entry-latency-us = <350>;
> + exit-latency-us = <461>;
> + min-residency-us = <1890>;
> + local-timer-stop;
> + idle-state-name = "power-down";
"big-cpu-power-down"
> + };
> +
> + C0_CPU_RPD: c0-rail-power-down {
big_cpu_sleep: big-cpu-sleep
> + compatible = "arm,idle-state";
> + arm,psci-suspend-param = <0x40000004>;
> + entry-latency-us = <360>;
> + exit-latency-us = <531>;
> + min-residency-us = <3934>;
> + local-timer-stop;
> + idle-state-name = "rail-power-down";
"big-cpu-rail-power-down"
> + };
> +
> + C4_CPU_PD: c4-power-down {
little_cpu_retention: little-cpu-retention
> + compatible = "arm,idle-state";
> + arm,psci-suspend-param = <0x40000003>;
> + entry-latency-us = <264>;
> + exit-latency-us = <621>;
> + min-residency-us = <952>;
> + local-timer-stop;
> + idle-state-name = "power-down";
"little-cpu-power-down" and so and so forth. You get the idea.
> + };
> +
> + C4_CPU_RPD: c4-rail-power-down {
> + compatible = "arm,idle-state";
> + arm,psci-suspend-param = <0x40000004>;
> + entry-latency-us = <702>;
> + exit-latency-us = <1061>;
> + min-residency-us = <4488>;
> + local-timer-stop;
> + idle-state-name = "rail-power-down";
> + };
> +
> + CLUSTER_PD: cluster-power-down {
> + compatible = "arm,idle-state";
> + arm,psci-suspend-param = <0x400000F4>;
> + entry-latency-us = <3263>;
> + exit-latency-us = <6562>;
> + min-residency-us = <9987>;
> + local-timer-stop;
> + idle-state-name = "cluster-power-down";
> + };
> + };
> };
>
> pmu {
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of the Code Aurora Forum, hosted by The Linux Foundation.
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2] arm64: dts: sdm845: Add PSCI cpuidle low power states
@ 2019-05-07 22:20 ` Amit Kucheria
0 siblings, 0 replies; 10+ messages in thread
From: Amit Kucheria @ 2019-05-07 22:20 UTC (permalink / raw)
To: Raju P.L.S.S.S.N
Cc: Andy Gross, David Brown, linux-arm-msm,
open list:ARM/QUALCOMM SUPPORT, Rajendra Nayak, Bjorn Andersson,
LKML, Linux PM list,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Stephen Boyd, evgreen, Douglas Anderson, Matthias Kaehlcke,
ilina
On Tue, Oct 30, 2018 at 10:53 PM Raju P.L.S.S.S.N
<rplsssn@codeaurora.org> wrote:
>
> Add device bindings for cpuidle states for cpu devices.
Raju: Did this patch fall through the cracks? It would be nice to land
this while Lina works on setting up the infrastructure to do
hierarchical power domains.
> Cc: <devicetree@vger.kernel.org>
> Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
> ---
> Changes in v2
> - Address comments from Doug
> ---
> ---
> arch/arm64/boot/dts/qcom/sdm845.dtsi | 62 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 62 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index 0c9a2aa..3a8381e 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -96,6 +96,7 @@
> reg = <0x0 0x0>;
> enable-method = "psci";
> next-level-cache = <&L2_0>;
> + cpu-idle-states = <&C0_CPU_PD &C0_CPU_RPD &CLUSTER_PD>;
May I suggest using more generic names here instead of C0_CPU_PD and
move the QC-specific description to the idle-state-name property? C0
and C4 isn't easy to understand at a glance. Neither is PD and RPD.
Something like big_cpu_retention, big_cpu_sleep, little_cpu_retention,
little_cpu_sleep, cluster_sleep?
Or big_cpu_idle_0, big_cpu_idle_1, big_cpu_idle_2 for states with
increasing breakeven times.
I've commented below on what it might look like.
> L2_0: l2-cache {
> compatible = "cache";
> next-level-cache = <&L3_0>;
> @@ -111,6 +112,7 @@
> reg = <0x0 0x100>;
> enable-method = "psci";
> next-level-cache = <&L2_100>;
> + cpu-idle-states = <&C0_CPU_PD &C0_CPU_RPD &CLUSTER_PD>;
> L2_100: l2-cache {
> compatible = "cache";
> next-level-cache = <&L3_0>;
> @@ -123,6 +125,7 @@
> reg = <0x0 0x200>;
> enable-method = "psci";
> next-level-cache = <&L2_200>;
> + cpu-idle-states = <&C0_CPU_PD &C0_CPU_RPD &CLUSTER_PD>;
> L2_200: l2-cache {
> compatible = "cache";
> next-level-cache = <&L3_0>;
> @@ -135,6 +138,7 @@
> reg = <0x0 0x300>;
> enable-method = "psci";
> next-level-cache = <&L2_300>;
> + cpu-idle-states = <&C0_CPU_PD &C0_CPU_RPD &CLUSTER_PD>;
> L2_300: l2-cache {
> compatible = "cache";
> next-level-cache = <&L3_0>;
> @@ -147,6 +151,7 @@
> reg = <0x0 0x400>;
> enable-method = "psci";
> next-level-cache = <&L2_400>;
> + cpu-idle-states = <&C4_CPU_PD &C4_CPU_RPD &CLUSTER_PD>;
> L2_400: l2-cache {
> compatible = "cache";
> next-level-cache = <&L3_0>;
> @@ -159,6 +164,7 @@
> reg = <0x0 0x500>;
> enable-method = "psci";
> next-level-cache = <&L2_500>;
> + cpu-idle-states = <&C4_CPU_PD &C4_CPU_RPD &CLUSTER_PD>;
> L2_500: l2-cache {
> compatible = "cache";
> next-level-cache = <&L3_0>;
> @@ -171,6 +177,7 @@
> reg = <0x0 0x600>;
> enable-method = "psci";
> next-level-cache = <&L2_600>;
> + cpu-idle-states = <&C4_CPU_PD &C4_CPU_RPD &CLUSTER_PD>;
> L2_600: l2-cache {
> compatible = "cache";
> next-level-cache = <&L3_0>;
> @@ -183,11 +190,66 @@
> reg = <0x0 0x700>;
> enable-method = "psci";
> next-level-cache = <&L2_700>;
> + cpu-idle-states = <&C4_CPU_PD &C4_CPU_RPD &CLUSTER_PD>;
> L2_700: l2-cache {
> compatible = "cache";
> next-level-cache = <&L3_0>;
> };
> };
> +
> + idle-states {
> + entry-method = "psci";
> +
> + C0_CPU_PD: c0-power-down {
big_cpu_retention: big-cpu-retention
> + compatible = "arm,idle-state";
> + arm,psci-suspend-param = <0x40000003>;
> + entry-latency-us = <350>;
> + exit-latency-us = <461>;
> + min-residency-us = <1890>;
> + local-timer-stop;
> + idle-state-name = "power-down";
"big-cpu-power-down"
> + };
> +
> + C0_CPU_RPD: c0-rail-power-down {
big_cpu_sleep: big-cpu-sleep
> + compatible = "arm,idle-state";
> + arm,psci-suspend-param = <0x40000004>;
> + entry-latency-us = <360>;
> + exit-latency-us = <531>;
> + min-residency-us = <3934>;
> + local-timer-stop;
> + idle-state-name = "rail-power-down";
"big-cpu-rail-power-down"
> + };
> +
> + C4_CPU_PD: c4-power-down {
little_cpu_retention: little-cpu-retention
> + compatible = "arm,idle-state";
> + arm,psci-suspend-param = <0x40000003>;
> + entry-latency-us = <264>;
> + exit-latency-us = <621>;
> + min-residency-us = <952>;
> + local-timer-stop;
> + idle-state-name = "power-down";
"little-cpu-power-down" and so and so forth. You get the idea.
> + };
> +
> + C4_CPU_RPD: c4-rail-power-down {
> + compatible = "arm,idle-state";
> + arm,psci-suspend-param = <0x40000004>;
> + entry-latency-us = <702>;
> + exit-latency-us = <1061>;
> + min-residency-us = <4488>;
> + local-timer-stop;
> + idle-state-name = "rail-power-down";
> + };
> +
> + CLUSTER_PD: cluster-power-down {
> + compatible = "arm,idle-state";
> + arm,psci-suspend-param = <0x400000F4>;
> + entry-latency-us = <3263>;
> + exit-latency-us = <6562>;
> + min-residency-us = <9987>;
> + local-timer-stop;
> + idle-state-name = "cluster-power-down";
> + };
> + };
> };
>
> pmu {
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of the Code Aurora Forum, hosted by The Linux Foundation.
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2] arm64: dts: sdm845: Add PSCI cpuidle low power states
2019-05-07 22:20 ` Amit Kucheria
@ 2019-05-10 11:32 ` Amit Kucheria
-1 siblings, 0 replies; 10+ messages in thread
From: Amit Kucheria @ 2019-05-10 11:32 UTC (permalink / raw)
To: Raju P.L.S.S.S.N, mkshah
Cc: Andy Gross, David Brown, linux-arm-msm,
open list:ARM/QUALCOMM SUPPORT, Rajendra Nayak, Bjorn Andersson,
LKML, Linux PM list,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Stephen Boyd, evgreen, Douglas Anderson, Matthias Kaehlcke,
ilina
On Wed, May 8, 2019 at 3:50 AM Amit Kucheria
<amit.kucheria@verdurent.com> wrote:
>
> On Tue, Oct 30, 2018 at 10:53 PM Raju P.L.S.S.S.N
> <rplsssn@codeaurora.org> wrote:
> >
> > Add device bindings for cpuidle states for cpu devices.
>
> Raju: Did this patch fall through the cracks? It would be nice to land
> this while Lina works on setting up the infrastructure to do
> hierarchical power domains.
After talking offline with Raju, I've added this patch with some
cleanups to my qcom cpuidle series.
> > Cc: <devicetree@vger.kernel.org>
> > Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
> > ---
> > Changes in v2
> > - Address comments from Doug
> > ---
> > ---
> > arch/arm64/boot/dts/qcom/sdm845.dtsi | 62 ++++++++++++++++++++++++++++++++++++
> > 1 file changed, 62 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > index 0c9a2aa..3a8381e 100644
> > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > @@ -96,6 +96,7 @@
> > reg = <0x0 0x0>;
> > enable-method = "psci";
> > next-level-cache = <&L2_0>;
> > + cpu-idle-states = <&C0_CPU_PD &C0_CPU_RPD &CLUSTER_PD>;
>
> May I suggest using more generic names here instead of C0_CPU_PD and
> move the QC-specific description to the idle-state-name property? C0
> and C4 isn't easy to understand at a glance. Neither is PD and RPD.
>
> Something like big_cpu_retention, big_cpu_sleep, little_cpu_retention,
> little_cpu_sleep, cluster_sleep?
> Or big_cpu_idle_0, big_cpu_idle_1, big_cpu_idle_2 for states with
> increasing breakeven times.
>
> I've commented below on what it might look like.
>
> > L2_0: l2-cache {
> > compatible = "cache";
> > next-level-cache = <&L3_0>;
> > @@ -111,6 +112,7 @@
> > reg = <0x0 0x100>;
> > enable-method = "psci";
> > next-level-cache = <&L2_100>;
> > + cpu-idle-states = <&C0_CPU_PD &C0_CPU_RPD &CLUSTER_PD>;
> > L2_100: l2-cache {
> > compatible = "cache";
> > next-level-cache = <&L3_0>;
> > @@ -123,6 +125,7 @@
> > reg = <0x0 0x200>;
> > enable-method = "psci";
> > next-level-cache = <&L2_200>;
> > + cpu-idle-states = <&C0_CPU_PD &C0_CPU_RPD &CLUSTER_PD>;
> > L2_200: l2-cache {
> > compatible = "cache";
> > next-level-cache = <&L3_0>;
> > @@ -135,6 +138,7 @@
> > reg = <0x0 0x300>;
> > enable-method = "psci";
> > next-level-cache = <&L2_300>;
> > + cpu-idle-states = <&C0_CPU_PD &C0_CPU_RPD &CLUSTER_PD>;
> > L2_300: l2-cache {
> > compatible = "cache";
> > next-level-cache = <&L3_0>;
> > @@ -147,6 +151,7 @@
> > reg = <0x0 0x400>;
> > enable-method = "psci";
> > next-level-cache = <&L2_400>;
> > + cpu-idle-states = <&C4_CPU_PD &C4_CPU_RPD &CLUSTER_PD>;
> > L2_400: l2-cache {
> > compatible = "cache";
> > next-level-cache = <&L3_0>;
> > @@ -159,6 +164,7 @@
> > reg = <0x0 0x500>;
> > enable-method = "psci";
> > next-level-cache = <&L2_500>;
> > + cpu-idle-states = <&C4_CPU_PD &C4_CPU_RPD &CLUSTER_PD>;
> > L2_500: l2-cache {
> > compatible = "cache";
> > next-level-cache = <&L3_0>;
> > @@ -171,6 +177,7 @@
> > reg = <0x0 0x600>;
> > enable-method = "psci";
> > next-level-cache = <&L2_600>;
> > + cpu-idle-states = <&C4_CPU_PD &C4_CPU_RPD &CLUSTER_PD>;
> > L2_600: l2-cache {
> > compatible = "cache";
> > next-level-cache = <&L3_0>;
> > @@ -183,11 +190,66 @@
> > reg = <0x0 0x700>;
> > enable-method = "psci";
> > next-level-cache = <&L2_700>;
> > + cpu-idle-states = <&C4_CPU_PD &C4_CPU_RPD &CLUSTER_PD>;
> > L2_700: l2-cache {
> > compatible = "cache";
> > next-level-cache = <&L3_0>;
> > };
> > };
> > +
> > + idle-states {
> > + entry-method = "psci";
> > +
> > + C0_CPU_PD: c0-power-down {
>
> big_cpu_retention: big-cpu-retention
>
> > + compatible = "arm,idle-state";
> > + arm,psci-suspend-param = <0x40000003>;
> > + entry-latency-us = <350>;
> > + exit-latency-us = <461>;
> > + min-residency-us = <1890>;
> > + local-timer-stop;
> > + idle-state-name = "power-down";
>
> "big-cpu-power-down"
>
> > + };
> > +
> > + C0_CPU_RPD: c0-rail-power-down {
>
> big_cpu_sleep: big-cpu-sleep
>
> > + compatible = "arm,idle-state";
> > + arm,psci-suspend-param = <0x40000004>;
> > + entry-latency-us = <360>;
> > + exit-latency-us = <531>;
> > + min-residency-us = <3934>;
> > + local-timer-stop;
> > + idle-state-name = "rail-power-down";
>
> "big-cpu-rail-power-down"
>
> > + };
> > +
> > + C4_CPU_PD: c4-power-down {
>
> little_cpu_retention: little-cpu-retention
>
> > + compatible = "arm,idle-state";
> > + arm,psci-suspend-param = <0x40000003>;
> > + entry-latency-us = <264>;
> > + exit-latency-us = <621>;
> > + min-residency-us = <952>;
> > + local-timer-stop;
> > + idle-state-name = "power-down";
>
> "little-cpu-power-down" and so and so forth. You get the idea.
>
> > + };
> > +
> > + C4_CPU_RPD: c4-rail-power-down {
> > + compatible = "arm,idle-state";
> > + arm,psci-suspend-param = <0x40000004>;
> > + entry-latency-us = <702>;
> > + exit-latency-us = <1061>;
> > + min-residency-us = <4488>;
> > + local-timer-stop;
> > + idle-state-name = "rail-power-down";
> > + };
> > +
> > + CLUSTER_PD: cluster-power-down {
> > + compatible = "arm,idle-state";
> > + arm,psci-suspend-param = <0x400000F4>;
> > + entry-latency-us = <3263>;
> > + exit-latency-us = <6562>;
> > + min-residency-us = <9987>;
> > + local-timer-stop;
> > + idle-state-name = "cluster-power-down";
> > + };
> > + };
> > };
> >
> > pmu {
> > --
> > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> > of the Code Aurora Forum, hosted by The Linux Foundation.
> >
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2] arm64: dts: sdm845: Add PSCI cpuidle low power states
@ 2019-05-10 11:32 ` Amit Kucheria
0 siblings, 0 replies; 10+ messages in thread
From: Amit Kucheria @ 2019-05-10 11:32 UTC (permalink / raw)
To: Raju P.L.S.S.S.N, mkshah
Cc: Andy Gross, David Brown, linux-arm-msm,
open list:ARM/QUALCOMM SUPPORT, Rajendra Nayak, Bjorn Andersson,
LKML, Linux PM list,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Stephen Boyd, evgreen, Douglas Anderson, Matthias Kaehlcke,
ilina
On Wed, May 8, 2019 at 3:50 AM Amit Kucheria
<amit.kucheria@verdurent.com> wrote:
>
> On Tue, Oct 30, 2018 at 10:53 PM Raju P.L.S.S.S.N
> <rplsssn@codeaurora.org> wrote:
> >
> > Add device bindings for cpuidle states for cpu devices.
>
> Raju: Did this patch fall through the cracks? It would be nice to land
> this while Lina works on setting up the infrastructure to do
> hierarchical power domains.
After talking offline with Raju, I've added this patch with some
cleanups to my qcom cpuidle series.
> > Cc: <devicetree@vger.kernel.org>
> > Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
> > ---
> > Changes in v2
> > - Address comments from Doug
> > ---
> > ---
> > arch/arm64/boot/dts/qcom/sdm845.dtsi | 62 ++++++++++++++++++++++++++++++++++++
> > 1 file changed, 62 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > index 0c9a2aa..3a8381e 100644
> > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > @@ -96,6 +96,7 @@
> > reg = <0x0 0x0>;
> > enable-method = "psci";
> > next-level-cache = <&L2_0>;
> > + cpu-idle-states = <&C0_CPU_PD &C0_CPU_RPD &CLUSTER_PD>;
>
> May I suggest using more generic names here instead of C0_CPU_PD and
> move the QC-specific description to the idle-state-name property? C0
> and C4 isn't easy to understand at a glance. Neither is PD and RPD.
>
> Something like big_cpu_retention, big_cpu_sleep, little_cpu_retention,
> little_cpu_sleep, cluster_sleep?
> Or big_cpu_idle_0, big_cpu_idle_1, big_cpu_idle_2 for states with
> increasing breakeven times.
>
> I've commented below on what it might look like.
>
> > L2_0: l2-cache {
> > compatible = "cache";
> > next-level-cache = <&L3_0>;
> > @@ -111,6 +112,7 @@
> > reg = <0x0 0x100>;
> > enable-method = "psci";
> > next-level-cache = <&L2_100>;
> > + cpu-idle-states = <&C0_CPU_PD &C0_CPU_RPD &CLUSTER_PD>;
> > L2_100: l2-cache {
> > compatible = "cache";
> > next-level-cache = <&L3_0>;
> > @@ -123,6 +125,7 @@
> > reg = <0x0 0x200>;
> > enable-method = "psci";
> > next-level-cache = <&L2_200>;
> > + cpu-idle-states = <&C0_CPU_PD &C0_CPU_RPD &CLUSTER_PD>;
> > L2_200: l2-cache {
> > compatible = "cache";
> > next-level-cache = <&L3_0>;
> > @@ -135,6 +138,7 @@
> > reg = <0x0 0x300>;
> > enable-method = "psci";
> > next-level-cache = <&L2_300>;
> > + cpu-idle-states = <&C0_CPU_PD &C0_CPU_RPD &CLUSTER_PD>;
> > L2_300: l2-cache {
> > compatible = "cache";
> > next-level-cache = <&L3_0>;
> > @@ -147,6 +151,7 @@
> > reg = <0x0 0x400>;
> > enable-method = "psci";
> > next-level-cache = <&L2_400>;
> > + cpu-idle-states = <&C4_CPU_PD &C4_CPU_RPD &CLUSTER_PD>;
> > L2_400: l2-cache {
> > compatible = "cache";
> > next-level-cache = <&L3_0>;
> > @@ -159,6 +164,7 @@
> > reg = <0x0 0x500>;
> > enable-method = "psci";
> > next-level-cache = <&L2_500>;
> > + cpu-idle-states = <&C4_CPU_PD &C4_CPU_RPD &CLUSTER_PD>;
> > L2_500: l2-cache {
> > compatible = "cache";
> > next-level-cache = <&L3_0>;
> > @@ -171,6 +177,7 @@
> > reg = <0x0 0x600>;
> > enable-method = "psci";
> > next-level-cache = <&L2_600>;
> > + cpu-idle-states = <&C4_CPU_PD &C4_CPU_RPD &CLUSTER_PD>;
> > L2_600: l2-cache {
> > compatible = "cache";
> > next-level-cache = <&L3_0>;
> > @@ -183,11 +190,66 @@
> > reg = <0x0 0x700>;
> > enable-method = "psci";
> > next-level-cache = <&L2_700>;
> > + cpu-idle-states = <&C4_CPU_PD &C4_CPU_RPD &CLUSTER_PD>;
> > L2_700: l2-cache {
> > compatible = "cache";
> > next-level-cache = <&L3_0>;
> > };
> > };
> > +
> > + idle-states {
> > + entry-method = "psci";
> > +
> > + C0_CPU_PD: c0-power-down {
>
> big_cpu_retention: big-cpu-retention
>
> > + compatible = "arm,idle-state";
> > + arm,psci-suspend-param = <0x40000003>;
> > + entry-latency-us = <350>;
> > + exit-latency-us = <461>;
> > + min-residency-us = <1890>;
> > + local-timer-stop;
> > + idle-state-name = "power-down";
>
> "big-cpu-power-down"
>
> > + };
> > +
> > + C0_CPU_RPD: c0-rail-power-down {
>
> big_cpu_sleep: big-cpu-sleep
>
> > + compatible = "arm,idle-state";
> > + arm,psci-suspend-param = <0x40000004>;
> > + entry-latency-us = <360>;
> > + exit-latency-us = <531>;
> > + min-residency-us = <3934>;
> > + local-timer-stop;
> > + idle-state-name = "rail-power-down";
>
> "big-cpu-rail-power-down"
>
> > + };
> > +
> > + C4_CPU_PD: c4-power-down {
>
> little_cpu_retention: little-cpu-retention
>
> > + compatible = "arm,idle-state";
> > + arm,psci-suspend-param = <0x40000003>;
> > + entry-latency-us = <264>;
> > + exit-latency-us = <621>;
> > + min-residency-us = <952>;
> > + local-timer-stop;
> > + idle-state-name = "power-down";
>
> "little-cpu-power-down" and so and so forth. You get the idea.
>
> > + };
> > +
> > + C4_CPU_RPD: c4-rail-power-down {
> > + compatible = "arm,idle-state";
> > + arm,psci-suspend-param = <0x40000004>;
> > + entry-latency-us = <702>;
> > + exit-latency-us = <1061>;
> > + min-residency-us = <4488>;
> > + local-timer-stop;
> > + idle-state-name = "rail-power-down";
> > + };
> > +
> > + CLUSTER_PD: cluster-power-down {
> > + compatible = "arm,idle-state";
> > + arm,psci-suspend-param = <0x400000F4>;
> > + entry-latency-us = <3263>;
> > + exit-latency-us = <6562>;
> > + min-residency-us = <9987>;
> > + local-timer-stop;
> > + idle-state-name = "cluster-power-down";
> > + };
> > + };
> > };
> >
> > pmu {
> > --
> > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> > of the Code Aurora Forum, hosted by The Linux Foundation.
> >
^ permalink raw reply [flat|nested] 10+ messages in thread