[PATCH 1/8] clk: Add range accessors

[Maxime Ripard]

On Tue, Mar 16, 2021 at 06:06:40PM -0700, Stephen Boyd wrote:
> Quoting Maxime Ripard (2021-03-03 00:45:27)
> > 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
> 
> How does the clk driver query the firmware but it can't be done
> directly by the drm driver? 

The configuration is done through a text file accessed by the firmware.
What I meant was that the kernel cannot access the content of that file
to make sure the right options have been enabled.

However, it can indeed communicate with the firmware through the extent
of the API it provides, but it's fairly limited. In our case, the only
way to tell is to look for side effects of the configuration option, ie
the maximum rate of the clock that has been increased.

> > > 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
> 
> Does using clk_round_rate() work just as well?

I guess it could work too, I'll try it out

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/20210317/ce45936b/attachment.sig>