* [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support @ 2019-05-06 19:31 Niklas Cassel 2019-05-07 5:35 ` Vinod Koul 2019-05-07 21:18 ` Amit Kucheria 0 siblings, 2 replies; 13+ messages in thread From: Niklas Cassel @ 2019-05-06 19:31 UTC (permalink / raw) To: Andy Gross, David Brown, Rob Herring, Mark Rutland Cc: amit.kucheria, jorge.ramirez-ortiz, lina.iyer, ulf.hansson, bjorn.andersson, Niklas Cassel, linux-arm-msm, devicetree, linux-kernel 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 { + 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support 2019-05-06 19:31 [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support Niklas Cassel @ 2019-05-07 5:35 ` Vinod Koul 2019-05-07 6:55 ` Bjorn Andersson 2019-05-07 21:18 ` Amit Kucheria 1 sibling, 1 reply; 13+ messages in thread From: Vinod Koul @ 2019-05-07 5:35 UTC (permalink / raw) To: Niklas Cassel Cc: Andy Gross, David Brown, Rob Herring, Mark Rutland, amit.kucheria, jorge.ramirez-ortiz, lina.iyer, ulf.hansson, bjorn.andersson, linux-arm-msm, devicetree, linux-kernel 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 -- ~Vinod ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support 2019-05-07 5:35 ` Vinod Koul @ 2019-05-07 6:55 ` Bjorn Andersson 2019-05-07 7:06 ` Vinod Koul 2019-05-10 11:31 ` Amit Kucheria 0 siblings, 2 replies; 13+ messages in thread From: Bjorn Andersson @ 2019-05-07 6:55 UTC (permalink / raw) To: Vinod Koul Cc: Niklas Cassel, Andy Gross, David Brown, Rob Herring, Mark Rutland, amit.kucheria, jorge.ramirez-ortiz, lina.iyer, ulf.hansson, linux-arm-msm, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support 2019-05-07 6:55 ` Bjorn Andersson @ 2019-05-07 7:06 ` Vinod Koul 2019-05-10 11:31 ` Amit Kucheria 1 sibling, 0 replies; 13+ messages in thread From: Vinod Koul @ 2019-05-07 7:06 UTC (permalink / raw) To: Bjorn Andersson Cc: Niklas Cassel, Andy Gross, David Brown, Rob Herring, Mark Rutland, amit.kucheria, jorge.ramirez-ortiz, lina.iyer, ulf.hansson, linux-arm-msm, devicetree, linux-kernel 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 -- ~Vinod ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support 2019-05-07 6:55 ` Bjorn Andersson 2019-05-07 7:06 ` Vinod Koul @ 2019-05-10 11:31 ` Amit Kucheria 1 sibling, 0 replies; 13+ messages in thread From: Amit Kucheria @ 2019-05-10 11:31 UTC (permalink / raw) To: Bjorn Andersson Cc: Vinod Koul, Niklas Cassel, Andy Gross, David Brown, Rob Herring, Mark Rutland, Jorge Ramirez-Ortiz, Lina Iyer, Ulf Hansson, linux-arm-msm, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support 2019-05-06 19:31 [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support Niklas Cassel 2019-05-07 5:35 ` Vinod Koul @ 2019-05-07 21:18 ` Amit Kucheria 2019-05-08 14:56 ` Niklas Cassel 1 sibling, 1 reply; 13+ messages in thread From: Amit Kucheria @ 2019-05-07 21:18 UTC (permalink / raw) To: Niklas Cassel Cc: Andy Gross, David Brown, Rob Herring, Mark Rutland, Jorge Ramirez-Ortiz, Lina Iyer, Ulf Hansson, Bjorn Andersson, linux-arm-msm, DTML, Linux Kernel Mailing List 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 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support 2019-05-07 21:18 ` Amit Kucheria @ 2019-05-08 14:56 ` Niklas Cassel 2019-05-09 17:49 ` Amit Kucheria 0 siblings, 1 reply; 13+ messages in thread From: Niklas Cassel @ 2019-05-08 14:56 UTC (permalink / raw) To: Amit Kucheria Cc: Andy Gross, David Brown, Rob Herring, Mark Rutland, Jorge Ramirez-Ortiz, Lina Iyer, Ulf Hansson, Bjorn Andersson, linux-arm-msm, DTML, Linux Kernel Mailing List 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 > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support 2019-05-08 14:56 ` Niklas Cassel @ 2019-05-09 17:49 ` Amit Kucheria 2019-05-10 9:24 ` Sudeep Holla 0 siblings, 1 reply; 13+ messages in thread From: Amit Kucheria @ 2019-05-09 17:49 UTC (permalink / raw) To: Niklas Cassel, Lorenzo Pieralisi, Sudeep Holla Cc: Andy Gross, David Brown, Rob Herring, Mark Rutland, Jorge Ramirez-Ortiz, Lina Iyer, Ulf Hansson, Bjorn Andersson, linux-arm-msm, DTML, Linux Kernel Mailing List (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 > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support 2019-05-09 17:49 ` Amit Kucheria @ 2019-05-10 9:24 ` Sudeep Holla 2019-05-10 18:28 ` Amit Kucheria 0 siblings, 1 reply; 13+ messages in thread From: Sudeep Holla @ 2019-05-10 9:24 UTC (permalink / raw) To: Amit Kucheria Cc: Niklas Cassel, Lorenzo Pieralisi, Andy Gross, David Brown, Rob Herring, Mark Rutland, Jorge Ramirez-Ortiz, Lina Iyer, Ulf Hansson, Bjorn Andersson, linux-arm-msm, DTML, Sudeep Holla, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support 2019-05-10 9:24 ` Sudeep Holla @ 2019-05-10 18:28 ` Amit Kucheria 2019-05-13 9:49 ` Sudeep Holla 0 siblings, 1 reply; 13+ messages in thread From: Amit Kucheria @ 2019-05-10 18:28 UTC (permalink / raw) To: Sudeep Holla Cc: Niklas Cassel, Lorenzo Pieralisi, Andy Gross, David Brown, Rob Herring, Mark Rutland, Jorge Ramirez-Ortiz, Lina Iyer, Ulf Hansson, Bjorn Andersson, linux-arm-msm, DTML, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support 2019-05-10 18:28 ` Amit Kucheria @ 2019-05-13 9:49 ` Sudeep Holla 2019-05-15 9:56 ` Amit Kucheria 0 siblings, 1 reply; 13+ messages in thread From: Sudeep Holla @ 2019-05-13 9:49 UTC (permalink / raw) To: Amit Kucheria Cc: Niklas Cassel, Lorenzo Pieralisi, Andy Gross, David Brown, Rob Herring, Mark Rutland, Jorge Ramirez-Ortiz, Lina Iyer, Ulf Hansson, Bjorn Andersson, linux-arm-msm, DTML, Sudeep Holla, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support 2019-05-13 9:49 ` Sudeep Holla @ 2019-05-15 9:56 ` Amit Kucheria 2019-05-15 10:30 ` Sudeep Holla 0 siblings, 1 reply; 13+ messages in thread From: Amit Kucheria @ 2019-05-15 9:56 UTC (permalink / raw) To: Sudeep Holla Cc: Niklas Cassel, Lorenzo Pieralisi, Andy Gross, David Brown, Rob Herring, Mark Rutland, Jorge Ramirez-Ortiz, Lina Iyer, Ulf Hansson, Bjorn Andersson, linux-arm-msm, DTML, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support 2019-05-15 9:56 ` Amit Kucheria @ 2019-05-15 10:30 ` Sudeep Holla 0 siblings, 0 replies; 13+ messages in thread From: Sudeep Holla @ 2019-05-15 10:30 UTC (permalink / raw) To: Amit Kucheria Cc: Niklas Cassel, Lorenzo Pieralisi, Andy Gross, David Brown, Rob Herring, Mark Rutland, Jorge Ramirez-Ortiz, Lina Iyer, Ulf Hansson, Bjorn Andersson, linux-arm-msm, DTML, Linux Kernel Mailing List, Sudeep Holla 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-05-15 10:30 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-06 19:31 [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support Niklas Cassel 2019-05-07 5:35 ` Vinod Koul 2019-05-07 6:55 ` Bjorn Andersson 2019-05-07 7:06 ` Vinod Koul 2019-05-10 11:31 ` Amit Kucheria 2019-05-07 21:18 ` Amit Kucheria 2019-05-08 14:56 ` Niklas Cassel 2019-05-09 17:49 ` Amit Kucheria 2019-05-10 9:24 ` Sudeep Holla 2019-05-10 18:28 ` Amit Kucheria 2019-05-13 9:49 ` Sudeep Holla 2019-05-15 9:56 ` Amit Kucheria 2019-05-15 10:30 ` Sudeep Holla
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).