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

["Michael Walle" <mwalle@kernel.org>]

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

Hi,

> > > > 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?
> > 
> > It's not just the clock which has to be enabled, but also it's
> > consumer. I.e. for this case, the GPU has to be enabled, until that
> > is the case the get_rate always returns 0. The clk framework already
> > has support for the runtime power management of the clock itself,
> > see for example clk_recalc().
>
> Why did we move away from the earlier approach [1] again?
> [1] https://lore.kernel.org/all/20250716134717.4085567-3-mwalle@kernel.org/

Because that not fixing the root case. Also it probably doesn't work
if there is no assigned-clocks property. Nishanth asked for the
latter and the soc dtsi should rely on the hardware default.

> > > > Thus, disable the cache altogether.
> > > >
> > > > Signed-off-by: Michael Walle <mwalle@kernel.org>
> > > > ---
> > > > I guess to make it work correctly with the caching of the linux
> > > > subsystem a new flag to query the real clock rate is needed. That
> > > > way, one could also query the default value without having to turn
> > > > the clock and consumer on first. That can be retrofitted later and
> > > > the driver could query the firmware capabilities.
> > > >
> > > > Regarding a Fixes: tag. I didn't include one because it might have a
> > > > slight performance impact because the firmware has to be queried
> > > > every time now and it doesn't have been a problem for now. OTOH I've
> > > > enabled tracing during boot and there were just a handful
> > > > clock_{get/set}_rate() calls.
> > >
> > > The performance hit is not just about boot time, it's for *every*
> > > [get|set]_rate call.  Since TISCI is relatively slow (involves RPC,
> > > mailbox, etc. to remote core), this may have a performance impact
> > > elsewhere too.
> > 
> > Yes of course. I have just looked what happened during boot and
> > (short) after the boot. I haven't had any real application running,
> > though, so that's not representative.
>
> I am not sure what cpufreq governor you had running, but depending on the governor,
> filesystem, etc. cpufreq can end up potentially doing a lot more of
> the clk_get|set_rates which could have some series performance degradation
> is what I'm worried about. Earlier maybe the clk_get_rate part was
> returning the cached CPU freqs, but now it will each time go query the
> firmware for it (unnecessarily)

There doesn't seem to be a cpufreq compatible for the am67a (which
might make sense to add though). But I'm wondering how much energy
that will save because the SoC can't do voltage scaling.

-michael

> I currently don't have any solid data to say how much of an impact
> for sure but I can run some tests locally and find out...
>
> > 
> > > That being said, I'm hoping it's unlikely that
> > > [get|set]_rate calls are in the fast path.
> > >
> > > All of that being said, I think the impacts of this patch are pretty
> > > minimal, so I don't have any real objections.
> > >
> > > Reviewed-by: Kevin Hilman <khilman@baylibre.com>
> > 
> > Thanks!
> > 
> > -michael


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