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: <[email protected]>, <[email protected]>
References:
<[email protected]_nt.nt.pmc-sierra.bc.ca>
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 <[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?
> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
