[U-Boot] use "boot select" jumper on NGW100 to select USART

[U-Boot] use "boot select" jumper on NGW100 to select USART

[Thomas Sprinkmeier]

From: Thomas Sprinkmeier <thomas.sprinkmeier@gmail.com>
Date: Mon, 9 Nov 2009 09:28:51 +1030
Subject: [PATCH] use "boot select" jumper on NGW100 to select USART

Without the "boot select" jumper U-Boot will use the USART selected
using the CONFIG_USART1, CONFIG_USART2, directive.

If  CONFIG_ALT_USART1 (or ..2, ..3, ..0) is defined then,
with the jumper in place, that USART is used instead.

Signed-off-by: Thomas Sprinkmeier <thomas.sprinkmeier@gmail.com
---
 board/atmel/atngw100/atngw100.c |    6 ++-
 drivers/serial/atmel_usart.c    |   15 +-------
 include/atmel_alt_usart.h       |   81 +++++++++++++++++++++++++++++++++++++++
 include/configs/atngw100.h      |    1 +
 4 files changed, 88 insertions(+), 15 deletions(-)
 create mode 100644 include/atmel_alt_usart.h

diff --git a/board/atmel/atngw100/atngw100.c b/board/atmel/atngw100/atngw100.c
index 004d8da..841d9ec 100644
--- a/board/atmel/atngw100/atngw100.c
+++ b/board/atmel/atngw100/atngw100.c
@@ -29,6 +29,8 @@
 #include <asm/arch/portmux.h>
 #include <netdev.h>
 
+#include <atmel_alt_usart.h>
+
 DECLARE_GLOBAL_DATA_PTR;
 
 static const struct sdram_config sdram_config = {
@@ -53,7 +55,9 @@ int board_early_init_f(void)
 	hmatrix_slave_write(EBI, SFR, HMATRIX_BIT(EBI_SDRAM_ENABLE));
 
 	portmux_enable_ebi(16, 23, 0, PORTMUX_DRIVE_HIGH);
-	portmux_enable_usart1(PORTMUX_DRIVE_MIN);
+
+	USART_JUMPER_CONFIG;
+	USART_ENABLE;
 
 #if defined(CONFIG_MACB)
 	portmux_enable_macb0(PORTMUX_MACB_MII, PORTMUX_DRIVE_HIGH);
diff --git a/drivers/serial/atmel_usart.c b/drivers/serial/atmel_usart.c
index f50552a..c78755c 100644
--- a/drivers/serial/atmel_usart.c
+++ b/drivers/serial/atmel_usart.c
@@ -21,20 +21,7 @@
 #include <asm/io.h>
 #include <asm/arch/clk.h>
 #include <asm/arch/memory-map.h>
-
-#if defined(CONFIG_USART0)
-# define USART_ID	0
-# define USART_BASE	USART0_BASE
-#elif defined(CONFIG_USART1)
-# define USART_ID	1
-# define USART_BASE	USART1_BASE
-#elif defined(CONFIG_USART2)
-# define USART_ID	2
-# define USART_BASE	USART2_BASE
-#elif defined(CONFIG_USART3)
-# define USART_ID	3
-# define USART_BASE	USART3_BASE
-#endif
+#include <atmel_alt_usart.h>
 
 #include "atmel_usart.h"
 
diff --git a/include/atmel_alt_usart.h b/include/atmel_alt_usart.h
new file mode 100644
index 0000000..1ed2fb8
--- /dev/null
+++ b/include/atmel_alt_usart.h
@@ -0,0 +1,81 @@
+/*
+ *  Copyright (C) 2009 Thomas Sprinkmeier
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+#ifndef __ATMEL_ALT_USART_H__
+#define __ATMEL_ALT_USART_H__
+
+/* Define alternate USART ID and BASE */
+#if   defined(CONFIG_ALT_USART0)
+# define USART_ALT_ID	0
+# define USART_ALT_BASE	USART0_BASE
+#elif defined(CONFIG_ALT_USART1)
+# define USART_ALT_ID	1
+# define USART_ALT_BASE	USART1_BASE
+#elif defined(CONFIG_ALT_USART2)
+# define USART_ALT_ID	2
+# define USART_ALT_BASE	USART2_BASE
+#elif defined(CONFIG_ALT_USART3)
+# define USART_ALT_ID	3
+# define USART_ALT_BASE	USART3_BASE
+#elif
+/* No alternate USART selected */
+#define USART_USE_ALT (0)
+#define USART_PIN_CONFIG
+#endif
+
+/* How to read/initialise the boot-select jumper */
+#ifndef USART_USE_ALT
+#define USART_USE_ALT (!gpio_get_value(GPIO_PIN_PB(30)))
+#define USART_JUMPER_CONFIG portmux_select_gpio(PORTMUX_PORT_B,		\
+						(1 << 30),		\
+						PORTMUX_DIR_INPUT)
+#endif
+
+/*Configure default USART ID and BASE*/
+#if   defined(CONFIG_USART0)
+# define USART_DEF_ID	0
+# define USART_DEF_BASE	USART0_BASE
+#elif defined(CONFIG_USART1)
+# define USART_DEF_ID	1
+# define USART_DEF_BASE	USART1_BASE
+#elif defined(CONFIG_USART2)
+# define USART_DEF_ID	2
+# define USART_DEF_BASE	USART2_BASE
+#elif defined(CONFIG_USART3)
+# define USART_DEF_ID	3
+# define USART_DEF_BASE	USART3_BASE
+#elif
+#error You must define CONFIG_USART[0..1] to select a console port
+#endif
+
+
+/* Which USART ID/BASE to use. */
+#define USART_ID   (USART_USE_ALT ? USART_ALT_ID   : USART_DEF_ID)
+#define USART_BASE (USART_USE_ALT ? USART_ALT_BASE : USART_DEF_BASE)
+
+/* Enable the appropriate USART */
+#define USART_ENABLE {							\
+  switch (USART_ID)							\
+    {									\
+    case 0:  portmux_enable_usart0(PORTMUX_DRIVE_MIN); break;		\
+    case 1:  portmux_enable_usart1(PORTMUX_DRIVE_MIN); break;		\
+    case 2:  portmux_enable_usart2(PORTMUX_DRIVE_MIN); break;		\
+    default: portmux_enable_usart3(PORTMUX_DRIVE_MIN); break;		\
+    }									\
+  } while(0)
+
+#endif /* __ATMEL_ALT_USART_H__ */
diff --git a/include/configs/atngw100.h b/include/configs/atngw100.h
index 4ed5514..60a3b60 100644
--- a/include/configs/atngw100.h
+++ b/include/configs/atngw100.h
@@ -59,6 +59,7 @@
 #define CONFIG_SYS_PLL0_OPT			0x04
 
 #define CONFIG_USART1			1
+#define CONFIG_ALT_USART3		1
 
 /* User serviceable stuff */
 #define CONFIG_DOS_PARTITION		1
-- 
1.6.0.4

[U-Boot] use "boot select" jumper on NGW100 to select USART

[Wolfgang Denk]

Dear Thomas Sprinkmeier,

In message <E1N7TGC-0000mS-Vq@lego.my-domain> you wrote:
> From: Thomas Sprinkmeier <thomas.sprinkmeier@gmail.com>
> Date: Mon, 9 Nov 2009 09:28:51 +1030
> Subject: [PATCH] use "boot select" jumper on NGW100 to select USART
> 
> Without the "boot select" jumper U-Boot will use the USART selected
> using the CONFIG_USART1, CONFIG_USART2, directive.
> 
> If  CONFIG_ALT_USART1 (or ..2, ..3, ..0) is defined then,
> with the jumper in place, that USART is used instead.
> 
> Signed-off-by: Thomas Sprinkmeier <thomas.sprinkmeier@gmail.com
> ---
>  board/atmel/atngw100/atngw100.c |    6 ++-
>  drivers/serial/atmel_usart.c    |   15 +-------
>  include/atmel_alt_usart.h       |   81 +++++++++++++++++++++++++++++++++++++++
>  include/configs/atngw100.h      |    1 +
>  4 files changed, 88 insertions(+), 15 deletions(-)
>  create mode 100644 include/atmel_alt_usart.h
> 
> diff --git a/board/atmel/atngw100/atngw100.c b/board/atmel/atngw100/atngw100.c
> index 004d8da..841d9ec 100644
> --- a/board/atmel/atngw100/atngw100.c
> +++ b/board/atmel/atngw100/atngw100.c
> @@ -29,6 +29,8 @@
>  #include <asm/arch/portmux.h>
>  #include <netdev.h>
>  
> +#include <atmel_alt_usart.h>
> +
>  DECLARE_GLOBAL_DATA_PTR;
>  
>  static const struct sdram_config sdram_config = {
> @@ -53,7 +55,9 @@ int board_early_init_f(void)
>  	hmatrix_slave_write(EBI, SFR, HMATRIX_BIT(EBI_SDRAM_ENABLE));
>  
>  	portmux_enable_ebi(16, 23, 0, PORTMUX_DRIVE_HIGH);
> -	portmux_enable_usart1(PORTMUX_DRIVE_MIN);
> +
> +	USART_JUMPER_CONFIG;
> +	USART_ENABLE;

Function / Macro when used as funxctions should always look as such,
i. e. have parens with them.


> diff --git a/include/atmel_alt_usart.h b/include/atmel_alt_usart.h
> new file mode 100644
> index 0000000..1ed2fb8
...
> +/* Enable the appropriate USART */
> +#define USART_ENABLE {							\
> +  switch (USART_ID)							\
> +    {									\
> +    case 0:  portmux_enable_usart0(PORTMUX_DRIVE_MIN); break;		\
> +    case 1:  portmux_enable_usart1(PORTMUX_DRIVE_MIN); break;		\
> +    case 2:  portmux_enable_usart2(PORTMUX_DRIVE_MIN); break;		\
> +    default: portmux_enable_usart3(PORTMUX_DRIVE_MIN); break;		\
> +    }									\
> +  } while(0)

NAK.  When you use a function-style macro it should look like one. But
why is this a macro at all? Please make it a (static) inline function
instead.

Also please note that the Coding Style requires indentation by TABs.

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
F u cn rd ths u cnt spl wrth a dm!

[U-Boot] use "boot select" jumper on NGW100 to select USART

[Thomas Sprinkmeier]

Dear Wolfgang Denk,

>> + ? ? USART_JUMPER_CONFIG;
>> + ? ? USART_ENABLE;
>
> Function / Macro when used as funxctions should always look as such,
> i. e. have parens with them.

So the patch for atmel_usart.c would look like this:

 	hmatrix_slave_write(EBI, SFR, HMATRIX_BIT(EBI_SDRAM_ENABLE));

 	portmux_enable_ebi(16, 23, 0, PORTMUX_DRIVE_HIGH);
-	portmux_enable_usart1(PORTMUX_DRIVE_MIN);
+
+	USART_JUMPER_CONFIG();
+	atmel_usart_enable();

and atmel_alt_usart.h would change to:

+#elif
+/* No alternate USART selected */
+#define USART_USE_ALT() (0)
+#define USART_PIN_CONFIG()
+#endif
+
+/* How to read/initialise the boot-select jumper */
+#ifndef USART_USE_ALT
+#define USART_USE_ALT() (!gpio_get_value(GPIO_PIN_PB(30)))
+#define USART_JUMPER_CONFIG() portmux_select_gpio(PORTMUX_PORT_B,	\
+						(1 << 30),										\
+						PORTMUX_DIR_INPUT)
+#endif

or should USART_USE_ALT and USART_JUMPER_CONFIG become static inline
functions (as below)?

I was trying to make as little impact as possible, somehow a macro
seemed 'better' than a function.

>> diff --git a/include/atmel_alt_usart.h b/include/atmel_alt_usart.h
>> new file mode 100644
>> index 0000000..1ed2fb8
> ...
>> +/* Enable the appropriate USART */
>> +#define USART_ENABLE { ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
>> + ?switch (USART_ID) ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
>> + ? ?{ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
>> + ? ?case 0: ?portmux_enable_usart0(PORTMUX_DRIVE_MIN); break; ? ? ? ? ? ? ? ?\
>> + ? ?case 1: ?portmux_enable_usart1(PORTMUX_DRIVE_MIN); break; ? ? ? ? ? ? ? ?\
>> + ? ?case 2: ?portmux_enable_usart2(PORTMUX_DRIVE_MIN); break; ? ? ? ? ? ? ? ?\
>> + ? ?default: portmux_enable_usart3(PORTMUX_DRIVE_MIN); break; ? ? ? ? ? ? ? ?\
>> + ? ?} ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
>> + ?} while(0)
>
> NAK. ?When you use a function-style macro it should look like one. But
> why is this a macro at all? Please make it a (static) inline function
> instead.



+/* Enable the appropriate USART */
+static inline void atmel_usart_enable()
+{
+	switch (USART_ID) {
+	case 0:
+		portmux_enable_usart0(PORTMUX_DRIVE_MIN);
+		break;
+	case 1:
+		portmux_enable_usart1(PORTMUX_DRIVE_MIN);
+		break;
+	case 2:
+		portmux_enable_usart2(PORTMUX_DRIVE_MIN);
+		break;
+	default:
+		portmux_enable_usart3(PORTMUX_DRIVE_MIN);
+	}
+}
+
+#endif /* __ATMEL_ALT_USART_H__ */

> Also please note that the Coding Style requires indentation by TABs.

fixed.

Thank you for your patience,

Thomas

[U-Boot] use "boot select" jumper on NGW100 to select USART

[Wolfgang Denk]

Dear Thomas,

In message <e355f1140911092249l5c585dedy6970a9f8ce663b8f@mail.gmail.com> you wrote:
>
> So the patch for atmel_usart.c would look like this:
>
>  	hmatrix_slave_write(EBI, SFR, HMATRIX_BIT(EBI_SDRAM_ENABLE));
>
>  	portmux_enable_ebi(16, 23, 0, PORTMUX_DRIVE_HIGH);
> -	portmux_enable_usart1(PORTMUX_DRIVE_MIN);
> +
> +	USART_JUMPER_CONFIG();
> +	atmel_usart_enable();

Why not make USART_JUMPER_CONFIG() a function as well?

> or should USART_USE_ALT and USART_JUMPER_CONFIG become static inline
> functions (as below)?

That would probably be better, indeed.

> I was trying to make as little impact as possible, somehow a macro
> seemed 'better' than a function.

See "Documentation/CodingStyle", Chapter 12: Macros, Enums and RTL:

	Generally, inline functions are preferable to macros
	resembling functions.

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
How many hardware guys does it take to change a light bulb? "Well the
diagnostics say it's fine buddy, so it's a software problem."