[PATCH] serial: 8250: enable UART_BUG_NOMSR for Tegra

[PATCH] serial: 8250: enable UART_BUG_NOMSR for Tegra

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

Tegra chips have 4 or 5 identical UART modules embedded. UARTs C..E have
their MODEM-control signals tied off to a static state. However UARTs A
and B can optionally route those signals to/from package pins, depending
on the exact pinmux configuration.

When these signals are not routed to package pins, false interrupts may
trigger either temporarily, or permanently, all while not showing up in
the IIR; it will read as NO_INT. This will eventually lead to the UART
IRQ being disabled due to unhandled interrupts. When this happens, the
kernel may print e.g.:

    irq 68: nobody cared (try booting with the "irqpoll" option)

In order to prevent this, enable UART_BUG_NOMSR. This prevents
UART_IER_MSI from being enabled, which prevents the false interrupts
from triggering.

In practice, this is not needed under any of the following conditions:

* On Tegra chips after Tegra30, since the HW bug has apparently been
  fixed.

* On UARTs C..E since their MODEM control signals are tied to the correct
  static state which doesn't trigger the issue.

* On UARTs A..B if the MODEM control signals are routed out to package
  pins, since they will then carry valid signals.

However, we ignore these exceptions for now, since they are only relevant
if a board actually hooks up more than a 4-wire UART, and no currently
supported board does this. If we ever support a board that does, we can
refine the algorithm that enables UART_BUG_NOMSR to take those exceptions
into account, and/or read a flag from DT/... that indicates that the
board has hooked up and pinmux'd more than a 4-wire UART.

Reported-by: Olof Johansson <olof@lixom.net> # autotester
Cc: <stable@vger.kernel.org>
Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 drivers/tty/serial/8250/8250_core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index e33d38cb170f..61ecd709a722 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -2670,6 +2670,10 @@ static void serial8250_config_port(struct uart_port *port, int flags)
 	if (port->type == PORT_16550A && port->iotype == UPIO_AU)
 		up->bugs |= UART_BUG_NOMSR;
 
+	/* HW bugs may trigger IRQ while IIR == NO_INT */
+	if (port->type == PORT_TEGRA)
+		up->bugs |= UART_BUG_NOMSR;
+
 	if (port->type != PORT_UNKNOWN && flags & UART_CONFIG_IRQ)
 		autoconfig_irq(up);
 
-- 
1.8.1.5

Re: [PATCH] serial: 8250: enable UART_BUG_NOMSR for Tegra

[Heikki Krogerus]

Hi,

On Tue, Jan 07, 2014 at 03:00:12PM -0700, Stephen Warren wrote:
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index e33d38cb170f..61ecd709a722 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -2670,6 +2670,10 @@ static void serial8250_config_port(struct uart_port *port, int flags)
>  	if (port->type == PORT_16550A && port->iotype == UPIO_AU)
>  		up->bugs |= UART_BUG_NOMSR;
>  
> +	/* HW bugs may trigger IRQ while IIR == NO_INT */
> +	if (port->type == PORT_TEGRA)
> +		up->bugs |= UART_BUG_NOMSR;
> +

Is there any reason this needs to be in 8250_core.c? Why not set it in
drivers/tty/serial/of_serial.c?

-- 
heikki

Re: [PATCH] serial: 8250: enable UART_BUG_NOMSR for Tegra

[Stephen Warren]

On 01/15/2014 04:12 AM, Heikki Krogerus wrote:
> Hi,
> 
> On Tue, Jan 07, 2014 at 03:00:12PM -0700, Stephen Warren wrote:
>> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
>> index e33d38cb170f..61ecd709a722 100644
>> --- a/drivers/tty/serial/8250/8250_core.c
>> +++ b/drivers/tty/serial/8250/8250_core.c
>> @@ -2670,6 +2670,10 @@ static void serial8250_config_port(struct uart_port *port, int flags)
>>  	if (port->type == PORT_16550A && port->iotype == UPIO_AU)
>>  		up->bugs |= UART_BUG_NOMSR;
>>  
>> +	/* HW bugs may trigger IRQ while IIR == NO_INT */
>> +	if (port->type == PORT_TEGRA)
>> +		up->bugs |= UART_BUG_NOMSR;
>> +
> 
> Is there any reason this needs to be in 8250_core.c? Why not set it in
> drivers/tty/serial/of_serial.c?

I imagine the code could go in either place.

I don't see any advantage moving it to of_serial.c, and it'd actively be
a disadvantage as far as I'm concerned; PORT_TEGRA is defined in 8250.c,
and other code which enable the same UART_BUG_NOMSR flag is in exactly
the same place, which keeps similar code co-located.

Re: [PATCH] serial: 8250: enable UART_BUG_NOMSR for Tegra

[Heikki Krogerus]

On Wed, Jan 15, 2014 at 10:30:05AM -0700, Stephen Warren wrote:
> On 01/15/2014 04:12 AM, Heikki Krogerus wrote:
> > On Tue, Jan 07, 2014 at 03:00:12PM -0700, Stephen Warren wrote:
> >> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> >> index e33d38cb170f..61ecd709a722 100644
> >> --- a/drivers/tty/serial/8250/8250_core.c
> >> +++ b/drivers/tty/serial/8250/8250_core.c
> >> @@ -2670,6 +2670,10 @@ static void serial8250_config_port(struct uart_port *port, int flags)
> >>  	if (port->type == PORT_16550A && port->iotype == UPIO_AU)
> >>  		up->bugs |= UART_BUG_NOMSR;
> >>  
> >> +	/* HW bugs may trigger IRQ while IIR == NO_INT */
> >> +	if (port->type == PORT_TEGRA)
> >> +		up->bugs |= UART_BUG_NOMSR;
> >> +
> > 
> > Is there any reason this needs to be in 8250_core.c? Why not set it in
> > drivers/tty/serial/of_serial.c?
> 
> I imagine the code could go in either place.
> 
> I don't see any advantage moving it to of_serial.c, and it'd actively be
> a disadvantage as far as I'm concerned; PORT_TEGRA is defined in 8250.c,
> and other code which enable the same UART_BUG_NOMSR flag is in exactly
> the same place, which keeps similar code co-located.

You are adding platform specific quirk to 8250 even though there is
nothing prevention you from keeping it in your probe driver. That is
_not_ acceptable. 8250_core.c is now the layer used by all 8250/16550
compatible UARTs. It must be kept as clean as possible of any
platform/UART specific information, even if it was not done before.

It's already really full of UART/platform specific quirks like this,
and some of them are really bad. Instead of adding one more, we need
to start cleaning the driver of the existing.

About the port types. Since we can now deliver 8250_core.c all the
capabilities and settings from the probe drivers except fcr, and we
need to fix that as well, we should start reducing the number of types
in the static uart config array. Ideally we only have the standard
types in it. So instead of using the custom PORT_TEGRA type, you would
just use perhaps PORT_16550A or PORT_16750, and replace the
capabilities that don't match your HW before registering the ports.

I guess it's fare to say, 8250 is a bit messy. Originally it was just
a driver, but it has now been made into (or it has turned into) an
abstraction layer, so it's probable not always so clear how it should
be used. In any case, the same rules apply as to any layer. It needs
to be kept as platform independent as possible.

Thanks,

-- 
heikki