| Message ID | 20240510-imx-clk-v2-5-c998f315d29c@nxp.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | clk: imx: misc update/fix | expand |
On 10/05/2024 11.19, Peng Fan (OSS) wrote: > From: Shengjiu Wang <shengjiu.wang@nxp.com> > > The fvco frequency range is between 1600MHz and 3200MHz, without > this constraint the fvco may out of range, the real output > frequency is no accurate. Could you please point everybody in the direction of where that requirement is stated? The imx8mp reference manual, for example, merely lists constraints for p, m, s and k. > > /* First try if we can get the desired rate from one of the static entries */ > @@ -193,6 +195,10 @@ static void imx_pll14xx_calc_settings(struct clk_pll14xx *pll, unsigned long rat > kdiv = pll1443x_calc_kdiv(mdiv, pdiv, sdiv, rate, prate); > fout = pll14xx_calc_rate(pll, mdiv, pdiv, sdiv, kdiv, prate); > > + fvco = fout << sdiv; > + > + if (fvco < 1600000000 || fvco > 3200000000) > + continue; If this is really a necessary constraint, it seems that one could just up-front compute the only possible value of s, or at least change the logic so that one loops over a smaller range of possible values of s. Rasmus
On 10/05/2024 11.19, Peng Fan (OSS) wrote: > > Aslo correct the name for fvco and fout clock. Btw., that part of the commit log is misleading. The patch does no such thing, as that part is already done in f52f00069888 that went into v6.8. Please don't mindlessly cherry-pick stuff from the NXP fork and fixup stuff so it builds and just ignore whether the commit log still makes sense. Rasmus
> Subject: Re: [PATCH v2 05/17] clk: imx: pll14xx: Add constraint for fvco > frequency > > On 10/05/2024 11.19, Peng Fan (OSS) wrote: > > From: Shengjiu Wang <shengjiu.wang@nxp.com> > > > > The fvco frequency range is between 1600MHz and 3200MHz, without this > > constraint the fvco may out of range, the real output frequency is no > > accurate. > > Could you please point everybody in the direction of where that requirement > is stated? VCO has a range, we need update reference manual. Regards, Peng. The imx8mp reference manual, for example, merely lists constraints > for p, m, s and k. > > > > > > /* First try if we can get the desired rate from one of the static > > entries */ @@ -193,6 +195,10 @@ static void > imx_pll14xx_calc_settings(struct clk_pll14xx *pll, unsigned long rat > > kdiv = pll1443x_calc_kdiv(mdiv, pdiv, sdiv, rate, > prate); > > fout = pll14xx_calc_rate(pll, mdiv, pdiv, sdiv, kdiv, > prate); > > > > + fvco = fout << sdiv; > > + > > + if (fvco < 1600000000 || fvco > 3200000000) > > + continue; > > If this is really a necessary constraint, it seems that one could just up-front > compute the only possible value of s, or at least change the logic so that one > loops over a smaller range of possible values of s. > > Rasmus
On Fri, May 10, 2024 at 4:14 AM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote: > > From: Shengjiu Wang <shengjiu.wang@nxp.com> > > The fvco frequency range is between 1600MHz and 3200MHz, without > this constraint the fvco may out of range, the real output > frequency is no accurate. > > Aslo correct the name for fvco and fout clock. > > Fixes: b09c68dc57c9 ("clk: imx: pll14xx: Support dynamic rates") > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com> > Acked-by: Jacky Bai <ping.bai@nxp.com> > Tested-by: Chancel Liu <chancel.liu@nxp.com> > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > drivers/clk/imx/clk-pll14xx.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c > index d63564dbb12c..55812bfb9ec2 100644 > --- a/drivers/clk/imx/clk-pll14xx.c > +++ b/drivers/clk/imx/clk-pll14xx.c > @@ -131,7 +131,7 @@ static void imx_pll14xx_calc_settings(struct clk_pll14xx *pll, unsigned long rat > { > u32 pll_div_ctl0, pll_div_ctl1; > int mdiv, pdiv, sdiv, kdiv; > - long fout, rate_min, rate_max, dist, best = LONG_MAX; > + long fvco, fout, rate_min, rate_max, dist, best = LONG_MAX; > const struct imx_pll14xx_rate_table *tt; > > /* > @@ -144,6 +144,8 @@ static void imx_pll14xx_calc_settings(struct clk_pll14xx *pll, unsigned long rat > * > * fvco = (m * 65536 + k) * prate / (p * 65536) > * fout = (m * 65536 + k) * prate / (p * 65536) / (1 << sdiv) > + * > + * e) 1600MHz <= fvco <= 3200MHz > */ > > /* First try if we can get the desired rate from one of the static entries */ > @@ -193,6 +195,10 @@ static void imx_pll14xx_calc_settings(struct clk_pll14xx *pll, unsigned long rat > kdiv = pll1443x_calc_kdiv(mdiv, pdiv, sdiv, rate, prate); > fout = pll14xx_calc_rate(pll, mdiv, pdiv, sdiv, kdiv, prate); > > + fvco = fout << sdiv; The calculation of fvco here does not match the comment above where fvco = (m * 65536 + k) * prate / (p * 65536) > + > + if (fvco < 1600000000 || fvco > 3200000000) > + continue; > /* best match */ > dist = abs((long)rate - (long)fout); > if (dist < best) { > > -- > 2.37.1 > >
> Subject: Re: [PATCH v2 05/17] clk: imx: pll14xx: Add constraint for fvco > frequency > > On 10/05/2024 11.19, Peng Fan (OSS) wrote: > > > > Aslo correct the name for fvco and fout clock. > > Btw., that part of the commit log is misleading. The patch does no such thing, > as that part is already done in f52f00069888 that went into v6.8. > > Please don't mindlessly cherry-pick stuff from the NXP fork and fixup stuff so > it builds and just ignore whether the commit log still makes sense. My bad. will go through tree history next time before post these. Regards, Peng. > > Rasmus
diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c index d63564dbb12c..55812bfb9ec2 100644 --- a/drivers/clk/imx/clk-pll14xx.c +++ b/drivers/clk/imx/clk-pll14xx.c @@ -131,7 +131,7 @@ static void imx_pll14xx_calc_settings(struct clk_pll14xx *pll, unsigned long rat { u32 pll_div_ctl0, pll_div_ctl1; int mdiv, pdiv, sdiv, kdiv; - long fout, rate_min, rate_max, dist, best = LONG_MAX; + long fvco, fout, rate_min, rate_max, dist, best = LONG_MAX; const struct imx_pll14xx_rate_table *tt; /* @@ -144,6 +144,8 @@ static void imx_pll14xx_calc_settings(struct clk_pll14xx *pll, unsigned long rat * * fvco = (m * 65536 + k) * prate / (p * 65536) * fout = (m * 65536 + k) * prate / (p * 65536) / (1 << sdiv) + * + * e) 1600MHz <= fvco <= 3200MHz */ /* First try if we can get the desired rate from one of the static entries */ @@ -193,6 +195,10 @@ static void imx_pll14xx_calc_settings(struct clk_pll14xx *pll, unsigned long rat kdiv = pll1443x_calc_kdiv(mdiv, pdiv, sdiv, rate, prate); fout = pll14xx_calc_rate(pll, mdiv, pdiv, sdiv, kdiv, prate); + fvco = fout << sdiv; + + if (fvco < 1600000000 || fvco > 3200000000) + continue; /* best match */ dist = abs((long)rate - (long)fout); if (dist < best) {