Re: [PATCH v5 08/10] arm64: dts: qcom: Add initial support for MSM8917

[barnabas.czeman@mainlining.org]

On 2024-11-13 10:10, Stephan Gerhold wrote:
> On Tue, Nov 12, 2024 at 07:49:18PM +0100, 
> barnabas.czeman@mainlining.org wrote:
>> On 2024-11-12 18:27, Stephan Gerhold wrote:
>> > On Tue, Nov 12, 2024 at 04:49:38PM +0100, Barnabás Czémán wrote:
>> > > From: Otto Pflüger <otto.pflueger@abscue.de>
>> > >
>> > > Add initial support for MSM8917 SoC.
>> > >
>> > > Signed-off-by: Otto Pflüger <otto.pflueger@abscue.de>
>> > > [reword commit, rebase, fix schema errors]
>> > > Signed-off-by: Barnabás Czémán <barnabas.czeman@mainlining.org>
>> > > ---
>> > >  arch/arm64/boot/dts/qcom/msm8917.dtsi | 1974
>> > > +++++++++++++++++++++++++++++++++
>> > >  1 file changed, 1974 insertions(+)
>> > >
>> > > diff --git a/arch/arm64/boot/dts/qcom/msm8917.dtsi
>> > > b/arch/arm64/boot/dts/qcom/msm8917.dtsi
>> > > new file mode 100644
>> > > index 0000000000000000000000000000000000000000..cf0a0eec1141e11faca0ee9705d6348ab32a0f50
>> > > --- /dev/null
>> > > +++ b/arch/arm64/boot/dts/qcom/msm8917.dtsi
>> > > @@ -0,0 +1,1974 @@
>> > > [...]
>> > > +		domain-idle-states {
>> > > +			cluster_sleep_0: cluster-sleep-0 {
>> > > +				compatible = "domain-idle-state";
>> > > +				arm,psci-suspend-param = <0x41000023>;
>> > > +				entry-latency-us = <700>;
>> > > +				exit-latency-us = <650>;
>> > > +				min-residency-us = <1972>;
>> > > +			};
>> > > +
>> > > +			cluster_sleep_1: cluster-sleep-1 {
>> > > +				compatible = "domain-idle-state";
>> > > +				arm,psci-suspend-param = <0x41000043>;
>> > > +				entry-latency-us = <240>;
>> > > +				exit-latency-us = <280>;
>> > > +				min-residency-us = <806>;
>> > > +			};
>> >
>> > I think my comment here is still open:
>> >
>> > This is strange, the deeper sleep state has lower timings than the
>> > previous one?
>> I was reordering based on Konrad comments when i have renamed the 
>> nodes
>> maybe it is not correct then.
>> I am searching for how to validate these levels, i have find these
>> https://git.codelinaro.org/clo/la/kernel/msm-4.9/-/blob/LA.UM.10.6.2.c26-01500-89xx.0/arch/arm64/boot/dts/qcom/msm8917-pm.dtsi#L45-91
> 
> I think you translated them correctly. It feels like downstream is 
> weird
> or even wrong here. Usually a higher psci-mode (retention = 2, gdhs = 
> 4)
> also implies a deeper idle state. But at some point the
> perf-l2-retention and perf-l2-gdhs state were swapped downstream:
> 
> https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/commit/dea262a17a9e80dacb86b7c2f269bcc7b4df3a13
> 
> I don't know if this is intended or just an oversight. If no one can
> clarify why this change was done I guess we can just choose between the
> following two options:
> 
>  1. Describe it exactly like it was done downstream. In that case I
>     would suggest swapping the node order back to what you had in v1.
>     Even if that means that a lower idle state has the higher psci-mode
>     (arm,psci-suspend-param). That should match what downstream did.
> 
> OR
> 
>  2. Omit cluster-sleep-0 and cluster-sleep-1. I doubt anyone will 
> notice
>     the minor difference in power consumption. The most important idle
>     state is the deepest "power collapse" (PC) state.
> 
> @Konrad: Do you have any opinion here?
> 
>> Do you know where can i find psci-suspend-param-s?
> 
> You need to translate it like in this code here:
> https://git.codelinaro.org/clo/la/kernel/msm-4.9/-/blob/LA.UM.10.6.2.c26-01500-89xx.0/drivers/cpuidle/lpm-levels.c#L1337-1340
> 
> Roughly described:
>  - Set BIT(30) if the CPU state has qcom,is-reset
>  - Affinity level is the hierarchy level that goes idle.
>    In your case: CPU = 0, L2 cache/cluster = 1.
>    Shift that to bit 24 (1 << 24 for cache/cluster)
>  - For the state itself you need to combine the qcom,psci-cpu-mode and
>    qcom,psci-mode according to the qcom,psci-mode-shift.
> 
> E.g. for the "perf-l2-pc" state, combined with the deepest CPU state
> ("pc"):
> 
>  - BIT(30) is set because of qcom,is-reset
>  - (1 << 24) because it's a L2 cache/cluster idle state
>  - (qcom,psci-cpu-mode = <3>) << (qcom,psci-mode-shift = <0>) = (3 << 
> 0)
>  - (qcom,psci-mode = <5>) << (qcom,psci-mode-shift = <4>) = (5 << 4)
> 
> All that combined: BIT(30) | (1 << 24) | (3 << 0) | (5 << 4)
>   = 0x41000053
> 
Thanks a lot this is a very useful description.
> Which is what you have for cluster-sleep-2. The ones you have look
> correct to me. :-)
> 
>> Should I also add wfi level?
> 
> I think we usually omit those for the CPU at least. Not sure about the
> cache/cluster one. As I mentioned, at the end the most important idle
> state to have is the deepest ones. Those will get used during suspend
> and when you don't use the device. The others are more minor
> optimization for light usage, which will be less noticeable.
> 
> Thanks,
> Stephan