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