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

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

[Michael Zaidman]

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.

Signed-off-by: Michael Zaidman <michael.zaidman@gmail.com>
---
 drivers/serial/serial.c |  125 +++++++++++++++++++++++++++++++++-------------
 include/serial.h        |    2 +
 2 files changed, 91 insertions(+), 36 deletions(-)

diff --git a/drivers/serial/serial.c b/drivers/serial/serial.c
index dd5f332..0b30ff4 100644
--- a/drivers/serial/serial.c
+++ b/drivers/serial/serial.c
@@ -2,6 +2,29 @@
  * (C) Copyright 2000
  * Rob Taylor, Flying Pig Systems. robt at flyingpig.com.
  *
+ * (C) Copyright 2010
+ * Eastman Kodak Company, <www.kodak.com>
+ * Michael Zaidman, <michael.zaidman@kodak.com>
+ *
+ * Added support for extra ns16550 chip extending total number of
+ * supported COMs 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. In order to
+ * configure its clock two new CONFIGs have been added:
+ * CONFIG_SYS_NS16550_HI_PORTS_BGN - specifies index of the first
+ *		High COM port, i.e. first port of the extra chip.
+ *		For example: if CPU chip has 2 COMs then its value should
+ *		be configured to 3.
+ *		If no CONFIG_SYS_NS16550_HI_PORTS_BGN was defined all COMs
+ *		will be configured with CONFIG_SYS_NS16550_CLK
+ *
+ * CONFIG_SYS_NS16550_HI_PORTS_CLK - High COMs clock.
+ *
+ * Also in order to add COM5 and COM6 the following configuration
+ * entries should be defined:
+ * CONFIG_SYS_NS16550_COM5,
+ * CONFIG_SYS_NS16550_COM6
+ *
  * See file CREDITS for list of people who contributed to this
  * project.
  *
@@ -22,8 +45,8 @@
  */
 
 #include <common.h>
-
 #include <ns16550.h>
+
 #ifdef CONFIG_NS87308
 #include <ns87308.h>
 #endif
@@ -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)
 #error	"Invalid console index value."
 #endif
 
@@ -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
 
 /* 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
+
 #ifdef CONFIG_SYS_NS16550_COM2
-	(NS16550_t)CONFIG_SYS_NS16550_COM2,
-#else
-	NULL,
+	,(NS16550_t)CONFIG_SYS_NS16550_COM2
+#elif \
+	defined(CONFIG_SYS_NS16550_COM3) || \
+	defined(CONFIG_SYS_NS16550_COM4) || \
+	defined(CONFIG_SYS_NS16550_COM5) || \
+	defined(CONFIG_SYS_NS16550_COM6)
+	,NULL
 #endif
+
 #ifdef CONFIG_SYS_NS16550_COM3
-	(NS16550_t)CONFIG_SYS_NS16550_COM3,
-#else
-	NULL,
+	,(NS16550_t)CONFIG_SYS_NS16550_COM3
+#elif \
+	defined(CONFIG_SYS_NS16550_COM4) || \
+	defined(CONFIG_SYS_NS16550_COM5) || \
+	defined(CONFIG_SYS_NS16550_COM6)
+	,NULL
 #endif
+
 #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
+
+#ifdef CONFIG_SYS_NS16550_COM5
+	,(NS16550_t)CONFIG_SYS_NS16550_COM5
+#elif defined(CONFIG_SYS_NS16550_COM6)
+	,NULL
+#endif
+
+#ifdef CONFIG_SYS_NS16550_COM6
+	,(NS16550_t)CONFIG_SYS_NS16550_COM6
 #endif
 };
 
+#define MAX_SER_DEV ((sizeof(serial_ports)/sizeof(NS16550_t)))
+
 #define PORT	serial_ports[port-1]
 #if defined(CONFIG_CONS_INDEX)
 #define CONSOLE	(serial_ports[CONFIG_CONS_INDEX-1])
@@ -96,7 +147,7 @@ static NS16550_t serial_ports[4] = {
 #define DECLARE_ESERIAL_FUNCTIONS(port) \
     int  eserial##port##_init (void) {\
 	int clock_divisor; \
-	clock_divisor = calc_divisor(serial_ports[port-1]); \
+	clock_divisor = calc_divisor(port-1); \
 	NS16550_init(serial_ports[port-1], clock_divisor); \
 	return(0);}\
     void eserial##port##_setbrg (void) {\
@@ -123,9 +174,12 @@ static NS16550_t serial_ports[4] = {
 
 #endif /* CONFIG_SERIAL_MULTI */
 
-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];
 	/* If can't cleanly clock 115200 set div to 1 */
 	if ((CONFIG_SYS_NS16550_CLK == 12000000) && (gd->baudrate == 115200)) {
 		port->osc_12m_sel = OSC_12M_SEL;	/* enable 6.5 * divisor */
@@ -151,35 +205,27 @@ static int calc_divisor (NS16550_t port)
 	 * but we need to round that value by adding 0.5.
 	 * Rounding is especially important at high baud rates.
 	 */
-	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);
+
 }
 
 #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));
 
 	return (0);
 }
@@ -226,7 +272,7 @@ _serial_setbrg (const int port)
 {
 	int clock_divisor;
 
-	clock_divisor = calc_divisor(PORT);
+	clock_divisor = calc_divisor(port-1);
 	NS16550_reinit(PORT, clock_divisor);
 }
 
@@ -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 */
+
diff --git a/include/serial.h b/include/serial.h
index f2638ec..be2f8bc 100644
--- a/include/serial.h
+++ b/include/serial.h
@@ -33,6 +33,8 @@ extern struct serial_device eserial1_device;
 extern struct serial_device eserial2_device;
 extern struct serial_device eserial3_device;
 extern struct serial_device eserial4_device;
+extern struct serial_device eserial5_device;
+extern struct serial_device eserial6_device;
 #endif /* CONFIG_SYS_NS16550_SERIAL */
 
 #endif
-- 
1.6.3.3

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

[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@denx.de> wrote:
> Dear Michael Zaidman,
>
> In message <1269267840-15285-1-git-send-email-michael.zaidman@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

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

[Detlev Zundel]

Hi Michael,

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

Additionally to Wolfgangs comments, let me make a few observations here.
In a different project I also thought on how to implement functionality
compared to what you try to achieve and in the end I got the impression
that this is a very good chance to introduce a real device model into
U-Boot.  We have been thinking about this for a long time[1] but nobody
came around hacking on it yet.  If you check the BOF page, you can see
that this will also open up a way to do a dynamic driver configuration,
e.g. by device tree.  This is one direction we really need to cover
somehow - sooner or later.

What do you think - do you want to go the extra distance?

For a transitional time, I think we can also live with two serial driver
implementatinos - after all, we have 5 "mpsc.c" files in the tree
currently, so every chance for a cleanup is welcome :)

Cheers
  Detlev

[1] http://www.denx.de/wiki/view/U-Boot/OLSUbootBOF

-- 
["From 2.4 to 2.6 to 2.7" discussing release numbering of the Linux kernel]
Let the bike-shed-painting begin.
                                     -- Linus Torvalds 
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

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

[Wolfgang Denk]

Dear Michael Zaidman,

In message <660c0f821003230108t579e90femaac28b937e04329c@mail.gmail.com> you wrote:
> Dear Wolfgand,

s/d/g/

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

I don't like this distinction. I think we should rather use a flag, or
pointer to handler functions, etc. This way we can get rid of the
index checking here and there in the code.

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

We should try to come up with siomething that really scales well. Both
4 or 6 ports are pretty arbitrary limitations (OTOH - who needs to
support that many ports in U-Boot?).

I think it should be possible to use a "packed" array without unused
entries. Also, the order in this array should be flexible and not
dictated by concepts like "low" or "high" ports.

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

Understood. I think we should drop this index based distinction and
use some other flag.

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

No. I did not understand the code.

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

The code was there before, and when you added more of it it became
clear that this implementation does not scale, so we should look for
another, better approach.

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
A door is what a dog is perpetually on the wrong side of.
                                                        - Ogden Nash

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

[Michael Zaidman]

Dear Wolfgang,

On Tue, Mar 23, 2010 at 5:04 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Michael Zaidman,
>
> In message <660c0f821003230108t579e90femaac28b937e04329c@mail.gmail.com> you wrote:
>
>> 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.
>
> I don't like this distinction. I think we should rather use a flag, or
> pointer to handler functions, etc. This way we can get rid of the
> index checking here and there in the code.
>
At the first glance I also did not like it... BUT  It fits perfectly
here for following reasons:
1) We need this distinguishing for clock divisor calculation only,
i.e. single check in single place;
2) The clock is property of the UART chip and not the UART port. All
ports of the same chip get the same clock;
3) Any pointers or flags addition will increase memory footprint,
while the intention is to reduce;
4) The last but not least consideration - do not make changes
affecting a wide range of boards configuration files.

I already did most of corrections you pointed to in your initial code
review. Should I submit it?

Thanks,
Michael