On 10/4/22 20:26, Stephen Boyd wrote:
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?
I don't quite understand the question here. Clock which shouldn't be turned off should have CLK_IS_CRITICAL flag set.
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?
Would your proposal be something that would have to be implemented in every single clock driver separately ?
+ 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.
Where could this be implemented in the clock core ?
+ 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.
The core->ops is copied from struct clk_init_data .ops in __clk_register() , just before __clk_register_critical_clock() is called, so that op is already in clk_init_data.
