lkml.org 
[lkml]   [2023]   [Aug]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] clk: imx: composite-8m: avoid glitches when set_rate would be a no-op
From


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

\
 
 \ /
  Last update: 2023-08-04 04:06    [W:0.041 / U:0.308 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site