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

[Wolfgang Denk]

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?

> + * 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?

> @@ -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?

> @@ -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?

>  /* 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.


> -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?

> -	return (CONFIG_SYS_NS16550_CLK + (gd->baudrate * (MODE_X_DIV / 2))) /
> -		(MODE_X_DIV * gd->baudrate);
> +#ifdef CONFIG_SYS_NS16550_HI_PORTS_BGN
> +	if ((index + 1) >= CONFIG_SYS_NS16550_HI_PORTS_BGN)
> +		clock = CONFIG_SYS_NS16550_HI_PORTS_CLK;
> +#endif
> +	return (clock + (gd->baudrate * (MODE_X_DIV / 2))) /
> +			(MODE_X_DIV * gd->baudrate);
> +
>  }

Argh...

>  #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?

> @@ -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.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
It must be remembered that there is nothing more difficult  to  plan,
more  doubtful  of  success,  nor  more dangerous to manage, than the
creation of a new system. For the initiator has the enmity of all who
would profit by the preservation of the old institutions  and  merely
lukewarm defenders in those who would gain by the new ones.
- Machiavelli