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

[Randolph Sapp <rs@ti.com>]

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