On 08:55-20260102, Michael Walle wrote: > On Tue Dec 30, 2025 at 9:12 PM CET, Nishanth Menon wrote: > > On 13:47-20251223, Michael Walle wrote: > >> 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. Thus, disable the cache > >> altogether. > >> > >> Signed-off-by: Michael Walle <[email protected]> > >> Reviewed-by: Kevin Hilman <[email protected]> > >> Reviewed-by: Randolph Sapp <[email protected]> > >> --- > >> drivers/clk/keystone/sci-clk.c | 8 ++++++++ > >> 1 file changed, 8 insertions(+) > >> > >> diff --git a/drivers/clk/keystone/sci-clk.c > >> b/drivers/clk/keystone/sci-clk.c > >> index 9d5071223f4c..0a1565fdbb3b 100644 > >> --- a/drivers/clk/keystone/sci-clk.c > >> +++ b/drivers/clk/keystone/sci-clk.c > >> @@ -333,6 +333,14 @@ static int _sci_clk_build(struct sci_clk_provider > >> *provider, > >> > >> init.ops = &sci_clk_ops; > >> init.num_parents = sci_clk->num_parents; > >> + > >> + /* > >> + * A clock rate query to the SCI firmware will return 0 if either the > >> + * clock itself is disabled or the attached device/consumer is disabled. > >> + * This makes it inherently unsuitable for the caching of the clk > >> + * framework. > >> + */ > >> + init.flags = CLK_GET_RATE_NOCACHE; > >> sci_clk->hw.init = &init; > >> > >> ret = devm_clk_hw_register(provider->dev, &sci_clk->hw); > >> -- > >> 2.47.3 > >> > > > > Reviewed-by: Nishanth Menon <[email protected]> > > > > I wish there was a better scheme, but inherently, just like SCMI and > > other systems where power management co-processor controls clocks, there > > is no real feasible caching scheme I can think of. I wonder if Stephen > > or others have a thought on this? > > > > That said, I wonder if we need fixes tag to this? I am sure there are > > other clocks susceptible to this as well. I wonder if > > commit 3c13933c6033 ("clk: keystone: sci-clk: add support for > > dynamically probing clocks") is the appropriate tag? > > >From my previous versions of this patch: > > > 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. > > I'm still undecided if this needs a Fixes tag or not. Strictly > speaking it would need one. Although, I'm not sure it's the one > you mentioned, because the culprit is the "we return 0 if the clock > or it's consumer is disabled", which then caches the wrong value. > So it is probably the very first commit b745c0794e2f ("clk: > keystone: Add sci-clk driver support").
Stephen: Can you pick the patch or suggest improvements? -- Regards, Nishanth Menon Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D https://ti.com/opensource
