Re: [PATCH] clk: Do not recalc rate for reparented clocks

[Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

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

On Tue, Mar 10, 2020 at 11:42:27AM +0100, Thierry Reding wrote:
> On Tue, Mar 10, 2020 at 12:25:01PM +0200, Mikko Perttunen wrote:
> > On 3/10/20 3:48 AM, Stephen Boyd wrote:
> > > Quoting Thierry Reding (2020-03-05 09:51:38)
> > > > From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > > > 
> > > > As part of the clock frequency change sequence, a driver may need to
> > > > reparent a clock. In that case, the rate will already have been updated
> > > > and the cached parent rate will no longer be valid, so just skip the
> > > > recalculation.
> > > 
> > > Can you describe more about what's going on and why this needs to
> > > change? For example, we have 'set_rate_and_parent' for cases where a
> > > driver reparents a clk during a rate change. Is that not sufficient
> > > here?
> > > 
> > 
> > I believe this is related to the memory clock change sequence on Tegra,
> > where the EMC clock may have to be reparented multiple times during the rate
> > change sequence.
> > 
> > If EMC is running off parent A, and the requested new OPP also uses parent
> > A, we have to temporarily switch to a different OPP with parent B so that
> > the rate change sequence can be executed on parent A without affecting the
> > system's memory accesses.
> 
> It's not only that. The mismatch can happen every time that we change
> the operating point because each such change could cause either the
> parent and/or the parent rate to change. The CCF always caches the
> parent rate from before the rate change, assuming that the parent will
> not change.
> 
> Perhaps a clearer way of saying this is that the parent change is an
> internal part of the rate change. clk_hw_reparent() already allows us to
> notify the CCF of the hierarchical change in the clock tree. But then we
> need to additionally either override the parent rate in the driver, like
> we currently do on Tegra124, or have the core ignore the cached parent
> rate if the parent is no longer what it expects.
> 
> I should probably have followed up this patch with a corresponding patch
> to the Tegra124 EMC driver that removes the workaround, like below:
> 
> --- >8 ---
> diff --git a/drivers/clk/tegra/clk-emc.c b/drivers/clk/tegra/clk-emc.c
> index 745f9faa98d8..f29f0a0e48de 100644
> --- a/drivers/clk/tegra/clk-emc.c
> +++ b/drivers/clk/tegra/clk-emc.c
> @@ -92,12 +92,6 @@ static unsigned long emc_recalc_rate(struct clk_hw *hw,
>  
>  	tegra = container_of(hw, struct tegra_clk_emc, hw);
>  
> -	/*
> -	 * CCF wrongly assumes that the parent won't change during set_rate,
> -	 * so get the parent rate explicitly.
> -	 */
> -	parent_rate = clk_hw_get_rate(clk_hw_get_parent(hw));
> -
>  	val = readl(tegra->clk_regs + CLK_SOURCE_EMC);
>  	div = val & CLK_SOURCE_EMC_EMC_2X_CLK_DIVISOR_MASK;
>  
> --- >8 ---
> 
> But I haven't actually tested that yet because I don't have a Tegra124
> board set up at the moment.

I did test the above change along with the core change on a Jetson TK1
board (which is Tegra124) and unfortunately that doesn't work for all
rate changes. I'll need to dig deeper to see what exactly is going on,
and to see if there's a simple fix for that.

If not it might be better to leave these types of workarounds in the
drivers instead.

Thierry

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