[PATCH V2] clk: Propagate clk_set_rate() if CLK_SET_PARENT_RATE present for gate and mux

[Michael Nazzareno Trimarchi]

Hi Sam

On Thu, Jul 25, 2024 at 4:41 AM Sam Protsenko
<semen.protsenko at linaro.org> wrote:
>
> On Wed, Jul 24, 2024 at 4:44 AM Michael Nazzareno Trimarchi
> <michael at amarulasolutions.com> wrote:
>
> [snip]
>
> > > Ok , so you suggest to have something like in the core
> >
> > diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
> > index 8faf5a56e1..a309108dd4 100644
> > --- a/drivers/clk/clk-uclass.c
> > +++ b/drivers/clk/clk-uclass.c
> > @@ -467,8 +467,14 @@ ulong clk_get_rate(struct clk *clk)
> >                 return 0;
> >         ops = clk_dev_ops(clk->dev);
> >
> > -       if (!ops->get_rate)
> > -               return -ENOSYS;
> > +       if (!ops->get_rate) {
> > +               parent = clk_get_parent(clk);
> > +               if (!clk_is_valid(parent))
> > +                       return -ENOSYS;
> > +               ops = clk_dev_ops(parent->dev);
> > +               if (!ops->get_rate)
> > +                       return -ENOSYS;
> > +       }
> >
> >         return ops->get_rate(clk);
> >  }
> >
> > You suggest to remove get_rate, set_rate function in mux and gate and
> > use some propagation in the core, am I right?
>
> Something like that, yes. But maybe instead of
>
>     if (!ops->get_rate)
>
> use something like:
>
>     while (!ops->get_rate)
>
> like I did in my patch [1] for clk_set_rate(). That while loop is
> needed to handle cases when there are multiple clocks without
> .get_rate() operation connected together (like mux or gate clocks). So
> you'd have to iterate them all up the tree to find some DIV clock
> which actually has .get_rate() defined.
>
> But at the moment I'm more concerned about .set_rate() propagation of
> course, which is really needed for my E850-96 MMC enablement series
> [2], which has already been pending on the review for a while.
>
> [1] https://lists.denx.de/pipermail/u-boot/2024-March/547719.html
> [2] https://lists.denx.de/pipermail/u-boot/2024-July/559602.html
>
> [snip]
>
> > > > implementations simple and brings the propagation logic to the actual
> > > > clk API (clk_set_rate()), which in turn is able to cover all possible
> > > > cases: even if some new clock types are implemented later, they will
> > > > be covered already. That also reduces some code duplication and makes
> > > > it easier to control that behavior in a centralized manner. The patch
> > > > with the discussed implementation was already submitted a while ago
> > > > [2], and still pending on the review.
> > >
> > > I have seen it and because I'm extending the clk framework a bit more [1]
> > > I was thinking about a more complete approach. Nothing against your
> > > patch, but just
> > > I have observed in my use case (display) that I need more from the
> > > clock framework we have
> > >
>
> I see. Do you think it'd possible for you to use my patch for
> clk_set_rate() propagation for your case, and base your patches (for
> CLK_OPS_PARENT_ENABLE, etc) on top of that? Will it cover your
> requirements?
>

If I remember how it works now we don't need the loop. Each get_clk_rate
or clk_set_rate should implement propagation in their implementation if the
CLK PARENT SET is present

Michael


> Would be also nice to hear what maintainers think about that :)
>
> Thanks!
>
> > > Michael
> > >
> > > [1] https://patchwork.amarulasolutions.com/patch/3291/
> > >
> > > >
>
> [snip]



-- 
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael at amarulasolutions.com
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info at amarulasolutions.com
www.amarulasolutions.com