diff mbox series

[RESEND,v2,1/1] arm64: dts: qcom: sm6115: Add CPU idle-states

Message ID 20230330193303.612475-1-bhupesh.sharma@linaro.org
State New
Headers
Series [RESEND,v2,1/1] arm64: dts: qcom: sm6115: Add CPU idle-states |

Commit Message

Bhupesh Sharma March 30, 2023, 7:33 p.m. UTC 
  Add CPU idle-state nodes and power-domains in Qualcomm sm6115 SoC dtsi.

Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
---
Changes since v1:
- v1 can be viewed here: https://lore.kernel.org/lkml/e5cda4cf-5c2a-a7ed-9e1d-1fe9f2cbef40@linaro.org
- Addressed Konrad's comments on v1 and added GDHS and Power Collapse
  cluster power states.

 arch/arm64/boot/dts/qcom/sm6115.dtsi | 136 +++++++++++++++++++++++++++
 1 file changed, 136 insertions(+)

Comments

Konrad Dybcio April 1, 2023, 12:21 p.m. UTC | #1 
On 30.03.2023 21:33, Bhupesh Sharma wrote:
> Add CPU idle-state nodes and power-domains in Qualcomm sm6115 SoC dtsi.
> 
> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
> ---
> Changes since v1:
> - v1 can be viewed here: https://lore.kernel.org/lkml/e5cda4cf-5c2a-a7ed-9e1d-1fe9f2cbef40@linaro.org
> - Addressed Konrad's comments on v1 and added GDHS and Power Collapse
>   cluster power states.
> 
>  arch/arm64/boot/dts/qcom/sm6115.dtsi | 136 +++++++++++++++++++++++++++
>  1 file changed, 136 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> index 2a51c938bbcb..b63395d476ed 100644
> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> @@ -45,6 +45,8 @@ CPU0: cpu@0 {
>  			enable-method = "psci";
>  			next-level-cache = <&L2_0>;
>  			qcom,freq-domain = <&cpufreq_hw 0>;
> +			power-domains = <&CPU_PD0>;
> +			power-domain-names = "psci";
>  			L2_0: l2-cache {
>  				compatible = "cache";
>  				cache-level = <2>;
> @@ -61,6 +63,8 @@ CPU1: cpu@1 {
>  			enable-method = "psci";
>  			next-level-cache = <&L2_0>;
>  			qcom,freq-domain = <&cpufreq_hw 0>;
> +			power-domains = <&CPU_PD1>;
> +			power-domain-names = "psci";
>  		};
>  
>  		CPU2: cpu@2 {
> @@ -73,6 +77,8 @@ CPU2: cpu@2 {
>  			enable-method = "psci";
>  			next-level-cache = <&L2_0>;
>  			qcom,freq-domain = <&cpufreq_hw 0>;
> +			power-domains = <&CPU_PD2>;
> +			power-domain-names = "psci";
>  		};
>  
>  		CPU3: cpu@3 {
> @@ -85,6 +91,8 @@ CPU3: cpu@3 {
>  			enable-method = "psci";
>  			next-level-cache = <&L2_0>;
>  			qcom,freq-domain = <&cpufreq_hw 0>;
> +			power-domains = <&CPU_PD3>;
> +			power-domain-names = "psci";
>  		};
>  
>  		CPU4: cpu@100 {
> @@ -97,6 +105,8 @@ CPU4: cpu@100 {
>  			dynamic-power-coefficient = <282>;
>  			next-level-cache = <&L2_1>;
>  			qcom,freq-domain = <&cpufreq_hw 1>;
> +			power-domains = <&CPU_PD4>;
> +			power-domain-names = "psci";
>  			L2_1: l2-cache {
>  				compatible = "cache";
>  				cache-level = <2>;
> @@ -113,6 +123,8 @@ CPU5: cpu@101 {
>  			enable-method = "psci";
>  			next-level-cache = <&L2_1>;
>  			qcom,freq-domain = <&cpufreq_hw 1>;
> +			power-domains = <&CPU_PD5>;
> +			power-domain-names = "psci";
>  		};
>  
>  		CPU6: cpu@102 {
> @@ -125,6 +137,8 @@ CPU6: cpu@102 {
>  			enable-method = "psci";
>  			next-level-cache = <&L2_1>;
>  			qcom,freq-domain = <&cpufreq_hw 1>;
> +			power-domains = <&CPU_PD6>;
> +			power-domain-names = "psci";
>  		};
>  
>  		CPU7: cpu@103 {
> @@ -137,6 +151,8 @@ CPU7: cpu@103 {
>  			enable-method = "psci";
>  			next-level-cache = <&L2_1>;
>  			qcom,freq-domain = <&cpufreq_hw 1>;
> +			power-domains = <&CPU_PD7>;
> +			power-domain-names = "psci";
>  		};
>  
>  		cpu-map {
> @@ -176,6 +192,68 @@ core3 {
>  				};
>  			};
>  		};
> +
> +		idle-states {
> +			entry-method = "psci";
> +
> +			LITTLE_CPU_SLEEP_0: cpu-sleep-0-0 {
> +				compatible = "arm,idle-state";
> +				idle-state-name = "silver-rail-power-collapse";
> +				arm,psci-suspend-param = <0x40000003>;
> +				entry-latency-us = <290>;
> +				exit-latency-us = <376>;
> +				min-residency-us = <1182>;
> +				local-timer-stop;
> +			};
> +
> +			BIG_CPU_SLEEP_0: cpu-sleep-1-0 {
> +				compatible = "arm,idle-state";
> +				idle-state-name = "gold-rail-power-collapse";
> +				arm,psci-suspend-param = <0x40000003>;
> +				entry-latency-us = <297>;
> +				exit-latency-us = <324>;
> +				min-residency-us = <1110>;
> +				local-timer-stop;
> +			};
> +		};
> +
> +		domain-idle-states {
> +			CLUSTER_0_SLEEP_0: cluster-sleep-0-0 {
> +				/* GDHS */
> +				compatible = "domain-idle-state";
> +				arm,psci-suspend-param = <0x40000022>;
This 0x22 ending seems very sus.

The last nibble represents the core-level power state and the
penultimate one represents the same at cluster level. A value
of 2 in that cluster nibble is actually undefined by the PSCI spec,
whereas the value of 4 (as you have in all of the other idle
states, including D3G for the perf cluster) corresponds to
"Retention", so unless there's a very weird nuance in the
TZ for this SoC, it should probably end in 0x42.

Otherwise I think this LGTM now!

Konrad
> +				entry-latency-us = <360>;
> +				exit-latency-us = <421>;
> +				min-residency-us = <782>;
> +			};
> +
> +			CLUSTER_0_SLEEP_1: cluster-sleep-0-1 {
> +				/* Power Collapse */
> +				compatible = "domain-idle-state";
> +				arm,psci-suspend-param = <0x41000044>;
> +				entry-latency-us = <800>;
> +				exit-latency-us = <2118>;
> +				min-residency-us = <7376>;
> +			};
> +
> +			CLUSTER_1_SLEEP_0: cluster-sleep-1-0 {
> +				/* GDHS */
> +				compatible = "domain-idle-state";
> +				arm,psci-suspend-param = <0x40000042>;
> +				entry-latency-us = <314>;
> +				exit-latency-us = <345>;
> +				min-residency-us = <660>;
> +			};
> +
> +			CLUSTER_1_SLEEP_1: cluster-sleep-1-1 {
> +				/* Power Collapse */
> +				compatible = "domain-idle-state";
> +				arm,psci-suspend-param = <0x41000044>;
> +				entry-latency-us = <640>;
> +				exit-latency-us = <1654>;
> +				min-residency-us = <8094>;
> +			};
> +		};
>  	};
>  
>  	firmware {
> @@ -199,6 +277,64 @@ pmu {
>  	psci {
>  		compatible = "arm,psci-1.0";
>  		method = "smc";
> +
> +		CPU_PD0: power-domain-cpu0 {
> +			#power-domain-cells = <0>;
> +			power-domains = <&CLUSTER_0_PD>;
> +			domain-idle-states = <&LITTLE_CPU_SLEEP_0>;
> +		};
> +
> +		CPU_PD1: power-domain-cpu1 {
> +			#power-domain-cells = <0>;
> +			power-domains = <&CLUSTER_0_PD>;
> +			domain-idle-states = <&LITTLE_CPU_SLEEP_0>;
> +		};
> +
> +		CPU_PD2: power-domain-cpu2 {
> +			#power-domain-cells = <0>;
> +			power-domains = <&CLUSTER_0_PD>;
> +			domain-idle-states = <&LITTLE_CPU_SLEEP_0>;
> +		};
> +
> +		CPU_PD3: power-domain-cpu3 {
> +			#power-domain-cells = <0>;
> +			power-domains = <&CLUSTER_0_PD>;
> +			domain-idle-states = <&LITTLE_CPU_SLEEP_0>;
> +		};
> +
> +		CPU_PD4: power-domain-cpu4 {
> +			#power-domain-cells = <0>;
> +			power-domains = <&CLUSTER_1_PD>;
> +			domain-idle-states = <&BIG_CPU_SLEEP_0>;
> +		};
> +
> +		CPU_PD5: power-domain-cpu5 {
> +			#power-domain-cells = <0>;
> +			power-domains = <&CLUSTER_1_PD>;
> +			domain-idle-states = <&BIG_CPU_SLEEP_0>;
> +		};
> +
> +		CPU_PD6: power-domain-cpu6 {
> +			#power-domain-cells = <0>;
> +			power-domains = <&CLUSTER_1_PD>;
> +			domain-idle-states = <&BIG_CPU_SLEEP_0>;
> +		};
> +
> +		CPU_PD7: power-domain-cpu7 {
> +			#power-domain-cells = <0>;
> +			power-domains = <&CLUSTER_1_PD>;
> +			domain-idle-states = <&BIG_CPU_SLEEP_0>;
> +		};
> +
> +		CLUSTER_0_PD: power-domain-cpu-cluster0 {
> +			#power-domain-cells = <0>;
> +			domain-idle-states = <&CLUSTER_0_SLEEP_0>, <&CLUSTER_0_SLEEP_1>;
> +		};
> +
> +		CLUSTER_1_PD: power-domain-cpu-cluster1 {
> +			#power-domain-cells = <0>;
> +			domain-idle-states = <&CLUSTER_1_SLEEP_0>, <&CLUSTER_1_SLEEP_1>;
> +		};
>  	};
>  
>  	reserved_memory: reserved-memory {
Bhupesh Sharma April 1, 2023, 6:26 p.m. UTC | #2 
Hi Konrad,

On Sat, 1 Apr 2023 at 17:51, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
>
>
> On 30.03.2023 21:33, Bhupesh Sharma wrote:
> > Add CPU idle-state nodes and power-domains in Qualcomm sm6115 SoC dtsi.
> >
> > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
> > ---
> > Changes since v1:
> > - v1 can be viewed here: https://lore.kernel.org/lkml/e5cda4cf-5c2a-a7ed-9e1d-1fe9f2cbef40@linaro.org
> > - Addressed Konrad's comments on v1 and added GDHS and Power Collapse
> >   cluster power states.
> >
> >  arch/arm64/boot/dts/qcom/sm6115.dtsi | 136 +++++++++++++++++++++++++++
> >  1 file changed, 136 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> > index 2a51c938bbcb..b63395d476ed 100644
> > --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> > @@ -45,6 +45,8 @@ CPU0: cpu@0 {
> >                       enable-method = "psci";
> >                       next-level-cache = <&L2_0>;
> >                       qcom,freq-domain = <&cpufreq_hw 0>;
> > +                     power-domains = <&CPU_PD0>;
> > +                     power-domain-names = "psci";
> >                       L2_0: l2-cache {
> >                               compatible = "cache";
> >                               cache-level = <2>;
> > @@ -61,6 +63,8 @@ CPU1: cpu@1 {
> >                       enable-method = "psci";
> >                       next-level-cache = <&L2_0>;
> >                       qcom,freq-domain = <&cpufreq_hw 0>;
> > +                     power-domains = <&CPU_PD1>;
> > +                     power-domain-names = "psci";
> >               };
> >
> >               CPU2: cpu@2 {
> > @@ -73,6 +77,8 @@ CPU2: cpu@2 {
> >                       enable-method = "psci";
> >                       next-level-cache = <&L2_0>;
> >                       qcom,freq-domain = <&cpufreq_hw 0>;
> > +                     power-domains = <&CPU_PD2>;
> > +                     power-domain-names = "psci";
> >               };
> >
> >               CPU3: cpu@3 {
> > @@ -85,6 +91,8 @@ CPU3: cpu@3 {
> >                       enable-method = "psci";
> >                       next-level-cache = <&L2_0>;
> >                       qcom,freq-domain = <&cpufreq_hw 0>;
> > +                     power-domains = <&CPU_PD3>;
> > +                     power-domain-names = "psci";
> >               };
> >
> >               CPU4: cpu@100 {
> > @@ -97,6 +105,8 @@ CPU4: cpu@100 {
> >                       dynamic-power-coefficient = <282>;
> >                       next-level-cache = <&L2_1>;
> >                       qcom,freq-domain = <&cpufreq_hw 1>;
> > +                     power-domains = <&CPU_PD4>;
> > +                     power-domain-names = "psci";
> >                       L2_1: l2-cache {
> >                               compatible = "cache";
> >                               cache-level = <2>;
> > @@ -113,6 +123,8 @@ CPU5: cpu@101 {
> >                       enable-method = "psci";
> >                       next-level-cache = <&L2_1>;
> >                       qcom,freq-domain = <&cpufreq_hw 1>;
> > +                     power-domains = <&CPU_PD5>;
> > +                     power-domain-names = "psci";
> >               };
> >
> >               CPU6: cpu@102 {
> > @@ -125,6 +137,8 @@ CPU6: cpu@102 {
> >                       enable-method = "psci";
> >                       next-level-cache = <&L2_1>;
> >                       qcom,freq-domain = <&cpufreq_hw 1>;
> > +                     power-domains = <&CPU_PD6>;
> > +                     power-domain-names = "psci";
> >               };
> >
> >               CPU7: cpu@103 {
> > @@ -137,6 +151,8 @@ CPU7: cpu@103 {
> >                       enable-method = "psci";
> >                       next-level-cache = <&L2_1>;
> >                       qcom,freq-domain = <&cpufreq_hw 1>;
> > +                     power-domains = <&CPU_PD7>;
> > +                     power-domain-names = "psci";
> >               };
> >
> >               cpu-map {
> > @@ -176,6 +192,68 @@ core3 {
> >                               };
> >                       };
> >               };
> > +
> > +             idle-states {
> > +                     entry-method = "psci";
> > +
> > +                     LITTLE_CPU_SLEEP_0: cpu-sleep-0-0 {
> > +                             compatible = "arm,idle-state";
> > +                             idle-state-name = "silver-rail-power-collapse";
> > +                             arm,psci-suspend-param = <0x40000003>;
> > +                             entry-latency-us = <290>;
> > +                             exit-latency-us = <376>;
> > +                             min-residency-us = <1182>;
> > +                             local-timer-stop;
> > +                     };
> > +
> > +                     BIG_CPU_SLEEP_0: cpu-sleep-1-0 {
> > +                             compatible = "arm,idle-state";
> > +                             idle-state-name = "gold-rail-power-collapse";
> > +                             arm,psci-suspend-param = <0x40000003>;
> > +                             entry-latency-us = <297>;
> > +                             exit-latency-us = <324>;
> > +                             min-residency-us = <1110>;
> > +                             local-timer-stop;
> > +                     };
> > +             };
> > +
> > +             domain-idle-states {
> > +                     CLUSTER_0_SLEEP_0: cluster-sleep-0-0 {
> > +                             /* GDHS */
> > +                             compatible = "domain-idle-state";
> > +                             arm,psci-suspend-param = <0x40000022>;
> This 0x22 ending seems very sus.
>
> The last nibble represents the core-level power state and the
> penultimate one represents the same at cluster level. A value
> of 2 in that cluster nibble is actually undefined by the PSCI spec,
> whereas the value of 4 (as you have in all of the other idle
> states, including D3G for the perf cluster) corresponds to
> "Retention", so unless there's a very weird nuance in the
> TZ for this SoC, it should probably end in 0x42.
>
> Otherwise I think this LGTM now!

I am also learning by experiment about the exact values to use here,
as the only ready reckoner of how these values are calculated, seems
to be available via [1].

Also it seems the downstream code uses the following approach to
calculate the LPM state suspend-param, which for example for
CLUSTER_0_SLEEP_1 states turns out to be:

    state_id = get_cluster_id(cpu->parent, &affinity_level, from_idle); = 0x40
    power_state = (is-reset << 30) = 0x40000000
    affinity_level = (affinity level & 0x3) << 24 = 0x1000000
    state_id += power_state + affinity_level + psci_id;

    = 0x40000000 + 0x1000000 + 0x40 + 0x4 = 0x41000044

For the D3G cases as well, I just used the 'qcom,psci-mode = <2>'
value as provided in downstream code (see [2]), for the overall
calculations.

Also, the only usage of D3G state I could find upstream (in qcom dtsi
files0 is for 'msm8916' (see [3]), which also uses the value with
ending 0x2 -> 'arm,psci-suspend-param = <0x41000032>'

[1]. https://patchwork.kernel.org/project/linux-arm-msm/patch/20201221002907.2870059-3-danny@kdrag0n.dev/#23857409
[2]. https://android.googlesource.com/kernel/msm-extra/devicetree/+/refs/tags/android-11.0.0_r0.56/qcom/bengal-pm.dtsi#127
[3]. https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/qcom/msm8916.dtsi#L209

Thanks,
Bhupesh

> > +                             entry-latency-us = <360>;
> > +                             exit-latency-us = <421>;
> > +                             min-residency-us = <782>;
> > +                     };
> > +
> > +                     CLUSTER_0_SLEEP_1: cluster-sleep-0-1 {
> > +                             /* Power Collapse */
> > +                             compatible = "domain-idle-state";
> > +                             arm,psci-suspend-param = <0x41000044>;
> > +                             entry-latency-us = <800>;
> > +                             exit-latency-us = <2118>;
> > +                             min-residency-us = <7376>;
> > +                     };
> > +
> > +                     CLUSTER_1_SLEEP_0: cluster-sleep-1-0 {
> > +                             /* GDHS */
> > +                             compatible = "domain-idle-state";
> > +                             arm,psci-suspend-param = <0x40000042>;
> > +                             entry-latency-us = <314>;
> > +                             exit-latency-us = <345>;
> > +                             min-residency-us = <660>;
> > +                     };
> > +
> > +                     CLUSTER_1_SLEEP_1: cluster-sleep-1-1 {
> > +                             /* Power Collapse */
> > +                             compatible = "domain-idle-state";
> > +                             arm,psci-suspend-param = <0x41000044>;
> > +                             entry-latency-us = <640>;
> > +                             exit-latency-us = <1654>;
> > +                             min-residency-us = <8094>;
> > +                     };
> > +             };
> >       };
> >
> >       firmware {
> > @@ -199,6 +277,64 @@ pmu {
> >       psci {
> >               compatible = "arm,psci-1.0";
> >               method = "smc";
> > +
> > +             CPU_PD0: power-domain-cpu0 {
> > +                     #power-domain-cells = <0>;
> > +                     power-domains = <&CLUSTER_0_PD>;
> > +                     domain-idle-states = <&LITTLE_CPU_SLEEP_0>;
> > +             };
> > +
> > +             CPU_PD1: power-domain-cpu1 {
> > +                     #power-domain-cells = <0>;
> > +                     power-domains = <&CLUSTER_0_PD>;
> > +                     domain-idle-states = <&LITTLE_CPU_SLEEP_0>;
> > +             };
> > +
> > +             CPU_PD2: power-domain-cpu2 {
> > +                     #power-domain-cells = <0>;
> > +                     power-domains = <&CLUSTER_0_PD>;
> > +                     domain-idle-states = <&LITTLE_CPU_SLEEP_0>;
> > +             };
> > +
> > +             CPU_PD3: power-domain-cpu3 {
> > +                     #power-domain-cells = <0>;
> > +                     power-domains = <&CLUSTER_0_PD>;
> > +                     domain-idle-states = <&LITTLE_CPU_SLEEP_0>;
> > +             };
> > +
> > +             CPU_PD4: power-domain-cpu4 {
> > +                     #power-domain-cells = <0>;
> > +                     power-domains = <&CLUSTER_1_PD>;
> > +                     domain-idle-states = <&BIG_CPU_SLEEP_0>;
> > +             };
> > +
> > +             CPU_PD5: power-domain-cpu5 {
> > +                     #power-domain-cells = <0>;
> > +                     power-domains = <&CLUSTER_1_PD>;
> > +                     domain-idle-states = <&BIG_CPU_SLEEP_0>;
> > +             };
> > +
> > +             CPU_PD6: power-domain-cpu6 {
> > +                     #power-domain-cells = <0>;
> > +                     power-domains = <&CLUSTER_1_PD>;
> > +                     domain-idle-states = <&BIG_CPU_SLEEP_0>;
> > +             };
> > +
> > +             CPU_PD7: power-domain-cpu7 {
> > +                     #power-domain-cells = <0>;
> > +                     power-domains = <&CLUSTER_1_PD>;
> > +                     domain-idle-states = <&BIG_CPU_SLEEP_0>;
> > +             };
> > +
> > +             CLUSTER_0_PD: power-domain-cpu-cluster0 {
> > +                     #power-domain-cells = <0>;
> > +                     domain-idle-states = <&CLUSTER_0_SLEEP_0>, <&CLUSTER_0_SLEEP_1>;
> > +             };
> > +
> > +             CLUSTER_1_PD: power-domain-cpu-cluster1 {
> > +                     #power-domain-cells = <0>;
> > +                     domain-idle-states = <&CLUSTER_1_SLEEP_0>, <&CLUSTER_1_SLEEP_1>;
> > +             };
> >       };
> >
> >       reserved_memory: reserved-memory {
Dmitry Baryshkov April 1, 2023, 7:58 p.m. UTC | #3 
On 01/04/2023 21:26, Bhupesh Sharma wrote:
> Hi Konrad,
> 
> On Sat, 1 Apr 2023 at 17:51, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>>
>>
>>
>> On 30.03.2023 21:33, Bhupesh Sharma wrote:
>>> Add CPU idle-state nodes and power-domains in Qualcomm sm6115 SoC dtsi.
>>>
>>> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
>>> ---
>>> Changes since v1:
>>> - v1 can be viewed here: https://lore.kernel.org/lkml/e5cda4cf-5c2a-a7ed-9e1d-1fe9f2cbef40@linaro.org
>>> - Addressed Konrad's comments on v1 and added GDHS and Power Collapse
>>>    cluster power states.
>>>
>>>   arch/arm64/boot/dts/qcom/sm6115.dtsi | 136 +++++++++++++++++++++++++++
>>>   1 file changed, 136 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
>>> index 2a51c938bbcb..b63395d476ed 100644
>>> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
>>> @@ -45,6 +45,8 @@ CPU0: cpu@0 {
>>>                        enable-method = "psci";
>>>                        next-level-cache = <&L2_0>;
>>>                        qcom,freq-domain = <&cpufreq_hw 0>;
>>> +                     power-domains = <&CPU_PD0>;
>>> +                     power-domain-names = "psci";
>>>                        L2_0: l2-cache {
>>>                                compatible = "cache";
>>>                                cache-level = <2>;
>>> @@ -61,6 +63,8 @@ CPU1: cpu@1 {
>>>                        enable-method = "psci";
>>>                        next-level-cache = <&L2_0>;
>>>                        qcom,freq-domain = <&cpufreq_hw 0>;
>>> +                     power-domains = <&CPU_PD1>;
>>> +                     power-domain-names = "psci";
>>>                };
>>>
>>>                CPU2: cpu@2 {
>>> @@ -73,6 +77,8 @@ CPU2: cpu@2 {
>>>                        enable-method = "psci";
>>>                        next-level-cache = <&L2_0>;
>>>                        qcom,freq-domain = <&cpufreq_hw 0>;
>>> +                     power-domains = <&CPU_PD2>;
>>> +                     power-domain-names = "psci";
>>>                };
>>>
>>>                CPU3: cpu@3 {
>>> @@ -85,6 +91,8 @@ CPU3: cpu@3 {
>>>                        enable-method = "psci";
>>>                        next-level-cache = <&L2_0>;
>>>                        qcom,freq-domain = <&cpufreq_hw 0>;
>>> +                     power-domains = <&CPU_PD3>;
>>> +                     power-domain-names = "psci";
>>>                };
>>>
>>>                CPU4: cpu@100 {
>>> @@ -97,6 +105,8 @@ CPU4: cpu@100 {
>>>                        dynamic-power-coefficient = <282>;
>>>                        next-level-cache = <&L2_1>;
>>>                        qcom,freq-domain = <&cpufreq_hw 1>;
>>> +                     power-domains = <&CPU_PD4>;
>>> +                     power-domain-names = "psci";
>>>                        L2_1: l2-cache {
>>>                                compatible = "cache";
>>>                                cache-level = <2>;
>>> @@ -113,6 +123,8 @@ CPU5: cpu@101 {
>>>                        enable-method = "psci";
>>>                        next-level-cache = <&L2_1>;
>>>                        qcom,freq-domain = <&cpufreq_hw 1>;
>>> +                     power-domains = <&CPU_PD5>;
>>> +                     power-domain-names = "psci";
>>>                };
>>>
>>>                CPU6: cpu@102 {
>>> @@ -125,6 +137,8 @@ CPU6: cpu@102 {
>>>                        enable-method = "psci";
>>>                        next-level-cache = <&L2_1>;
>>>                        qcom,freq-domain = <&cpufreq_hw 1>;
>>> +                     power-domains = <&CPU_PD6>;
>>> +                     power-domain-names = "psci";
>>>                };
>>>
>>>                CPU7: cpu@103 {
>>> @@ -137,6 +151,8 @@ CPU7: cpu@103 {
>>>                        enable-method = "psci";
>>>                        next-level-cache = <&L2_1>;
>>>                        qcom,freq-domain = <&cpufreq_hw 1>;
>>> +                     power-domains = <&CPU_PD7>;
>>> +                     power-domain-names = "psci";
>>>                };
>>>
>>>                cpu-map {
>>> @@ -176,6 +192,68 @@ core3 {
>>>                                };
>>>                        };
>>>                };
>>> +
>>> +             idle-states {
>>> +                     entry-method = "psci";
>>> +
>>> +                     LITTLE_CPU_SLEEP_0: cpu-sleep-0-0 {
>>> +                             compatible = "arm,idle-state";
>>> +                             idle-state-name = "silver-rail-power-collapse";
>>> +                             arm,psci-suspend-param = <0x40000003>;
>>> +                             entry-latency-us = <290>;
>>> +                             exit-latency-us = <376>;
>>> +                             min-residency-us = <1182>;
>>> +                             local-timer-stop;
>>> +                     };
>>> +
>>> +                     BIG_CPU_SLEEP_0: cpu-sleep-1-0 {
>>> +                             compatible = "arm,idle-state";
>>> +                             idle-state-name = "gold-rail-power-collapse";
>>> +                             arm,psci-suspend-param = <0x40000003>;
>>> +                             entry-latency-us = <297>;
>>> +                             exit-latency-us = <324>;
>>> +                             min-residency-us = <1110>;
>>> +                             local-timer-stop;
>>> +                     };
>>> +             };
>>> +
>>> +             domain-idle-states {
>>> +                     CLUSTER_0_SLEEP_0: cluster-sleep-0-0 {
>>> +                             /* GDHS */
>>> +                             compatible = "domain-idle-state";
>>> +                             arm,psci-suspend-param = <0x40000022>;
>> This 0x22 ending seems very sus.
>>
>> The last nibble represents the core-level power state and the
>> penultimate one represents the same at cluster level. A value
>> of 2 in that cluster nibble is actually undefined by the PSCI spec,
>> whereas the value of 4 (as you have in all of the other idle
>> states, including D3G for the perf cluster) corresponds to
>> "Retention", so unless there's a very weird nuance in the
>> TZ for this SoC, it should probably end in 0x42.
>>
>> Otherwise I think this LGTM now!
> 
> I am also learning by experiment about the exact values to use here,
> as the only ready reckoner of how these values are calculated, seems
> to be available via [1].
> 
> Also it seems the downstream code uses the following approach to
> calculate the LPM state suspend-param, which for example for
> CLUSTER_0_SLEEP_1 states turns out to be:
> 
>      state_id = get_cluster_id(cpu->parent, &affinity_level, from_idle); = 0x40
>      power_state = (is-reset << 30) = 0x40000000
>      affinity_level = (affinity level & 0x3) << 24 = 0x1000000
>      state_id += power_state + affinity_level + psci_id;
> 
>      = 0x40000000 + 0x1000000 + 0x40 + 0x4 = 0x41000044
> 
> For the D3G cases as well, I just used the 'qcom,psci-mode = <2>'
> value as provided in downstream code (see [2]), for the overall
> calculations.
> 
> Also, the only usage of D3G state I could find upstream (in qcom dtsi
> files0 is for 'msm8916' (see [3]), which also uses the value with
> ending 0x2 -> 'arm,psci-suspend-param = <0x41000032>'

D3G has min-child-idx = 1, so the end PSCI param should be 0x41000023
D3 is 0x41000043

> 
> [1]. https://patchwork.kernel.org/project/linux-arm-msm/patch/20201221002907.2870059-3-danny@kdrag0n.dev/#23857409
> [2]. https://android.googlesource.com/kernel/msm-extra/devicetree/+/refs/tags/android-11.0.0_r0.56/qcom/bengal-pm.dtsi#127
> [3]. https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/qcom/msm8916.dtsi#L209
> 
> Thanks,
> Bhupesh
> 
>>> +                             entry-latency-us = <360>;
>>> +                             exit-latency-us = <421>;
>>> +                             min-residency-us = <782>;
>>> +                     };
>>> +
>>> +                     CLUSTER_0_SLEEP_1: cluster-sleep-0-1 {
>>> +                             /* Power Collapse */
>>> +                             compatible = "domain-idle-state";
>>> +                             arm,psci-suspend-param = <0x41000044>;
>>> +                             entry-latency-us = <800>;
>>> +                             exit-latency-us = <2118>;
>>> +                             min-residency-us = <7376>;
>>> +                     };
>>> +
>>> +                     CLUSTER_1_SLEEP_0: cluster-sleep-1-0 {
>>> +                             /* GDHS */
>>> +                             compatible = "domain-idle-state";
>>> +                             arm,psci-suspend-param = <0x40000042>;
>>> +                             entry-latency-us = <314>;
>>> +                             exit-latency-us = <345>;
>>> +                             min-residency-us = <660>;
>>> +                     };
>>> +
>>> +                     CLUSTER_1_SLEEP_1: cluster-sleep-1-1 {
>>> +                             /* Power Collapse */
>>> +                             compatible = "domain-idle-state";
>>> +                             arm,psci-suspend-param = <0x41000044>;
>>> +                             entry-latency-us = <640>;
>>> +                             exit-latency-us = <1654>;
>>> +                             min-residency-us = <8094>;
>>> +                     };
>>> +             };
>>>        };
>>>
>>>        firmware {
>>> @@ -199,6 +277,64 @@ pmu {
>>>        psci {
>>>                compatible = "arm,psci-1.0";
>>>                method = "smc";
>>> +
>>> +             CPU_PD0: power-domain-cpu0 {
>>> +                     #power-domain-cells = <0>;
>>> +                     power-domains = <&CLUSTER_0_PD>;
>>> +                     domain-idle-states = <&LITTLE_CPU_SLEEP_0>;
>>> +             };
>>> +
>>> +             CPU_PD1: power-domain-cpu1 {
>>> +                     #power-domain-cells = <0>;
>>> +                     power-domains = <&CLUSTER_0_PD>;
>>> +                     domain-idle-states = <&LITTLE_CPU_SLEEP_0>;
>>> +             };
>>> +
>>> +             CPU_PD2: power-domain-cpu2 {
>>> +                     #power-domain-cells = <0>;
>>> +                     power-domains = <&CLUSTER_0_PD>;
>>> +                     domain-idle-states = <&LITTLE_CPU_SLEEP_0>;
>>> +             };
>>> +
>>> +             CPU_PD3: power-domain-cpu3 {
>>> +                     #power-domain-cells = <0>;
>>> +                     power-domains = <&CLUSTER_0_PD>;
>>> +                     domain-idle-states = <&LITTLE_CPU_SLEEP_0>;
>>> +             };
>>> +
>>> +             CPU_PD4: power-domain-cpu4 {
>>> +                     #power-domain-cells = <0>;
>>> +                     power-domains = <&CLUSTER_1_PD>;
>>> +                     domain-idle-states = <&BIG_CPU_SLEEP_0>;
>>> +             };
>>> +
>>> +             CPU_PD5: power-domain-cpu5 {
>>> +                     #power-domain-cells = <0>;
>>> +                     power-domains = <&CLUSTER_1_PD>;
>>> +                     domain-idle-states = <&BIG_CPU_SLEEP_0>;
>>> +             };
>>> +
>>> +             CPU_PD6: power-domain-cpu6 {
>>> +                     #power-domain-cells = <0>;
>>> +                     power-domains = <&CLUSTER_1_PD>;
>>> +                     domain-idle-states = <&BIG_CPU_SLEEP_0>;
>>> +             };
>>> +
>>> +             CPU_PD7: power-domain-cpu7 {
>>> +                     #power-domain-cells = <0>;
>>> +                     power-domains = <&CLUSTER_1_PD>;
>>> +                     domain-idle-states = <&BIG_CPU_SLEEP_0>;
>>> +             };
>>> +
>>> +             CLUSTER_0_PD: power-domain-cpu-cluster0 {
>>> +                     #power-domain-cells = <0>;
>>> +                     domain-idle-states = <&CLUSTER_0_SLEEP_0>, <&CLUSTER_0_SLEEP_1>;
>>> +             };
>>> +
>>> +             CLUSTER_1_PD: power-domain-cpu-cluster1 {
>>> +                     #power-domain-cells = <0>;
>>> +                     domain-idle-states = <&CLUSTER_1_SLEEP_0>, <&CLUSTER_1_SLEEP_1>;
>>> +             };
>>>        };
>>>
>>>        reserved_memory: reserved-memory {
Bhupesh Sharma April 2, 2023, 5:35 a.m. UTC | #4 
On Sun, 2 Apr 2023 at 01:28, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On 01/04/2023 21:26, Bhupesh Sharma wrote:
> > Hi Konrad,
> >
> > On Sat, 1 Apr 2023 at 17:51, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> >>
> >>
> >>
> >> On 30.03.2023 21:33, Bhupesh Sharma wrote:
> >>> Add CPU idle-state nodes and power-domains in Qualcomm sm6115 SoC dtsi.
> >>>
> >>> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
> >>> ---
> >>> Changes since v1:
> >>> - v1 can be viewed here: https://lore.kernel.org/lkml/e5cda4cf-5c2a-a7ed-9e1d-1fe9f2cbef40@linaro.org
> >>> - Addressed Konrad's comments on v1 and added GDHS and Power Collapse
> >>>    cluster power states.
> >>>
> >>>   arch/arm64/boot/dts/qcom/sm6115.dtsi | 136 +++++++++++++++++++++++++++
> >>>   1 file changed, 136 insertions(+)
> >>>
> >>> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> >>> index 2a51c938bbcb..b63395d476ed 100644
> >>> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
> >>> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> >>> @@ -45,6 +45,8 @@ CPU0: cpu@0 {
> >>>                        enable-method = "psci";
> >>>                        next-level-cache = <&L2_0>;
> >>>                        qcom,freq-domain = <&cpufreq_hw 0>;
> >>> +                     power-domains = <&CPU_PD0>;
> >>> +                     power-domain-names = "psci";
> >>>                        L2_0: l2-cache {
> >>>                                compatible = "cache";
> >>>                                cache-level = <2>;
> >>> @@ -61,6 +63,8 @@ CPU1: cpu@1 {
> >>>                        enable-method = "psci";
> >>>                        next-level-cache = <&L2_0>;
> >>>                        qcom,freq-domain = <&cpufreq_hw 0>;
> >>> +                     power-domains = <&CPU_PD1>;
> >>> +                     power-domain-names = "psci";
> >>>                };
> >>>
> >>>                CPU2: cpu@2 {
> >>> @@ -73,6 +77,8 @@ CPU2: cpu@2 {
> >>>                        enable-method = "psci";
> >>>                        next-level-cache = <&L2_0>;
> >>>                        qcom,freq-domain = <&cpufreq_hw 0>;
> >>> +                     power-domains = <&CPU_PD2>;
> >>> +                     power-domain-names = "psci";
> >>>                };
> >>>
> >>>                CPU3: cpu@3 {
> >>> @@ -85,6 +91,8 @@ CPU3: cpu@3 {
> >>>                        enable-method = "psci";
> >>>                        next-level-cache = <&L2_0>;
> >>>                        qcom,freq-domain = <&cpufreq_hw 0>;
> >>> +                     power-domains = <&CPU_PD3>;
> >>> +                     power-domain-names = "psci";
> >>>                };
> >>>
> >>>                CPU4: cpu@100 {
> >>> @@ -97,6 +105,8 @@ CPU4: cpu@100 {
> >>>                        dynamic-power-coefficient = <282>;
> >>>                        next-level-cache = <&L2_1>;
> >>>                        qcom,freq-domain = <&cpufreq_hw 1>;
> >>> +                     power-domains = <&CPU_PD4>;
> >>> +                     power-domain-names = "psci";
> >>>                        L2_1: l2-cache {
> >>>                                compatible = "cache";
> >>>                                cache-level = <2>;
> >>> @@ -113,6 +123,8 @@ CPU5: cpu@101 {
> >>>                        enable-method = "psci";
> >>>                        next-level-cache = <&L2_1>;
> >>>                        qcom,freq-domain = <&cpufreq_hw 1>;
> >>> +                     power-domains = <&CPU_PD5>;
> >>> +                     power-domain-names = "psci";
> >>>                };
> >>>
> >>>                CPU6: cpu@102 {
> >>> @@ -125,6 +137,8 @@ CPU6: cpu@102 {
> >>>                        enable-method = "psci";
> >>>                        next-level-cache = <&L2_1>;
> >>>                        qcom,freq-domain = <&cpufreq_hw 1>;
> >>> +                     power-domains = <&CPU_PD6>;
> >>> +                     power-domain-names = "psci";
> >>>                };
> >>>
> >>>                CPU7: cpu@103 {
> >>> @@ -137,6 +151,8 @@ CPU7: cpu@103 {
> >>>                        enable-method = "psci";
> >>>                        next-level-cache = <&L2_1>;
> >>>                        qcom,freq-domain = <&cpufreq_hw 1>;
> >>> +                     power-domains = <&CPU_PD7>;
> >>> +                     power-domain-names = "psci";
> >>>                };
> >>>
> >>>                cpu-map {
> >>> @@ -176,6 +192,68 @@ core3 {
> >>>                                };
> >>>                        };
> >>>                };
> >>> +
> >>> +             idle-states {
> >>> +                     entry-method = "psci";
> >>> +
> >>> +                     LITTLE_CPU_SLEEP_0: cpu-sleep-0-0 {
> >>> +                             compatible = "arm,idle-state";
> >>> +                             idle-state-name = "silver-rail-power-collapse";
> >>> +                             arm,psci-suspend-param = <0x40000003>;
> >>> +                             entry-latency-us = <290>;
> >>> +                             exit-latency-us = <376>;
> >>> +                             min-residency-us = <1182>;
> >>> +                             local-timer-stop;
> >>> +                     };
> >>> +
> >>> +                     BIG_CPU_SLEEP_0: cpu-sleep-1-0 {
> >>> +                             compatible = "arm,idle-state";
> >>> +                             idle-state-name = "gold-rail-power-collapse";
> >>> +                             arm,psci-suspend-param = <0x40000003>;
> >>> +                             entry-latency-us = <297>;
> >>> +                             exit-latency-us = <324>;
> >>> +                             min-residency-us = <1110>;
> >>> +                             local-timer-stop;
> >>> +                     };
> >>> +             };
> >>> +
> >>> +             domain-idle-states {
> >>> +                     CLUSTER_0_SLEEP_0: cluster-sleep-0-0 {
> >>> +                             /* GDHS */
> >>> +                             compatible = "domain-idle-state";
> >>> +                             arm,psci-suspend-param = <0x40000022>;
> >> This 0x22 ending seems very sus.
> >>
> >> The last nibble represents the core-level power state and the
> >> penultimate one represents the same at cluster level. A value
> >> of 2 in that cluster nibble is actually undefined by the PSCI spec,
> >> whereas the value of 4 (as you have in all of the other idle
> >> states, including D3G for the perf cluster) corresponds to
> >> "Retention", so unless there's a very weird nuance in the
> >> TZ for this SoC, it should probably end in 0x42.
> >>
> >> Otherwise I think this LGTM now!
> >
> > I am also learning by experiment about the exact values to use here,
> > as the only ready reckoner of how these values are calculated, seems
> > to be available via [1].
> >
> > Also it seems the downstream code uses the following approach to
> > calculate the LPM state suspend-param, which for example for
> > CLUSTER_0_SLEEP_1 states turns out to be:
> >
> >      state_id = get_cluster_id(cpu->parent, &affinity_level, from_idle); = 0x40
> >      power_state = (is-reset << 30) = 0x40000000
> >      affinity_level = (affinity level & 0x3) << 24 = 0x1000000
> >      state_id += power_state + affinity_level + psci_id;
> >
> >      = 0x40000000 + 0x1000000 + 0x40 + 0x4 = 0x41000044
> >
> > For the D3G cases as well, I just used the 'qcom,psci-mode = <2>'
> > value as provided in downstream code (see [2]), for the overall
> > calculations.
> >
> > Also, the only usage of D3G state I could find upstream (in qcom dtsi
> > files0 is for 'msm8916' (see [3]), which also uses the value with
> > ending 0x2 -> 'arm,psci-suspend-param = <0x41000032>'
>
> D3G has min-child-idx = 1, so the end PSCI param should be 0x41000023
> D3 is 0x41000043

Ok, let me recheck at my end as well.

Thanks
Bhupesh
Konrad Dybcio April 3, 2023, 10:33 a.m. UTC | #5 
On 2.04.2023 07:35, Bhupesh Sharma wrote:
> On Sun, 2 Apr 2023 at 01:28, Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
>>
>> On 01/04/2023 21:26, Bhupesh Sharma wrote:
>>> Hi Konrad,
>>>
>>> On Sat, 1 Apr 2023 at 17:51, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>>>>
>>>>
>>>>
>>>> On 30.03.2023 21:33, Bhupesh Sharma wrote:
>>>>> Add CPU idle-state nodes and power-domains in Qualcomm sm6115 SoC dtsi.
>>>>>
>>>>> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
>>>>> ---
>>>>> Changes since v1:
>>>>> - v1 can be viewed here: https://lore.kernel.org/lkml/e5cda4cf-5c2a-a7ed-9e1d-1fe9f2cbef40@linaro.org
>>>>> - Addressed Konrad's comments on v1 and added GDHS and Power Collapse
>>>>>    cluster power states.
>>>>>
>>>>>   arch/arm64/boot/dts/qcom/sm6115.dtsi | 136 +++++++++++++++++++++++++++
>>>>>   1 file changed, 136 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
>>>>> index 2a51c938bbcb..b63395d476ed 100644
>>>>> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
>>>>> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
>>>>> @@ -45,6 +45,8 @@ CPU0: cpu@0 {
>>>>>                        enable-method = "psci";
>>>>>                        next-level-cache = <&L2_0>;
>>>>>                        qcom,freq-domain = <&cpufreq_hw 0>;
>>>>> +                     power-domains = <&CPU_PD0>;
>>>>> +                     power-domain-names = "psci";
>>>>>                        L2_0: l2-cache {
>>>>>                                compatible = "cache";
>>>>>                                cache-level = <2>;
>>>>> @@ -61,6 +63,8 @@ CPU1: cpu@1 {
>>>>>                        enable-method = "psci";
>>>>>                        next-level-cache = <&L2_0>;
>>>>>                        qcom,freq-domain = <&cpufreq_hw 0>;
>>>>> +                     power-domains = <&CPU_PD1>;
>>>>> +                     power-domain-names = "psci";
>>>>>                };
>>>>>
>>>>>                CPU2: cpu@2 {
>>>>> @@ -73,6 +77,8 @@ CPU2: cpu@2 {
>>>>>                        enable-method = "psci";
>>>>>                        next-level-cache = <&L2_0>;
>>>>>                        qcom,freq-domain = <&cpufreq_hw 0>;
>>>>> +                     power-domains = <&CPU_PD2>;
>>>>> +                     power-domain-names = "psci";
>>>>>                };
>>>>>
>>>>>                CPU3: cpu@3 {
>>>>> @@ -85,6 +91,8 @@ CPU3: cpu@3 {
>>>>>                        enable-method = "psci";
>>>>>                        next-level-cache = <&L2_0>;
>>>>>                        qcom,freq-domain = <&cpufreq_hw 0>;
>>>>> +                     power-domains = <&CPU_PD3>;
>>>>> +                     power-domain-names = "psci";
>>>>>                };
>>>>>
>>>>>                CPU4: cpu@100 {
>>>>> @@ -97,6 +105,8 @@ CPU4: cpu@100 {
>>>>>                        dynamic-power-coefficient = <282>;
>>>>>                        next-level-cache = <&L2_1>;
>>>>>                        qcom,freq-domain = <&cpufreq_hw 1>;
>>>>> +                     power-domains = <&CPU_PD4>;
>>>>> +                     power-domain-names = "psci";
>>>>>                        L2_1: l2-cache {
>>>>>                                compatible = "cache";
>>>>>                                cache-level = <2>;
>>>>> @@ -113,6 +123,8 @@ CPU5: cpu@101 {
>>>>>                        enable-method = "psci";
>>>>>                        next-level-cache = <&L2_1>;
>>>>>                        qcom,freq-domain = <&cpufreq_hw 1>;
>>>>> +                     power-domains = <&CPU_PD5>;
>>>>> +                     power-domain-names = "psci";
>>>>>                };
>>>>>
>>>>>                CPU6: cpu@102 {
>>>>> @@ -125,6 +137,8 @@ CPU6: cpu@102 {
>>>>>                        enable-method = "psci";
>>>>>                        next-level-cache = <&L2_1>;
>>>>>                        qcom,freq-domain = <&cpufreq_hw 1>;
>>>>> +                     power-domains = <&CPU_PD6>;
>>>>> +                     power-domain-names = "psci";
>>>>>                };
>>>>>
>>>>>                CPU7: cpu@103 {
>>>>> @@ -137,6 +151,8 @@ CPU7: cpu@103 {
>>>>>                        enable-method = "psci";
>>>>>                        next-level-cache = <&L2_1>;
>>>>>                        qcom,freq-domain = <&cpufreq_hw 1>;
>>>>> +                     power-domains = <&CPU_PD7>;
>>>>> +                     power-domain-names = "psci";
>>>>>                };
>>>>>
>>>>>                cpu-map {
>>>>> @@ -176,6 +192,68 @@ core3 {
>>>>>                                };
>>>>>                        };
>>>>>                };
>>>>> +
>>>>> +             idle-states {
>>>>> +                     entry-method = "psci";
>>>>> +
>>>>> +                     LITTLE_CPU_SLEEP_0: cpu-sleep-0-0 {
>>>>> +                             compatible = "arm,idle-state";
>>>>> +                             idle-state-name = "silver-rail-power-collapse";
>>>>> +                             arm,psci-suspend-param = <0x40000003>;
>>>>> +                             entry-latency-us = <290>;
>>>>> +                             exit-latency-us = <376>;
>>>>> +                             min-residency-us = <1182>;
>>>>> +                             local-timer-stop;
>>>>> +                     };
>>>>> +
>>>>> +                     BIG_CPU_SLEEP_0: cpu-sleep-1-0 {
>>>>> +                             compatible = "arm,idle-state";
>>>>> +                             idle-state-name = "gold-rail-power-collapse";
>>>>> +                             arm,psci-suspend-param = <0x40000003>;
>>>>> +                             entry-latency-us = <297>;
>>>>> +                             exit-latency-us = <324>;
>>>>> +                             min-residency-us = <1110>;
>>>>> +                             local-timer-stop;
>>>>> +                     };
>>>>> +             };
>>>>> +
>>>>> +             domain-idle-states {
>>>>> +                     CLUSTER_0_SLEEP_0: cluster-sleep-0-0 {
>>>>> +                             /* GDHS */
>>>>> +                             compatible = "domain-idle-state";
>>>>> +                             arm,psci-suspend-param = <0x40000022>;
>>>> This 0x22 ending seems very sus.
>>>>
>>>> The last nibble represents the core-level power state and the
>>>> penultimate one represents the same at cluster level. A value
>>>> of 2 in that cluster nibble is actually undefined by the PSCI spec,
>>>> whereas the value of 4 (as you have in all of the other idle
>>>> states, including D3G for the perf cluster) corresponds to
>>>> "Retention", so unless there's a very weird nuance in the
>>>> TZ for this SoC, it should probably end in 0x42.
>>>>
>>>> Otherwise I think this LGTM now!
>>>
>>> I am also learning by experiment about the exact values to use here,
>>> as the only ready reckoner of how these values are calculated, seems
>>> to be available via [1].
>>>
>>> Also it seems the downstream code uses the following approach to
>>> calculate the LPM state suspend-param, which for example for
>>> CLUSTER_0_SLEEP_1 states turns out to be:
>>>
>>>      state_id = get_cluster_id(cpu->parent, &affinity_level, from_idle); = 0x40
>>>      power_state = (is-reset << 30) = 0x40000000
>>>      affinity_level = (affinity level & 0x3) << 24 = 0x1000000
>>>      state_id += power_state + affinity_level + psci_id;
>>>
>>>      = 0x40000000 + 0x1000000 + 0x40 + 0x4 = 0x41000044
>>>
>>> For the D3G cases as well, I just used the 'qcom,psci-mode = <2>'
>>> value as provided in downstream code (see [2]), for the overall
>>> calculations.>>>
>>> Also, the only usage of D3G state I could find upstream (in qcom dtsi
>>> files0 is for 'msm8916' (see [3]), which also uses the value with
>>> ending 0x2 -> 'arm,psci-suspend-param = <0x41000032>'
Yes, the lowest '2' must be correct. I am concerned about the one above
it (val & 0xf0).

>>
>> D3G has min-child-idx = 1, so the end PSCI param should be 0x41000023
>> D3 is 0x41000043
Not sure what that has to do with it. Looking at ancient kernel doc:

qcom,min-child-idx: The minimum level that a child CPU should be in
	before this level can be chosen. This property is required for all
        non-default level.

And it looks like the downstream code ensures that we don't just jump
from "CPU running" to "CPU [some stage of] power collapse".


So it looks like this patch may be good after all.. Can you verify with
sysfs/debugfs that this idle state is being entered correctly?

Konrad
> 
> Ok, let me recheck at my end as well.
> 
> Thanks
> Bhupesh
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
index 2a51c938bbcb..b63395d476ed 100644
--- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
@@ -45,6 +45,8 @@  CPU0: cpu@0 {
 			enable-method = "psci";
 			next-level-cache = <&L2_0>;
 			qcom,freq-domain = <&cpufreq_hw 0>;
+			power-domains = <&CPU_PD0>;
+			power-domain-names = "psci";
 			L2_0: l2-cache {
 				compatible = "cache";
 				cache-level = <2>;
@@ -61,6 +63,8 @@  CPU1: cpu@1 {
 			enable-method = "psci";
 			next-level-cache = <&L2_0>;
 			qcom,freq-domain = <&cpufreq_hw 0>;
+			power-domains = <&CPU_PD1>;
+			power-domain-names = "psci";
 		};
 
 		CPU2: cpu@2 {
@@ -73,6 +77,8 @@  CPU2: cpu@2 {
 			enable-method = "psci";
 			next-level-cache = <&L2_0>;
 			qcom,freq-domain = <&cpufreq_hw 0>;
+			power-domains = <&CPU_PD2>;
+			power-domain-names = "psci";
 		};
 
 		CPU3: cpu@3 {
@@ -85,6 +91,8 @@  CPU3: cpu@3 {
 			enable-method = "psci";
 			next-level-cache = <&L2_0>;
 			qcom,freq-domain = <&cpufreq_hw 0>;
+			power-domains = <&CPU_PD3>;
+			power-domain-names = "psci";
 		};
 
 		CPU4: cpu@100 {
@@ -97,6 +105,8 @@  CPU4: cpu@100 {
 			dynamic-power-coefficient = <282>;
 			next-level-cache = <&L2_1>;
 			qcom,freq-domain = <&cpufreq_hw 1>;
+			power-domains = <&CPU_PD4>;
+			power-domain-names = "psci";
 			L2_1: l2-cache {
 				compatible = "cache";
 				cache-level = <2>;
@@ -113,6 +123,8 @@  CPU5: cpu@101 {
 			enable-method = "psci";
 			next-level-cache = <&L2_1>;
 			qcom,freq-domain = <&cpufreq_hw 1>;
+			power-domains = <&CPU_PD5>;
+			power-domain-names = "psci";
 		};
 
 		CPU6: cpu@102 {
@@ -125,6 +137,8 @@  CPU6: cpu@102 {
 			enable-method = "psci";
 			next-level-cache = <&L2_1>;
 			qcom,freq-domain = <&cpufreq_hw 1>;
+			power-domains = <&CPU_PD6>;
+			power-domain-names = "psci";
 		};
 
 		CPU7: cpu@103 {
@@ -137,6 +151,8 @@  CPU7: cpu@103 {
 			enable-method = "psci";
 			next-level-cache = <&L2_1>;
 			qcom,freq-domain = <&cpufreq_hw 1>;
+			power-domains = <&CPU_PD7>;
+			power-domain-names = "psci";
 		};
 
 		cpu-map {
@@ -176,6 +192,68 @@  core3 {
 				};
 			};
 		};
+
+		idle-states {
+			entry-method = "psci";
+
+			LITTLE_CPU_SLEEP_0: cpu-sleep-0-0 {
+				compatible = "arm,idle-state";
+				idle-state-name = "silver-rail-power-collapse";
+				arm,psci-suspend-param = <0x40000003>;
+				entry-latency-us = <290>;
+				exit-latency-us = <376>;
+				min-residency-us = <1182>;
+				local-timer-stop;
+			};
+
+			BIG_CPU_SLEEP_0: cpu-sleep-1-0 {
+				compatible = "arm,idle-state";
+				idle-state-name = "gold-rail-power-collapse";
+				arm,psci-suspend-param = <0x40000003>;
+				entry-latency-us = <297>;
+				exit-latency-us = <324>;
+				min-residency-us = <1110>;
+				local-timer-stop;
+			};
+		};
+
+		domain-idle-states {
+			CLUSTER_0_SLEEP_0: cluster-sleep-0-0 {
+				/* GDHS */
+				compatible = "domain-idle-state";
+				arm,psci-suspend-param = <0x40000022>;
+				entry-latency-us = <360>;
+				exit-latency-us = <421>;
+				min-residency-us = <782>;
+			};
+
+			CLUSTER_0_SLEEP_1: cluster-sleep-0-1 {
+				/* Power Collapse */
+				compatible = "domain-idle-state";
+				arm,psci-suspend-param = <0x41000044>;
+				entry-latency-us = <800>;
+				exit-latency-us = <2118>;
+				min-residency-us = <7376>;
+			};
+
+			CLUSTER_1_SLEEP_0: cluster-sleep-1-0 {
+				/* GDHS */
+				compatible = "domain-idle-state";
+				arm,psci-suspend-param = <0x40000042>;
+				entry-latency-us = <314>;
+				exit-latency-us = <345>;
+				min-residency-us = <660>;
+			};
+
+			CLUSTER_1_SLEEP_1: cluster-sleep-1-1 {
+				/* Power Collapse */
+				compatible = "domain-idle-state";
+				arm,psci-suspend-param = <0x41000044>;
+				entry-latency-us = <640>;
+				exit-latency-us = <1654>;
+				min-residency-us = <8094>;
+			};
+		};
 	};
 
 	firmware {
@@ -199,6 +277,64 @@  pmu {
 	psci {
 		compatible = "arm,psci-1.0";
 		method = "smc";
+
+		CPU_PD0: power-domain-cpu0 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_0_PD>;
+			domain-idle-states = <&LITTLE_CPU_SLEEP_0>;
+		};
+
+		CPU_PD1: power-domain-cpu1 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_0_PD>;
+			domain-idle-states = <&LITTLE_CPU_SLEEP_0>;
+		};
+
+		CPU_PD2: power-domain-cpu2 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_0_PD>;
+			domain-idle-states = <&LITTLE_CPU_SLEEP_0>;
+		};
+
+		CPU_PD3: power-domain-cpu3 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_0_PD>;
+			domain-idle-states = <&LITTLE_CPU_SLEEP_0>;
+		};
+
+		CPU_PD4: power-domain-cpu4 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_1_PD>;
+			domain-idle-states = <&BIG_CPU_SLEEP_0>;
+		};
+
+		CPU_PD5: power-domain-cpu5 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_1_PD>;
+			domain-idle-states = <&BIG_CPU_SLEEP_0>;
+		};
+
+		CPU_PD6: power-domain-cpu6 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_1_PD>;
+			domain-idle-states = <&BIG_CPU_SLEEP_0>;
+		};
+
+		CPU_PD7: power-domain-cpu7 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_1_PD>;
+			domain-idle-states = <&BIG_CPU_SLEEP_0>;
+		};
+
+		CLUSTER_0_PD: power-domain-cpu-cluster0 {
+			#power-domain-cells = <0>;
+			domain-idle-states = <&CLUSTER_0_SLEEP_0>, <&CLUSTER_0_SLEEP_1>;
+		};
+
+		CLUSTER_1_PD: power-domain-cpu-cluster1 {
+			#power-domain-cells = <0>;
+			domain-idle-states = <&CLUSTER_1_SLEEP_0>, <&CLUSTER_1_SLEEP_1>;
+		};
 	};
 
 	reserved_memory: reserved-memory {