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

[Ray Jui]

On Wed, Jan 28, 2026 at 4:41 PM Mark Tomlinson <mark.tomlinson@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx>
---
 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@xxxxxxxxxxxx> 

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature