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

Maxime Ripard
2022-04-08T12:55:38Z

[-- Attachment #1: Type: text/plain, Size: 10200 bytes --]

On Fri, Apr 08, 2022 at 01:24:59PM +0200, Jerome Brunet wrote:
> On Fri 08 Apr 2022 at 12:41, Maxime Ripard <maxime@cerno.tech> wrote:
> > [[PGP Signed Part:Undecided]]
> > On Fri, Apr 08, 2022 at 11:18:58AM +0200, 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.
> >
> > I think the larger issue is around the semantics of clk_get_rate(), and
> > especially whether we can call it without a clk_enable(), and whether
> > returning 0 is fine.
> >
> > The (clk.h) documentation of clk_get_rate() mentions that "This is only
> > valid once the clock source has been enabled", and it's fairly
> > ambiguous. I can see how it could be interpreted as "you need to call
> > clk_enable() before calling clk_get_rate()", but it can also be
> > interpreted as "The returned rate will only be valid once clk_enable()
> > is called".
> >
> > I think the latter is the proper interpretation though based on what the
> > drivers are doing, and even the CCF itself will call recalc_rate without
> > making sure that the clock is enabled (in __clk_core_init() for example).
> >
> > Then there is the question of whether returning 0 is fine. Again
> > clk_get_rate() (clk.c) documentation states that "If clk is NULL then
> > returns 0.". This is indeed returned in case of an error condition (in
> > clk_get_rate() itself, but also in clk_core_get_rate_nolock()).
> >
> > All the drivers I could find either assume the rate is valid, or test
> > whether it's 0 or not (randomly picked, but across completely different
> > platforms):
> > https://elixir.bootlin.com/linux/latest/source/drivers/clocksource/armv7m_systick.c#L50
> > https://elixir.bootlin.com/linux/latest/source/drivers/cpufreq/armada-8k-cpufreq.c#L74
> > https://elixir.bootlin.com/linux/latest/source/sound/soc/sti/uniperif_player.c#L194
> > https://elixir.bootlin.com/linux/latest/source/sound/soc/tegra/tegra20_i2s.c#L278
> >
> > So my understanding is that the consensus is that clk_get_rate() can be
> > called even if the clock hasn't been enabled, and that returning 0 is
> > only meant to be used for errors in general, a NULL pointer according to
> > the documentation.
> >
> > That would mean that pcie_pll_dco is buggy because it assumes that
> > clk_enable()
>
> This one indeed does everything in the enable and I could agree it is
> fishy, but since it supports only a single rate I don't think it is a
> problem. Even if it had a proper set_rate(), it would not change your
> problem since it would still return 0 until some consumer actually needs
> its parameter to change.
>
> > is going to be called before clk_get_rate(), gp0_pll_dco
> > and hifi_pll_dco because they expect "someone" to call clk_set_rate()
> > before clk_get_rate(),
>
> 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.

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

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

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

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.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]