| Message ID | 20221210203835.9714-1-kgroeneveld@lenbrook.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | clk: imx: pll14xx: fix recalc_rate for negative kdiv | expand |
Quoting Kevin Groeneveld (2022-12-10 12:38:35) > kdiv is a signed 16 bit value in the DEV_CTL1 register. Commit > 53990cf9d5b4 ("clk: imx: pll14xx: consolidate rate calculation") changed > the kdiv variable from a short int to just int. When the value read from > the DIV_CTL1 register is assigned directly to an int the sign of the value > is lost resulting in incorrect results when the value is negative. Adding Can the field be negative? > a s16 cast to the register value fixes the issue. > > Fixes: 53990cf9d5b4 ("clk: imx: pll14xx: consolidate rate calculation") > Signed-off-by: Kevin Groeneveld <kgroeneveld@lenbrook.com>
On 2023-02-10 18:24, Stephen Boyd wrote: > Quoting Kevin Groeneveld (2022-12-10 12:38:35) >> kdiv is a signed 16 bit value in the DEV_CTL1 register. Commit >> 53990cf9d5b4 ("clk: imx: pll14xx: consolidate rate calculation") changed >> the kdiv variable from a short int to just int. When the value read from >> the DIV_CTL1 register is assigned directly to an int the sign of the value >> is lost resulting in incorrect results when the value is negative. Adding > > Can the field be negative? Yes it can be. That is how I found the issue as I had a negative value in imx_pll14xx_rate_table which used to work but broke with the above mentioned commit. The i.MX8MM reference manual states: >Formula for Fraction PLLOUT: >* FOUT=((m+k/65536) x FIN) /(p x 2s) >* Where, 1 ≤ p ≤ 63, 64 ≤ m ≤ 1023, 0 ≤ s ≤ 6, -32768 ≤ k ≤ 32767 >* p, m, and s are unsigned integers. k is a two's complement integer. The comments in the imx_pll14xx_calc_settings function also mention the same range for kdiv. And this function also uses negative values. For example: >/* Then see if we can get the desired rate by only adjusting kdiv (glitch free) */ >rate_min = pll14xx_calc_rate(pll, mdiv, pdiv, sdiv, KDIV_MIN, prate); >rate_max = pll14xx_calc_rate(pll, mdiv, pdiv, sdiv, KDIV_MAX, prate); Where KDIV_MIN is defined as SHRT_MIN which is negative. Kevin
On 22-12-10 15:38:35, Kevin Groeneveld wrote: > kdiv is a signed 16 bit value in the DEV_CTL1 register. Commit > 53990cf9d5b4 ("clk: imx: pll14xx: consolidate rate calculation") changed > the kdiv variable from a short int to just int. When the value read from > the DIV_CTL1 register is assigned directly to an int the sign of the value > is lost resulting in incorrect results when the value is negative. Adding > a s16 cast to the register value fixes the issue. > > Fixes: 53990cf9d5b4 ("clk: imx: pll14xx: consolidate rate calculation") > Signed-off-by: Kevin Groeneveld <kgroeneveld@lenbrook.com> Reviewed-by: Abel Vesa <abel.vesa@linaro.org> Stephen, can you pick this up through fixes? > --- > drivers/clk/imx/clk-pll14xx.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c > index 1d0f79e9c346..d12194d17b10 100644 > --- a/drivers/clk/imx/clk-pll14xx.c > +++ b/drivers/clk/imx/clk-pll14xx.c > @@ -254,7 +254,7 @@ static unsigned long clk_pll14xx_recalc_rate(struct clk_hw *hw, > > if (pll->type == PLL_1443X) { > pll_div_ctl1 = readl_relaxed(pll->base + DIV_CTL1); > - kdiv = FIELD_GET(KDIV_MASK, pll_div_ctl1); > + kdiv = (s16)FIELD_GET(KDIV_MASK, pll_div_ctl1); > } else { > kdiv = 0; > } > -- > 2.17.1 >
Quoting Kevin Groeneveld (2022-12-10 12:38:35) > kdiv is a signed 16 bit value in the DEV_CTL1 register. Commit > 53990cf9d5b4 ("clk: imx: pll14xx: consolidate rate calculation") changed > the kdiv variable from a short int to just int. When the value read from > the DIV_CTL1 register is assigned directly to an int the sign of the value > is lost resulting in incorrect results when the value is negative. Adding > a s16 cast to the register value fixes the issue. > > Fixes: 53990cf9d5b4 ("clk: imx: pll14xx: consolidate rate calculation") > Signed-off-by: Kevin Groeneveld <kgroeneveld@lenbrook.com> > --- Applied to clk-fixes
diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c index 1d0f79e9c346..d12194d17b10 100644 --- a/drivers/clk/imx/clk-pll14xx.c +++ b/drivers/clk/imx/clk-pll14xx.c @@ -254,7 +254,7 @@ static unsigned long clk_pll14xx_recalc_rate(struct clk_hw *hw, if (pll->type == PLL_1443X) { pll_div_ctl1 = readl_relaxed(pll->base + DIV_CTL1); - kdiv = FIELD_GET(KDIV_MASK, pll_div_ctl1); + kdiv = (s16)FIELD_GET(KDIV_MASK, pll_div_ctl1); } else { kdiv = 0; }
kdiv is a signed 16 bit value in the DEV_CTL1 register. Commit 53990cf9d5b4 ("clk: imx: pll14xx: consolidate rate calculation") changed the kdiv variable from a short int to just int. When the value read from the DIV_CTL1 register is assigned directly to an int the sign of the value is lost resulting in incorrect results when the value is negative. Adding a s16 cast to the register value fixes the issue. Fixes: 53990cf9d5b4 ("clk: imx: pll14xx: consolidate rate calculation") Signed-off-by: Kevin Groeneveld <kgroeneveld@lenbrook.com> --- drivers/clk/imx/clk-pll14xx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)