Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er

From: Marc St-Jean
Date: Mon Jan 22 2007 - 15:36:02 EST

Next message: lkml: "Re: OT: snow tux"
Previous message: Marc St-Jean: "RE: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git master"
In reply to: Marc St-Jean: "[PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

CCing to linux-kernel as per AC's suggestion...

-------- Original Message --------
Subject: Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git
mast er
Date: Mon, 22 Jan 2007 12:23:56 -0800
From: Sergei Shtylyov
Organization: MontaVista Software Inc.
To: Marc St-Jean
CC: <linux-mips@xxxxxxxxxxxxxx>, <linux-serial@xxxxxxxxxxxxxxx>
References:
<5C1FD43E5F1B824E83985A74F396286E03AD7632@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>

Hello.

Marc St-Jean wrote:

Your maile is strange: it line-wraps quotes but not your own text...
:-/

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

> Relative to the other two. Any recommended improvements on this one?

I already told: runtime check.

>>>Thanks,
>>>Marc

>>>Signed-off-by: Marc St-Jean <Marc_St-Jean@xxxxxxxxxxxxxx>

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

> Possibly but the current driver doesn't always save the LCR value
before trying to write it. For example see serial8250_set_termios()

> serial_outp(up, UART_LCR, cval); /* reset DLAB */
> up->lcr = cval; /* Save LCR */

This certainly may be fixed.

> It's impossible to ensure that going foward the driver will always
save the value before writing it.
> We need to have it saved before since the write will cause an
interrupt, hence the save in serial_out.

You may rework driver to always save it on write to 'lcr' field (in
case
of your UART). I don't think it's going to spoil something there...

> If that's not acceptable I can audit the driver and ensure all LCR
writes are first saved.

What's *certainly* undesirable is #ifdef mess. :-)

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

> I'm willing to do this, however IIRC there was a thread in late Sept.
which came
> to the conclusion that UPIO were to be used for different access
methods and not UART types.

AFAIR, almost every participant was left with his own opinion. I myself
changed it twice during the discussion, to finalyl mostly agree with
Russell. :-D

> Do you see this as an exception?

Well, there's already an incident of using this to work around the
register access errata (that UPIO_TSI I mentioned) -- and I must note that
this is certainly cleaner than #ifdef's, which should be avoided at all
costs. :-)

> In the second #ifdef CONFIG_PMC_MSP above the workaround is required
because of SoC
> interrupt design which is out-of-band with respect to r/w of UART
registers, it's not
> specific to the DesignWare UART.

Erm... so why is this under #ifdef?

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

> I didn't write the code so I haven't tried it personally but I
believe it was investigated.

> It looks like the results of the bugs are similar but the causes are
different. In the UART_BUG_TXEN case,
> if I understand the comment correctly, NO interrupt is generated on
enabling THRI.
> This is verified by looking for Transmitter Empty (0x40) and no
interrupt.

I must note that "transmitter empty" condition in *not* the cause of
the
THRI interrupt -- it signifies that the TX shift register is empty, not
the TX
holding register.

> In the UART_BUG_DWTHRE case an interrupt IS generated on enabling THRI.

If there was no prior sent characters, how it's generated?

> However no new THRE interupts will occur until a new character is
written
to the THR and it's sent.
> This is verified by looking for Transmit-Hold-Register Empty (0x20).

Again, THRE condition doesn't mean that the charecter is actually
sent. I
only means that it's been stored to the shift register and so, the
transmission started...

>> In any case, looks like this errata is auto-detectable just
like UART_BUG_TXEN.

> I don't know how we would be able to test this without sending junk
test characters
> to the console port.

IIUC, you must *not* send characters to detect it. :-)

> We currently enable the bug flag in our platform setup code when we
know the bug is present from the SoC version.
> Is that not acceptable?

Well, if that errata is indeed not readily detectable at runtime (or
even
effectively the same as already handled),
this seems like the only option indeed...

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

> I agree, that was badly named. I've changed it to UART_IIR_BUSY for
the next spin.

>>>+ /*
>>>+ * 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()...

> I can move the code to a function but I still see two issues:
> A) We could move the code the a new function. We could also check for
a new UART_BUG_* flag before calling the function,

> but in reality this is a feature of the UART not a bug.

May also check for certain UPIO_* once it's introduced...

> B) How to eliminate the platform #ifdef. How to pass in the address
of the UARTx_STATUS_REG in a platform independent way?

I'd consider defining the UART status reg. in serial_reg.h and also
doing
reads via serial_in(),
again under a new UPIO_* case...

> We could add it to one of the data structures like uart_port, but it
would need an #ifdef in the header file.

Add what, register?

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

> As mentioned earlier I have now renamed this UART_IIR_BUSY.

> From serial_reg.h, the receiver status is 0x06 and no interrupt is
0x01. I thought these couldn't both be set at the same time?

> In any case this is how DesignWare implemented the status so we can't
change it now.

Well, bits 1-2 have no meaning when bit 0 is set. Probably, chip
designers
decided to abuse this. :-)

> Thanks for the feedback,
> Marc

MBR, Sergei

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: lkml: "Re: OT: snow tux"
Previous message: Marc St-Jean: "RE: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git master"
In reply to: Marc St-Jean: "[PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]