LKML Archive on lore.kernel.org help / color / mirror / Atom feed
* [PATCH v3] clk: at91: PLL recalc_rate() now using cached MUL+DIV values @ 2018-04-27 17:56 Marcin Ziemianowicz 2018-04-29 10:59 ` kbuild test robot 2018-04-29 13:17 ` Boris Brezillon 0 siblings, 2 replies; 6+ messages in thread From: Marcin Ziemianowicz @ 2018-04-27 17:56 UTC (permalink / raw) To: Boris Brezillon, Nicolas Ferre, Alexandre Belloni Cc: Michael Turquette, Stephen Boyd, linux-clk, linux-arm-kernel, linux-kernel Stephen Boyd <sboyd@kernel.org>, linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Bcc: Subject: [PATCH v3] clk: at91: PLL recalc_rate() now using cached MUL and DIV values Reply-To: 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: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/283502.html but was later changed back via a large patch (maybe by mistake?): https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1bdf02326b71eae7e9b4b335b881856aaf9d1af6 Thank you for bearing with me about this Boris. 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 Signed-off-by: Marcin Ziemianowicz <marcin@ziemianowicz.com> --- 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..cc6e0364 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 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] clk: at91: PLL recalc_rate() now using cached MUL+DIV values 2018-04-27 17:56 [PATCH v3] clk: at91: PLL recalc_rate() now using cached MUL+DIV values Marcin Ziemianowicz @ 2018-04-29 10:59 ` kbuild test robot 2018-04-29 13:17 ` Boris Brezillon 1 sibling, 0 replies; 6+ messages in thread From: kbuild test robot @ 2018-04-29 10:59 UTC (permalink / raw) To: Marcin Ziemianowicz Cc: kbuild-all, Boris Brezillon, Nicolas Ferre, Alexandre Belloni, Michael Turquette, Stephen Boyd, linux-clk, linux-arm-kernel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1743 bytes --] Hi Marcin, Thank you for the patch! Yet something to improve: [auto build test ERROR on v4.17-rc2] [also build test ERROR on next-20180426] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Marcin-Ziemianowicz/clk-at91-PLL-recalc_rate-now-using-cached-MUL-DIV-values/20180429-134826 config: arm-at91_dt_defconfig (attached as .config) compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=arm All error/warnings (new ones prefixed by >>): drivers/clk/at91/clk-pll.c: In function 'clk_pll_recalc_rate': >> drivers/clk/at91/clk-pll.c:136:9: error: expected expression before 'return' return return (parent_rate / pll->div) * (pll->mul + 1); ^~~~~~ drivers/clk/at91/clk-pll.c:134:18: warning: unused variable 'pll' [-Wunused-variable] struct clk_pll *pll = to_clk_pll(hw); ^~~ >> drivers/clk/at91/clk-pll.c:137:1: warning: control reaches end of non-void function [-Wreturn-type] } ^ vim +/return +136 drivers/clk/at91/clk-pll.c 130 131 static unsigned long clk_pll_recalc_rate(struct clk_hw *hw, 132 unsigned long parent_rate) 133 { 134 struct clk_pll *pll = to_clk_pll(hw); 135 > 136 return return (parent_rate / pll->div) * (pll->mul + 1); > 137 } 138 --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 23245 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] clk: at91: PLL recalc_rate() now using cached MUL+DIV values 2018-04-27 17:56 [PATCH v3] clk: at91: PLL recalc_rate() now using cached MUL+DIV values Marcin Ziemianowicz 2018-04-29 10:59 ` kbuild test robot @ 2018-04-29 13:17 ` Boris Brezillon 2018-04-29 13:19 ` Boris Brezillon 2018-04-29 15:22 ` Marcin Ziemianowicz 1 sibling, 2 replies; 6+ messages in thread From: Boris Brezillon @ 2018-04-29 13:17 UTC (permalink / raw) To: Marcin Ziemianowicz Cc: Boris Brezillon, Nicolas Ferre, Alexandre Belloni, Michael Turquette, Stephen Boyd, linux-clk, linux-arm-kernel, linux-kernel Hi Marcin, On Fri, 27 Apr 2018 13:56:09 -0400 Marcin Ziemianowicz <marcin@ziemianowicz.com> wrote: > Stephen Boyd <sboyd@kernel.org>, > linux-clk@vger.kernel.org, > linux-arm-kernel@lists.infradead.org, > linux-kernel@vger.kernel.org > Bcc: > Subject: [PATCH v3] clk: at91: PLL recalc_rate() now using cached MUL and DIV > values > Reply-To: Hm, I don't know how you prepared and sent your patch, but you shouldn't have these fields in the body of your email. Please use git format-patch to prepare the patch and then git send-email to send it. > > 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: > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/283502.html > but was later changed back via a large patch (maybe by mistake?): > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1bdf02326b71eae7e9b4b335b881856aaf9d1af6 Yep, probably by mistake. I started this rework long before it has been submitted to the ML, so I probably messed something up when rebasing. Also, prefer commit IDs to links to the ML archive. The above would sentence would give: " 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"). " The following comment and the changelog should be placed after the '---' line, so that it's not part of the commit message. > Thank you for bearing with me about this Boris. > > 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 > You should add Fixes and Cc-stable tags so that the fix is backported to stable branches: Fixes: 1bdf02326b71 ("clk: at91: make use of syscon/regmap internally) Cc: <stable@vger.kernel.org> > Signed-off-by: Marcin Ziemianowicz <marcin@ziemianowicz.com> > --- > 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..cc6e0364 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 return (parent_rate / pll->div) * (pll->mul + 1); The fix looks good. Let me know if you struggle with git format-patch/send-email and I'll try to help you (or send the patch for you if you don't care learning the process, but I think it's better if you learn how to submit patches). Thanks, Boris > } > > static long clk_pll_get_best_div_mul(struct clk_pll *pll, unsigned long rate, ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] clk: at91: PLL recalc_rate() now using cached MUL+DIV values 2018-04-29 13:17 ` Boris Brezillon @ 2018-04-29 13:19 ` Boris Brezillon 2018-04-29 15:25 ` Marcin Ziemianowicz 2018-04-29 15:22 ` Marcin Ziemianowicz 1 sibling, 1 reply; 6+ messages in thread From: Boris Brezillon @ 2018-04-29 13:19 UTC (permalink / raw) To: Marcin Ziemianowicz Cc: Boris Brezillon, Nicolas Ferre, Alexandre Belloni, Michael Turquette, Stephen Boyd, linux-clk, linux-arm-kernel, linux-kernel On Sun, 29 Apr 2018 15:17:10 +0200 Boris Brezillon <boris.brezillon@bootlin.com> wrote: > Hi Marcin, > > On Fri, 27 Apr 2018 13:56:09 -0400 > Marcin Ziemianowicz <marcin@ziemianowicz.com> wrote: > > > Stephen Boyd <sboyd@kernel.org>, > > linux-clk@vger.kernel.org, > > linux-arm-kernel@lists.infradead.org, > > linux-kernel@vger.kernel.org > > Bcc: > > Subject: [PATCH v3] clk: at91: PLL recalc_rate() now using cached MUL and DIV > > values > > Reply-To: > > Hm, I don't know how you prepared and sent your patch, but you shouldn't > have these fields in the body of your email. Please use git format-patch > to prepare the patch and then git send-email to send it. > > > > > 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: > > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/283502.html > > but was later changed back via a large patch (maybe by mistake?): > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1bdf02326b71eae7e9b4b335b881856aaf9d1af6 > > Yep, probably by mistake. I started this rework long before it has been > submitted to the ML, so I probably messed something up when rebasing. > > Also, prefer commit IDs to links to the ML archive. The above would > sentence would give: > > " > 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"). > " > > > The following comment and the changelog should be placed after the > '---' line, so that it's not part of the commit message. > > > Thank you for bearing with me about this Boris. > > > > 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 > > > > You should add Fixes and Cc-stable tags so that the fix is backported > to stable branches: > > Fixes: 1bdf02326b71 ("clk: at91: make use of syscon/regmap internally) > Cc: <stable@vger.kernel.org> > > > Signed-off-by: Marcin Ziemianowicz <marcin@ziemianowicz.com> > > --- > > 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..cc6e0364 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 return (parent_rate / pll->div) * (pll->mul + 1); Oops, one too many return as reported by kbuild test robot. > > The fix looks good. Let me know if you struggle with git > format-patch/send-email and I'll try to help you (or send the patch for > you if you don't care learning the process, but I think it's better if > you learn how to submit patches). > > Thanks, > > Boris > > > } > > > > static long clk_pll_get_best_div_mul(struct clk_pll *pll, unsigned long rate, > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] clk: at91: PLL recalc_rate() now using cached MUL+DIV values 2018-04-29 13:19 ` Boris Brezillon @ 2018-04-29 15:25 ` Marcin Ziemianowicz 0 siblings, 0 replies; 6+ messages in thread From: Marcin Ziemianowicz @ 2018-04-29 15:25 UTC (permalink / raw) To: Boris Brezillon Cc: Boris Brezillon, Nicolas Ferre, Alexandre Belloni, Michael Turquette, Stephen Boyd, linux-clk, linux-arm-kernel, linux-kernel On Sun, Apr 29, 2018 at 03:19:35PM +0200, Boris Brezillon wrote: > On Sun, 29 Apr 2018 15:17:10 +0200 > Boris Brezillon <boris.brezillon@bootlin.com> wrote: > > > Hi Marcin, > > > > On Fri, 27 Apr 2018 13:56:09 -0400 > > Marcin Ziemianowicz <marcin@ziemianowicz.com> wrote: > > > > > Stephen Boyd <sboyd@kernel.org>, > > > linux-clk@vger.kernel.org, > > > linux-arm-kernel@lists.infradead.org, > > > linux-kernel@vger.kernel.org > > > Bcc: > > > Subject: [PATCH v3] clk: at91: PLL recalc_rate() now using cached MUL and DIV > > > values > > > Reply-To: > > > > Hm, I don't know how you prepared and sent your patch, but you shouldn't > > have these fields in the body of your email. Please use git format-patch > > to prepare the patch and then git send-email to send it. > > > > > > > > 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: > > > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/283502.html > > > but was later changed back via a large patch (maybe by mistake?): > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1bdf02326b71eae7e9b4b335b881856aaf9d1af6 > > > > Yep, probably by mistake. I started this rework long before it has been > > submitted to the ML, so I probably messed something up when rebasing. > > > > Also, prefer commit IDs to links to the ML archive. The above would > > sentence would give: > > > > " > > 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"). > > " > > > > > > The following comment and the changelog should be placed after the > > '---' line, so that it's not part of the commit message. > > > > > Thank you for bearing with me about this Boris. > > > > > > 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 > > > > > > > You should add Fixes and Cc-stable tags so that the fix is backported > > to stable branches: > > > > Fixes: 1bdf02326b71 ("clk: at91: make use of syscon/regmap internally) > > Cc: <stable@vger.kernel.org> > > > > > Signed-off-by: Marcin Ziemianowicz <marcin@ziemianowicz.com> > > > --- > > > 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..cc6e0364 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 return (parent_rate / pll->div) * (pll->mul + 1); > > Oops, one too many return as reported by kbuild test robot. That's what I get for bug fixing in a buildroot generated kernel directory and then copying it over to a new git clone'd kernel directory by hand. I will fix this in addition to all the problems you raised in the earlier email. Thank you again! > > > > > The fix looks good. Let me know if you struggle with git > > format-patch/send-email and I'll try to help you (or send the patch for > > you if you don't care learning the process, but I think it's better if > > you learn how to submit patches). > > > > Thanks, > > > > Boris > > > > > } > > > > > > static long clk_pll_get_best_div_mul(struct clk_pll *pll, unsigned long rate, > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] clk: at91: PLL recalc_rate() now using cached MUL+DIV values 2018-04-29 13:17 ` Boris Brezillon 2018-04-29 13:19 ` Boris Brezillon @ 2018-04-29 15:22 ` Marcin Ziemianowicz 1 sibling, 0 replies; 6+ messages in thread From: Marcin Ziemianowicz @ 2018-04-29 15:22 UTC (permalink / raw) To: Boris Brezillon Cc: Boris Brezillon, Nicolas Ferre, Alexandre Belloni, Michael Turquette, Stephen Boyd, linux-clk, linux-arm-kernel, linux-kernel On Sun, Apr 29, 2018 at 03:17:10PM +0200, Boris Brezillon wrote: > Hi Marcin, > > On Fri, 27 Apr 2018 13:56:09 -0400 > Marcin Ziemianowicz <marcin@ziemianowicz.com> wrote: > > > Stephen Boyd <sboyd@kernel.org>, > > linux-clk@vger.kernel.org, > > linux-arm-kernel@lists.infradead.org, > > linux-kernel@vger.kernel.org > > Bcc: > > Subject: [PATCH v3] clk: at91: PLL recalc_rate() now using cached MUL and DIV > > values > > Reply-To: > > Hm, I don't know how you prepared and sent your patch, but you shouldn't > have these fields in the body of your email. Please use git format-patch > to prepare the patch and then git send-email to send it. I have been using Mutt this entire time with the idea of being able to use Mutt for both sending the patch and replying to emails, but I am still fumbling around with this (as you can see). I used git format-patch HEAD^ and I think I additionally bungled something when using Mutt. I will try git send-email instead since it seems less error prone on my end. > > > > > 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: > > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/283502.html > > but was later changed back via a large patch (maybe by mistake?): > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1bdf02326b71eae7e9b4b335b881856aaf9d1af6 > > Yep, probably by mistake. I started this rework long before it has been > submitted to the ML, so I probably messed something up when rebasing. > Ah, it happens I guess. 99 little bugs in the code, 99 litle bugs. Take one down, patch it around, 117 little bugs in the code. ;) > Also, prefer commit IDs to links to the ML archive. The above would > sentence would give: > > " > 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"). > " > Ah, I was wondering what the best way is to reference old commits. Thank you, will do! > > The following comment and the changelog should be placed after the > '---' line, so that it's not part of the commit message. > > > Thank you for bearing with me about this Boris. > > > > 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 > > > > You should add Fixes and Cc-stable tags so that the fix is backported > to stable branches: > > Fixes: 1bdf02326b71 ("clk: at91: make use of syscon/regmap internally) > Cc: <stable@vger.kernel.org> > > > Signed-off-by: Marcin Ziemianowicz <marcin@ziemianowicz.com> > > --- > > 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..cc6e0364 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 return (parent_rate / pll->div) * (pll->mul + 1); > > The fix looks good. Let me know if you struggle with git > format-patch/send-email and I'll try to help you (or send the patch for > you if you don't care learning the process, but I think it's better if > you learn how to submit patches). > I definitely do want to learn how to do this myself, but thank you for the offer! I just hope I am not taking up too much of your and others time with me bumbling around like this. > Thanks, > > Boris > > > } > > > > static long clk_pll_get_best_div_mul(struct clk_pll *pll, unsigned long rate, > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-04-29 15:25 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-04-27 17:56 [PATCH v3] clk: at91: PLL recalc_rate() now using cached MUL+DIV values Marcin Ziemianowicz 2018-04-29 10:59 ` kbuild test robot 2018-04-29 13:17 ` Boris Brezillon 2018-04-29 13:19 ` Boris Brezillon 2018-04-29 15:25 ` Marcin Ziemianowicz 2018-04-29 15:22 ` Marcin Ziemianowicz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).