CCing linux-kernel as per AC's suggestion...
-----Original Message-----
From: Sergei Shtylyov
Sent: Friday, January 19, 2007 9:05 AM
To: Marc St-Jean
Cc: [EMAIL PROTECTED]; linux-serial@vger.kernel.org
Subject: Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git master
Hello.
Marc St-Jean wrote:
> Here is a serial driver patch for the PMC-Sierra MSP71xx device.
> There are three different fixes:
> 1. Fix for THRE errata
> 2. Fix for Busy Detect on LCR write
> 3. Workaround for interrupt/data concurrency issue
> The first fix is handled cleanly using a UART_BUG_* flag.
Hm, I wouldn't call it clean...
> The second and third fixes use platform tests. I couldn't think of a
> good way to implement them without using tests and not increase code
> and structure sizes for platforms not requiring them.
> Does anyone have any suggestions on implementing these without the
> platform flag?
> Thanks,
> Marc
> Signed-off-by: Marc St-Jean <[EMAIL PROTECTED]>
> Index: linux_2_6/drivers/serial/8250.c
> ===================================================================
> RCS file: linux_2_6/drivers/serial/8250.c,v retrieving revision
> 1.1.1.7 retrieving revision 1.9 diff -u -r1.1.1.7 -r1.9
> --- linux_2_6/drivers/serial/8250.c 19 Oct 2006 21:00:58 -0000 1.1.1.7
> +++ linux_2_6/drivers/serial/8250.c 19 Oct 2006 22:08:15 -0000 1.9
> @@ -44,6 +44,10 @@
> #include <asm/io.h>
> #include <asm/irq.h>
>
> +#ifdef CONFIG_PMC_MSP
> +#include <msp_regs.h>
> +#endif
> +
> #include "8250.h"
>
> /*
> @@ -130,6 +134,9 @@
> unsigned char mcr_mask; /* mask of user bits */
> unsigned char mcr_force; /* mask of forced bits */
> unsigned char lsr_break_flag;
> +#ifdef CONFIG_PMC_MSP
> + int dwapb_lcr; /* saved LCR
> for DW APB WAR */
> +#endif
There was already 'lcr' field there, couldn't it be used?
> @@ -333,6 +340,10 @@
> static void
> serial_out(struct uart_8250_port *up, int offset, int value)
> {
> +#ifdef CONFIG_PMC_MSP
> + /* Save the offset before it's remapped */
> + int dwapb_offset = offset;
> +#endif
> offset = map_8250_out_reg(up, offset) << up->port.regshift;
>
> switch (up->port.iotype) {
> @@ -342,7 +353,19 @@
> break;
>
> case UPIO_MEM:
> +#ifdef CONFIG_PMC_MSP
> + /* Save the LCR value so it can be re-written when a
> + * Busy Detect interrupt occurs. */
> + if (dwapb_offset == UART_LCR)
> + up->dwapb_lcr = value;
> +#endif
> writeb(value, up->port.membase + offset);
> +#ifdef CONFIG_PMC_MSP
> + /* Re-read the IER to ensure any interrupt disabling has
> + * completed before proceeding with ISR. */
> + if (dwapb_offset == UART_IER)
> + value = serial_in(up, dwapb_offset); #endif
> break;
Hm, was there really a need for #ifdef mess here?
I'd vote for introducing new UPIO_* here, like was done for TSi10x UARTs
just for the same reason.
> @@ -1016,6 +1039,17 @@
> up->bugs |= UART_BUG_NOMSR;
> #endif
>
> + /* Workaround:
> + * The DesignWare SoC UART part has a bug for all
> + * versions before 3.03a (2005-07-18)
> + * In brief, this is a non-standard 16550 in that the THRE interrupt
> + * will not re-assert itself simply by disabling and re-enabling the
> + * THRI bit in the IER, it is only re-enabled if a character is actually
> + * sent out.
> + */
> + if( up->port.flags & UPF_DW_THRE_BUG )
> + up->bugs |= UART_BUG_DWTHRE;
> +
> serial_outp(up, UART_LCR, save_lcr);
>
> if (up->capabilities != uart_config[up->port.type].flags) { @@
> -1141,6 +1175,12 @@
> iir = serial_in(up, UART_IIR);
> if (lsr & UART_LSR_TEMT && iir & UART_IIR_NO_INT)
> transmit_chars(up);
> + } else if (up->bugs & UART_BUG_DWTHRE) {
> + unsigned char lsr, iir;
> + lsr = serial_in(up, UART_LSR);
> + iir = serial_in(up, UART_IIR);
> + if (lsr & UART_LSR_THRE)
> + transmit_chars(up);
I don't see how this *really* differs from the UART_BUG_TXEN case.
Have you tried *that* workaround? In any case, looks like this errata is
auto-detectable just like UART_BUG_TXEN.
> @@ -1366,6 +1406,31 @@
> handled = 1;
>
> end = NULL;
> +#ifdef CONFIG_PMC_MSP
> + } else if ((iir & UART_IER_BUSY) == UART_IER_BUSY) {
Hm, masking IIR with IER mask, is this correct? Doubt it.
> + /*
> + * The MSP (DesignWare APB UART) serial subsystem has a
> + * non-standard interrupt condition (0x7) which means
> + * that the LCR was written while the UART was busy, so
> + * the LCR was not actually written. It is cleared by
> + * reading the special non-standard extended UART status
> + * register.
> + */
> + unsigned int tmp;
> + if( up->port.line == 0 )
> + tmp = *UART0_STATUS_REG;
> + else
> + tmp = *UART1_STATUS_REG;
> +
> + /* Check if saved on LCR write */
> + if( up->dwapb_lcr != -1 )
> + serial_outp(up, UART_LCR, up->dwapb_lcr);
> + else
> + printk(KERN_ERR "serial8250: UART BUSY, no LCR
> write!\n" );
> +
> + handled = 1;
> + end = NULL;
> +#endif
Not sure if this also shouldn't be handled in other places which check for
interrupt status, like serial8250_timeout()...
[...]
> Index: linux_2_6/drivers/serial/8250.h
> ===================================================================
> RCS file: linux_2_6/drivers/serial/8250.h,v retrieving revision
> 1.1.1.6 retrieving revision 1.4 diff -u -r1.1.1.6 -r1.4
> --- linux_2_6/drivers/serial/8250.h 19 Oct 2006 21:00:58 -0000 1.1.1.6
> +++ linux_2_6/drivers/serial/8250.h 19 Oct 2006 22:08:15 -0000 1.4
> @@ -49,6 +49,7 @@
> #define UART_BUG_QUOT (1 << 0) /* UART has buggy quot LSB */
> #define UART_BUG_TXEN (1 << 1) /* UART has buggy TX IIR status
> */
> #define UART_BUG_NOMSR (1 << 2) /* UART has buggy MSR status
> bits (Au1x00) */
> +#define UART_BUG_DWTHRE (1 << 3) /* UART has buggy DesignWare THRE
> interrupt re-assertion */
>
> #define PROBE_RSA (1 << 0)
> #define PROBE_ANY (~0)
> Index: linux_2_6/include/linux/serial_core.h
> ===================================================================
> RCS file: linux_2_6/include/linux/serial_core.h,v
> retrieving revision 1.1.1.7
> retrieving revision 1.5
> diff -u -r1.1.1.7 -r1.5
> --- linux_2_6/include/linux/serial_core.h 19 Oct 2006 21:01:02 -0000
> 1.1.1.7
> +++ linux_2_6/include/linux/serial_core.h 19 Oct 2006 22:08:16 -0000
> 1.5
> @@ -258,6 +258,8 @@
> #define UPF_CONS_FLOW ((__force upf_t) (1 << 23))
> #define UPF_SHARE_IRQ ((__force upf_t) (1 << 24))
> #define UPF_BOOT_AUTOCONF ((__force upf_t) (1 << 28))
> +#define UPF_DW_THRE_BUG ((__force upf_t)(1 << 29)) /*
> DesignWare THRE hardware BUG
The need for the new flag seems doubtful to me.
> +
> * (cannot be autodetected) */
The patch is linewrapped
> Index: linux_2_6/include/linux/serial_reg.h
> ===================================================================
> RCS file: linux_2_6/include/linux/serial_reg.h,v
> retrieving revision 1.1.1.2
> retrieving revision 1.3
> diff -u -r1.1.1.2 -r1.3
> --- linux_2_6/include/linux/serial_reg.h 19 Oct 2006 18:29:50 -0000
> 1.1.1.2
> +++ linux_2_6/include/linux/serial_reg.h 19 Oct 2006 19:45:04 -0000
> 1.3
> @@ -218,6 +218,10 @@
> #define UART_FCR_PXAR16 0x80 /* receive FIFO treshold = 16 */
> #define UART_FCR_PXAR32 0xc0 /* receive FIFO treshold = 32 */
>
> +/*
> + * DesignWare APB UART
> + */
> +#define UART_IER_BUSY 0x07 /* Busy Detect */
Are you sure it's not *IIR* value? Doesn't look like interrupt mask for
IER. And IIR value of 7 already means something else, namely, no interrupt and
receiver status. Hm...
MBR, Sergei
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
