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

[Marek Vasut <marex@denx.de>]

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.