Messages in this thread | | | Date | Fri, 4 Aug 2023 10:05:09 +0800 | Subject | Re: [PATCH] clk: imx: composite-8m: avoid glitches when set_rate would be a no-op | From | Peng Fan <> |
| |
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); >>> >> >
| |