Re: [PATCH v4] clk: Introduce 'always-on-clocks' property

[Stephen Boyd <sboyd@kernel.org>]

Quoting Marek Vasut (2022-09-24 10:45:17)
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index b70769d0db99f..6b07f1a086277 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3900,6 +3900,48 @@ static void clk_core_free_parent_map(struct clk_core *core)
>         kfree(core->parents);
>  }
>  
> +static void
> +__clk_register_critical_clock(struct clk_core *core, struct clk_hw *hw)
> +{
> +       struct device_node *np = core->of_node;
> +       struct of_phandle_args clkspec;
> +       u32 clksize, clktotal;
> +       int ret, i, index;
> +
> +       if (!np)
> +               return;
> +
> +       if (of_property_read_u32(np, "#clock-cells", &clksize))
> +               return;
> +
> +       /* Clock node with #clock-cells = <0> uses always-on-clocks; */
> +       if (clksize == 0) {
> +               if (of_property_read_bool(np, "always-on-clocks"))
> +                       core->flags |= CLK_IS_CRITICAL;

Why must we set the CLK_IS_CRITICAL flag like this? Instead, when the
clk provider is registered, parse the node of the provider and get the
clks to call clk_prepare_enable() on. We can set the critical flag or
make a new flag that causes clk_disable_unprepare() to not actually turn
the clk off, if we have some sort of underflow issue with other
consumers. Does that fail somehow?

> +               return;
> +       }
> +
> +       if (!core->ops->match_clkspec)
> +               return;
> +
> +       clkspec.np = np;
> +       clktotal = of_property_count_u32_elems(np, "always-on-clocks");
> +       clktotal /= clksize;
> +       for (index = 0; index < clktotal; index++) {
> +               for (i = 0; i < clksize; i++) {

I'm mainly thinking that we're going to spin on this loop constantly for
any clk providers that have many clks to register, but only a few to
keep always on. It would be best to avoid that and only run through the
DT property once.

> +                       ret = of_property_read_u32_index(np, "always-on-clocks",
> +                                                        (index * clksize) + i,
> +                                                        &(clkspec.args[i]));
> +                       if (ret) {
> +                               pr_warn("Skipping always-on-clocks index %d (ret=%d)\n",
> +                                       i, ret);
> +                       }
> +               }
> +               if (!core->ops->match_clkspec(hw, &clkspec))

This callback is provider specific, and not necessarily clk_hw specific.
For example, the clk_ops could be for a generic gate bit, but the
provider wants to keep that gate always on. To implement such a clk we
would have to copy the generic gate clk_ops and set this match_clkspec
op. I'd like to avoid that if possible. If the clk_op must exist, then
perhaps it should be in clk_init_data, which is sort of the place where
we put provider specific information for a clk, i.e. clk_parent_data.

> +                       core->flags |= CLK_IS_CRITICAL;
> +       }
> +}
> +
>  static struct clk *
>  __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
>  {