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

[Michael Nazzareno Trimarchi]

Hi Sam

On Tue, Jul 23, 2024 at 10:37 PM Sam Protsenko
<semen.protsenko at linaro.org> wrote:
>
> On Fri, Jul 5, 2024 at 2:14 AM Michael Trimarchi
> <michael at amarulasolutions.com> wrote:
> >
> > Gate and mux does not have .set_rate operation, but they could have
> > CLK_SET_PARENT_RATE flag set. In that case it's usually possible to find a
> > parent up the tree which is capable of setting the rate (div, pll, etc).
> > Add clk_generic_set_rate to allow them to trasverse the clock tree.
> >
> > Cc: Sam Protsenko <semen.protsenko at linaro.org>
> > Signed-off-by: Michael Trimarchi <michael at amarulasolutions.com>
> > ---
> >  drivers/clk/clk-gate.c       |  1 +
> >  drivers/clk/clk-mux.c        |  2 +-
> >  drivers/clk/clk-uclass.c     | 20 ++++++++++++++++++++
> >  drivers/clk/clk.c            |  9 +++++++++
> >  include/clk.h                |  9 +++++++++
> >  include/linux/clk-provider.h |  1 +
> >  6 files changed, 41 insertions(+), 1 deletion(-)
> > ---
> > V1->V2:
> >         Fix missing include
> > ---
> > diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c
> > index cfd90b717e7..c86083ac5a3 100644
> > --- a/drivers/clk/clk-gate.c
> > +++ b/drivers/clk/clk-gate.c
> > @@ -116,6 +116,7 @@ const struct clk_ops clk_gate_ops = {
> >         .enable = clk_gate_enable,
> >         .disable = clk_gate_disable,
> >         .get_rate = clk_generic_get_rate,
> > +       .set_rate = clk_generic_set_rate,
>
> I can see that clk_generic_get_rate() already exists here, and adding
> clk_generic_set_rate() tries to mimic that existing design. But
> frankly, I'm not sure it was a very good design choice in the first
> place. I mean, to have get/set rate operations for the clock types
> that clearly don't have such ability. I think it would be better to
> model the CCF clocks after real world clocks, and handle the
> propagation on the clock logic level. In fact, that's how it's already
> implemented in Linux kernel [1]. That keeps the particular clock

Ok , so you suggest to have something like in the core

if (p->set_rate)
{
     set_rate())

}
else
{
    set_generic_rate()
}

The same for get_rate()

> 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

Michael

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

>
> Can I ask you if patch [2] doesn't cover some of your cases? Or maybe
> I'm missing something out and having .set_rate op in gate/mux would
> actually be better for some particular reason?
>
> Thanks!
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-gate.c#n120
> [2] https://lists.denx.de/pipermail/u-boot/2024-March/547719.html
>
>
> >  };
> >
> >  struct clk *clk_register_gate(struct device *dev, const char *name,
> > diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c
> > index e3481be95fa..f99a67ebd35 100644
> > --- a/drivers/clk/clk-mux.c
> > +++ b/drivers/clk/clk-mux.c
> > @@ -151,13 +151,13 @@ static int clk_mux_set_parent(struct clk *clk, struct clk *parent)
> >  #else
> >         writel(reg, mux->reg);
> >  #endif
> > -
> >         return 0;
> >  }
> >
> >  const struct clk_ops clk_mux_ops = {
> >         .get_rate = clk_generic_get_rate,
> >         .set_parent = clk_mux_set_parent,
> > +       .set_rate = clk_generic_set_rate,
> >  };
> >
> >  struct clk *clk_hw_register_mux_table(struct device *dev, const char *name,
> > diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
> > index ed6e60bc484..638864e6774 100644
> > --- a/drivers/clk/clk-uclass.c
> > +++ b/drivers/clk/clk-uclass.c
> > @@ -517,6 +517,26 @@ ulong clk_get_parent_rate(struct clk *clk)
> >         return pclk->rate;
> >  }
> >
> > +ulong clk_set_parent_rate(struct clk *clk, ulong rate)
> > +{
> > +       const struct clk_ops *ops;
> > +       struct clk *pclk;
> > +
> > +       debug("%s(clk=%p)\n", __func__, clk);
> > +       if (!clk_valid(clk))
> > +               return 0;
> > +
> > +       pclk = clk_get_parent(clk);
> > +       if (IS_ERR(pclk))
> > +               return -ENODEV;
> > +
> > +       ops = clk_dev_ops(pclk->dev);
> > +       if (!ops->set_rate)
> > +               return -ENOSYS;
> > +
> > +       return clk_set_rate(pclk, rate);
> > +}
> > +
> >  ulong clk_round_rate(struct clk *clk, ulong rate)
> >  {
> >         const struct clk_ops *ops;
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 6ede1b4d4dc..febd5314df2 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -14,6 +14,7 @@
> >  #include <dm/uclass.h>
> >  #include <dm/lists.h>
> >  #include <dm/device-internal.h>
> > +#include <linux/clk-provider.h>
> >
> >  int clk_register(struct clk *clk, const char *drv_name,
> >                  const char *name, const char *parent_name)
> > @@ -61,6 +62,14 @@ ulong clk_generic_get_rate(struct clk *clk)
> >         return clk_get_parent_rate(clk);
> >  }
> >
> > +ulong clk_generic_set_rate(struct clk *clk, ulong rate)
> > +{
> > +       if (clk->flags & CLK_SET_RATE_PARENT)
> > +               return clk_set_parent_rate(clk, rate);
> > +
> > +       return clk_get_parent_rate(clk);
> > +}
> > +
> >  const char *clk_hw_get_name(const struct clk *hw)
> >  {
> >         assert(hw);
> > diff --git a/include/clk.h b/include/clk.h
> > index af23e4f3475..1900377eddb 100644
> > --- a/include/clk.h
> > +++ b/include/clk.h
> > @@ -452,6 +452,15 @@ struct clk *clk_get_parent(struct clk *clk);
> >   */
> >  ulong clk_get_parent_rate(struct clk *clk);
> >
> > +/**
> > + * clk_set_parent_rate() - Set parent of current clock rate.
> > + * @clk:       A clock struct that was previously successfully requested by
> > + *             clk_request/get_by_*().
> > + *
> > + * Return: clock rate in Hz, or -ve error code.
> > + */
> > +ulong clk_set_parent_rate(struct clk *clk, ulong rate);
> > +
> >  /**
> >   * clk_round_rate() - Adjust a rate to the exact rate a clock can provide
> >   * @clk: A clock struct that was previously successfully requested by
> > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> > index 59f9c241b84..459fa2d15ce 100644
> > --- a/include/linux/clk-provider.h
> > +++ b/include/linux/clk-provider.h
> > @@ -253,6 +253,7 @@ struct clk *clk_register_fixed_rate(struct device *dev, const char *name,
> >
> >  const char *clk_hw_get_name(const struct clk *hw);
> >  ulong clk_generic_get_rate(struct clk *clk);
> > +ulong clk_generic_set_rate(struct clk *clk, ulong rate);
> >
> >  struct clk *dev_get_clk_ptr(struct udevice *dev);
> >
> > --
> > 2.43.0
> >



-- 
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