[PATCH 0/5] clk: at91: fix USB clk support on at91rm9200/sam9 SoCs

[PATCH 0/5] clk: at91: fix USB clk support on at91rm9200/sam9 SoCs

[Boris BREZILLON]

Hi,

This patch series fixes several bugs in the PLL driver preventing a proper
set_rate on the PLL clk.
It also enables propagation of set_rate request on the USB clk in order to
configure the PLL rate according to the USB block requirement (48 MHz).

Note that existing kernels, relying on the PLL configuration made by the
the bootloader should not be impacted by this bug, but others (those
directly booting from at91bootstrap or not enabling USB support in the
bootloader) will be.

This bug was reported by Ga?l, who's directly booting the kernel from the
bootstrap.

Best Regards,

Boris

Boris BREZILLON (5):
  clk: at91: fix PLL_MAX_COUNT macro definition
  clk: at91: rework PLL rate calculation
  clk: at91: fix recalc_rate implementation of PLL driver
  clk: at91: rework rm9200 USB clock to propagate set_rate to the parent
    clk
  clk: at91: fix div by zero in USB clock driver

 drivers/clk/at91/clk-pll.c | 160 +++++++++++++++++++++++----------------------
 drivers/clk/at91/clk-usb.c |  20 ++++--
 2 files changed, 96 insertions(+), 84 deletions(-)

-- 
1.9.1

[PATCH 1/5] clk: at91: fix PLL_MAX_COUNT macro definition

[Boris BREZILLON]

Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
Reported-by: Ga?l PORTAY <gael.portay@gmail.com>
---
 drivers/clk/at91/clk-pll.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/at91/clk-pll.c b/drivers/clk/at91/clk-pll.c
index cf6ed02..7b453b9 100644
--- a/drivers/clk/at91/clk-pll.c
+++ b/drivers/clk/at91/clk-pll.c
@@ -31,7 +31,7 @@
 				 (layout)->mul_mask)
 #define PLL_ICPR_SHIFT(id)	((id) * 16)
 #define PLL_ICPR_MASK(id)	(0xffff << PLL_ICPR_SHIFT(id))
-#define PLL_MAX_COUNT		0x3ff
+#define PLL_MAX_COUNT		0x3f
 #define PLL_COUNT_SHIFT		8
 #define PLL_OUT_SHIFT		14
 #define PLL_MAX_ID		1
-- 
1.9.1

[PATCH 2/5] clk: at91: rework PLL rate calculation

[Boris BREZILLON]

The AT91 PLL rate configuration is done by configuring a multiplier/divider
pair.
The previous calculation was over-complicated (and apparently buggy).
Simplify the implementation and add some comments to explain what is done
here.

Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
Reported-by: Ga?l PORTAY <gael.portay@gmail.com>
---
 drivers/clk/at91/clk-pll.c | 147 ++++++++++++++++++++++++---------------------
 1 file changed, 77 insertions(+), 70 deletions(-)

diff --git a/drivers/clk/at91/clk-pll.c b/drivers/clk/at91/clk-pll.c
index 7b453b9..a1adcf1 100644
--- a/drivers/clk/at91/clk-pll.c
+++ b/drivers/clk/at91/clk-pll.c
@@ -15,6 +15,7 @@
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
 #include <linux/io.h>
+#include <linux/kernel.h>
 #include <linux/wait.h>
 #include <linux/sched.h>
 #include <linux/interrupt.h>
@@ -29,6 +30,9 @@
 #define PLL_DIV(reg)		((reg) & PLL_DIV_MASK)
 #define PLL_MUL(reg, layout)	(((reg) >> (layout)->mul_shift) & \
 				 (layout)->mul_mask)
+#define PLL_MUL_MIN		2
+#define PLL_MUL_MASK(layout)	((layout)->mul_mask)
+#define PLL_MUL_MAX(layout)	(PLL_MUL_MASK(layout) + 1)
 #define PLL_ICPR_SHIFT(id)	((id) * 16)
 #define PLL_ICPR_MASK(id)	(0xffff << PLL_ICPR_SHIFT(id))
 #define PLL_MAX_COUNT		0x3f
@@ -163,99 +167,102 @@ static long clk_pll_get_best_div_mul(struct clk_pll *pll, unsigned long rate,
 				     unsigned long parent_rate,
 				     u32 *div, u32 *mul,
 				     u32 *index) {
-	unsigned long maxrate;
-	unsigned long minrate;
-	unsigned long divrate;
-	unsigned long bestdiv = 1;
-	unsigned long bestmul;
-	unsigned long tmpdiv;
-	unsigned long roundup;
-	unsigned long rounddown;
-	unsigned long remainder;
-	unsigned long bestremainder;
-	unsigned long maxmul;
-	unsigned long maxdiv;
-	unsigned long mindiv;
-	int i = 0;
 	const struct clk_pll_layout *layout = pll->layout;
 	const struct clk_pll_characteristics *characteristics =
 							pll->characteristics;
+	unsigned long bestremainder = ULONG_MAX;
+	unsigned long maxdiv, mindiv, tmpdiv;
+	long bestrate = -ERANGE;
+	unsigned long bestdiv;
+	unsigned long bestmul;
+	int i = 0;
 
-	/* Minimum divider = 1 */
-	/* Maximum multiplier = max_mul */
-	maxmul = layout->mul_mask + 1;
-	maxrate = (parent_rate * maxmul) / 1;
-
-	/* Maximum divider = max_div */
-	/* Minimum multiplier = 2 */
-	maxdiv = PLL_DIV_MAX;
-	minrate = (parent_rate * 2) / maxdiv;
-
+	/* Check if parent_rate is a valid input rate */
 	if (parent_rate < characteristics->input.min ||
-	    parent_rate < characteristics->input.max)
-		return -ERANGE;
-
-	if (parent_rate < minrate || parent_rate > maxrate)
+	    parent_rate > characteristics->input.max)
 		return -ERANGE;
 
-	for (i = 0; i < characteristics->num_output; i++) {
-		if (parent_rate >= characteristics->output[i].min &&
-		    parent_rate <= characteristics->output[i].max)
-			break;
-	}
-
-	if (i >= characteristics->num_output)
-		return -ERANGE;
-
-	bestmul = rate / parent_rate;
-	rounddown = parent_rate % rate;
-	roundup = rate - rounddown;
-	bestremainder = roundup < rounddown ? roundup : rounddown;
-
-	if (!bestremainder) {
-		if (div)
-			*div = bestdiv;
-		if (mul)
-			*mul = bestmul;
-		if (index)
-			*index = i;
-		return rate;
-	}
-
-	maxdiv = 255 / (bestmul + 1);
-	if (parent_rate / maxdiv < characteristics->input.min)
-		maxdiv = parent_rate / characteristics->input.min;
-	mindiv = parent_rate / characteristics->input.max;
-	if (parent_rate % characteristics->input.max)
-		mindiv++;
-
-	for (tmpdiv = mindiv; tmpdiv < maxdiv; tmpdiv++) {
-		divrate = parent_rate / tmpdiv;
-
-		rounddown = rate % divrate;
-		roundup = divrate - rounddown;
-		remainder = roundup < rounddown ? roundup : rounddown;
-
+	/*
+	 * Calculate minimum divider based on the minimum multiplier, the
+	 * parent_rate and the requested rate.
+	 * Should always be 2 according to the input and output characteristics
+	 * of the PLL blocks.
+	 */
+	mindiv = (parent_rate * PLL_MUL_MIN) / rate;
+	if (!mindiv)
+		mindiv = 1;
+
+	/*
+	 * Calculate the maximum divider which is limited by PLL register
+	 * layout (limited by the MUL or DIV field size).
+	 */
+	maxdiv = DIV_ROUND_UP(parent_rate * PLL_MUL_MAX(layout), rate);
+	if (maxdiv > PLL_DIV_MAX)
+		maxdiv = PLL_DIV_MAX;
+
+	/*
+	 * Iterate over the acceptable divider values to find the best
+	 * divider/multiplier pair (the one that generates the closest
+	 * rate to the requested one).
+	 */
+	for (tmpdiv = mindiv; tmpdiv <= maxdiv; tmpdiv++) {
+		unsigned long remainder;
+		unsigned long tmprate;
+		unsigned long tmpmul;
+
+		/*
+		 * Calculate the multiplier associated with the current
+		 * divider that provide the closest rate to the requested one.
+		 */
+		tmpmul = DIV_ROUND_CLOSEST(rate, parent_rate / tmpdiv);
+		tmprate = (parent_rate / tmpdiv) * tmpmul;
+		if (tmprate > rate)
+			remainder = tmprate - rate;
+		else
+			remainder = rate - tmprate;
+
+		/*
+		 * Compare the remainder with the best remainder found until
+		 * now and elect a new best multiplier/divider pair if the
+		 * current remainder is smaller than the best one.
+		 */
 		if (remainder < bestremainder) {
 			bestremainder = remainder;
-			bestmul = rate / divrate;
 			bestdiv = tmpdiv;
+			bestmul = tmpmul;
+			bestrate = tmprate;
 		}
 
+		/*
+		 * We've found a perfect match!
+		 * Stop searching now and use this multiplier/divider pair.
+		 */
 		if (!remainder)
 			break;
 	}
 
-	rate = (parent_rate / bestdiv) * bestmul;
+	/* We haven't found any multiplier/divider pair => return -ERANGE */
+	if (bestrate < 0)
+		return bestrate;
+
+	/* Check if bestrate is a valid output rate  */
+	for (i = 0; i < characteristics->num_output; i++) {
+		if (bestrate >= characteristics->output[i].min &&
+		    bestrate <= characteristics->output[i].max)
+			break;
+	}
+
+	if (i >= characteristics->num_output)
+		return -ERANGE;
 
 	if (div)
 		*div = bestdiv;
 	if (mul)
-		*mul = bestmul;
+		*mul = bestmul - 1;
 	if (index)
 		*index = i;
 
-	return rate;
+	return bestrate;
 }
 
 static long clk_pll_round_rate(struct clk_hw *hw, unsigned long rate,
-- 
1.9.1

[PATCH 3/5] clk: at91: fix recalc_rate implementation of PLL driver

[Boris BREZILLON]

Use the cached values to calculate PLL rate instead of the register values.
This is required to prevent erroneous PLL rate return when the PLL rate
has been configured but the PLL is not prepared yet.

Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
Reported-by: Ga?l PORTAY <gael.portay@gmail.com>
---
 drivers/clk/at91/clk-pll.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/at91/clk-pll.c b/drivers/clk/at91/clk-pll.c
index a1adcf1..6ec79db 100644
--- a/drivers/clk/at91/clk-pll.c
+++ b/drivers/clk/at91/clk-pll.c
@@ -151,16 +151,11 @@ static unsigned long clk_pll_recalc_rate(struct clk_hw *hw,
 					 unsigned long parent_rate)
 {
 	struct clk_pll *pll = to_clk_pll(hw);
-	const struct clk_pll_layout *layout = pll->layout;
-	struct at91_pmc *pmc = pll->pmc;
-	int offset = PLL_REG(pll->id);
-	u32 tmp = pmc_read(pmc, offset) & layout->pllr_mask;
-	u8 div = PLL_DIV(tmp);
-	u16 mul = PLL_MUL(tmp, layout);
-	if (!div || !mul)
+
+	if (!pll->div || !pll->mul)
 		return 0;
 
-	return (parent_rate * (mul + 1)) / div;
+	return (parent_rate / pll->div) * (pll->mul + 1);
 }
 
 static long clk_pll_get_best_div_mul(struct clk_pll *pll, unsigned long rate,
-- 
1.9.1

[PATCH 4/5] clk: at91: rework rm9200 USB clock to propagate set_rate to the parent clk

[Boris BREZILLON]

The RM9200 USB clock is actually connected to a single parent (the PLLB)
on which we can apply a specific divider.
The USB clock divider does not allow for fine grained control on the USB
clock frequency, hence propagating the set_rate request to the parent is
the only choice we have to properly configure the USB clock rate.

Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
Reported-by: Ga?l PORTAY <gael.portay@gmail.com>
---
 drivers/clk/at91/clk-usb.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/at91/clk-usb.c b/drivers/clk/at91/clk-usb.c
index 7d1d26a..1838777 100644
--- a/drivers/clk/at91/clk-usb.c
+++ b/drivers/clk/at91/clk-usb.c
@@ -238,16 +238,22 @@ static long at91rm9200_clk_usb_round_rate(struct clk_hw *hw, unsigned long rate,
 					  unsigned long *parent_rate)
 {
 	struct at91rm9200_clk_usb *usb = to_at91rm9200_clk_usb(hw);
+	struct clk *parent = __clk_get_parent(hw->clk);
 	unsigned long bestrate = 0;
 	int bestdiff = -1;
 	unsigned long tmprate;
 	int tmpdiff;
 	int i = 0;
 
-	for (i = 0; i < 4; i++) {
+	for (i = 0; i < RM9200_USB_DIV_TAB_SIZE; i++) {
+		unsigned long tmp_parent_rate;
+
 		if (!usb->divisors[i])
 			continue;
-		tmprate = *parent_rate / usb->divisors[i];
+
+		tmp_parent_rate = rate * usb->divisors[i];
+		tmp_parent_rate = __clk_round_rate(parent, tmp_parent_rate);
+		tmprate = tmp_parent_rate / usb->divisors[i];
 		if (tmprate < rate)
 			tmpdiff = rate - tmprate;
 		else
@@ -256,6 +262,7 @@ static long at91rm9200_clk_usb_round_rate(struct clk_hw *hw, unsigned long rate,
 		if (bestdiff < 0 || bestdiff > tmpdiff) {
 			bestrate = tmprate;
 			bestdiff = tmpdiff;
+			*parent_rate = tmp_parent_rate;
 		}
 
 		if (!bestdiff)
@@ -311,7 +318,7 @@ at91rm9200_clk_register_usb(struct at91_pmc *pmc, const char *name,
 	init.ops = &at91rm9200_usb_ops;
 	init.parent_names = &parent_name;
 	init.num_parents = 1;
-	init.flags = 0;
+	init.flags = CLK_SET_RATE_PARENT;
 
 	usb->hw.init = &init;
 	usb->pmc = pmc;
-- 
1.9.1

[PATCH 5/5] clk: at91: fix div by zero in USB clock driver

[Boris BREZILLON]

Test rate value before calculating the div value to avoid div by zero.

Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
Reported-by: Ga?l PORTAY <gael.portay@gmail.com>
---
 drivers/clk/at91/clk-usb.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/at91/clk-usb.c b/drivers/clk/at91/clk-usb.c
index 1838777..24b5b02 100644
--- a/drivers/clk/at91/clk-usb.c
+++ b/drivers/clk/at91/clk-usb.c
@@ -279,10 +279,13 @@ static int at91rm9200_clk_usb_set_rate(struct clk_hw *hw, unsigned long rate,
 	int i;
 	struct at91rm9200_clk_usb *usb = to_at91rm9200_clk_usb(hw);
 	struct at91_pmc *pmc = usb->pmc;
-	unsigned long div = parent_rate / rate;
+	unsigned long div;
 
-	if (parent_rate % rate)
+	if (!rate || parent_rate % rate)
 		return -EINVAL;
+
+	div = parent_rate / rate;
+
 	for (i = 0; i < RM9200_USB_DIV_TAB_SIZE; i++) {
 		if (usb->divisors[i] == div) {
 			tmp = pmc_read(pmc, AT91_CKGR_PLLBR) &
-- 
1.9.1

[PATCH 0/5] clk: at91: fix USB clk support on at91rm9200/sam9 SoCs

[Mike Turquette]

Quoting Boris BREZILLON (2014-09-02 00:50:13)
> Hi,
> 
> This patch series fixes several bugs in the PLL driver preventing a proper
> set_rate on the PLL clk.
> It also enables propagation of set_rate request on the USB clk in order to
> configure the PLL rate according to the USB block requirement (48 MHz).
> 
> Note that existing kernels, relying on the PLL configuration made by the
> the bootloader should not be impacted by this bug, but others (those
> directly booting from at91bootstrap or not enabling USB support in the
> bootloader) will be.
> 
> This bug was reported by Ga?l, who's directly booting the kernel from the
> bootstrap.

Applied to clk-next.

Regards,
Mike

> 
> Best Regards,
> 
> Boris
> 
> Boris BREZILLON (5):
>   clk: at91: fix PLL_MAX_COUNT macro definition
>   clk: at91: rework PLL rate calculation
>   clk: at91: fix recalc_rate implementation of PLL driver
>   clk: at91: rework rm9200 USB clock to propagate set_rate to the parent
>     clk
>   clk: at91: fix div by zero in USB clock driver
> 
>  drivers/clk/at91/clk-pll.c | 160 +++++++++++++++++++++++----------------------
>  drivers/clk/at91/clk-usb.c |  20 ++++--
>  2 files changed, 96 insertions(+), 84 deletions(-)
> 
> -- 
> 1.9.1
> 

[PATCH 0/5] clk: at91: fix USB clk support on at91rm9200/sam9 SoCs

[Boris BREZILLON]

Hi Mike,

On Tue, 02 Sep 2014 15:38:46 -0700
Mike Turquette <mturquette@linaro.org> wrote:

> Quoting Boris BREZILLON (2014-09-02 00:50:13)
> > Hi,
> > 
> > This patch series fixes several bugs in the PLL driver preventing a proper
> > set_rate on the PLL clk.
> > It also enables propagation of set_rate request on the USB clk in order to
> > configure the PLL rate according to the USB block requirement (48 MHz).
> > 
> > Note that existing kernels, relying on the PLL configuration made by the
> > the bootloader should not be impacted by this bug, but others (those
> > directly booting from at91bootstrap or not enabling USB support in the
> > bootloader) will be.
> > 
> > This bug was reported by Ga?l, who's directly booting the kernel from the
> > bootstrap.
> 
> Applied to clk-next.

Thanks for applying this series to clk-next, but I wonder if we
shouldn't try to get this merged in 3.17 (in order to avoid back-porting
these fixes to 3.17 stable branch).

I know I said it shouldn't impact that much users, but IMHO we
shouldn't rely on this assumption.

Let me know if you're okay to take these patches in clk-fixes, because
this patch [1] needs to be applied first (I guess Nicolas will take it
through his -fixes branch).

Best Regards,

Boris

[1]https://lkml.org/lkml/2014/9/1/634

> 
> Regards,
> Mike
> 
> > 
> > Best Regards,
> > 
> > Boris
> > 
> > Boris BREZILLON (5):
> >   clk: at91: fix PLL_MAX_COUNT macro definition
> >   clk: at91: rework PLL rate calculation
> >   clk: at91: fix recalc_rate implementation of PLL driver
> >   clk: at91: rework rm9200 USB clock to propagate set_rate to the parent
> >     clk
> >   clk: at91: fix div by zero in USB clock driver
> > 
> >  drivers/clk/at91/clk-pll.c | 160 +++++++++++++++++++++++----------------------
> >  drivers/clk/at91/clk-usb.c |  20 ++++--
> >  2 files changed, 96 insertions(+), 84 deletions(-)
> > 
> > -- 
> > 1.9.1
> > 



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

[PATCH 0/5] clk: at91: fix USB clk support on at91rm9200/sam9 SoCs

[Boris BREZILLON]

Hi Mike,

On Wed, 03 Sep 2014 10:20:05 -0700
Mike Turquette <mturquette@linaro.org> wrote:

> Quoting Boris BREZILLON (2014-09-03 01:08:28)
> > Hi Mike,
> > 
> > On Tue, 02 Sep 2014 15:38:46 -0700
> > Mike Turquette <mturquette@linaro.org> wrote:
> > 
> > > Quoting Boris BREZILLON (2014-09-02 00:50:13)
> > > > Hi,
> > > > 
> > > > This patch series fixes several bugs in the PLL driver preventing a proper
> > > > set_rate on the PLL clk.
> > > > It also enables propagation of set_rate request on the USB clk in order to
> > > > configure the PLL rate according to the USB block requirement (48 MHz).
> > > > 
> > > > Note that existing kernels, relying on the PLL configuration made by the
> > > > the bootloader should not be impacted by this bug, but others (those
> > > > directly booting from at91bootstrap or not enabling USB support in the
> > > > bootloader) will be.
> > > > 
> > > > This bug was reported by Ga?l, who's directly booting the kernel from the
> > > > bootstrap.
> > > 
> > > Applied to clk-next.
> > 
> > Thanks for applying this series to clk-next, but I wonder if we
> > shouldn't try to get this merged in 3.17 (in order to avoid back-porting
> > these fixes to 3.17 stable branch).
> 
> These bug have been in there since 2013, so I guess you'll need to
> backport to any other stable branches as well?
> 
> > 
> > I know I said it shouldn't impact that much users, but IMHO we
> > shouldn't rely on this assumption.
> 
> Well, the bugs have existed since last year I guess, and they are not a
> new regression as such. In fact I think that these problems have existed
> since drivers/clk/at91/clk-pll.c and drivers/clk/at91/clk-usb.c were
> introduced, no?

Well, actually the code is here since the beginning, but it's only used
since 3.17: these fixes are touching at91sam9260/at91rm9200 specific
clks code, and these 2 families have been moved to the CCF in 3.17 (DT
node definitions and CCF Kconfig option selection for these SoCs).
I'm not sure we have to backport the fixes in this case.

> 
> > 
> > Let me know if you're okay to take these patches in clk-fixes, because
> > this patch [1] needs to be applied first (I guess Nicolas will take it
> > through his -fixes branch).
> 
> The dependency is another reason I am not thrilled about taking this
> through -fixes. But additionally, "rework rm9200 USB clock to propagate
> set_rate to the parent clk" is more like a new feature than a fix.

Kind of. But without this feature, USB IPs (both host and
device) won't work if the PLL is not correctly initialized by the
bootloader, so this can be considered as a bug fix.

> 
> I feel that the code here is a bit more than I usually like to take just
> for fixing a new regression, especially when this is neither a
> regression nor is it new.

I understand.

> 
> Let me know if you still disagree.

I'd certainly prefer not having to back port these "fixes" in 3.17 after
3.18 is out, but I can deal with it.

Best Regards,

Boris


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