[PATCH] clk: do not trust cached rates for disabled clocks

[PATCH] clk: do not trust cached rates for disabled clocks

[rs]

From: Randolph Sapp <rs@ti.com>

Recalculate the clock rate for unprepared clocks. This cached value can
vary depending on the clocking architecture. On platforms with clocks
that have shared management it's possible that:

 - Previously disabled clocks have been enabled by other entities
 - Rates calculated during clock tree initialization could have changed

Signed-off-by: Randolph Sapp <rs@ti.com>
---

I'm hoping this will start a bit of a discussion. I'm still curious why people
would want to read the rate of an unprepared clock, but there were so many
logged operations on my test platforms that I assumed it must have some purpose.

Either way, I don't believe cached values should ever be trusted in this
scenario.

 drivers/clk/clk.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 85d2f2481acf..9c8b9036b6f6 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1971,8 +1971,16 @@ static void __clk_recalc_rates(struct clk_core *core, bool update_req,
 
 static unsigned long clk_core_get_rate_recalc(struct clk_core *core)
 {
-	if (core && (core->flags & CLK_GET_RATE_NOCACHE))
-		__clk_recalc_rates(core, false, 0);
+	if (core) {
+		bool prepared = clk_core_is_prepared(core);
+
+		if (core->flags & CLK_GET_RATE_NOCACHE || !prepared) {
+			if (!prepared)
+				pr_debug("%s: rate requested for unprepared clock %s\n",
+					 __func__, core->name);
+			__clk_recalc_rates(core, false, 0);
+		}
+	}
 
 	return clk_core_get_rate_nolock(core);
 }
-- 
2.51.0

Re: [PATCH] clk: do not trust cached rates for disabled clocks

[Randolph Sapp]

On Fri Oct 3, 2025 at 5:29 PM CDT, rs wrote:
> From: Randolph Sapp <rs@ti.com>
>
> Recalculate the clock rate for unprepared clocks. This cached value can
> vary depending on the clocking architecture. On platforms with clocks
> that have shared management it's possible that:
>
>  - Previously disabled clocks have been enabled by other entities
>  - Rates calculated during clock tree initialization could have changed
>
> Signed-off-by: Randolph Sapp <rs@ti.com>
> ---
>
> I'm hoping this will start a bit of a discussion. I'm still curious why people
> would want to read the rate of an unprepared clock, but there were so many
> logged operations on my test platforms that I assumed it must have some purpose.
>
> Either way, I don't believe cached values should ever be trusted in this
> scenario.

Ignoring the typo in the subject (s/disabled/unprepared/), was there any
feedback regarding this patch?

>  drivers/clk/clk.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 85d2f2481acf..9c8b9036b6f6 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1971,8 +1971,16 @@ static void __clk_recalc_rates(struct clk_core *core, bool update_req,
>  
>  static unsigned long clk_core_get_rate_recalc(struct clk_core *core)
>  {
> -	if (core && (core->flags & CLK_GET_RATE_NOCACHE))
> -		__clk_recalc_rates(core, false, 0);
> +	if (core) {
> +		bool prepared = clk_core_is_prepared(core);
> +
> +		if (core->flags & CLK_GET_RATE_NOCACHE || !prepared) {
> +			if (!prepared)
> +				pr_debug("%s: rate requested for unprepared clock %s\n",
> +					 __func__, core->name);
> +			__clk_recalc_rates(core, false, 0);
> +		}
> +	}
>  
>  	return clk_core_get_rate_nolock(core);
>  }

Re: [PATCH] clk: do not trust cached rates for disabled clocks

[Michael Walle]

[-- Attachment #1: Type: text/plain, Size: 2179 bytes --]

Hi,

On Sat Oct 4, 2025 at 12:29 AM CEST, rs wrote:
> From: Randolph Sapp <rs@ti.com>
>
> Recalculate the clock rate for unprepared clocks. This cached value can
> vary depending on the clocking architecture. On platforms with clocks
> that have shared management it's possible that:
>
>  - Previously disabled clocks have been enabled by other entities
>  - Rates calculated during clock tree initialization could have changed
>
> Signed-off-by: Randolph Sapp <rs@ti.com>
> ---
>
> I'm hoping this will start a bit of a discussion. I'm still curious why people
> would want to read the rate of an unprepared clock, but there were so many
> logged operations on my test platforms that I assumed it must have some purpose.

> Either way, I don't believe cached values should ever be trusted in this
> scenario.
>
>  drivers/clk/clk.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 85d2f2481acf..9c8b9036b6f6 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1971,8 +1971,16 @@ static void __clk_recalc_rates(struct clk_core *core, bool update_req,
>  
>  static unsigned long clk_core_get_rate_recalc(struct clk_core *core)
>  {
> -	if (core && (core->flags & CLK_GET_RATE_NOCACHE))
> -		__clk_recalc_rates(core, false, 0);
> +	if (core) {
> +		bool prepared = clk_core_is_prepared(core);
> +
> +		if (core->flags & CLK_GET_RATE_NOCACHE || !prepared) {
> +			if (!prepared)
> +				pr_debug("%s: rate requested for unprepared clock %s\n",
> +					 __func__, core->name);
> +			__clk_recalc_rates(core, false, 0);
> +		}
> +	}

I'm not sure this patch is correct. In case the clock is not
prepared, the rate is still cached in __clk_recalc_rates(). Thus,
I'd expect the following sequence to return a wrong rate:

  # assuming clock is unprepared and will return 0
  clk_get_rate()       // this will fetch the (wrong) rate and cache it
  clk_prepare_enable()
  clk_get_rate()       // this will then return the cached rate

Or do I miss something here?

-michael

>  
>  	return clk_core_get_rate_nolock(core);
>  }


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]

Re: [PATCH] clk: do not trust cached rates for disabled clocks

[Randolph Sapp]

On Thu Oct 16, 2025 at 6:23 AM CDT, Michael Walle wrote:
> Hi,
>
> On Sat Oct 4, 2025 at 12:29 AM CEST, rs wrote:
>> From: Randolph Sapp <rs@ti.com>
>>
>> Recalculate the clock rate for unprepared clocks. This cached value can
>> vary depending on the clocking architecture. On platforms with clocks
>> that have shared management it's possible that:
>>
>>  - Previously disabled clocks have been enabled by other entities
>>  - Rates calculated during clock tree initialization could have changed
>>
>> Signed-off-by: Randolph Sapp <rs@ti.com>
>> ---
>>
>> I'm hoping this will start a bit of a discussion. I'm still curious why people
>> would want to read the rate of an unprepared clock, but there were so many
>> logged operations on my test platforms that I assumed it must have some purpose.
>
>> Either way, I don't believe cached values should ever be trusted in this
>> scenario.
>>
>>  drivers/clk/clk.c | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index 85d2f2481acf..9c8b9036b6f6 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -1971,8 +1971,16 @@ static void __clk_recalc_rates(struct clk_core *core, bool update_req,
>>  
>>  static unsigned long clk_core_get_rate_recalc(struct clk_core *core)
>>  {
>> -	if (core && (core->flags & CLK_GET_RATE_NOCACHE))
>> -		__clk_recalc_rates(core, false, 0);
>> +	if (core) {
>> +		bool prepared = clk_core_is_prepared(core);
>> +
>> +		if (core->flags & CLK_GET_RATE_NOCACHE || !prepared) {
>> +			if (!prepared)
>> +				pr_debug("%s: rate requested for unprepared clock %s\n",
>> +					 __func__, core->name);
>> +			__clk_recalc_rates(core, false, 0);
>> +		}
>> +	}
>
> I'm not sure this patch is correct. In case the clock is not
> prepared, the rate is still cached in __clk_recalc_rates(). Thus,
> I'd expect the following sequence to return a wrong rate:
>
>   # assuming clock is unprepared and will return 0
>   clk_get_rate()       // this will fetch the (wrong) rate and cache it
>   clk_prepare_enable()
>   clk_get_rate()       // this will then return the cached rate
>
> Or do I miss something here?
>
> -michael

Ah, that's a really good point. I guess since my test is only looking at
instances where people are doing a clk_get_rate (occurring outside of driver),
clk_prepare_enable, and clk_set_rate it works right now. Need to adjust that.

Am I correct in my assumption about running clk_get_rate on unprepared clocks
though? (That it shouldn't be allowed or, if it is, that the result shouldn't be
cached.)

- Randolph

>>  
>>  	return clk_core_get_rate_nolock(core);
>>  }

Re: [PATCH] clk: do not trust cached rates for disabled clocks

[Randolph Sapp]

On Fri Oct 17, 2025 at 1:09 PM CDT, Randolph Sapp wrote:
> On Thu Oct 16, 2025 at 6:23 AM CDT, Michael Walle wrote:
>> Hi,
>>
>> On Sat Oct 4, 2025 at 12:29 AM CEST, rs wrote:
>>> From: Randolph Sapp <rs@ti.com>
>>>
>>> Recalculate the clock rate for unprepared clocks. This cached value can
>>> vary depending on the clocking architecture. On platforms with clocks
>>> that have shared management it's possible that:
>>>
>>>  - Previously disabled clocks have been enabled by other entities
>>>  - Rates calculated during clock tree initialization could have changed
>>>
>>> Signed-off-by: Randolph Sapp <rs@ti.com>
>>> ---
>>>
>>> I'm hoping this will start a bit of a discussion. I'm still curious why people
>>> would want to read the rate of an unprepared clock, but there were so many
>>> logged operations on my test platforms that I assumed it must have some purpose.
>>
>>> Either way, I don't believe cached values should ever be trusted in this
>>> scenario.
>>>
>>>  drivers/clk/clk.c | 12 ++++++++++--
>>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>>> index 85d2f2481acf..9c8b9036b6f6 100644
>>> --- a/drivers/clk/clk.c
>>> +++ b/drivers/clk/clk.c
>>> @@ -1971,8 +1971,16 @@ static void __clk_recalc_rates(struct clk_core *core, bool update_req,
>>>  
>>>  static unsigned long clk_core_get_rate_recalc(struct clk_core *core)
>>>  {
>>> -	if (core && (core->flags & CLK_GET_RATE_NOCACHE))
>>> -		__clk_recalc_rates(core, false, 0);
>>> +	if (core) {
>>> +		bool prepared = clk_core_is_prepared(core);
>>> +
>>> +		if (core->flags & CLK_GET_RATE_NOCACHE || !prepared) {
>>> +			if (!prepared)
>>> +				pr_debug("%s: rate requested for unprepared clock %s\n",
>>> +					 __func__, core->name);
>>> +			__clk_recalc_rates(core, false, 0);
>>> +		}
>>> +	}
>>
>> I'm not sure this patch is correct. In case the clock is not
>> prepared, the rate is still cached in __clk_recalc_rates(). Thus,
>> I'd expect the following sequence to return a wrong rate:
>>
>>   # assuming clock is unprepared and will return 0
>>   clk_get_rate()       // this will fetch the (wrong) rate and cache it
>>   clk_prepare_enable()
>>   clk_get_rate()       // this will then return the cached rate
>>
>> Or do I miss something here?
>>
>> -michael
>
> Ah, that's a really good point. I guess since my test is only looking at
> instances where people are doing a clk_get_rate (occurring outside of driver),
> clk_prepare_enable, and clk_set_rate it works right now. Need to adjust that.
>
> Am I correct in my assumption about running clk_get_rate on unprepared clocks
> though? (That it shouldn't be allowed or, if it is, that the result shouldn't be
> cached.)
>
> - Randolph

Any follow up to this Michael? I wanted to be sure this was something the
subsystem should allow before I look into further workarounds.

- Randolph

>>>  
>>>  	return clk_core_get_rate_nolock(core);
>>>  }

Re: [PATCH] clk: do not trust cached rates for disabled clocks

[Michael Walle]

Hi,

>> Am I correct in my assumption about running clk_get_rate on unprepared clocks
>> though? (That it shouldn't be allowed or, if it is, that the result shouldn't be
>> cached.)
>>
> Any follow up to this Michael? I wanted to be sure this was something the
> subsystem should allow before I look into further workarounds.

I don't know. I'm not that familar with the ccs. My first reaction
was that it's asymmetrical that a .set is allowed (and works for
tisci) and that .get is not allowed. That way you can't read the
hardware clock (or think of a fixed clock, where you want to get the
frequency) before enabling it. I could imagine some devices which
needs to be configured first, before you might turn the clock on.

OTOH Maxime pointed out the comment in the kdoc of clk_get_rate()
which clearly states that it's only valid if the clock is on.

-michael

Re: [PATCH] clk: do not trust cached rates for disabled clocks

[Randolph Sapp]

On Wed Oct 22, 2025 at 1:23 AM CDT, Michael Walle wrote:
> Hi,
>
>>> Am I correct in my assumption about running clk_get_rate on unprepared clocks
>>> though? (That it shouldn't be allowed or, if it is, that the result shouldn't be
>>> cached.)
>>>
>> Any follow up to this Michael? I wanted to be sure this was something the
>> subsystem should allow before I look into further workarounds.
>
> I don't know. I'm not that familar with the ccs. My first reaction
> was that it's asymmetrical that a .set is allowed (and works for
> tisci) and that .get is not allowed. That way you can't read the
> hardware clock (or think of a fixed clock, where you want to get the
> frequency) before enabling it. I could imagine some devices which
> needs to be configured first, before you might turn the clock on.
>
> OTOH Maxime pointed out the comment in the kdoc of clk_get_rate()
> which clearly states that it's only valid if the clock is on.
>
> -michael

Yeah, I still don't like the way we handle clock in firmware but I've already
been shut down on that front.

Regardless, there are quite a few drivers right now that use clk_get_rate prior
to preparing the clock. If the kdoc reports clk_get_rate is only valid if the
clock is enabled, should we report a proper warning when this occurs?

- Randolph

Re: [PATCH] clk: do not trust cached rates for disabled clocks

[Michael Walle]

[-- Attachment #1: Type: text/plain, Size: 1500 bytes --]

On Thu Oct 23, 2025 at 1:18 AM CEST, Randolph Sapp wrote:
> On Wed Oct 22, 2025 at 1:23 AM CDT, Michael Walle wrote:
>> Hi,
>>
>>>> Am I correct in my assumption about running clk_get_rate on unprepared clocks
>>>> though? (That it shouldn't be allowed or, if it is, that the result shouldn't be
>>>> cached.)
>>>>
>>> Any follow up to this Michael? I wanted to be sure this was something the
>>> subsystem should allow before I look into further workarounds.
>>
>> I don't know. I'm not that familar with the ccs. My first reaction
>> was that it's asymmetrical that a .set is allowed (and works for
>> tisci) and that .get is not allowed. That way you can't read the
>> hardware clock (or think of a fixed clock, where you want to get the
>> frequency) before enabling it. I could imagine some devices which
>> needs to be configured first, before you might turn the clock on.
>>
>> OTOH Maxime pointed out the comment in the kdoc of clk_get_rate()
>> which clearly states that it's only valid if the clock is on.
>>
>> -michael
>
> Yeah, I still don't like the way we handle clock in firmware but I've already
> been shut down on that front.

TBC, I was talking about the CCS set/get API in general.

-michael

> Regardless, there are quite a few drivers right now that use clk_get_rate prior
> to preparing the clock. If the kdoc reports clk_get_rate is only valid if the
> clock is enabled, should we report a proper warning when this occurs?
>
> - Randolph


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]

Re: [PATCH] clk: do not trust cached rates for disabled clocks

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2245 bytes --]

On Wed, Oct 22, 2025 at 06:18:22PM -0500, Randolph Sapp wrote:
> On Wed Oct 22, 2025 at 1:23 AM CDT, Michael Walle wrote:
> > Hi,
> >
> >>> Am I correct in my assumption about running clk_get_rate on unprepared clocks
> >>> though? (That it shouldn't be allowed or, if it is, that the result shouldn't be
> >>> cached.)
> >>>
> >> Any follow up to this Michael? I wanted to be sure this was something the
> >> subsystem should allow before I look into further workarounds.
> >
> > I don't know. I'm not that familar with the ccs. My first reaction
> > was that it's asymmetrical that a .set is allowed (and works for
> > tisci) and that .get is not allowed. That way you can't read the
> > hardware clock (or think of a fixed clock, where you want to get the
> > frequency) before enabling it. I could imagine some devices which
> > needs to be configured first, before you might turn the clock on.
> >
> > OTOH Maxime pointed out the comment in the kdoc of clk_get_rate()
> > which clearly states that it's only valid if the clock is on.
> >
> > -michael
> 
> Yeah, I still don't like the way we handle clock in firmware but I've already
> been shut down on that front.
> 
> Regardless, there are quite a few drivers right now that use clk_get_rate prior
> to preparing the clock. If the kdoc reports clk_get_rate is only valid if the
> clock is enabled, should we report a proper warning when this occurs?

It's more complicated than that.

The clock API documentation mentions that, and the CCF is one
implementation of that API. It's now the dominant implementation, but
the CCF itself never mentioned or required it.

And thus drivers started to rely on the CCF behaviour.

Plus, leaving the documentation part aside, being able to call
clk_get_rate when the clock is disabled has value.

How can you setup a clock before enabling it to avoid any frequency
change while the device operates otherwise?

Or how do you make sure the clock is in its operating range and thus the
device *can* operate?

There's a reason people have started using it. And it might be
abstracted away by the firmware in some cases, but not all platforms use
a firmware, so you can't rely on that either.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH] clk: do not trust cached rates for disabled clocks

[Randolph Sapp]

On Thu Oct 23, 2025 at 3:36 AM CDT, Maxime Ripard wrote:
> On Wed, Oct 22, 2025 at 06:18:22PM -0500, Randolph Sapp wrote:
>> On Wed Oct 22, 2025 at 1:23 AM CDT, Michael Walle wrote:
>> > Hi,
>> >
>> >>> Am I correct in my assumption about running clk_get_rate on unprepared clocks
>> >>> though? (That it shouldn't be allowed or, if it is, that the result shouldn't be
>> >>> cached.)
>> >>>
>> >> Any follow up to this Michael? I wanted to be sure this was something the
>> >> subsystem should allow before I look into further workarounds.
>> >
>> > I don't know. I'm not that familar with the ccs. My first reaction
>> > was that it's asymmetrical that a .set is allowed (and works for
>> > tisci) and that .get is not allowed. That way you can't read the
>> > hardware clock (or think of a fixed clock, where you want to get the
>> > frequency) before enabling it. I could imagine some devices which
>> > needs to be configured first, before you might turn the clock on.
>> >
>> > OTOH Maxime pointed out the comment in the kdoc of clk_get_rate()
>> > which clearly states that it's only valid if the clock is on.
>> >
>> > -michael
>> 
>> Yeah, I still don't like the way we handle clock in firmware but I've already
>> been shut down on that front.
>> 
>> Regardless, there are quite a few drivers right now that use clk_get_rate prior
>> to preparing the clock. If the kdoc reports clk_get_rate is only valid if the
>> clock is enabled, should we report a proper warning when this occurs?
>
> It's more complicated than that.
>
> The clock API documentation mentions that, and the CCF is one
> implementation of that API. It's now the dominant implementation, but
> the CCF itself never mentioned or required it.
>
> And thus drivers started to rely on the CCF behaviour.
>
> Plus, leaving the documentation part aside, being able to call
> clk_get_rate when the clock is disabled has value.
>
> How can you setup a clock before enabling it to avoid any frequency
> change while the device operates otherwise?

Why would enabling a clock change it's rate unless the current rate wasn't
actually valid? I can only see a change explicitly occurring if the clock parent
has decided that the associated rate was not acceptable.

If some device always resets the rate when enabled, isn't that already
problematic?

> Or how do you make sure the clock is in its operating range and thus the
> device *can* operate?

Well, if enabling a clock doesn't change it's rate there's nothing stopping
people from enabling the clock prior to getting the rate.

> There's a reason people have started using it. And it might be
> abstracted away by the firmware in some cases, but not all platforms use
> a firmware, so you can't rely on that either.
>
> Maxime

Thanks for taking the time to talk with me about this. I assume there is some
specific thing that violates my understanding of how this should work, but I
feel like things are too loosely defined as is.

- Randolph

Re: [PATCH] clk: do not trust cached rates for disabled clocks

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3962 bytes --]

On Thu, Oct 23, 2025 at 05:55:45PM -0500, Randolph Sapp wrote:
> On Thu Oct 23, 2025 at 3:36 AM CDT, Maxime Ripard wrote:
> > On Wed, Oct 22, 2025 at 06:18:22PM -0500, Randolph Sapp wrote:
> >> On Wed Oct 22, 2025 at 1:23 AM CDT, Michael Walle wrote:
> >> > Hi,
> >> >
> >> >>> Am I correct in my assumption about running clk_get_rate on unprepared clocks
> >> >>> though? (That it shouldn't be allowed or, if it is, that the result shouldn't be
> >> >>> cached.)
> >> >>>
> >> >> Any follow up to this Michael? I wanted to be sure this was something the
> >> >> subsystem should allow before I look into further workarounds.
> >> >
> >> > I don't know. I'm not that familar with the ccs. My first reaction
> >> > was that it's asymmetrical that a .set is allowed (and works for
> >> > tisci) and that .get is not allowed. That way you can't read the
> >> > hardware clock (or think of a fixed clock, where you want to get the
> >> > frequency) before enabling it. I could imagine some devices which
> >> > needs to be configured first, before you might turn the clock on.
> >> >
> >> > OTOH Maxime pointed out the comment in the kdoc of clk_get_rate()
> >> > which clearly states that it's only valid if the clock is on.
> >> >
> >> > -michael
> >> 
> >> Yeah, I still don't like the way we handle clock in firmware but I've already
> >> been shut down on that front.
> >> 
> >> Regardless, there are quite a few drivers right now that use clk_get_rate prior
> >> to preparing the clock. If the kdoc reports clk_get_rate is only valid if the
> >> clock is enabled, should we report a proper warning when this occurs?
> >
> > It's more complicated than that.
> >
> > The clock API documentation mentions that, and the CCF is one
> > implementation of that API. It's now the dominant implementation, but
> > the CCF itself never mentioned or required it.
> >
> > And thus drivers started to rely on the CCF behaviour.
> >
> > Plus, leaving the documentation part aside, being able to call
> > clk_get_rate when the clock is disabled has value.
> >
> > How can you setup a clock before enabling it to avoid any frequency
> > change while the device operates otherwise?
> 
> Why would enabling a clock change it's rate unless the current rate wasn't
> actually valid?

That's not what I said, and it's also not something that enable is
allowed to do anyway.

Consider this: the clock feeding a hardware component is out of its
operating range. You enable it. The device gets stuck. How do you
recover from that?

> I can only see a change explicitly occurring if the clock parent has
> decided that the associated rate was not acceptable.
> 
> If some device always resets the rate when enabled, isn't that already
> problematic?

Resets the rate to what?

> > Or how do you make sure the clock is in its operating range and thus the
> > device *can* operate?
> 
> Well, if enabling a clock doesn't change it's rate there's nothing stopping
> people from enabling the clock prior to getting the rate.

The first thing clk_set_rate is doing is a clk_get_rate. If you want to
actually enforce a rule that hasn't been applied for 15y, go ahead, but
that means also mandating that you can only make clk_set_rate calls once
the clock has been enabled.

> > There's a reason people have started using it. And it might be
> > abstracted away by the firmware in some cases, but not all platforms use
> > a firmware, so you can't rely on that either.
>
> Thanks for taking the time to talk with me about this. I assume there is some
> specific thing that violates my understanding of how this should work, but I
> feel like things are too loosely defined as is.

I mean, I kind of agree, but also, the clock framework is newer and has
been more liberal than the clock API. And the clock framework is by far
the dominant implementation now, so I'm not sure what the big deal is.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH] clk: do not trust cached rates for disabled clocks

[Randolph Sapp]

On Fri Oct 24, 2025 at 6:23 AM CDT, Maxime Ripard wrote:
> On Thu, Oct 23, 2025 at 05:55:45PM -0500, Randolph Sapp wrote:
>> On Thu Oct 23, 2025 at 3:36 AM CDT, Maxime Ripard wrote:
>> > On Wed, Oct 22, 2025 at 06:18:22PM -0500, Randolph Sapp wrote:
>> >> On Wed Oct 22, 2025 at 1:23 AM CDT, Michael Walle wrote:
>> >> > Hi,
>> >> >
>> >> >>> Am I correct in my assumption about running clk_get_rate on unprepared clocks
>> >> >>> though? (That it shouldn't be allowed or, if it is, that the result shouldn't be
>> >> >>> cached.)
>> >> >>>
>> >> >> Any follow up to this Michael? I wanted to be sure this was something the
>> >> >> subsystem should allow before I look into further workarounds.
>> >> >
>> >> > I don't know. I'm not that familar with the ccs. My first reaction
>> >> > was that it's asymmetrical that a .set is allowed (and works for
>> >> > tisci) and that .get is not allowed. That way you can't read the
>> >> > hardware clock (or think of a fixed clock, where you want to get the
>> >> > frequency) before enabling it. I could imagine some devices which
>> >> > needs to be configured first, before you might turn the clock on.
>> >> >
>> >> > OTOH Maxime pointed out the comment in the kdoc of clk_get_rate()
>> >> > which clearly states that it's only valid if the clock is on.
>> >> >
>> >> > -michael
>> >> 
>> >> Yeah, I still don't like the way we handle clock in firmware but I've already
>> >> been shut down on that front.
>> >> 
>> >> Regardless, there are quite a few drivers right now that use clk_get_rate prior
>> >> to preparing the clock. If the kdoc reports clk_get_rate is only valid if the
>> >> clock is enabled, should we report a proper warning when this occurs?
>> >
>> > It's more complicated than that.
>> >
>> > The clock API documentation mentions that, and the CCF is one
>> > implementation of that API. It's now the dominant implementation, but
>> > the CCF itself never mentioned or required it.
>> >
>> > And thus drivers started to rely on the CCF behaviour.
>> >
>> > Plus, leaving the documentation part aside, being able to call
>> > clk_get_rate when the clock is disabled has value.
>> >
>> > How can you setup a clock before enabling it to avoid any frequency
>> > change while the device operates otherwise?
>> 
>> Why would enabling a clock change it's rate unless the current rate wasn't
>> actually valid?
>
> That's not what I said, and it's also not something that enable is
> allowed to do anyway.
>
> Consider this: the clock feeding a hardware component is out of its
> operating range. You enable it. The device gets stuck. How do you
> recover from that?
>
>> I can only see a change explicitly occurring if the clock parent has
>> decided that the associated rate was not acceptable.
>> 
>> If some device always resets the rate when enabled, isn't that already
>> problematic?
>
> Resets the rate to what?

Doesn't matter. The initial comment was about the rate changing when the clock
was enabled. I said reset here because any change on power on would assume some
default state it was returning to.

>> > Or how do you make sure the clock is in its operating range and thus the
>> > device *can* operate?
>> 
>> Well, if enabling a clock doesn't change it's rate there's nothing stopping
>> people from enabling the clock prior to getting the rate.
>
> The first thing clk_set_rate is doing is a clk_get_rate. If you want to
> actually enforce a rule that hasn't been applied for 15y, go ahead, but
> that means also mandating that you can only make clk_set_rate calls once
> the clock has been enabled.

The clk_set_rate only runs clk_get_rate to see if any change needs to occur. If
the clock isn't enabled then this is likely part of startup or resume. The
likelihood of needing to change the rate in this scenario would be high anyway.

>> > There's a reason people have started using it. And it might be
>> > abstracted away by the firmware in some cases, but not all platforms use
>> > a firmware, so you can't rely on that either.
>>
>> Thanks for taking the time to talk with me about this. I assume there is some
>> specific thing that violates my understanding of how this should work, but I
>> feel like things are too loosely defined as is.
>
> I mean, I kind of agree, but also, the clock framework is newer and has
> been more liberal than the clock API. And the clock framework is by far
> the dominant implementation now, so I'm not sure what the big deal is.
>
> Maxime

There's no big deal, I was curious if there was a consensus around clock
initialization that could push this fix in a different direction.

Considering there doesn't seem to be any consensus I'm fine with moving forward
with the initial cached rate workaround. I just need to rework it a bit to cover
the initial corner case outlined.

- Randolph

Re: [PATCH] clk: do not trust cached rates for disabled clocks

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 5800 bytes --]

On Mon, Oct 27, 2025 at 06:44:37PM -0500, Randolph Sapp wrote:
> On Fri Oct 24, 2025 at 6:23 AM CDT, Maxime Ripard wrote:
> > On Thu, Oct 23, 2025 at 05:55:45PM -0500, Randolph Sapp wrote:
> >> On Thu Oct 23, 2025 at 3:36 AM CDT, Maxime Ripard wrote:
> >> > On Wed, Oct 22, 2025 at 06:18:22PM -0500, Randolph Sapp wrote:
> >> >> On Wed Oct 22, 2025 at 1:23 AM CDT, Michael Walle wrote:
> >> >> > Hi,
> >> >> >
> >> >> >>> Am I correct in my assumption about running clk_get_rate on unprepared clocks
> >> >> >>> though? (That it shouldn't be allowed or, if it is, that the result shouldn't be
> >> >> >>> cached.)
> >> >> >>>
> >> >> >> Any follow up to this Michael? I wanted to be sure this was something the
> >> >> >> subsystem should allow before I look into further workarounds.
> >> >> >
> >> >> > I don't know. I'm not that familar with the ccs. My first reaction
> >> >> > was that it's asymmetrical that a .set is allowed (and works for
> >> >> > tisci) and that .get is not allowed. That way you can't read the
> >> >> > hardware clock (or think of a fixed clock, where you want to get the
> >> >> > frequency) before enabling it. I could imagine some devices which
> >> >> > needs to be configured first, before you might turn the clock on.
> >> >> >
> >> >> > OTOH Maxime pointed out the comment in the kdoc of clk_get_rate()
> >> >> > which clearly states that it's only valid if the clock is on.
> >> >> >
> >> >> > -michael
> >> >> 
> >> >> Yeah, I still don't like the way we handle clock in firmware but I've already
> >> >> been shut down on that front.
> >> >> 
> >> >> Regardless, there are quite a few drivers right now that use clk_get_rate prior
> >> >> to preparing the clock. If the kdoc reports clk_get_rate is only valid if the
> >> >> clock is enabled, should we report a proper warning when this occurs?
> >> >
> >> > It's more complicated than that.
> >> >
> >> > The clock API documentation mentions that, and the CCF is one
> >> > implementation of that API. It's now the dominant implementation, but
> >> > the CCF itself never mentioned or required it.
> >> >
> >> > And thus drivers started to rely on the CCF behaviour.
> >> >
> >> > Plus, leaving the documentation part aside, being able to call
> >> > clk_get_rate when the clock is disabled has value.
> >> >
> >> > How can you setup a clock before enabling it to avoid any frequency
> >> > change while the device operates otherwise?
> >> 
> >> Why would enabling a clock change it's rate unless the current rate wasn't
> >> actually valid?
> >
> > That's not what I said, and it's also not something that enable is
> > allowed to do anyway.
> >
> > Consider this: the clock feeding a hardware component is out of its
> > operating range. You enable it. The device gets stuck. How do you
> > recover from that?
> >
> >> I can only see a change explicitly occurring if the clock parent has
> >> decided that the associated rate was not acceptable.
> >> 
> >> If some device always resets the rate when enabled, isn't that already
> >> problematic?
> >
> > Resets the rate to what?
> 
> Doesn't matter. The initial comment was about the rate changing when the clock
> was enabled. I said reset here because any change on power on would assume some
> default state it was returning to.
> 
> >> > Or how do you make sure the clock is in its operating range and thus the
> >> > device *can* operate?
> >> 
> >> Well, if enabling a clock doesn't change it's rate there's nothing stopping
> >> people from enabling the clock prior to getting the rate.
> >
> > The first thing clk_set_rate is doing is a clk_get_rate. If you want to
> > actually enforce a rule that hasn't been applied for 15y, go ahead, but
> > that means also mandating that you can only make clk_set_rate calls once
> > the clock has been enabled.
> 
> The clk_set_rate only runs clk_get_rate to see if any change needs to occur. If
> the clock isn't enabled then this is likely part of startup or resume. The
> likelihood of needing to change the rate in this scenario would be high anyway.
> 
> >> > There's a reason people have started using it. And it might be
> >> > abstracted away by the firmware in some cases, but not all platforms use
> >> > a firmware, so you can't rely on that either.
> >>
> >> Thanks for taking the time to talk with me about this. I assume there is some
> >> specific thing that violates my understanding of how this should work, but I
> >> feel like things are too loosely defined as is.
> >
> > I mean, I kind of agree, but also, the clock framework is newer and has
> > been more liberal than the clock API. And the clock framework is by far
> > the dominant implementation now, so I'm not sure what the big deal is.
>
> There's no big deal, I was curious if there was a consensus around clock
> initialization that could push this fix in a different direction.
> 
> Considering there doesn't seem to be any consensus I'm fine with moving forward
> with the initial cached rate workaround. I just need to rework it a bit to cover
> the initial corner case outlined.

Don't get me wrong, I would like some clarification as well, but I guess
what I was trying to say is that clk_get_rate is such a mess when it
comes to what it's allowed to return or not already than trying to clean
up when it's allowed to be called is going to be a nightmare and you'll
break tons of platforms in the process.

Last time I tried, I ended up in a philosophical discussion with another
platform maintainer that went nowhere, and we ended up reverting the
patches.

So unless you get a maintainer that steps in and say what needs to be
done, and enforces it, my advice is you should workaround the issue.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH] clk: do not trust cached rates for disabled clocks

[Randolph Sapp]

On Wed Oct 29, 2025 at 4:05 AM CDT, Maxime Ripard wrote:
> On Mon, Oct 27, 2025 at 06:44:37PM -0500, Randolph Sapp wrote:
>> On Fri Oct 24, 2025 at 6:23 AM CDT, Maxime Ripard wrote:
>> > On Thu, Oct 23, 2025 at 05:55:45PM -0500, Randolph Sapp wrote:
>> >> On Thu Oct 23, 2025 at 3:36 AM CDT, Maxime Ripard wrote:
>> >> > On Wed, Oct 22, 2025 at 06:18:22PM -0500, Randolph Sapp wrote:
>> >> >> On Wed Oct 22, 2025 at 1:23 AM CDT, Michael Walle wrote:
>> >> >> > Hi,
>> >> >> >
>> >> >> >>> Am I correct in my assumption about running clk_get_rate on unprepared clocks
>> >> >> >>> though? (That it shouldn't be allowed or, if it is, that the result shouldn't be
>> >> >> >>> cached.)
>> >> >> >>>
>> >> >> >> Any follow up to this Michael? I wanted to be sure this was something the
>> >> >> >> subsystem should allow before I look into further workarounds.
>> >> >> >
>> >> >> > I don't know. I'm not that familar with the ccs. My first reaction
>> >> >> > was that it's asymmetrical that a .set is allowed (and works for
>> >> >> > tisci) and that .get is not allowed. That way you can't read the
>> >> >> > hardware clock (or think of a fixed clock, where you want to get the
>> >> >> > frequency) before enabling it. I could imagine some devices which
>> >> >> > needs to be configured first, before you might turn the clock on.
>> >> >> >
>> >> >> > OTOH Maxime pointed out the comment in the kdoc of clk_get_rate()
>> >> >> > which clearly states that it's only valid if the clock is on.
>> >> >> >
>> >> >> > -michael
>> >> >> 
>> >> >> Yeah, I still don't like the way we handle clock in firmware but I've already
>> >> >> been shut down on that front.
>> >> >> 
>> >> >> Regardless, there are quite a few drivers right now that use clk_get_rate prior
>> >> >> to preparing the clock. If the kdoc reports clk_get_rate is only valid if the
>> >> >> clock is enabled, should we report a proper warning when this occurs?
>> >> >
>> >> > It's more complicated than that.
>> >> >
>> >> > The clock API documentation mentions that, and the CCF is one
>> >> > implementation of that API. It's now the dominant implementation, but
>> >> > the CCF itself never mentioned or required it.
>> >> >
>> >> > And thus drivers started to rely on the CCF behaviour.
>> >> >
>> >> > Plus, leaving the documentation part aside, being able to call
>> >> > clk_get_rate when the clock is disabled has value.
>> >> >
>> >> > How can you setup a clock before enabling it to avoid any frequency
>> >> > change while the device operates otherwise?
>> >> 
>> >> Why would enabling a clock change it's rate unless the current rate wasn't
>> >> actually valid?
>> >
>> > That's not what I said, and it's also not something that enable is
>> > allowed to do anyway.
>> >
>> > Consider this: the clock feeding a hardware component is out of its
>> > operating range. You enable it. The device gets stuck. How do you
>> > recover from that?
>> >
>> >> I can only see a change explicitly occurring if the clock parent has
>> >> decided that the associated rate was not acceptable.
>> >> 
>> >> If some device always resets the rate when enabled, isn't that already
>> >> problematic?
>> >
>> > Resets the rate to what?
>> 
>> Doesn't matter. The initial comment was about the rate changing when the clock
>> was enabled. I said reset here because any change on power on would assume some
>> default state it was returning to.
>> 
>> >> > Or how do you make sure the clock is in its operating range and thus the
>> >> > device *can* operate?
>> >> 
>> >> Well, if enabling a clock doesn't change it's rate there's nothing stopping
>> >> people from enabling the clock prior to getting the rate.
>> >
>> > The first thing clk_set_rate is doing is a clk_get_rate. If you want to
>> > actually enforce a rule that hasn't been applied for 15y, go ahead, but
>> > that means also mandating that you can only make clk_set_rate calls once
>> > the clock has been enabled.
>> 
>> The clk_set_rate only runs clk_get_rate to see if any change needs to occur. If
>> the clock isn't enabled then this is likely part of startup or resume. The
>> likelihood of needing to change the rate in this scenario would be high anyway.
>> 
>> >> > There's a reason people have started using it. And it might be
>> >> > abstracted away by the firmware in some cases, but not all platforms use
>> >> > a firmware, so you can't rely on that either.
>> >>
>> >> Thanks for taking the time to talk with me about this. I assume there is some
>> >> specific thing that violates my understanding of how this should work, but I
>> >> feel like things are too loosely defined as is.
>> >
>> > I mean, I kind of agree, but also, the clock framework is newer and has
>> > been more liberal than the clock API. And the clock framework is by far
>> > the dominant implementation now, so I'm not sure what the big deal is.
>>
>> There's no big deal, I was curious if there was a consensus around clock
>> initialization that could push this fix in a different direction.
>> 
>> Considering there doesn't seem to be any consensus I'm fine with moving forward
>> with the initial cached rate workaround. I just need to rework it a bit to cover
>> the initial corner case outlined.
>
> Don't get me wrong, I would like some clarification as well, but I guess
> what I was trying to say is that clk_get_rate is such a mess when it
> comes to what it's allowed to return or not already than trying to clean
> up when it's allowed to be called is going to be a nightmare and you'll
> break tons of platforms in the process.
>
> Last time I tried, I ended up in a philosophical discussion with another
> platform maintainer that went nowhere, and we ended up reverting the
> patches.
>
> So unless you get a maintainer that steps in and say what needs to be
> done, and enforces it, my advice is you should workaround the issue.
>
> Maxime

That was my conclusion as well. If we purposefully aren't enforcing some
*suggestions* of the common clock framework and our clocks don't play by the
current rules then we need to reevaluate.

Kicked all involved parties on our end again. We'll see what the consensus is. I
have a feeling we're going to have to implement some horribly hacky selective
caching solution to make everyone happy. I say horribly hacky because it will
still have requirements about the ordering of particular calls that will need to
be manually enforced in all corresponding drivers.

Well, this will be fun.

- Randolph