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

From: Neil Armstrong <narmstrong@baylibre.com>
To: Maxime Ripard <maxime@cerno.tech>, Jerome Brunet <jbrunet@baylibre.com>
Cc: Mike Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>,
	linux-clk@vger.kernel.org,
	Naresh Kamboju <naresh.kamboju@linaro.org>,
	Alexander Stein <alexander.stein@ew.tq-group.com>,
	Marek Szyprowski <m.szyprowski@samsung.com>,
	Tony Lindgren <tony@atomide.com>,
	Yassine Oudjana <y.oudjana@protonmail.com>
Subject: Re: [PATCH 22/22] clk: Prevent a clock without a rate to register
Date: Mon, 11 Apr 2022 09:40:03 +0200	[thread overview]
Message-ID: <a30e6471-eb1d-cf41-9ca9-cf33fd570c6a@baylibre.com> (raw)
In-Reply-To: <20220408153625.ugodcmfwtanr75gu@houat>

On 08/04/2022 17:36, Maxime Ripard wrote:
> On Fri, Apr 08, 2022 at 04:48:08PM +0200, Jerome Brunet wrote:
>>>>> and hdmi_pll_dco because it will always return 0,
>>>>
>>>> It is a read-only clock - whatever we do in CCF, it is not going to
>>>> change. CCF has always supported RO clocks.
>>>
>>> Yes, if a clock has a rate of 0Hz it's entirely useless. And if it's
>>> useful in anyway, it should report its current rate. Read-only or not.
>>>
>>
>> It does report its rate, it does not have any.
>> ... and It would pretty weird to initialize a read-only clock.
> 
> The KMS driver doesn't seem to have a problem with that.
> 
>>>>> unless the display driver comes around and updates it. If it never does,
>>>>> or if it's not compiled in, then you're out of luck.
>>>>
>>>> I'm all for managing the display clocks from CCF but it has proved tricky
>>>> so far. Maybe it will someday.
>>>>
>>>> Being a read-only clock, the value is what it is and CCF should
>>>> deal with it gracefully. It has so far.
>>>>
>>>> If the driver actually managing the clock is not compiled in, then the
>>>> clock will never be set, and it should not be a problem either.
>>>
>>> Again, it depends on what you expect from clk_get_rate(). If it can only
>>> report a rate of 0 on error, then it's definitely a problem.
>>
>> Agreed, it depends on what you expect from clk_get_rate().
>> Still when something does not oscillate, the rate is 0.
>>
>> If a driver call clk_get_rate() on an uninitialized, unlocked PLL, I
>> think returning 0 makes sense.
> 
> You're confusing two things: the rate output by the hardware, and what
> the CCF needs to return. We're discussing the latter here, a software
> construct. It models the hardware, but it doesn't have to be true to the
> hardware.
> 
> And even the meson driver doesn't follow what you're claiming to the
> letter and is inconsistent with what you're saying. Any disabled gate
> will also have a hardware rate of 0. Yet, it doesn't return 0 in such a
> case. And no one does, because clk_get_rate() isn't supposed to return
> the actual rate in hardware at the moment. It's supposed to return what
> would be the rate if it was enabled.

You're pointing a real issue, what should we return as a rate then ?

The rate is not only the rate output by the HW but is also used by the whole
subtree of the PLL to calculate the rates.

If we return a fake or dummy rate instead of 0 (which is the physical reality),
the clock subtree will be populated from a fake rate and if we did set a clock
rate using assigned-clock, eventually this fake rate would match and no set_rate()
would be called on the PLL.

This is why we return the true physical rate, it's the software to adapt in order
to handle the possible HW states.

> 
>>> And it's not because it was done before that it wasn't just as
>>> problematic then.
>>>
>>>>>> 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.
>>>>>
>>>>> If there's not a sensible default in the hardware already, I don't see
>>>>> what the big issue is to be honest. You already kind of do that for all
>>>>> the other PLL parameters with init_regs
>>>>
>>>> Those initial parameters are "magic" analog setting which don't have an
>>>> impact on the rate setting. The initial rate of the clock is never set
>>>> by the clock driver on purpose.
>>>
>>> It's still fundamentally the same: whatever is there at boot isn't good
>>> enough, so you change it to have a somewhat sensible default.
>>
>> If you don't need it, no.
> 
> I mean, I provided multiple examples that the meson driver is being
> inconsistent with both the CCF documentation and the expected usage of
> the CCF consumers. And I suggested solutions to fix this inconsistency
> both here and on IRC to Neil and you.

It's not that we don't like it, we only care the actual HW state is correctly
reported in the clock tree, nothing else.

If a new flag like CLK_INVALID_RATE existed then we would use it.

But so far a zero rate never was problematic, and iMX8mp HDMI PLL and MSM mmpll2
also return a 0 rate as init because it's probably the default PLL state at
HW startup.

> 
> The only argument you provided so far is that you don't like or want it.
> I'm sorry you feel this way, but it doesn't change either the consensus
> or the documentation that every other driver and consumer is following.
> Nor does it provide any solution.
> 
> In the grand scheme of things, I don't care, if you want to have your
> own conventions a policies, go ahead. Just don't expect me to debug
> whatever is going on next time it falls apart.
> 
> Maxime

Neil