clk: imx: pll14xx: avoid glitch when set rate

Commit Message

Peng Fan Aug. 20, 2019, 2:17 a.m. UTC

From: Peng Fan <[e-mail]>

According to PLL1443XA and PLL1416X spec,
"When BYPASS is 0 and RESETB is changed from 0 to 1, FOUT starts to
output unstable clock until lock time passes. PLL1416X/PLL1443XA may
generate a glitch at FOUT."

So set BYPASS when RESETB is changed from 0 to 1 to avoid glitch.
In the end of set rate, BYPASS will be cleared.

Fixes: 8646d4dcc7fb ("clk: imx: Add PLLs driver for imx8mm soc")
Signed-off-by: Peng Fan <[e-mail]>
---
 drivers/clk/imx/clk-pll14xx.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Leonard Crestez Aug. 22, 2019, 8:20 a.m. UTC | #1

On 20.08.2019 05:17, Peng Fan wrote:
> According to PLL1443XA and PLL1416X spec,
> "When BYPASS is 0 and RESETB is changed from 0 to 1, FOUT starts to
> output unstable clock until lock time passes. PLL1416X/PLL1443XA may
> generate a glitch at FOUT."
> 
> So set BYPASS when RESETB is changed from 0 to 1 to avoid glitch.
> In the end of set rate, BYPASS will be cleared.
> 
> @@ -191,6 +191,10 @@ static int clk_pll1416x_set_rate(struct clk_hw *hw, unsigned long drate,
>   	tmp &= ~RST_MASK;
>   	writel_relaxed(tmp, pll->base);
>   
> +	/* Enable BYPASS */
> +	tmp |= BYPASS_MASK;
> +	writel(tmp, pll->base);
> +

Shouldn't BYPASS be set before reset?

Also, isn't a similar bypass/unbypass dance also needed in 
clk_pll14xx_prepare? As far as I understand that could also output 
glitches until the PLL is locked. It could be a separate patch.

It's strange that this BYPASS bit is also handled by muxes like 
audio_pll1_bypass in clk-imx8mm.c but that's a separate issue not 
strictly related to the glitches you're trying to fix here.

--
Regards,
Leonard

Peng Fan Aug. 22, 2019, 9:18 a.m. UTC | #2

> Subject: Re: [PATCH] clk: imx: pll14xx: avoid glitch when set rate
> 
> On 20.08.2019 05:17, Peng Fan wrote:
> > According to PLL1443XA and PLL1416X spec, "When BYPASS is 0 and RESETB
> > is changed from 0 to 1, FOUT starts to output unstable clock until
> > lock time passes. PLL1416X/PLL1443XA may generate a glitch at FOUT."
> >
> > So set BYPASS when RESETB is changed from 0 to 1 to avoid glitch.
> > In the end of set rate, BYPASS will be cleared.
> >
> > @@ -191,6 +191,10 @@ static int clk_pll1416x_set_rate(struct clk_hw *hw,
> unsigned long drate,
> >   	tmp &= ~RST_MASK;
> >   	writel_relaxed(tmp, pll->base);
> >
> > +	/* Enable BYPASS */
> > +	tmp |= BYPASS_MASK;
> > +	writel(tmp, pll->base);
> > +
> 
> Shouldn't BYPASS be set before reset?

No. the glitch happens when RESET changes from 0 to 1, not from 1 to 0.

> 
> Also, isn't a similar bypass/unbypass dance also needed in
> clk_pll14xx_prepare? As far as I understand that could also output glitches
> until the PLL is locked. It could be a separate patch.

Yes, that might also output glitch. Fix in v2.

> 
> It's strange that this BYPASS bit is also handled by muxes like
> audio_pll1_bypass in clk-imx8mm.c but that's a separate issue not strictly
> related to the glitches you're trying to fix here.

Yes, need use EXT_BYPASS for the mux usage.

Regards,
Peng.

> 
> --
> Regards,
> Leonard

Leonard Crestez Aug. 22, 2019, 12:34 p.m. UTC | #3

On 22.08.2019 12:18, Peng Fan wrote:
>> Subject: Re: [PATCH] clk: imx: pll14xx: avoid glitch when set rate
>>
>> On 20.08.2019 05:17, Peng Fan wrote:
>>> According to PLL1443XA and PLL1416X spec, "When BYPASS is 0 and RESETB
>>> is changed from 0 to 1, FOUT starts to output unstable clock until
>>> lock time passes. PLL1416X/PLL1443XA may generate a glitch at FOUT."
>>>
>>> So set BYPASS when RESETB is changed from 0 to 1 to avoid glitch.
>>> In the end of set rate, BYPASS will be cleared.
>>>
>>> @@ -191,6 +191,10 @@ static int clk_pll1416x_set_rate(struct clk_hw *hw,
>> unsigned long drate,
>>>    	tmp &= ~RST_MASK;
>>>    	writel_relaxed(tmp, pll->base);
>>>
>>> +	/* Enable BYPASS */
>>> +	tmp |= BYPASS_MASK;
>>> +	writel(tmp, pll->base);
>>> +
>>
>> Shouldn't BYPASS be set before reset?
> 
> No. the glitch happens when RESET changes from 0 to 1, not from 1 to 0.

You're right, sorry.

>> Also, isn't a similar bypass/unbypass dance also needed in
>> clk_pll14xx_prepare? As far as I understand that could also output glitches
>> until the PLL is locked. It could be a separate patch.
> 
> Yes, that might also output glitch. Fix in v2.
> 
>>
>> It's strange that this BYPASS bit is also handled by muxes like
>> audio_pll1_bypass in clk-imx8mm.c but that's a separate issue not strictly
>> related to the glitches you're trying to fix here.
> 
> Yes, need use EXT_BYPASS for the mux usage.

Might make sense to post as a series.

Patch

diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
index b7213023b238..bd072556bc44 100644
--- a/drivers/clk/imx/clk-pll14xx.c
+++ b/drivers/clk/imx/clk-pll14xx.c
@@ -191,6 +191,10 @@  static int clk_pll1416x_set_rate(struct clk_hw *hw, unsigned long drate,
 	tmp &= ~RST_MASK;
 	writel_relaxed(tmp, pll->base);
 
+	/* Enable BYPASS */
+	tmp |= BYPASS_MASK;
+	writel(tmp, pll->base);
+
 	div_val = (rate->mdiv << MDIV_SHIFT) | (rate->pdiv << PDIV_SHIFT) |
 		(rate->sdiv << SDIV_SHIFT);
 	writel_relaxed(div_val, pll->base + 0x4);
@@ -250,6 +254,10 @@  static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
 	tmp &= ~RST_MASK;
 	writel_relaxed(tmp, pll->base);
 
+	/* Enable BYPASS */
+	tmp |= BYPASS_MASK;
+	writel_relaxed(tmp, pll->base);
+
 	div_val = (rate->mdiv << MDIV_SHIFT) | (rate->pdiv << PDIV_SHIFT) |
 		(rate->sdiv << SDIV_SHIFT);
 	writel_relaxed(div_val, pll->base + 0x4);