Re: [PATCH] serial: 8250_dw: Improve unwritable LCR workaround
From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Date: 2013-09-25 11:42:57
Also in:
lkml
Hi Tim, On Tue, Sep 24, 2013 at 05:39:09PM -0700, Tim Kryger wrote:
The Designware UART has a limitation where it ignores writes into the LCR if the UART is busy. The current workaround stashes a copy of the last written LCR and writes it back down to the hardware if it receives a special busy interrupt which is raised when a write was ignored. Unfortunately, interrupts are typically disabled prior to performing a sequence of register writes that include the LCR so the point at which the retry occurs is too late. An example is serial8250_do_set_termios() where an ignored LCR write results in the baud divisor not being set and instead a garbage character is sent out the transmitter. Furthermore, since serial_port_out() offers no way to indicate failure, a serious effort must be made to ensure that the LCR is actually updated before returning back to the caller. This is difficult, however, as a UART that was busy during the first attempt is likely to still be busy when a subsequent attempt is made unless some extra action is taken. This updated workaround takes the extreme action of clearing the TX/RX FIFOs and reading the receive buffer before writing down the LCR in the hope that doing so will force the UART into an idle state. While this may seem unnecessarily aggressive, writes to the LCR are used to change the baud rate, parity, stop bit, or data length so the data that may be lost is likely not important. Admittedly, this is far from ideal but it seems to be the best that can be done given the hardware limitations.
<snip>
quoted hunk ↗ jump to hunk
@@ -76,17 +75,35 @@ static inline int dw8250_modify_msr(struct uart_port *p, int offset, int value) return value; } +/* The UART will ignore writes to LCR when busy we take aggressive steps + * to ensure that it is idle before attempting to write to LCR */ +static void dw8250_force_idle(struct uart_port *p) +{ + serial8250_clear_and_reinit_fifos(container_of + (p, struct uart_8250_port, port)); + (void)p->serial_in(p, UART_LSR); + (void)p->serial_in(p, UART_MSR); + (void)p->serial_in(p, UART_RX); +}
This looks pretty brutal. Is it really necessary?
static void dw8250_serial_out(struct uart_port *p, int offset, int value)
{
struct dw8250_data *d = p->private_data;
- if (offset == UART_LCR)
- d->last_lcr = value;
-
- if (offset == UART_MCR)
- d->last_mcr = value;
-
- writeb(value, p->membase + (offset << p->regshift));
+ if (offset == UART_LCR) {
+ int tries = 1000;
+ while (tries--) {
+ if (value == p->serial_in(p, UART_LCR))
+ return;
+ dw8250_force_idle(p);
+ writeb(value, p->membase + (UART_LCR << p->regshift));
+ }
+ dev_err(p->dev, "Couldn't set LCR to %d\n", value);Is it not enough to simply poll USR[0] to see when the UART becomes free? -- heikki