[PATCH] serial: 8250_dw: Fix LCR workaround regression

[PATCH] serial: 8250_dw: Fix LCR workaround regression

[James Hogan]

From: James Hogan <james.hogan@imgtec.com>

Commit c49436b657d0 (serial: 8250_dw: Improve unwritable LCR workaround)
caused a regression. It added a check that the LCR was written properly
to detect and workaround the busy quirk, but the behaviour of bit 5
(UART_LCR_SPAR) differs between IP versions 3.00a and 3.14c per the
docs. On older versions this caused the check to fail and it would
repeatedly force idle and rewrite the LCR register, causing delays and
preventing any input from serial being received.

This is fixed by masking out UART_LCR_SPAR before making the comparison.

Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.cz>
Cc: Tim Kryger <tim.kryger@linaro.org>
Cc: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Cc: Matt Porter <matt.porter@linaro.org>
Cc: Markus Mayer <markus.mayer@linaro.org>
Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Tested-by: Tim Kryger <tim.kryger@linaro.org>
Tested-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/tty/serial/8250/8250_dw.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 4658e3e..5f096c7 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -96,7 +96,8 @@ static void dw8250_serial_out(struct uart_port *p, int offset, int value)
 	if (offset == UART_LCR) {
 		int tries = 1000;
 		while (tries--) {
-			if (value == p->serial_in(p, UART_LCR))
+			unsigned int lcr = p->serial_in(p, UART_LCR);
+			if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
 				return;
 			dw8250_force_idle(p);
 			writeb(value, p->membase + (UART_LCR << p->regshift));
@@ -132,7 +133,8 @@ static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
 	if (offset == UART_LCR) {
 		int tries = 1000;
 		while (tries--) {
-			if (value == p->serial_in(p, UART_LCR))
+			unsigned int lcr = p->serial_in(p, UART_LCR);
+			if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
 				return;
 			dw8250_force_idle(p);
 			writel(value, p->membase + (UART_LCR << p->regshift));
-- 
1.8.3.2

Re: [PATCH] serial: 8250_dw: Fix LCR workaround regression

[Heikki Krogerus]

Hi,

On Tue, Dec 10, 2013 at 10:28:04PM +0000, James Hogan wrote:
> From: James Hogan <james.hogan@imgtec.com>
> 
> Commit c49436b657d0 (serial: 8250_dw: Improve unwritable LCR workaround)
> caused a regression. It added a check that the LCR was written properly
> to detect and workaround the busy quirk, but the behaviour of bit 5
> (UART_LCR_SPAR) differs between IP versions 3.00a and 3.14c per the
> docs. On older versions this caused the check to fail and it would
> repeatedly force idle and rewrite the LCR register, causing delays and
> preventing any input from serial being received.
> 
> This is fixed by masking out UART_LCR_SPAR before making the comparison.
> 
> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jiri Slaby <jslaby@suse.cz>
> Cc: Tim Kryger <tim.kryger@linaro.org>
> Cc: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> Cc: Matt Porter <matt.porter@linaro.org>
> Cc: Markus Mayer <markus.mayer@linaro.org>
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Tested-by: Tim Kryger <tim.kryger@linaro.org>
> Tested-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>

I tested this with HW without the busy functionality
(UART_16550_COMPATIBLE == yes). No problems here. FWIW:

Tested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>


-- 
heikki

Re: [PATCH] serial: 8250_dw: Fix LCR workaround regression

[Tim Kryger]

On Wed, Dec 11, 2013 at 6:00 AM, Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
> Hi,
>
> On Tue, Dec 10, 2013 at 10:28:04PM +0000, James Hogan wrote:
>> From: James Hogan <james.hogan@imgtec.com>
>>
>> Commit c49436b657d0 (serial: 8250_dw: Improve unwritable LCR workaround)
>> caused a regression. It added a check that the LCR was written properly
>> to detect and workaround the busy quirk, but the behaviour of bit 5
>> (UART_LCR_SPAR) differs between IP versions 3.00a and 3.14c per the
>> docs. On older versions this caused the check to fail and it would
>> repeatedly force idle and rewrite the LCR register, causing delays and
>> preventing any input from serial being received.
>>
>> This is fixed by masking out UART_LCR_SPAR before making the comparison.
>>
>> Signed-off-by: James Hogan <james.hogan@imgtec.com>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: Jiri Slaby <jslaby@suse.cz>
>> Cc: Tim Kryger <tim.kryger@linaro.org>
>> Cc: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
>> Cc: Matt Porter <matt.porter@linaro.org>
>> Cc: Markus Mayer <markus.mayer@linaro.org>
>> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>> Tested-by: Tim Kryger <tim.kryger@linaro.org>
>> Tested-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
>
> I tested this with HW without the busy functionality
> (UART_16550_COMPATIBLE == yes). No problems here. FWIW:
>
> Tested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

Greg, can you consider taking this patch as a regression fix for 3.13?

Just in case you want a little more background beyond what is in the
commit message...

Statically declared DW UARTs will execute autoconfig_16550a during
probe which attempts to set the SPAR bit of LCR.  Some old versions of
the DW IP don't implement the stick parity feature and for these
versions the SPAR bit always reads back as zero. Since these writes
aren't fully accepted by the hardware, they trigger the workaround
which interprets the partially rejected write to mean the UART was
busy.  A number of attempts are made to idle the UART and re-write the
LCR but eventually the workaround gives up and prints a warning.  DT
declared DW UARTs don't run autoconfig and don't see this issue as far
as I know.

Thanks,
Tim Kryger

Re: [PATCH] serial: 8250_dw: Fix LCR workaround regression

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 2542 bytes --]

On Wednesday 11 December 2013 13:00:33 Tim Kryger wrote:
> On Wed, Dec 11, 2013 at 6:00 AM, Heikki Krogerus
> 
> <heikki.krogerus@linux.intel.com> wrote:
> > Hi,
> > 
> > On Tue, Dec 10, 2013 at 10:28:04PM +0000, James Hogan wrote:
> >> From: James Hogan <james.hogan@imgtec.com>
> >> 
> >> Commit c49436b657d0 (serial: 8250_dw: Improve unwritable LCR workaround)
> >> caused a regression. It added a check that the LCR was written properly
> >> to detect and workaround the busy quirk, but the behaviour of bit 5
> >> (UART_LCR_SPAR) differs between IP versions 3.00a and 3.14c per the
> >> docs. On older versions this caused the check to fail and it would
> >> repeatedly force idle and rewrite the LCR register, causing delays and
> >> preventing any input from serial being received.
> >> 
> >> This is fixed by masking out UART_LCR_SPAR before making the comparison.
> >> 
> >> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >> Cc: Jiri Slaby <jslaby@suse.cz>
> >> Cc: Tim Kryger <tim.kryger@linaro.org>
> >> Cc: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> >> Cc: Matt Porter <matt.porter@linaro.org>
> >> Cc: Markus Mayer <markus.mayer@linaro.org>
> >> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> >> Tested-by: Tim Kryger <tim.kryger@linaro.org>
> >> Tested-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > 
> > I tested this with HW without the busy functionality
> > (UART_16550_COMPATIBLE == yes). No problems here. FWIW:
> > 
> > Tested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> 
> Greg, can you consider taking this patch as a regression fix for 3.13?
> 
> Just in case you want a little more background beyond what is in the
> commit message...
> 
> Statically declared DW UARTs will execute autoconfig_16550a during
> probe which attempts to set the SPAR bit of LCR.  Some old versions of
> the DW IP don't implement the stick parity feature and for these
> versions the SPAR bit always reads back as zero. Since these writes
> aren't fully accepted by the hardware, they trigger the workaround
> which interprets the partially rejected write to mean the UART was
> busy.  A number of attempts are made to idle the UART and re-write the
> LCR but eventually the workaround gives up and prints a warning. DT
> declared DW UARTs don't run autoconfig and don't see this issue as far
> as I know.

Hi Tim

Thanks for the extra description. FYI, I was instantiating DW UART from DT 
when I saw the problem.

Cheers
James

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] serial: 8250_dw: Fix LCR workaround regression

[Tim Kryger]

On Wed, Dec 11, 2013 at 1:05 PM, James Hogan <james.hogan@imgtec.com> wrote:
> On Wednesday 11 December 2013 13:00:33 Tim Kryger wrote:

>> DT declared DW UARTs don't run autoconfig and don't see this issue as
>> far as I know.

> Thanks for the extra description. FYI, I was instantiating DW UART from DT
> when I saw the problem.

You are right.  It is a little more nuanced.

The regression will also appear for DT declared UARTs if they were
configured without FIFOs or with UART_ADD_ENCODED_PARAMS = 0.

-Tim