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

[Marc St-Jean <Marc_St-Jean@pmc-sierra.com>]

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