From: Marcin Ziemianowicz <marcin@ziemianowicz.com> To: Boris Brezillon <boris.brezillon@bootlin.com> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>, Nicolas Ferre <nicolas.ferre@microchip.com>, Alexandre Belloni <alexandre.belloni@bootlin.com>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, stable@vger.kernel.org, Michael Turquette <mturquette@baylibre.com>, Stephen Boyd <sboyd@kernel.org>, linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH V4] clk: at91: PLL recalc_rate() now using cached MUL and DIV values Date: Wed, 9 May 2018 00:32:05 -0400 [thread overview] Message-ID: <20180509043205.GA57916@hak8or> (raw) In-Reply-To: <20180430075847.29706679@bbrezillon> 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, >
WARNING: multiple messages have this Message-ID (diff)
From: marcin@ziemianowicz.com (Marcin Ziemianowicz) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH V4] clk: at91: PLL recalc_rate() now using cached MUL and DIV values Date: Wed, 9 May 2018 00:32:05 -0400 [thread overview] Message-ID: <20180509043205.GA57916@hak8or> (raw) In-Reply-To: <20180430075847.29706679@bbrezillon> 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, >
next prev parent reply other threads:[~2018-05-09 4:32 UTC|newest] Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-04-29 19:01 [PATCH V4] clk: at91: PLL recalc_rate() now using cached MUL and DIV values Marcin Ziemianowicz 2018-04-29 19:01 ` Marcin Ziemianowicz 2018-04-30 5:58 ` Boris Brezillon 2018-04-30 5:58 ` Boris Brezillon 2018-05-09 4:32 ` Marcin Ziemianowicz [this message] 2018-05-09 4:32 ` Marcin Ziemianowicz 2018-05-15 23:04 ` Stephen Boyd 2018-05-15 23:04 ` Stephen Boyd 2018-05-15 23:04 ` Stephen Boyd 2018-05-16 6:39 ` Stephen Boyd 2018-05-16 6:39 ` Stephen Boyd 2018-05-16 6:39 ` Stephen Boyd -- strict thread matches above, loose matches on Subject: below -- 2018-04-29 18:50 Marcin Ziemianowicz
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20180509043205.GA57916@hak8or \ --to=marcin@ziemianowicz.com \ --cc=alexandre.belloni@bootlin.com \ --cc=boris.brezillon@bootlin.com \ --cc=boris.brezillon@free-electrons.com \ --cc=gregkh@linuxfoundation.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-clk@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mturquette@baylibre.com \ --cc=nicolas.ferre@microchip.com \ --cc=sboyd@kernel.org \ --cc=stable@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.