* [PATCH v2 0/2] clk: meson: MPLL fixes for Meson8b @ 2017-04-07 15:34 ` Jerome Brunet 0 siblings, 0 replies; 15+ messages in thread From: Jerome Brunet @ 2017-04-07 15:34 UTC (permalink / raw) To: Martin Blumenstingl, Neil Armstrong, Kevin Hilman, Michael Turquette, Stephen Boyd Cc: Jerome Brunet, linux-amlogic, linux-clk, linux-arm-kernel, carlo MPLL clocks have been recently added to the Meson8b clock driver: [0] On meson8b boards this unfortunately causes a division by zero error which is fixed by patch #1 in this series. While investigating this I found that there also seems to be a 32bit overflow in the calculation in rate_from_params(), which is fixed by patch #2 in this series. Changes since v1: [1] - Return an error code when the mpll parameters are out of the specified range. - As agreed with martin, use DIV_ROUND_UP_ULL instead of mul_u64_u32_div. [0] http://lists.infradead.org/pipermail/linux-amlogic/2017-March/002757.html [1] https://lkml.kernel.org/r/20170401130225.8811-1-martin.blumenstingl@googlemail.com Martin Blumenstingl (2): clk: meson: mpll: fix division by zero in rate_from_params clk: meson: mpll: use 64bit math in rate_from_params drivers/clk/meson/clk-mpll.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) -- 2.9.3 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 0/2] clk: meson: MPLL fixes for Meson8b @ 2017-04-07 15:34 ` Jerome Brunet 0 siblings, 0 replies; 15+ messages in thread From: Jerome Brunet @ 2017-04-07 15:34 UTC (permalink / raw) To: linus-amlogic MPLL clocks have been recently added to the Meson8b clock driver: [0] On meson8b boards this unfortunately causes a division by zero error which is fixed by patch #1 in this series. While investigating this I found that there also seems to be a 32bit overflow in the calculation in rate_from_params(), which is fixed by patch #2 in this series. Changes since v1: [1] - Return an error code when the mpll parameters are out of the specified range. - As agreed with martin, use DIV_ROUND_UP_ULL instead of mul_u64_u32_div. [0] http://lists.infradead.org/pipermail/linux-amlogic/2017-March/002757.html [1] https://lkml.kernel.org/r/20170401130225.8811-1-martin.blumenstingl at googlemail.com Martin Blumenstingl (2): clk: meson: mpll: fix division by zero in rate_from_params clk: meson: mpll: use 64bit math in rate_from_params drivers/clk/meson/clk-mpll.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) -- 2.9.3 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 0/2] clk: meson: MPLL fixes for Meson8b @ 2017-04-07 15:34 ` Jerome Brunet 0 siblings, 0 replies; 15+ messages in thread From: Jerome Brunet @ 2017-04-07 15:34 UTC (permalink / raw) To: linux-arm-kernel MPLL clocks have been recently added to the Meson8b clock driver: [0] On meson8b boards this unfortunately causes a division by zero error which is fixed by patch #1 in this series. While investigating this I found that there also seems to be a 32bit overflow in the calculation in rate_from_params(), which is fixed by patch #2 in this series. Changes since v1: [1] - Return an error code when the mpll parameters are out of the specified range. - As agreed with martin, use DIV_ROUND_UP_ULL instead of mul_u64_u32_div. [0] http://lists.infradead.org/pipermail/linux-amlogic/2017-March/002757.html [1] https://lkml.kernel.org/r/20170401130225.8811-1-martin.blumenstingl at googlemail.com Martin Blumenstingl (2): clk: meson: mpll: fix division by zero in rate_from_params clk: meson: mpll: use 64bit math in rate_from_params drivers/clk/meson/clk-mpll.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) -- 2.9.3 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 1/2] clk: meson: mpll: fix division by zero in rate_from_params 2017-04-07 15:34 ` Jerome Brunet (?) @ 2017-04-07 15:34 ` Jerome Brunet -1 siblings, 0 replies; 15+ messages in thread From: Jerome Brunet @ 2017-04-07 15:34 UTC (permalink / raw) To: Martin Blumenstingl, Neil Armstrong, Kevin Hilman, Michael Turquette, Stephen Boyd Cc: linux-amlogic, linux-clk, linux-arm-kernel, carlo, Jerome Brunet From: Martin Blumenstingl <martin.blumenstingl@googlemail.com> According to the public datasheet all register bits in HHI_MPLL_CNTL7, HHI_MPLL_CNTL8 and HHI_MPLL_CNTL9 default to zero. On all GX SoCs these seem to be initialized by the bootloader to some default value. However, on my Meson8 board they are not initialized, leading to a division by zero in rate_from_params as the math is: (parent_rate * SDM_DEN) / ((SDM_DEN * 0) + 0) According to the datasheet, the minimum n2 value is 4. The rate provided by the clock when n2 is less than this minimum is unpredictable. In such case, we report an error. Although the rate_from_params function was only introduced recently the original bug has been there for much longer. It was only exposed recently when the MPLL clocks were added to the Meson8b clock driver. Fixes: 1c50da4f27 ("clk: meson: add mpll support") Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> --- drivers/clk/meson/clk-mpll.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/drivers/clk/meson/clk-mpll.c b/drivers/clk/meson/clk-mpll.c index 540dabe5adad..d9462b505dcc 100644 --- a/drivers/clk/meson/clk-mpll.c +++ b/drivers/clk/meson/clk-mpll.c @@ -65,18 +65,21 @@ #include "clkc.h" #define SDM_DEN 16384 -#define SDM_MIN 1 -#define SDM_MAX 16383 #define N2_MIN 4 #define N2_MAX 511 #define to_meson_clk_mpll(_hw) container_of(_hw, struct meson_clk_mpll, hw) -static unsigned long rate_from_params(unsigned long parent_rate, +static long rate_from_params(unsigned long parent_rate, unsigned long sdm, unsigned long n2) { - return (parent_rate * SDM_DEN) / ((SDM_DEN * n2) + sdm); + unsigned long divisor = (SDM_DEN * n2) + sdm; + + if (n2 < N2_MIN) + return -EINVAL; + + return (parent_rate * SDM_DEN) / divisor; } static void params_from_rate(unsigned long requested_rate, @@ -89,17 +92,13 @@ static void params_from_rate(unsigned long requested_rate, if (div < N2_MIN) { *n2 = N2_MIN; - *sdm = SDM_MIN; + *sdm = 0; } else if (div > N2_MAX) { *n2 = N2_MAX; - *sdm = SDM_MAX; + *sdm = SDM_DEN - 1; } else { *n2 = div; *sdm = DIV_ROUND_UP(rem * SDM_DEN, requested_rate); - if (*sdm < SDM_MIN) - *sdm = SDM_MIN; - else if (*sdm > SDM_MAX) - *sdm = SDM_MAX; } } @@ -109,6 +108,7 @@ static unsigned long mpll_recalc_rate(struct clk_hw *hw, struct meson_clk_mpll *mpll = to_meson_clk_mpll(hw); struct parm *p; unsigned long reg, sdm, n2; + long rate; p = &mpll->sdm; reg = readl(mpll->base + p->reg_off); @@ -118,7 +118,11 @@ static unsigned long mpll_recalc_rate(struct clk_hw *hw, reg = readl(mpll->base + p->reg_off); n2 = PARM_GET(p->width, p->shift, reg); - return rate_from_params(parent_rate, sdm, n2); + rate = rate_from_params(parent_rate, sdm, n2); + if (rate < 0) + return 0; + + return rate; } static long mpll_round_rate(struct clk_hw *hw, -- 2.9.3 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 1/2] clk: meson: mpll: fix division by zero in rate_from_params @ 2017-04-07 15:34 ` Jerome Brunet 0 siblings, 0 replies; 15+ messages in thread From: Jerome Brunet @ 2017-04-07 15:34 UTC (permalink / raw) To: linus-amlogic From: Martin Blumenstingl <martin.blumenstingl@googlemail.com> According to the public datasheet all register bits in HHI_MPLL_CNTL7, HHI_MPLL_CNTL8 and HHI_MPLL_CNTL9 default to zero. On all GX SoCs these seem to be initialized by the bootloader to some default value. However, on my Meson8 board they are not initialized, leading to a division by zero in rate_from_params as the math is: (parent_rate * SDM_DEN) / ((SDM_DEN * 0) + 0) According to the datasheet, the minimum n2 value is 4. The rate provided by the clock when n2 is less than this minimum is unpredictable. In such case, we report an error. Although the rate_from_params function was only introduced recently the original bug has been there for much longer. It was only exposed recently when the MPLL clocks were added to the Meson8b clock driver. Fixes: 1c50da4f27 ("clk: meson: add mpll support") Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> --- drivers/clk/meson/clk-mpll.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/drivers/clk/meson/clk-mpll.c b/drivers/clk/meson/clk-mpll.c index 540dabe5adad..d9462b505dcc 100644 --- a/drivers/clk/meson/clk-mpll.c +++ b/drivers/clk/meson/clk-mpll.c @@ -65,18 +65,21 @@ #include "clkc.h" #define SDM_DEN 16384 -#define SDM_MIN 1 -#define SDM_MAX 16383 #define N2_MIN 4 #define N2_MAX 511 #define to_meson_clk_mpll(_hw) container_of(_hw, struct meson_clk_mpll, hw) -static unsigned long rate_from_params(unsigned long parent_rate, +static long rate_from_params(unsigned long parent_rate, unsigned long sdm, unsigned long n2) { - return (parent_rate * SDM_DEN) / ((SDM_DEN * n2) + sdm); + unsigned long divisor = (SDM_DEN * n2) + sdm; + + if (n2 < N2_MIN) + return -EINVAL; + + return (parent_rate * SDM_DEN) / divisor; } static void params_from_rate(unsigned long requested_rate, @@ -89,17 +92,13 @@ static void params_from_rate(unsigned long requested_rate, if (div < N2_MIN) { *n2 = N2_MIN; - *sdm = SDM_MIN; + *sdm = 0; } else if (div > N2_MAX) { *n2 = N2_MAX; - *sdm = SDM_MAX; + *sdm = SDM_DEN - 1; } else { *n2 = div; *sdm = DIV_ROUND_UP(rem * SDM_DEN, requested_rate); - if (*sdm < SDM_MIN) - *sdm = SDM_MIN; - else if (*sdm > SDM_MAX) - *sdm = SDM_MAX; } } @@ -109,6 +108,7 @@ static unsigned long mpll_recalc_rate(struct clk_hw *hw, struct meson_clk_mpll *mpll = to_meson_clk_mpll(hw); struct parm *p; unsigned long reg, sdm, n2; + long rate; p = &mpll->sdm; reg = readl(mpll->base + p->reg_off); @@ -118,7 +118,11 @@ static unsigned long mpll_recalc_rate(struct clk_hw *hw, reg = readl(mpll->base + p->reg_off); n2 = PARM_GET(p->width, p->shift, reg); - return rate_from_params(parent_rate, sdm, n2); + rate = rate_from_params(parent_rate, sdm, n2); + if (rate < 0) + return 0; + + return rate; } static long mpll_round_rate(struct clk_hw *hw, -- 2.9.3 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 1/2] clk: meson: mpll: fix division by zero in rate_from_params @ 2017-04-07 15:34 ` Jerome Brunet 0 siblings, 0 replies; 15+ messages in thread From: Jerome Brunet @ 2017-04-07 15:34 UTC (permalink / raw) To: linux-arm-kernel From: Martin Blumenstingl <martin.blumenstingl@googlemail.com> According to the public datasheet all register bits in HHI_MPLL_CNTL7, HHI_MPLL_CNTL8 and HHI_MPLL_CNTL9 default to zero. On all GX SoCs these seem to be initialized by the bootloader to some default value. However, on my Meson8 board they are not initialized, leading to a division by zero in rate_from_params as the math is: (parent_rate * SDM_DEN) / ((SDM_DEN * 0) + 0) According to the datasheet, the minimum n2 value is 4. The rate provided by the clock when n2 is less than this minimum is unpredictable. In such case, we report an error. Although the rate_from_params function was only introduced recently the original bug has been there for much longer. It was only exposed recently when the MPLL clocks were added to the Meson8b clock driver. Fixes: 1c50da4f27 ("clk: meson: add mpll support") Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> --- drivers/clk/meson/clk-mpll.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/drivers/clk/meson/clk-mpll.c b/drivers/clk/meson/clk-mpll.c index 540dabe5adad..d9462b505dcc 100644 --- a/drivers/clk/meson/clk-mpll.c +++ b/drivers/clk/meson/clk-mpll.c @@ -65,18 +65,21 @@ #include "clkc.h" #define SDM_DEN 16384 -#define SDM_MIN 1 -#define SDM_MAX 16383 #define N2_MIN 4 #define N2_MAX 511 #define to_meson_clk_mpll(_hw) container_of(_hw, struct meson_clk_mpll, hw) -static unsigned long rate_from_params(unsigned long parent_rate, +static long rate_from_params(unsigned long parent_rate, unsigned long sdm, unsigned long n2) { - return (parent_rate * SDM_DEN) / ((SDM_DEN * n2) + sdm); + unsigned long divisor = (SDM_DEN * n2) + sdm; + + if (n2 < N2_MIN) + return -EINVAL; + + return (parent_rate * SDM_DEN) / divisor; } static void params_from_rate(unsigned long requested_rate, @@ -89,17 +92,13 @@ static void params_from_rate(unsigned long requested_rate, if (div < N2_MIN) { *n2 = N2_MIN; - *sdm = SDM_MIN; + *sdm = 0; } else if (div > N2_MAX) { *n2 = N2_MAX; - *sdm = SDM_MAX; + *sdm = SDM_DEN - 1; } else { *n2 = div; *sdm = DIV_ROUND_UP(rem * SDM_DEN, requested_rate); - if (*sdm < SDM_MIN) - *sdm = SDM_MIN; - else if (*sdm > SDM_MAX) - *sdm = SDM_MAX; } } @@ -109,6 +108,7 @@ static unsigned long mpll_recalc_rate(struct clk_hw *hw, struct meson_clk_mpll *mpll = to_meson_clk_mpll(hw); struct parm *p; unsigned long reg, sdm, n2; + long rate; p = &mpll->sdm; reg = readl(mpll->base + p->reg_off); @@ -118,7 +118,11 @@ static unsigned long mpll_recalc_rate(struct clk_hw *hw, reg = readl(mpll->base + p->reg_off); n2 = PARM_GET(p->width, p->shift, reg); - return rate_from_params(parent_rate, sdm, n2); + rate = rate_from_params(parent_rate, sdm, n2); + if (rate < 0) + return 0; + + return rate; } static long mpll_round_rate(struct clk_hw *hw, -- 2.9.3 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] clk: meson: mpll: fix division by zero in rate_from_params 2017-04-07 15:34 ` Jerome Brunet (?) @ 2017-04-07 15:39 ` Neil Armstrong -1 siblings, 0 replies; 15+ messages in thread From: Neil Armstrong @ 2017-04-07 15:39 UTC (permalink / raw) To: Jerome Brunet, Martin Blumenstingl, Kevin Hilman, Michael Turquette, Stephen Boyd Cc: linux-amlogic, linux-clk, linux-arm-kernel, carlo On 04/07/2017 05:34 PM, Jerome Brunet wrote: > From: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > According to the public datasheet all register bits in HHI_MPLL_CNTL7, > HHI_MPLL_CNTL8 and HHI_MPLL_CNTL9 default to zero. On all GX SoCs these > seem to be initialized by the bootloader to some default value. > However, on my Meson8 board they are not initialized, leading to a > division by zero in rate_from_params as the math is: > (parent_rate * SDM_DEN) / ((SDM_DEN * 0) + 0) > > According to the datasheet, the minimum n2 value is 4. The rate provided > by the clock when n2 is less than this minimum is unpredictable. In such > case, we report an error. > > Although the rate_from_params function was only introduced recently the > original bug has been there for much longer. It was only exposed > recently when the MPLL clocks were added to the Meson8b clock driver. > > Fixes: 1c50da4f27 ("clk: meson: add mpll support") > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > --- > drivers/clk/meson/clk-mpll.c | 26 +++++++++++++++----------- > 1 file changed, 15 insertions(+), 11 deletions(-) > > diff --git a/drivers/clk/meson/clk-mpll.c b/drivers/clk/meson/clk-mpll.c > index 540dabe5adad..d9462b505dcc 100644 > --- a/drivers/clk/meson/clk-mpll.c > +++ b/drivers/clk/meson/clk-mpll.c > @@ -65,18 +65,21 @@ > #include "clkc.h" > > #define SDM_DEN 16384 > -#define SDM_MIN 1 > -#define SDM_MAX 16383 > #define N2_MIN 4 > #define N2_MAX 511 > > #define to_meson_clk_mpll(_hw) container_of(_hw, struct meson_clk_mpll, hw) > > -static unsigned long rate_from_params(unsigned long parent_rate, > +static long rate_from_params(unsigned long parent_rate, > unsigned long sdm, > unsigned long n2) > { > - return (parent_rate * SDM_DEN) / ((SDM_DEN * n2) + sdm); > + unsigned long divisor = (SDM_DEN * n2) + sdm; > + > + if (n2 < N2_MIN) > + return -EINVAL; > + > + return (parent_rate * SDM_DEN) / divisor; > } > > static void params_from_rate(unsigned long requested_rate, > @@ -89,17 +92,13 @@ static void params_from_rate(unsigned long requested_rate, > > if (div < N2_MIN) { > *n2 = N2_MIN; > - *sdm = SDM_MIN; > + *sdm = 0; > } else if (div > N2_MAX) { > *n2 = N2_MAX; > - *sdm = SDM_MAX; > + *sdm = SDM_DEN - 1; > } else { > *n2 = div; > *sdm = DIV_ROUND_UP(rem * SDM_DEN, requested_rate); > - if (*sdm < SDM_MIN) > - *sdm = SDM_MIN; > - else if (*sdm > SDM_MAX) > - *sdm = SDM_MAX; > } > } > > @@ -109,6 +108,7 @@ static unsigned long mpll_recalc_rate(struct clk_hw *hw, > struct meson_clk_mpll *mpll = to_meson_clk_mpll(hw); > struct parm *p; > unsigned long reg, sdm, n2; > + long rate; > > p = &mpll->sdm; > reg = readl(mpll->base + p->reg_off); > @@ -118,7 +118,11 @@ static unsigned long mpll_recalc_rate(struct clk_hw *hw, > reg = readl(mpll->base + p->reg_off); > n2 = PARM_GET(p->width, p->shift, reg); > > - return rate_from_params(parent_rate, sdm, n2); > + rate = rate_from_params(parent_rate, sdm, n2); > + if (rate < 0) > + return 0; > + > + return rate; > } > > static long mpll_round_rate(struct clk_hw *hw, > Reviewed-by: Neil Armstrong <narmstrong@baylibre.com> ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 1/2] clk: meson: mpll: fix division by zero in rate_from_params @ 2017-04-07 15:39 ` Neil Armstrong 0 siblings, 0 replies; 15+ messages in thread From: Neil Armstrong @ 2017-04-07 15:39 UTC (permalink / raw) To: linus-amlogic On 04/07/2017 05:34 PM, Jerome Brunet wrote: > From: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > According to the public datasheet all register bits in HHI_MPLL_CNTL7, > HHI_MPLL_CNTL8 and HHI_MPLL_CNTL9 default to zero. On all GX SoCs these > seem to be initialized by the bootloader to some default value. > However, on my Meson8 board they are not initialized, leading to a > division by zero in rate_from_params as the math is: > (parent_rate * SDM_DEN) / ((SDM_DEN * 0) + 0) > > According to the datasheet, the minimum n2 value is 4. The rate provided > by the clock when n2 is less than this minimum is unpredictable. In such > case, we report an error. > > Although the rate_from_params function was only introduced recently the > original bug has been there for much longer. It was only exposed > recently when the MPLL clocks were added to the Meson8b clock driver. > > Fixes: 1c50da4f27 ("clk: meson: add mpll support") > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > --- > drivers/clk/meson/clk-mpll.c | 26 +++++++++++++++----------- > 1 file changed, 15 insertions(+), 11 deletions(-) > > diff --git a/drivers/clk/meson/clk-mpll.c b/drivers/clk/meson/clk-mpll.c > index 540dabe5adad..d9462b505dcc 100644 > --- a/drivers/clk/meson/clk-mpll.c > +++ b/drivers/clk/meson/clk-mpll.c > @@ -65,18 +65,21 @@ > #include "clkc.h" > > #define SDM_DEN 16384 > -#define SDM_MIN 1 > -#define SDM_MAX 16383 > #define N2_MIN 4 > #define N2_MAX 511 > > #define to_meson_clk_mpll(_hw) container_of(_hw, struct meson_clk_mpll, hw) > > -static unsigned long rate_from_params(unsigned long parent_rate, > +static long rate_from_params(unsigned long parent_rate, > unsigned long sdm, > unsigned long n2) > { > - return (parent_rate * SDM_DEN) / ((SDM_DEN * n2) + sdm); > + unsigned long divisor = (SDM_DEN * n2) + sdm; > + > + if (n2 < N2_MIN) > + return -EINVAL; > + > + return (parent_rate * SDM_DEN) / divisor; > } > > static void params_from_rate(unsigned long requested_rate, > @@ -89,17 +92,13 @@ static void params_from_rate(unsigned long requested_rate, > > if (div < N2_MIN) { > *n2 = N2_MIN; > - *sdm = SDM_MIN; > + *sdm = 0; > } else if (div > N2_MAX) { > *n2 = N2_MAX; > - *sdm = SDM_MAX; > + *sdm = SDM_DEN - 1; > } else { > *n2 = div; > *sdm = DIV_ROUND_UP(rem * SDM_DEN, requested_rate); > - if (*sdm < SDM_MIN) > - *sdm = SDM_MIN; > - else if (*sdm > SDM_MAX) > - *sdm = SDM_MAX; > } > } > > @@ -109,6 +108,7 @@ static unsigned long mpll_recalc_rate(struct clk_hw *hw, > struct meson_clk_mpll *mpll = to_meson_clk_mpll(hw); > struct parm *p; > unsigned long reg, sdm, n2; > + long rate; > > p = &mpll->sdm; > reg = readl(mpll->base + p->reg_off); > @@ -118,7 +118,11 @@ static unsigned long mpll_recalc_rate(struct clk_hw *hw, > reg = readl(mpll->base + p->reg_off); > n2 = PARM_GET(p->width, p->shift, reg); > > - return rate_from_params(parent_rate, sdm, n2); > + rate = rate_from_params(parent_rate, sdm, n2); > + if (rate < 0) > + return 0; > + > + return rate; > } > > static long mpll_round_rate(struct clk_hw *hw, > Reviewed-by: Neil Armstrong <narmstrong@baylibre.com> ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 1/2] clk: meson: mpll: fix division by zero in rate_from_params @ 2017-04-07 15:39 ` Neil Armstrong 0 siblings, 0 replies; 15+ messages in thread From: Neil Armstrong @ 2017-04-07 15:39 UTC (permalink / raw) To: linux-arm-kernel On 04/07/2017 05:34 PM, Jerome Brunet wrote: > From: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > According to the public datasheet all register bits in HHI_MPLL_CNTL7, > HHI_MPLL_CNTL8 and HHI_MPLL_CNTL9 default to zero. On all GX SoCs these > seem to be initialized by the bootloader to some default value. > However, on my Meson8 board they are not initialized, leading to a > division by zero in rate_from_params as the math is: > (parent_rate * SDM_DEN) / ((SDM_DEN * 0) + 0) > > According to the datasheet, the minimum n2 value is 4. The rate provided > by the clock when n2 is less than this minimum is unpredictable. In such > case, we report an error. > > Although the rate_from_params function was only introduced recently the > original bug has been there for much longer. It was only exposed > recently when the MPLL clocks were added to the Meson8b clock driver. > > Fixes: 1c50da4f27 ("clk: meson: add mpll support") > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > --- > drivers/clk/meson/clk-mpll.c | 26 +++++++++++++++----------- > 1 file changed, 15 insertions(+), 11 deletions(-) > > diff --git a/drivers/clk/meson/clk-mpll.c b/drivers/clk/meson/clk-mpll.c > index 540dabe5adad..d9462b505dcc 100644 > --- a/drivers/clk/meson/clk-mpll.c > +++ b/drivers/clk/meson/clk-mpll.c > @@ -65,18 +65,21 @@ > #include "clkc.h" > > #define SDM_DEN 16384 > -#define SDM_MIN 1 > -#define SDM_MAX 16383 > #define N2_MIN 4 > #define N2_MAX 511 > > #define to_meson_clk_mpll(_hw) container_of(_hw, struct meson_clk_mpll, hw) > > -static unsigned long rate_from_params(unsigned long parent_rate, > +static long rate_from_params(unsigned long parent_rate, > unsigned long sdm, > unsigned long n2) > { > - return (parent_rate * SDM_DEN) / ((SDM_DEN * n2) + sdm); > + unsigned long divisor = (SDM_DEN * n2) + sdm; > + > + if (n2 < N2_MIN) > + return -EINVAL; > + > + return (parent_rate * SDM_DEN) / divisor; > } > > static void params_from_rate(unsigned long requested_rate, > @@ -89,17 +92,13 @@ static void params_from_rate(unsigned long requested_rate, > > if (div < N2_MIN) { > *n2 = N2_MIN; > - *sdm = SDM_MIN; > + *sdm = 0; > } else if (div > N2_MAX) { > *n2 = N2_MAX; > - *sdm = SDM_MAX; > + *sdm = SDM_DEN - 1; > } else { > *n2 = div; > *sdm = DIV_ROUND_UP(rem * SDM_DEN, requested_rate); > - if (*sdm < SDM_MIN) > - *sdm = SDM_MIN; > - else if (*sdm > SDM_MAX) > - *sdm = SDM_MAX; > } > } > > @@ -109,6 +108,7 @@ static unsigned long mpll_recalc_rate(struct clk_hw *hw, > struct meson_clk_mpll *mpll = to_meson_clk_mpll(hw); > struct parm *p; > unsigned long reg, sdm, n2; > + long rate; > > p = &mpll->sdm; > reg = readl(mpll->base + p->reg_off); > @@ -118,7 +118,11 @@ static unsigned long mpll_recalc_rate(struct clk_hw *hw, > reg = readl(mpll->base + p->reg_off); > n2 = PARM_GET(p->width, p->shift, reg); > > - return rate_from_params(parent_rate, sdm, n2); > + rate = rate_from_params(parent_rate, sdm, n2); > + if (rate < 0) > + return 0; > + > + return rate; > } > > static long mpll_round_rate(struct clk_hw *hw, > Reviewed-by: Neil Armstrong <narmstrong@baylibre.com> ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 2/2] clk: meson: mpll: use 64bit math in rate_from_params 2017-04-07 15:34 ` Jerome Brunet (?) @ 2017-04-07 15:34 ` Jerome Brunet -1 siblings, 0 replies; 15+ messages in thread From: Jerome Brunet @ 2017-04-07 15:34 UTC (permalink / raw) To: Martin Blumenstingl, Neil Armstrong, Kevin Hilman, Michael Turquette, Stephen Boyd Cc: linux-amlogic, linux-clk, linux-arm-kernel, carlo, Jerome Brunet From: Martin Blumenstingl <martin.blumenstingl@googlemail.com> On Meson8b the MPLL parent clock (fixed_pll) has a rate of 2550MHz. Multiplying this with SDM_DEN results in a value greater than 32bits. This is not a problem on the 64bit Meson GX SoCs, but it may result in undefined behavior on the older 32bit Meson8b SoC. While rate_from_params was only introduced recently to make the math reusable from _round_rate and _recalc_rate the original bug exists much longer. Fixes: 1c50da4f27 ("clk: meson: add mpll support") Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> [as discussed on the ml, use DIV_ROUND_UP_ULL] Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> --- drivers/clk/meson/clk-mpll.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/clk/meson/clk-mpll.c b/drivers/clk/meson/clk-mpll.c index d9462b505dcc..39eab69fe51a 100644 --- a/drivers/clk/meson/clk-mpll.c +++ b/drivers/clk/meson/clk-mpll.c @@ -79,7 +79,7 @@ static long rate_from_params(unsigned long parent_rate, if (n2 < N2_MIN) return -EINVAL; - return (parent_rate * SDM_DEN) / divisor; + return DIV_ROUND_UP_ULL((u64)parent_rate * SDM_DEN, divisor); } static void params_from_rate(unsigned long requested_rate, -- 2.9.3 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 2/2] clk: meson: mpll: use 64bit math in rate_from_params @ 2017-04-07 15:34 ` Jerome Brunet 0 siblings, 0 replies; 15+ messages in thread From: Jerome Brunet @ 2017-04-07 15:34 UTC (permalink / raw) To: linus-amlogic From: Martin Blumenstingl <martin.blumenstingl@googlemail.com> On Meson8b the MPLL parent clock (fixed_pll) has a rate of 2550MHz. Multiplying this with SDM_DEN results in a value greater than 32bits. This is not a problem on the 64bit Meson GX SoCs, but it may result in undefined behavior on the older 32bit Meson8b SoC. While rate_from_params was only introduced recently to make the math reusable from _round_rate and _recalc_rate the original bug exists much longer. Fixes: 1c50da4f27 ("clk: meson: add mpll support") Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> [as discussed on the ml, use DIV_ROUND_UP_ULL] Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> --- drivers/clk/meson/clk-mpll.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/clk/meson/clk-mpll.c b/drivers/clk/meson/clk-mpll.c index d9462b505dcc..39eab69fe51a 100644 --- a/drivers/clk/meson/clk-mpll.c +++ b/drivers/clk/meson/clk-mpll.c @@ -79,7 +79,7 @@ static long rate_from_params(unsigned long parent_rate, if (n2 < N2_MIN) return -EINVAL; - return (parent_rate * SDM_DEN) / divisor; + return DIV_ROUND_UP_ULL((u64)parent_rate * SDM_DEN, divisor); } static void params_from_rate(unsigned long requested_rate, -- 2.9.3 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 2/2] clk: meson: mpll: use 64bit math in rate_from_params @ 2017-04-07 15:34 ` Jerome Brunet 0 siblings, 0 replies; 15+ messages in thread From: Jerome Brunet @ 2017-04-07 15:34 UTC (permalink / raw) To: linux-arm-kernel From: Martin Blumenstingl <martin.blumenstingl@googlemail.com> On Meson8b the MPLL parent clock (fixed_pll) has a rate of 2550MHz. Multiplying this with SDM_DEN results in a value greater than 32bits. This is not a problem on the 64bit Meson GX SoCs, but it may result in undefined behavior on the older 32bit Meson8b SoC. While rate_from_params was only introduced recently to make the math reusable from _round_rate and _recalc_rate the original bug exists much longer. Fixes: 1c50da4f27 ("clk: meson: add mpll support") Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> [as discussed on the ml, use DIV_ROUND_UP_ULL] Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> --- drivers/clk/meson/clk-mpll.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/clk/meson/clk-mpll.c b/drivers/clk/meson/clk-mpll.c index d9462b505dcc..39eab69fe51a 100644 --- a/drivers/clk/meson/clk-mpll.c +++ b/drivers/clk/meson/clk-mpll.c @@ -79,7 +79,7 @@ static long rate_from_params(unsigned long parent_rate, if (n2 < N2_MIN) return -EINVAL; - return (parent_rate * SDM_DEN) / divisor; + return DIV_ROUND_UP_ULL((u64)parent_rate * SDM_DEN, divisor); } static void params_from_rate(unsigned long requested_rate, -- 2.9.3 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/2] clk: meson: mpll: use 64bit math in rate_from_params 2017-04-07 15:34 ` Jerome Brunet (?) @ 2017-04-07 15:39 ` Neil Armstrong -1 siblings, 0 replies; 15+ messages in thread From: Neil Armstrong @ 2017-04-07 15:39 UTC (permalink / raw) To: Jerome Brunet, Martin Blumenstingl, Kevin Hilman, Michael Turquette, Stephen Boyd Cc: linux-amlogic, linux-clk, linux-arm-kernel, carlo On 04/07/2017 05:34 PM, Jerome Brunet wrote: > From: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > On Meson8b the MPLL parent clock (fixed_pll) has a rate of 2550MHz. > Multiplying this with SDM_DEN results in a value greater than 32bits. > This is not a problem on the 64bit Meson GX SoCs, but it may result in > undefined behavior on the older 32bit Meson8b SoC. > > While rate_from_params was only introduced recently to make the math > reusable from _round_rate and _recalc_rate the original bug exists much > longer. > > Fixes: 1c50da4f27 ("clk: meson: add mpll support") > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > [as discussed on the ml, use DIV_ROUND_UP_ULL] > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > --- > drivers/clk/meson/clk-mpll.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/clk/meson/clk-mpll.c b/drivers/clk/meson/clk-mpll.c > index d9462b505dcc..39eab69fe51a 100644 > --- a/drivers/clk/meson/clk-mpll.c > +++ b/drivers/clk/meson/clk-mpll.c > @@ -79,7 +79,7 @@ static long rate_from_params(unsigned long parent_rate, > if (n2 < N2_MIN) > return -EINVAL; > > - return (parent_rate * SDM_DEN) / divisor; > + return DIV_ROUND_UP_ULL((u64)parent_rate * SDM_DEN, divisor); > } > > static void params_from_rate(unsigned long requested_rate, > Reviewed-by: Neil Armstrong <narmstrong@baylibre.com> ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 2/2] clk: meson: mpll: use 64bit math in rate_from_params @ 2017-04-07 15:39 ` Neil Armstrong 0 siblings, 0 replies; 15+ messages in thread From: Neil Armstrong @ 2017-04-07 15:39 UTC (permalink / raw) To: linus-amlogic On 04/07/2017 05:34 PM, Jerome Brunet wrote: > From: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > On Meson8b the MPLL parent clock (fixed_pll) has a rate of 2550MHz. > Multiplying this with SDM_DEN results in a value greater than 32bits. > This is not a problem on the 64bit Meson GX SoCs, but it may result in > undefined behavior on the older 32bit Meson8b SoC. > > While rate_from_params was only introduced recently to make the math > reusable from _round_rate and _recalc_rate the original bug exists much > longer. > > Fixes: 1c50da4f27 ("clk: meson: add mpll support") > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > [as discussed on the ml, use DIV_ROUND_UP_ULL] > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > --- > drivers/clk/meson/clk-mpll.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/clk/meson/clk-mpll.c b/drivers/clk/meson/clk-mpll.c > index d9462b505dcc..39eab69fe51a 100644 > --- a/drivers/clk/meson/clk-mpll.c > +++ b/drivers/clk/meson/clk-mpll.c > @@ -79,7 +79,7 @@ static long rate_from_params(unsigned long parent_rate, > if (n2 < N2_MIN) > return -EINVAL; > > - return (parent_rate * SDM_DEN) / divisor; > + return DIV_ROUND_UP_ULL((u64)parent_rate * SDM_DEN, divisor); > } > > static void params_from_rate(unsigned long requested_rate, > Reviewed-by: Neil Armstrong <narmstrong@baylibre.com> ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 2/2] clk: meson: mpll: use 64bit math in rate_from_params @ 2017-04-07 15:39 ` Neil Armstrong 0 siblings, 0 replies; 15+ messages in thread From: Neil Armstrong @ 2017-04-07 15:39 UTC (permalink / raw) To: linux-arm-kernel On 04/07/2017 05:34 PM, Jerome Brunet wrote: > From: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > On Meson8b the MPLL parent clock (fixed_pll) has a rate of 2550MHz. > Multiplying this with SDM_DEN results in a value greater than 32bits. > This is not a problem on the 64bit Meson GX SoCs, but it may result in > undefined behavior on the older 32bit Meson8b SoC. > > While rate_from_params was only introduced recently to make the math > reusable from _round_rate and _recalc_rate the original bug exists much > longer. > > Fixes: 1c50da4f27 ("clk: meson: add mpll support") > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > [as discussed on the ml, use DIV_ROUND_UP_ULL] > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > --- > drivers/clk/meson/clk-mpll.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/clk/meson/clk-mpll.c b/drivers/clk/meson/clk-mpll.c > index d9462b505dcc..39eab69fe51a 100644 > --- a/drivers/clk/meson/clk-mpll.c > +++ b/drivers/clk/meson/clk-mpll.c > @@ -79,7 +79,7 @@ static long rate_from_params(unsigned long parent_rate, > if (n2 < N2_MIN) > return -EINVAL; > > - return (parent_rate * SDM_DEN) / divisor; > + return DIV_ROUND_UP_ULL((u64)parent_rate * SDM_DEN, divisor); > } > > static void params_from_rate(unsigned long requested_rate, > Reviewed-by: Neil Armstrong <narmstrong@baylibre.com> ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2017-04-07 15:39 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-07 15:34 [PATCH v2 0/2] clk: meson: MPLL fixes for Meson8b Jerome Brunet 2017-04-07 15:34 ` Jerome Brunet 2017-04-07 15:34 ` Jerome Brunet 2017-04-07 15:34 ` [PATCH v2 1/2] clk: meson: mpll: fix division by zero in rate_from_params Jerome Brunet 2017-04-07 15:34 ` Jerome Brunet 2017-04-07 15:34 ` Jerome Brunet 2017-04-07 15:39 ` Neil Armstrong 2017-04-07 15:39 ` Neil Armstrong 2017-04-07 15:39 ` Neil Armstrong 2017-04-07 15:34 ` [PATCH v2 2/2] clk: meson: mpll: use 64bit math " Jerome Brunet 2017-04-07 15:34 ` Jerome Brunet 2017-04-07 15:34 ` Jerome Brunet 2017-04-07 15:39 ` Neil Armstrong 2017-04-07 15:39 ` Neil Armstrong 2017-04-07 15:39 ` Neil Armstrong
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.