Patchwork

login
register
about
Submitter Stefan Reinauer
Date 2011-04-21 23:06:29
Message ID <20110421230628.GA25213@coreboot.org>
Download mbox | patch
Permalink /patch/2902/
State Accepted
Headers show

Comments

Stefan Reinauer - 2011-04-21 23:06:29
See patch
Peter Stuge - 2011-04-21 23:43:49
Stefan Reinauer wrote:
> The UART divider should be calculated based on the base frequency
> and baudrate, not hardcoded in addition to that.
> 
> Signed-off-by: Stefan Reinauer <stefan.reinauer@coreboot.org>

With some changes pointed out below it's:

Acked-by: Peter Stuge <peter@stuge.se>


> +++ src/mainboard/iwave/iWRainbowG6/Kconfig	(working copy)
> @@ -37,9 +37,7 @@
>  	int
>  	default 10
>  
> -# WTF, is this 19200?
> -config TTYS0_DIV
> -	int
> -	default 6
> +# This mainboard might have a higher clocked UART or might not be able to run
> +# serial output at 115200 baud

I don't think this board has a special UART, I think the setting was
just chosen by the original developer as a way to make coreboot
configure the desired baudrate. So just drop the setting and let the
regular baudrate mechanism in Kconfig do the job.


> +++ src/console/uart8250_console.c	(working copy)
> @@ -25,7 +25,13 @@
>  {
>  	static const unsigned char div[8] = { 1, 2, 3, 6, 12, 24, 48, 96 };
>  	int b_index = 0;
> -	unsigned int divisor = CONFIG_TTYS0_DIV;
> +	/* TODO the divisor calculation is hard coded to standard UARTs. Some
> +	 * UARTs won't work with these values. This should be a property of the
> +	 * UART used, worst case a Kconfig variable. For now live with hard
> +	 * codes as the only devices that are different are the iWave
> +	 * iRainbowG6 and the OXPCIe952 card (and the latter is memory mapped)

Also change this wording since it's not really certain that
iRainbowG6 actually is different. (Btw it would be nice to make the
mainboard directory name all lowercase, like with every other board.)


> +++ src/lib/uart8250.c	(working copy)
> @@ -88,6 +88,12 @@
>  #if defined(__PRE_RAM__) || defined(__SMM__)
>  void uart_init(void)
>  {
> +	/* TODO the divisor calculation is hard coded to standard UARTs. Some
> +	 * UARTs won't work with these values. This should be a property of the
> +	 * UART used, worst case a Kconfig variable. For now live with hard
> +	 * codes as the only devices that are different are the iWave
> +	 * iRainbowG6 and the OXPCIe952 card (and the latter is memory mapped)

Same with this comment.


//Peter
Stefan Reinauer - 2011-04-22 02:15:45
On 4/21/11 4:43 PM, Peter Stuge wrote:
> Stefan Reinauer wrote:
>> The UART divider should be calculated based on the base frequency
>> and baudrate, not hardcoded in addition to that.
>>
>> Signed-off-by: Stefan Reinauer<stefan.reinauer@coreboot.org>
> With some changes pointed out below it's:
>
> Acked-by: Peter Stuge<peter@stuge.se>
>
Thanks. Comments improved and committed as r6538

Patch

The UART divider should be calculated based on the base frequency
and baudrate, not hardcoded in addition to that.

Signed-off-by: Stefan Reinauer <stefan.reinauer@coreboot.org>

Index: src/include/uart8250.h
===================================================================
--- src/include/uart8250.h	(revision 6537)
+++ src/include/uart8250.h	(working copy)
@@ -28,13 +28,9 @@ 
 #ifndef CONFIG_TTYS0_BAUD
 #define CONFIG_TTYS0_BAUD 115200
 #endif
-
-#ifndef CONFIG_TTYS0_DIV
 #if ((115200%CONFIG_TTYS0_BAUD) != 0)
 #error Bad ttys0 baud rate
 #endif
-#define CONFIG_TTYS0_DIV	(115200/CONFIG_TTYS0_BAUD)
-#endif
 
 /* Line Control Settings */
 #ifndef CONFIG_TTYS0_LCS
Index: src/mainboard/iwave/iWRainbowG6/Kconfig
===================================================================
--- src/mainboard/iwave/iWRainbowG6/Kconfig	(revision 6537)
+++ src/mainboard/iwave/iWRainbowG6/Kconfig	(working copy)
@@ -37,9 +37,7 @@ 
 	int
 	default 10
 
-# WTF, is this 19200?
-config TTYS0_DIV
-	int
-	default 6
+# This mainboard might have a higher clocked UART or might not be able to run
+# serial output at 115200 baud
 
 endif
Index: src/console/uart8250_console.c
===================================================================
--- src/console/uart8250_console.c	(revision 6537)
+++ src/console/uart8250_console.c	(working copy)
@@ -25,7 +25,13 @@ 
 {
 	static const unsigned char div[8] = { 1, 2, 3, 6, 12, 24, 48, 96 };
 	int b_index = 0;
-	unsigned int divisor = CONFIG_TTYS0_DIV;
+	/* TODO the divisor calculation is hard coded to standard UARTs. Some
+	 * UARTs won't work with these values. This should be a property of the
+	 * UART used, worst case a Kconfig variable. For now live with hard
+	 * codes as the only devices that are different are the iWave
+	 * iRainbowG6 and the OXPCIe952 card (and the latter is memory mapped)
+	 */
+	unsigned int divisor = 115200 / CONFIG_TTYS0_BAUD;
 
 	if (get_option(&b_index, "baud_rate") == 0) {
 		divisor = div[b_index];
Index: src/lib/uart8250.c
===================================================================
--- src/lib/uart8250.c	(revision 6537)
+++ src/lib/uart8250.c	(working copy)
@@ -88,6 +88,12 @@ 
 #if defined(__PRE_RAM__) || defined(__SMM__)
 void uart_init(void)
 {
+	/* TODO the divisor calculation is hard coded to standard UARTs. Some
+	 * UARTs won't work with these values. This should be a property of the
+	 * UART used, worst case a Kconfig variable. For now live with hard
+	 * codes as the only devices that are different are the iWave
+	 * iRainbowG6 and the OXPCIe952 card (and the latter is memory mapped)
+	 */
 #if CONFIG_USE_OPTION_TABLE && !defined(__SMM__)
         static const unsigned char divisor[] = { 1, 2, 3, 6, 12, 24, 48, 96 };
         unsigned ttys0_div, ttys0_index;
@@ -97,7 +103,7 @@ 
 
 	uart8250_init(CONFIG_TTYS0_BASE, ttys0_div);
 #else
-	uart8250_init(CONFIG_TTYS0_BASE, CONFIG_TTYS0_DIV);
+	uart8250_init(CONFIG_TTYS0_BASE, (115200 / CONFIG_TTYS0_BAUD));
 #endif
 }
 #endif