Re: [PATCH 6/12] drivers: PMC MSP71xx serial driver

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

Sergei Shtylyov wrote:
> Hello.
> 
> Marc St-Jean wrote:
> 
>  >> > diff --git a/arch/mips/pmc-sierra/msp71xx/msp_serial.c
>  >>b/arch/mips/pmc-sierra/msp71xx/msp_serial.c
>  >> > new file mode 100644
>  >> > index 0000000..3b956e9
>  >> > --- /dev/null
>  >> > +++ b/arch/mips/pmc-sierra/msp71xx/msp_serial.c
>  >> > @@ -0,0 +1,185 @@
>  >>[...]
>  >> > +#ifdef CONFIG_KGDB
>  >> > +/*
>  >> > + * kgdb uses serial port 1 so the console can remain on port 0.
>  >> > + * To use port 0 change the definition to read as follows:
>  >> > + * #define DEBUG_PORT_BASE KSEG1ADDR(MSP_UART0_BASE)
>  >> > + */
>  >> > +#define DEBUG_PORT_BASE KSEG1ADDR(MSP_UART1_BASE)
>  >> > +
>  >> > +int putDebugChar(char c)
>  >> > +{
>  >> > +     volatile uint32_t *uart = (volatile uint32_t *)DEBUG_PORT_BASE;
>  >> > +     uint32_t val = (uint32_t)c;
>  >> > +
>  >> > +     local_irq_disable();
>  >> > +     while (!(uart[5] & 0x20)); /* Wait for TXRDY */
>  >> > +     uart[0] = val;
>  >> > +     while (!(uart[5] & 0x20)); /* Wait for TXRDY */
>  >> > +     local_irq_enable();
> 
>  >>    Gah, why you decided to put local_irq_enable() there?!  KGDB expects
>  >>interrupts to be *disabled* while it has control, else some subtle state
>  >>corruptions will ensue, and it will eventually lock up. Please remove
>  >>these 2 calls completely.
> 
>  > Hmmm, this has been working for several months. I'll remove, retest and
>  > resubmit.
> 
>     I should probably have said "may".  From my experience with KGDBoE 
> though
> (well, it's somewhat different KGDB implementation :-) it locks up pretty
> quickly because of local_irq_enable() or spin_unlock_irq().  Nevertheless,
> enabling interrupts while KGDB has control is undesirable.  And look at the
> other KGDB serial code inarch/mips/ -- nobody else does this.

A search showed that some other architectures do use local_irq_disable()/enable()
and our code may have been derived from it. None of the MIPS platforms do and
the kgdb implementation seams to be arch specific so I'll drop them.

>  > Are you aware if this is the case for the "putchar" used by early_printk
>  > as well?
> 
>     No idea, ask Ralf. ;-)

I searched putchar implementations and there only seems to be one more for the
cobalt. It also doesn't  twiddle the irg state so I'll drop it there as well.

I tested and seems well without the local_irq_disable()/enable(), I'll resubmit
the path.

Thanks,
Marc