Re: [PATCH 22/22] clk: Prevent a clock without a rate to register

Marek Szyprowski
2022-04-08T12:18:06Z

Hi All,

On 08.04.2022 11:18, Jerome Brunet wrote:
> On Fri 08 Apr 2022 at 11:10, Maxime Ripard <maxime@cerno.tech> wrote:
>
>> A rate of 0 for a clock is considered an error, as evidenced by the
>> documentation of clk_get_rate() and the code of clk_get_rate() and
>> clk_core_get_rate_nolock().
>>
>> The main source of that error is if the clock is supposed to have a
>> parent but is orphan at the moment of the call. This is likely to be
>> transient and solved later in the life of the system as more clocks are
>> registered.
>>
>> The corollary is thus that if a clock is not an orphan, has a parent that
>> has a rate (so is not an orphan itself either) but returns a rate of 0,
>> something is wrong in the driver. Let's return an error in such a case.
>>
>> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
>> ---
>> drivers/clk/clk.c | 10 ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index 8bbb6adeeead..e8c55678da85 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -3773,6 +3773,16 @@ static int __clk_core_init(struct clk_core *core)
>> rate = 0;
>> core->rate = core->req_rate = rate;
>>
>> + /*
>> + * If we're not an orphan clock and our parent has a rate, then
>> + * if our rate is 0, something is badly broken in recalc_rate.
>> + */
>> + if (!core->orphan && (parent && parent->rate) && !core->rate) {
>> + ret = -EINVAL;
>> + pr_warn("%s: recalc_rate returned a null rate\n", core->name);
>> + goto out;
>> + }
>> +
> As hinted in the cover letter, I don't really agree with that.
>
> There are situations where we can't compute the rate. Getting invalid
> value in the register is one reason.
>
> You mentioned the PLLs of the Amlogic SoCs (it is not limited to g12 - all
> SoCs would be affected):
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/meson/clk-pll.c#n82
> Yes, PLL that have not been previously used (by the ROMCode or the
> bootloader) tend to have the value of the divider set to 0 which in
> invalid as it would result in a division by zero.
>
> I don't think this is a bug. It is just what the HW is, an unlocked,
> uninitialized PLL. There is no problem here and the PLL can remain like
> that until it is needed.
>
> IMO, whenever possible we should not put default values in the clocks
> which is why I chose to leave it like that.
>
> The PLL being unlocked, it has no rate. It is not set to have any rate.
> IMO a returning a rate of 0 is valid here.

I agree that it should be possible to register so called unconfigured
clock, which doesn't have any rate set yet. It might be used later by
some drivers, which will do the whole initialization (set rate and or
parents). I also wonder why the above patch triggers warning on the
Amlogic Socs, while on Exynos I still have some unused clocks with 0
rate registered properly.

Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland