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 <mwalle@kernel.org>
Reviewed-by: Kevin Hilman <khilman@baylibre.com>
Reviewed-by: Randolph Sapp <rs@ti.com>
---
 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

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 <mwalle@kernel.org>
> Reviewed-by: Kevin Hilman <khilman@baylibre.com>
> Reviewed-by: Randolph Sapp <rs@ti.com>
> ---
>  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 <nm@ti.com>

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?


-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D
https://ti.com/opensource

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 <mwalle@kernel.org>
>> Reviewed-by: Kevin Hilman <khilman@baylibre.com>
>> Reviewed-by: Randolph Sapp <rs@ti.com>
>> ---
>>  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 <nm@ti.com>
>
> 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").

-michael

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 <mwalle@kernel.org>
> >> Reviewed-by: Kevin Hilman <khilman@baylibre.com>
> >> Reviewed-by: Randolph Sapp <rs@ti.com>
> >> ---
> >>  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 <nm@ti.com>
> >
> > 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