linux-pm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Ulf Hansson <ulf.hansson@linaro.org>
To: Stephan Gerhold <stephan@gerhold.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	Kevin Hilman <khilman@kernel.org>, Nishanth Menon <nm@ti.com>,
	Stephen Boyd <sboyd@kernel.org>,
	Linux PM <linux-pm@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Niklas Cassel <nks@flawful.org>
Subject: Re: [RFC PATCH 3/3] opp: Power on (virtual) power domains managed by the OPP core
Date: Tue, 25 Aug 2020 14:42:54 +0200	[thread overview]
Message-ID: <CAPDyKFr-gfpVypFs_13hb9Pi5FfQoB32fg=C_gtdSKVDN1U=gQ@mail.gmail.com> (raw)
In-Reply-To: <20200825073348.GA1048@gerhold.net>

On Tue, 25 Aug 2020 at 09:34, Stephan Gerhold <stephan@gerhold.net> wrote:
>
> On Tue, Aug 25, 2020 at 08:43:42AM +0200, Ulf Hansson wrote:
> > On Tue, 25 Aug 2020 at 06:43, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >
> > > On 24-08-20, 17:08, Stephan Gerhold wrote:
> > > > On Mon, Aug 24, 2020 at 04:36:57PM +0200, Ulf Hansson wrote:
> > > > > That said, perhaps should rely on the consumer to deploy runtime PM
> > > > > support, but let the OPP core to set up the device links for the genpd
> > > > > virtual devices!?
> > > > >
> > > >
> > > > Yes, that would be the alternative option.
> > >
> > > That is the right option IMO.
> > >
> > > > I would be fine with it as long as it also works for the CPUfreq case.
> > > >
> > > > I don't think anything manages runtime PM for the CPU device, just
> > > > like no-one calls dev_pm_opp_set_rate(cpu_dev, 0). So with my patch the
> > > > power domain is essentially kept always-on (except for system suspend).
> > > > At least in my case this is intended.
> > > >
> > > > If device links also keep the power domains on if the consumer device
> > > > does not make use of runtime PM it should work fine for my case.
> > >
> > > With device link, you only need to do rpm enable/disable on the consumer device
> > > and it will get propagated by itself.
> >
> > Note that the default state for the genpd virtual device(s) is that
> > runtime PM has been enabled for them. This means it's left in runtime
> > suspended state, which allows its PM domain to be powered off (if all
> > other devices and child domains for it allow that too, of course).
> >
> > >
> > > > Personally, I think my original patch (without device links) fits better
> > > > into the OPP API, for the following two reasons.
> > > >
> > > > With device links:
> > > >
> > > >   1. Unlike regulators/interconnects, attached power domains would be
> > > >      controlled by runtime PM instead of dev_pm_opp_set_rate(opp_dev, 0).
> > > >
> > > >   2. ... some driver using OPP tables might not make use of runtime PM.
> > > >      In that case, the power domains would stay on the whole time,
> > > >      even if dev_pm_opp_set_rate(opp_dev, 0) was called.
> > > >
> > > > With my patch, the power domain state is directly related to the
> > > > dev_pm_opp_set_rate(opp_dev, 0) call, which is more intuitive than
> > > > relying on the runtime PM state in my opinion.
> > >
> > > So opp-set-rate isn't in the best of shape TBH, some things are left for the
> > > drivers while other are done by it. Regulator-enable/disable was moved to it
> > > some time back as people needed something like that. While on the other hand,
> > > clk_enable/disable doesn't happen there, nor does rpm enable/disable.
> > >
> > > Maybe one day we may want to do that, but lets make sure someone wants to do
> > > that first.
> > >
> > > Anyway, even in that case both of the changes would be required. We must make
> > > device links nevertheless first. And later on if required, we may want to do rpm
> > > enable/disable on the consumer device itself.
> >
> > This sounds like a reasonable step-by-step approach.
> >
> > Then, to create the device links, we should use DL_FLAG_PM_RUNTIME,
> > DL_FLAG_STATELESS.
> >
>
> OK, I will give this a try later this week.
>
> > But whether we should use DL_FLAG_RPM_ACTIVE as well, to initially
> > runtime resume the supplier (the genpd virtual device), is harder to
> > know - as that kind of depends on expectations by the consumer device
> > driver.
> >
>
> I'm not sure I understand the purpose of that flag. I thought we want to
> link the PM state of the virtual genpd device (supplier) to the PM state
> of the device of the OPP table (consumer).

Correct, but this is about synchronizing the initial runtime PM state
of the consumer and supplier.

>
> Shouldn't it just determine the initial state based on the state of the
> consumer device?

We could do that. Then something along the lines of the below, should work:

pm_runtime_get_noresume(consumer) - to prevent runtime suspend, temporarily.

if(pm_runtime_active(consumer))
    create links with DL_FLAG_RPM_ACTIVE
else
    create links without DL_FLAG_RPM_ACTIVE

pm_runtime_put(consumer)

Kind regards
Uffe

  reply	other threads:[~2020-08-25 12:43 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-30  8:01 [RFC PATCH 0/3] opp: required_opps: Power on genpd, scale down in reverse order Stephan Gerhold
2020-07-30  8:01 ` [RFC PATCH 1/3] opp: Reduce code duplication in _set_required_opps() Stephan Gerhold
2020-08-24 11:18   ` Viresh Kumar
2020-08-24 11:30     ` Stephan Gerhold
2020-08-24 12:10       ` Viresh Kumar
2020-08-24 12:23         ` Stephan Gerhold
2020-07-30  8:01 ` [RFC PATCH 2/3] opp: Set required OPPs in reverse order when scaling down Stephan Gerhold
2020-08-21 16:31   ` Stephan Gerhold
2020-08-24 11:30     ` Viresh Kumar
2020-08-24 11:42       ` Stephan Gerhold
2020-07-30  8:01 ` [RFC PATCH 3/3] opp: Power on (virtual) power domains managed by the OPP core Stephan Gerhold
2020-08-24 11:27   ` Viresh Kumar
2020-08-24 11:55     ` Stephan Gerhold
2020-08-24 14:36       ` Ulf Hansson
2020-08-24 15:08         ` Stephan Gerhold
2020-08-25  4:43           ` Viresh Kumar
2020-08-25  6:43             ` Ulf Hansson
2020-08-25  7:33               ` Stephan Gerhold
2020-08-25 12:42                 ` Ulf Hansson [this message]
2020-08-26  8:31                   ` Stephan Gerhold
2020-08-12  8:53 ` [RFC PATCH 0/3] opp: required_opps: Power on genpd, scale down in reverse order Ulf Hansson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAPDyKFr-gfpVypFs_13hb9Pi5FfQoB32fg=C_gtdSKVDN1U=gQ@mail.gmail.com' \
    --to=ulf.hansson@linaro.org \
    --cc=khilman@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=nks@flawful.org \
    --cc=nm@ti.com \
    --cc=rjw@rjwysocki.net \
    --cc=sboyd@kernel.org \
    --cc=stephan@gerhold.net \
    --cc=viresh.kumar@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).