Re: [PATCH 2/3] clk: keystone: don't cache clock rate

[Maxime Ripard <mripard@kernel.org>]

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

On Wed, Sep 24, 2025 at 09:26:17PM -0500, Randolph Sapp wrote:
> On Wed Sep 17, 2025 at 10:24 AM CDT, Kevin Hilman wrote:
> > Michael Walle <mwalle@kernel.org> writes:
> >
> >> The TISCI firmware will return 0 if the clock or consumer is not
> >> enabled although there is a stored value in the firmware. IOW a call to
> >> set rate will work but at get rate will always return 0 if the clock is
> >> disabled.
> >> The clk framework will try to cache the clock rate when it's requested
> >> by a consumer. If the clock or consumer is not enabled at that point,
> >> the cached value is 0, which is wrong.
> >
> > Hmm, it also seems wrong to me that the clock framework would cache a
> > clock rate when it's disabled.  On platforms with clocks that may have
> > shared management (eg. TISCI or other platforms using SCMI) it's
> > entirely possible that when Linux has disabled a clock, some other
> > entity may have changed it.
> >
> > Could another solution here be to have the clk framework only cache when
> > clocks are enabled?
> 
> So I looked into that. There are still about 34 clock operations that are
> functionally uncached, but it does seem more logical than treating everything as
> uncached.
> 
> Side note, why would someone even want to read the rate of an unprepared clock?
> I dumped some debug info for all the clocks tripping this new uncached path.
> Seems weird that it's even happening this often. Even weirder that it's
> apparently happening 3 times to cpu0's core clock on the board I'm currently
> testing.

The short, unsatisfying, answer is that the API explicitly allowed it so far.

It's also somewhat natural when you have a functional rate to set it up
before enabling it and the logic driven by it so that you would avoid a
rate change, or something like a cycle where you would enable, shut
down, reparent, enable the clock again.

In such a case, we would either need the cache, or to read the rate, to
know if we have to change the clock rate in the first place.

Maxime

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