Re: [PATCH] clk: imx: composite-8m: avoid glitches when set_rate would be a no-op

[Ahmad Fatoum]

Hello Peng,

On 02.08.23 03:25, Peng Fan wrote:
> 
> 
> On 8/2/2023 12:27 AM, Ahmad Fatoum wrote:
>> Reconfiguring the clock divider to the exact same value is observed
>> on an i.MX8MN to often cause a short clock pause, probably because
>> the divider restarts counting from the time the register is written.
>>
>> This issue doesn't show up normally, because the clock framework will
>> take care to not call set_rate when the clock rate is the same.
>> However, when we configure an upstream clock (e.g. an audio_pll), the
>> common code will call set_rate with the newly calculated rate on all
>> children. As the new rate is different, we enter set_rate and compute
>> the same divider values, write them back and cause the glitch (e.g.
>> on a SAI's MCLK).
> 
> 
> The CCM root has glitch-free mux. When upstream pll freq change,
> the child set rate will also touch the mux bit, since div and mux
> in one register, so the mux logic will also function.
> 
> Per design, it is glitch free, so I not understand well why glitch.
> 
> When you configure pll, the downstream sai clk should still not be enabled, right?

  - sai5 is running normally and divides Audio PLL out by 16.
  - audio_pll1 is increased by 32 Hz -> only kdiv changes, so no glitch
  - imx8m_clk_composite_divider_set_rate(sai5) is called with
    32 / 16 = 2 Hz more
  - imx8m_clk_composite_divider_set_rate computes same divider as before
    and writes register
  - divider starts counting from zero, so we have a longer clock pause
    than usual, e.g. 40ns -> 125ns, external MCLK consumer doesn't like that at all.

So it's not a glitch in the transient high frequency sense, but rather a transient
low frequency period. I can reword the commit message to s/glitch/clock pause/
if you prefer.

And yes, if mux is switched, we will probably get the same clock pause, but
that is not a problem for me currently, because we don't switch parents except
at boot up. Afterwards, only PLL is tuned.

Cheers,
Ahmad

> 
> Thanks,
> Peng.
> 
>>
>> To avoid the glitch, we skip writing the same value back again.
>>
>> Fixes: d3ff9728134e ("clk: imx: Add imx composite clock")
>> Signed-off-by: Ahmad Fatoum <a.fatoum@xxxxxxxxxxxxxx>
>> ---
>>   drivers/clk/imx/clk-composite-8m.c | 12 +++++++-----
>>   1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c
>> index cbf0d7955a00..3e9a092e136c 100644
>> --- a/drivers/clk/imx/clk-composite-8m.c
>> +++ b/drivers/clk/imx/clk-composite-8m.c
>> @@ -97,7 +97,7 @@ static int imx8m_clk_composite_divider_set_rate(struct clk_hw *hw,
>>       int prediv_value;
>>       int div_value;
>>       int ret;
>> -    u32 val;
>> +    u32 orig, val;
>>         ret = imx8m_clk_composite_compute_dividers(rate, parent_rate,
>>                           &prediv_value, &div_value);
>> @@ -106,13 +106,15 @@ static int imx8m_clk_composite_divider_set_rate(struct clk_hw *hw,
>>         spin_lock_irqsave(divider->lock, flags);
>>   -    val = readl(divider->reg);
>> -    val &= ~((clk_div_mask(divider->width) << divider->shift) |
>> -            (clk_div_mask(PCG_DIV_WIDTH) << PCG_DIV_SHIFT));
>> +    orig = readl(divider->reg);
>> +    val = orig & ~((clk_div_mask(divider->width) << divider->shift) |
>> +               (clk_div_mask(PCG_DIV_WIDTH) << PCG_DIV_SHIFT));
>>         val |= (u32)(prediv_value  - 1) << divider->shift;
>>       val |= (u32)(div_value - 1) << PCG_DIV_SHIFT;
>> -    writel(val, divider->reg);
>> +
>> +    if (val != orig)
>> +        writel(val, divider->reg);
>>         spin_unlock_irqrestore(divider->lock, flags);
>>   
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |