Re: [PATCH] serial: 8250_dw: Improve unwritable LCR workaround

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

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>

> @@ -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