Re: [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support

[Sudeep Holla <sudeep.holla@arm.com>]

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