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

Michael Nazzareno Trimarchi michael at amarulasolutions.com
Wed Jul 24 11:44:35 CEST 2024


Hi Sam

On Wed, Jul 24, 2024 at 9:25 AM Michael Nazzareno Trimarchi
<michael at amarulasolutions.com> wrote:
>
> 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()
> }
>

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?


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



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


More information about the U-Boot mailing list