Re: [PATCH v4 13/28] clk: Take into account uncached clocks in clk_set_rate_range()

[Maxime Ripard <maxime@cerno.tech>]

Hi,

On Tue, May 24, 2022 at 02:32:29PM -0400, Jim Quinlan wrote:
> On Fri, May 13, 2022 at 3:56 AM Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > clk_set_rate_range() will use the last requested rate for the clock when
> > it calls into the driver set_rate hook.
> >
> > However, if CLK_GET_RATE_NOCACHE is set on that clock, the last
> > requested rate might not be matching the current rate of the clock. In
> > such a case, let's read out the rate from the hardware and use that in
> > our set_rate instead.
>
> I'm probably out of the loop on this but I am wondering why we don't
> also apply this same reasoning to
> 
>         clk_core_set_rate_nolock()
> 
> which may usea stale clock rate and skip the actual setting of the rate:
> 
> static int clk_core_set_rate_nolock(core, req_rate)
> {
>         /* ... */
>         rate = clk_core_req_round_rate_nolock(core, req_rate);
> 
>         /* bail early if nothing to do */
>         if (rate == clk_core_get_rate_nolock(core))    /* [JQ] Does
> not check for CLK_GET_RATE_NOCACHE */
>                 return 0;
> 
> I can see that 9-10 years ago someone submitted a pullreq to fix above
> but I cannot locate the response and it obviously never made it
> upstream.

Yeah, that change would make sense to me.

Stephen was hinting at a different solution here though:
https://lore.kernel.org/linux-clk/20220423044228.2AA7AC385A0@smtp.kernel.org/

> Just thinking out loud, the simpler solution is to probably drop all
> rate caching in the CCF and get the frequency on a clk_get_rate()
> call. It complicates some of the core though when we check to see if
> we need to update clk rates. We could have some middle ground where
> drivers indicate that they want to update their cached rate because
> it's valid now (either from their enable path or from somewhere else).
> This may be nice actually because we could have clk providers call
> this to force a recalc down the tree from where they've updated. I
> think the qcom DisplayPort phy would want this.

Maxime