[PATCH 0/2] clk: at91: usb: rate calculation fixes

[PATCH 0/2] clk: at91: usb: rate calculation fixes

[Boris Brezillon]

Hello,

Here are a few more fixes for the at91 usb clk driver.

These bugs are related to rate calculation, and have been reported by
Andreas.

As this is not my first mistake in this rate calculation algorithm, I'd
really like to have reviews/feedback on this fixes to avoid potential
regressions.

Mike, when/if we agree on these fixes, can we make them go into one of
the 3.18-rc cycle ?

Thanks,

Boris

Boris Brezillon (2):
  clk: at91: usb: fix at91rm9200 round and set rate
  clk: at91: usb: fix at91sam9x5 recalc, round and set rate

 drivers/clk/at91/clk-usb.c | 35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)

-- 
1.9.1

[PATCH 1/2] clk: at91: usb: fix at91rm9200 round and set rate

[Boris Brezillon]

at91rm9200_clk_usb_set_rate might fail depending on the requested rate,
because the parent_rate / rate remainder is not necessarily zero.
Moreover, when rounding down the calculated rate we might alter the
divisor calculation and end up with an invalid divisor.

To solve those problems, accept a non zero remainder, and always round
division to the closest result.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Reported-by: Andreas Henriksson <andreas.henriksson@endian.se>
---
 drivers/clk/at91/clk-usb.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/at91/clk-usb.c b/drivers/clk/at91/clk-usb.c
index 24b5b02..5b3b63c 100644
--- a/drivers/clk/at91/clk-usb.c
+++ b/drivers/clk/at91/clk-usb.c
@@ -253,7 +253,7 @@ static long at91rm9200_clk_usb_round_rate(struct clk_hw *hw, unsigned long rate,
 
 		tmp_parent_rate = rate * usb->divisors[i];
 		tmp_parent_rate = __clk_round_rate(parent, tmp_parent_rate);
-		tmprate = tmp_parent_rate / usb->divisors[i];
+		tmprate = DIV_ROUND_CLOSEST(tmp_parent_rate, usb->divisors[i]);
 		if (tmprate < rate)
 			tmpdiff = rate - tmprate;
 		else
@@ -281,10 +281,10 @@ static int at91rm9200_clk_usb_set_rate(struct clk_hw *hw, unsigned long rate,
 	struct at91_pmc *pmc = usb->pmc;
 	unsigned long div;
 
-	if (!rate || parent_rate % rate)
+	if (!rate)
 		return -EINVAL;
 
-	div = parent_rate / rate;
+	div = DIV_ROUND_CLOSEST(parent_rate, rate);
 
 	for (i = 0; i < RM9200_USB_DIV_TAB_SIZE; i++) {
 		if (usb->divisors[i] == div) {
-- 
1.9.1

[PATCH 1/2] clk: at91: usb: fix at91rm9200 round and set rate

[Andreas Henriksson]

Hello!

Just confirming this exact patch fixes the problem I was running into.

On Wed, Nov 05, 2014 at 10:33:14AM +0100, Boris Brezillon wrote:
> at91rm9200_clk_usb_set_rate might fail depending on the requested rate,
> because the parent_rate / rate remainder is not necessarily zero.
> Moreover, when rounding down the calculated rate we might alter the
> divisor calculation and end up with an invalid divisor.
> 
> To solve those problems, accept a non zero remainder, and always round
> division to the closest result.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> Reported-by: Andreas Henriksson <andreas.henriksson@endian.se>

Tested-by: Andreas Henriksson <andreas.henriksson@endian.se>

> ---
>  drivers/clk/at91/clk-usb.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/at91/clk-usb.c b/drivers/clk/at91/clk-usb.c
> index 24b5b02..5b3b63c 100644
> --- a/drivers/clk/at91/clk-usb.c
> +++ b/drivers/clk/at91/clk-usb.c
> @@ -253,7 +253,7 @@ static long at91rm9200_clk_usb_round_rate(struct clk_hw *hw, unsigned long rate,
>  
>  		tmp_parent_rate = rate * usb->divisors[i];
>  		tmp_parent_rate = __clk_round_rate(parent, tmp_parent_rate);
> -		tmprate = tmp_parent_rate / usb->divisors[i];
> +		tmprate = DIV_ROUND_CLOSEST(tmp_parent_rate, usb->divisors[i]);
>  		if (tmprate < rate)
>  			tmpdiff = rate - tmprate;
>  		else
> @@ -281,10 +281,10 @@ static int at91rm9200_clk_usb_set_rate(struct clk_hw *hw, unsigned long rate,
>  	struct at91_pmc *pmc = usb->pmc;
>  	unsigned long div;
>  
> -	if (!rate || parent_rate % rate)
> +	if (!rate)
>  		return -EINVAL;
>  
> -	div = parent_rate / rate;
> +	div = DIV_ROUND_CLOSEST(parent_rate, rate);
>  
>  	for (i = 0; i < RM9200_USB_DIV_TAB_SIZE; i++) {
>  		if (usb->divisors[i] == div) {
> -- 
> 1.9.1
> 

[PATCH 2/2] clk: at91: usb: fix at91sam9x5 recalc, round and set rate

[Boris Brezillon]

First check for rate == 0 in set_rate and round_rate to avoid div by zero.
Then, in order to get the closest rate, round all divisions to the closest
result instead of rounding them down.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/clk/at91/clk-usb.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/clk/at91/clk-usb.c b/drivers/clk/at91/clk-usb.c
index 5b3b63c..7980e8c 100644
--- a/drivers/clk/at91/clk-usb.c
+++ b/drivers/clk/at91/clk-usb.c
@@ -52,7 +52,8 @@ static unsigned long at91sam9x5_clk_usb_recalc_rate(struct clk_hw *hw,
 
 	tmp = pmc_read(pmc, AT91_PMC_USB);
 	usbdiv = (tmp & AT91_PMC_OHCIUSBDIV) >> SAM9X5_USB_DIV_SHIFT;
-	return parent_rate / (usbdiv + 1);
+
+	return DIV_ROUND_CLOSEST(parent_rate, (usbdiv + 1));
 }
 
 static long at91sam9x5_clk_usb_round_rate(struct clk_hw *hw, unsigned long rate,
@@ -62,19 +63,19 @@ static long at91sam9x5_clk_usb_round_rate(struct clk_hw *hw, unsigned long rate,
 	unsigned long bestrate;
 	unsigned long tmp;
 
+	if (!rate)
+		return DIV_ROUND_CLOSEST(*parent_rate, SAM9X5_USB_MAX_DIV + 1);
+
 	if (rate >= *parent_rate)
 		return *parent_rate;
 
-	div = *parent_rate / rate;
-	if (div >= SAM9X5_USB_MAX_DIV)
-		return *parent_rate / (SAM9X5_USB_MAX_DIV + 1);
+	div = DIV_ROUND_CLOSEST(*parent_rate, rate);
+	if (div > SAM9X5_USB_MAX_DIV + 1)
+		div = SAM9X5_USB_MAX_DIV + 1;
+	else if (!div)
+		div = 1;
 
-	bestrate = *parent_rate / div;
-	tmp = *parent_rate / (div + 1);
-	if (bestrate - rate > rate - tmp)
-		bestrate = tmp;
-
-	return bestrate;
+	return DIV_ROUND_CLOSEST(*parent_rate, div);
 }
 
 static int at91sam9x5_clk_usb_set_parent(struct clk_hw *hw, u8 index)
@@ -106,9 +107,13 @@ static int at91sam9x5_clk_usb_set_rate(struct clk_hw *hw, unsigned long rate,
 	u32 tmp;
 	struct at91sam9x5_clk_usb *usb = to_at91sam9x5_clk_usb(hw);
 	struct at91_pmc *pmc = usb->pmc;
-	unsigned long div = parent_rate / rate;
+	unsigned long div;
 
-	if (parent_rate % rate || div < 1 || div >= SAM9X5_USB_MAX_DIV)
+	if (!rate)
+		return -EINVAL;
+
+	div = DIV_ROUND_CLOSEST(parent_rate, rate);
+	if (div > SAM9X5_USB_MAX_DIV + 1 || !div)
 		return -EINVAL;
 
 	tmp = pmc_read(pmc, AT91_PMC_USB) & ~AT91_PMC_OHCIUSBDIV;
-- 
1.9.1

[PATCH 2/2] clk: at91: usb: fix at91sam9x5 recalc, round and set rate

[Alexandre Belloni]

On 05/11/2014 at 10:33:15 +0100, Boris Brezillon wrote :
> First check for rate == 0 in set_rate and round_rate to avoid div by zero.
> Then, in order to get the closest rate, round all divisions to the closest
> result instead of rounding them down.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>  drivers/clk/at91/clk-usb.c | 29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/clk/at91/clk-usb.c b/drivers/clk/at91/clk-usb.c
> index 5b3b63c..7980e8c 100644
> --- a/drivers/clk/at91/clk-usb.c
> +++ b/drivers/clk/at91/clk-usb.c
> @@ -52,7 +52,8 @@ static unsigned long at91sam9x5_clk_usb_recalc_rate(struct clk_hw *hw,
>  
>  	tmp = pmc_read(pmc, AT91_PMC_USB);
>  	usbdiv = (tmp & AT91_PMC_OHCIUSBDIV) >> SAM9X5_USB_DIV_SHIFT;
> -	return parent_rate / (usbdiv + 1);
> +
> +	return DIV_ROUND_CLOSEST(parent_rate, (usbdiv + 1));
>  }
>  
>  static long at91sam9x5_clk_usb_round_rate(struct clk_hw *hw, unsigned long rate,
> @@ -62,19 +63,19 @@ static long at91sam9x5_clk_usb_round_rate(struct clk_hw *hw, unsigned long rate,
>  	unsigned long bestrate;
>  	unsigned long tmp;
>  
> +	if (!rate)
> +		return DIV_ROUND_CLOSEST(*parent_rate, SAM9X5_USB_MAX_DIV + 1);
> +

Maybe I'm missing something but I would return -EINVAL here.

>  	if (rate >= *parent_rate)
>  		return *parent_rate;
>  
> -	div = *parent_rate / rate;
> -	if (div >= SAM9X5_USB_MAX_DIV)
> -		return *parent_rate / (SAM9X5_USB_MAX_DIV + 1);
> +	div = DIV_ROUND_CLOSEST(*parent_rate, rate);
> +	if (div > SAM9X5_USB_MAX_DIV + 1)
> +		div = SAM9X5_USB_MAX_DIV + 1;
> +	else if (!div)
> +		div = 1;

In that case, you are also screwed, I would return -EINVAL.

>  
> -	bestrate = *parent_rate / div;
> -	tmp = *parent_rate / (div + 1);
> -	if (bestrate - rate > rate - tmp)
> -		bestrate = tmp;
> -
> -	return bestrate;
> +	return DIV_ROUND_CLOSEST(*parent_rate, div);
>  }
>  
>  static int at91sam9x5_clk_usb_set_parent(struct clk_hw *hw, u8 index)
> @@ -106,9 +107,13 @@ static int at91sam9x5_clk_usb_set_rate(struct clk_hw *hw, unsigned long rate,
>  	u32 tmp;
>  	struct at91sam9x5_clk_usb *usb = to_at91sam9x5_clk_usb(hw);
>  	struct at91_pmc *pmc = usb->pmc;
> -	unsigned long div = parent_rate / rate;
> +	unsigned long div;
>  
> -	if (parent_rate % rate || div < 1 || div >= SAM9X5_USB_MAX_DIV)
> +	if (!rate)
> +		return -EINVAL;
> +
> +	div = DIV_ROUND_CLOSEST(parent_rate, rate);
> +	if (div > SAM9X5_USB_MAX_DIV + 1 || !div)
>  		return -EINVAL;
>  
>  	tmp = pmc_read(pmc, AT91_PMC_USB) & ~AT91_PMC_OHCIUSBDIV;
> -- 
> 1.9.1
> 

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 2/2] clk: at91: usb: fix at91sam9x5 recalc, round and set rate

[Boris Brezillon]

On Fri, 7 Nov 2014 18:51:39 +0100
Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote:

> On 05/11/2014 at 10:33:15 +0100, Boris Brezillon wrote :
> > First check for rate == 0 in set_rate and round_rate to avoid div by zero.
> > Then, in order to get the closest rate, round all divisions to the closest
> > result instead of rounding them down.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> >  drivers/clk/at91/clk-usb.c | 29 +++++++++++++++++------------
> >  1 file changed, 17 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/clk/at91/clk-usb.c b/drivers/clk/at91/clk-usb.c
> > index 5b3b63c..7980e8c 100644
> > --- a/drivers/clk/at91/clk-usb.c
> > +++ b/drivers/clk/at91/clk-usb.c
> > @@ -52,7 +52,8 @@ static unsigned long at91sam9x5_clk_usb_recalc_rate(struct clk_hw *hw,
> >  
> >  	tmp = pmc_read(pmc, AT91_PMC_USB);
> >  	usbdiv = (tmp & AT91_PMC_OHCIUSBDIV) >> SAM9X5_USB_DIV_SHIFT;
> > -	return parent_rate / (usbdiv + 1);
> > +
> > +	return DIV_ROUND_CLOSEST(parent_rate, (usbdiv + 1));
> >  }
> >  
> >  static long at91sam9x5_clk_usb_round_rate(struct clk_hw *hw, unsigned long rate,
> > @@ -62,19 +63,19 @@ static long at91sam9x5_clk_usb_round_rate(struct clk_hw *hw, unsigned long rate,
> >  	unsigned long bestrate;
> >  	unsigned long tmp;
> >  
> > +	if (!rate)
> > +		return DIV_ROUND_CLOSEST(*parent_rate, SAM9X5_USB_MAX_DIV + 1);
> > +
> 
> Maybe I'm missing something but I would return -EINVAL here.

That's what I did at first, but just realized maybe 0 is a valid
request and we should try to be as close as possible to 0.

Anyway, I'm not really convinced we need that, so I can drop it.

> 
> >  	if (rate >= *parent_rate)
> >  		return *parent_rate;
> >  
> > -	div = *parent_rate / rate;
> > -	if (div >= SAM9X5_USB_MAX_DIV)
> > -		return *parent_rate / (SAM9X5_USB_MAX_DIV + 1);
> > +	div = DIV_ROUND_CLOSEST(*parent_rate, rate);
> > +	if (div > SAM9X5_USB_MAX_DIV + 1)
> > +		div = SAM9X5_USB_MAX_DIV + 1;
> > +	else if (!div)
> > +		div = 1;
> 
> In that case, you are also screwed, I would return -EINVAL.

Well, actually that cannot happen, because I already tested
rate >= *parent_rate.
I'll remove that line.



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH 0/2] clk: at91: usb: rate calculation fixes

[Nicolas Ferre]

On 05/11/2014 10:33, Boris Brezillon :
> Hello,
> 
> Here are a few more fixes for the at91 usb clk driver.
> 
> These bugs are related to rate calculation, and have been reported by
> Andreas.
> 
> As this is not my first mistake in this rate calculation algorithm, I'd
> really like to have reviews/feedback on this fixes to avoid potential
> regressions.
> 
> Mike, when/if we agree on these fixes, can we make them go into one of
> the 3.18-rc cycle ?

Can you give us a hint about the "stable tag" that would be good to add?

Bye,


> Boris Brezillon (2):
>   clk: at91: usb: fix at91rm9200 round and set rate
>   clk: at91: usb: fix at91sam9x5 recalc, round and set rate
> 
>  drivers/clk/at91/clk-usb.c | 35 ++++++++++++++++++++---------------
>  1 file changed, 20 insertions(+), 15 deletions(-)
> 


-- 
Nicolas Ferre

[PATCH 0/2] clk: at91: usb: rate calculation fixes

[Boris Brezillon]

Hi Nicolas,

On Wed, 5 Nov 2014 18:13:25 +0100
Nicolas Ferre <nicolas.ferre@atmel.com> wrote:

> On 05/11/2014 10:33, Boris Brezillon :
> > Hello,
> > 
> > Here are a few more fixes for the at91 usb clk driver.
> > 
> > These bugs are related to rate calculation, and have been reported by
> > Andreas.
> > 
> > As this is not my first mistake in this rate calculation algorithm, I'd
> > really like to have reviews/feedback on this fixes to avoid potential
> > regressions.
> > 
> > Mike, when/if we agree on these fixes, can we make them go into one of
> > the 3.18-rc cycle ?
> 
> Can you give us a hint about the "stable tag" that would be good to add?

None for the moment: these 2 patches apply on top of other patches that
were merge in 3.18.
What we should do though is backport the set of usb fixes merged in 3.18
to 3.16/3.17 once 3.18 is out...

Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH 0/2] clk: at91: usb: rate calculation fixes

[Nicolas Ferre]

On 05/11/2014 10:33, Boris Brezillon :
> Hello,
> 
> Here are a few more fixes for the at91 usb clk driver.
> 
> These bugs are related to rate calculation, and have been reported by
> Andreas.
> 
> As this is not my first mistake in this rate calculation algorithm, I'd
> really like to have reviews/feedback on this fixes to avoid potential
> regressions.
> 
> Mike, when/if we agree on these fixes, can we make them go into one of
> the 3.18-rc cycle ?

With Alex modifications, you can add my:

Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>

To your next series.

Bye,


> Boris Brezillon (2):
>   clk: at91: usb: fix at91rm9200 round and set rate
>   clk: at91: usb: fix at91sam9x5 recalc, round and set rate
> 
>  drivers/clk/at91/clk-usb.c | 35 ++++++++++++++++++++---------------
>  1 file changed, 20 insertions(+), 15 deletions(-)
> 


-- 
Nicolas Ferre

[PATCH 0/2] clk: at91: usb: rate calculation fixes

[Mike Turquette]

Quoting Nicolas Ferre (2014-11-13 02:59:35)
> On 05/11/2014 10:33, Boris Brezillon :
> > Hello,
> > 
> > Here are a few more fixes for the at91 usb clk driver.
> > 
> > These bugs are related to rate calculation, and have been reported by
> > Andreas.
> > 
> > As this is not my first mistake in this rate calculation algorithm, I'd
> > really like to have reviews/feedback on this fixes to avoid potential
> > regressions.
> > 
> > Mike, when/if we agree on these fixes, can we make them go into one of
> > the 3.18-rc cycle ?
> 
> With Alex modifications, you can add my:
> 
> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>

Yes, I'll take them in for the next -rc once V2 comes out.

Regards,
Mike

> 
> To your next series.
> 
> Bye,
> 
> 
> > Boris Brezillon (2):
> >   clk: at91: usb: fix at91rm9200 round and set rate
> >   clk: at91: usb: fix at91sam9x5 recalc, round and set rate
> > 
> >  drivers/clk/at91/clk-usb.c | 35 ++++++++++++++++++++---------------
> >  1 file changed, 20 insertions(+), 15 deletions(-)
> > 
> 
> 
> -- 
> Nicolas Ferre