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

[Sam Protsenko]

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