Hi Michael, On Sep 18, 2025 at 11:48:34 +0200, Michael Walle wrote: > On Wed Sep 17, 2025 at 5:24 PM CEST, Kevin Hilman wrote: > > Michael Walle <mwa...@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? > > 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/ > > > > 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) 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 -- Best regards, Dhruva Gole Texas Instruments Incorporated
