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

[Jerome Brunet <jbrunet@baylibre.com>]

On Fri 08 Apr 2022 at 14:55, Maxime Ripard <maxime@cerno.tech> wrote:

>> 
>> No, they don't expect anything. They will return 0 until they are set
>> with a an actual rate. I don't think returning 0 should be
>> problem and it has not been so far.
>
> So, if I was to rephrase, gp0_pll_dco and hifi_pll_dco expect someone to
> call clk_set_rate() before clk_get_rate() to have it return anything
> other than 0.

Yes. It has no rate. I won't change until something make it so

>
>> I understand the ambiguity you mentioned above. If the framework decides
>> it is clearly forbidden to return 0, we'll change.
>> 
>> Still I don't think it would be wise. What are the alternative if you
>> can't compute a rate ? return 1 ? This looks aweful to me. At least 0 is
>> a clear indication that the clock is not in a working state.
>
> No, the alternative would be to initialize the clock to something
> sensible before registering it (or in init).

Well, I disagree :/

>
>> > 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.

>> > 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.

>
> 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.

>
>> > , and most drivers do that for
>> > various stuff:
>> > https://elixir.bootlin.com/linux/latest/source/drivers/clk/imx/clk-imx6q.c#L917
>> > https://elixir.bootlin.com/linux/latest/source/drivers/clk/nxp/clk-lpc32xx.c#L1550
>> > https://elixir.bootlin.com/linux/latest/source/drivers/clk/rockchip/clk-rk3036.c#L448
>> > https://elixir.bootlin.com/linux/latest/source/drivers/clk/sunxi-ng/ccu-sun8i-h3.c#L1157
>> > https://elixir.bootlin.com/linux/latest/source/drivers/clk/tegra/clk-tegra20.c#L1013
>> 
>> It is done by other drivers or controllers, yes. It does not make it
>> right (again, it is just my opinion). Rate should never be set by the
>> clock driver or the clock controller - Those should just implement what
>> consumer wants. I would agree it sometimes proves tricky to hold onto
>> this.
>> 
>> Taking one of the example above:
>> https://elixir.bootlin.com/linux/latest/source/drivers/clk/nxp/clk-lpc32xx.c#L1550
>> 
>> I think it would be better to have an "assigned-clock" on the related
>> PLL in the USB node of the platform DT. That way the PLL is set when
>> needed.
>
> Both are valid. Assigned-clocks is arguably more fragile, but that's not
> really the point.
>
>> If we go down the road of "others are doing it, so why not ?", I think Marek
>> initial regression report mentioned Exynos too ;) 
>
> Yes, he did mention exynos as well, but the issue was entirely
> different.
>
> Exynos had the issue that req_rate wasn't updated whenever a clock
> wasn't orphan anymore because it changed parent. It's fixed in patch 10.
>
> Out of the drivers I tested this week (sunxi-ng, exynos, omap3, qcom,
> imx6, imx8 and g12b), only meson is in this case.
>
>> > If the driver needs that kind of quirks in order to make the clock
>> > usable in itself, then it just makes sense to do that, especially if
>> > it's to avoid breaking a generic API.
>> 
>> As it is the clock are usable and it did not break anything so far.
>
> Anything but the abstraction.
>
>> I have no problem updating the drivers if need be. I do have a problem
>> with the framework changing and requiring the clock driver to set an
>> initial rate to make it happy.
>
> I mean, the alternative is changing the semantics of clk_get_rate(), and
> all the call sites. Fixing one inconsistent driver definitely seems
> preferable.
>
> Maxime
>
> [[End of PGP Signed Part]]