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

3172 diff mbox series

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@linaro.org>
Signed-off-by: Michael Trimarchi <michael@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
---

On Fri, Jul 5, 2024 at 2:14 AM Michael Trimarchi
<michael@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@linaro.org>
> Signed-off-by: Michael Trimarchi <michael@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
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.

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
>

To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.

Hi Sam

On Tue, Jul 23, 2024 at 10:37 PM Sam Protsenko
<semen.protsenko@linaro.org> wrote:
>
> On Fri, Jul 5, 2024 at 2:14 AM Michael Trimarchi
> <michael@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@linaro.org>
> > Signed-off-by: Michael Trimarchi <michael@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
> >

Hi Sam

On Wed, Jul 24, 2024 at 9:25 AM Michael Nazzareno Trimarchi
<michael@amarulasolutions.com> wrote:
>
> Hi Sam
>
> On Tue, Jul 23, 2024 at 10:37 PM Sam Protsenko
> <semen.protsenko@linaro.org> wrote:
> >
> > On Fri, Jul 5, 2024 at 2:14 AM Michael Trimarchi
> > <michael@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@linaro.org>
> > > Signed-off-by: Michael Trimarchi <michael@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@amarulasolutions.com
> __________________________________
>
> Amarula Solutions BV
> Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> T. +31 (0)85 111 9172
> info@amarulasolutions.com
> www.amarulasolutions.com

On Wed, Jul 24, 2024 at 4:44 AM Michael Nazzareno Trimarchi
<michael@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]

To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.

Hi Sam

On Thu, Jul 25, 2024 at 4:41 AM Sam Protsenko
<semen.protsenko@linaro.org> wrote:
>
> On Wed, Jul 24, 2024 at 4:44 AM Michael Nazzareno Trimarchi
> <michael@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]