Re: [PATCH 2/2] cpuidle: psci: Enable s2idle when using OSI with the PM domain topology

From: Ulf Hansson <ulf.hansson@linaro.org>
To: "Rafael J . Wysocki" <rjw@rjwysocki.net>
Cc: Sudeep Holla <sudeep.holla@arm.com>,
	Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>,
	Linux PM <linux-pm@vger.kernel.org>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	Lina Iyer <ilina@codeaurora.org>,
	Lukasz Luba <lukasz.luba@arm.com>,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Stephen Boyd <sboyd@kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Benjamin Gaignard <benjamin.gaignard@st.com>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH 2/2] cpuidle: psci: Enable s2idle when using OSI with the PM domain topology
Date: Thu, 1 Oct 2020 13:44:30 +0200	[thread overview]
Message-ID: <CAPDyKFqarjOgK1Ryfe37pUkv-LmA+_oxT6bi=gpgwaM9G5PjsQ@mail.gmail.com> (raw)
In-Reply-To: <CAPDyKFqNrCo=jGWMp67bKstErE5ZYR+3JzoPyYUtb4y2rK+dOA@mail.gmail.com>

On Thu, 1 Oct 2020 at 13:32, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Thu, 1 Oct 2020 at 12:18, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Tue, Sep 01, 2020 at 10:27:07AM +0200, Ulf Hansson wrote:
> > > To select domain idle states for cpuidle-psci, the PM domains via genpd are
> > > being managed by using runtime PM. This works fine for the regular idle
> > > path, but it doesn't when doing s2idle.
> > >
> > > More precisely, the domain idle states becomes temporarily disabled, which
> > > is because the PM core disables runtime PM for devices during system
> > > suspend.
> >
> > When you refer system suspend above, you mean both S2R and S2I ?
>
> Correct.
>
> Although, there is no problem with S2R to reach the proper idlestate,
> because of the way we offline all but the boot CPU.
>
> >
> > > Even if genpd tries to power off the PM domain in the
> > > suspend_noirq phase, that doesn't help to properly select a domain idle
> > > state, as this needs to be done on per CPU basis.
> > >
> >
> > And what prevents doing per CPU basis ?
>
> The PM core doesn't execute the system suspend callbacks on a per CPU basis.
>
> >
> > > Let's address the issue by enabling the syscore flag for the attached CPU
> > > devices. This prevents genpd from trying to power off the corresponding PM
> > > domains in the suspend_noirq phase. Moreover, let's assign a specific
> > > ->enter_s2idle() callback for the corresponding domain idle state and let
> > > it invoke pm_genpd_syscore_poweroff|poweron(), rather than using runtime
> > > PM.
> > >
> >
> > The syscore_suspend is not executed for S2I and using syscore APIs here
> > is bit confusing IMO. If Rafael is fine, I have no objections.
>
> That's correct, the syscore phase doesn't exist in the S2I path.
>
> However, in some cases the same functions that are being called in the
> syscore phase, are also being called for S2I. For example, have a look
> at timekeeping_suspend(), which is being called from both paths.
>
> In the end, I think the confusing part is the name of the genpd functions.
>
> Maybe we should rename pm_genpd_syscore_poweroff|poweron() to
> pm_genpd_suspend|resume() - or something along those lines.
>

[...]

Rafael, I understand you have objections to the approach. Would
renaming the genpd APIs be a way forward? Another option is to add two
new genpd APIs along the side of the existing, not sure what would be
the best.

Kind regards
Uffe