Re: [PATCH] NEC EMMA2RH support, revisited

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

Hello.

Sergei Shtylyov wrote:

>> The patch below is to support NEC EMMA2RH Mark-eins board (R5500-based)
>> Thanks for helping and valuable comments:
>>      the whole linux-mips comminuty
>>     Ralf Baechle
>>     Martin Michlmayr
>>     Thiemo Seufer

>    It seems that the community have overlooked at least one issue with 
> this code:

    Not a big issue as it seems since UARTs are registered as the 8250 
platform devices elsewhere.

>> Index: linux/arch/mips/emma2rh/markeins/setup.c
>> ===================================================================
>> --- /dev/null
>> +++ linux/arch/mips/emma2rh/markeins/setup.c
> 
> [...]
> 
>> +static void inline __init markeins_sio_setup(void)
>> +{
>> +#ifdef CONFIG_KGDB_8250

>    I wonder what it's doing in the Linux/MIPS patch while it's only 
> relevant to the cross-arch KGDB patchset.

    Well, certainly this code does not belong here...

>> +    struct uart_port emma_port;
>> +
>> +    memset(&emma_port, 0, sizeof(emma_port));
>> +
>> +    emma_port.flags =
>> +        UPF_BOOT_AUTOCONF | UPF_SKIP_TEST;
>> +    emma_port.iotype = UPIO_MEM;
>> +    emma_port.regshift = 4;    /* I/O addresses are every 8 bytes */
>> +    emma_port.uartclk = 18544000;    /* Clock rate of the chip */
>> +
>> +    emma_port.line = 0;
>> +    emma_port.mapbase = KSEG1ADDR(EMMA2RH_PFUR0_BASE + 3);
>> +    emma_port.membase = (u8*)emma_port.mapbase;
>> +    early_serial_setup(&emma_port);
>> +
>> +    emma_port.line = 1;
>> +    emma_port.mapbase = KSEG1ADDR(EMMA2RH_PFUR1_BASE + 3);
>> +    emma_port.membase = (u8*)emma_port.mapbase;
>> +    early_serial_setup(&emma_port);
>> +
>> +    emma_port.irq = EMMA2RH_IRQ_PFUR1;
>> +    kgdb8250_add_port(1, &emma_port);
>> +#endif

>    Why you #ifdef out early_serial_setup() calls is even more 
> interesting. How this kernel is supposed to work at all with such code?!

    Why there was need to register ports early with 8250 driver if KGDB driver 
was enabled -- they don't have that much to do with each other. This  code 
looks dubious even in the context of the KGDB-2 patchset.

WBR, Sergei