Re: [PATCH v3 10/13] cpuidle: psci: Prepare to use OS initiated suspend mode via PM domains

From: Ulf Hansson <ulf.hansson@linaro.org>
To: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Sudeep Holla <sudeep.holla@arm.com>,
	Rob Herring <robh+dt@kernel.org>,
	Linux PM <linux-pm@vger.kernel.org>,
	"Rafael J . Wysocki" <rjw@rjwysocki.net>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Lina Iyer <ilina@codeaurora.org>,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Stephen Boyd <sboyd@kernel.org>, Andy Gross <agross@kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Kevin Hilman <khilman@kernel.org>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	Lina Iyer <lina.iyer@linaro.org>
Subject: Re: [PATCH v3 10/13] cpuidle: psci: Prepare to use OS initiated suspend mode via PM domains
Date: Thu, 5 Dec 2019 21:38:45 +0100	[thread overview]
Message-ID: <CAPDyKFpRLWO0D9an_OH3VJYDXM=SQWDfx0A4tOhufA+_L2Zp-g@mail.gmail.com> (raw)
In-Reply-To: <CAPDyKFra-C_EKrcec6Yys2P10bB+KBtVAKNtVFgqDvV=tzbDRQ@mail.gmail.com>

On Thu, 5 Dec 2019 at 21:25, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Thu, 5 Dec 2019 at 19:35, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> >
> > On Wed, Nov 27, 2019 at 11:29:11AM +0100, Ulf Hansson wrote:
> >
> > [...]
> >
> > > -static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node,
> > > +static int __init psci_dt_cpu_init_idle(struct cpuidle_driver *drv,
> > > +                                     struct device_node *cpu_node,
> > >                                       unsigned int state_count, int cpu)
> > >  {
> > >       int i, ret = 0;
> > > @@ -118,6 +152,11 @@ static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node,
> > >               goto free_mem;
> > >       }
> > >
> > > +     /* Manage the deepest state via a dedicated enter-function. */
> > > +     if (dev)
> > > +             drv->states[state_count - 1].enter =
> > > +                     psci_enter_domain_idle_state;
> >
> >
> > It is unfortunate to make this arbitrary choice, it would be best
> > if you could detect which states are "domain" states aka are governed
> > by multiple cpus.
>
> The domain states are managed and selected by the genpd providers, via
> using runtime PM reference counting. Please have a closer look at the
> code in cpuidle-psci-domain.c and in the generic PM domain, that
> should give you the needed details.
>
> I am overriding the enter callback for the *deepest* known idle state
> of the CPU, which is according to what you requested [1].
>
> So, unless I am missing your point, I think the above code does
> exactly what you want, no?
>
> In regards to the "arbitrary choice" of what cpuidle state to use,
> there are more details about why that is, in the changelog.

Correction: Since I have moved patches around, I realized that the
explanation is actually put in the changelog of patch11.

For clarity, let me cut and paste it here as well:

"The triggering point for when runtime PM reference counting should be done,
has been selected to the deepest idle state for the CPU. However, from the
hierarchical point view, there may be good reasons to do runtime PM
reference counting even on shallower idle states, but at this point this
isn't supported, mainly due to limitations set by the generic PM domain."

Is that good enough or you want some of this information also in the
changelog of $subject patch? Or if you have any other idea for how to
make this more clear?

[...]

Kind regards
Uffe