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>
---
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);
--
2.39.5On 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 <mwalle@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.
> ---
> 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/20250808232522.1296240-1-rs@ti.com/
I suppose I'll submit that again on it's own.
Reviewed-by: Randolph Sapp <rs@ti.com>
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. Thus, disable the cache
> > altogether.
> >
> > Signed-off-by: Michael Walle <mwalle@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.
> > ---
> > 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.
Yeah. And you can't query the clock rate you've just set if the
clock is off (and if enabled the clock will have the frequency of
the last set_rate). I still think that is a gap in the firmware
interface.
> 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/20250808232522.1296240-1-rs@ti.com/
Ahh, I was puzzled why it was working for me. But then I've noticed
that your patch is for the am62p. I'm working with the am67a and
there the ranges are correct for the GPU.
> I suppose I'll submit that again on it's own.
>
> Reviewed-by: Randolph Sapp <rs@ti.com>
Thanks.
-michael
Michael Walle <mwalle@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?
> Thus, disable the cache altogether.
>
> Signed-off-by: Michael Walle <mwalle@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. 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 <khilman@baylibre.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 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);
> --
> 2.39.5
On Wed, Sep 17, 2025 at 08:24:47AM -0700, Kevin Hilman wrote: > Michael Walle <mwalle@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. It doesn't really help that the CCF doesn't seem to agree on if it should do that in the first place :) In the original clk API definition, you're not supposed to call clk_get_rate() when the clock is disabled. https://elixir.bootlin.com/linux/v6.16.8/source/include/linux/clk.h#L746 However, it's been allowed by the CCF since forever: https://elixir.bootlin.com/linux/v6.16.8/source/drivers/clk/clk.c#L1986 But then, some drivers will return 0 as a valid value, and not an error code (whatever 0Hz for a clock means). It's kind of a mess, and very regression prone, so I don't really expect it to change anytime soon. Maxime
On Wed Sep 17, 2025 at 5:24 PM CEST, Kevin Hilman wrote:
> Michael Walle <mwalle@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().
> > Thus, disable the cache altogether.
> >
> > Signed-off-by: Michael Walle <mwalle@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.
> 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 <khilman@baylibre.com>
Thanks!
-michael
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 <mwalle@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-mwalle@kernel.org/
>
> > > Thus, disable the cache altogether.
> > >
> > > Signed-off-by: Michael Walle <mwalle@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 <khilman@baylibre.com>
>
> Thanks!
>
> -michael
--
Best regards,
Dhruva Gole
Texas Instruments Incorporated
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-mwalle@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 <mwalle@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 <khilman@baylibre.com>
> >
> > Thanks!
> >
> > -michael
© 2016 - 2025 Red Hat, Inc.