| Message ID | c3e86b5a832a14278e8ba670d51defc70ee78d84.1567590349.git.leonard.crestez@nxp.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | clk: imx: pll14xx: Fix quick switch of S/K parameter | expand |
Quoting Leonard Crestez (2019-09-04 02:49:18) > The PLL14xx on imx8m can change the S and K parameter without requiring > a reset and relock of the whole PLL. > > Fix clk_pll144xx_mp_change register reading and use it for pll1443 as > well since no reset+relock is required on K changes either. > > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> > --- > drivers/clk/imx/clk-pll14xx.c | 40 +++++++---------------------------- > 1 file changed, 8 insertions(+), 32 deletions(-) > > The PLLs are currently table-based and none of the entries differ only > in S/K so further work would be required to make use of this. The > prospective user is audio doing tiny freq adjustments and there is no > standard API for that. sub-Hz adjustments? > > Lacking users is not a good reason to carry broken code around. Maybe add a Fixes tag so if anyone wants to use it in LTS kernels there might be a chance that they'll find this patch mention code they're using.
On 06.09.2019 20:24, Stephen Boyd wrote: > Quoting Leonard Crestez (2019-09-04 02:49:18) >> The PLL14xx on imx8m can change the S and K parameter without requiring >> a reset and relock of the whole PLL. >> >> Fix clk_pll144xx_mp_change register reading and use it for pll1443 as >> well since no reset+relock is required on K changes either. >> >> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> >> --- >> drivers/clk/imx/clk-pll14xx.c | 40 +++++++---------------------------- >> 1 file changed, 8 insertions(+), 32 deletions(-) >> >> The PLLs are currently table-based and none of the entries differ only >> in S/K so further work would be required to make use of this. The >> prospective user is audio doing tiny freq adjustments and there is no >> standard API for that. > > sub-Hz adjustments? Maybe at the audio level? The PLL itself runs at ~400Mhz so wouldn't need sub-hz adjustment. My understanding is that adjustments would be made based on an external clock so if CLK framework rounds to 1hz then it would just take longer for adjustment to kick in. >> Lacking users is not a good reason to carry broken code around. > > Maybe add a Fixes tag so if anyone wants to use it in LTS kernels there > might be a chance that they'll find this patch mention code they're > using. It doesn't meet stable kernel rules because it doesn't "fix a real bug that bothers people" but it's still technically a fix: Fixes: 8646d4dcc7fb ("clk: imx: Add PLLs driver for imx8mm soc") -- Regards, Leonard
Quoting Leonard Crestez (2019-09-06 12:36:47) > On 06.09.2019 20:24, Stephen Boyd wrote: > > Quoting Leonard Crestez (2019-09-04 02:49:18) > >> The PLL14xx on imx8m can change the S and K parameter without requiring > >> a reset and relock of the whole PLL. > >> > >> Fix clk_pll144xx_mp_change register reading and use it for pll1443 as > >> well since no reset+relock is required on K changes either. > >> > >> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> > >> --- > >> drivers/clk/imx/clk-pll14xx.c | 40 +++++++---------------------------- > >> 1 file changed, 8 insertions(+), 32 deletions(-) > >> > >> The PLLs are currently table-based and none of the entries differ only > >> in S/K so further work would be required to make use of this. The > >> prospective user is audio doing tiny freq adjustments and there is no > >> standard API for that. > > > > sub-Hz adjustments? > > Maybe at the audio level? The PLL itself runs at ~400Mhz so wouldn't > need sub-hz adjustment. > > My understanding is that adjustments would be made based on an external > clock so if CLK framework rounds to 1hz then it would just take longer > for adjustment to kick in. Ok. > > >> Lacking users is not a good reason to carry broken code around. > > > > Maybe add a Fixes tag so if anyone wants to use it in LTS kernels there > > might be a chance that they'll find this patch mention code they're > > using. > > It doesn't meet stable kernel rules because it doesn't "fix a real bug > that bothers people" but it's still technically a fix: > > Fixes: 8646d4dcc7fb ("clk: imx: Add PLLs driver for imx8mm soc") > Sure. Thanks! I assume Shawn will pick this up.
On Wed, Sep 04, 2019 at 12:49:18PM +0300, Leonard Crestez wrote: > The PLL14xx on imx8m can change the S and K parameter without requiring > a reset and relock of the whole PLL. > > Fix clk_pll144xx_mp_change register reading and use it for pll1443 as > well since no reset+relock is required on K changes either. > > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> Applied, thanks.
diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c index b7213023b238..25342297e5a6 100644 --- a/drivers/clk/imx/clk-pll14xx.c +++ b/drivers/clk/imx/clk-pll14xx.c @@ -110,47 +110,21 @@ static unsigned long clk_pll1443x_recalc_rate(struct clk_hw *hw, do_div(fvco, pdiv << sdiv); return fvco; } -static inline bool clk_pll1416x_mp_change(const struct imx_pll14xx_rate_table *rate, +static inline bool clk_pll14xx_mp_change(const struct imx_pll14xx_rate_table *rate, u32 pll_div) { u32 old_mdiv, old_pdiv; - old_mdiv = (pll_div >> MDIV_SHIFT) & MDIV_MASK; - old_pdiv = (pll_div >> PDIV_SHIFT) & PDIV_MASK; + old_mdiv = (pll_div & MDIV_MASK) >> MDIV_SHIFT; + old_pdiv = (pll_div & PDIV_MASK) >> PDIV_SHIFT; return rate->mdiv != old_mdiv || rate->pdiv != old_pdiv; } -static inline bool clk_pll1443x_mpk_change(const struct imx_pll14xx_rate_table *rate, - u32 pll_div_ctl0, u32 pll_div_ctl1) -{ - u32 old_mdiv, old_pdiv, old_kdiv; - - old_mdiv = (pll_div_ctl0 >> MDIV_SHIFT) & MDIV_MASK; - old_pdiv = (pll_div_ctl0 >> PDIV_SHIFT) & PDIV_MASK; - old_kdiv = (pll_div_ctl1 >> KDIV_SHIFT) & KDIV_MASK; - - return rate->mdiv != old_mdiv || rate->pdiv != old_pdiv || - rate->kdiv != old_kdiv; -} - -static inline bool clk_pll1443x_mp_change(const struct imx_pll14xx_rate_table *rate, - u32 pll_div_ctl0, u32 pll_div_ctl1) -{ - u32 old_mdiv, old_pdiv, old_kdiv; - - old_mdiv = (pll_div_ctl0 >> MDIV_SHIFT) & MDIV_MASK; - old_pdiv = (pll_div_ctl0 >> PDIV_SHIFT) & PDIV_MASK; - old_kdiv = (pll_div_ctl1 >> KDIV_SHIFT) & KDIV_MASK; - - return rate->mdiv != old_mdiv || rate->pdiv != old_pdiv || - rate->kdiv != old_kdiv; -} - static int clk_pll14xx_wait_lock(struct clk_pll14xx *pll) { u32 val; return readl_poll_timeout(pll->base, val, val & LOCK_TIMEOUT_US, 0, @@ -172,11 +146,11 @@ static int clk_pll1416x_set_rate(struct clk_hw *hw, unsigned long drate, return -EINVAL; } tmp = readl_relaxed(pll->base + 4); - if (!clk_pll1416x_mp_change(rate, tmp)) { + if (!clk_pll14xx_mp_change(rate, tmp)) { tmp &= ~(SDIV_MASK) << SDIV_SHIFT; tmp |= rate->sdiv << SDIV_SHIFT; writel_relaxed(tmp, pll->base + 4); return 0; @@ -233,17 +207,19 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate, drate, clk_hw_get_name(hw)); return -EINVAL; } tmp = readl_relaxed(pll->base + 4); - div_val = readl_relaxed(pll->base + 8); - if (!clk_pll1443x_mpk_change(rate, tmp, div_val)) { + if (!clk_pll14xx_mp_change(rate, tmp)) { tmp &= ~(SDIV_MASK) << SDIV_SHIFT; tmp |= rate->sdiv << SDIV_SHIFT; writel_relaxed(tmp, pll->base + 4); + tmp = rate->kdiv << KDIV_SHIFT; + writel_relaxed(tmp, pll->base + 8); + return 0; } /* Enable RST */ tmp = readl_relaxed(pll->base);
The PLL14xx on imx8m can change the S and K parameter without requiring a reset and relock of the whole PLL. Fix clk_pll144xx_mp_change register reading and use it for pll1443 as well since no reset+relock is required on K changes either. Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> --- drivers/clk/imx/clk-pll14xx.c | 40 +++++++---------------------------- 1 file changed, 8 insertions(+), 32 deletions(-) The PLLs are currently table-based and none of the entries differ only in S/K so further work would be required to make use of this. The prospective user is audio doing tiny freq adjustments and there is no standard API for that. Lacking users is not a good reason to carry broken code around.