2018-10-15 17:03:21

by Raju P.L.S.S.S.N

[permalink] [raw]
Subject: [PATCH] arm64: dts: sdm845: Add PSCI cpuidle low power states

Add device bindings for cpuidle states for cpu devices.

Cc: <[email protected]>
Signed-off-by: Raju P.L.S.S.S.N <[email protected]>
---
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..32262b0 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_SPC &C0_CPU_PC &CLUSTER_PC>;
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_SPC &C0_CPU_PC &CLUSTER_PC>;
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_SPC &C0_CPU_PC &CLUSTER_PC>;
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_SPC &C0_CPU_PC &CLUSTER_PC>;
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_SPC &C4_CPU_PC &CLUSTER_PC>;
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_SPC &C4_CPU_PC &CLUSTER_PC>;
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_SPC &C4_CPU_PC &CLUSTER_PC>;
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_SPC &C4_CPU_PC &CLUSTER_PC>;
L2_700: l2-cache {
compatible = "cache";
next-level-cache = <&L3_0>;
};
};
+
+ idle-states {
+ entry-method = "psci";
+
+ C0_CPU_SPC: c0_spc {
+ 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 = "pc";
+ };
+
+ C0_CPU_PC: c0_pc {
+ 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 pc";
+ };
+
+ C4_CPU_SPC: c4_spc {
+ 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 = "pc";
+ };
+
+ C4_CPU_PC: c4_pc {
+ 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 pc";
+ };
+
+ CLUSTER_PC: cluster_pc {
+ 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 pc";
+ };
+ };
};

pmu {
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of the Code Aurora Forum, hosted by The Linux Foundation.



2018-10-19 19:48:48

by Lina Iyer

[permalink] [raw]
Subject: Re: [PATCH] arm64: dts: sdm845: Add PSCI cpuidle low power states

On Mon, Oct 15 2018 at 11:02 -0600, Raju P.L.S.S.S.N wrote:
>Add device bindings for cpuidle states for cpu devices.
>
>Cc: <[email protected]>
>Signed-off-by: Raju P.L.S.S.S.N <[email protected]>
>---
Reviewed-by: Lina Iyer <[email protected]>

> 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..32262b0 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_SPC &C0_CPU_PC &CLUSTER_PC>;
> 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_SPC &C0_CPU_PC &CLUSTER_PC>;
> 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_SPC &C0_CPU_PC &CLUSTER_PC>;
> 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_SPC &C0_CPU_PC &CLUSTER_PC>;
> 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_SPC &C4_CPU_PC &CLUSTER_PC>;
> 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_SPC &C4_CPU_PC &CLUSTER_PC>;
> 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_SPC &C4_CPU_PC &CLUSTER_PC>;
> 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_SPC &C4_CPU_PC &CLUSTER_PC>;
> L2_700: l2-cache {
> compatible = "cache";
> next-level-cache = <&L3_0>;
> };
> };
>+
>+ idle-states {
>+ entry-method = "psci";
>+
>+ C0_CPU_SPC: c0_spc {
>+ 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 = "pc";
>+ };
>+
>+ C0_CPU_PC: c0_pc {
>+ 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 pc";
>+ };
>+
>+ C4_CPU_SPC: c4_spc {
>+ 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 = "pc";
>+ };
>+
>+ C4_CPU_PC: c4_pc {
>+ 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 pc";
>+ };
>+
>+ CLUSTER_PC: cluster_pc {
>+ 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 pc";
>+ };
>+ };
> };
>
> pmu {
>--
>QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
>of the Code Aurora Forum, hosted by The Linux Foundation.
>

2018-10-22 19:19:28

by Evan Green

[permalink] [raw]
Subject: Re: [PATCH] arm64: dts: sdm845: Add PSCI cpuidle low power states

On Mon, Oct 15, 2018 at 10:02 AM Raju P.L.S.S.S.N
<[email protected]> wrote:
>
> Add device bindings for cpuidle states for cpu devices.
>
> Cc: <[email protected]>
> Signed-off-by: Raju P.L.S.S.S.N <[email protected]>
> ---
> arch/arm64/boot/dts/qcom/sdm845.dtsi | 62 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 62 insertions(+)
>

Reviewed-by: Evan Green <[email protected]>

2018-10-23 18:38:19

by Doug Anderson

[permalink] [raw]
Subject: Re: [PATCH] arm64: dts: sdm845: Add PSCI cpuidle low power states

Hi,

On Mon, Oct 15, 2018 at 10:02 AM Raju P.L.S.S.S.N
<[email protected]> wrote:
> + idle-states {
> + entry-method = "psci";
> +
> + C0_CPU_SPC: c0_spc {

nit: all these nodes should have dashes instead of spaces in the node
names (labels can still have spaces). AKA:

C0_CPU_SPC: c0-spc {


> + 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 = "pc";

It seems weird that the idle state with the node name "spc" has the
name "pc" and the idle state with the node name "pc" has the name
"rail pc". Can this be more consistent or is there a reason why they
need to be mismatched?

Also: AAHTUFSWDKWTM (acronyms are hard to understand for someone who
doesn't know what they mean). If you really need to use an the
acronyms "PC" and "SPC" please document them somewhere. In the very
least the commit message, but having a comment in the file is good
too. ...or (even better) don't use the acronym and spell out what
you're talking about. Please correct me if I'm wrong, but I don't
think it's obvious what the "PC" and "SPC" idle states mean.


-Doug

2018-10-30 16:54:25

by Doug Anderson

[permalink] [raw]
Subject: Re: [PATCH] arm64: dts: sdm845: Add PSCI cpuidle low power states

Resending since I accidentally clicked back to HTML and lists all bounced. :(

On Tue, Oct 30, 2018 at 9:24 AM Raju P L S S S N <[email protected]> wrote:
>
> On 10/24/2018 12:06 AM, Doug Anderson wrote:
> > Hi,
> >
> > On Mon, Oct 15, 2018 at 10:02 AM Raju P.L.S.S.S.N
> > <[email protected]> wrote:
> >> + idle-states {
> >> + entry-method = "psci";
> >> +
> >> + C0_CPU_SPC: c0_spc {
> >
> > nit: all these nodes should have dashes instead of spaces in the node
> > names (labels can still have spaces). AKA:
> >
> > C0_CPU_SPC: c0-spc {
>
> Sure. I will change this. (However, from device tree specification v0.2,
> I see that Table 2.1 mentions underscore as valid character for node.
> Please correct me if I'm missing something)

You're right that I don't see it there. I've always heard that they
are allowed but discouraged. One place that mentions this:

https://elinux.org/Device_Tree_Linux

> Linux conventions
> * node names
> * use dash "-" instead of underscore "_"

That same site points that the majority of people use dashes in node
names instead of underscores.

-Doug

2018-10-30 18:24:50

by Raju P.L.S.S.S.N

[permalink] [raw]
Subject: Re: [PATCH] arm64: dts: sdm845: Add PSCI cpuidle low power states



On 10/24/2018 12:06 AM, Doug Anderson wrote:
> Hi,
>
> On Mon, Oct 15, 2018 at 10:02 AM Raju P.L.S.S.S.N
> <[email protected]> wrote:
>> + idle-states {
>> + entry-method = "psci";
>> +
>> + C0_CPU_SPC: c0_spc {
>
> nit: all these nodes should have dashes instead of spaces in the node
> names (labels can still have spaces). AKA:
>
> C0_CPU_SPC: c0-spc {

Sure. I will change this. (However, from device tree specification v0.2,
I see that Table 2.1 mentions underscore as valid character for node.
Please correct me if I'm missing something)

>
>
>> + 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 = "pc";
>
> It seems weird that the idle state with the node name "spc" has the
> name "pc" and the idle state with the node name "pc" has the name
> "rail pc". Can this be more consistent or is there a reason why they
> need to be mismatched?

Agree. They are confusing. Will change this.

>
> Also: AAHTUFSWDKWTM (acronyms are hard to understand for someone who
> doesn't know what they mean). If you really need to use an the
> acronyms "PC" and "SPC" please document them somewhere. In the very
> least the commit message, but having a comment in the file is good
> too. ...or (even better) don't use the acronym and spell out what
> you're talking about. Please correct me if I'm wrong, but I don't
> think it's obvious what the "PC" and "SPC" idle states mean.

Sure. Will change this.


Thanks for feedback Doug.

>
>
> -Doug
>