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-mwa...@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 <mwa...@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 <khil...@baylibre.com>
> >
> > Thanks!
> >
> > -michael
signature.asc
Description: PGP signature
