clk: at91: usb: rate calculation fixes

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 <[email protected]>

Reported-by Andreas Henriksson <[email protected]>

---
 drivers/clk/at91/clk-usb.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/clk/at91/clk-usb.c +3 -3

index 24b5b02..5b3b63c 100644
@@ -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) {

Signature · 1 line

1.9.1

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 <[email protected]>

---
 drivers/clk/at91/clk-usb.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

drivers/clk/at91/clk-usb.c +2 -1

index 5b3b63c..7980e8c 100644
@@ -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,

drivers/clk/at91/clk-usb.c +7 -8

-	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;

Signature · 1 line

1.9.1

Quoted 27 lines · 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 <[email protected]>

---
 drivers/clk/at91/clk-usb.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

drivers/clk/at91/clk-usb.c +2 -1

index 5b3b63c..7980e8c 100644
@@ -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;
 

drivers/clk/at91/clk-usb.c static long at91sam9x5_clk_usb_round_rate(struct clk_hw *hw, unsigned long rate, · line 68

Quoted 3 lines

drivers/clk/at91/clk-usb.c +3 -0

+	if (!rate)
+		return DIV_ROUND_CLOSEST(*parent_rate, SAM9X5_USB_MAX_DIV + 1);
+

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

Quoted 4 lines

drivers/clk/at91/clk-usb.c +0 -0

 	if (rate >= *parent_rate)
 		return *parent_rate;
 

drivers/clk/at91/clk-usb.c static long at91sam9x5_clk_usb_round_rate(struct clk_hw *hw, unsigned long rate, · line 76

Quoted 8 lines

drivers/clk/at91/clk-usb.c +5 -3

-	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.

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 ?

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(-)

On 05/11/2014 10:33, Boris Brezillon :

Quoted 7 lines

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.

Quoted 6 lines

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?

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 ?

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(-)

On 05/11/2014 10:33, Boris Brezillon :

Quoted 11 lines

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.

Quoted 2 lines

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 <[email protected]>

To your next series.

Bye,

Quoted 7 lines

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(-)

Signature · 1 line

Nicolas Ferre