clk: at91: PLL recalc_rate() now using cached MUL and DIV values
Subsystems (3)
PATCH v4 1/1 clk: at91: PLL recalc_rate() now using cached MUL and DIV values
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").
---
Thank you for bearing with me about this Boris.
Changes since V3:
Fix for double returns found by kbluild test robot
Quoted 1 line
Comments by Boris Brezillon about email formatting issues
Changes since V2:
Removed all logging/debug messages I added
Quoted 1 line
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
Quoted 2 lines
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(-)
drivers/clk/at91/clk-pll.c +1 -12
@@ -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,
Signature · 1 line
2.17.0
Quoted 16 lines · On Sun, 29 Apr 2018 15:01:11 -0400 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 inprepare(), but therecalc_rate()function instead queries the hardware instead of using these cached values. Therefore, ifrecalc_rate()is called between aset_rate()andprepare(), 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.
Quoted 8 lines
As an alternative explanation, I noticed this was fixed in the past by
87e2ed338f1b("clk: at91: fixrecalc_rateimplementation of PLL driver") but the bug was later re-introduced by1bdf02326b71("clk:
at91: make use of syscon/regmap internally").Fixes 1bdf02326b71 ("clk: at91: make use of syscon/regmap internally)Cc <[email protected]>Signed-off-by Marcin Ziemianowicz <[email protected]>
Quoted 43 lines
---
Thank you for bearing with me about this Boris.Changes since V3:
Fix for double returns found by kbluild test robotQuoted 1 line
Comments by Boris Brezillon about email formatting issues
Changes since V2:
Removed all logging/debug messages I addedQuoted 1 line
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 longQuoted 2 lines
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(-)drivers/clk/at91/clk-pll.c +1
-12@@ -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,
Quoted 24 lines · Boris Brezillon
Quoted 21 lines · On Sun, 29 Apr 2018 15:01:11 -0400 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 inprepare(), but therecalc_rate()function instead queries the hardware instead of using these cached values. Therefore, ifrecalc_rate()is called between aset_rate()andprepare(), 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: fixrecalc_rateimplementation of PLL driver") but the bug was later re-introduced by1bdf02326b71("clk:
at91: make use of syscon/regmap internally").
Quoted 5 lines
Quoted 3 lines
Fixes 1bdf02326b71 ("clk: at91: make use of syscon/regmap internally)Cc <[email protected]>Signed-off-by Marcin Ziemianowicz <[email protected]>Acked-by Boris Brezillon <[email protected]>
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/
Quoted 45 lines
Quoted 44 lines
---
Thank you for bearing with me about this Boris.Changes since V3:
Fix for double returns found by kbluild test robotQuoted 1 line
Comments by Boris Brezillon about email formatting issues
Changes since V2:
Removed all logging/debug messages I addedQuoted 1 line
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 longQuoted 2 lines
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(-)drivers/clk/at91/clk-pll.c +1
-12@@ -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,
Stephen Boyd
maintainerQuoting Marcin Ziemianowicz (2018-05-08 21:32:05)
Quoted 29 lines · Boris Brezillon
Quoted 28 lines
Quoted 24 lines · On Sun, 29 Apr 2018 15:01:11 -0400 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 inprepare(), but therecalc_rate()function instead queries the hardware instead of using these cached values. Therefore, ifrecalc_rate()is called between aset_rate()andprepare(), 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: fixrecalc_rateimplementation of PLL driver") but the bug was later re-introduced by1bdf02326b71("clk:
at91: make use of syscon/regmap internally").Fixes 1bdf02326b71 ("clk: at91: make use of syscon/regmap internally)Cc <[email protected]>Signed-off-by Marcin Ziemianowicz <[email protected]>
Quoted 8 lines
Quoted 1 line
Acked-by Boris Brezillon <[email protected]>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.
Quoting Marcin Ziemianowicz (2018-04-29 12:01:11)
Quoted 21 lines
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 inprepare(), but therecalc_rate()function instead queries the hardware instead of using these cached values. Therefore, ifrecalc_rate()is called between aset_rate()andprepare(), 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: fixrecalc_rateimplementation of PLL driver") but the bug was later re-introduced by1bdf02326b71("clk:
at91: make use of syscon/regmap internally").
Quoted 4 lines
Fixes 1bdf02326b71 ("clk: at91: make use of syscon/regmap internally)Cc <[email protected]>Signed-off-by Marcin Ziemianowicz <[email protected]>---
Applied to clk-next