[PATCH 0/2] clk: meson: MPLL fixes for Meson8b

[PATCH 0/2] clk: meson: MPLL fixes for Meson8b

[Martin Blumenstingl]

Jerome recently added the MPLL clocks to the Meson8b clock driver: [0]

On my board this unfortunatley 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.
If the review of patch #2 reveals problems then patch #1 should still
be applied.

This series is based to the "clk-meson" branch (e65ae3fb97b4
"dt-bindings: clock: gxbb-clkc: Add GXL compatible variant").


[0] http://lists.infradead.org/pipermail/linux-amlogic/2017-March/002757.html

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 | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

-- 
2.12.1

[PATCH 1/2] clk: meson: mpll: fix division by zero in rate_from_params

[Martin Blumenstingl]

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)

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>
---
 drivers/clk/meson/clk-mpll.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/meson/clk-mpll.c b/drivers/clk/meson/clk-mpll.c
index 540dabe5adad..551aa2a5b291 100644
--- a/drivers/clk/meson/clk-mpll.c
+++ b/drivers/clk/meson/clk-mpll.c
@@ -76,7 +76,12 @@ static unsigned 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 (divisor == 0)
+		return 0;
+	else
+		return (parent_rate * SDM_DEN) / divisor;
 }
 
 static void params_from_rate(unsigned long requested_rate,
-- 
2.12.1

Re: [PATCH 1/2] clk: meson: mpll: fix division by zero in rate_from_params

[Jerome Brunet]

On Sat, 2017-04-01 at 15:02 +0200, Martin Blumenstingl wrote:
> 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)
> 
> 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.

The Documentation of the S905 also state that N2 minimal value is 4 and SDM
minimal is 1 ... I should have been more careful ! Thanks for catching this
Martin. We definitely need to fix this.

Patch seems ok but I have one question. Is the rate actually zero when the
divisor is zero ?
I'll be away from my boards until Wednesday but I can check this when I get back
if you can't.

> 
> Fixes: 1c50da4f27 ("clk: meson: add mpll support")
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/clk/meson/clk-mpll.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/meson/clk-mpll.c b/drivers/clk/meson/clk-mpll.c
> index 540dabe5adad..551aa2a5b291 100644
> --- a/drivers/clk/meson/clk-mpll.c
> +++ b/drivers/clk/meson/clk-mpll.c
> @@ -76,7 +76,12 @@ static unsigned 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 (divisor == 0)
> +		return 0;
> +	else
> +		return (parent_rate * SDM_DEN) / divisor;
>  }
>  
>  static void params_from_rate(unsigned long requested_rate,

Re: [PATCH 1/2] clk: meson: mpll: fix division by zero in rate_from_params

[Martin Blumenstingl]

Hi Jerome,

On Sun, Apr 2, 2017 at 4:49 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> On Sat, 2017-04-01 at 15:02 +0200, Martin Blumenstingl wrote:
>> 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)
>>
>> 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.
>
> The Documentation of the S905 also state that N2 minimal value is 4 and SDM
> minimal is 1 ... I should have been more careful ! Thanks for catching this
> Martin. We definitely need to fix this.
>
> Patch seems ok but I have one question. Is the rate actually zero when the
> divisor is zero ?
> I'll be away from my boards until Wednesday but I can check this when I get back
> if you can't.
is there a way how I can check this? I have a consumer device so I
can't measure the frequencies at some pin.

>>
>> Fixes: 1c50da4f27 ("clk: meson: add mpll support")
>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> ---
>>  drivers/clk/meson/clk-mpll.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/meson/clk-mpll.c b/drivers/clk/meson/clk-mpll.c
>> index 540dabe5adad..551aa2a5b291 100644
>> --- a/drivers/clk/meson/clk-mpll.c
>> +++ b/drivers/clk/meson/clk-mpll.c
>> @@ -76,7 +76,12 @@ static unsigned 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 (divisor == 0)
>> +             return 0;
>> +     else
>> +             return (parent_rate * SDM_DEN) / divisor;
>>  }
>>
>>  static void params_from_rate(unsigned long requested_rate,

Re: [PATCH 1/2] clk: meson: mpll: fix division by zero in rate_from_params

[Jerome Brunet]

On Sun, 2017-04-02 at 20:43 +0200, Martin Blumenstingl wrote:
> Hi Jerome,
> 
> On Sun, Apr 2, 2017 at 4:49 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> > On Sat, 2017-04-01 at 15:02 +0200, Martin Blumenstingl wrote:
> > > 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)
> > > 
> > > 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.
> > 
> > The Documentation of the S905 also state that N2 minimal value is 4 and SDM
> > minimal is 1 ... I should have been more careful ! Thanks for catching this
> > Martin. We definitely need to fix this.
> > 
> > Patch seems ok but I have one question. Is the rate actually zero when the
> > divisor is zero ?
> > I'll be away from my boards until Wednesday but I can check this when I get
> > back
> > if you can't.
> 
> is there a way how I can check this? I have a consumer device so I
> can't measure the frequencies at some pin.

I'll try through the audio pins on wednesday. Don't worry about it. 

> 
> > > 
> > > Fixes: 1c50da4f27 ("clk: meson: add mpll support")
> > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> > > ---
> > >  drivers/clk/meson/clk-mpll.c | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/clk/meson/clk-mpll.c b/drivers/clk/meson/clk-mpll.c
> > > index 540dabe5adad..551aa2a5b291 100644
> > > --- a/drivers/clk/meson/clk-mpll.c
> > > +++ b/drivers/clk/meson/clk-mpll.c
> > > @@ -76,7 +76,12 @@ static unsigned 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 (divisor == 0)
> > > +             return 0;
> > > +     else
> > > +             return (parent_rate * SDM_DEN) / divisor;
> > >  }
> > > 
> > >  static void params_from_rate(unsigned long requested_rate,

Re: [PATCH 1/2] clk: meson: mpll: fix division by zero in rate_from_params

[Jerome Brunet]

On Sun, 2017-04-02 at 23:34 +0200, Jerome Brunet wrote:
> On Sun, 2017-04-02 at 20:43 +0200, Martin Blumenstingl wrote:
> > Hi Jerome,
> > 
> > On Sun, Apr 2, 2017 at 4:49 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> > > On Sat, 2017-04-01 at 15:02 +0200, Martin Blumenstingl wrote:
> > > > 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)
> > > > 
> > > > 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.
> > > 
> > > The Documentation of the S905 also state that N2 minimal value is 4 and
> > > SDM
> > > minimal is 1 ... I should have been more careful ! Thanks for catching
> > > this
> > > Martin. We definitely need to fix this.
> > > 
> > > Patch seems ok but I have one question. Is the rate actually zero when the
> > > divisor is zero ?
> > > I'll be away from my boards until Wednesday but I can check this when I
> > > get
> > > back
> > > if you can't.
> > 
> > is there a way how I can check this? I have a consumer device so I
> > can't measure the frequencies at some pin.
> 
> I'll try through the audio pins on wednesday. Don't worry about it.

So I checked, when N2 is less than the documented minimum (4), the clock get
into some weird state but is not grounded.
For example, on gxbb with N2 the rate of the mpll is around 4Mhz.

We have to return an error code here.
I've reworked the patch ... i'll send it very soon

> 
> > 
> > > > 
> > > > Fixes: 1c50da4f27 ("clk: meson: add mpll support")
> > > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> > > > ---
> > > >  drivers/clk/meson/clk-mpll.c | 7 ++++++-
> > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/clk/meson/clk-mpll.c b/drivers/clk/meson/clk-mpll.c
> > > > index 540dabe5adad..551aa2a5b291 100644
> > > > --- a/drivers/clk/meson/clk-mpll.c
> > > > +++ b/drivers/clk/meson/clk-mpll.c
> > > > @@ -76,7 +76,12 @@ static unsigned 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 (divisor == 0)
> > > > +             return 0;
> > > > +     else
> > > > +             return (parent_rate * SDM_DEN) / divisor;
> > > >  }
> > > > 
> > > >  static void params_from_rate(unsigned long requested_rate,

[PATCH 2/2] clk: meson: mpll: use 64bit math in rate_from_params

[Martin Blumenstingl]

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>
---
 drivers/clk/meson/clk-mpll.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/meson/clk-mpll.c b/drivers/clk/meson/clk-mpll.c
index 551aa2a5b291..4306ad833a4f 100644
--- a/drivers/clk/meson/clk-mpll.c
+++ b/drivers/clk/meson/clk-mpll.c
@@ -62,6 +62,7 @@
  */
 
 #include <linux/clk-provider.h>
+#include <linux/math64.h>
 #include "clkc.h"
 
 #define SDM_DEN 16384
@@ -81,7 +82,7 @@ static unsigned long rate_from_params(unsigned long parent_rate,
 	if (divisor == 0)
 		return 0;
 	else
-		return (parent_rate * SDM_DEN) / divisor;
+		return mul_u64_u32_div(parent_rate, SDM_DEN, divisor);
 }
 
 static void params_from_rate(unsigned long requested_rate,
-- 
2.12.1

Re: [PATCH 2/2] clk: meson: mpll: use 64bit math in rate_from_params

[Jerome Brunet]

On Sat, 2017-04-01 at 15:02 +0200, Martin Blumenstingl wrote:
> 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.

Again, thanks for testing and fixing this Martin !

> 
> Fixes: 1c50da4f27 ("clk: meson: add mpll support")
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/clk/meson/clk-mpll.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/meson/clk-mpll.c b/drivers/clk/meson/clk-mpll.c
> index 551aa2a5b291..4306ad833a4f 100644
> --- a/drivers/clk/meson/clk-mpll.c
> +++ b/drivers/clk/meson/clk-mpll.c
> @@ -62,6 +62,7 @@
>   */
>  
>  #include <linux/clk-provider.h>
> +#include <linux/math64.h>
>  #include "clkc.h"
>  
>  #define SDM_DEN 16384
> @@ -81,7 +82,7 @@ static unsigned long rate_from_params(unsigned long
> parent_rate,
>  	if (divisor == 0)
>  		return 0;
>  	else
> -		return (parent_rate * SDM_DEN) / divisor;
> +		return mul_u64_u32_div(parent_rate, SDM_DEN, divisor);

Just casting parent_rate to u64 should be enough, don't you think ?
Actually, I was thinking of copying a pattern that is used a lot in CCF and use
DIV_ROUND_UP_ULL((u64)foo, bar). See clk-divider.c.

Would this be ok with you ?

>  }
>  
>  static void params_from_rate(unsigned long requested_rate,

Re: [PATCH 2/2] clk: meson: mpll: use 64bit math in rate_from_params

[Martin Blumenstingl]

Hi Jerome,

On Sun, Apr 2, 2017 at 5:09 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> On Sat, 2017-04-01 at 15:02 +0200, Martin Blumenstingl wrote:
>> 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.
>
> Again, thanks for testing and fixing this Martin !
>
>>
>> Fixes: 1c50da4f27 ("clk: meson: add mpll support")
>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> ---
>>  drivers/clk/meson/clk-mpll.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/meson/clk-mpll.c b/drivers/clk/meson/clk-mpll.c
>> index 551aa2a5b291..4306ad833a4f 100644
>> --- a/drivers/clk/meson/clk-mpll.c
>> +++ b/drivers/clk/meson/clk-mpll.c
>> @@ -62,6 +62,7 @@
>>   */
>>
>>  #include <linux/clk-provider.h>
>> +#include <linux/math64.h>
>>  #include "clkc.h"
>>
>>  #define SDM_DEN 16384
>> @@ -81,7 +82,7 @@ static unsigned long rate_from_params(unsigned long
>> parent_rate,
>>       if (divisor == 0)
>>               return 0;
>>       else
>> -             return (parent_rate * SDM_DEN) / divisor;
>> +             return mul_u64_u32_div(parent_rate, SDM_DEN, divisor);
>
> Just casting parent_rate to u64 should be enough, don't you think ?
> Actually, I was thinking of copying a pattern that is used a lot in CCF and use
> DIV_ROUND_UP_ULL((u64)foo, bar). See clk-divider.c.
>
> Would this be ok with you ?
I'm actually happy with any patch that fixes bugs (properly) :)
the current code seems to round the values down instead of up. the
easiest way to check if this is a problem or not is probably by
testing it with your audio driver (which is probably sensitive to this
kind of stuff)


>>  }
>>
>>  static void params_from_rate(unsigned long requested_rate,