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

Maxime Ripard
2022-04-12T13:12:47Z

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

On Mon, Apr 11, 2022 at 09:40:03AM +0200, Neil Armstrong wrote:
> 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.

I'm not sure why you insist so much on the "true physical rate" tbh.
That was never really a thing. It's not for a gate, it's not for a
spread-spectrum clock (which would be even more interesting to report
properly following your criteria). And then you have the clock
accuracies that are throwing it off entirely too.

It just isn't practical, and that's why no-one relies on it.

Or let's turn it the other way around. When you'll have called
clk_enable() on those clocks (but only that), what do you expect the
rate to be? 0? Yet, the "true physical rate" probably changed by then.

If that was a thing, we would also have to:

* make sure clk_set_rate can't be called on a disabled clock, because
the true physical rate would never match what is required

* rate ranges just wouldn't be usable either, because we would have no
way to check that the true physical rate is actually within that
range at any given time.

* All clocks rate wouldn't be cacheable to account for the jitter in
the system clock source.

* We would need to use some hardware engine that reads the clock rate
being output at any given time, to account for spread-spectrum and
the clock source accuracy.

* We wouldn't be able to report phases properly, because you can't
really measure the clock skew either.

I understand that it's convenient for you, but it isn't practical if we
look at it from the framework's PoV.

You could very well initialize the PLLs to have a 1/1 ratio to their
parent, that would probably be sensible and would make the framework
happy.

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

That patch isn't upstream.

> and MSM mmpll2 also return a 0 rate as init because it's probably the
> default PLL state at HW startup.

And two wrongs never made a right.

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