Message-Id: <DCWL7R9MZLH1.2FR5WWQEKB638@kernel.org>
Date: Fri, 19 Sep 2025 09:10:21 +0200
From: "Michael Walle" <mwalle@...nel.org>
To: "Dhruva Gole" <d-gole@...com>
Cc: "Kevin Hilman" <khilman@...nel.org>, "Frank Binns"
 <frank.binns@...tec.com>, "Matt Coster" <matt.coster@...tec.com>, "Maarten
 Lankhorst" <maarten.lankhorst@...ux.intel.com>, "Maxime Ripard"
 <mripard@...nel.org>, "Thomas Zimmermann" <tzimmermann@...e.de>, "David
 Airlie" <airlied@...il.com>, "Simona Vetter" <simona@...ll.ch>, "Rob
 Herring" <robh@...nel.org>, "Krzysztof Kozlowski" <krzk+dt@...nel.org>,
 "Conor Dooley" <conor+dt@...nel.org>, "Nishanth Menon" <nm@...com>,
 "Vignesh Raghavendra" <vigneshr@...com>, "Tero Kristo" <kristo@...nel.org>,
 "Santosh Shilimkar" <ssantosh@...nel.org>, "Michael Turquette"
 <mturquette@...libre.com>, "Stephen Boyd" <sboyd@...nel.org>, "Andrew
 Davis" <afd@...com>, <dri-devel@...ts.freedesktop.org>,
 <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 <linux-arm-kernel@...ts.infradead.org>, <linux-clk@...r.kernel.org>
Subject: Re: [PATCH 2/3] clk: keystone: don't cache clock rate

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@...nel.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@...libre.com>
> > 
> > Thanks!
> > 
> > -michael


Download attachment "signature.asc" of type "application/pgp-signature" (298 bytes)

Powered by blists - more mailing lists