* [PATCH 0/1] clk: nuvoton: ma35d1: fix PLL frequency calculation @ 2026-05-11 3:15 Joey Lu 2026-05-11 3:15 ` [PATCH 1/1] " Joey Lu 0 siblings, 1 reply; 4+ messages in thread From: Joey Lu @ 2026-05-11 3:15 UTC (permalink / raw) To: mturquette, sboyd Cc: ychuang3, schung, yclu4, linux-arm-kernel, linux-clk, linux-kernel, Joey Lu Fix four bugs in the MA35D1 PLL clock driver that cause incorrect frequency values returned from recalc_rate() and determine_rate(). -- 2.49.0 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 1/1] clk: nuvoton: ma35d1: fix PLL frequency calculation 2026-05-11 3:15 [PATCH 0/1] clk: nuvoton: ma35d1: fix PLL frequency calculation Joey Lu @ 2026-05-11 3:15 ` Joey Lu 2026-05-11 15:54 ` Brian Masney 0 siblings, 1 reply; 4+ messages in thread From: Joey Lu @ 2026-05-11 3:15 UTC (permalink / raw) To: mturquette, sboyd Cc: ychuang3, schung, yclu4, linux-arm-kernel, linux-clk, linux-kernel, Joey Lu Fix four bugs in the MA35D1 PLL driver: 1. PLL_CTL1_FRAC was defined as GENMASK(31, 24) (8 bits), but the hardware fractional field spans bits [31:8] (24 bits). This caused wrong frequency calculation in fractional and spread-spectrum modes. 2. div_u64() does not modify its argument in-place; the quotient must be assigned from the return value. Both ma35d1_calc_smic_pll_freq() and ma35d1_calc_pll_freq() discarded the return value, leaving pll_freq undivided and orders of magnitude too high. 3. The fractional-mode calculation divided x by FIELD_MAX(PLL_CTL1_FRAC) to get 2 decimal digits. After correcting the mask to 24 bits, update the arithmetic to use 3 decimal digits with proper 24-bit fixed-point rounding. 4. ma35d1_clk_pll_determine_rate() called ma35d1_pll_find_closest() unconditionally before the switch, but then overwrote its result by reading the current hardware registers for every PLL type. Move the find_closest() call inside the configurable-PLL branch (APLL, EPLL, VPLL). CAPLL and DDRPLL do not support runtime rate changes and correctly return the current hardware rate. Fixes: 691521a367cf ("clk: nuvoton: Add clock driver for ma35d1 clock controller") Signed-off-by: Joey Lu <a0987203069@gmail.com> --- drivers/clk/nuvoton/clk-ma35d1-pll.c | 34 +++++++++++++-------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/drivers/clk/nuvoton/clk-ma35d1-pll.c b/drivers/clk/nuvoton/clk-ma35d1-pll.c index 4620acfe47e8..314b81e7727c 100644 --- a/drivers/clk/nuvoton/clk-ma35d1-pll.c +++ b/drivers/clk/nuvoton/clk-ma35d1-pll.c @@ -48,7 +48,7 @@ #define PLL_CTL1_PD BIT(0) #define PLL_CTL1_BP BIT(1) #define PLL_CTL1_OUTDIV GENMASK(6, 4) -#define PLL_CTL1_FRAC GENMASK(31, 24) +#define PLL_CTL1_FRAC GENMASK(31, 8) #define PLL_CTL2_SLOPE GENMASK(23, 0) #define INDIV_MIN 1 @@ -92,7 +92,7 @@ static unsigned long ma35d1_calc_smic_pll_freq(u32 pll0_ctl0, p = FIELD_GET(SPLL0_CTL0_OUTDIV, pll0_ctl0); outdiv = 1 << p; pll_freq = (u64)parent_rate * n; - div_u64(pll_freq, m * outdiv); + pll_freq = div_u64(pll_freq, m * outdiv); return pll_freq; } @@ -110,12 +110,12 @@ static unsigned long ma35d1_calc_pll_freq(u8 mode, u32 *reg_ctl, unsigned long p if (mode == PLL_MODE_INT) { pll_freq = (u64)parent_rate * n; - div_u64(pll_freq, m * p); + pll_freq = div_u64(pll_freq, m * p); } else { x = FIELD_GET(PLL_CTL1_FRAC, reg_ctl[1]); - /* 2 decimal places floating to integer (ex. 1.23 to 123) */ - n = n * 100 + ((x * 100) / FIELD_MAX(PLL_CTL1_FRAC)); - pll_freq = div_u64(parent_rate * n, 100 * m * p); + /* x is 24-bit fractional part, convert to 3 decimal digits */ + n = n * 1000 + (u32)(((u64)x * 1000 + 500) >> 24); + pll_freq = div_u64((u64)parent_rate * n, 1000 * m * p); } return pll_freq; } @@ -255,32 +255,32 @@ static int ma35d1_clk_pll_determine_rate(struct clk_hw *hw, if (req->best_parent_rate < PLL_FREF_MIN_FREQ || req->best_parent_rate > PLL_FREF_MAX_FREQ) return -EINVAL; - ret = ma35d1_pll_find_closest(pll, req->rate, req->best_parent_rate, - reg_ctl, &pll_freq); - if (ret < 0) - return ret; - switch (pll->id) { case CAPLL: + case DDRPLL: + /* Read-only PLLs: return current rate */ reg_ctl[0] = readl_relaxed(pll->ctl0_base); - pll_freq = ma35d1_calc_smic_pll_freq(reg_ctl[0], req->best_parent_rate); + if (pll->id == CAPLL) { + pll_freq = ma35d1_calc_smic_pll_freq(reg_ctl[0], req->best_parent_rate); + } else { + reg_ctl[1] = readl_relaxed(pll->ctl1_base); + pll_freq = ma35d1_calc_pll_freq(pll->mode, reg_ctl, req->best_parent_rate); + } req->rate = pll_freq; - return 0; - case DDRPLL: case APLL: case EPLL: case VPLL: - reg_ctl[0] = readl_relaxed(pll->ctl0_base); - reg_ctl[1] = readl_relaxed(pll->ctl1_base); - pll_freq = ma35d1_calc_pll_freq(pll->mode, reg_ctl, req->best_parent_rate); + /* Configurable PLLs: find closest achievable rate */ + ret = ma35d1_pll_find_closest(pll, req->rate, req->best_parent_rate, + reg_ctl, &pll_freq); + if (ret < 0) + return ret; req->rate = pll_freq; - return 0; } req->rate = 0; - return 0; } -- 2.49.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] clk: nuvoton: ma35d1: fix PLL frequency calculation 2026-05-11 3:15 ` [PATCH 1/1] " Joey Lu @ 2026-05-11 15:54 ` Brian Masney 2026-05-12 3:49 ` Joey Lu 0 siblings, 1 reply; 4+ messages in thread From: Brian Masney @ 2026-05-11 15:54 UTC (permalink / raw) To: Joey Lu Cc: mturquette, sboyd, ychuang3, schung, yclu4, linux-arm-kernel, linux-clk, linux-kernel Hi Joey, On Mon, May 11, 2026 at 11:15:59AM +0800, Joey Lu wrote: > Fix four bugs in the MA35D1 PLL driver: > > 1. PLL_CTL1_FRAC was defined as GENMASK(31, 24) (8 bits), but the > hardware fractional field spans bits [31:8] (24 bits). This caused > wrong frequency calculation in fractional and spread-spectrum modes. > > 2. div_u64() does not modify its argument in-place; the quotient must > be assigned from the return value. Both ma35d1_calc_smic_pll_freq() > and ma35d1_calc_pll_freq() discarded the return value, leaving > pll_freq undivided and orders of magnitude too high. > > 3. The fractional-mode calculation divided x by FIELD_MAX(PLL_CTL1_FRAC) > to get 2 decimal digits. After correcting the mask to 24 bits, update > the arithmetic to use 3 decimal digits with proper 24-bit fixed-point > rounding. > > 4. ma35d1_clk_pll_determine_rate() called ma35d1_pll_find_closest() > unconditionally before the switch, but then overwrote its result by > reading the current hardware registers for every PLL type. Move the > find_closest() call inside the configurable-PLL branch (APLL, EPLL, > VPLL). CAPLL and DDRPLL do not support runtime rate changes and > correctly return the current hardware rate. > > Fixes: 691521a367cf ("clk: nuvoton: Add clock driver for ma35d1 clock controller") > Signed-off-by: Joey Lu <a0987203069@gmail.com> Thanks for the patch, however this should really be broken up into more patches. If possible, one patch for each of the fixes. Brian > --- > drivers/clk/nuvoton/clk-ma35d1-pll.c | 34 +++++++++++++-------------- > 1 file changed, 17 insertions(+), 17 deletions(-) > > diff --git a/drivers/clk/nuvoton/clk-ma35d1-pll.c b/drivers/clk/nuvoton/clk-ma35d1-pll.c > index 4620acfe47e8..314b81e7727c 100644 > --- a/drivers/clk/nuvoton/clk-ma35d1-pll.c > +++ b/drivers/clk/nuvoton/clk-ma35d1-pll.c > @@ -48,7 +48,7 @@ > #define PLL_CTL1_PD BIT(0) > #define PLL_CTL1_BP BIT(1) > #define PLL_CTL1_OUTDIV GENMASK(6, 4) > -#define PLL_CTL1_FRAC GENMASK(31, 24) > +#define PLL_CTL1_FRAC GENMASK(31, 8) > #define PLL_CTL2_SLOPE GENMASK(23, 0) > > #define INDIV_MIN 1 > @@ -92,7 +92,7 @@ static unsigned long ma35d1_calc_smic_pll_freq(u32 pll0_ctl0, > p = FIELD_GET(SPLL0_CTL0_OUTDIV, pll0_ctl0); > outdiv = 1 << p; > pll_freq = (u64)parent_rate * n; > - div_u64(pll_freq, m * outdiv); > + pll_freq = div_u64(pll_freq, m * outdiv); > return pll_freq; > } > > @@ -110,12 +110,12 @@ static unsigned long ma35d1_calc_pll_freq(u8 mode, u32 *reg_ctl, unsigned long p > > if (mode == PLL_MODE_INT) { > pll_freq = (u64)parent_rate * n; > - div_u64(pll_freq, m * p); > + pll_freq = div_u64(pll_freq, m * p); > } else { > x = FIELD_GET(PLL_CTL1_FRAC, reg_ctl[1]); > - /* 2 decimal places floating to integer (ex. 1.23 to 123) */ > - n = n * 100 + ((x * 100) / FIELD_MAX(PLL_CTL1_FRAC)); > - pll_freq = div_u64(parent_rate * n, 100 * m * p); > + /* x is 24-bit fractional part, convert to 3 decimal digits */ > + n = n * 1000 + (u32)(((u64)x * 1000 + 500) >> 24); > + pll_freq = div_u64((u64)parent_rate * n, 1000 * m * p); > } > return pll_freq; > } > @@ -255,32 +255,32 @@ static int ma35d1_clk_pll_determine_rate(struct clk_hw *hw, > if (req->best_parent_rate < PLL_FREF_MIN_FREQ || req->best_parent_rate > PLL_FREF_MAX_FREQ) > return -EINVAL; > > - ret = ma35d1_pll_find_closest(pll, req->rate, req->best_parent_rate, > - reg_ctl, &pll_freq); > - if (ret < 0) > - return ret; > - > switch (pll->id) { > case CAPLL: > + case DDRPLL: > + /* Read-only PLLs: return current rate */ > reg_ctl[0] = readl_relaxed(pll->ctl0_base); > - pll_freq = ma35d1_calc_smic_pll_freq(reg_ctl[0], req->best_parent_rate); > + if (pll->id == CAPLL) { > + pll_freq = ma35d1_calc_smic_pll_freq(reg_ctl[0], req->best_parent_rate); > + } else { > + reg_ctl[1] = readl_relaxed(pll->ctl1_base); > + pll_freq = ma35d1_calc_pll_freq(pll->mode, reg_ctl, req->best_parent_rate); > + } > req->rate = pll_freq; > - > return 0; > - case DDRPLL: > case APLL: > case EPLL: > case VPLL: > - reg_ctl[0] = readl_relaxed(pll->ctl0_base); > - reg_ctl[1] = readl_relaxed(pll->ctl1_base); > - pll_freq = ma35d1_calc_pll_freq(pll->mode, reg_ctl, req->best_parent_rate); > + /* Configurable PLLs: find closest achievable rate */ > + ret = ma35d1_pll_find_closest(pll, req->rate, req->best_parent_rate, > + reg_ctl, &pll_freq); > + if (ret < 0) > + return ret; > req->rate = pll_freq; > - > return 0; > } > > req->rate = 0; > - > return 0; > } > > -- > 2.49.0 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] clk: nuvoton: ma35d1: fix PLL frequency calculation 2026-05-11 15:54 ` Brian Masney @ 2026-05-12 3:49 ` Joey Lu 0 siblings, 0 replies; 4+ messages in thread From: Joey Lu @ 2026-05-12 3:49 UTC (permalink / raw) To: Brian Masney Cc: mturquette, sboyd, ychuang3, schung, yclu4, linux-arm-kernel, linux-clk, linux-kernel On 5/11/2026 11:54 PM, Brian Masney wrote: > Hi Joey, > > On Mon, May 11, 2026 at 11:15:59AM +0800, Joey Lu wrote: >> Fix four bugs in the MA35D1 PLL driver: >> >> 1. PLL_CTL1_FRAC was defined as GENMASK(31, 24) (8 bits), but the >> hardware fractional field spans bits [31:8] (24 bits). This caused >> wrong frequency calculation in fractional and spread-spectrum modes. >> >> 2. div_u64() does not modify its argument in-place; the quotient must >> be assigned from the return value. Both ma35d1_calc_smic_pll_freq() >> and ma35d1_calc_pll_freq() discarded the return value, leaving >> pll_freq undivided and orders of magnitude too high. >> >> 3. The fractional-mode calculation divided x by FIELD_MAX(PLL_CTL1_FRAC) >> to get 2 decimal digits. After correcting the mask to 24 bits, update >> the arithmetic to use 3 decimal digits with proper 24-bit fixed-point >> rounding. >> >> 4. ma35d1_clk_pll_determine_rate() called ma35d1_pll_find_closest() >> unconditionally before the switch, but then overwrote its result by >> reading the current hardware registers for every PLL type. Move the >> find_closest() call inside the configurable-PLL branch (APLL, EPLL, >> VPLL). CAPLL and DDRPLL do not support runtime rate changes and >> correctly return the current hardware rate. >> >> Fixes: 691521a367cf ("clk: nuvoton: Add clock driver for ma35d1 clock controller") >> Signed-off-by: Joey Lu <a0987203069@gmail.com> > Thanks for the patch, however this should really be broken up into more > patches. If possible, one patch for each of the fixes. > > Brian Thanks for the feedback.🙂 I understand your point and will split the changes into several patches, with each patch addressing one fix. > >> --- >> drivers/clk/nuvoton/clk-ma35d1-pll.c | 34 +++++++++++++-------------- >> 1 file changed, 17 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/clk/nuvoton/clk-ma35d1-pll.c b/drivers/clk/nuvoton/clk-ma35d1-pll.c >> index 4620acfe47e8..314b81e7727c 100644 >> --- a/drivers/clk/nuvoton/clk-ma35d1-pll.c >> +++ b/drivers/clk/nuvoton/clk-ma35d1-pll.c >> @@ -48,7 +48,7 @@ >> #define PLL_CTL1_PD BIT(0) >> #define PLL_CTL1_BP BIT(1) >> #define PLL_CTL1_OUTDIV GENMASK(6, 4) >> -#define PLL_CTL1_FRAC GENMASK(31, 24) >> +#define PLL_CTL1_FRAC GENMASK(31, 8) >> #define PLL_CTL2_SLOPE GENMASK(23, 0) >> >> #define INDIV_MIN 1 >> @@ -92,7 +92,7 @@ static unsigned long ma35d1_calc_smic_pll_freq(u32 pll0_ctl0, >> p = FIELD_GET(SPLL0_CTL0_OUTDIV, pll0_ctl0); >> outdiv = 1 << p; >> pll_freq = (u64)parent_rate * n; >> - div_u64(pll_freq, m * outdiv); >> + pll_freq = div_u64(pll_freq, m * outdiv); >> return pll_freq; >> } >> >> @@ -110,12 +110,12 @@ static unsigned long ma35d1_calc_pll_freq(u8 mode, u32 *reg_ctl, unsigned long p >> >> if (mode == PLL_MODE_INT) { >> pll_freq = (u64)parent_rate * n; >> - div_u64(pll_freq, m * p); >> + pll_freq = div_u64(pll_freq, m * p); >> } else { >> x = FIELD_GET(PLL_CTL1_FRAC, reg_ctl[1]); >> - /* 2 decimal places floating to integer (ex. 1.23 to 123) */ >> - n = n * 100 + ((x * 100) / FIELD_MAX(PLL_CTL1_FRAC)); >> - pll_freq = div_u64(parent_rate * n, 100 * m * p); >> + /* x is 24-bit fractional part, convert to 3 decimal digits */ >> + n = n * 1000 + (u32)(((u64)x * 1000 + 500) >> 24); >> + pll_freq = div_u64((u64)parent_rate * n, 1000 * m * p); >> } >> return pll_freq; >> } >> @@ -255,32 +255,32 @@ static int ma35d1_clk_pll_determine_rate(struct clk_hw *hw, >> if (req->best_parent_rate < PLL_FREF_MIN_FREQ || req->best_parent_rate > PLL_FREF_MAX_FREQ) >> return -EINVAL; >> >> - ret = ma35d1_pll_find_closest(pll, req->rate, req->best_parent_rate, >> - reg_ctl, &pll_freq); >> - if (ret < 0) >> - return ret; >> - >> switch (pll->id) { >> case CAPLL: >> + case DDRPLL: >> + /* Read-only PLLs: return current rate */ >> reg_ctl[0] = readl_relaxed(pll->ctl0_base); >> - pll_freq = ma35d1_calc_smic_pll_freq(reg_ctl[0], req->best_parent_rate); >> + if (pll->id == CAPLL) { >> + pll_freq = ma35d1_calc_smic_pll_freq(reg_ctl[0], req->best_parent_rate); >> + } else { >> + reg_ctl[1] = readl_relaxed(pll->ctl1_base); >> + pll_freq = ma35d1_calc_pll_freq(pll->mode, reg_ctl, req->best_parent_rate); >> + } >> req->rate = pll_freq; >> - >> return 0; >> - case DDRPLL: >> case APLL: >> case EPLL: >> case VPLL: >> - reg_ctl[0] = readl_relaxed(pll->ctl0_base); >> - reg_ctl[1] = readl_relaxed(pll->ctl1_base); >> - pll_freq = ma35d1_calc_pll_freq(pll->mode, reg_ctl, req->best_parent_rate); >> + /* Configurable PLLs: find closest achievable rate */ >> + ret = ma35d1_pll_find_closest(pll, req->rate, req->best_parent_rate, >> + reg_ctl, &pll_freq); >> + if (ret < 0) >> + return ret; >> req->rate = pll_freq; >> - >> return 0; >> } >> >> req->rate = 0; >> - >> return 0; >> } >> >> -- >> 2.49.0 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2026-05-12 3:49 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-05-11 3:15 [PATCH 0/1] clk: nuvoton: ma35d1: fix PLL frequency calculation Joey Lu 2026-05-11 3:15 ` [PATCH 1/1] " Joey Lu 2026-05-11 15:54 ` Brian Masney 2026-05-12 3:49 ` Joey Lu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox