Hi,

On Thu, Jun 08, 2023 at 06:41:56PM -0700, Stephen Boyd wrote:
> Quoting Maxime Ripard (2023-05-05 04:25:09)
> > The single parent clock in our kunit tests implements a mux with a
> > set_parent hook, but doesn't provide a determine_rate implementation.
> > 
> > This is not entirely unexpected, since its whole purpose it to have a
> > single parent. When determine_rate is missing, and since
> > CLK_SET_RATE_PARENT is set for all its instances, the default behaviour
> > of the framework will be to forward it to the current parent.
> > 
> > This is totally fine as far as the tests are concerned, but we'll start
> > to mandate a determine_rate implementation when set_parent is set, so
> > let's fill it with __clk_mux_determine_rate() which will have the same
> > behavior.
> > 
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> >  drivers/clk/clk_test.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
> > index b3ed3b0e4c31..a154ec9d0111 100644
> > --- a/drivers/clk/clk_test.c
> > +++ b/drivers/clk/clk_test.c
> > @@ -104,6 +104,23 @@ static const struct clk_ops clk_dummy_minimize_rate_ops = {
> >  };
> >  
> >  static const struct clk_ops clk_dummy_single_parent_ops = {
> > +       /*
> > +        * FIXME: Even though we should probably be able to use
> 
> Are we ever going to fix this?

We probably should, but I'm not entirely sure how.

> > +        * __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.
Maxime