All of lore.kernel.org
 help / color / mirror / Atom feed
From: Michael Turquette <mturquette@baylibre.com>
To: Stephen Boyd <sboyd@kernel.org>, Ulf Hansson <ulf.hansson@linaro.org>
Cc: Viresh Kumar <viresh.kumar@linaro.org>,
	grahamr@codeaurora.org, linux-clk <linux-clk@vger.kernel.org>,
	Linux PM <linux-pm@vger.kernel.org>,
	Doug Anderson <dianders@chromium.org>,
	Taniya Das <tdas@codeaurora.org>,
	Rajendra Nayak <rnayak@codeaurora.org>,
	Amit Nischal <anischal@codeaurora.org>,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Amit Kucheria <amit.kucheria@linaro.org>
Subject: Re: [RFD] Voltage dependencies for clocks (DVFS)
Date: Fri, 03 Aug 2018 14:11:32 -0700	[thread overview]
Message-ID: <20180803211132.71539.18482@harbor.lan> (raw)
In-Reply-To: <CAPDyKFr7xxUV0pmQCoM1T_fZdBifWqJF6QJgHQe2JGUU0a6MJg@mail.gmail.com>

Quoting Ulf Hansson (2018-07-31 03:35:50)
> [...]
> =

> >>
> >> Another option is to use the ->runtime_suspend|resume() callbacks at
> >> the genpd level. In principle when the device enters runtime suspend,
> >> genpd can decide to drop the votes for the device (if some exist). At
> >> runtime resume, genpd then should re-store the votes for the device it
> >> had before it became runtime suspended (or if the vote became updated
> >> in-between).
> >
> > I'd prefer to see the genpd layer do it instead of OPP layer controlling
> > the on/off state. Calling dev_pm_opp_set_rate(dev, 0) just looks very
> > weird and conflates clk frequency with power domain state which isn't
> > exactly what we want. We want it to be related to the frequency _and_
> > the enabled state. Plus it's like saying clk_set_rate(clk, 0) should
> > turn the clk off, which we've never done before.
> =

> Perhaps you misunderstood my suggestion!?
> =

> Instead of having the driver to call dev_pm_opp_set_rate(dev, 0) from
> their ->runtime_suspend() callbacks, my suggestion is to let genpd,
> from its ->runtime_suspend() callback, temporarily drop the requested
> performance level for the device. Hence genpd also needs to re-compute
> and set a new aggregated performance value for the PM domain.
> =

> The purpose was to make it transparent for drivers, as all they need
> do is whatever they did before.
> =

> >
> > It would also be good to handle the on/off state in genpd because the
> > genpd could have a governor attached to it that blocks on/off requests
> > from drivers if the governor decides that the device is clk gating on
> > and off quickly but it's not beneficial to change the voltage during the
> > quick gating scenario because we spend more time changing voltage than
> > clk gating. Connecting this all to genpd allows us to use the device
> > power state as a signal to adjust the voltage, but leaves us flexibility
> > to override it if it makes sense policy wise.
> =

> Well, genpd already manages the on/off state!?
> =

> Of course, I fully agree to the above, which seems also to be a good
> reason to why my suggestion makes sense. Or am I missing something
> here?
> =

> >
> > And please remember the other point here that many devices may all be
> > sharing the same power domain and therefore the voltage that the domain
> > runs at depends on many different clk frequencies for different devices
> > and if those clks are enabled or not, aggregated into a single voltage
> > requirement for the power domain.
> =

> Right, the aggregation in genpd is done per device - not per clock. I
> don't understand your concern, but again I may be missing something!?

I believe that this is an argument against using OPP as the performance
selection mechanism for all SoCs. It's data structures, along with the
fact that is seems to have no awareness of device idleness, means that
it's not really sufficient for more complex SoCs.

> =

> > The runtime suspend state might be
> > part of that aggregation equation, so that if a device's clk is off and
> > it's been suspended for some time the voltage requirement for that clk
> > frequency could be dropped, but if the device is resumed and the clk is
> > still off then the voltage requirement can be ignored. We only need to
> > make sure the power domain is at a high enough voltage when the clk is
> > actually enabled though, so tying the voltage requirement drop to
> > runtime PM of the device is not exactly correct.
> =

> I see.
> =

> For my understanding; you are saying that there may be use-cases when
> one want to keep the device runtime resumed, but without having its
> clock ungated, right?
> =

> My guess is that this is a rather unusual case, no?

This is a common case if I understand Stephen's example: Imagine a power
island that scales voltage and may power clamp to zero. Now imagine that
within that power island are a dozen separate functional devices, each
consuming a dedicated functional clock input signal.

It's possible to have the power island turned on so that one device can
function, but the other 11 devices will remain clock gated.

I assume the right way to handle this is to nest a dozen device-level
genpd's within the power island-level genpd? That mirrors the topology I
described above, and allows the lowest level genpds (functional
device-level) to gate and ungate clocks, and the parent genpd (power
island-level) will only be concerned with voltage control.

> =

> The point is, I think we can simply have the driver to call
> dev_pm_opp_set_rate() to cover optimizations for these scenarios.
> =

> >
> > It may be that a device is composed of multiple power domains and those
> > power domains each have their own clks with voltage requirements too.
> > In this scenario, connecting the voltage requirement of the genpd to the
> > runtime PM state of the device doesn't make sense at all. We'll get into
> > a situation where half the device is "off" because a power domain and
> > the clk for that power domain are off but we can't drop the voltage
> > requirement because the overall device isn't runtime suspended. It's
> > like the single genpd per device problem all over again. Let's not do
> > that.
> =

> I am *not* suggesting to remove the APIs in genpd for setting a
> performance level. We can still do that and that should be sufficient
> to cover any corner cases, don't you think?
> =

> My suggestion was only to allow genpd to drop the votes (temporarily)
> for a device that becomes runtime suspended.
> =

> >
> > So I'd rather see drivers calling some genpd API directly to indicate
> > that the clk should turn off and the voltage domain constraint has gone
> > away. We might want to transform OPP into a genpd of its own and then
> > connect that OPP genpd to a clk domain genpd for some clk
> > on/off/frequency part and a power domain for the voltage/corner scaling
> > part. If we did that, then everything is a genpd that gets all
> > aggregated in the genpd layer and drivers that only use OPP would be OK
> > with their runtime PM state of their device affecting the OPP genpd's
> > on/off state of clks and voltage requirements. Drivers that want finer
> > control would be able to use some new genpd API to turn on and off the
> > genpd when they want to.
> =

> Sound like we need a white-board. :-)

We really do. Maybe I'll make the trip to Vancouver in September :-)

I don't like the idea to "transform OPP into a genpd of its own", as
suggested by Stephen above. But I think all he needs to solve this issue
is nesting some functional clock-level genpds inside of a parent power
island-level genpd. This is a repeat of the three paragraphs I've
written above.

This approach achieves the same effect as turning opp into a genpd, but
I'm not inclined to try and change the OPP library to solve this
problem.

I think OPP is fine as-is: it makes it easy to handle the very simplest
DVFS cases.  Stephen's requirements (and that of most mobile SoCs) goes
beyond what the OPP library can and should offer.

Regards,
Mike

> =

> Anyway, I am reading the following replies in the thread to hopefully
> understand your suggestions a bit better.
> =

> Kind regards
> Uffe

  reply	other threads:[~2018-08-03 21:11 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-18 20:44 [RFD] Voltage dependencies for clocks (DVFS) grahamr
2018-07-02  5:13 ` Michael Turquette
2018-07-04  6:55 ` Viresh Kumar
2018-07-04 12:50   ` Ulf Hansson
2018-07-04 12:54     ` Rafael J. Wysocki
2018-07-04 12:58       ` Ulf Hansson
2018-07-20 17:12     ` Stephen Boyd
2018-07-20 17:56       ` Michael Turquette
2018-07-24 23:13         ` Stephen Boyd
2018-07-25  5:51           ` Michael Turquette
2018-07-23  8:26       ` Peter De Schrijver
2018-07-24 23:04         ` Stephen Boyd
2018-07-25  5:44           ` Michael Turquette
2018-07-25 11:27             ` Peter De Schrijver
2018-07-25 18:40               ` Michael Turquette
2018-07-31 11:56               ` Ulf Hansson
2018-07-31 20:02                 ` grahamr
2018-08-23 13:20                   ` Ulf Hansson
2018-09-18 23:00                     ` Michael Turquette
2018-09-19  7:05                       ` Geert Uytterhoeven
2018-09-19 18:07                         ` Michael Turquette
2018-09-25 13:11                           ` Geert Uytterhoeven
2018-09-25 21:26                       ` grahamr
2018-10-01 19:00                         ` Michael Turquette
2018-10-04  0:37                           ` Graham Roff
2018-10-04 21:23                             ` Michael Turquette
2018-09-18 17:25                   ` Kevin Hilman
2018-08-03 23:05                 ` Michael Turquette
2018-08-23 12:13                   ` Ulf Hansson
2018-09-18 22:48                     ` Michael Turquette
2018-07-31 10:35       ` Ulf Hansson
2018-08-03 21:11         ` Michael Turquette [this message]
2018-08-23 11:10           ` Ulf Hansson
2018-07-05  8:19 ` Peter De Schrijver

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=20180803211132.71539.18482@harbor.lan \
    --to=mturquette@baylibre.com \
    --cc=amit.kucheria@linaro.org \
    --cc=anischal@codeaurora.org \
    --cc=dianders@chromium.org \
    --cc=grahamr@codeaurora.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=rnayak@codeaurora.org \
    --cc=sboyd@kernel.org \
    --cc=tdas@codeaurora.org \
    --cc=ulf.hansson@linaro.org \
    --cc=vincent.guittot@linaro.org \
    --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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.