| Message ID | 20200506233136.11842-3-Sergey.Semin@baikalelectronics.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | serial: 8250_dw: Fix ref clock usage | expand |
On Thu, May 07, 2020 at 02:31:33AM +0300, Serge Semin wrote: > Some platforms can be designed in a way so the UART port reference clock > might be asynchronously changed at some point. In Baikal-T1 SoC this may > happen due to the reference clock being shared between two UART ports, on > the Allwinner SoC the reference clock is derived from the CPU clock, so > any CPU frequency change should get to be known/reflected by/in the UART > controller as well. But it's not enough to just update the > uart_port->uartclk field of the corresponding UART port, the 8250 > controller reference clock divisor should be altered so to preserve > current baud rate setting. All of these things is done in a coherent > way by calling the serial8250_update_uartclk() method provided in this > patch. Though note that it isn't supposed to be called from within the > UART port callbacks because the locks using to the protect the UART port > data are already taken in there. > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru> > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> > Cc: Paul Burton <paulburton@kernel.org> > Cc: Ralf Baechle <ralf@linux-mips.org> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Long Cheng <long.cheng@mediatek.com> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Cc: Maxime Ripard <mripard@kernel.org> > Cc: Catalin Marinas <catalin.marinas@arm.com> > 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 > Cc: linux-mediatek@lists.infradead.org > --- > drivers/tty/serial/8250/8250_port.c | 38 +++++++++++++++++++++++++++++ > include/linux/serial_8250.h | 2 ++ > 2 files changed, 40 insertions(+) > > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c > index 4d83c85a7389..484ff9df1432 100644 > --- a/drivers/tty/serial/8250/8250_port.c > +++ b/drivers/tty/serial/8250/8250_port.c > @@ -2628,6 +2628,44 @@ static unsigned int serial8250_get_baud_rate(struct uart_port *port, > (port->uartclk + tolerance) / 16); > } > > +/* > + * Note in order to avoid the tty port mutex deadlock don't use the next method > + * within the uart port callbacks. Primarily it's supposed to be utilized to > + * handle a sudden reference clock rate change. > + */ > +void serial8250_update_uartclk(struct uart_port *port, unsigned int uartclk) > +{ > + struct uart_8250_port *up = up_to_u8250p(port); > + unsigned int baud, quot, frac = 0; > + struct ktermios *termios; > + unsigned long flags; > + > + mutex_lock(&port->state->port.mutex); > + > + if (port->uartclk == uartclk) > + goto out_lock; > + > + port->uartclk = uartclk; > + termios = &port->state->port.tty->termios; > + > + baud = serial8250_get_baud_rate(port, termios, NULL); > + quot = serial8250_get_divisor(port, baud, &frac); > + > + spin_lock_irqsave(&port->lock, flags); > + > + uart_update_timeout(port, termios->c_cflag, baud); > + > + serial8250_set_divisor(port, baud, quot, frac); > + serial_port_out(port, UART_LCR, up->lcr); > + serial8250_out_MCR(up, UART_MCR_DTR | UART_MCR_RTS); > + > + spin_unlock_irqrestore(&port->lock, flags); > + > +out_lock: > + mutex_unlock(&port->state->port.mutex); > +} > +EXPORT_SYMBOL(serial8250_update_uartclk); EXPORT_SYMBOL_GPL() please. thanks, greg k-h
On Fri, May 15, 2020 at 02:45:25PM +0200, Greg Kroah-Hartman wrote: > On Thu, May 07, 2020 at 02:31:33AM +0300, Serge Semin wrote: > > Some platforms can be designed in a way so the UART port reference clock > > might be asynchronously changed at some point. In Baikal-T1 SoC this may > > happen due to the reference clock being shared between two UART ports, on > > the Allwinner SoC the reference clock is derived from the CPU clock, so > > any CPU frequency change should get to be known/reflected by/in the UART > > controller as well. But it's not enough to just update the > > uart_port->uartclk field of the corresponding UART port, the 8250 > > controller reference clock divisor should be altered so to preserve > > current baud rate setting. All of these things is done in a coherent > > way by calling the serial8250_update_uartclk() method provided in this > > patch. Though note that it isn't supposed to be called from within the > > UART port callbacks because the locks using to the protect the UART port > > data are already taken in there. > > > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru> > > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> > > Cc: Paul Burton <paulburton@kernel.org> > > Cc: Ralf Baechle <ralf@linux-mips.org> > > Cc: Arnd Bergmann <arnd@arndb.de> > > Cc: Long Cheng <long.cheng@mediatek.com> > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Cc: Maxime Ripard <mripard@kernel.org> > > Cc: Catalin Marinas <catalin.marinas@arm.com> > > 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 > > Cc: linux-mediatek@lists.infradead.org > > --- > > drivers/tty/serial/8250/8250_port.c | 38 +++++++++++++++++++++++++++++ > > include/linux/serial_8250.h | 2 ++ > > 2 files changed, 40 insertions(+) > > > > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c > > index 4d83c85a7389..484ff9df1432 100644 > > --- a/drivers/tty/serial/8250/8250_port.c > > +++ b/drivers/tty/serial/8250/8250_port.c > > @@ -2628,6 +2628,44 @@ static unsigned int serial8250_get_baud_rate(struct uart_port *port, > > (port->uartclk + tolerance) / 16); > > } > > > > +/* > > + * Note in order to avoid the tty port mutex deadlock don't use the next method > > + * within the uart port callbacks. Primarily it's supposed to be utilized to > > + * handle a sudden reference clock rate change. > > + */ > > +void serial8250_update_uartclk(struct uart_port *port, unsigned int uartclk) > > +{ > > + struct uart_8250_port *up = up_to_u8250p(port); > > + unsigned int baud, quot, frac = 0; > > + struct ktermios *termios; > > + unsigned long flags; > > + > > + mutex_lock(&port->state->port.mutex); > > + > > + if (port->uartclk == uartclk) > > + goto out_lock; > > + > > + port->uartclk = uartclk; > > + termios = &port->state->port.tty->termios; > > + > > + baud = serial8250_get_baud_rate(port, termios, NULL); > > + quot = serial8250_get_divisor(port, baud, &frac); > > + > > + spin_lock_irqsave(&port->lock, flags); > > + > > + uart_update_timeout(port, termios->c_cflag, baud); > > + > > + serial8250_set_divisor(port, baud, quot, frac); > > + serial_port_out(port, UART_LCR, up->lcr); > > + serial8250_out_MCR(up, UART_MCR_DTR | UART_MCR_RTS); > > + > > + spin_unlock_irqrestore(&port->lock, flags); > > + > > +out_lock: > > + mutex_unlock(&port->state->port.mutex); > > +} > > +EXPORT_SYMBOL(serial8250_update_uartclk); > > EXPORT_SYMBOL_GPL() please. Ok. I guess I've just copied the line from some of the export symbol statements below. My mistake. Sorry. -Sergey > > thanks, > > greg k-h
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c index 4d83c85a7389..484ff9df1432 100644 --- a/drivers/tty/serial/8250/8250_port.c +++ b/drivers/tty/serial/8250/8250_port.c @@ -2628,6 +2628,44 @@ static unsigned int serial8250_get_baud_rate(struct uart_port *port, (port->uartclk + tolerance) / 16); } +/* + * Note in order to avoid the tty port mutex deadlock don't use the next method + * within the uart port callbacks. Primarily it's supposed to be utilized to + * handle a sudden reference clock rate change. + */ +void serial8250_update_uartclk(struct uart_port *port, unsigned int uartclk) +{ + struct uart_8250_port *up = up_to_u8250p(port); + unsigned int baud, quot, frac = 0; + struct ktermios *termios; + unsigned long flags; + + mutex_lock(&port->state->port.mutex); + + if (port->uartclk == uartclk) + goto out_lock; + + port->uartclk = uartclk; + termios = &port->state->port.tty->termios; + + baud = serial8250_get_baud_rate(port, termios, NULL); + quot = serial8250_get_divisor(port, baud, &frac); + + spin_lock_irqsave(&port->lock, flags); + + uart_update_timeout(port, termios->c_cflag, baud); + + serial8250_set_divisor(port, baud, quot, frac); + serial_port_out(port, UART_LCR, up->lcr); + serial8250_out_MCR(up, UART_MCR_DTR | UART_MCR_RTS); + + spin_unlock_irqrestore(&port->lock, flags); + +out_lock: + mutex_unlock(&port->state->port.mutex); +} +EXPORT_SYMBOL(serial8250_update_uartclk); + void serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios, struct ktermios *old) diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h index 6545f8cfc8fa..2b70f736b091 100644 --- a/include/linux/serial_8250.h +++ b/include/linux/serial_8250.h @@ -155,6 +155,8 @@ extern int early_serial_setup(struct uart_port *port); extern int early_serial8250_setup(struct earlycon_device *device, const char *options); +extern void serial8250_update_uartclk(struct uart_port *port, + unsigned int uartclk); extern void serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios, struct ktermios *old); extern void serial8250_do_set_ldisc(struct uart_port *port,
Some platforms can be designed in a way so the UART port reference clock might be asynchronously changed at some point. In Baikal-T1 SoC this may happen due to the reference clock being shared between two UART ports, on the Allwinner SoC the reference clock is derived from the CPU clock, so any CPU frequency change should get to be known/reflected by/in the UART controller as well. But it's not enough to just update the uart_port->uartclk field of the corresponding UART port, the 8250 controller reference clock divisor should be altered so to preserve current baud rate setting. All of these things is done in a coherent way by calling the serial8250_update_uartclk() method provided in this patch. Though note that it isn't supposed to be called from within the UART port callbacks because the locks using to the protect the UART port data are already taken in there. Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> Cc: Paul Burton <paulburton@kernel.org> Cc: Ralf Baechle <ralf@linux-mips.org> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Long Cheng <long.cheng@mediatek.com> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: Maxime Ripard <mripard@kernel.org> Cc: Catalin Marinas <catalin.marinas@arm.com> 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 Cc: linux-mediatek@lists.infradead.org --- drivers/tty/serial/8250/8250_port.c | 38 +++++++++++++++++++++++++++++ include/linux/serial_8250.h | 2 ++ 2 files changed, 40 insertions(+)