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

From: Konrad Dybcio
Date: Mon Apr 03 2023 - 06:34:03 EST

Next message: Vladimir Oltean: "[PATCH v4 net-next 0/9] Add tc-mqprio and tc-taprio support for preemptible traffic classes"
Previous message: Fabio Baltieri: "[PATCH] HID: hid-stadiaff: add support for Stadia force feedback"
In reply to: Bhupesh Sharma: "Re: [RESEND PATCH v2 1/1] arm64: dts: qcom: sm6115: Add CPU idle-states"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 2.04.2023 07:35, Bhupesh Sharma wrote:
> On Sun, 2 Apr 2023 at 01:28, Dmitry Baryshkov
> <dmitry.baryshkov@xxxxxxxxxx> wrote:
>>
>> On 01/04/2023 21:26, Bhupesh Sharma wrote:
>>> Hi Konrad,
>>>
>>> On Sat, 1 Apr 2023 at 17:51, Konrad Dybcio <konrad.dybcio@xxxxxxxxxx> 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@xxxxxxxxxx>
>>>>> ---
>>>>> Changes since v1:
>>>>> - v1 can be viewed here: https://lore.kernel.org/lkml/e5cda4cf-5c2a-a7ed-9e1d-1fe9f2cbef40@xxxxxxxxxx
>>>>> - 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

Next message: Vladimir Oltean: "[PATCH v4 net-next 0/9] Add tc-mqprio and tc-taprio support for preemptible traffic classes"
Previous message: Fabio Baltieri: "[PATCH] HID: hid-stadiaff: add support for Stadia force feedback"
In reply to: Bhupesh Sharma: "Re: [RESEND PATCH v2 1/1] arm64: dts: qcom: sm6115: Add CPU idle-states"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]