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

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

[Marc St-Jean]

Thank Alan. I made the changes yesterday but I'll wait another day
before reposting, in case other interested people have more comments.

Marc

Alan wrote:
>  >       unsigned char           hub6;                   /* this should 
> be in the 8250 driver */
>  >       unsigned char           unused[3];
>  > +     void                            *data;                  /* 
> generic platform data pointer */
>  >   };
> 
> Convention is "private_data"
> 
>  >
>  >   /*
>  > diff --git a/include/linux/serial_reg.h b/include/linux/serial_reg.h
>  > index 3c8a6aa..b3550cc 100644
>  > --- a/include/linux/serial_reg.h
>  > +++ b/include/linux/serial_reg.h
>  > @@ -37,6 +37,7 @@ #define UART_IIR_MSI                0x00 /* Modem stat
>  >   #define UART_IIR_THRI               0x02 /* Transmitter holding 
> register empty */
>  >   #define UART_IIR_RDI                0x04 /* Receiver data interrupt */
>  >   #define UART_IIR_RLSI               0x06 /* Receiver line status 
> interrupt */
>  > +#define UART_IIR_BUSY                0x07 /* DesignWare APB Busy 
> Detect */
> 
> Please move this down a line to break it from "official" values and call
> it DESIGNWARE_UART_IIR_BUSY, so it is obviously designware specific.
> 
> Otherwise looks much less invasive and messy
> 

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

[Marc St-Jean]

Sergei Shtylyov wrote:
> Hello.
> 
> Marc St-Jean wrote:
> 
>  > There are three different fixes:
>  > 1. Fix for DesignWare APB THRE errata:
>  > 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.
>  > It appears that the "8250-uart-backup-timer.patch" in the "mm" tree also
>  > fixes it so we have dropped our initial workaround.
>  > This patch now needs to be applied on top of that "mm" patch.
> 
>     This patch has hit the mainline kernel already.

I see, I'll drop the reference to the "mm" patch.


>  > 3. Workaround for interrupt/data concurrency issue:
>  > The SoC needs to ensure that writes that can cause interrupts to be
>  > cleared reach the UART before returning from the ISR. This fix reads
>  > a non-destructive register on the UART so the read transaction
>  > completion ensures the previously queued write transaction has
>  > also completed.
> 
>  > The differences from the last attempt is dropping the call to
>  > in_irq() and including more detailed description of the fixes.
> 
>     The difference part would better be under the "---" cutoff line, along
> with diffstat.
> This way it gets auto-removed by quilt/git when applying the patch.

I'll add to the next posting.

>  > diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
>  > index 3d91bfc..bfaacc5 100644
>  > --- a/drivers/serial/8250.c
>  > +++ b/drivers/serial/8250.c
>  > @@ -308,6 +308,7 @@ static unsigned int serial_in(struct uar
>  >               return inb(up->port.iobase + 1);
>  > 
>  >       case UPIO_MEM:
>  > +     case UPIO_DWAPB:
>  >               return readb(up->port.membase + offset);
>  > 
>  >       case UPIO_MEM32:
>  > @@ -333,6 +334,8 @@ #endif
>  >  static void
>  >  serial_out(struct uart_8250_port *up, int offset, int value)
>  >  {
>  > +     /* Save the offset before it's remapped */
>  > +     int save_offset = offset;
>  >       offset = map_8250_out_reg(up, offset) << up->port.regshift;
>  > 
>  >       switch (up->port.iotype) {
> 
>     I've just got an idea how to "beautify" this code -- rename the 
> 'offset'
> arg to something like reg, and then declare/init 'offset' as local 
> variable.
> Would certainly look better (and worth doing in all serial_* accessros).

I agree but am having enough of a hard time getting the bare minimum
accepted that I don't dare touch any unnecessary lines.


>  > @@ -359,6 +362,18 @@ #endif
>  >                       writeb(value, up->port.membase + offset);
>  >               break;
>  > 
>  > +     case UPIO_DWAPB:
>  > +             /* Save the LCR value so it can be re-written when a
>  > +              * Busy Detect interrupt occurs. */
>  > +             if (save_offset == UART_LCR)
>  > +                     up->lcr = value;
>  > +             writeb(value, up->port.membase + offset);
>  > +             /* Read the IER to ensure any interrupt is cleared before
>  > +              * returning from ISR. */
>  > +             if (save_offset == UART_TX || save_offset == UART_IER)
>  > +                     value = serial_in(up, UART_IER);
>  > +             break;
>  > +            
>  >       default:
>  >               outb(value, up->port.iobase + offset);
>  >       }
>  > @@ -2454,8 +2485,8 @@ int __init serial8250_start_console(stru
>  > 
>  >       add_preferred_console("ttyS", line, options);
>  >       printk("Adding console on ttyS%d at %s 0x%lx (options '%s')\n",
>  > -             line, port->iotype == UPIO_MEM ? "MMIO" : "I/O port",
>  > -             port->iotype == UPIO_MEM ? (unsigned long) port->mapbase :
>  > +             line, port->iotype >= UPIO_MEM ? "MMIO" : "I/O port",
>  > +             port->iotype >= UPIO_MEM ? (unsigned long) port->mapbase :
>  >                   (unsigned long) port->iobase, options);
>  >       if (!(serial8250_console.flags & CON_ENABLED)) {
>  >               serial8250_console.flags &= ~CON_PRINTBUFFER;
> 
>     I've remembered why I decided not to fix it: this code only gets called
> from 8250__eraly.c which can't handle anything beyond UPIO_MEM anyway.

We don't use 8250_early and I've tested it works without, so I'll drop this
change.

Thanks,
Marc

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

[Sergei Shtylyov]

Hello.

Marc St-Jean wrote:

>> > diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
>> > index 3d91bfc..bfaacc5 100644
>> > --- a/drivers/serial/8250.c
>> > +++ b/drivers/serial/8250.c
>> > @@ -308,6 +308,7 @@ static unsigned int serial_in(struct uar
>> >               return inb(up->port.iobase + 1);
>> > 
>> >       case UPIO_MEM:
>> > +     case UPIO_DWAPB:
>> >               return readb(up->port.membase + offset);
>> > 
>> >       case UPIO_MEM32:
>> > @@ -333,6 +334,8 @@ #endif
>> >  static void
>> >  serial_out(struct uart_8250_port *up, int offset, int value)
>> >  {
>> > +     /* Save the offset before it's remapped */
>> > +     int save_offset = offset;
>> >       offset = map_8250_out_reg(up, offset) << up->port.regshift;
>> > 
>> >       switch (up->port.iotype) {

>>    I've just got an idea how to "beautify" this code -- rename the 
>>'offset'
>>arg to something like reg, and then declare/init 'offset' as local 
>>variable.
>>Would certainly look better (and worth doing in all serial_* accessros).

> I agree but am having enough of a hard time getting the bare minimum
> accepted that I don't dare touch any unnecessary lines.

    Well, I can try pushing this simple cleanup myself... seems worth doing 
for alike future cases anyway.

>> > @@ -359,6 +362,18 @@ #endif
>> >                       writeb(value, up->port.membase + offset);
>> >               break;
>> > 
>> > +     case UPIO_DWAPB:
>> > +             /* Save the LCR value so it can be re-written when a
>> > +              * Busy Detect interrupt occurs. */
>> > +             if (save_offset == UART_LCR)
>> > +                     up->lcr = value;
>> > +             writeb(value, up->port.membase + offset);
>> > +             /* Read the IER to ensure any interrupt is cleared before
>> > +              * returning from ISR. */
>> > +             if (save_offset == UART_TX || save_offset == UART_IER)
>> > +                     value = serial_in(up, UART_IER);
>> > +             break;
>> > +            
>> >       default:
>> >               outb(value, up->port.iobase + offset);
>> >       }
>> > @@ -2454,8 +2485,8 @@ int __init serial8250_start_console(stru
>> > 
>> >       add_preferred_console("ttyS", line, options);
>> >       printk("Adding console on ttyS%d at %s 0x%lx (options '%s')\n",
>> > -             line, port->iotype == UPIO_MEM ? "MMIO" : "I/O port",
>> > -             port->iotype == UPIO_MEM ? (unsigned long) port->mapbase :
>> > +             line, port->iotype >= UPIO_MEM ? "MMIO" : "I/O port",
>> > +             port->iotype >= UPIO_MEM ? (unsigned long) port->mapbase :
>> >                   (unsigned long) port->iobase, options);
>> >       if (!(serial8250_console.flags & CON_ENABLED)) {
>> >               serial8250_console.flags &= ~CON_PRINTBUFFER;

>>    I've remembered why I decided not to fix it: this code only gets called
>>from 8250__eraly.c which can't handle anything beyond UPIO_MEM anyway.

> We don't use 8250_early and I've tested it works without, so I'll drop this
> change.

    No need I guess since this is the Right Thing (TM) anyway.

> Thanks,
> Marc

WBR, Sergei

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

[Marc St-Jean]

Andrew Morton wrote:
> On Thu, 15 Feb 2007 13:26:29 -0600
> Marc St-Jean <stjeanma@pmc-sierra.com> wrote:
> 
>  > +                     status = *(volatile u32 *)up->port.private_data;
> 
> It distresses me that this patch uses a variable which this patch
> doesn't initialise anywhere.  It isn't complete.
> 
> The sub-driver code whch sets up this field shuld be included in the
> patch, no?
> 

OK, I'll re-post with the platform file which initializes the variable.
That file however will reference other files in the platform which has
not been posted yet. I thought it was better to post the driver-only code.

Marc

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

[Marc St-Jean]

Andrew Morton wrote:
> On Mon, 12 Feb 2007 12:04:08 -0600 Marc St-Jean 
> <stjeanma@pmc-sierra.com> wrote:
> 
>  > There are three different fixes:
>  > 1. Fix for DesignWare THRE errata
>  > - Dropped our fix since the "8250-uart-backup-timer.patch" in the "mm"
>  > tree also fixes it. This patch needs to be applied on top of "mm" patch.
>  >
>  > 2. Fix for Busy Detect on LCR write
>  > - No changes since last patch.
>  >
>  > 3. Workaround for interrupt/data concurrency issue
>  > - No changes since last patch.
> 
> A couple of things.
> 
> - When preparing a changelog, please just tell us what the patch does.
>   The information about how this patch differes from a previous version of
>   the patch is irrelevant when the patch hits the mainline repository hence
>   I must edit it all.
> 
>   It's OK to add the what-i-changed-since-last-time details, but please 
> make
>   that separate and remember that it will be removed for when the patch 
> goes
>   upstream.
> 
> - When fixing a bug, please tell us in detail what that bug *is*.  None
>   of the above three items tell us any of this, which is essential
>   information for those who are to review the change.

Understood, I'm adding the original description here and I'll add to the
next patch update.

1. Fix for DesignWare APB THRE errata:
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.
It appears that the "8250-uart-backup-timer.patch" in the "mm" tree also
fixes it so we have dropped our initial workaround.
This patch now needs to be applied on top of that "mm" patch.

2. Fix for Busy Detect on LCR write:
The DesignWare APB UART has a feature which causes a new Busy Detect
interrupt to be generated if it's busy when the LCR is written. This
fix saves the value of the LCR and rewrites it after clearing the
interrupt.

3. Workaround for interrupt/data concurrency issue:
The SoC needs to ensure that writes that can cause interrupts to be
cleared reach the UART before returning from the ISR. This fix reads
a non-destructive register on the UART so the read transaction
completion ensures the previously queued write transaction has
also completed.


>  > 
>  > +     case UPIO_DWAPB:
>  > +             /* Save the LCR value so it can be re-written when a
>  > +              * Busy Detect interrupt occurs. */
>  > +             if (save_offset == UART_LCR)
>  > +                     up->lcr = value;
>  > +             writeb(value, up->port.membase + offset);
>  > +             /* Read the IER to ensure any interrupt is cleared before
>  > +              * returning from ISR. */
>  > +             if ((save_offset == UART_TX || save_offset == UART_IER) 
> && in_irq())
> 
> The in_irq() is a worry.  If the serial driver is used as the system
> console, then it can be called from _any_ interrupt handler.  eg a printk()
> in the sata code.
> 
> What happens then?

The in_irq() is there to improve performance. The logic being that
writing to registers outside the interrupt context will not require
the fix for issue 3.
There should be no issues if called from other interrupt context
as the read is non-destructive. I can remove the call to in_irq() if
you prefer.


>  > @@ -1383,6 +1399,19 @@ static irqreturn_t serial8250_interrupt(
>  >                       handled = 1;
>  > 
>  >                       end = NULL;
>  > +             } else if (up->port.iotype == UPIO_DWAPB &&
>  > +                       (iir & UART_IIR_BUSY) == UART_IIR_BUSY) {
>  > +                     /* The DesignWare APB UART has an Busy Detect 
> (0x07)
>  > +                      * interrupt meaning an LCR write attempt 
> occured while the
>  > +                      * UART was busy. The interrupt must be cleared 
> by reading
>  > +                      * the UART status register (USR) and the LCR 
> re-written. */
>  > +                     unsigned int status;
>  > +                     status = *(volatile u32 *)up->port.private_data;
> 
> Are you sure this is right?  The use of volatile is generally discouraged
> and is a sign that something is wrong.  What is the driver attempting to
> read here?  Should it be using readl()?

The driver is reading the UART's USR (UART Status Register) which is
a non-standard register at a non-fixed offset, hence the need for
private_data. This was discussed in detail in the patch thread and the
goal was to avoid using platform specific #ifdefs as part of the fix

The register is accessed in kseg1 (unmapped on the mips architecture) so
readl is not needed. The register definitions in this space are all marked
volatile but since it's now accessed through private_data, the read had
to be made explicitly volatile.


> Also, the newly-added private_data field is not being initialised to
> anything anywhere in this patch.

private_data is initialized in the platform specific initialization code
which will be submitted to the l-m.o That patch contains only code which
lives in arch/mips and include/asm-mips so I was not planning on
submitting to the main kernel list. This level of procedural detail is
not in the maillist FAQ, I'm assuming that is normal procedure?

Marc

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

[Marc St-Jean]

Sergei Shtylyov wrote:
> Hello.
> 
> Marc St-Jean wrote:
> 
>  >> > Fourth attempt at the serial driver patch for the PMC-Sierra MSP71xx
>  >>device.
> 
>     I think you need to submit your patch to Andrew Morton since it 
> requires a patch from his tree.

OK, will do.


>  >> > @@ -1383,6 +1399,19 @@ static irqreturn_t serial8250_interrupt(
>  >> >                       handled = 1;
>  >> >
>  >> >                       end = NULL;
>  >> > +             } else if (up->port.iotype == UPIO_DWAPB &&
>  >> > +                             (iir & UART_IIR_BUSY) == 
> UART_IIR_BUSY) {
> 
>  >>     Worth aligning this line with the opening paren of if... but's 
> that's
>  >>nitpicking. :-)
> 
>  > No problem I'll change it. I just usually align to the closest tab 
> stop to
>  > the opening parenthesis.
> 
>     It haven't really changed in the last patch. :-)

I will respin with 8 char tabs.


>  >> > +                     /* The DesignWare APB UART has an Busy Detect
>  >>(0x07)
>  >> > +                      * interrupt meaning an LCR write attempt
>  >>occured while the
>  >> > +                      * UART was busy. The interrupt must be cleared
>  >>by reading
>  >> > +                      * the UART status register (USR) and the LCR
>  >>re-written. */
>  >> > +                     unsigned int status;
>  >> > +                     status = *(volatile u32 
> *)up->port.private_data;
>  >> > +                     serial_out(up, UART_LCR, up->lcr);
>  >> > +
>  >> > +                     handled = 1;
>  >> > +
>  >> > +                     end = NULL;
>  >> >               } else if (end == NULL)
>  >> >                       end = l;
>  >> >
>  >> >       return 0;
> 
>  >>    Still, shouldn't you be doing this in serial8250_timeout()
> 
>  > No, the serial8250_timeout is for issue 1 at the top, this is for
>  > issue 2.
> 
>     It's for lost interrupts, IIUC. They use anothe timeout handler for the
> workaround...

This issue (2) is a completely new type of interrupt generated but the
DesignWare APB uart, it has nothing to do with lost interrupts.


>  >>also?
>  >>What IRQ numbers this UART is using, BTW?
> 
>  > For the ports on the device they are 27 and 42. Is there any 
> significance
>  > that I'm not aware of?
> 
>     Yeah, IRQ0 is treated as no IRQ by 8250, and in this case it falls 
> back to
> using serial8250_timeout() to handle "interrupts".

Good to know. It won't be affecting us then.


> 
>  >>    Oops, your mailer went and did it again. :-)
> 
>  > I'm completely giving up on Thunderbird,unless someone can point out
> 
>     Ypu should have long ago. :-)
> 
>  > the specific internal configuration items which needs a kick!
> 
>     Only the attachments will work in the Mozilla kind mailer, AFAIK.
> The last patch looked OK at last. :-)

The attachemnents appear to be MIME which is a no-no according the
linux FAQ at kernel.org. I guess I'll stick with /bin/mail.

Thanks,
Marc

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

[Sergei Shtylyov]

Hello.

Marc St-Jean wrote:

>> >> > Fourth attempt at the serial driver patch for the PMC-Sierra MSP71xx
>> >>device.

>>    I think you need to submit your patch to Andrew Morton since it 
>>requires a patch from his tree.

> OK, will do.

    In fact, since the serial drivers are not maintained anymore, this seems 
the only option.

>> >> > +                     /* The DesignWare APB UART has an Busy Detect
>> >>(0x07)
>> >> > +                      * interrupt meaning an LCR write attempt
>> >>occured while the
>> >> > +                      * UART was busy. The interrupt must be cleared
>> >>by reading
>> >> > +                      * the UART status register (USR) and the LCR
>> >>re-written. */
>> >> > +                     unsigned int status;
>> >> > +                     status = *(volatile u32 
>>*)up->port.private_data;
>> >> > +                     serial_out(up, UART_LCR, up->lcr);
>> >> > +
>> >> > +                     handled = 1;
>> >> > +
>> >> > +                     end = NULL;
>> >> >               } else if (end == NULL)
>> >> >                       end = l;

>> >> >       return 0;

>> >>    Still, shouldn't you be doing this in serial8250_timeout()

>> > No, the serial8250_timeout is for issue 1 at the top, this is for
>> > issue 2.

>>    It's for lost interrupts, IIUC. They use anothe timeout handler for the
>>workaround...

> This issue (2) is a completely new type of interrupt generated but the
> DesignWare APB uart, it has nothing to do with lost interrupts.

    Yeah, I just thought that the lost interrupts might be a "generic" issue.

>> >>also?
>> >>What IRQ numbers this UART is using, BTW?

>> > For the ports on the device they are 27 and 42. Is there any 
>>significance
>> > that I'm not aware of?

>>    Yeah, IRQ0 is treated as no IRQ by 8250, and in this case it falls 
>>back to using serial8250_timeout() to handle "interrupts".

> Good to know. It won't be affecting us then.

    This may be overriden anyway...

>> >>    Oops, your mailer went and did it again. :-)

>> > I'm completely giving up on Thunderbird,unless someone can point out

>>    Ypu should have long ago. :-)

>> > the specific internal configuration items which needs a kick!

>>    Only the attachments will work in the Mozilla kind mailer, AFAIK.
>>The last patch looked OK at last. :-)

> The attachemnents appear to be MIME which is a no-no according the

    The text/plain type attachments seem to be acceptable for the most 
maintainers.  This Mozilla can do, at least. :-)

> linux FAQ at kernel.org. I guess I'll stick with /bin/mail.

> Thanks,
> Marc

WBR, Sergei

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

[Marc St-Jean]

Sergei Shtylyov wrote:
> Marc St-Jean wrote:
>  > Fourth attempt at the serial driver patch for the PMC-Sierra MSP71xx 
> device.
>  >
>  > There are three different fixes:
>  > 1. Fix for DesignWare THRE errata
>  > - Dropped our fix since the "8250-uart-backup-timer.patch" in the "mm"
>  > tree also fixes it. This patch needs to be applied on top of "mm" patch.
>  >
>  > 2. Fix for Busy Detect on LCR write
>  > - Changed the ordering of test in serial8250_interrupt().
>  > - Combined test for UPIO_DWAPB with UPIO_MEM in 
> serial8250_start_console().
>  > - Renamed new uart_8250_port member to private_data.
>  >
>  > 3. Workaround for interrupt/data concurrency issue
>  > - No changes since last patch.
> 
>  > @@ -1383,6 +1399,19 @@ static irqreturn_t serial8250_interrupt(
>  >                       handled = 1;
>  >
>  >                       end = NULL;
>  > +             } else if (up->port.iotype == UPIO_DWAPB &&
>  > +                             (iir & UART_IIR_BUSY) == UART_IIR_BUSY) {
> 
>      Worth aligning this line with the opening paren of if... but's that's
> nitpicking. :-)

No problem I'll change it. I just usually align to the closest tab stop to
the opening parenthesis.


>  > +                     /* The DesignWare APB UART has an Busy Detect 
> (0x07)
>  > +                      * interrupt meaning an LCR write attempt 
> occured while the
>  > +                      * UART was busy. The interrupt must be cleared 
> by reading
>  > +                      * the UART status register (USR) and the LCR 
> re-written. */
>  > +                     unsigned int status;
>  > +                     status = *(volatile u32 *)up->port.private_data;
>  > +                     serial_out(up, UART_LCR, up->lcr);
>  > +
>  > +                     handled = 1;
>  > +
>  > +                     end = NULL;
>  >               } else if (end == NULL)
>  >                       end = l;
>  >
>  >       return 0;
> 
>     Still, shouldn't you be doing this in serial8250_timeout()

No, the serial8250_timeout is for issue 1 at the top, this is for
issue 2.


> also?
> What IRQ numbers this UART is using, BTW?

For the ports on the device they are 27 and 42. Is there any significance
that I'm not aware of?


>  > diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
>  > index cf23813..bd9711a 100644
>  > --- a/include/linux/serial_core.h
>  > +++ b/include/linux/serial_core.h
>  > @@ -276,6 +277,7 @@ #define UPF_USR_MASK              ((__force upf_t) (
>  >       struct device           *dev;                   /* parent 
> device */
>  >       unsigned char           hub6;                   /* this should 
> be in the 8250 driver */
>  >       unsigned char           unused[3];
>  > +     void                            *private_data;          /* 
> generic platform data pointer */
> 
>     One tab to many before *private_data...

In my editor using 4 columns per tab it lines up with everything. Is there
some convention that patches should be made assuming 8 columns?


>  > diff --git a/include/linux/serial_reg.h b/include/linux/serial_reg.h
>  > index 3c8a6aa..1c5ed7d 100644
>  > --- a/include/linux/serial_reg.h
>  > +++ b/include/linux/serial_reg.h
>  > @@ -38,6 +38,8 @@ #define UART_IIR_THRI               0x02 /* Transmitt
>  >   #define UART_IIR_RDI                0x04 /* Receiver data interrupt */
>  >   #define UART_IIR_RLSI               0x06 /* Receiver line status 
> interrupt */
>  >
>  > +#define UART_IIR_BUSY                0x07 /* DesignWare APB Busy 
> Detect */
>  > +
>  >   #define UART_FCR    2       /* Out: FIFO Control Register */
>  >   #define UART_FCR_ENABLE_FIFO        0x01 /* Enable the FIFO */
>  >   #define UART_FCR_CLEAR_RCVR 0x02 /* Clear the RCVR FIFO */
> 
>     Oops, your mailer went and did it again. :-)

I'm completely giving up on Thunderbird,unless someone can point out
the specific internal configuration items which needs a kick!

Thanks,
Marc

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

[Sergei Shtylyov]

Hello.

Marc St-Jean wrote:

>> > Fourth attempt at the serial driver patch for the PMC-Sierra MSP71xx 
>>device.

>> > There are three different fixes:
>> > 1. Fix for DesignWare THRE errata
>> > - Dropped our fix since the "8250-uart-backup-timer.patch" in the "mm"
>> > tree also fixes it. This patch needs to be applied on top of "mm" patch.

    I think you need to submit your patch to Andrew Morton since it requires a 
patch from his tree.

>> > @@ -1383,6 +1399,19 @@ static irqreturn_t serial8250_interrupt(
>> >                       handled = 1;
>> >
>> >                       end = NULL;
>> > +             } else if (up->port.iotype == UPIO_DWAPB &&
>> > +                             (iir & UART_IIR_BUSY) == UART_IIR_BUSY) {

>>     Worth aligning this line with the opening paren of if... but's that's
>>nitpicking. :-)

> No problem I'll change it. I just usually align to the closest tab stop to
> the opening parenthesis.

    It haven't really changed in the last patch. :-)

>> > +                     /* The DesignWare APB UART has an Busy Detect 
>>(0x07)
>> > +                      * interrupt meaning an LCR write attempt 
>>occured while the
>> > +                      * UART was busy. The interrupt must be cleared 
>>by reading
>> > +                      * the UART status register (USR) and the LCR 
>>re-written. */
>> > +                     unsigned int status;
>> > +                     status = *(volatile u32 *)up->port.private_data;
>> > +                     serial_out(up, UART_LCR, up->lcr);
>> > +
>> > +                     handled = 1;
>> > +
>> > +                     end = NULL;
>> >               } else if (end == NULL)
>> >                       end = l;
>> >
>> >       return 0;

>>    Still, shouldn't you be doing this in serial8250_timeout()

> No, the serial8250_timeout is for issue 1 at the top, this is for
> issue 2.

    It's for lost interrupts, IIUC. They use anothe timeout handler for the 
workaround...

>>also?
>>What IRQ numbers this UART is using, BTW?

> For the ports on the device they are 27 and 42. Is there any significance
> that I'm not aware of?

    Yeah, IRQ0 is treated as no IRQ by 8250, and in this case it falls back to 
using serial8250_timeout() to handle "interrupts".

>> > diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
>> > index cf23813..bd9711a 100644
>> > --- a/include/linux/serial_core.h
>> > +++ b/include/linux/serial_core.h
>> > @@ -276,6 +277,7 @@ #define UPF_USR_MASK              ((__force upf_t) (
>> >       struct device           *dev;                   /* parent 
>>device */
>> >       unsigned char           hub6;                   /* this should 
>>be in the 8250 driver */
>> >       unsigned char           unused[3];
>> > +     void                            *private_data;          /* 
>>generic platform data pointer */

>>    One tab to many before *private_data...

> In my editor using 4 columns per tab it lines up with everything. Is there
> some convention that patches should be made assuming 8 columns?

    Documentation/CodingStyle, chapter 1. :-)

>>    Oops, your mailer went and did it again. :-)

> I'm completely giving up on Thunderbird,unless someone can point out

    Ypu should have long ago. :-)

> the specific internal configuration items which needs a kick!

    Only the attachments will work in the Mozilla kind mailer, AFAIK.
The last patch looked OK at last. :-)

> Thanks,
> Marc

WBR, Sergei

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

[Marc St-Jean]

Sergei Shtylyov wrote:
> Marc St-Jean wrote:
>  > Third attempt at the serial driver patch for the PMC-Sierra MSP71xx 
> device.
>  >
>  > There are three different fixes:
>  > 1. Fix for DesignWare THRE errata
>  > - Dropped our fix since the "8250-uart-backup-timer.patch" in the "mm"
>  > tree also fixes it. This patch needs to be applied on top of it.
>  >
>  > 2. Fix for Busy Detect on LCR write
>  > - Dropped the addition of UPIO_DWAPB iotype to 8250_early.c as Sergei
>  > pointed out the fix wasn't complete and we don't require it.
>  >
>  > 3. Workaround for interrupt/data concurrency issue
>  > - Fix must remain serial8250_interrupt() in order to mark interrupt as
>  > handled.
>  >
>  > Thanks,
>  > Marc
>  >
>  > Signed-off-by: Marc St-Jean <Marc_St-Jean@pmc-sierra.com>
>  >
>  > diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
...
>  >               break;
>  > @@ -1383,6 +1399,19 @@ static irqreturn_t serial8250_interrupt(
>  >                       handled = 1;
>  >
>  >                       end = NULL;
>  > +             } else if ((iir & UART_IIR_BUSY) == UART_IIR_BUSY &&
>  > +                             up->port.iotype == UPIO_DWAPB) {
> 
>     Makes sense to swap the checks, i.e. to only test for UART_IIR_BUSY is
> this is UPIO_DWAPB.

I had left in the other order for readability with the previous test.
I agree this will save a few cycles so I've reordered.


> [...]
> 
>  > @@ -2454,9 +2485,12 @@ int __init serial8250_start_console(stru
>  >
>  >       add_preferred_console("ttyS", line, options);
>  >       printk("Adding console on ttyS%d at %s 0x%lx (options '%s')\n",
>  > -             line, port->iotype == UPIO_MEM ? "MMIO" : "I/O port",
>  > -             port->iotype == UPIO_MEM ? (unsigned long) port->mapbase :
>  > -                 (unsigned long) port->iobase, options);
>  > +             line,
>  > +             (port->iotype == UPIO_MEM || port->iotype == UPIO_DWAPB)
>  > +                     ? "MMIO" : "I/O port",
>  > +             (port->iotype == UPIO_MEM || port->iotype == UPIO_DWAPB)
>  > +                     ? (unsigned long) port->mapbase : (unsigned 
> long) port->iobase,
>  > +             options);
> 
>     Please turn this check into port->iotype >= UPIO_MEM, since this 
> would be
> the Right Thing (RM).  All iotypes beyond UPIO_MEM are memory mapped.  
> And I
> thought I fixed that -- was wrong, obviously... :-/

This is news to me, I thought the numbering was of no particular importance.
I've made the change.


>  > diff --git a/include/linux/serial_reg.h b/include/linux/serial_reg.h
>  > index 3c8a6aa..b3550cc 100644
>  > --- a/include/linux/serial_reg.h
>  > +++ b/include/linux/serial_reg.h
>  > @@ -37,6 +37,7 @@ #define UART_IIR_MSI                0x00 /* Modem stat
>  >   #define UART_IIR_THRI               0x02 /* Transmitter holding 
> register empty */
>  >   #define UART_IIR_RDI                0x04 /* Receiver data interrupt */
>  >   #define UART_IIR_RLSI               0x06 /* Receiver line status 
> interrupt */
>  > +#define UART_IIR_BUSY                0x07 /* DesignWare APB Busy 
> Detect */
> 
>     Alan already said about this one... :-)

Yes, changed.

>     BTW, your patches are still corrupt by your mailer (space added to 
> lines
> starting with space)

Argh! There were no spaces in the message window before sending, I swear!
I've solved the problem for the next patch.

Marc

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

[Marc St-Jean]

Sergei Shtylyov wrote:
> Hello.
> 
> Marc St-Jean wrote:
> 
>  >> > Index: linux_2_6/drivers/serial/8250.c
>  >> > ===================================================================
>  >> > RCS file: linux_2_6/drivers/serial/8250.c,v
>  >> > retrieving revision 1.1.1.7
>  >> > diff -u -r1.1.1.7 8250.c
>  >> > --- 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   24 Jan 2007 23:55:27 -0000
>  >>[...]
>  >> > @@ -333,6 +334,8 @@
>  >> >   static void
>  >> >   serial_out(struct uart_8250_port *up, int offset, int value)
> 
>  >>   Your patch is clearly garbled again, something added an extra space
>  >>to all
>  >>lines stating with space... :-/
> 
>  > I see that but was under the impression cvs diff did that so all lines
>  > lineup correctly whether they have a +, -, or neither.
> 
>     Yes. And your mailer has probably added another space to lines already
> begginign with space. Some mailers tend to do it, don't know sure why... 
> :-/

I'll look into it for the next patch.


>  > It doesn't affect applying the patches for me, did you try?
> 
>     I hadn't but I had no doubts it'll fail... just tried, not a single 
> hunk
> applied. :-]
> 
>  >> >   {
>  >> > +     /* Save the offset before it's remapped */
>  >> > +     int save_offset = offset;
> 
>  >>    Is there real need to save this? What regshift equals for this UART?
> 
>  > Yes, we have a regshift of 2 on this SoC and it could be different on 
> other
>  > SoCs which reuse the UART IP.
> 
>     Agreed then (though still seems avoidable).
> 
>  >> >       offset = map_8250_out_reg(up, offset) << up->port.regshift;
> 
>  >> >       switch (up->port.iotype) {
>  >> > @@ -359,6 +362,19 @@
>  >> >                       writeb(value, up->port.membase + offset);
>  >> >               break;
>  >> >
>  >> > +     case UPIO_DWAPB:
>  >> > +             /* Save the LCR value so it can be re-written when a
>  >> > +              * Busy Detect interrupt occurs. */
>  >> > +             if (save_offset == UART_LCR)
>  >> > +                     up->lcr = value;
>  >> > +             writeb(value, up->port.membase + offset);
>  >> > +             /* Read the IER to ensure any interrupt is cleared 
> before
>  >> > +              * returning from ISR. */
>  >> > +             if ((save_offset == UART_TX || save_offset == 
> UART_IER) &&
> 
>  >>    Not sure how an IER read ensures that...
> 
>  > It's just a dummy read to ensure that any writes which clear 
> interrupts have
>  > reached the device before returning from the IRQ.
> 
>     Hm, isn't it CPU write buffer flush? Shouldn't this be handled more
> generically?

No because this peripheral is across an internal SoC bus. Only a read
accessing it will ensure the write is complete. We must insure the
interrupt source is cleared to avoid spurious interrupts.


> [...]
>  >> > Index: linux_2_6/drivers/serial/8250_early.c
>  >> > ===================================================================
>  >> > RCS file: linux_2_6/drivers/serial/8250_early.c,v
>  >> > retrieving revision 1.1.1.3
>  >> > diff -u -r1.1.1.3 8250_early.c
>  >> > --- linux_2_6/drivers/serial/8250_early.c     19 Oct 2006 20:08:20
>  >>-0000      1.1.1.3
>  >> > +++ linux_2_6/drivers/serial/8250_early.c     24 Jan 2007 23:55:27 
> -0000
>  >> > @@ -46,7 +46,7 @@
>  >> >
>  >> >   static unsigned int __init serial_in(struct uart_port *port, int
>  >>offset)
>  >> >   {
>  >> > -     if (port->iotype == UPIO_MEM)
>  >> > +     if (port->iotype == UPIO_MEM || port->iotype == UPIO_DWAPB)
>  >> >               return readb(port->membase + offset);
>  >> >       else
>  >> >               return inb(port->iobase + offset);
>  >> > @@ -54,7 +54,7 @@
>  >> >
>  >> >   static void __init serial_out(struct uart_port *port, int offset,
>  >>int value)
>  >> >   {
>  >> > -     if (port->iotype == UPIO_MEM)
>  >> > +     if (port->iotype == UPIO_MEM || port->iotype == UPIO_DWAPB)
>  >> >               writeb(value, port->membase + offset);
>  >> >       else
>  >> >               outb(value, port->iobase + offset);
>  >> > @@ -233,7 +233,7 @@
>  >> >               return 0;
>  >> >
>  >> >       /* Try to start the normal driver on a matching line.  */
>  >> > -     mmio = (port->iotype == UPIO_MEM);
>  >> > +     mmio = (port->iotype == UPIO_MEM || port->iotype == 
> UPIO_DWAPB);
>  >> >       line = serial8250_start_console(port, device->options);
>  >> >       if (line < 0)
>  >> >               printk("No ttyS device at %s 0x%lx for console\n",
> 
>  >>    From your 8250_eraly.c changes I can conclude regshift == 1 (it 
> doesn't
>  >>currently support other cases). So, serial_pot() doesn't need
>  >>save_offset. :-)
> 
>  > Our regshift is definitely 2 on this SoC and we don't have any 
> problems with
>  > console output before the serial driver opens. So assuming it's using
>  > 8250_early.c for initial console output I can only conclude that it 
> works
> 
>     It comes into action only if you specify console=uart,... kernel option
> for the eraly console support.  The "plain" 8250 console driver is 
> containded
> within 8250.c itself.
> 
>  > because UART_TX is offset 0 and the port was left configured from our
>  > ROM monitor.
> 
>     Well, this part of the patch can't be considered complete then (how LSR
> polling is going to work?), so should either be removed or the proper 
> regshift
> support added.

Since we don't require it I will drop the 8250_early.c changes from the patch.

I've tested the "mm tree" patch for the backup timer and it works, although
the output seems a little choppy at times. I'll drop the UART_BUG_DWTHRE flag
and associated code.

Marc

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

[Marc St-Jean]

Sergei Shtylyov wrote:
> Marc St-Jean wrote:
>  > Here is my second attempt at the serial driver patch for the
>  > PMC-Sierra MSP71xx device.
>  >
>  > There are three different fixes:
>  > 1. Fix for THRE errata
>  > - I verified the UART_BUG_TXEN fix does not help with this erratum.
>  > - I left our current fix in until I get our platform booting on
>  > 2.6.20-rc4 to try the mm tree "8250-uart-backup-timer.patch".
>  > Feel free to ignore for now.
>  >
>  > 2. Fix for Busy Detect on LCR write
>  > - Moved to new UPIO_DWAPB iotype. Because the new type is a memory
>  > mapped device and there are several tests for UPIO_MEM, this involved
>  > updating serial_core.c and 8250_early.c in addition to 8250.c.
>  > - I tried implementing this totally in serial_in as suggested, but
>  > it can't be done because of bit overlap between UART_IIR_NO_INT and
>  > UART_IIR_BUSY. Also there is no way to set the interrupt "handled = 1"
>  > from serial_in.
>  >
>  > 3. Workaround for interrupt/data concurrency issue
>  > - Moved to new UPIO_DWAPB iotype.
> 
>  > Index: linux_2_6/drivers/serial/8250.c
>  > ===================================================================
>  > RCS file: linux_2_6/drivers/serial/8250.c,v
>  > retrieving revision 1.1.1.7
>  > diff -u -r1.1.1.7 8250.c
>  > --- 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   24 Jan 2007 23:55:27 -0000
> [...]
>  > @@ -333,6 +334,8 @@
>  >   static void
>  >   serial_out(struct uart_8250_port *up, int offset, int value)
> 
>    Your patch is clearly garbled again, something added an extra space 
> to all
> lines stating with space... :-/

I see that but was under the impression cvs diff did that so all lines
lineup correctly whether they have a +, -, or neither. It doesn't affect
applying the patches for me, did you try?


>  >   {
>  > +     /* Save the offset before it's remapped */
>  > +     int save_offset = offset;
> 
>     Is there real need to save this? What regshift equals for this UART?

Yes, we have a regshift of 2 on this SoC and it could be different on other
SoCs which reuse the UART IP.


>  >       offset = map_8250_out_reg(up, offset) << up->port.regshift;
> 
>  >       switch (up->port.iotype) {
>  > @@ -359,6 +362,19 @@
>  >                       writeb(value, up->port.membase + offset);
>  >               break;
>  >
>  > +     case UPIO_DWAPB:
>  > +             /* Save the LCR value so it can be re-written when a
>  > +              * Busy Detect interrupt occurs. */
>  > +             if (save_offset == UART_LCR)
>  > +                     up->lcr = value;
>  > +             writeb(value, up->port.membase + offset);
>  > +             /* Read the IER to ensure any interrupt is cleared before
>  > +              * returning from ISR. */
>  > +             if ((save_offset == UART_TX || save_offset == UART_IER) &&
> 
>     Not sure how an IER read ensures that...

It's just a dummy read to ensure that any writes which clear interrupts have
reached the device before returning from the IRQ.


>  > +                             in_irq())
> 
>     I'd suggest to either indent this line right (start below 2ns paren 
> of if
> stmt) or keep on the same line.

OK, moved onto same line.


>  > +                     value = serial_in(up, UART_IER);
>  > +             break;
>  > +            
>  >       default:
>  >               outb(value, up->port.iobase + offset);
>  >       }
>  > @@ -1016,6 +1032,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 +1168,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)
> 
>     Why read IIR if you don't check it?

Good point. I didn't write that one and assumed reading the IIR was part of the fix.
It just tested fine without it so it's gone.


>  > @@ -2352,9 +2402,12 @@
>  >
>  >       add_preferred_console("ttyS", line, options);
>  >       printk("Adding console on ttyS%d at %s 0x%lx (options '%s')\n",
>  > -             line, port->iotype == UPIO_MEM ? "MMIO" : "I/O port",
>  > -             port->iotype == UPIO_MEM ? (unsigned long) port->mapbase :
>  > -                 (unsigned long) port->iobase, options);
>  > +             line,
>  > +             (port->iotype == UPIO_MEM || port->iotype == UPIO_DWAPB)
>  > +                     ? "MMIO" : "I/O port",
>  > +             (port->iotype == UPIO_MEM || port->iotype == UPIO_DWAPB)
>  > +                     ? (unsigned long) port->mapbase : (unsigned 
> long) port->iobase,
>  > +             options);
>  >       if (!(serial8250_console.flags & CON_ENABLED)) {
>  >               serial8250_console.flags &= ~CON_PRINTBUFFER;
>  >               register_console(&serial8250_console);
> [...]
>  > Index: linux_2_6/drivers/serial/8250_early.c
>  > ===================================================================
>  > RCS file: linux_2_6/drivers/serial/8250_early.c,v
>  > retrieving revision 1.1.1.3
>  > diff -u -r1.1.1.3 8250_early.c
>  > --- linux_2_6/drivers/serial/8250_early.c     19 Oct 2006 20:08:20 
> -0000      1.1.1.3
>  > +++ linux_2_6/drivers/serial/8250_early.c     24 Jan 2007 23:55:27 -0000
>  > @@ -46,7 +46,7 @@
>  >
>  >   static unsigned int __init serial_in(struct uart_port *port, int 
> offset)
>  >   {
>  > -     if (port->iotype == UPIO_MEM)
>  > +     if (port->iotype == UPIO_MEM || port->iotype == UPIO_DWAPB)
>  >               return readb(port->membase + offset);
>  >       else
>  >               return inb(port->iobase + offset);
>  > @@ -54,7 +54,7 @@
>  >
>  >   static void __init serial_out(struct uart_port *port, int offset, 
> int value)
>  >   {
>  > -     if (port->iotype == UPIO_MEM)
>  > +     if (port->iotype == UPIO_MEM || port->iotype == UPIO_DWAPB)
>  >               writeb(value, port->membase + offset);
>  >       else
>  >               outb(value, port->iobase + offset);
>  > @@ -233,7 +233,7 @@
>  >               return 0;
>  >
>  >       /* Try to start the normal driver on a matching line.  */
>  > -     mmio = (port->iotype == UPIO_MEM);
>  > +     mmio = (port->iotype == UPIO_MEM || port->iotype == UPIO_DWAPB);
>  >       line = serial8250_start_console(port, device->options);
>  >       if (line < 0)
>  >               printk("No ttyS device at %s 0x%lx for console\n",
> 
>     From your 8250_eraly.c changes I can conclude regshift == 1 (it doesn't
> currently support other cases). So, serial_pot() doesn't need 
> save_offset. :-)

Our regshift is definitely 2 on this SoC and we don't have any problems with
console output before the serial driver opens. So assuming it's using
8250_early.c for initial console output I can only conclude that it works
because UART_TX is offset 0 and the port was left configured from our
ROM monitor.


>  > Index: linux_2_6/drivers/serial/serial_core.c
>  > ===================================================================
>  > RCS file: linux_2_6/drivers/serial/serial_core.c,v
>  > retrieving revision 1.1.1.7
>  > diff -u -r1.1.1.7 serial_core.c
>  > --- linux_2_6/drivers/serial/serial_core.c    19 Oct 2006 21:00:58 
> -0000      1.1.1.7
>  > +++ linux_2_6/drivers/serial/serial_core.c    24 Jan 2007 23:55:28 -0000
>  > @@ -1669,9 +1669,10 @@
>  >
>  >       ret = sprintf(buf, "%d: uart:%s %s%08lX irq:%d",
>  >                       port->line, uart_type(port),
>  > -                     port->iotype == UPIO_MEM ? "mmio:0x" : "port:",
>  > -                     port->iotype == UPIO_MEM ? port->mapbase :
>  > -                                             (unsigned long) 
> port->iobase,
>  > +                     (port->iotype == UPIO_MEM || port->iotype == 
> UPIO_DWAPB)
>  > +                             ? "mmio:0x" : "port:",
>  > +                     (port->iotype == UPIO_MEM || port->iotype == 
> UPIO_DWAPB)
>  > +                             ? port->mapbase : (unsigned long) 
> port->iobase,
>  >                       port->irq);
>  >       if (port->type == PORT_UNKNOWN) {
> 
>     Needless change. My patch that fixes this function is in Linus' tree 
> since
> September, not sure why you don't have it:
> 
> http://www2.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=6c6a2334a1e8af7c3eaab992732825fa9ade77cf

I have it, I forgot to respin against the master.


> 
>  > 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
>  > diff -u -r1.1.1.7 serial_core.h
>  > --- 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     24 Jan 2007 23:55:28 -0000
> [...]
>  > @@ -274,6 +277,7 @@
>  >       struct device           *dev;                   /* parent 
> device */
>  >       unsigned char           hub6;                   /* this should 
> be in the 8250 driver */
>  >       unsigned char           unused[3];
>  > +     void                            *user;                  /* 
> generic platform 'user' pointer */
> 
>     Erm, 'private' or 'data' would've sounded better in the kernel context,
> IMHO... :-)

OK, I picked 'data'.


>  > 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
>  > diff -u -r1.1.1.2 serial_reg.h
>  > --- 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      24 Jan 2007 23:55:29 -0000
>  > @@ -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_IIR_BUSY                0x07    /* Busy Detect */
> 
>     I'd suggest keeping this with other UART_IIR_* #defines, separated 
> by the
> more elaborate comment.

Will do. I noticed that the other non-standard value UART_IIR_TOD 0x08 is with
the Xscale stuff so I followed suit.

Marc

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

[Sergei Shtylyov]

Hello.

Marc St-Jean wrote:

>> > Here is my second attempt at the serial driver patch for the
>> > PMC-Sierra MSP71xx device.

>> > There are three different fixes:
>> > 1. Fix for THRE errata
>> > - I verified the UART_BUG_TXEN fix does not help with this erratum.
>> > - I left our current fix in until I get our platform booting on
>> > 2.6.20-rc4 to try the mm tree "8250-uart-backup-timer.patch".
>> > Feel free to ignore for now.

>> > 2. Fix for Busy Detect on LCR write
>> > - Moved to new UPIO_DWAPB iotype. Because the new type is a memory
>> > mapped device and there are several tests for UPIO_MEM, this involved
>> > updating serial_core.c and 8250_early.c in addition to 8250.c.
>> > - I tried implementing this totally in serial_in as suggested, but
>> > it can't be done because of bit overlap between UART_IIR_NO_INT and
>> > UART_IIR_BUSY. Also there is no way to set the interrupt "handled = 1"
>> > from serial_in.

>> > 3. Workaround for interrupt/data concurrency issue
>> > - Moved to new UPIO_DWAPB iotype.

>> > Index: linux_2_6/drivers/serial/8250.c
>> > ===================================================================
>> > RCS file: linux_2_6/drivers/serial/8250.c,v
>> > retrieving revision 1.1.1.7
>> > diff -u -r1.1.1.7 8250.c
>> > --- 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   24 Jan 2007 23:55:27 -0000
>>[...]
>> > @@ -333,6 +334,8 @@
>> >   static void
>> >   serial_out(struct uart_8250_port *up, int offset, int value)

>>   Your patch is clearly garbled again, something added an extra space 
>>to all
>>lines stating with space... :-/

> I see that but was under the impression cvs diff did that so all lines
> lineup correctly whether they have a +, -, or neither.

    Yes. And your mailer has probably added another space to lines already 
begginign with space. Some mailers tend to do it, don't know sure why... :-/

> It doesn't affect applying the patches for me, did you try?

    I hadn't but I had no doubts it'll fail... just tried, not a single hunk 
applied. :-]

>> >   {
>> > +     /* Save the offset before it's remapped */
>> > +     int save_offset = offset;

>>    Is there real need to save this? What regshift equals for this UART?

> Yes, we have a regshift of 2 on this SoC and it could be different on other
> SoCs which reuse the UART IP.

    Agreed then (though still seems avoidable).

>> >       offset = map_8250_out_reg(up, offset) << up->port.regshift;

>> >       switch (up->port.iotype) {
>> > @@ -359,6 +362,19 @@
>> >                       writeb(value, up->port.membase + offset);
>> >               break;
>> >
>> > +     case UPIO_DWAPB:
>> > +             /* Save the LCR value so it can be re-written when a
>> > +              * Busy Detect interrupt occurs. */
>> > +             if (save_offset == UART_LCR)
>> > +                     up->lcr = value;
>> > +             writeb(value, up->port.membase + offset);
>> > +             /* Read the IER to ensure any interrupt is cleared before
>> > +              * returning from ISR. */
>> > +             if ((save_offset == UART_TX || save_offset == UART_IER) &&

>>    Not sure how an IER read ensures that...

> It's just a dummy read to ensure that any writes which clear interrupts have
> reached the device before returning from the IRQ.

    Hm, isn't it CPU write buffer flush? Shouldn't this be handled more 
generically?

[...]
>> > Index: linux_2_6/drivers/serial/8250_early.c
>> > ===================================================================
>> > RCS file: linux_2_6/drivers/serial/8250_early.c,v
>> > retrieving revision 1.1.1.3
>> > diff -u -r1.1.1.3 8250_early.c
>> > --- linux_2_6/drivers/serial/8250_early.c     19 Oct 2006 20:08:20 
>>-0000      1.1.1.3
>> > +++ linux_2_6/drivers/serial/8250_early.c     24 Jan 2007 23:55:27 -0000
>> > @@ -46,7 +46,7 @@
>> >
>> >   static unsigned int __init serial_in(struct uart_port *port, int 
>>offset)
>> >   {
>> > -     if (port->iotype == UPIO_MEM)
>> > +     if (port->iotype == UPIO_MEM || port->iotype == UPIO_DWAPB)
>> >               return readb(port->membase + offset);
>> >       else
>> >               return inb(port->iobase + offset);
>> > @@ -54,7 +54,7 @@
>> >
>> >   static void __init serial_out(struct uart_port *port, int offset, 
>>int value)
>> >   {
>> > -     if (port->iotype == UPIO_MEM)
>> > +     if (port->iotype == UPIO_MEM || port->iotype == UPIO_DWAPB)
>> >               writeb(value, port->membase + offset);
>> >       else
>> >               outb(value, port->iobase + offset);
>> > @@ -233,7 +233,7 @@
>> >               return 0;
>> >
>> >       /* Try to start the normal driver on a matching line.  */
>> > -     mmio = (port->iotype == UPIO_MEM);
>> > +     mmio = (port->iotype == UPIO_MEM || port->iotype == UPIO_DWAPB);
>> >       line = serial8250_start_console(port, device->options);
>> >       if (line < 0)
>> >               printk("No ttyS device at %s 0x%lx for console\n",

>>    From your 8250_eraly.c changes I can conclude regshift == 1 (it doesn't
>>currently support other cases). So, serial_pot() doesn't need 
>>save_offset. :-)

> Our regshift is definitely 2 on this SoC and we don't have any problems with
> console output before the serial driver opens. So assuming it's using
> 8250_early.c for initial console output I can only conclude that it works

    It comes into action only if you specify console=uart,... kernel option 
for the eraly console support.  The "plain" 8250 console driver is containded 
within 8250.c itself.

> because UART_TX is offset 0 and the port was left configured from our
> ROM monitor.

    Well, this part of the patch can't be considered complete then (how LSR 
polling is going to work?), so should either be removed or the proper regshift 
support added.

>> > 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
>> > diff -u -r1.1.1.2 serial_reg.h
>> > --- 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      24 Jan 2007 23:55:29 -0000
>> > @@ -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_IIR_BUSY                0x07    /* Busy Detect */
>>
>>    I'd suggest keeping this with other UART_IIR_* #defines, separated 
>>by the
>>more elaborate comment.

> Will do. I noticed that the other non-standard value UART_IIR_TOD 0x08 is with
> the Xscale stuff so I followed suit.

    Well, Xscale introduced several bits. And note that 16650 definitions are 
kept with the "standard" 16550 ones.  Well, be sure to elaborate the comment 
whereever you stick that #define.

> Marc

MBR, Sergei

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

[Marc St-Jean]

Sergei Shtylyov wrote:
> Hello.
> 
> Marc St-Jean wrote:
> 
>  >> >>This I would hope you can hide in the platform specific
>  >> >>serial_in/serial_out functions. If you write the UART_LCR save it in
>  >> >>serial_out(), if you read IER etc.
> 
>  >> > I couldn't find hooks for platform specific serial_in/out functions.
> 
>  >>    It's because there are none. :-)
> 
>  >> > Do you mean using the up->port.iotype's in serial_in/out from 8250.c?
> 
>  >>    Not sure what Alan meant, but this seems the only option for now.
> 
>  > That's the conclusion I came to. I've rewritten the patch to use 
> port.type
>  > instead of iotype since one of the fix is SoC and not UART specific. 
> I guess
> 
>     I failed to folkow your logic. :-)

No longer matters as I can't use port.type. See next comment.

>  > I could use both iotype and type with a test on each for the appropriate
>  > bug, what do you recommend?
> 
>     I think iotype would be enough. You can't pass type for platform 
> devices
> anyway, IIRC (the thing I don't quite like).

I just found that out the hard way, it get's overwritten during autoconfig* and
ends up back at PORT_16550A.

I'm now trying to use my own iotype = UPIO_DWAPB and I've added it to all cases
that check for UPIO_MEM. However at boot time I'm getting:
	"serial8250: ttyS0 at *unknown* (irq = 27) is a 16550A".
It looks like something outside of 8250.c must be checking for UPIO_MEM, I'm
looking into it.

> 
>  >>  >>And we might want to add a void * for board specific insanity to 
> the 8250
>  >> >>structures if we really have to so you can hang your brain damage
>  >> >>privately off that ?
> 
>  >> > Sounds good to me, it would give us a location to store the 
> address of the
>  >> > UART_STATUS_REG required by this UART variant.
> 
>  >>    I doubt we really need to *store* it somewhere. Isn't it an fixed 
> offset
>  >>from UART's base (I haven't seen the header)?
> 
>  > Unfortunately it's not a constant offset from the UART in the SoC 
> register
> 
>     Hm...
> 
>  > space. I've used Alan suggestion and added a classic, on some other 
> OSes %-|,
>  > void "user" pointer.
> 
>     Only do not do it under #ifdef.

Understood, getting rid of them is why I started this thread.

Marc

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

[Sergei Shtylyov]

Hello.

Marc St-Jean wrote:

>> > I could use both iotype and type with a test on each for the appropriate
>> > bug, what do you recommend?

>>    I think iotype would be enough. You can't pass type for platform 
>>devices anyway, IIRC (the thing I don't quite like).

> I just found that out the hard way, it get's overwritten during autoconfig* and
> ends up back at PORT_16550A.

> I'm now trying to use my own iotype = UPIO_DWAPB and I've added it to all cases
> that check for UPIO_MEM. However at boot time I'm getting:
> 	"serial8250: ttyS0 at *unknown* (irq = 27) is a 16550A".
> It looks like something outside of 8250.c must be checking for UPIO_MEM, I'm
> looking into it.

    Yeah, be sure to change serial_core.c as well (whereever you'll see 
UPIO_AU/TSI there)... Quite ugly. :-/

WBR, Sergei

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

[Marc St-Jean]

Sergei Shtylyov wrote:
> 
>  >>This I would hope you can hide in the platform specific
>  >>serial_in/serial_out functions. If you write the UART_LCR save it in
>  >>serial_out(), if you read IER etc.
> 
>  > I couldn't find hooks for platform specific serial_in/out functions.
> 
>     It's because there are none. :-)
> 
>  > Do you mean using the up->port.iotype's in serial_in/out from 8250.c?
> 
>     Not sure what Alan meant, but this seems the only option for now.

That's the conclusion I came to. I've rewritten the patch to use port.type
instead of iotype since one of the fix is SoC and not UART specific. I guess
I could use both iotype and type with a test on each for the appropriate
bug, what do you recommend?


>   >>And we might want to add a void * for board specific insanity to the 
> 8250
>  >>structures if we really have to so you can hang your brain damage
>  >>privately off that ?
> 
>  > Sounds good to me, it would give us a location to store the address 
> of the
>  > UART_STATUS_REG required by this UART variant.
> 
>     I doubt we really need to *store* it somewhere. Isn't it an fixed 
> offset
> from UART's base (I haven't seen the header)?

Unfortunately it's not a constant offset from the UART in the SoC register
space. I've used Alan suggestion and added a classic, on some other OSes %-|,
void "user" pointer.

I'll repost as soon I complete testing and try the new timer patch.

Marc

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

[Sergei Shtylyov]

Hello.

Marc St-Jean wrote:

>> >>This I would hope you can hide in the platform specific
>> >>serial_in/serial_out functions. If you write the UART_LCR save it in
>> >>serial_out(), if you read IER etc.

>> > I couldn't find hooks for platform specific serial_in/out functions.

>>    It's because there are none. :-)

>> > Do you mean using the up->port.iotype's in serial_in/out from 8250.c?

>>    Not sure what Alan meant, but this seems the only option for now.

> That's the conclusion I came to. I've rewritten the patch to use port.type
> instead of iotype since one of the fix is SoC and not UART specific. I guess

    I failed to folkow your logic. :-)

> I could use both iotype and type with a test on each for the appropriate
> bug, what do you recommend?

    I think iotype would be enough. You can't pass type for platform devices 
anyway, IIRC (the thing I don't quite like).

>>  >>And we might want to add a void * for board specific insanity to the 8250
>> >>structures if we really have to so you can hang your brain damage
>> >>privately off that ?

>> > Sounds good to me, it would give us a location to store the address of the
>> > UART_STATUS_REG required by this UART variant.

>>    I doubt we really need to *store* it somewhere. Isn't it an fixed offset
>>from UART's base (I haven't seen the header)?

> Unfortunately it's not a constant offset from the UART in the SoC register

    Hm...

> space. I've used Alan suggestion and added a classic, on some other OSes %-|,
> void "user" pointer.

    Only do not do it under #ifdef.

> Marc

WBR, Sergei

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

[Marc St-Jean]

Sergei Shtylyov wrote:
> Hello, I 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...
> 
>  >> Relative to the other two. Any recommended improvements on this one?
> 
>  >    I already told: runtime check.
> 
>     BTW, I've just came accross 2 patches in the -mm tree related to the
> transmitter bug:
> 
> http://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.20-rc4/2.6.20-rc4-mm1/broken-out/8250-make-probing-for-txen-bug-a-config-option.patch

Thanks. The first of these is not at all what we are seeing. However I guess
there could be a remote chance it's triggering our THRE errata as it did for
the pnx4008 boards.

> http://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.20-rc4/2.6.20-rc4-mm1/broken-out/8250-uart-backup-timer.patch

This second patch failure description is identical to what we are seeing without
the THRE work-around. This must be the timer patch Alan mentioned but it's not
in the linux.git at l-m.o.

Could you please explain what you mean by and where I can find the "-mm tree"?

Marc

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

[Sergei Shtylyov]

Hello.

Marc St-Jean wrote:

>>http://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.20-rc4/2.6.20-rc4-mm1/broken-out/8250-uart-backup-timer.patch

> This second patch failure description is identical to what we are seeing without
> the THRE work-around. This must be the timer patch Alan mentioned but it's not
> in the linux.git at l-m.o.

    Yeah, it must still be considered "experimental" as it resides only within 
the -mm tree.

> Could you please explain what you mean by and where I can find the "-mm tree"?

    http://kernel.org/patchtypes/mm.html

    As the serial drivers have no maintainer now, you probably have to CC the 
final patch to Andrew Morton (akpm@osdl.org), so it'd be better to be 
applicable against the recent -mm patch.

> Marc

MBR, Sergei

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

[Marc St-Jean]

Alan wrote:
>  > There are three different fixes:
>  > 1. Fix for THRE errata
> 
> That should be handled anyway. The current code actually spots this and
> uses a backup timer for dodgy UARTS

Thanks, I'll retest without this fix on the current l-m.o git master and see
if it still solves our errata.

>  > 2. Fix for Busy Detect on LCR write
>  > 3. Workaround for interrupt/data concurrency issue
> 
>  >       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;
> 
> This I would hope you can hide in the platform specific
> serial_in/serial_out functions. If you write the UART_LCR save it in
> serial_out(), if you read IER etc.

I couldn't find hooks for platform specific serial_in/out functions.
Do you mean using the up->port.iotype's in serial_in/out from 8250.c?

> 
>  > +#ifdef CONFIG_PMC_MSP
>  > +             } else if ((iir & UART_IER_BUSY) == UART_IER_BUSY) {
>  > +                     /*
>  > +                      * 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.
> 
> Ditto... spot this case and whack it in your serial methods.

A serial_in(up, UART_IIR) calls occur in more places that just the interrupt
handler (i.e. autoconfig*, serial8250_start_tx, etc). We will need to check
if we are in an interrupt on each IIR read, hopefully that won't be too much
overhead!

> 
> And we might want to add a void * for board specific insanity to the 8250
> structures if we really have to so you can hang your brain damage
> privately off that ?

Sounds good to me, it would give us a location to store the address of the
UART_STATUS_REG required by this UART variant.

Marc

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

[Alan]

On Tue, 23 Jan 2007 14:37:23 -0800
Marc St-Jean <Marc_St-Jean@pmc-sierra.com> wrote:
> > This I would hope you can hide in the platform specific
> > serial_in/serial_out functions. If you write the UART_LCR save it in
> > serial_out(), if you read IER etc.
> 
> I couldn't find hooks for platform specific serial_in/out functions.
> Do you mean using the up->port.iotype's in serial_in/out from 8250.c?

If you can have other uarts as well then yes. Basically I want all the
ugliness to belong to you not to the 8250 driver (and ditto for any other
half baked uart or demented piece of broken glue logic). If we don't do
that then 8250.c will end up unmanagable. So long as you crap in your own
pond everyone else is fine ;)

> A serial_in(up, UART_IIR) calls occur in more places that just the interrupt
> handler (i.e. autoconfig*, serial8250_start_tx, etc). We will need to check
> if we are in an interrupt on each IIR read, hopefully that won't be too much
> overhead!

Or if need be we make that hint generally available as we can do that bit
cleanly.

> > 
> > And we might want to add a void * for board specific insanity to the 8250
> > structures if we really have to so you can hang your brain damage
> > privately off that ?
> 
> Sounds good to me, it would give us a location to store the address of the
> UART_STATUS_REG required by this UART variant.

and for anything harder people can hang a struct off it.

Alan

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

[Sergei Shtylyov]

Hello.

Marc St-Jean wrote:

>> > 2. Fix for Busy Detect on LCR write
>> > 3. Workaround for interrupt/data concurrency issue

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

>>This I would hope you can hide in the platform specific
>>serial_in/serial_out functions. If you write the UART_LCR save it in
>>serial_out(), if you read IER etc.

> I couldn't find hooks for platform specific serial_in/out functions.

    It's because there are none. :-)

> Do you mean using the up->port.iotype's in serial_in/out from 8250.c?

    Not sure what Alan meant, but this seems the only option for now.

  >>And we might want to add a void * for board specific insanity to the 8250
>>structures if we really have to so you can hang your brain damage
>>privately off that ?

> Sounds good to me, it would give us a location to store the address of the
> UART_STATUS_REG required by this UART variant.

    I doubt we really need to *store* it somewhere. Isn't it an fixed offset 
from UART's base (I haven't seen the header)?

> Marc

MBR, Sergei

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

[Marc St-Jean]

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@linux-mips.org>, <linux-serial@vger.kernel.org>
References:
<5C1FD43E5F1B824E83985A74F396286E03AD7632@bby1exm08.pmc_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 <Marc_St-Jean@pmc-sierra.com>

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

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

[Marc St-Jean]

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

-------- Original Message --------
Subject: RE: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git master
Date: Mon, 22 Jan 2007 10:11:04 -0800
From: Marc St-Jean
To: Sergei Shtylyov
CC: <linux-mips@linux-mips.org>,	<linux-serial@vger.kernel.org>

> -----Original Message-----
> From: Sergei Shtylyov [mailto:sshtylyov@ru.mvista.com] 
> Sent: Friday, January 19, 2007 9:05 AM
> To: Marc St-Jean
> Cc: linux-mips@linux-mips.org; 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...

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


> > 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 <Marc_St-Jean@pmc-sierra.com>
> 
> > 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 */

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.

If that's not acceptable I can audit the driver and ensure all LCR
writes are first saved. Maybe comments would reduce the chance of future
updates breaking this?


> > @@ -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. Do you see this as an exception?

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.


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

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.

In the UART_BUG_DWTHRE case an interrupt IS generated on enabling THRI.
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).


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


> > @@ -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.
B) How to eliminate the platform #ifdef. How to pass in the address of
the UARTx_STATUS_REG in a platform independent way?
We could add it to one of the data structures like uart_port, but it
would need an #ifdef in the header file.


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

Please see my earlier comment on this issue being different than
UART_BUG_TXEN.


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

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.

Thanks for the feedback,
Marc

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

[Marc St-Jean]

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

> -----Original Message-----
> From: Ralf Baechle [mailto:ralf@linux-mips.org]
> Sent: Friday, January 19, 2007 8:18 AM
> To: Marc St-Jean
> Cc: linux-mips@linux-mips.org; linux-serial@vger.kernel.org
> Subject: Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git 
> master
> 
> On Thu, Jan 18, 2007 at 04:23:01PM -0800, Marc St-Jean wrote:
> 
> > 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
> 
> CONFIG_PMC_MSP is not defined anywhere.  msp_regs.h does not exist.
> 
>   Ralf
> 

Hi Ralf,

CONFIG_PMC_MSP is defined in the main platform patch (arch/mips/Kconfig). I doesn't apply against the git HEAD yet. I'm still working on it and will post as soon as it does.

msp_regs.h defines the UART0_STATUS_REG and UART1_STATUS_REG SoC addresses for the DesignWare UART. I thought of putting them in include/linux/serial_reg.h but it contains register offsets only, no platform addresses.

That is one of the main issues, how to add MSP71xx specific register mapping without platform tests/includes?

Marc

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

[Marc St-Jean]

> -----Original Message-----
> From: Sergei Shtylyov [mailto:sshtylyov@ru.mvista.com] 
> Sent: Friday, January 19, 2007 9:05 AM
> To: Marc St-Jean
> Cc: linux-mips@linux-mips.org; 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...

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


> > 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 <Marc_St-Jean@pmc-sierra.com>
> 
> > 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 */

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.

If that's not acceptable I can audit the driver and ensure all LCR writes are first saved. Maybe comments would reduce the chance of future updates breaking this?


> > @@ -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. Do you see this as an exception?

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.


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

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.

In the UART_BUG_DWTHRE case an interrupt IS generated on enabling THRI. 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). 


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


> > @@ -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.
B) How to eliminate the platform #ifdef. How to pass in the address of the UARTx_STATUS_REG in a platform independent way?
We could add it to one of the data structures like uart_port, but it would need an #ifdef in the header file.

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

Please see my earlier comment on this issue being different than UART_BUG_TXEN.


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

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.

Thanks for the feedback,
Marc

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

[Marc St-Jean]

> -----Original Message-----
> From: Sergei Shtylyov [mailto:sshtylyov@ru.mvista.com] 
> Sent: Friday, January 19, 2007 9:05 AM
> To: Marc St-Jean
> Cc: linux-mips@linux-mips.org; 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...

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


> > 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 <Marc_St-Jean@pmc-sierra.com>
> 
> > 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 */

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.

If that's not acceptable I can audit the driver and ensure all LCR writes are first saved. Maybe comments would reduce the chance of future updates breaking this?


> > @@ -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. Do you see this as an exception?

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.


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

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.

In the UART_BUG_DWTHRE case an interrupt IS generated on enabling THRI. 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). 


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


> > @@ -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.
B) How to eliminate the platform #ifdef. How to pass in the address of the UARTx_STATUS_REG in a platform independent way?
We could add it to one of the data structures like uart_port, but it would need an #ifdef in the header file.

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

Please see my earlier comment on this issue being different than UART_BUG_TXEN.


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

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.

Thanks for the feedback,
Marc

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

[Marc St-Jean]

> -----Original Message-----
> From: Sergei Shtylyov [mailto:sshtylyov@ru.mvista.com] 
> Sent: Friday, January 19, 2007 9:05 AM
> To: Marc St-Jean
> Cc: linux-mips@linux-mips.org; 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...

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


> > 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 <Marc_St-Jean@pmc-sierra.com>
> 
> > 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 */

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.

If that's not acceptable I can audit the driver and ensure all LCR writes are first saved. Maybe comments would reduce the chance of future updates breaking this?


> > @@ -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. Do you see this as an exception?

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.


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

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.

In the UART_BUG_DWTHRE case an interrupt IS generated on enabling THRI. 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). 


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


> > @@ -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.
B) How to eliminate the platform #ifdef. How to pass in the address of the UARTx_STATUS_REG in a platform independent way?
We could add it to one of the data structures like uart_port, but it would need an #ifdef in the header file.

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

Please see my earlier comment on this issue being different than UART_BUG_TXEN.


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

As mentioned earlier I have now renamed this UART_IIR_BUSY.

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

[Sergei Shtylyov]

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@pmc-sierra.com>

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

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

[Sergei Shtylyov]

Hello, I 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...

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

>    I already told: runtime check.

    BTW, I've just came accross 2 patches in the -mm tree related to the 
transmitter bug:

http://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.20-rc4/2.6.20-rc4-mm1/broken-out/8250-make-probing-for-txen-bug-a-config-option.patch
http://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.20-rc4/2.6.20-rc4-mm1/broken-out/8250-uart-backup-timer.patch

>>>> Thanks,
>>>> Marc

WBR, Sergei

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

[Marc St-Jean]

> -----Original Message-----
> From: Ralf Baechle [mailto:ralf@linux-mips.org]
> Sent: Friday, January 19, 2007 8:18 AM
> To: Marc St-Jean
> Cc: linux-mips@linux-mips.org; linux-serial@vger.kernel.org
> Subject: Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git 
> master
> 
> On Thu, Jan 18, 2007 at 04:23:01PM -0800, Marc St-Jean wrote:
> 
> > 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
> 
> CONFIG_PMC_MSP is not defined anywhere.  msp_regs.h does not exist.
> 
>   Ralf
> 

Hi Ralf,

CONFIG_PMC_MSP is defined in the main platform patch (arch/mips/Kconfig). I doesn't apply against the git HEAD yet. I'm still working on it and will post as soon as it does.

msp_regs.h defines the UART0_STATUS_REG and UART1_STATUS_REG SoC addresses for the DesignWare UART. I thought of putting them in include/linux/serial_reg.h but it contains register offsets only, no platform addresses.

That is one of the main issues, how to add MSP71xx specific register mapping without platform tests/includes?

Marc

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

[Marc St-Jean]

 

> -----Original Message-----
> From: Ralf Baechle [mailto:ralf@linux-mips.org] 
> Sent: Friday, January 19, 2007 8:18 AM
> To: Marc St-Jean
> Cc: linux-mips@linux-mips.org; linux-serial@vger.kernel.org
> Subject: Re: [PATCH] serial driver PMC MSP71xx, kernel 
> linux-mips.git master
> 
> On Thu, Jan 18, 2007 at 04:23:01PM -0800, Marc St-Jean wrote:
> 
> > 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
> 
> CONFIG_PMC_MSP is not defined anywhere.  msp_regs.h does not exist.
> 
>   Ralf
> 

Hi Ralf,

CONFIG_PMC_MSP is defined in the main platform patch (arch/mips/Kconfig). I doesn't apply against the git HEAD yet. I'm still working on it and will post as soon as it does.

msp_regs.h defines the UART0_STATUS_REG and UART1_STATUS_REG SoC addresses for the DesignWare UART. I thought of putting them in include/linux/serial_reg.h but it contains register offsets only, no platform addresses.

That is one of the main issues, how to add MSP71xx specific register mapping without platform tests/includes?

Marc