Message-ID: <20220112114652.hmfdcpqil5jg2vz6@houat>
Date:   Wed, 12 Jan 2022 12:46:52 +0100
From:   Maxime Ripard <maxime@...no.tech>
To:     Stephen Boyd <sboyd@...nel.org>
Cc:     Daniel Vetter <daniel.vetter@...el.com>,
        David Airlie <airlied@...ux.ie>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Mike Turquette <mturquette@...libre.com>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        dri-devel@...ts.freedesktop.org,
        Jerome Brunet <jbrunet@...libre.com>,
        linux-clk@...r.kernel.org,
        Dave Stevenson <dave.stevenson@...pberrypi.com>,
        Phil Elwell <phil@...pberrypi.com>,
        Tim Gover <tim.gover@...pberrypi.com>,
        Dom Cobley <dom@...pberrypi.com>,
        Emma Anholt <emma@...olt.net>, linux-kernel@...r.kernel.org,
        Russell King <linux@...linux.org.uk>
Subject: Re: [PATCH v2 1/3] clk: Introduce a clock request API

Hi Stephen,

Thanks for your answer

On Tue, Jan 11, 2022 at 07:37:15PM -0800, Stephen Boyd wrote:
> Sorry for being super delayed on response here. I'm buried in other
> work. +Jerome for exclusive clk API.
> 
> Quoting Maxime Ripard (2021-09-14 02:35:13)
> > It's not unusual to find clocks being shared across multiple devices
> > that need to change the rate depending on what the device is doing at a
> > given time.
> > 
> > The SoC found on the RaspberryPi4 (BCM2711) is in such a situation
> > between its two HDMI controllers that share a clock that needs to be
> > raised depending on the output resolution of each controller.
> > 
> > The current clk_set_rate API doesn't really allow to support that case
> > since there's really no synchronisation between multiple users, it's
> > essentially a fire-and-forget solution.
> 
> I'd also say a "last caller wins"
> 
> > 
> > clk_set_min_rate does allow for such a synchronisation, but has another
> > drawback: it doesn't allow to reduce the clock rate once the work is
> > over.
> 
> What does "work over" mean specifically? Does it mean one of the clk
> consumers has decided to stop using the clk?

That, or it doesn't need to enforce that minimum anymore. We have
several cases like this on the RPi. For example, during a change of
display mode a (shared) clock needs to be raised to a minimum, but
another shared one needs to raise its minimum based on the resolution.

In the former case, we only need the minimum to be enforced during the
resolution change, so it's fairly quick, but the latter requires its
minimum for as long as the display is on.

> Why doesn't clk_set_rate_range() work? Or clk_set_rate_range() combined
> with clk_set_rate_exclusive()?

clk_set_rate_range could work (it's what we have right now in mainline
after all), but it's suboptimal since the clock is never scaled down.

It's especially showing in my first example where we need to raise the
clock only for the duration of the resolution change. Using
clk_set_min_rate works but we end up with that fairly high clock (at
least 500MHz) for the rest of the system life even though we usually can
get away with using a clock around 200MHz outside of that (short) window.

This is fairly inefficient, and is mostly what I'm trying to address.

> > In our previous example, this means that if we were to raise the
> > resolution of one HDMI controller to the largest resolution and then
> > changing for a smaller one, we would still have the clock running at the
> > largest resolution rate resulting in a poor power-efficiency.
> 
> Does this example have two HDMI controllers where they share one clk and
> want to use the most efficient frequency for both of the HDMI devices? I
> think I'm following along but it's hard. It would be clearer if there
> was some psuedo-code explaining how it is both non-workable with current
> APIs and workable with the new APIs.

The fact that we have two HDMI controllers that share one clock is why
we use clk_set_min_rate in the first place, but you can have that
behavior with clk_set_min_rate only with a single user.

With pseudo-code, if you do something like

clk = clk_get(NULL);
clk_set_min_rate(600 * 1000 * 1000);
clk_set_min_rate(1000);

The clock will still remain at 600MHz, even though you would be totally
fine with the clock running at 1kHz.

If you really wanted to make the clock run at 1kHz, you'd need to have:

clk = clk_get(NULL);
clk_set_min_rate(600 * 1000 * 1000);
clk_set_min_rate(1000);
clk_set_rate(1000);

And that works fine for a single user.

If you have a clock shared by multiple drivers though, things get
tricky. Indeed, you can't really find out what the minimum for that
clock is, so figuring out the rate to pass to the clk_set_rate call
would be difficult already. And it wouldn't be atomic anyway.

It's made even more difficult since in clk_calc_new_rates the core
checks that the rate is within the boundaries and will error out if it
isn't, so even using clk_set_rate(0) wouldn't work.

It could work if the clock driver makes sure in round/determine_rate
that the rate passed in within the boundaries of the clock, but then you
start working around the core and relying on the behavior of clock
drivers, which is a fairly significant abstraction violation.

> > In order to address both issues, let's create an API that allows user to
> > create temporary requests to increase the rate to a minimum, before
> > going back to the initial rate once the request is done.
> > 
> > This introduces mainly two side-effects:
> > 
> >   * There's an interaction between clk_set_rate and requests. This has
> >     been addressed by having clk_set_rate increasing the rate if it's
> >     greater than what the requests asked for, and in any case changing
> >     the rate the clock will return to once all the requests are done.
> > 
> >   * Similarly, clk_round_rate has been adjusted to take the requests
> >     into account and return a rate that will be greater or equal to the
> >     requested rates.
> > 
> 
> I believe clk_set_rate_range() is broken but it can be fixed. I'm
> forgetting the details though. If the intended user of this new API
> can't use that range API then it would be good to understand why it
> can't be used. I imagine it would be something like
> 
> 	struct clk *clk_hdmi1, *clk_hdmi2;
> 
> 	clk_set_rate_range(&clk_hdmi1, HDMI1_MIN, HDMI1_MAX);
> 	clk_set_rate_range(&clk_hdmi2, HDMI2_MIN, HDMI2_MAX);
> 	clk_set_rate_range(&clk_hdmi2, 0, UINT_MAX);
> 
> and then the goal would be for HDMI1_MIN to be used, or at the least for
> the last call to clk_set_rate_range() to drop the rate constraint and
> re-evaluate the frequency of the clk again based on hdmi1's rate range.

This is pretty much what this series was doing. I was being conservative
and didn't really want to modify the behavior of existing functions, but
that will work fine.

Thanks!
Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists