| Message ID | 20190506193115.20909-1-niklas.cassel@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: dts: qcom: qcs404: Add PSCI cpuidle support | expand |
On 06-05-19, 21:31, Niklas Cassel wrote: > Add device bindings for CPUs to suspend using PSCI as the enable-method. > > Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org> > --- > arch/arm64/boot/dts/qcom/qcs404.dtsi | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi > index ffedf9640af7..f9db9f3ee10c 100644 > --- a/arch/arm64/boot/dts/qcom/qcs404.dtsi > +++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi > @@ -31,6 +31,7 @@ > reg = <0x100>; > enable-method = "psci"; > next-level-cache = <&L2_0>; > + cpu-idle-states = <&CPU_PC>; > }; > > CPU1: cpu@101 { > @@ -39,6 +40,7 @@ > reg = <0x101>; > enable-method = "psci"; > next-level-cache = <&L2_0>; > + cpu-idle-states = <&CPU_PC>; > }; > > CPU2: cpu@102 { > @@ -47,6 +49,7 @@ > reg = <0x102>; > enable-method = "psci"; > next-level-cache = <&L2_0>; > + cpu-idle-states = <&CPU_PC>; > }; > > CPU3: cpu@103 { > @@ -55,12 +58,24 @@ > reg = <0x103>; > enable-method = "psci"; > next-level-cache = <&L2_0>; > + cpu-idle-states = <&CPU_PC>; > }; > > L2_0: l2-cache { > compatible = "cache"; > cache-level = <2>; > }; > + > + idle-states { Since we are trying to sort the file per address and alphabetically, it would be great if this can be moved before l2-cache :) Other than that this lgtm > + CPU_PC: pc { > + compatible = "arm,idle-state"; > + arm,psci-suspend-param = <0x40000003>; > + entry-latency-us = <125>; > + exit-latency-us = <180>; > + min-residency-us = <595>; > + local-timer-stop; > + }; > + }; > }; > > firmware { > -- > 2.21.0
On Mon 06 May 22:35 PDT 2019, Vinod Koul wrote: > On 06-05-19, 21:31, Niklas Cassel wrote: > > Add device bindings for CPUs to suspend using PSCI as the enable-method. > > > > Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org> > > --- > > arch/arm64/boot/dts/qcom/qcs404.dtsi | 15 +++++++++++++++ > > 1 file changed, 15 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi > > index ffedf9640af7..f9db9f3ee10c 100644 > > --- a/arch/arm64/boot/dts/qcom/qcs404.dtsi > > +++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi > > @@ -31,6 +31,7 @@ > > reg = <0x100>; > > enable-method = "psci"; > > next-level-cache = <&L2_0>; > > + cpu-idle-states = <&CPU_PC>; > > }; > > > > CPU1: cpu@101 { > > @@ -39,6 +40,7 @@ > > reg = <0x101>; > > enable-method = "psci"; > > next-level-cache = <&L2_0>; > > + cpu-idle-states = <&CPU_PC>; > > }; > > > > CPU2: cpu@102 { > > @@ -47,6 +49,7 @@ > > reg = <0x102>; > > enable-method = "psci"; > > next-level-cache = <&L2_0>; > > + cpu-idle-states = <&CPU_PC>; > > }; > > > > CPU3: cpu@103 { > > @@ -55,12 +58,24 @@ > > reg = <0x103>; > > enable-method = "psci"; > > next-level-cache = <&L2_0>; > > + cpu-idle-states = <&CPU_PC>; > > }; > > > > L2_0: l2-cache { > > compatible = "cache"; > > cache-level = <2>; > > }; > > + > > + idle-states { > > Since we are trying to sort the file per address and > alphabetically, it would be great if this can be moved before l2-cache > :) > Picked up, with the order adjusted. > Other than that this lgtm > I presume that lgtm == Reviewed-by... Thanks, Bjorn > > + CPU_PC: pc { > > + compatible = "arm,idle-state"; > > + arm,psci-suspend-param = <0x40000003>; > > + entry-latency-us = <125>; > > + exit-latency-us = <180>; > > + min-residency-us = <595>; > > + local-timer-stop; > > + }; > > + }; > > }; > > > > firmware { > > -- > > 2.21.0 > > -- > ~Vinod
On 06-05-19, 23:55, Bjorn Andersson wrote: > On Mon 06 May 22:35 PDT 2019, Vinod Koul wrote: > > > On 06-05-19, 21:31, Niklas Cassel wrote: > > > Add device bindings for CPUs to suspend using PSCI as the enable-method. > > > > > > Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org> > > > --- > > > arch/arm64/boot/dts/qcom/qcs404.dtsi | 15 +++++++++++++++ > > > 1 file changed, 15 insertions(+) > > > > > > diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi > > > index ffedf9640af7..f9db9f3ee10c 100644 > > > --- a/arch/arm64/boot/dts/qcom/qcs404.dtsi > > > +++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi > > > @@ -31,6 +31,7 @@ > > > reg = <0x100>; > > > enable-method = "psci"; > > > next-level-cache = <&L2_0>; > > > + cpu-idle-states = <&CPU_PC>; > > > }; > > > > > > CPU1: cpu@101 { > > > @@ -39,6 +40,7 @@ > > > reg = <0x101>; > > > enable-method = "psci"; > > > next-level-cache = <&L2_0>; > > > + cpu-idle-states = <&CPU_PC>; > > > }; > > > > > > CPU2: cpu@102 { > > > @@ -47,6 +49,7 @@ > > > reg = <0x102>; > > > enable-method = "psci"; > > > next-level-cache = <&L2_0>; > > > + cpu-idle-states = <&CPU_PC>; > > > }; > > > > > > CPU3: cpu@103 { > > > @@ -55,12 +58,24 @@ > > > reg = <0x103>; > > > enable-method = "psci"; > > > next-level-cache = <&L2_0>; > > > + cpu-idle-states = <&CPU_PC>; > > > }; > > > > > > L2_0: l2-cache { > > > compatible = "cache"; > > > cache-level = <2>; > > > }; > > > + > > > + idle-states { > > > > Since we are trying to sort the file per address and > > alphabetically, it would be great if this can be moved before l2-cache > > :) > > > > Picked up, with the order adjusted. > > > Other than that this lgtm > > > > I presume that lgtm == Reviewed-by... yes :-) and formally here as well.. Reviewed-by: Vinod Koul <vkoul@kernel.org> > > Thanks, > Bjorn > > > > + CPU_PC: pc { > > > + compatible = "arm,idle-state"; > > > + arm,psci-suspend-param = <0x40000003>; > > > + entry-latency-us = <125>; > > > + exit-latency-us = <180>; > > > + min-residency-us = <595>; > > > + local-timer-stop; > > > + }; > > > + }; > > > }; > > > > > > firmware { > > > -- > > > 2.21.0 > > > > -- > > ~Vinod
On Tue, May 7, 2019 at 1:01 AM Niklas Cassel <niklas.cassel@linaro.org> wrote: > > Add device bindings for CPUs to suspend using PSCI as the enable-method. > > Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org> > --- > arch/arm64/boot/dts/qcom/qcs404.dtsi | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi > index ffedf9640af7..f9db9f3ee10c 100644 > --- a/arch/arm64/boot/dts/qcom/qcs404.dtsi > +++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi > @@ -31,6 +31,7 @@ > reg = <0x100>; > enable-method = "psci"; > next-level-cache = <&L2_0>; > + cpu-idle-states = <&CPU_PC>; > }; > > CPU1: cpu@101 { > @@ -39,6 +40,7 @@ > reg = <0x101>; > enable-method = "psci"; > next-level-cache = <&L2_0>; > + cpu-idle-states = <&CPU_PC>; > }; > > CPU2: cpu@102 { > @@ -47,6 +49,7 @@ > reg = <0x102>; > enable-method = "psci"; > next-level-cache = <&L2_0>; > + cpu-idle-states = <&CPU_PC>; > }; > > CPU3: cpu@103 { > @@ -55,12 +58,24 @@ > reg = <0x103>; > enable-method = "psci"; > next-level-cache = <&L2_0>; > + cpu-idle-states = <&CPU_PC>; > }; > > L2_0: l2-cache { > compatible = "cache"; > cache-level = <2>; > }; > + > + idle-states { entry-method="psci" property goes here. I have a patch fixing it for 410c ;-) I don't think the psci_cpuidle_ops will even get called without this. Did you see any changes in consumption with this patch? I was trying to measure that before sending this out. > + CPU_PC: pc { > + compatible = "arm,idle-state"; > + arm,psci-suspend-param = <0x40000003>; > + entry-latency-us = <125>; > + exit-latency-us = <180>; > + min-residency-us = <595>; > + local-timer-stop; > + }; > + }; > }; > > firmware { > -- > 2.21.0 >
On Wed, May 08, 2019 at 02:48:19AM +0530, Amit Kucheria wrote: > On Tue, May 7, 2019 at 1:01 AM Niklas Cassel <niklas.cassel@linaro.org> wrote: > > > > Add device bindings for CPUs to suspend using PSCI as the enable-method. > > > > Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org> > > --- > > arch/arm64/boot/dts/qcom/qcs404.dtsi | 15 +++++++++++++++ > > 1 file changed, 15 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi > > index ffedf9640af7..f9db9f3ee10c 100644 > > --- a/arch/arm64/boot/dts/qcom/qcs404.dtsi > > +++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi > > @@ -31,6 +31,7 @@ > > reg = <0x100>; > > enable-method = "psci"; > > next-level-cache = <&L2_0>; > > + cpu-idle-states = <&CPU_PC>; > > }; > > > > CPU1: cpu@101 { > > @@ -39,6 +40,7 @@ > > reg = <0x101>; > > enable-method = "psci"; > > next-level-cache = <&L2_0>; > > + cpu-idle-states = <&CPU_PC>; > > }; > > > > CPU2: cpu@102 { > > @@ -47,6 +49,7 @@ > > reg = <0x102>; > > enable-method = "psci"; > > next-level-cache = <&L2_0>; > > + cpu-idle-states = <&CPU_PC>; > > }; > > > > CPU3: cpu@103 { > > @@ -55,12 +58,24 @@ > > reg = <0x103>; > > enable-method = "psci"; > > next-level-cache = <&L2_0>; > > + cpu-idle-states = <&CPU_PC>; > > }; > > > > L2_0: l2-cache { > > compatible = "cache"; > > cache-level = <2>; > > }; > > + > > + idle-states { > > entry-method="psci" property goes here. I have a patch fixing it for 410c ;-) > > I don't think the psci_cpuidle_ops will even get called without this. Hello Amit, I added debug prints in psci_cpu_suspend_enter() and arm_cpuidle_suspend() when verifying this patch, and psci_cpu_suspend_enter() is indeed called, with the correct psci suspend parameter. The output from: grep "" /sys/bus/cpu/devices/cpu0/cpuidle/state?/* also looks sane. However, if 'entry-method="psci"' is required according to the DT binding, perhaps you can send a 2/2 series that fixes both this patch and msm8916 ? > Did you see any changes in consumption with this patch? I was trying > to measure that before sending this out. I don't know of any way to measure the power consumption on this board, so no, I haven't been able to verify that the firmware actually does the right thing here. Kind regards, Niklas > > > + CPU_PC: pc { > > + compatible = "arm,idle-state"; > > + arm,psci-suspend-param = <0x40000003>; > > + entry-latency-us = <125>; > > + exit-latency-us = <180>; > > + min-residency-us = <595>; > > + local-timer-stop; > > + }; > > + }; > > }; > > > > firmware { > > -- > > 2.21.0 > >
(Adding Lorenzo and Sudeep) On Wed, May 8, 2019 at 8:26 PM Niklas Cassel <niklas.cassel@linaro.org> wrote: > > On Wed, May 08, 2019 at 02:48:19AM +0530, Amit Kucheria wrote: > > On Tue, May 7, 2019 at 1:01 AM Niklas Cassel <niklas.cassel@linaro.org> wrote: > > > > > > Add device bindings for CPUs to suspend using PSCI as the enable-method. > > > > > > Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org> > > > --- > > > arch/arm64/boot/dts/qcom/qcs404.dtsi | 15 +++++++++++++++ > > > 1 file changed, 15 insertions(+) > > > > > > diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi > > > index ffedf9640af7..f9db9f3ee10c 100644 > > > --- a/arch/arm64/boot/dts/qcom/qcs404.dtsi > > > +++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi > > > @@ -31,6 +31,7 @@ > > > reg = <0x100>; > > > enable-method = "psci"; > > > next-level-cache = <&L2_0>; > > > + cpu-idle-states = <&CPU_PC>; > > > }; > > > > > > CPU1: cpu@101 { > > > @@ -39,6 +40,7 @@ > > > reg = <0x101>; > > > enable-method = "psci"; > > > next-level-cache = <&L2_0>; > > > + cpu-idle-states = <&CPU_PC>; > > > }; > > > > > > CPU2: cpu@102 { > > > @@ -47,6 +49,7 @@ > > > reg = <0x102>; > > > enable-method = "psci"; > > > next-level-cache = <&L2_0>; > > > + cpu-idle-states = <&CPU_PC>; > > > }; > > > > > > CPU3: cpu@103 { > > > @@ -55,12 +58,24 @@ > > > reg = <0x103>; > > > enable-method = "psci"; > > > next-level-cache = <&L2_0>; > > > + cpu-idle-states = <&CPU_PC>; > > > }; > > > > > > L2_0: l2-cache { > > > compatible = "cache"; > > > cache-level = <2>; > > > }; > > > + > > > + idle-states { > > > > entry-method="psci" property goes here. I have a patch fixing it for 410c ;-) > > > > I don't think the psci_cpuidle_ops will even get called without this. > > Hello Amit, > > I added debug prints in psci_cpu_suspend_enter() and arm_cpuidle_suspend() > when verifying this patch, and psci_cpu_suspend_enter() is indeed called, > with the correct psci suspend parameter. > > The output from: > grep "" /sys/bus/cpu/devices/cpu0/cpuidle/state?/* > also looks sane. > > However, if 'entry-method="psci"' is required according to the DT binding, > perhaps you can send a 2/2 series that fixes both this patch and msm8916 ? Last time I discussed this with Lorenzo and Sudeep (on IRC), I pointed out that entry-method="psci" isn't checked for in code anywhere. Let's get their view on this for posterity. What does entry-method="psci" in the idle-states node achieve that enable-method="psci" in the cpu node doesn't achieve? (Note: enable- vs. entry-). The enable-method property is the one that sets up the psci_cpuidle_ops callbacks through the CPUIDLE_METHOD_OF_DECLARE macro. IOW, if we deprecated the entry-method property, everything would still work, wouldn't it? Do we expect to support PSCI platforms that might have a different entry-method for idle states? Should I whip up a patch removing entry-method? Since we don't check for it today, it won't break the old DTs either. Regards, Amit > > Did you see any changes in consumption with this patch? I was trying > > to measure that before sending this out. > > I don't know of any way to measure the power consumption on this board, > so no, I haven't been able to verify that the firmware actually does > the right thing here. > > > Kind regards, > Niklas > > > > > > + CPU_PC: pc { > > > + compatible = "arm,idle-state"; > > > + arm,psci-suspend-param = <0x40000003>; > > > + entry-latency-us = <125>; > > > + exit-latency-us = <180>; > > > + min-residency-us = <595>; > > > + local-timer-stop; > > > + }; > > > + }; > > > }; > > > > > > firmware { > > > -- > > > 2.21.0 > > >
On Thu, May 09, 2019 at 11:19:23PM +0530, Amit Kucheria wrote: > (Adding Lorenzo and Sudeep) > > On Wed, May 8, 2019 at 8:26 PM Niklas Cassel <niklas.cassel@linaro.org> wrote: > > > > On Wed, May 08, 2019 at 02:48:19AM +0530, Amit Kucheria wrote: > > > On Tue, May 7, 2019 at 1:01 AM Niklas Cassel <niklas.cassel@linaro.org> wrote: > > > > > > > > Add device bindings for CPUs to suspend using PSCI as the enable-method. > > > > > > > > Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org> > > > > --- > > > > arch/arm64/boot/dts/qcom/qcs404.dtsi | 15 +++++++++++++++ > > > > 1 file changed, 15 insertions(+) > > > > > > > > diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi > > > > index ffedf9640af7..f9db9f3ee10c 100644 > > > > --- a/arch/arm64/boot/dts/qcom/qcs404.dtsi > > > > +++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi > > > > @@ -31,6 +31,7 @@ > > > > reg = <0x100>; > > > > enable-method = "psci"; > > > > next-level-cache = <&L2_0>; > > > > + cpu-idle-states = <&CPU_PC>; > > > > }; > > > > > > > > CPU1: cpu@101 { > > > > @@ -39,6 +40,7 @@ > > > > reg = <0x101>; > > > > enable-method = "psci"; > > > > next-level-cache = <&L2_0>; > > > > + cpu-idle-states = <&CPU_PC>; > > > > }; > > > > > > > > CPU2: cpu@102 { > > > > @@ -47,6 +49,7 @@ > > > > reg = <0x102>; > > > > enable-method = "psci"; > > > > next-level-cache = <&L2_0>; > > > > + cpu-idle-states = <&CPU_PC>; > > > > }; > > > > > > > > CPU3: cpu@103 { > > > > @@ -55,12 +58,24 @@ > > > > reg = <0x103>; > > > > enable-method = "psci"; > > > > next-level-cache = <&L2_0>; > > > > + cpu-idle-states = <&CPU_PC>; > > > > }; > > > > > > > > L2_0: l2-cache { > > > > compatible = "cache"; > > > > cache-level = <2>; > > > > }; > > > > + > > > > + idle-states { > > > > > > entry-method="psci" property goes here. I have a patch fixing it for 410c ;-) > > > > > > I don't think the psci_cpuidle_ops will even get called without this. > > > > Hello Amit, > > > > I added debug prints in psci_cpu_suspend_enter() and arm_cpuidle_suspend() > > when verifying this patch, and psci_cpu_suspend_enter() is indeed called, > > with the correct psci suspend parameter. > > > > The output from: > > grep "" /sys/bus/cpu/devices/cpu0/cpuidle/state?/* > > also looks sane. > > > > However, if 'entry-method="psci"' is required according to the DT binding, > > perhaps you can send a 2/2 series that fixes both this patch and msm8916 ? > > Last time I discussed this with Lorenzo and Sudeep (on IRC), I pointed > out that entry-method="psci" isn't checked for in code anywhere. Let's > get their view on this for posterity. > Yes entry-method="psci" is required as per DT binding but not checked in code on arm64. We have CPU ops with idle enabled only for "psci", so there's not need to check. Once we have DT schema validation, this will be caught, so it's better to fix it. > What does entry-method="psci" in the idle-states node achieve that > enable-method="psci" in the cpu node doesn't achieve? (Note: enable- > vs. entry-). > From DT binding perspective, we can have different CPU enable-method and CPU idle entry-method. However on arm64, it's restricted to PSCI only. I need to check what happens on arm32 though, as the driver invocation happens via CPUIDLE_METHOD_OF_DECLARE. > The enable-method property is the one that sets up the > psci_cpuidle_ops callbacks through the CPUIDLE_METHOD_OF_DECLARE > macro. > Indeed. > IOW, if we deprecated the entry-method property, everything would > still work, wouldn't it? Why do you want to deprecated just because Linux kernel doesn't want to use it. That's not a valid reason IMO. > Do we expect to support PSCI platforms that might have a different > entry-method for idle states? Not on ARM64, but same DT bindings can be used for idle-states on say RISC-V and have some value other than "psci". > Should I whip up a patch removing entry-method? Since we don't check > for it today, it won't break the old DTs either. > Nope, I don't think so. But if it's causing issues, we can look into it. I don't want to restrict the use of the bindings for ARM/ARM64 or psci only. -- Regards, Sudeep
On Tue, May 7, 2019 at 12:25 PM Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > > On Mon 06 May 22:35 PDT 2019, Vinod Koul wrote: > > > On 06-05-19, 21:31, Niklas Cassel wrote: > > > Add device bindings for CPUs to suspend using PSCI as the enable-method. > > > > > > Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org> > > > --- > > > arch/arm64/boot/dts/qcom/qcs404.dtsi | 15 +++++++++++++++ > > > 1 file changed, 15 insertions(+) > > > > > > diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi > > > index ffedf9640af7..f9db9f3ee10c 100644 > > > --- a/arch/arm64/boot/dts/qcom/qcs404.dtsi > > > +++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi > > > @@ -31,6 +31,7 @@ > > > reg = <0x100>; > > > enable-method = "psci"; > > > next-level-cache = <&L2_0>; > > > + cpu-idle-states = <&CPU_PC>; > > > }; > > > > > > CPU1: cpu@101 { > > > @@ -39,6 +40,7 @@ > > > reg = <0x101>; > > > enable-method = "psci"; > > > next-level-cache = <&L2_0>; > > > + cpu-idle-states = <&CPU_PC>; > > > }; > > > > > > CPU2: cpu@102 { > > > @@ -47,6 +49,7 @@ > > > reg = <0x102>; > > > enable-method = "psci"; > > > next-level-cache = <&L2_0>; > > > + cpu-idle-states = <&CPU_PC>; > > > }; > > > > > > CPU3: cpu@103 { > > > @@ -55,12 +58,24 @@ > > > reg = <0x103>; > > > enable-method = "psci"; > > > next-level-cache = <&L2_0>; > > > + cpu-idle-states = <&CPU_PC>; > > > }; > > > > > > L2_0: l2-cache { > > > compatible = "cache"; > > > cache-level = <2>; > > > }; > > > + > > > + idle-states { > > > > Since we are trying to sort the file per address and > > alphabetically, it would be great if this can be moved before l2-cache > > :) > > > > Picked up, with the order adjusted. > > > Other than that this lgtm > > > > I presume that lgtm == Reviewed-by... > > Thanks, > Bjorn Hi Bjorn, Please drop this patch and check the one I've sent as part of the qcom cpuidle series? Regards, Amit > > > + CPU_PC: pc { > > > + compatible = "arm,idle-state"; > > > + arm,psci-suspend-param = <0x40000003>; > > > + entry-latency-us = <125>; > > > + exit-latency-us = <180>; > > > + min-residency-us = <595>; > > > + local-timer-stop; > > > + }; > > > + }; > > > }; > > > > > > firmware { > > > -- > > > 2.21.0 > > > > -- > > ~Vinod
On Fri, May 10, 2019 at 2:54 PM Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Thu, May 09, 2019 at 11:19:23PM +0530, Amit Kucheria wrote: > > (Adding Lorenzo and Sudeep) > > > > On Wed, May 8, 2019 at 8:26 PM Niklas Cassel <niklas.cassel@linaro.org> wrote: > > > > > > On Wed, May 08, 2019 at 02:48:19AM +0530, Amit Kucheria wrote: > > > > On Tue, May 7, 2019 at 1:01 AM Niklas Cassel <niklas.cassel@linaro.org> wrote: > > > > > > > > > > Add device bindings for CPUs to suspend using PSCI as the enable-method. > > > > > > > > > > Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org> > > > > > --- > > > > > arch/arm64/boot/dts/qcom/qcs404.dtsi | 15 +++++++++++++++ > > > > > 1 file changed, 15 insertions(+) > > > > > > > > > > diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi > > > > > index ffedf9640af7..f9db9f3ee10c 100644 > > > > > --- a/arch/arm64/boot/dts/qcom/qcs404.dtsi > > > > > +++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi > > > > > @@ -31,6 +31,7 @@ > > > > > reg = <0x100>; > > > > > enable-method = "psci"; > > > > > next-level-cache = <&L2_0>; > > > > > + cpu-idle-states = <&CPU_PC>; > > > > > }; > > > > > > > > > > CPU1: cpu@101 { > > > > > @@ -39,6 +40,7 @@ > > > > > reg = <0x101>; > > > > > enable-method = "psci"; > > > > > next-level-cache = <&L2_0>; > > > > > + cpu-idle-states = <&CPU_PC>; > > > > > }; > > > > > > > > > > CPU2: cpu@102 { > > > > > @@ -47,6 +49,7 @@ > > > > > reg = <0x102>; > > > > > enable-method = "psci"; > > > > > next-level-cache = <&L2_0>; > > > > > + cpu-idle-states = <&CPU_PC>; > > > > > }; > > > > > > > > > > CPU3: cpu@103 { > > > > > @@ -55,12 +58,24 @@ > > > > > reg = <0x103>; > > > > > enable-method = "psci"; > > > > > next-level-cache = <&L2_0>; > > > > > + cpu-idle-states = <&CPU_PC>; > > > > > }; > > > > > > > > > > L2_0: l2-cache { > > > > > compatible = "cache"; > > > > > cache-level = <2>; > > > > > }; > > > > > + > > > > > + idle-states { > > > > > > > > entry-method="psci" property goes here. I have a patch fixing it for 410c ;-) > > > > > > > > I don't think the psci_cpuidle_ops will even get called without this. > > > > > > Hello Amit, > > > > > > I added debug prints in psci_cpu_suspend_enter() and arm_cpuidle_suspend() > > > when verifying this patch, and psci_cpu_suspend_enter() is indeed called, > > > with the correct psci suspend parameter. > > > > > > The output from: > > > grep "" /sys/bus/cpu/devices/cpu0/cpuidle/state?/* > > > also looks sane. > > > > > > However, if 'entry-method="psci"' is required according to the DT binding, > > > perhaps you can send a 2/2 series that fixes both this patch and msm8916 ? > > > > Last time I discussed this with Lorenzo and Sudeep (on IRC), I pointed > > out that entry-method="psci" isn't checked for in code anywhere. Let's > > get their view on this for posterity. > > > > Yes entry-method="psci" is required as per DT binding but not checked > in code on arm64. We have CPU ops with idle enabled only for "psci", so > there's not need to check. I don't see it being checked on arm32 either. > Once we have DT schema validation, this will be caught, so it's better > to fix it. > > > What does entry-method="psci" in the idle-states node achieve that > > enable-method="psci" in the cpu node doesn't achieve? (Note: enable- > > vs. entry-). > > > > From DT binding perspective, we can have different CPU enable-method > and CPU idle entry-method. However on arm64, it's restricted to PSCI > only. I need to check what happens on arm32 though, as the driver > invocation happens via CPUIDLE_METHOD_OF_DECLARE. > > > The enable-method property is the one that sets up the > > psci_cpuidle_ops callbacks through the CPUIDLE_METHOD_OF_DECLARE > > macro. > > > > Indeed. > > > IOW, if we deprecated the entry-method property, everything would > > still work, wouldn't it? > > Why do you want to deprecated just because Linux kernel doesn't want to > use it. That's not a valid reason IMO. Fair enough. Just want to make sure that it isn't some vestigial property that was never used. Do you know if another OS is actually using it? > > Do we expect to support PSCI platforms that might have a different > > entry-method for idle states? > > Not on ARM64, but same DT bindings can be used for idle-states on > say RISC-V and have some value other than "psci". Both enable-method and entry-method properties are currently only used (and documented) for ARM platforms. Hence this discussion about deprecation of one of them. > > Should I whip up a patch removing entry-method? Since we don't check > > for it today, it won't break the old DTs either. > > > > Nope, I don't think so. But if it's causing issues, we can look into it. > I don't want to restrict the use of the bindings for ARM/ARM64 or psci only. Only a couple of minor issues: 1. There is a trickle of DTs that need fixing up every now and then because they don't use entry-method in their idle-states node. Schema validation ought to fix that. 2. A property that isn't ready by any code is a bit confusing. Perhaps we can mention something to the effect in the documentation? Regards, Amit
On Fri, May 10, 2019 at 11:58:40PM +0530, Amit Kucheria wrote: > On Fri, May 10, 2019 at 2:54 PM Sudeep Holla <sudeep.holla@arm.com> wrote: > > [...] > > > > Yes entry-method="psci" is required as per DT binding but not checked > > in code on arm64. We have CPU ops with idle enabled only for "psci", so > > there's not need to check. > > I don't see it being checked on arm32 either. > arm_cpuidle_get_ops in arch/arm/kernel/cpuidle.c checks the method, has to match "psci" for drivers/firmware/psci.c to work on arm32 [...] > > > > Why do you want to deprecated just because Linux kernel doesn't want to > > use it. That's not a valid reason IMO. > > Fair enough. Just want to make sure that it isn't some vestigial > property that was never used. Do you know if another OS is actually > using it? > Not that I am aware of. But Linux uses it on arm32, so it's not entirely unused. > > > Do we expect to support PSCI platforms that might have a different > > > entry-method for idle states? > > > > Not on ARM64, but same DT bindings can be used for idle-states on > > say RISC-V and have some value other than "psci". > > Both enable-method and entry-method properties are currently only used > (and documented) for ARM platforms. Hence this discussion about > deprecation of one of them. > Yes, it's used on arm32 as mentioned above. > > > Should I whip up a patch removing entry-method? Since we don't check > > > for it today, it won't break the old DTs either. > > > > > > > Nope, I don't think so. But if it's causing issues, we can look into it. > > I don't want to restrict the use of the bindings for ARM/ARM64 or psci only. > > Only a couple of minor issues: > 1. There is a trickle of DTs that need fixing up every now and then > because they don't use entry-method in their idle-states node. Schema > validation ought to fix that. I understand, scheme should fix it. This is not just restricted to this, it's generic DT problem. So let's hope we get schema based validation soon. > 2. A property that isn't ready by any code is a bit confusing. Perhaps > we can mention something to the effect in the documentation? > Not entirely true. We have quite a lot of bindings that are added just because downstream drivers use e.g. GPU and even standard ePAPR or DT specification has lots of bindings which OS like Linux may choose not to use at all. Same applies to ACPI, so I am not for removing bindings just because there are no users in Linux. -- Regards, Sudeep
On Mon, May 13, 2019 at 3:19 PM Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Fri, May 10, 2019 at 11:58:40PM +0530, Amit Kucheria wrote: > > On Fri, May 10, 2019 at 2:54 PM Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > > [...] > > > > > > > Yes entry-method="psci" is required as per DT binding but not checked > > > in code on arm64. We have CPU ops with idle enabled only for "psci", so > > > there's not need to check. > > > > I don't see it being checked on arm32 either. > > > > arm_cpuidle_get_ops in arch/arm/kernel/cpuidle.c checks the method, has > to match "psci" for drivers/firmware/psci.c to work on arm32 That is a check for the enable-method, not entry-method. We don't check for entry-method anywhere, AFAICT. > [...] > > > > > > > Why do you want to deprecated just because Linux kernel doesn't want to > > > use it. That's not a valid reason IMO. > > > > Fair enough. Just want to make sure that it isn't some vestigial > > property that was never used. Do you know if another OS is actually > > using it? > > > > Not that I am aware of. But Linux uses it on arm32, so it's not entirely > unused. entry-method is not read in Linux code (see above). > > > > Do we expect to support PSCI platforms that might have a different > > > > entry-method for idle states? > > > > > > Not on ARM64, but same DT bindings can be used for idle-states on > > > say RISC-V and have some value other than "psci". > > > > Both enable-method and entry-method properties are currently only used > > (and documented) for ARM platforms. Hence this discussion about > > deprecation of one of them. > > > > Yes, it's used on arm32 as mentioned above. Only enable-method is checked. > > > > Should I whip up a patch removing entry-method? Since we don't check > > > > for it today, it won't break the old DTs either. > > > > > > > > > > Nope, I don't think so. But if it's causing issues, we can look into it. > > > I don't want to restrict the use of the bindings for ARM/ARM64 or psci only. > > > > Only a couple of minor issues: > > 1. There is a trickle of DTs that need fixing up every now and then > > because they don't use entry-method in their idle-states node. Schema > > validation ought to fix that. > > I understand, scheme should fix it. This is not just restricted to this, > it's generic DT problem. So let's hope we get schema based validation soon. > > > 2. A property that isn't ready by any code is a bit confusing. Perhaps > > we can mention something to the effect in the documentation? > > > > Not entirely true. We have quite a lot of bindings that are added just > because downstream drivers use e.g. GPU and even standard ePAPR or DT > specification has lots of bindings which OS like Linux may choose > not to use at all. Same applies to ACPI, so I am not for removing bindings > just because there are no users in Linux. That is a fair point. But in those cases, the binding is probably used by another OS. entry-method seems to an example of one that isn't used by Linux or other OSes. Regards, Amit
On Wed, May 15, 2019 at 03:26:14PM +0530, Amit Kucheria wrote: > On Mon, May 13, 2019 at 3:19 PM Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > On Fri, May 10, 2019 at 11:58:40PM +0530, Amit Kucheria wrote: > > > On Fri, May 10, 2019 at 2:54 PM Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > > > > > [...] > > > > > > > > > > Yes entry-method="psci" is required as per DT binding but not checked > > > > in code on arm64. We have CPU ops with idle enabled only for "psci", so > > > > there's not need to check. > > > > > > I don't see it being checked on arm32 either. > > > > > > > arm_cpuidle_get_ops in arch/arm/kernel/cpuidle.c checks the method, has > > to match "psci" for drivers/firmware/psci.c to work on arm32 > > That is a check for the enable-method, not entry-method. > > We don't check for entry-method anywhere, AFAICT. > Ah, you right. My eyes can't distinguish between entry-method and enable-method anymore :( -- Regards, Sudeep
diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi index ffedf9640af7..f9db9f3ee10c 100644 --- a/arch/arm64/boot/dts/qcom/qcs404.dtsi +++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi @@ -31,6 +31,7 @@ reg = <0x100>; enable-method = "psci"; next-level-cache = <&L2_0>; + cpu-idle-states = <&CPU_PC>; }; CPU1: cpu@101 { @@ -39,6 +40,7 @@ reg = <0x101>; enable-method = "psci"; next-level-cache = <&L2_0>; + cpu-idle-states = <&CPU_PC>; }; CPU2: cpu@102 { @@ -47,6 +49,7 @@ reg = <0x102>; enable-method = "psci"; next-level-cache = <&L2_0>; + cpu-idle-states = <&CPU_PC>; }; CPU3: cpu@103 { @@ -55,12 +58,24 @@ reg = <0x103>; enable-method = "psci"; next-level-cache = <&L2_0>; + cpu-idle-states = <&CPU_PC>; }; L2_0: l2-cache { compatible = "cache"; cache-level = <2>; }; + + idle-states { + CPU_PC: pc { + compatible = "arm,idle-state"; + arm,psci-suspend-param = <0x40000003>; + entry-latency-us = <125>; + exit-latency-us = <180>; + min-residency-us = <595>; + local-timer-stop; + }; + }; }; firmware {
Add device bindings for CPUs to suspend using PSCI as the enable-method. Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org> --- arch/arm64/boot/dts/qcom/qcs404.dtsi | 15 +++++++++++++++ 1 file changed, 15 insertions(+)