Scaling back down a shared clock

Scaling back down a shared clock

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1908 bytes --]

Hi Mike, Stephen,

The SoCs used in the RaspberryPi have a bunch of shared clocks (between
two HDMI controllers for example) that need to be at a given minimum
rate for each instance (based on the resolution for the HDMI
controllers), but don't really have any maximum requirement other than
the operating boundaries of that clock.

We've supported it so far using the clk_set_min_rate function which
handles nicely that requirement and the fact that it's shared.

However, those clocks run at a fairly high frequency and there's some
interest in keeping them at their minimum to draw less power. Currently,
if we increase the minimum to a rate higher than the current clock rate,
it will raise its rate, but once that minimum is lowered the clock rate
doesn't change (which makes sense).

How could we put some kind of downward pressure on the clock rate to
make it run at its minimum all the time? It looks like the PM QoS
framework implements something similar for the bus bandwidth, except
that it's not really a bus that have this requirement here.

We were thinking about either adding a flag to the clock that would make
it run always at its lowest frequency, or to introduce a "boost" API to
bump temporarily the clock rate and then once done brings it back to its
default rate.

The first one though would be a bit dangerous, since it's mostly
use-case based, and if we don't get any clock user reporting a minimum
it might have a bunch of weird side effects, making clk_set_rate_min
required de facto while it wasn't before.

The boost API semantics would probably have a bunch of weird corner
cases. For example, how long is "temporarily" exactly, since for all we
know it might actually last forever. What would happen if we change the
boundaries or even the rate of the clock during a boost? etc.

I'd really like to get your opinion on this before I start working on
something.

Thanks!
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: Scaling back down a shared clock

[Michael Turquette]

Hi Maxime,

On Fri, Mar 19, 2021 at 8:03 AM Maxime Ripard <maxime@cerno.tech> wrote:
>
> Hi Mike, Stephen,
>
> The SoCs used in the RaspberryPi have a bunch of shared clocks (between
> two HDMI controllers for example) that need to be at a given minimum
> rate for each instance (based on the resolution for the HDMI
> controllers), but don't really have any maximum requirement other than
> the operating boundaries of that clock.
>
> We've supported it so far using the clk_set_min_rate function which
> handles nicely that requirement and the fact that it's shared.
>
> However, those clocks run at a fairly high frequency and there's some
> interest in keeping them at their minimum to draw less power. Currently,
> if we increase the minimum to a rate higher than the current clock rate,
> it will raise its rate, but once that minimum is lowered the clock rate
> doesn't change (which makes sense).
>
> How could we put some kind of downward pressure on the clock rate to
> make it run at its minimum all the time? It looks like the PM QoS
> framework implements something similar for the bus bandwidth, except
> that it's not really a bus that have this requirement here.

I'm a fan of _get/_put semantics for these types of critical sections.
In hindsight I wish that the all of the rate change APIs followed a
_set/_release or _get/_put pattern. clk_rate_exclusive{_get,_put}
probably gets the idea right, but only for a limited use case.

In your case, it would be cool to set the min rate at driver probe,
then _set the rate to perform work and then later _release the rate
once work was finished, and the default behavior in that case could be
to try and target the lowest possible rate.

Seeing as how the common clk framework just celebrated its ninth
birthday a few days ago on March 16, maybe it's time for a rewrite
with almost a decade of lessons learned? ;-)

Anyways, I digress. For a real solution to your actual problem, could
your driver do something like:

  clk_set_min_rate(clk, min_rate);
  clk_set_rate(clk, operating_rate);
  do_work();
  clk_set_rate(clk, min_rate);

The final rate at the end of this sequence may end up being min_rate,
or could be something higher based on the constraints of a different
struct clk in another device driver, which is taken into consideration
during clk_core_get_boundaries() which gets called in the clk_set_rate
-> clk_calc_new_rates path.

>
> We were thinking about either adding a flag to the clock that would make
> it run always at its lowest frequency, or to introduce a "boost" API to
> bump temporarily the clock rate and then once done brings it back to its
> default rate.

New flags and boost APIs both make me a little sad :-(

Best regards,
Mike


>
> The first one though would be a bit dangerous, since it's mostly
> use-case based, and if we don't get any clock user reporting a minimum
> it might have a bunch of weird side effects, making clk_set_rate_min
> required de facto while it wasn't before.
>
> The boost API semantics would probably have a bunch of weird corner
> cases. For example, how long is "temporarily" exactly, since for all we
> know it might actually last forever. What would happen if we change the
> boundaries or even the rate of the clock during a boost? etc.
>
> I'd really like to get your opinion on this before I start working on
> something.
>
> Thanks!
> Maxime

Re: Scaling back down a shared clock

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 5043 bytes --]

Hi Mike 

Thanks for your answer

On Fri, Mar 19, 2021 at 12:58:05PM -0700, Michael Turquette wrote:
> On Fri, Mar 19, 2021 at 8:03 AM Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > Hi Mike, Stephen,
> >
> > The SoCs used in the RaspberryPi have a bunch of shared clocks (between
> > two HDMI controllers for example) that need to be at a given minimum
> > rate for each instance (based on the resolution for the HDMI
> > controllers), but don't really have any maximum requirement other than
> > the operating boundaries of that clock.
> >
> > We've supported it so far using the clk_set_min_rate function which
> > handles nicely that requirement and the fact that it's shared.
> >
> > However, those clocks run at a fairly high frequency and there's some
> > interest in keeping them at their minimum to draw less power. Currently,
> > if we increase the minimum to a rate higher than the current clock rate,
> > it will raise its rate, but once that minimum is lowered the clock rate
> > doesn't change (which makes sense).
> >
> > How could we put some kind of downward pressure on the clock rate to
> > make it run at its minimum all the time? It looks like the PM QoS
> > framework implements something similar for the bus bandwidth, except
> > that it's not really a bus that have this requirement here.
> 
> I'm a fan of _get/_put semantics for these types of critical sections.
> In hindsight I wish that the all of the rate change APIs followed a
> _set/_release or _get/_put pattern. clk_rate_exclusive{_get,_put}
> probably gets the idea right, but only for a limited use case.
> 
> In your case, it would be cool to set the min rate at driver probe,
> then _set the rate to perform work and then later _release the rate
> once work was finished, and the default behavior in that case could be
> to try and target the lowest possible rate.
> 
> Seeing as how the common clk framework just celebrated its ninth
> birthday a few days ago on March 16, maybe it's time for a rewrite
> with almost a decade of lessons learned? ;-)

Happy birthday I guess ? :) 

> Anyways, I digress. For a real solution to your actual problem, could
> your driver do something like:
> 
>   clk_set_min_rate(clk, min_rate);
>   clk_set_rate(clk, operating_rate);
>   do_work();
>   clk_set_rate(clk, min_rate);

Like I said, it wouldn't really work in our case unfortunately

We have several occurrences of that in the SoC, but the one I'm the most
familiar with is that we have two HDMI controllers that have an internal
state machine, and both are fed from the same clock.

This state machine needs a clock that is at least as fast as the pixel
rate (plus some margin), but there's no real upper boundary, it just
needs to be fast enough to move pixels around (as opposed to a "real"
pixel clock or an audio clock that is fairly rate sensitive).

The thing starts to get a bit messy when both HDMI controllers would run
at different resolution, since in this case, you would have one
controller that would call clk_set_rate to enforce its state machine
rate, and then the other.

This would work fine is the second controller runs with the same or a
higher resolution, but if the second controller runs at a lower
frequency you just stalled the first one by dropping the rate below
what its state machine requires.

clk_set_min_rate feels like it's aimed at what we want to do: we set a
minimal frequency for each user, and the clock runs at the maximum of
all the minimums.

Our sole issue with it is really that whenever that maximum of minimums
drops to some lower value (because the display is disabled for example,
or changed to a lower resolution) the actual rate of the clock isn't
changed, consuming more than we would expect from that workload.

This makes sense to some extent if we consider clk_set_min_rate and
clk_set_max_rate functions only as functions to modify the boundaries,
since we are still totally within the operating boundaries of the clock.

But then we're missing something to negociate the best rate for a given
clock between all its users.

> The final rate at the end of this sequence may end up being min_rate,
> or could be something higher based on the constraints of a different
> struct clk in another device driver, which is taken into consideration
> during clk_core_get_boundaries() which gets called in the clk_set_rate
> -> clk_calc_new_rates path.
> 
> >
> > We were thinking about either adding a flag to the clock that would make
> > it run always at its lowest frequency, or to introduce a "boost" API to
> > bump temporarily the clock rate and then once done brings it back to its
> > default rate.
> 
> New flags and boost APIs both make me a little sad :-(

I guess another way to implement something like that could be a callback
to return the ideal state of a clock for a given clock + users, that
would be ran each time a clock parameter (rate or boundary at least)
would change?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: Scaling back down a shared clock

[Linus Walleij]

On Fri, Mar 19, 2021 at 4:04 PM Maxime Ripard <maxime@cerno.tech> wrote:

> How could we put some kind of downward pressure on the clock rate to
> make it run at its minimum all the time? It looks like the PM QoS
> framework implements something similar for the bus bandwidth, except
> that it's not really a bus that have this requirement here.

Unsolicited comment: this is similar to what _regulator_do_set_voltage()
is doing with voltages that can be expressed as linear intervals or
selectors (fixed voltage steps) when a consumer calls
regulator_set_voltage().

Yours,
Linus Walleij

Re: Scaling back down a shared clock

[Abel Vesa]

On 21-03-19 16:03:55, Maxime Ripard wrote:
> Hi Mike, Stephen,
> 
> The SoCs used in the RaspberryPi have a bunch of shared clocks (between
> two HDMI controllers for example) that need to be at a given minimum
> rate for each instance (based on the resolution for the HDMI
> controllers), but don't really have any maximum requirement other than
> the operating boundaries of that clock.
> 
> We've supported it so far using the clk_set_min_rate function which
> handles nicely that requirement and the fact that it's shared.
> 
> However, those clocks run at a fairly high frequency and there's some
> interest in keeping them at their minimum to draw less power. Currently,
> if we increase the minimum to a rate higher than the current clock rate,
> it will raise its rate, but once that minimum is lowered the clock rate
> doesn't change (which makes sense).
> 
> How could we put some kind of downward pressure on the clock rate to
> make it run at its minimum all the time? It looks like the PM QoS
> framework implements something similar for the bus bandwidth, except
> that it's not really a bus that have this requirement here.
> 

Maybe interconnect + devfreq is the solution here.

I did something similar here:

https://lore.kernel.org/lkml/1613750416-11901-1-git-send-email-abel.vesa@nxp.com/

> We were thinking about either adding a flag to the clock that would make
> it run always at its lowest frequency, or to introduce a "boost" API to
> bump temporarily the clock rate and then once done brings it back to its
> default rate.
> 
> The first one though would be a bit dangerous, since it's mostly
> use-case based, and if we don't get any clock user reporting a minimum
> it might have a bunch of weird side effects, making clk_set_rate_min
> required de facto while it wasn't before.
> 
> The boost API semantics would probably have a bunch of weird corner
> cases. For example, how long is "temporarily" exactly, since for all we
> know it might actually last forever. What would happen if we change the
> boundaries or even the rate of the clock during a boost? etc.
> 
> I'd really like to get your opinion on this before I start working on
> something.
> 
> Thanks!
> Maxime

Re: Scaling back down a shared clock

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1075 bytes --]

Hi Linus,

On Thu, Mar 25, 2021 at 09:01:31AM +0100, Linus Walleij wrote:
> On Fri, Mar 19, 2021 at 4:04 PM Maxime Ripard <maxime@cerno.tech> wrote:
> 
> > How could we put some kind of downward pressure on the clock rate to
> > make it run at its minimum all the time? It looks like the PM QoS
> > framework implements something similar for the bus bandwidth, except
> > that it's not really a bus that have this requirement here.
> 
> Unsolicited comment: this is similar to what _regulator_do_set_voltage()
> is doing with voltages that can be expressed as linear intervals or
> selectors (fixed voltage steps) when a consumer calls
> regulator_set_voltage().

Thanks for the pointer :)

It's definitely relevant, regulators and clocks are fairly similar in
what constraints they have and how consumers would want to express them
I guess.

To some extent regulator_get_optimal_voltage is also something relevant
to the discussion, since it will try to find the best match for the
opposite scenario: a single user with multiple regulators

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: Scaling back down a shared clock

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1597 bytes --]

On Thu, Mar 25, 2021 at 11:58:07AM +0200, Abel Vesa wrote:
> On 21-03-19 16:03:55, Maxime Ripard wrote:
> > Hi Mike, Stephen,
> > 
> > The SoCs used in the RaspberryPi have a bunch of shared clocks (between
> > two HDMI controllers for example) that need to be at a given minimum
> > rate for each instance (based on the resolution for the HDMI
> > controllers), but don't really have any maximum requirement other than
> > the operating boundaries of that clock.
> > 
> > We've supported it so far using the clk_set_min_rate function which
> > handles nicely that requirement and the fact that it's shared.
> > 
> > However, those clocks run at a fairly high frequency and there's some
> > interest in keeping them at their minimum to draw less power. Currently,
> > if we increase the minimum to a rate higher than the current clock rate,
> > it will raise its rate, but once that minimum is lowered the clock rate
> > doesn't change (which makes sense).
> > 
> > How could we put some kind of downward pressure on the clock rate to
> > make it run at its minimum all the time? It looks like the PM QoS
> > framework implements something similar for the bus bandwidth, except
> > that it's not really a bus that have this requirement here.
> > 
> 
> Maybe interconnect + devfreq is the solution here.
> 
> I did something similar here:
> 
> https://lore.kernel.org/lkml/1613750416-11901-1-git-send-email-abel.vesa@nxp.com/

Unfortunately, this is some internal device requirements (internal state
machine) so we don't really have an interconnect there

maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]