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

[Sam Protsenko]

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?

Would be also nice to hear what maintainers think about that :)

Thanks!

> > Michael
> >
> > [1] https://patchwork.amarulasolutions.com/patch/3291/
> >
> > >

[snip]