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

[Randolph Sapp <rs@ti.com>]

On Thu Sep 25, 2025 at 6:43 AM CDT, Maxime Ripard wrote:
> 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

Thanks Maxime. I'll take this as a hint to stop digging for the moment. Right
now, treating unprepared clocks as untrusted / uncached makes sense and
shouldn't be too much of a performance issue.

- Randolph