Re: [PATCH v4 07/68] clk: test: Add a determine_rate hook

[Maxime Ripard <maxime@cerno.tech>]

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

Hi Stephen,

On Tue, Jun 13, 2023 at 11:39:58AM -0700, Stephen Boyd wrote:
> Quoting Maxime Ripard (2023-06-13 01:21:59)
> > On Thu, Jun 08, 2023 at 06:41:56PM -0700, Stephen Boyd wrote:
> > > Quoting Maxime Ripard (2023-05-05 04:25:09)
> > 
> > > > +        * __clk_mux_determine_rate() here, if we use it and call
> > > > +        * clk_round_rate() or clk_set_rate() with a rate lower than
> > > > +        * what all the parents can provide, it will return -EINVAL.
> > > > +        *
> > > > +        * This is due to the fact that it has the undocumented
> > > > +        * behaviour to always pick up the closest rate higher than the
> > > > +        * requested rate. If we get something lower, it thus considers
> > > > +        * that it's not acceptable and will return an error.
> > > > +        *
> > > > +        * It's somewhat inconsistent and creates a weird threshold
> > > > +        * between rates above the parent rate which would be rounded to
> > > > +        * what the parent can provide, but rates below will simply
> > > > +        * return an error.
> > > > +        */
> > 
> > I guess it's mostly a policy decision: __clk_mux_determine_rate() always
> > has been returning rates that are lower or equal to the target rate.
> > 
> > If no parent can provide one, then the obvious solution is to error out,
> > which creates the inconsistency mentioned above.
> > 
> > Another solution would be to pick up a parent by default (the current
> > one maybe?) but then we could return a rate higher than the target rate
> > which breaks what we've been doing so far.
> > 
> > I'm not sure if one is better than the other.
> 
> We should pick a rounding policy that we want for the test. The test
> shouldn't be checking the rounding policy. It should be checking that
> the code under test does what is expected when the rounding policy is
> what we choose.
> 
> If __clk_mux_determine_rate() returns an error when the rate is lower
> than the parent supports then we should test that as well and make sure
> it does return an error in this case. We can directly call
> __clk_mux_determine_rate() after registering the clks so that the
> function is isolated.

I guess my point is that, in general, we don't really expect an error in
clk_round_rate(). Most clocks will clamp the rates between the minimum
and maximum allowed and will select the closest parent or dividers to
match that rate. So we could end up with a rate fairly different from
the argument, but we'll get a rate.

For muxes using __clk_mux_determine_rate*() without CLK_SET_RATE_PARENT,
if the rate is too low (or if the range is loo low) and is right below
what one of the parent can provide, we end up with -EINVAL.

AFAIK, it's pretty much the only situation where that happens. A fixed
clock will always return its fixed rate for example, even if the rate is
very different from what we asked for. But for some reason, on some
muxes, with some rates, boom, EINVAL.

It's very unexpected and inconsistent to me. Hence the FIXME.

Maxime

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