[PATCH v2 0/2] clk: imx: fix AV PLL rate setting

[PATCH v2 0/2] clk: imx: fix AV PLL rate setting

[Emil Lundmark]

Hi,

I did not use a cover letter in v1, so here is a summary instead.

I discovered a problem when trying to set the rate of the audio PLL
(pll4_post_div) on an i.MX6Q. The rate I wanted to set was 196.608 MHz, but
the actual rate I got was 192.000570 MHz. This patch series fixes this
issue and also improves the precision of the audio/video PLL.

There are no code changes in v2, only clarifications in the commit
messages.

Changes since v1:
- Use correct subsystem name in commit summary.
- Use 'Fixes:' tag to indicate bug fix.
- Revise argument for choosing the denominator register value after
  comments from Lothar Wa?mann.

v1: http://lists.infradead.org/pipermail/linux-arm-kernel/2016-October/460350.html

Emil Lundmark (2):
  clk: imx: fix integer overflow in AV PLL round rate
  clk: imx: improve precision of AV PLL to 1 Hz

 drivers/clk/imx/clk-pllv3.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

-- 
2.7.4

[PATCH v2 1/2] clk: imx: fix integer overflow in AV PLL round rate

[Emil Lundmark]

Since 'parent_rate * mfn' may overflow 32 bits, the result should be
stored using 64 bits.

Fixes: ba7f4f557eb6 ("clk: imx: correct AV PLL rate formula")
Signed-off-by: Emil Lundmark <emil@limesaudio.com>
---
 drivers/clk/imx/clk-pllv3.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/imx/clk-pllv3.c b/drivers/clk/imx/clk-pllv3.c
index 19f9b622981a..bc7f163ea13c 100644
--- a/drivers/clk/imx/clk-pllv3.c
+++ b/drivers/clk/imx/clk-pllv3.c
@@ -247,7 +247,11 @@ static long clk_pllv3_av_round_rate(struct clk_hw *hw, unsigned long rate,
 	do_div(temp64, parent_rate);
 	mfn = temp64;
 
-	return parent_rate * div + parent_rate * mfn / mfd;
+	temp64 = (u64)parent_rate;
+	temp64 *= mfn;
+	do_div(temp64, mfd);
+
+	return parent_rate * div + (u32)temp64;
 }
 
 static int clk_pllv3_av_set_rate(struct clk_hw *hw, unsigned long rate,
-- 
2.7.4

[PATCH v2 1/2] clk: imx: fix integer overflow in AV PLL round rate

[Uwe Kleine-König]

On Mon, Oct 10, 2016 at 12:03:05PM +0200, Emil Lundmark wrote:
> Since 'parent_rate * mfn' may overflow 32 bits, the result should be
> stored using 64 bits.
> 
> Fixes: ba7f4f557eb6 ("clk: imx: correct AV PLL rate formula")
> Signed-off-by: Emil Lundmark <emil@limesaudio.com>
> ---
>  drivers/clk/imx/clk-pllv3.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/imx/clk-pllv3.c b/drivers/clk/imx/clk-pllv3.c
> index 19f9b622981a..bc7f163ea13c 100644
> --- a/drivers/clk/imx/clk-pllv3.c
> +++ b/drivers/clk/imx/clk-pllv3.c
> @@ -247,7 +247,11 @@ static long clk_pllv3_av_round_rate(struct clk_hw *hw, unsigned long rate,
>  	do_div(temp64, parent_rate);
>  	mfn = temp64;
>  
> -	return parent_rate * div + parent_rate * mfn / mfd;
> +	temp64 = (u64)parent_rate;
> +	temp64 *= mfn;
> +	do_div(temp64, mfd);

If you change parent_rate from unsigned long to u64 this simplifies to

	temp64 = parent_rate * mfn
	do_div(temp64, mfd);

> +
> +	return parent_rate * div + (u32)temp64;

When thinking about overflow problems: Should this fail somehow if
temp64 != (u32)temp64?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH v2 1/2] clk: imx: fix integer overflow in AV PLL round rate

[Emil Lundmark]

On Mon, Oct 10, 2016 at 03:54:54PM +0200, Uwe Kleine-K?nig wrote:
> On Mon, Oct 10, 2016 at 12:03:05PM +0200, Emil Lundmark wrote:
> > Since 'parent_rate * mfn' may overflow 32 bits, the result should be
> > stored using 64 bits.
> > 
> > Fixes: ba7f4f557eb6 ("clk: imx: correct AV PLL rate formula")
> > Signed-off-by: Emil Lundmark <emil@limesaudio.com>
> > ---
> >  drivers/clk/imx/clk-pllv3.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/clk/imx/clk-pllv3.c b/drivers/clk/imx/clk-pllv3.c
> > index 19f9b622981a..bc7f163ea13c 100644
> > --- a/drivers/clk/imx/clk-pllv3.c
> > +++ b/drivers/clk/imx/clk-pllv3.c
> > @@ -247,7 +247,11 @@ static long clk_pllv3_av_round_rate(struct clk_hw *hw, unsigned long rate,
> >  	do_div(temp64, parent_rate);
> >  	mfn = temp64;
> >  
> > -	return parent_rate * div + parent_rate * mfn / mfd;
> > +	temp64 = (u64)parent_rate;
> > +	temp64 *= mfn;
> > +	do_div(temp64, mfd);
> 
> If you change parent_rate from unsigned long to u64 this simplifies to
> 
> 	temp64 = parent_rate * mfn
> 	do_div(temp64, mfd);
> 

Yes, I took my inspiration from clk_pllv3_av_recalc_rate().

> > +
> > +	return parent_rate * div + (u32)temp64;
> 
> When thinking about overflow problems: Should this fail somehow if
> temp64 != (u32)temp64?

Well, it will fail in the sence that the desired clock will not be
returned. However, since mfn / mfd < 1 it should be fine as long as
ULONG_MAX <= U32_MAX. So, maybe it would be better to cast it to unsigned
long? This would then also need to be changed in
clk_pllv3_av_recalc_rate().

-- 
Emil Lundmark

[PATCH v2 1/2] clk: imx: fix integer overflow in AV PLL round rate

[Fabio Estevam]

On Mon, Oct 10, 2016 at 7:03 AM, Emil Lundmark <emil@limesaudio.com> wrote:
> Since 'parent_rate * mfn' may overflow 32 bits, the result should be
> stored using 64 bits.

It would be nice to add the text you put in the cover letter where you
explain the PLL4 clock discrepancy here in the commit log.

>
> Fixes: ba7f4f557eb6 ("clk: imx: correct AV PLL rate formula")

Would be nice to Cc the author of this commit (Anson Huang). Added on Cc.

Another hint: ./scripts/get_maintainer.pl  drivers/clk/imx/clk-pllv3.c
gives you some suggestions on people and lists to add to Cc.

[PATCH v2 1/2] clk: imx: fix integer overflow in AV PLL round rate

[Emil Lundmark]

On Mon, Oct 10, 2016 at 11:08:40AM -0300, Fabio Estevam wrote:
> On Mon, Oct 10, 2016 at 7:03 AM, Emil Lundmark <emil@limesaudio.com> wrote:
> > Since 'parent_rate * mfn' may overflow 32 bits, the result should be
> > stored using 64 bits.
> 
> It would be nice to add the text you put in the cover letter where you
> explain the PLL4 clock discrepancy here in the commit log.

I will do that in v3.
 
> >
> > Fixes: ba7f4f557eb6 ("clk: imx: correct AV PLL rate formula")
> 
> Would be nice to Cc the author of this commit (Anson Huang). Added on Cc.

Good point, will keep this in mind in the future.
 
> Another hint: ./scripts/get_maintainer.pl  drivers/clk/imx/clk-pllv3.c
> gives you some suggestions on people and lists to add to Cc.

I did that, but read somewhere that you should send it to the maintainers
and CC the appropriate list. Should I also send it to reviewers?

Since this patch series only affects i.MX, I chose to not include the
people from the common clock framework. Was that wrong?

-- 
Emil Lundmark

[PATCH v2 1/2] clk: imx: fix integer overflow in AV PLL round rate

[Fabio Estevam]

On Tue, Oct 11, 2016 at 6:58 AM, Emil Lundmark <emil@limesaudio.com> wrote:

>> Another hint: ./scripts/get_maintainer.pl  drivers/clk/imx/clk-pllv3.c
>> gives you some suggestions on people and lists to add to Cc.
>
> I did that, but read somewhere that you should send it to the maintainers
> and CC the appropriate list. Should I also send it to reviewers?

It would be nice to send to reviewers as well.

>
> Since this patch series only affects i.MX, I chose to not include the
> people from the common clock framework. Was that wrong?

Yes, please include the clock folks and list as well.

Thanks

[PATCH v2 2/2] clk: imx: improve precision of AV PLL to 1 Hz

[Emil Lundmark]

The audio and video PLLs are designed to have a precision of 1 Hz if some
conditions are met. The current implementation only allows a precision that
depends on the rate of the parent clock. E.g., if the parent clock is 24
MHz, the precision will be 24 Hz; or more generally the precision will be

    p / 10^6 Hz

where p is the parent clock rate. This comes down to how the register
values for the PLL's fractional loop divider are chosen.

The clock rate calculation for the PLL is

    PLL output frequency = Fref * (DIV_SELECT + NUM / DENOM)

or with a shorter notation

    r = p * (d + a / b)

In addition to all variables being integers, we also have the following
conditions:

    27 <= d <= 54

    -2^29 <= a <= 2^29-1
     0    <  b <= 2^30-1
    |a| < b

Here, d, a and b are register values for the fractional loop divider. We
want to chose d, a and b such that f(p, r) = p, i.e. f is our round_rate
function. Currently, d and b are chosen as

    d = r / p
    b = 10^6

hence we get the poor precision. And a is defined in terms of r, d, p and
b:

    a = (r - d * p) * b / p

I propose that if p <= 2^30-1 (i.e., the max value for b), we chose b as

    b = p

We can do this since

    |a| < b

    |(r - d * p) * b / p| < b

    |r - d * p| < p

Which have two solutions, one of them is when p < 0, so we can skip that
one. The other is when p > 0 and

    p * (d - 1) < r < p * (d + 1)

Substitute d = r / p:

    (r - p) < r < (r + p)  <=>  p > 0

So, as long as p > 0, we can chose b = p. This is a good choice for b since

    a = (r - d * p) * b / p
      = r - d * p

    r = p * (d + a / b)
      = p * d + p * a / b
      = p * d + a

and if d = r / p:

    a = r - d * p
      = r - r / p * p
      = 0

    r = p * d + a
      = p * d + 0
      = p * r / p
      = r

I reckon this is the intention by the design of the clock rate formula.

Signed-off-by: Emil Lundmark <emil@limesaudio.com>
---
 drivers/clk/imx/clk-pllv3.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/clk/imx/clk-pllv3.c b/drivers/clk/imx/clk-pllv3.c
index bc7f163ea13c..5ff55ff5fd81 100644
--- a/drivers/clk/imx/clk-pllv3.c
+++ b/drivers/clk/imx/clk-pllv3.c
@@ -234,6 +234,7 @@ static long clk_pllv3_av_round_rate(struct clk_hw *hw, unsigned long rate,
 	unsigned long max_rate = parent_rate * 54;
 	u32 div;
 	u32 mfn, mfd = 1000000;
+	u32 max_mfd = 0x3FFFFFFF;
 	u64 temp64;
 
 	if (rate > max_rate)
@@ -241,6 +242,9 @@ static long clk_pllv3_av_round_rate(struct clk_hw *hw, unsigned long rate,
 	else if (rate < min_rate)
 		rate = min_rate;
 
+	if (parent_rate <= max_mfd)
+		mfd = parent_rate;
+
 	div = rate / parent_rate;
 	temp64 = (u64) (rate - div * parent_rate);
 	temp64 *= mfd;
@@ -262,11 +266,15 @@ static int clk_pllv3_av_set_rate(struct clk_hw *hw, unsigned long rate,
 	unsigned long max_rate = parent_rate * 54;
 	u32 val, div;
 	u32 mfn, mfd = 1000000;
+	u32 max_mfd = 0x3FFFFFFF;
 	u64 temp64;
 
 	if (rate < min_rate || rate > max_rate)
 		return -EINVAL;
 
+	if (parent_rate <= max_mfd)
+		mfd = parent_rate;
+
 	div = rate / parent_rate;
 	temp64 = (u64) (rate - div * parent_rate);
 	temp64 *= mfd;
-- 
2.7.4