On Wed, Jul 31, 2019 at 12:45:45PM -0500, Aaron Sierra wrote:
> Move INT0 clearing out of common, per-port serial8250_do_startup()
> into PCI device probe/resume.
>
> As described in commit 2c0ac5b48a35 ("serial: exar: Fix stuck MSIs"),
> the purpose of clearing INT0 is to prevent the PCI interrupt line from
> becoming stuck asserted, "which is fatal with edge-triggered MSIs".
>
> Like the clearing via interrupt handler that moved from common code in
> commit c7e1b4059075 ("tty: serial: exar: Relocate sleep wake-up
> handling"), this clearing at startup can be better handled at the PCI
> device level.
>
Looks good to me,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
P.S. When you have a chance to test my series, please, give a tag.
> Cc: Jan Kiszka <jan.kiszka@xxxxxxxxxxx>
> Cc: Sudip Mukherjee <sudip.mukherjee@xxxxxxxxxxxxxxx>
> Cc: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> Signed-off-by: Aaron Sierra <asierra@xxxxxxxxxxx>
> ---
>
> The embedded patch is written to follow this patch from Andy Shevchenko:
> [PATCH v4 3/3] serial: 8250_exar: Move custom divisor support out from 8250_port
>
> drivers/tty/serial/8250/8250_exar.c | 24 ++++++++++++++++--------
> drivers/tty/serial/8250/8250_port.c | 9 ---------
> 2 files changed, 16 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> index 4f8f45e2fc1d..248be217f528 100644
> --- a/drivers/tty/serial/8250/8250_exar.c
> +++ b/drivers/tty/serial/8250/8250_exar.c
> @@ -492,6 +492,16 @@ static void pci_xr17v35x_exit(struct pci_dev *pcidev)
> port->port.private_data = NULL;
> }
>
> +static inline void exar_misc_clear(struct exar8250 *priv)
> +{
> + /* Clear all PCI interrupts by reading INT0. No effect on IIR */
> + readb(priv->virt + UART_EXAR_INT0);
> +
> + /* Clear INT0 for Expansion Interface slave ports, too */
> + if (priv->board->num_ports > 8)
> + readb(priv->virt + 0x2000 + UART_EXAR_INT0);
> +}
> +
> /*
> * These Exar UARTs have an extra interrupt indicator that could fire for a
> * few interrupts that are not presented/cleared through IIR. One of which is
> @@ -503,14 +513,7 @@ static void pci_xr17v35x_exit(struct pci_dev *pcidev)
> */
> static irqreturn_t exar_misc_handler(int irq, void *data)
> {
> - struct exar8250 *priv = data;
> -
> - /* Clear all PCI interrupts by reading INT0. No effect on IIR */
> - readb(priv->virt + UART_EXAR_INT0);
> -
> - /* Clear INT0 for Expansion Interface slave ports, too */
> - if (priv->board->num_ports > 8)
> - readb(priv->virt + 0x2000 + UART_EXAR_INT0);
> + exar_misc_clear(data);
>
> return IRQ_HANDLED;
> }
> @@ -563,6 +566,9 @@ exar_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *ent)
> if (rc)
> return rc;
>
> + /* Clear interrupts */
> + exar_misc_clear(priv);
> +
> for (i = 0; i < nr_ports && i < maxnr; i++) {
> rc = board->setup(priv, pcidev, &uart, i);
> if (rc) {
> @@ -622,6 +628,8 @@ static int __maybe_unused exar_resume(struct device *dev)
> struct exar8250 *priv = pci_get_drvdata(pcidev);
> unsigned int i;
>
> + exar_misc_clear(priv);
> +
> for (i = 0; i < priv->nr; i++)
> if (priv->line[i] >= 0)
> serial8250_resume_port(priv->line[i]);
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 995a7e8b7839..706645f89132 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -40,11 +40,6 @@
>
> #include "8250.h"
>
> -/*
> - * These are definitions for the Exar XR17V35X and XR17(C|D)15X
> - */
> -#define UART_EXAR_INT0 0x80
> -
> /* Nuvoton NPCM timeout register */
> #define UART_NPCM_TOR 7
> #define UART_NPCM_TOIE BIT(7) /* Timeout Interrupt Enable */
> @@ -2138,8 +2133,6 @@ int serial8250_do_startup(struct uart_port *port)
> serial_port_in(port, UART_RX);
> serial_port_in(port, UART_IIR);
> serial_port_in(port, UART_MSR);
> - if ((port->type == PORT_XR17V35X) || (port->type == PORT_XR17D15X))
> - serial_port_in(port, UART_EXAR_INT0);
>
> /*
> * At this point, there's no way the LSR could still be 0xff;
> @@ -2297,8 +2290,6 @@ int serial8250_do_startup(struct uart_port *port)
> serial_port_in(port, UART_RX);
> serial_port_in(port, UART_IIR);
> serial_port_in(port, UART_MSR);
> - if ((port->type == PORT_XR17V35X) || (port->type == PORT_XR17D15X))
> - serial_port_in(port, UART_EXAR_INT0);
> up->lsr_saved_flags = 0;
> up->msr_saved_flags = 0;
>
> --
> 2.17.1
>
--
With Best Regards,
Andy Shevchenko
