clk: bcm: iproc: Fix overflow in clock calculation

[PATCH] clk: bcm: iproc: Fix overflow in clock calculation

Posted by Mark Tomlinson 1 week, 4 days ago

Some BCM iproc devices run at 1.25GHz. This comes from a 50MHZ crystal
with a pre-divider of two, a post-divider of two and a PLL multiplier of
100. As the multiply is done first (50M * 100) this overflows a 32-bit
value. The multiplication was done as a 64-bit multiply, but the result
assigned to a 32-bit variable before the division was done. To fix this,
use a 64-bit temporary variable and use the do_div() macro to perform a
64-bit/32-bit division.

Fixes: 5fe225c105fd ("clk: iproc: add initial common clock support")
Signed-off-by: Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz>
---
 drivers/clk/bcm/clk-iproc-armpll.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/bcm/clk-iproc-armpll.c b/drivers/clk/bcm/clk-iproc-armpll.c
index 9e86c0c10b57..665f1c531c4f 100644
--- a/drivers/clk/bcm/clk-iproc-armpll.c
+++ b/drivers/clk/bcm/clk-iproc-armpll.c
@@ -180,7 +180,7 @@ static unsigned int __get_ndiv(struct iproc_arm_pll *pll)
  *   mdiv = ARM PLL post divider
  *
  * The frequency is calculated by:
- *   ((ndiv * parent clock rate) / pdiv) / mdiv
+ *   (ndiv * parent clock rate) / (pdiv * mdiv)
  */
 static unsigned long iproc_arm_pll_recalc_rate(struct clk_hw *hw,
 		unsigned long parent_rate)
@@ -189,6 +189,7 @@ static unsigned long iproc_arm_pll_recalc_rate(struct clk_hw *hw,
 	u32 val;
 	int mdiv;
 	u64 ndiv;
+	u64 rate;
 	unsigned int pdiv;
 
 	/* in bypass mode, use parent rate */
@@ -216,8 +217,9 @@ static unsigned long iproc_arm_pll_recalc_rate(struct clk_hw *hw,
 		pll->rate = 0;
 		return 0;
 	}
-	pll->rate = (ndiv * parent_rate) >> 20;
-	pll->rate = (pll->rate / pdiv) / mdiv;
+	rate = (ndiv * parent_rate) >> 20;
+	do_div(rate, pdiv * mdiv);
+	pll->rate = rate;
 
 	pr_debug("%s: ARM PLL rate: %lu. parent rate: %lu\n", __func__,
 		 pll->rate, parent_rate);
-- 
2.52.0

Re: [PATCH] clk: bcm: iproc: Fix overflow in clock calculation

Posted by Ray Jui 1 week, 2 days ago

On Wed, Jan 28, 2026 at 4:41 PM Mark Tomlinson <
mark.tomlinson@alliedtelesis.co.nz> wrote:

> Some BCM iproc devices run at 1.25GHz. This comes from a 50MHZ crystal
> with a pre-divider of two, a post-divider of two and a PLL multiplier of
> 100. As the multiply is done first (50M * 100) this overflows a 32-bit
> value. The multiplication was done as a 64-bit multiply, but the result
> assigned to a 32-bit variable before the division was done. To fix this,
> use a 64-bit temporary variable and use the do_div() macro to perform a
> 64-bit/32-bit division.
>
> Fixes: 5fe225c105fd ("clk: iproc: add initial common clock support")
> Signed-off-by: Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz>
> ---
>  drivers/clk/bcm/clk-iproc-armpll.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clk/bcm/clk-iproc-armpll.c
> b/drivers/clk/bcm/clk-iproc-armpll.c
> index 9e86c0c10b57..665f1c531c4f 100644
> --- a/drivers/clk/bcm/clk-iproc-armpll.c
> +++ b/drivers/clk/bcm/clk-iproc-armpll.c
> @@ -180,7 +180,7 @@ static unsigned int __get_ndiv(struct iproc_arm_pll
> *pll)
>   *   mdiv = ARM PLL post divider
>   *
>   * The frequency is calculated by:
> - *   ((ndiv * parent clock rate) / pdiv) / mdiv
> + *   (ndiv * parent clock rate) / (pdiv * mdiv)
>   */
>  static unsigned long iproc_arm_pll_recalc_rate(struct clk_hw *hw,
>                 unsigned long parent_rate)
> @@ -189,6 +189,7 @@ static unsigned long iproc_arm_pll_recalc_rate(struct
> clk_hw *hw,
>         u32 val;
>         int mdiv;
>         u64 ndiv;
> +       u64 rate;
>         unsigned int pdiv;
>
>         /* in bypass mode, use parent rate */
> @@ -216,8 +217,9 @@ static unsigned long iproc_arm_pll_recalc_rate(struct
> clk_hw *hw,
>                 pll->rate = 0;
>                 return 0;
>         }
> -       pll->rate = (ndiv * parent_rate) >> 20;
>

50 MHz crystal * ndiv of 100 indeed > 32-bit, but the right shift by 20
would have ensured the result is way less than 32-bit.
I guess this is not a real world problem (you won't have a crystal as
reference clock of a PLL running at a frequency much higher than 50 MHz).
But just to be safe, using a 64-bit temporary variable for 'rate' before
further division below does not hurt.


> -       pll->rate = (pll->rate / pdiv) / mdiv;
> +       rate = (ndiv * parent_rate) >> 20;
> +       do_div(rate, pdiv * mdiv);
> +       pll->rate = rate;
>
>         pr_debug("%s: ARM PLL rate: %lu. parent rate: %lu\n", __func__,
>                  pll->rate, parent_rate);
> --
> 2.52.0
>
>
Acked-by: Ray Jui <ray.jui@broadcom.com>

Re: [PATCH] clk: bcm: iproc: Fix overflow in clock calculation

Posted by Mark Tomlinson 1 week ago

On Fri, 2026-01-30 at 08:52 -0800, Ray Jui wrote:
> 
> 
> On Wed, Jan 28, 2026 at 4:41 PM Mark Tomlinson
> <mark.tomlinson@alliedtelesis.co.nz> wrote:
> > Some BCM iproc devices run at 1.25GHz. This comes from a 50MHZ crystal
> > with a pre-divider of two, a post-divider of two and a PLL multiplier
> > of
> > 100. As the multiply is done first (50M * 100) this overflows a 32-bit
> > value. The multiplication was done as a 64-bit multiply, but the result
> > assigned to a 32-bit variable before the division was done. To fix
> > this,
> > use a 64-bit temporary variable and use the do_div() macro to perform a
> > 64-bit/32-bit division.
> > 
> > Fixes: 5fe225c105fd ("clk: iproc: add initial common clock support")
> > Signed-off-by: Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz>
> > ---
> >  drivers/clk/bcm/clk-iproc-armpll.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/clk/bcm/clk-iproc-armpll.c b/drivers/clk/bcm/clk-
> > iproc-armpll.c
> > index 9e86c0c10b57..665f1c531c4f 100644
> > --- a/drivers/clk/bcm/clk-iproc-armpll.c
> > +++ b/drivers/clk/bcm/clk-iproc-armpll.c
> > @@ -180,7 +180,7 @@ static unsigned int __get_ndiv(struct iproc_arm_pll
> > *pll)
> >   *   mdiv = ARM PLL post divider
> >   *
> >   * The frequency is calculated by:
> > - *   ((ndiv * parent clock rate) / pdiv) / mdiv
> > + *   (ndiv * parent clock rate) / (pdiv * mdiv)
> >   */
> >  static unsigned long iproc_arm_pll_recalc_rate(struct clk_hw *hw,
> >                 unsigned long parent_rate)
> > @@ -189,6 +189,7 @@ static unsigned long
> > iproc_arm_pll_recalc_rate(struct clk_hw *hw,
> >         u32 val;
> >         int mdiv;
> >         u64 ndiv;
> > +       u64 rate;
> >         unsigned int pdiv;
> > 
> >         /* in bypass mode, use parent rate */
> > @@ -216,8 +217,9 @@ static unsigned long
> > iproc_arm_pll_recalc_rate(struct clk_hw *hw,
> >                 pll->rate = 0;
> >                 return 0;
> >         }
> > -       pll->rate = (ndiv * parent_rate) >> 20;
> > 
> 
> 
> 50 MHz crystal * ndiv of 100 indeed > 32-bit, but the right shift by 20
> would have ensured the result is way less than 32-bit.
> I guess this is not a real world problem (you won't have a crystal as
> reference clock of a PLL running at a frequency much higher than 50 MHz).
> But just to be safe, using a 64-bit temporary variable for 'rate' before
> further division below does not hurt.

My description was a bit simplified. ndiv at this point is shifted left by
20 bits, so is 100 << 20, not just 100. So the calculation after the >> 20
was still 5 billion. This is a real problem on the BCM56160, which uses
these values.

Thanks for the Ack.