[PATCH V4] clk: at91: PLL recalc_rate() now using cached MUL and DIV values

[PATCH V4] clk: at91: PLL recalc_rate() now using cached MUL and DIV values

[Marcin Ziemianowicz]

When a USB device is connected to the USB host port on the SAM9N12 then
you get "-62" error which seems to indicate USB replies from the device
are timing out. Based on a logic sniffer, I saw the USB bus was running
at half speed.

The PLL code uses cached MUL and DIV values which get set in set_rate()
and applied in prepare(), but the recalc_rate() function instead
queries the hardware instead of using these cached values. Therefore,
if recalc_rate() is called between a set_rate() and prepare(), the
wrong frequency is calculated and later the USB clock divider for the
SAM9N12 SOC will be configured for an incorrect clock.

In my case, the PLL hardware was set to 96 Mhz before the OHCI
driver loads, and therefore the usb clock divider was being set
to /2 even though the OHCI driver set the PLL to 48 Mhz.

As an alternative explanation, I noticed this was fixed in the past by
87e2ed338f1b ("clk: at91: fix recalc_rate implementation of PLL
driver") but the bug was later re-introduced by 1bdf02326b71 ("clk:
at91: make use of syscon/regmap internally").

Fixes: 1bdf02326b71 ("clk: at91: make use of syscon/regmap internally)
Cc: <stable@vger.kernel.org>
Signed-off-by: Marcin Ziemianowicz <marcin@ziemianowicz.com>
---
Thank you for bearing with me about this Boris.

Changes since V3:
  Fix for double returns found by kbluild test robot
  > Comments by Boris Brezillon about email formatting issues
Changes since V2:
  Removed all logging/debug messages I added
  > Comment by Boris Brezillon about my fix being wrong addressed
Changes since V1:
  Added patch set cover letter
  Shortened lines which were over >80 characters long
  > Comment by Greg Kroah-Hartman about "from" field in email addressed
  > Comment by Alan Stern about redundant debug lines addressed

 drivers/clk/at91/clk-pll.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/clk/at91/clk-pll.c b/drivers/clk/at91/clk-pll.c
index 7d3223fc..72b6091e 100644
--- a/drivers/clk/at91/clk-pll.c
+++ b/drivers/clk/at91/clk-pll.c
@@ -132,19 +132,8 @@ static unsigned long clk_pll_recalc_rate(struct clk_hw *hw,
 					 unsigned long parent_rate)
 {
 	struct clk_pll *pll = to_clk_pll(hw);
-	unsigned int pllr;
-	u16 mul;
-	u8 div;
-
-	regmap_read(pll->regmap, PLL_REG(pll->id), &pllr);
-
-	div = PLL_DIV(pllr);
-	mul = PLL_MUL(pllr, pll->layout);
-
-	if (!div || !mul)
-		return 0;
 
-	return (parent_rate / div) * (mul + 1);
+	return (parent_rate / pll->div) * (pll->mul + 1);
 }
 
 static long clk_pll_get_best_div_mul(struct clk_pll *pll, unsigned long rate,
-- 
2.17.0

[PATCH V4] clk: at91: PLL recalc_rate() now using cached MUL and DIV values

[Marcin Ziemianowicz]

When a USB device is connected to the USB host port on the SAM9N12 then
you get "-62" error which seems to indicate USB replies from the device
are timing out. Based on a logic sniffer, I saw the USB bus was running
at half speed.

The PLL code uses cached MUL and DIV values which get set in set_rate()
and applied in prepare(), but the recalc_rate() function instead
queries the hardware instead of using these cached values. Therefore,
if recalc_rate() is called between a set_rate() and prepare(), the
wrong frequency is calculated and later the USB clock divider for the
SAM9N12 SOC will be configured for an incorrect clock.

In my case, the PLL hardware was set to 96 Mhz before the OHCI
driver loads, and therefore the usb clock divider was being set
to /2 even though the OHCI driver set the PLL to 48 Mhz.

As an alternative explanation, I noticed this was fixed in the past by
87e2ed338f1b ("clk: at91: fix recalc_rate implementation of PLL
driver") but the bug was later re-introduced by 1bdf02326b71 ("clk:
at91: make use of syscon/regmap internally").

Fixes: 1bdf02326b71 ("clk: at91: make use of syscon/regmap internally)
Cc: <stable@vger.kernel.org>
Signed-off-by: Marcin Ziemianowicz <marcin@ziemianowicz.com>
---
Thank you for bearing with me about this Boris.

Changes since V3:
  Fix for double returns found by kbluild test robot
  > Comments by Boris Brezillon about email formatting issues
Changes since V2:
  Removed all logging/debug messages I added
  > Comment by Boris Brezillon about my fix being wrong addressed
Changes since V1:
  Added patch set cover letter
  Shortened lines which were over >80 characters long
  > Comment by Greg Kroah-Hartman about "from" field in email addressed
  > Comment by Alan Stern about redundant debug lines addressed

 drivers/clk/at91/clk-pll.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/clk/at91/clk-pll.c b/drivers/clk/at91/clk-pll.c
index 7d3223fc..72b6091e 100644
--- a/drivers/clk/at91/clk-pll.c
+++ b/drivers/clk/at91/clk-pll.c
@@ -132,19 +132,8 @@ static unsigned long clk_pll_recalc_rate(struct clk_hw *hw,
 					 unsigned long parent_rate)
 {
 	struct clk_pll *pll = to_clk_pll(hw);
-	unsigned int pllr;
-	u16 mul;
-	u8 div;
-
-	regmap_read(pll->regmap, PLL_REG(pll->id), &pllr);
-
-	div = PLL_DIV(pllr);
-	mul = PLL_MUL(pllr, pll->layout);
-
-	if (!div || !mul)
-		return 0;
 
-	return (parent_rate / div) * (mul + 1);
+	return (parent_rate / pll->div) * (pll->mul + 1);
 }
 
 static long clk_pll_get_best_div_mul(struct clk_pll *pll, unsigned long rate,
-- 
2.17.0

Re: [PATCH V4] clk: at91: PLL recalc_rate() now using cached MUL and DIV values

[Boris Brezillon]

On Sun, 29 Apr 2018 15:01:11 -0400
Marcin Ziemianowicz <marcin@ziemianowicz.com> wrote:

> When a USB device is connected to the USB host port on the SAM9N12 then
> you get "-62" error which seems to indicate USB replies from the device
> are timing out. Based on a logic sniffer, I saw the USB bus was running
> at half speed.
> 
> The PLL code uses cached MUL and DIV values which get set in set_rate()
> and applied in prepare(), but the recalc_rate() function instead
> queries the hardware instead of using these cached values. Therefore,
> if recalc_rate() is called between a set_rate() and prepare(), the
> wrong frequency is calculated and later the USB clock divider for the
> SAM9N12 SOC will be configured for an incorrect clock.
> 
> In my case, the PLL hardware was set to 96 Mhz before the OHCI
> driver loads, and therefore the usb clock divider was being set
> to /2 even though the OHCI driver set the PLL to 48 Mhz.
> 
> As an alternative explanation, I noticed this was fixed in the past by
> 87e2ed338f1b ("clk: at91: fix recalc_rate implementation of PLL
> driver") but the bug was later re-introduced by 1bdf02326b71 ("clk:
> at91: make use of syscon/regmap internally").
> 
> Fixes: 1bdf02326b71 ("clk: at91: make use of syscon/regmap internally)
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Marcin Ziemianowicz <marcin@ziemianowicz.com>

Acked-by: Boris Brezillon <boris.brezillon@bootlin.com>

> ---
> Thank you for bearing with me about this Boris.
> 
> Changes since V3:
>   Fix for double returns found by kbluild test robot
>   > Comments by Boris Brezillon about email formatting issues  
> Changes since V2:
>   Removed all logging/debug messages I added
>   > Comment by Boris Brezillon about my fix being wrong addressed  
> Changes since V1:
>   Added patch set cover letter
>   Shortened lines which were over >80 characters long
>   > Comment by Greg Kroah-Hartman about "from" field in email addressed
>   > Comment by Alan Stern about redundant debug lines addressed  
> 
>  drivers/clk/at91/clk-pll.c | 13 +------------
>  1 file changed, 1 insertion(+), 12 deletions(-)
> 
> diff --git a/drivers/clk/at91/clk-pll.c b/drivers/clk/at91/clk-pll.c
> index 7d3223fc..72b6091e 100644
> --- a/drivers/clk/at91/clk-pll.c
> +++ b/drivers/clk/at91/clk-pll.c
> @@ -132,19 +132,8 @@ static unsigned long clk_pll_recalc_rate(struct clk_hw *hw,
>  					 unsigned long parent_rate)
>  {
>  	struct clk_pll *pll = to_clk_pll(hw);
> -	unsigned int pllr;
> -	u16 mul;
> -	u8 div;
> -
> -	regmap_read(pll->regmap, PLL_REG(pll->id), &pllr);
> -
> -	div = PLL_DIV(pllr);
> -	mul = PLL_MUL(pllr, pll->layout);
> -
> -	if (!div || !mul)
> -		return 0;
>  
> -	return (parent_rate / div) * (mul + 1);
> +	return (parent_rate / pll->div) * (pll->mul + 1);
>  }
>  
>  static long clk_pll_get_best_div_mul(struct clk_pll *pll, unsigned long rate,

[PATCH V4] clk: at91: PLL recalc_rate() now using cached MUL and DIV values

[Boris Brezillon]

On Sun, 29 Apr 2018 15:01:11 -0400
Marcin Ziemianowicz <marcin@ziemianowicz.com> wrote:

> When a USB device is connected to the USB host port on the SAM9N12 then
> you get "-62" error which seems to indicate USB replies from the device
> are timing out. Based on a logic sniffer, I saw the USB bus was running
> at half speed.
> 
> The PLL code uses cached MUL and DIV values which get set in set_rate()
> and applied in prepare(), but the recalc_rate() function instead
> queries the hardware instead of using these cached values. Therefore,
> if recalc_rate() is called between a set_rate() and prepare(), the
> wrong frequency is calculated and later the USB clock divider for the
> SAM9N12 SOC will be configured for an incorrect clock.
> 
> In my case, the PLL hardware was set to 96 Mhz before the OHCI
> driver loads, and therefore the usb clock divider was being set
> to /2 even though the OHCI driver set the PLL to 48 Mhz.
> 
> As an alternative explanation, I noticed this was fixed in the past by
> 87e2ed338f1b ("clk: at91: fix recalc_rate implementation of PLL
> driver") but the bug was later re-introduced by 1bdf02326b71 ("clk:
> at91: make use of syscon/regmap internally").
> 
> Fixes: 1bdf02326b71 ("clk: at91: make use of syscon/regmap internally)
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Marcin Ziemianowicz <marcin@ziemianowicz.com>

Acked-by: Boris Brezillon <boris.brezillon@bootlin.com>

> ---
> Thank you for bearing with me about this Boris.
> 
> Changes since V3:
>   Fix for double returns found by kbluild test robot
>   > Comments by Boris Brezillon about email formatting issues  
> Changes since V2:
>   Removed all logging/debug messages I added
>   > Comment by Boris Brezillon about my fix being wrong addressed  
> Changes since V1:
>   Added patch set cover letter
>   Shortened lines which were over >80 characters long
>   > Comment by Greg Kroah-Hartman about "from" field in email addressed
>   > Comment by Alan Stern about redundant debug lines addressed  
> 
>  drivers/clk/at91/clk-pll.c | 13 +------------
>  1 file changed, 1 insertion(+), 12 deletions(-)
> 
> diff --git a/drivers/clk/at91/clk-pll.c b/drivers/clk/at91/clk-pll.c
> index 7d3223fc..72b6091e 100644
> --- a/drivers/clk/at91/clk-pll.c
> +++ b/drivers/clk/at91/clk-pll.c
> @@ -132,19 +132,8 @@ static unsigned long clk_pll_recalc_rate(struct clk_hw *hw,
>  					 unsigned long parent_rate)
>  {
>  	struct clk_pll *pll = to_clk_pll(hw);
> -	unsigned int pllr;
> -	u16 mul;
> -	u8 div;
> -
> -	regmap_read(pll->regmap, PLL_REG(pll->id), &pllr);
> -
> -	div = PLL_DIV(pllr);
> -	mul = PLL_MUL(pllr, pll->layout);
> -
> -	if (!div || !mul)
> -		return 0;
>  
> -	return (parent_rate / div) * (mul + 1);
> +	return (parent_rate / pll->div) * (pll->mul + 1);
>  }
>  
>  static long clk_pll_get_best_div_mul(struct clk_pll *pll, unsigned long rate,

Re: [PATCH V4] clk: at91: PLL recalc_rate() now using cached MUL and DIV values

[Marcin Ziemianowicz]

On Mon, Apr 30, 2018 at 07:58:47AM +0200, Boris Brezillon wrote:
> On Sun, 29 Apr 2018 15:01:11 -0400
> Marcin Ziemianowicz <marcin@ziemianowicz.com> wrote:
> 
> > When a USB device is connected to the USB host port on the SAM9N12 then
> > you get "-62" error which seems to indicate USB replies from the device
> > are timing out. Based on a logic sniffer, I saw the USB bus was running
> > at half speed.
> > 
> > The PLL code uses cached MUL and DIV values which get set in set_rate()
> > and applied in prepare(), but the recalc_rate() function instead
> > queries the hardware instead of using these cached values. Therefore,
> > if recalc_rate() is called between a set_rate() and prepare(), the
> > wrong frequency is calculated and later the USB clock divider for the
> > SAM9N12 SOC will be configured for an incorrect clock.
> > 
> > In my case, the PLL hardware was set to 96 Mhz before the OHCI
> > driver loads, and therefore the usb clock divider was being set
> > to /2 even though the OHCI driver set the PLL to 48 Mhz.
> > 
> > As an alternative explanation, I noticed this was fixed in the past by
> > 87e2ed338f1b ("clk: at91: fix recalc_rate implementation of PLL
> > driver") but the bug was later re-introduced by 1bdf02326b71 ("clk:
> > at91: make use of syscon/regmap internally").
> > 
> > Fixes: 1bdf02326b71 ("clk: at91: make use of syscon/regmap internally)
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Marcin Ziemianowicz <marcin@ziemianowicz.com>
> 
> Acked-by: Boris Brezillon <boris.brezillon@bootlin.com>

Apologies for being a bother, but since it's been a bit over a week,
should I do something with this now that it has been ACK'd? I was thinking
I would see it somewhere on the git group repo but am not seeing it there
yet. Googling says that there is a "review cycle" for some maintainers, but
I am not clear on if I need to initiate it manually or anything of the sort.

https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/ 

> 
> > ---
> > Thank you for bearing with me about this Boris.
> > 
> > Changes since V3:
> >   Fix for double returns found by kbluild test robot
> >   > Comments by Boris Brezillon about email formatting issues  
> > Changes since V2:
> >   Removed all logging/debug messages I added
> >   > Comment by Boris Brezillon about my fix being wrong addressed  
> > Changes since V1:
> >   Added patch set cover letter
> >   Shortened lines which were over >80 characters long
> >   > Comment by Greg Kroah-Hartman about "from" field in email addressed
> >   > Comment by Alan Stern about redundant debug lines addressed  
> > 
> >  drivers/clk/at91/clk-pll.c | 13 +------------
> >  1 file changed, 1 insertion(+), 12 deletions(-)
> > 
> > diff --git a/drivers/clk/at91/clk-pll.c b/drivers/clk/at91/clk-pll.c
> > index 7d3223fc..72b6091e 100644
> > --- a/drivers/clk/at91/clk-pll.c
> > +++ b/drivers/clk/at91/clk-pll.c
> > @@ -132,19 +132,8 @@ static unsigned long clk_pll_recalc_rate(struct clk_hw *hw,
> >  					 unsigned long parent_rate)
> >  {
> >  	struct clk_pll *pll = to_clk_pll(hw);
> > -	unsigned int pllr;
> > -	u16 mul;
> > -	u8 div;
> > -
> > -	regmap_read(pll->regmap, PLL_REG(pll->id), &pllr);
> > -
> > -	div = PLL_DIV(pllr);
> > -	mul = PLL_MUL(pllr, pll->layout);
> > -
> > -	if (!div || !mul)
> > -		return 0;
> >  
> > -	return (parent_rate / div) * (mul + 1);
> > +	return (parent_rate / pll->div) * (pll->mul + 1);
> >  }
> >  
> >  static long clk_pll_get_best_div_mul(struct clk_pll *pll, unsigned long rate,
> 

[PATCH V4] clk: at91: PLL recalc_rate() now using cached MUL and DIV values

[Marcin Ziemianowicz]

On Mon, Apr 30, 2018 at 07:58:47AM +0200, Boris Brezillon wrote:
> On Sun, 29 Apr 2018 15:01:11 -0400
> Marcin Ziemianowicz <marcin@ziemianowicz.com> wrote:
> 
> > When a USB device is connected to the USB host port on the SAM9N12 then
> > you get "-62" error which seems to indicate USB replies from the device
> > are timing out. Based on a logic sniffer, I saw the USB bus was running
> > at half speed.
> > 
> > The PLL code uses cached MUL and DIV values which get set in set_rate()
> > and applied in prepare(), but the recalc_rate() function instead
> > queries the hardware instead of using these cached values. Therefore,
> > if recalc_rate() is called between a set_rate() and prepare(), the
> > wrong frequency is calculated and later the USB clock divider for the
> > SAM9N12 SOC will be configured for an incorrect clock.
> > 
> > In my case, the PLL hardware was set to 96 Mhz before the OHCI
> > driver loads, and therefore the usb clock divider was being set
> > to /2 even though the OHCI driver set the PLL to 48 Mhz.
> > 
> > As an alternative explanation, I noticed this was fixed in the past by
> > 87e2ed338f1b ("clk: at91: fix recalc_rate implementation of PLL
> > driver") but the bug was later re-introduced by 1bdf02326b71 ("clk:
> > at91: make use of syscon/regmap internally").
> > 
> > Fixes: 1bdf02326b71 ("clk: at91: make use of syscon/regmap internally)
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Marcin Ziemianowicz <marcin@ziemianowicz.com>
> 
> Acked-by: Boris Brezillon <boris.brezillon@bootlin.com>

Apologies for being a bother, but since it's been a bit over a week,
should I do something with this now that it has been ACK'd? I was thinking
I would see it somewhere on the git group repo but am not seeing it there
yet. Googling says that there is a "review cycle" for some maintainers, but
I am not clear on if I need to initiate it manually or anything of the sort.

https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/ 

> 
> > ---
> > Thank you for bearing with me about this Boris.
> > 
> > Changes since V3:
> >   Fix for double returns found by kbluild test robot
> >   > Comments by Boris Brezillon about email formatting issues  
> > Changes since V2:
> >   Removed all logging/debug messages I added
> >   > Comment by Boris Brezillon about my fix being wrong addressed  
> > Changes since V1:
> >   Added patch set cover letter
> >   Shortened lines which were over >80 characters long
> >   > Comment by Greg Kroah-Hartman about "from" field in email addressed
> >   > Comment by Alan Stern about redundant debug lines addressed  
> > 
> >  drivers/clk/at91/clk-pll.c | 13 +------------
> >  1 file changed, 1 insertion(+), 12 deletions(-)
> > 
> > diff --git a/drivers/clk/at91/clk-pll.c b/drivers/clk/at91/clk-pll.c
> > index 7d3223fc..72b6091e 100644
> > --- a/drivers/clk/at91/clk-pll.c
> > +++ b/drivers/clk/at91/clk-pll.c
> > @@ -132,19 +132,8 @@ static unsigned long clk_pll_recalc_rate(struct clk_hw *hw,
> >  					 unsigned long parent_rate)
> >  {
> >  	struct clk_pll *pll = to_clk_pll(hw);
> > -	unsigned int pllr;
> > -	u16 mul;
> > -	u8 div;
> > -
> > -	regmap_read(pll->regmap, PLL_REG(pll->id), &pllr);
> > -
> > -	div = PLL_DIV(pllr);
> > -	mul = PLL_MUL(pllr, pll->layout);
> > -
> > -	if (!div || !mul)
> > -		return 0;
> >  
> > -	return (parent_rate / div) * (mul + 1);
> > +	return (parent_rate / pll->div) * (pll->mul + 1);
> >  }
> >  
> >  static long clk_pll_get_best_div_mul(struct clk_pll *pll, unsigned long rate,
> 

Re: [PATCH V4] clk: at91: PLL recalc_rate() now using cached MUL and DIV values

[Stephen Boyd]

Quoting Marcin Ziemianowicz (2018-05-08 21:32:05)
> On Mon, Apr 30, 2018 at 07:58:47AM +0200, Boris Brezillon wrote:
> > On Sun, 29 Apr 2018 15:01:11 -0400
> > Marcin Ziemianowicz <marcin@ziemianowicz.com> wrote:
> > 
> > > When a USB device is connected to the USB host port on the SAM9N12 then
> > > you get "-62" error which seems to indicate USB replies from the device
> > > are timing out. Based on a logic sniffer, I saw the USB bus was running
> > > at half speed.
> > > 
> > > The PLL code uses cached MUL and DIV values which get set in set_rate()
> > > and applied in prepare(), but the recalc_rate() function instead
> > > queries the hardware instead of using these cached values. Therefore,
> > > if recalc_rate() is called between a set_rate() and prepare(), the
> > > wrong frequency is calculated and later the USB clock divider for the
> > > SAM9N12 SOC will be configured for an incorrect clock.
> > > 
> > > In my case, the PLL hardware was set to 96 Mhz before the OHCI
> > > driver loads, and therefore the usb clock divider was being set
> > > to /2 even though the OHCI driver set the PLL to 48 Mhz.
> > > 
> > > As an alternative explanation, I noticed this was fixed in the past by
> > > 87e2ed338f1b ("clk: at91: fix recalc_rate implementation of PLL
> > > driver") but the bug was later re-introduced by 1bdf02326b71 ("clk:
> > > at91: make use of syscon/regmap internally").
> > > 
> > > Fixes: 1bdf02326b71 ("clk: at91: make use of syscon/regmap internally)
> > > Cc: <stable@vger.kernel.org>
> > > Signed-off-by: Marcin Ziemianowicz <marcin@ziemianowicz.com>
> > 
> > Acked-by: Boris Brezillon <boris.brezillon@bootlin.com>
> 
> Apologies for being a bother, but since it's been a bit over a week,
> should I do something with this now that it has been ACK'd? I was thinking
> I would see it somewhere on the git group repo but am not seeing it there
> yet. Googling says that there is a "review cycle" for some maintainers, but
> I am not clear on if I need to initiate it manually or anything of the sort.
> 

I'll apply it to clk-next. Should appear in linux-next in day or so.

Re: [PATCH V4] clk: at91: PLL recalc_rate() now using cached MUL and DIV values

[Stephen Boyd]

Quoting Marcin Ziemianowicz (2018-05-08 21:32:05)
> On Mon, Apr 30, 2018 at 07:58:47AM +0200, Boris Brezillon wrote:
> > On Sun, 29 Apr 2018 15:01:11 -0400
> > Marcin Ziemianowicz <marcin@ziemianowicz.com> wrote:
> > =

> > > When a USB device is connected to the USB host port on the SAM9N12 th=
en
> > > you get "-62" error which seems to indicate USB replies from the devi=
ce
> > > are timing out. Based on a logic sniffer, I saw the USB bus was runni=
ng
> > > at half speed.
> > > =

> > > The PLL code uses cached MUL and DIV values which get set in set_rate=
()
> > > and applied in prepare(), but the recalc_rate() function instead
> > > queries the hardware instead of using these cached values. Therefore,
> > > if recalc_rate() is called between a set_rate() and prepare(), the
> > > wrong frequency is calculated and later the USB clock divider for the
> > > SAM9N12 SOC will be configured for an incorrect clock.
> > > =

> > > In my case, the PLL hardware was set to 96 Mhz before the OHCI
> > > driver loads, and therefore the usb clock divider was being set
> > > to /2 even though the OHCI driver set the PLL to 48 Mhz.
> > > =

> > > As an alternative explanation, I noticed this was fixed in the past by
> > > 87e2ed338f1b ("clk: at91: fix recalc_rate implementation of PLL
> > > driver") but the bug was later re-introduced by 1bdf02326b71 ("clk:
> > > at91: make use of syscon/regmap internally").
> > > =

> > > Fixes: 1bdf02326b71 ("clk: at91: make use of syscon/regmap internally)
> > > Cc: <stable@vger.kernel.org>
> > > Signed-off-by: Marcin Ziemianowicz <marcin@ziemianowicz.com>
> > =

> > Acked-by: Boris Brezillon <boris.brezillon@bootlin.com>
> =

> Apologies for being a bother, but since it's been a bit over a week,
> should I do something with this now that it has been ACK'd? I was thinking
> I would see it somewhere on the git group repo but am not seeing it there
> yet. Googling says that there is a "review cycle" for some maintainers, b=
ut
> I am not clear on if I need to initiate it manually or anything of the so=
rt.
> =


I'll apply it to clk-next. Should appear in linux-next in day or so.

[PATCH V4] clk: at91: PLL recalc_rate() now using cached MUL and DIV values

[Stephen Boyd]

Quoting Marcin Ziemianowicz (2018-05-08 21:32:05)
> On Mon, Apr 30, 2018 at 07:58:47AM +0200, Boris Brezillon wrote:
> > On Sun, 29 Apr 2018 15:01:11 -0400
> > Marcin Ziemianowicz <marcin@ziemianowicz.com> wrote:
> > 
> > > When a USB device is connected to the USB host port on the SAM9N12 then
> > > you get "-62" error which seems to indicate USB replies from the device
> > > are timing out. Based on a logic sniffer, I saw the USB bus was running
> > > at half speed.
> > > 
> > > The PLL code uses cached MUL and DIV values which get set in set_rate()
> > > and applied in prepare(), but the recalc_rate() function instead
> > > queries the hardware instead of using these cached values. Therefore,
> > > if recalc_rate() is called between a set_rate() and prepare(), the
> > > wrong frequency is calculated and later the USB clock divider for the
> > > SAM9N12 SOC will be configured for an incorrect clock.
> > > 
> > > In my case, the PLL hardware was set to 96 Mhz before the OHCI
> > > driver loads, and therefore the usb clock divider was being set
> > > to /2 even though the OHCI driver set the PLL to 48 Mhz.
> > > 
> > > As an alternative explanation, I noticed this was fixed in the past by
> > > 87e2ed338f1b ("clk: at91: fix recalc_rate implementation of PLL
> > > driver") but the bug was later re-introduced by 1bdf02326b71 ("clk:
> > > at91: make use of syscon/regmap internally").
> > > 
> > > Fixes: 1bdf02326b71 ("clk: at91: make use of syscon/regmap internally)
> > > Cc: <stable@vger.kernel.org>
> > > Signed-off-by: Marcin Ziemianowicz <marcin@ziemianowicz.com>
> > 
> > Acked-by: Boris Brezillon <boris.brezillon@bootlin.com>
> 
> Apologies for being a bother, but since it's been a bit over a week,
> should I do something with this now that it has been ACK'd? I was thinking
> I would see it somewhere on the git group repo but am not seeing it there
> yet. Googling says that there is a "review cycle" for some maintainers, but
> I am not clear on if I need to initiate it manually or anything of the sort.
> 

I'll apply it to clk-next. Should appear in linux-next in day or so.

Re: [PATCH V4] clk: at91: PLL recalc_rate() now using cached MUL and DIV values

[Stephen Boyd]

Quoting Marcin Ziemianowicz (2018-04-29 12:01:11)
> When a USB device is connected to the USB host port on the SAM9N12 then
> you get "-62" error which seems to indicate USB replies from the device
> are timing out. Based on a logic sniffer, I saw the USB bus was running
> at half speed.
> 
> The PLL code uses cached MUL and DIV values which get set in set_rate()
> and applied in prepare(), but the recalc_rate() function instead
> queries the hardware instead of using these cached values. Therefore,
> if recalc_rate() is called between a set_rate() and prepare(), the
> wrong frequency is calculated and later the USB clock divider for the
> SAM9N12 SOC will be configured for an incorrect clock.
> 
> In my case, the PLL hardware was set to 96 Mhz before the OHCI
> driver loads, and therefore the usb clock divider was being set
> to /2 even though the OHCI driver set the PLL to 48 Mhz.
> 
> As an alternative explanation, I noticed this was fixed in the past by
> 87e2ed338f1b ("clk: at91: fix recalc_rate implementation of PLL
> driver") but the bug was later re-introduced by 1bdf02326b71 ("clk:
> at91: make use of syscon/regmap internally").
> 
> Fixes: 1bdf02326b71 ("clk: at91: make use of syscon/regmap internally)
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Marcin Ziemianowicz <marcin@ziemianowicz.com>
> ---

Applied to clk-next

Re: [PATCH V4] clk: at91: PLL recalc_rate() now using cached MUL and DIV values

[Stephen Boyd]

Quoting Marcin Ziemianowicz (2018-04-29 12:01:11)
> When a USB device is connected to the USB host port on the SAM9N12 then
> you get "-62" error which seems to indicate USB replies from the device
> are timing out. Based on a logic sniffer, I saw the USB bus was running
> at half speed.
> =

> The PLL code uses cached MUL and DIV values which get set in set_rate()
> and applied in prepare(), but the recalc_rate() function instead
> queries the hardware instead of using these cached values. Therefore,
> if recalc_rate() is called between a set_rate() and prepare(), the
> wrong frequency is calculated and later the USB clock divider for the
> SAM9N12 SOC will be configured for an incorrect clock.
> =

> In my case, the PLL hardware was set to 96 Mhz before the OHCI
> driver loads, and therefore the usb clock divider was being set
> to /2 even though the OHCI driver set the PLL to 48 Mhz.
> =

> As an alternative explanation, I noticed this was fixed in the past by
> 87e2ed338f1b ("clk: at91: fix recalc_rate implementation of PLL
> driver") but the bug was later re-introduced by 1bdf02326b71 ("clk:
> at91: make use of syscon/regmap internally").
> =

> Fixes: 1bdf02326b71 ("clk: at91: make use of syscon/regmap internally)
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Marcin Ziemianowicz <marcin@ziemianowicz.com>
> ---

Applied to clk-next

[PATCH V4] clk: at91: PLL recalc_rate() now using cached MUL and DIV values

[Stephen Boyd]

Quoting Marcin Ziemianowicz (2018-04-29 12:01:11)
> When a USB device is connected to the USB host port on the SAM9N12 then
> you get "-62" error which seems to indicate USB replies from the device
> are timing out. Based on a logic sniffer, I saw the USB bus was running
> at half speed.
> 
> The PLL code uses cached MUL and DIV values which get set in set_rate()
> and applied in prepare(), but the recalc_rate() function instead
> queries the hardware instead of using these cached values. Therefore,
> if recalc_rate() is called between a set_rate() and prepare(), the
> wrong frequency is calculated and later the USB clock divider for the
> SAM9N12 SOC will be configured for an incorrect clock.
> 
> In my case, the PLL hardware was set to 96 Mhz before the OHCI
> driver loads, and therefore the usb clock divider was being set
> to /2 even though the OHCI driver set the PLL to 48 Mhz.
> 
> As an alternative explanation, I noticed this was fixed in the past by
> 87e2ed338f1b ("clk: at91: fix recalc_rate implementation of PLL
> driver") but the bug was later re-introduced by 1bdf02326b71 ("clk:
> at91: make use of syscon/regmap internally").
> 
> Fixes: 1bdf02326b71 ("clk: at91: make use of syscon/regmap internally)
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Marcin Ziemianowicz <marcin@ziemianowicz.com>
> ---

Applied to clk-next

[PATCH V4] clk: at91: PLL recalc_rate() now using cached MUL and DIV values

[Marcin Ziemianowicz]

When a USB device is connected to the USB host port on the SAM9N12 then
you get "-62" error which seems to indicate USB replies from the device
are timing out. Based on a logic sniffer, I saw the USB bus was running
at half speed.

The PLL code uses cached MUL and DIV values which get set in set_rate()
and applied in prepare(), but the recalc_rate() function instead
queries the hardware instead of using these cached values. Therefore,
if recalc_rate() is called between a set_rate() and prepare(), the
wrong frequency is calculated and later the USB clock divider for the
SAM9N12 SOC will be configured for an incorrect clock.

In my case, the PLL hardware was set to 96 Mhz before the OHCI
driver loads, and therefore the usb clock divider was being set
to /2 even though the OHCI driver set the PLL to 48 Mhz.

As an alternative explanation, I noticed this was fixed in the past by
87e2ed338f1b ("clk: at91: fix recalc_rate implementation of PLL
driver") but the bug was later re-introduced by 1bdf02326b71 ("clk:
at91: make use of syscon/regmap internally").

Fixes: 1bdf02326b71 ("clk: at91: make use of syscon/regmap internally)
Cc: <stable@vger.kernel.org>
Signed-off-by: Marcin Ziemianowicz <marcin@ziemianowicz.com>
---
Thank you for bearing with me about this Boris.

Changes since V3:
  Fix for double returns found by kbluild test robot
  > Comments by Boris Brezillon about email formatting issues
Changes since V2:
  Removed all logging/debug messages I added
  > Comment by Boris Brezillon about my fix being wrong addressed
Changes since V1:
  Added patch set cover letter
  Shortened lines which were over >80 characters long
  > Comment by Greg Kroah-Hartman about "from" field in email addressed
  > Comment by Alan Stern about redundant debug lines addressed

 drivers/clk/at91/clk-pll.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/clk/at91/clk-pll.c b/drivers/clk/at91/clk-pll.c
index 7d3223fc..72b6091e 100644
--- a/drivers/clk/at91/clk-pll.c
+++ b/drivers/clk/at91/clk-pll.c
@@ -132,19 +132,8 @@ static unsigned long clk_pll_recalc_rate(struct clk_hw *hw,
 					 unsigned long parent_rate)
 {
 	struct clk_pll *pll = to_clk_pll(hw);
-	unsigned int pllr;
-	u16 mul;
-	u8 div;
-
-	regmap_read(pll->regmap, PLL_REG(pll->id), &pllr);
-
-	div = PLL_DIV(pllr);
-	mul = PLL_MUL(pllr, pll->layout);
-
-	if (!div || !mul)
-		return 0;
 
-	return (parent_rate / div) * (mul + 1);
+	return (parent_rate / pll->div) * (pll->mul + 1);
 }
 
 static long clk_pll_get_best_div_mul(struct clk_pll *pll, unsigned long rate,
-- 
2.17.0