Messages in this thread | | | Date | Wed, 2 Aug 2023 08:30:39 +0200 | Subject | Re: [PATCH] clk: imx: composite-8m: avoid glitches when set_rate would be a no-op | From | 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@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); >> >
-- 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 |
| |