On Mon Sep 15, 2025 at 9:34 AM CDT, 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]> > --- > 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. > --- > 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 c5894fc9395e..d73858b5ca7a 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);
Thanks for looking into this Michael. I'm still convinced that it's unusual to report 0 for a clock rate when the device is powered down. In most cases it's not actually 0 and is actually just in bypass mode. I was told it's a way to indicate clock status and probably won't be changing any time soon though. Ignore the fact that we also already have a separate way to query clock status. :) This series looks good, but won't quite result in a functional GPU without the following patch: https://lore.kernel.org/all/[email protected]/ I suppose I'll submit that again on it's own. Reviewed-by: Randolph Sapp <[email protected]>
