[U-Boot] [PATCH 1/2] Serial support extended up to 6 COMs

[Michael Zaidman]

Dear Wolfgand,

On Mon, Mar 22, 2010 at 10:04 PM, Wolfgang Denk <wd at denx.de> wrote:
> Dear Michael Zaidman,
>
> In message <1269267840-15285-1-git-send-email-michael.zaidman at gmail.com> you wrote:
>> Added support for extra ns16550 chip extending total number of
>> supported COMs up to 6. This targets the cases when due to the
>> insufficient number of UART ports on the CPU chip designers are
>> forced to put additional ns16550 chip on board.
>
> What about systems which use dual-, quad- or even octal-UART chips
> instead?

Most of modern embedded CPU chips incorporate 1-2 UART ports. In some
cases it is not enough and extra UART chip are added. It looks like
total six ports could be reasonable number. The port count of
particular chip does not matter as far as the chip is compatible to
the industry standard 16C550.

All these chips are treated in the same way by this patch. Only
frequency of crystal oscillator or external clock can differs from
UART ports belonging to CPU. I tried to explain it in the description
of the change placed at the head of the file. For this purpose the
CONFIG_SYS_NS16550_HI_PORTS_BGN was added which allows us to
distinguish between “low” ports belonging to CPU chip and “high” ports
of additional chip while calculating clock divisor.

>> + * Also in order to add COM5 and COM6 the following configuration
>> + * entries should be defined:
>> + * CONFIG_SYS_NS16550_COM5,
>> + * CONFIG_SYS_NS16550_COM6
>
> How would that work with, say a quad-UART?

On my board there are 2 UARTS on mpc8349 chip and quad-UART XR16854. I
added following definitions to the board's config file:
#define CONFIG_SYS_NS16550_HI_PORTS_BGN 3
#define CONFIG_SYS_NS16550_HI_PORTS_CLK 1843200 /* crystal
oscillator/external clock */
#define CONFIG_SYS_NS16550_COM3		(CONFIG_SYS_QUART_BASE)
#define CONFIG_SYS_NS16550_COM4		(CONFIG_SYS_QUART_BASE+8)
#define CONFIG_SYS_NS16550_COM5		(CONFIG_SYS_QUART_BASE+16)
#define CONFIG_SYS_NS16550_COM6		(CONFIG_SYS_QUART_BASE+24)


>
>> @@ -45,7 +68,7 @@ DECLARE_GLOBAL_DATA_PTR;
>>  #else
>>  #error       "No console index specified."
>>  #endif /* CONFIG_SERIAL_MULTI */
>> -#elif (CONFIG_CONS_INDEX < 1) || (CONFIG_CONS_INDEX > 4)
>> +#elif (CONFIG_CONS_INDEX < 1) || (CONFIG_CONS_INDEX > 6)
>
> I don;t like this. We switch one arbitrary limitation (4) for another
> one (6). When we change this anyway, can we then please make this
> configurable?

OK, I will do this.

>> @@ -57,34 +80,62 @@ DECLARE_GLOBAL_DATA_PTR;
>>  #error       "Console port 3 defined but not configured."
>>  #elif CONFIG_CONS_INDEX == 4 && !defined(CONFIG_SYS_NS16550_COM4)
>>  #error       "Console port 4 defined but not configured."
>> +#elif CONFIG_CONS_INDEX == 5 && !defined(CONFIG_SYS_NS16550_COM5)
>> +#error       "Console port 5 defined but not configured."
>> +#elif CONFIG_CONS_INDEX == 6 && !defined(CONFIG_SYS_NS16550_COM6)
>> +#error       "Console port 6 defined but not configured."
>>  #endif
>
> What a maintenance nightmare. Should we not at least define and use a
> macro here to simplify the checking?
>

OK, I will try...

>>  /* Note: The port number specified in the functions is 1 based.
>>   *    the array is 0 based.
>>   */
>> -static NS16550_t serial_ports[4] = {
>> +static NS16550_t serial_ports[] = {
>>  #ifdef CONFIG_SYS_NS16550_COM1
>> -     (NS16550_t)CONFIG_SYS_NS16550_COM1,
>> +     (NS16550_t)CONFIG_SYS_NS16550_COM1
>>  #else
>> -     NULL,
>> +     NULL
>>  #endif
>
> Please leave the trailing comma in place.
>
>>  #ifdef CONFIG_SYS_NS16550_COM2
>> -     (NS16550_t)CONFIG_SYS_NS16550_COM2,
>> -#else
>> -     NULL,
>> +     ,(NS16550_t)CONFIG_SYS_NS16550_COM2
>
> Please always put the comma at the END of the line.
>
>>  #ifdef CONFIG_SYS_NS16550_COM4
>> -     (NS16550_t)CONFIG_SYS_NS16550_COM4
>> -#else
>> -     NULL
>> +     ,(NS16550_t)CONFIG_SYS_NS16550_COM4
>> +#elif \
>> +     defined(CONFIG_SYS_NS16550_COM5) || \
>> +     defined(CONFIG_SYS_NS16550_COM6)
>> +     ,NULL
>> +#endif
>
> Hm... I cannot help it, but this all is extremely ugly. This needs to
> be reworked.
>
The intention here was to minimize memory consumption (20 bytes @
single UART port on board) Most of the boards have 1-2 UART ports
only. So I looked for a way to remove unused NULL pointers from the
serial_ports array which now have 6 entries. Any better idea how to
achieve it? The code looks ugly to me too and if you insist I will
turn it to the previous form at the expense of memory…

>
>> -static int calc_divisor (NS16550_t port)
>> +static int calc_divisor (int index)
>>  {
>> +     ulong clock = CONFIG_SYS_NS16550_CLK;
>> +
>>  #ifdef CONFIG_OMAP1510
>> +     NS16550_t port = serial_ports[index];
>
> Why did you change that?

Because I need the index of the port in this routine in order to
distinguish between “low” and “high” UART ports and I did not wont to
add second parameter to the routine’s interface.

>
>>  #if !defined(CONFIG_SERIAL_MULTI)
>>  int serial_init (void)
>>  {
>> -     int clock_divisor;
>> +     int i;
>>
>>  #ifdef CONFIG_NS87308
>>       initialise_ns87308();
>>  #endif
>>
>> -#ifdef CONFIG_SYS_NS16550_COM1
>> -     clock_divisor = calc_divisor(serial_ports[0]);
>> -     NS16550_init(serial_ports[0], clock_divisor);
>> -#endif
>> -#ifdef CONFIG_SYS_NS16550_COM2
>> -     clock_divisor = calc_divisor(serial_ports[1]);
>> -     NS16550_init(serial_ports[1], clock_divisor);
>> -#endif
>> -#ifdef CONFIG_SYS_NS16550_COM3
>> -     clock_divisor = calc_divisor(serial_ports[2]);
>> -     NS16550_init(serial_ports[2], clock_divisor);
>> -#endif
>> -#ifdef CONFIG_SYS_NS16550_COM4
>> -     clock_divisor = calc_divisor(serial_ports[3]);
>> -     NS16550_init(serial_ports[3], clock_divisor);
>> -#endif
>> +     for (i = 0; i < MAX_SER_DEV; i++)
>> +             if (serial_ports[i] != NULL)
>> +                     NS16550_init(serial_ports[i], calc_divisor(i));
>
> What happens if not all available serial pors are available for use in
> U-Boot, i. e. when the array serial_ports[] is only sparsely
> populated, or certainports must not be accessed by U-Boot?
>
The “ugly” code we discussed above places NULL pointers in the “holes”
up to the highest defined COM. The MAX_SER_DEV gets index of the
highest defined COM as well. So, this loop will access all defined
COMs. Or I misunderstood your question?

>> @@ -328,4 +374,11 @@ struct serial_device eserial3_device =
>>  DECLARE_ESERIAL_FUNCTIONS(4);
>>  struct serial_device eserial4_device =
>>       INIT_ESERIAL_STRUCTURE(4,"eserial3","EUART4");
>> +DECLARE_ESERIAL_FUNCTIONS(5);
>> +struct serial_device eserial5_device =
>> +     INIT_ESERIAL_STRUCTURE(5,"eserial4","EUART5");
>> +DECLARE_ESERIAL_FUNCTIONS(6);
>> +struct serial_device eserial6_device =
>> +     INIT_ESERIAL_STRUCTURE(6,"eserial5","EUART6");
>>  #endif /* CONFIG_SERIAL_MULTI */
>
> This needs rework, too.

This was here before, I just added two ports. Could you please be more specific?

Thanks,
Michael