[PATCH 1/8] clk: Add range accessors

[Maxime Ripard]

Hi Stephen,

On Tue, Mar 02, 2021 at 03:18:58PM -0800, Stephen Boyd wrote:
> Quoting Maxime Ripard (2021-02-25 07:59:02)
> > Some devices might need to access the current available range of a clock
> > to discover their capabilities. Let's add those accessors.
> 
> This needs more than two sentences to describe what's required.
> 
> > 
> > Signed-off-by: Maxime Ripard <maxime at cerno.tech>
> > ---
> >  drivers/clk/clk.c   | 30 ++++++++++++++++++++++++++++++
> >  include/linux/clk.h | 16 ++++++++++++++++
> >  2 files changed, 46 insertions(+)
> > 
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 8c1d04db990d..b7307d4f090d 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -2407,6 +2407,36 @@ int clk_set_max_rate(struct clk *clk, unsigned long rate)
> >  }
> >  EXPORT_SYMBOL_GPL(clk_set_max_rate);
> >  
> > +long clk_get_min_rate(struct clk *clk)
> 
> I need to read the rest of the patches but I don't see the justification
> for this sort of API vs. having the consumer constrain the clk frequency
> that it wants. Is the code that's setting the min/max constraints not
> the same as the code that's calling this API? Would an OPP table better
> serve this so the device knows what frequencies are valid?s Please
> provide the use case/justification in the commit text.

The patch that uses it is the patch 4

The issue I'm trying to solve is that all the RaspberryPi have a
configuration file for the firmware, and the firmware is in charge of
the clocks communicating through a mailbox interface.

By default, one of the main clocks in the system can only reach 500MHz,
and that's the range reported by the firmware when queried. However, in
order to support display modes with a fairly high bandwidth such as 4k
at 60Hz, that clock needs to be raised to at least 550MHz, and the
firmware configuration has a special parameter for that one. Setting
that parameter will increase the range of the clock to have proper
boundaries for that display mode.

If a user doesn't enable it and tries to use those display modes, the
display will be completely blank.

There's no way to query the firmware configuration directly, so we can
instead query the range of the clock and see if the firmware enables us
to use those modes, warn the user and ignore the modes that wouldn't
work. That's what those accessors are here for

> Why two functions instead of one function to get both min and max?

Since we have clk_set_min_rate and clk_set_max_rate, it made sense to me
to mirror that, but I'd be happy to change if you think otherwise

I'll address your other commenst

Maxime
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20210303/f1525b39/attachment.sig>