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

[Sergei Shtylyov <sshtylyov@ru.mvista.com>]

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.

> Are you aware if this is the case for the "putchar" used by early_printk
> as well?

    No idea, ask Ralf. ;-)

> Thanks,
> Marc

WBR, Sergei