Date:   Fri, 4 Aug 2023 10:05:09 +0800
From:   Peng Fan <peng.fan@....nxp.com>
To:     Ahmad Fatoum <a.fatoum@...gutronix.de>,
        Abel Vesa <abelvesa@...nel.org>, Peng Fan <peng.fan@....com>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Shawn Guo <shawnguo@...nel.org>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        Fabio Estevam <festevam@...il.com>,
        NXP Linux Team <linux-imx@....com>
Cc:     linux-clk@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] clk: imx: composite-8m: avoid glitches when set_rate
 would be a no-op



On 8/2/2023 2:30 PM, Ahmad Fatoum wrote:
> 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.

Thanks for detailed explaination, I would expect write this down in 
commit message.

> 
> 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.

clock pause would be better, since the hardware design could avoid real 
glitch.

Thanks,
Peng.

> 
> 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@...gutronix.de>
>>> ---
>>>    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);
>>>    
>>
> 

Powered by blists - more mailing lists