| Message ID | 20200619200251.9066-3-Sergey.Semin@baikalelectronics.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | serial: 8250_dw: Fix ref clock usage | expand |
On Fri, Jun 19, 2020 at 11:02:50PM +0300, Serge Semin wrote: > Really instead of twice checking the clk_round_rate() return value > we could do it once, and if it isn't error the clock rate can be changed. > By doing so we decrease a number of ret-value tests and remove a weird > goto-based construction implemented in the dw8250_set_termios() method. This doesn't look right to me - neither the before code nor the after code. > clk_disable_unprepare(d->clk); > rate = clk_round_rate(d->clk, baud * 16); > - if (rate < 0) > - ret = rate; > - else if (rate == 0) > - ret = -ENOENT; > - else > + if (rate > 0) { > ret = clk_set_rate(d->clk, rate); > + if (!ret) > + p->uartclk = rate; > + } > clk_prepare_enable(d->clk); > > - if (ret) > - goto out; > - > - p->uartclk = rate; newrate = baud * 16; clk_disable_unprepare(d->clk); rate = clk_round_rate(newrate); ret = clk_set_rate(d->clk, newrate); if (!ret) p->uartclk = rate; ret = elk_prepare_enable(d->clk); /* check ret for failure, means the clock is no longer running */ is all that should be necessary: note that clk_round_rate() is required to return the rate that a successful call to clk_set_rate() would result in for that clock. It is equivalent to: ret = clk_set_rate(d->clk, newrate); if (ret == 0) p->uartclk = clk_get_rate(d->clk); The other commonly misunderstood thing about the clk API is that the rate you pass in to clk_round_rate() to discover the actual clock rate and the value passed in to clk_set_rate() _should_ be the same value. You should _not_ do clk_set_rate(clk, clk_round_rate(clk, newrate));
Hello Russell, Thanks for your comments. My response is below. On Sat, Jun 20, 2020 at 09:12:01AM +0100, Russell King - ARM Linux admin wrote: > On Fri, Jun 19, 2020 at 11:02:50PM +0300, Serge Semin wrote: > > Really instead of twice checking the clk_round_rate() return value > > we could do it once, and if it isn't error the clock rate can be changed. > > By doing so we decrease a number of ret-value tests and remove a weird > > goto-based construction implemented in the dw8250_set_termios() method. > > This doesn't look right to me - neither the before code nor the after > code. > > > clk_disable_unprepare(d->clk); > > rate = clk_round_rate(d->clk, baud * 16); > > - if (rate < 0) > > - ret = rate; > > - else if (rate == 0) > > - ret = -ENOENT; > > - else > > + if (rate > 0) { > > ret = clk_set_rate(d->clk, rate); > > + if (!ret) > > + p->uartclk = rate; > > + } > > clk_prepare_enable(d->clk); > > > > - if (ret) > > - goto out; > > - > > - p->uartclk = rate; > > newrate = baud * 16; > > clk_disable_unprepare(d->clk); > rate = clk_round_rate(newrate); > ret = clk_set_rate(d->clk, newrate); > if (!ret) > p->uartclk = rate; > > ret = elk_prepare_enable(d->clk); > /* check ret for failure, means the clock is no longer running */ > > is all that should be necessary: note that clk_round_rate() is required > to return the rate that a successful call to clk_set_rate() would result > in for that clock. While I do understand your note regarding the newrate passing to both methods, I don't fully get it why is it ok to skip checking the clk_round_rate() return value? As I see it there is no point in calling clk_set_rate() if clk_round_rate() has returned an error. From that perspective this patch is full acceptable, right? In addition to that in order to provide an optimization I'll have to check the return value of the clk_round_rate() anyway in the next patch of this series ("serial: 8250_dw: Fix common clocks usage race condition") since I'll need to set uartclk with that value before calling clk_set_rate() (see the patch and notes there for details). So there is no point in removing the check here since it will be got back in the next patch anyway. One more note in favor of checking the clk_round_rate() return value is below. > It is equivalent to: > > ret = clk_set_rate(d->clk, newrate); > if (ret == 0) > p->uartclk = clk_get_rate(d->clk); Alas neither this nor the suggested code above will work if the clock is optional. If it is, then d->clk will be NULL and clk_round_rate(), clk_set_rate() and clk_get_rate() will return zero. Thus in accordance with the fixes suggested by you we'll rewrite a fixed uartclk value supplied by a firmware. To sum it up getting a positive value returned from clk_round_rate() will mean not only an actual clock frequency, but also having a real reference clock installed. So we can use that value to update the uartclk field of the UART port descriptor. > > The other commonly misunderstood thing about the clk API is that the > rate you pass in to clk_round_rate() to discover the actual clock rate > and the value passed in to clk_set_rate() _should_ be the same value. > You should _not_ do clk_set_rate(clk, clk_round_rate(clk, newrate)); Agreed. Thanks for the comment. I'll fix it in the next version of the series. -Sergey > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c index aab3cccc6789..12866083731d 100644 --- a/drivers/tty/serial/8250/8250_dw.c +++ b/drivers/tty/serial/8250/8250_dw.c @@ -282,20 +282,13 @@ static void dw8250_set_termios(struct uart_port *p, struct ktermios *termios, clk_disable_unprepare(d->clk); rate = clk_round_rate(d->clk, baud * 16); - if (rate < 0) - ret = rate; - else if (rate == 0) - ret = -ENOENT; - else + if (rate > 0) { ret = clk_set_rate(d->clk, rate); + if (!ret) + p->uartclk = rate; + } clk_prepare_enable(d->clk); - if (ret) - goto out; - - p->uartclk = rate; - -out: p->status &= ~UPSTAT_AUTOCTS; if (termios->c_cflag & CRTSCTS) p->status |= UPSTAT_AUTOCTS;
Really instead of twice checking the clk_round_rate() return value we could do it once, and if it isn't error the clock rate can be changed. By doing so we decrease a number of ret-value tests and remove a weird goto-based construction implemented in the dw8250_set_termios() method. Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: Maxime Ripard <mripard@kernel.org> Cc: Will Deacon <will@kernel.org> Cc: Russell King <linux@armlinux.org.uk> Cc: linux-mips@vger.kernel.org Cc: linux-arm-kernel@lists.infradead.org --- Changelog v3: - This is a new patch. --- drivers/tty/serial/8250/8250_dw.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-)