[U-Boot] [PATCH 0/6] Introduce function mux library

[U-Boot] [PATCH 0/6] Introduce function mux library

[Simon Glass]

It is clear even at this early stage that some board/nvidia code
needs to move into the cpu area. Stephen Warren suggested two changes
which are the subject of the first two patches in this series.

This series also introduces a new function mux concept, which allows
selecting of pin options for a particular peripheral. This makes it
easy for boards to set up pin muxing without having to know the details,
and copy lots of code.

Most platforms will want UARTs to be configured, so we move this into
the cpu area also. The new board_init_uart_f() function looks after
configuring the UARTs as selected by the CONFIG_TEGRA2_ENABLE_...
options.

The last patch fixes a build error not introduced by this series.


Simon Glass (6):
  tegra: Move cpu_init_cp15() to arch_cpu_init()
  tegra: Move clock_early_init() to arch_cpu_init()
  tegra: Add a function mux feature
  tegra: Add support for UART init in cpu board.c
  tegra: Move boards over to use arch-level board UART function
  tegra: Fix build error in plutux, medcom

 arch/arm/cpu/armv7/tegra2/Makefile         |    2 +-
 arch/arm/cpu/armv7/tegra2/board.c          |   57 +++++++++++++++++++
 arch/arm/cpu/armv7/tegra2/funcmux.c        |   80 +++++++++++++++++++++++++++
 arch/arm/include/asm/arch-tegra2/board.h   |   30 ++++++++++
 arch/arm/include/asm/arch-tegra2/funcmux.h |   44 +++++++++++++++
 board/avionic-design/common/tamonten.c     |   50 +----------------
 board/nvidia/common/board.c                |   82 +---------------------------
 include/configs/medcom.h                   |    2 +
 include/configs/plutux.h                   |    2 +
 9 files changed, 221 insertions(+), 128 deletions(-)
 create mode 100644 arch/arm/cpu/armv7/tegra2/funcmux.c
 create mode 100644 arch/arm/include/asm/arch-tegra2/board.h
 create mode 100644 arch/arm/include/asm/arch-tegra2/funcmux.h

-- 
1.7.3.1

[U-Boot] [PATCH 1/6] tegra: Move cpu_init_cp15() to arch_cpu_init()

[Simon Glass]

This call is more of an architecture requirement than a board
one, so move it there.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 arch/arm/cpu/armv7/tegra2/board.c |    3 +++
 board/nvidia/common/board.c       |    3 ---
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/cpu/armv7/tegra2/board.c b/arch/arm/cpu/armv7/tegra2/board.c
index 59dce8f..f17b369 100644
--- a/arch/arm/cpu/armv7/tegra2/board.c
+++ b/arch/arm/cpu/armv7/tegra2/board.c
@@ -80,6 +80,9 @@ int arch_cpu_init(void)
 {
 	/* Fire up the Cortex A9 */
 	tegra2_start();
+
+	/* We didn't do this init in start.S, so do it now */
+	cpu_init_cp15();
 	return 0;
 }
 #endif
diff --git a/board/nvidia/common/board.c b/board/nvidia/common/board.c
index f39fb24..219b26a 100644
--- a/board/nvidia/common/board.c
+++ b/board/nvidia/common/board.c
@@ -133,9 +133,6 @@ int board_early_init_f(void)
 {
 	int uart_ids = 0;	/* bit mask of which UART ids to enable */
 
-	/* We didn't do this init in start.S, so do it now */
-	cpu_init_cp15();
-
 #ifdef CONFIG_TEGRA2_ENABLE_UARTA
 	uart_ids |= UARTA;
 #endif
-- 
1.7.3.1

[U-Boot] [PATCH 2/6] tegra: Move clock_early_init() to arch_cpu_init()

[Simon Glass]

The clock init is not board specific, so move it into
the cpu code.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 arch/arm/cpu/armv7/tegra2/board.c |    3 +++
 board/nvidia/common/board.c       |    3 ---
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/cpu/armv7/tegra2/board.c b/arch/arm/cpu/armv7/tegra2/board.c
index f17b369..2fd38ae 100644
--- a/arch/arm/cpu/armv7/tegra2/board.c
+++ b/arch/arm/cpu/armv7/tegra2/board.c
@@ -83,6 +83,9 @@ int arch_cpu_init(void)
 
 	/* We didn't do this init in start.S, so do it now */
 	cpu_init_cp15();
+
+	/* Initialize essential common plls */
+	clock_early_init();
 	return 0;
 }
 #endif
diff --git a/board/nvidia/common/board.c b/board/nvidia/common/board.c
index 219b26a..0ff656a 100644
--- a/board/nvidia/common/board.c
+++ b/board/nvidia/common/board.c
@@ -143,9 +143,6 @@ int board_early_init_f(void)
 	uart_ids |= UARTD;
 #endif
 
-	/* Initialize essential common plls */
-	clock_early_init();
-
 	/* Initialize UART clocks */
 	clock_init_uart(uart_ids);
 
-- 
1.7.3.1

[U-Boot] [PATCH 3/6] tegra: Add a function mux feature

[Simon Glass]

funcmux permits selection of config options for particular peripherals,
such as the pins that are used for that peripheral, if there are several
options.

Add UART selection to start with.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 arch/arm/cpu/armv7/tegra2/Makefile         |    2 +-
 arch/arm/cpu/armv7/tegra2/funcmux.c        |   80 ++++++++++++++++++++++++++++
 arch/arm/include/asm/arch-tegra2/funcmux.h |   44 +++++++++++++++
 3 files changed, 125 insertions(+), 1 deletions(-)
 create mode 100644 arch/arm/cpu/armv7/tegra2/funcmux.c
 create mode 100644 arch/arm/include/asm/arch-tegra2/funcmux.h

diff --git a/arch/arm/cpu/armv7/tegra2/Makefile b/arch/arm/cpu/armv7/tegra2/Makefile
index 955c3b6..f668a81 100644
--- a/arch/arm/cpu/armv7/tegra2/Makefile
+++ b/arch/arm/cpu/armv7/tegra2/Makefile
@@ -33,7 +33,7 @@ include $(TOPDIR)/config.mk
 LIB	=  $(obj)lib$(SOC).o
 
 SOBJS	:= lowlevel_init.o
-COBJS	:= ap20.o board.o clock.o pinmux.o sys_info.o timer.o
+COBJS	:= ap20.o board.o clock.o funcmux.o pinmux.o sys_info.o timer.o
 
 SRCS	:= $(SOBJS:.o=.S) $(COBJS:.o=.c)
 OBJS	:= $(addprefix $(obj),$(COBJS) $(SOBJS))
diff --git a/arch/arm/cpu/armv7/tegra2/funcmux.c b/arch/arm/cpu/armv7/tegra2/funcmux.c
new file mode 100644
index 0000000..7651cb4
--- /dev/null
+++ b/arch/arm/cpu/armv7/tegra2/funcmux.c
@@ -0,0 +1,80 @@
+/*
+ * Copyright (c) 2011 The Chromium OS Authors.
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * 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
+ */
+
+/* Tegra2 high-level function multiplexing */
+#include <common.h>
+#include <asm/arch/clock.h>
+#include <asm/arch/pinmux.h>
+
+static void enable_uart(enum periph_id pid)
+{
+	/* Assert UART reset and enable clock */
+	reset_set_enable(pid, 1);
+	clock_enable(pid);
+	clock_ll_set_source(pid, 0);	/* UARTx_CLK_SRC = 00, PLLP_OUT0 */
+
+	/* wait for 2us */
+	udelay(2);
+
+	/* De-assert reset to UART */
+	reset_set_enable(pid, 0);
+}
+
+void funcmux_select(enum periph_id id, int func)
+{
+	switch (id) {
+	case PERIPH_ID_UART1:
+		pinmux_set_func(PINGRP_IRRX, PMUX_FUNC_UARTA);
+		pinmux_set_func(PINGRP_IRTX, PMUX_FUNC_UARTA);
+		pinmux_tristate_disable(PINGRP_IRRX);
+		pinmux_tristate_disable(PINGRP_IRTX);
+		break;
+
+	case PERIPH_ID_UART2:
+		pinmux_set_func(PINGRP_UAD, PMUX_FUNC_IRDA);
+		pinmux_tristate_disable(PINGRP_UAD);
+		break;
+
+	case PERIPH_ID_UART4:
+		pinmux_set_func(PINGRP_GMC, PMUX_FUNC_UARTD);
+		pinmux_tristate_disable(PINGRP_GMC);
+		break;
+
+	default:
+		debug("%s: invalid periph_id %d", __func__, id);
+		break;
+	}
+}
+
+void funcmux_enable_f(enum periph_id id)
+{
+	switch (id) {
+	case PERIPH_ID_UART1:
+	case PERIPH_ID_UART2:
+	case PERIPH_ID_UART4:
+		enable_uart(id);
+		break;
+
+	default:
+		debug("%s: invalid periph_id %d", __func__, id);
+		break;
+	}
+}
diff --git a/arch/arm/include/asm/arch-tegra2/funcmux.h b/arch/arm/include/asm/arch-tegra2/funcmux.h
new file mode 100644
index 0000000..f357ab4
--- /dev/null
+++ b/arch/arm/include/asm/arch-tegra2/funcmux.h
@@ -0,0 +1,44 @@
+/*
+ * Copyright (c) 2011 The Chromium OS Authors.
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * 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
+ */
+
+/* Tegra2 high-level function multiplexing */
+
+/**
+ * Select a config for a particular peripheral.
+ *
+ * Each peripheral can operate through a number of configurations,
+ * which are sets of pins that it uses to bring out its signals.
+ * The basic config is 0, and higher numbers indicate different
+ * pinmux settings to bring the peripheral out on other pins,
+ *
+ * @param id		Peripheral id
+ * @param config	Configuration to use (generally 0)
+ */
+void funcmux_select(enum periph_id id, int config);
+
+/**
+ * Enable a particular peripheral prior to relocation, when the full clock
+ * features are not available. After relocation you should call
+ * clock_start_periph_pll() for this.
+ *
+ * This function currently only supports enabling the UARTs.
+ */
+void funcmux_enable_f(enum periph_id id);
-- 
1.7.3.1

[U-Boot] [PATCH 4/6] tegra: Add support for UART init in cpu board.c

[Simon Glass]

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 arch/arm/cpu/armv7/tegra2/board.c        |   51 ++++++++++++++++++++++++++++++
 arch/arm/include/asm/arch-tegra2/board.h |   30 +++++++++++++++++
 2 files changed, 81 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/include/asm/arch-tegra2/board.h

diff --git a/arch/arm/cpu/armv7/tegra2/board.c b/arch/arm/cpu/armv7/tegra2/board.c
index 2fd38ae..2fd493c 100644
--- a/arch/arm/cpu/armv7/tegra2/board.c
+++ b/arch/arm/cpu/armv7/tegra2/board.c
@@ -24,12 +24,22 @@
 #include <common.h>
 #include <asm/io.h>
 #include "ap20.h"
+#include <asm/arch/clock.h>
+#include <asm/arch/funcmux.h>
 #include <asm/arch/sys_proto.h>
 #include <asm/arch/tegra2.h>
 #include <asm/arch/pmc.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
+enum {
+	/* UARTs which we can enable */
+	UARTA	= 1 << 0,
+	UARTB	= 1 << 1,
+	UARTD	= 1 << 3,
+	UART_COUNT = 4,
+};
+
 /*
  * Boot ROM initializes the odmdata in APBDEV_PMC_SCRATCH20_0,
  * so we are using this value to identify memory size.
@@ -89,3 +99,44 @@ int arch_cpu_init(void)
 	return 0;
 }
 #endif
+
+/**
+ * Set up the specified uarts
+ *
+ * @param uarts_ids	Mask containing UARTs to init (UARTx)
+ */
+static void setup_uarts(int uart_ids)
+{
+	static enum periph_id id_for_uart[UART_COUNT] = {
+		PERIPH_ID_UART1,
+		PERIPH_ID_UART2,
+		PERIPH_ID_UART3,
+		PERIPH_ID_UART4,
+	};
+	int i;
+
+	for (i = 0; i < UART_COUNT; i++) {
+		if (uart_ids & (1 << i)) {
+			enum periph_id id = id_for_uart[i];
+
+			funcmux_select(id, 0);
+			funcmux_enable_f(id);
+		}
+	}
+}
+
+void board_init_uart_f(void)
+{
+	int uart_ids = 0;	/* bit mask of which UART ids to enable */
+
+#ifdef CONFIG_TEGRA2_ENABLE_UARTA
+	uart_ids |= UARTA;
+#endif
+#ifdef CONFIG_TEGRA2_ENABLE_UARTB
+	uart_ids |= UARTB;
+#endif
+#ifdef CONFIG_TEGRA2_ENABLE_UARTD
+	uart_ids |= UARTD;
+#endif
+	setup_uarts(uart_ids);
+}
diff --git a/arch/arm/include/asm/arch-tegra2/board.h b/arch/arm/include/asm/arch-tegra2/board.h
new file mode 100644
index 0000000..a90d36c
--- /dev/null
+++ b/arch/arm/include/asm/arch-tegra2/board.h
@@ -0,0 +1,30 @@
+/*
+ *  (C) Copyright 2010,2011
+ *  NVIDIA Corporation <www.nvidia.com>
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * 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 _TEGRA_BOARD_H_
+#define _TEGRA_BOARD_H_
+
+/* Setup UARTs for the board according to the selected config */
+void board_init_uart_f(void);
+
+#endif
-- 
1.7.3.1

[U-Boot] [PATCH 5/6] tegra: Move boards over to use arch-level board UART function

[Simon Glass]

Now that we can set up the UART in common tegra code, make the boards
use it.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 board/avionic-design/common/tamonten.c |   50 +--------------------
 board/nvidia/common/board.c            |   76 +-------------------------------
 2 files changed, 5 insertions(+), 121 deletions(-)

diff --git a/board/avionic-design/common/tamonten.c b/board/avionic-design/common/tamonten.c
index 98aa0f8..97e59fb 100644
--- a/board/avionic-design/common/tamonten.c
+++ b/board/avionic-design/common/tamonten.c
@@ -27,6 +27,7 @@
 #include <ns16550.h>
 #include <asm/io.h>
 #include <asm/gpio.h>
+#include <asm/arch/board.h>
 #include <asm/arch/tegra2.h>
 #include <asm/arch/sys_proto.h>
 #include <asm/arch/clk_rst.h>
@@ -55,44 +56,6 @@ int timer_init(void)
 	return 0;
 }
 
-static void enable_uart(enum periph_id pid)
-{
-	/* Assert UART reset and enable clock */
-	reset_set_enable(pid, 1);
-	clock_enable(pid);
-	clock_ll_set_source(pid, 0);	/* UARTx_CLK_SRC = 00, PLLP_OUT0 */
-
-	/* wait for 2us */
-	udelay(2);
-
-	/* De-assert reset to UART */
-	reset_set_enable(pid, 0);
-}
-
-/*
- * Routine: clock_init_uart
- * Description: init the PLL and clock for the UART(s)
- */
-static void clock_init_uart(void)
-{
-#if defined(CONFIG_TEGRA2_ENABLE_UARTD)
-	enable_uart(PERIPH_ID_UART4);
-#endif /* CONFIG_TEGRA2_ENABLE_UARTD */
-}
-
-/*
- * Routine: pin_mux_uart
- * Description: setup the pin muxes/tristate values for the UART(s)
- */
-static void pin_mux_uart(void)
-{
-#if defined(CONFIG_TEGRA2_ENABLE_UARTD)
-	pinmux_set_func(PINGRP_GMC, PMUX_FUNC_UARTD);
-
-	pinmux_tristate_disable(PINGRP_GMC);
-#endif /* CONFIG_TEGRA2_ENABLE_UARTD */
-}
-
 #ifdef CONFIG_TEGRA2_MMC
 /*
  * Routine: pin_mux_mmc
@@ -146,15 +109,8 @@ int board_mmc_init(bd_t *bd)
 #ifdef CONFIG_BOARD_EARLY_INIT_F
 int board_early_init_f(void)
 {
-	/* Initialize essential common plls */
-	clock_early_init();
-
-	/* Initialize UART clocks */
-	clock_init_uart();
-
-	/* Initialize periph pinmuxes */
-	pin_mux_uart();
-
+	/* Initialize selected UARTs */
+	board_init_uart_f();
 	return 0;
 }
 #endif /* EARLY_INIT */
diff --git a/board/nvidia/common/board.c b/board/nvidia/common/board.c
index 0ff656a..e8253a0 100644
--- a/board/nvidia/common/board.c
+++ b/board/nvidia/common/board.c
@@ -27,6 +27,7 @@
 #include <asm/arch/tegra2.h>
 #include <asm/arch/sys_proto.h>
 
+#include <asm/arch/board.h>
 #include <asm/arch/clk_rst.h>
 #include <asm/arch/clock.h>
 #include <asm/arch/pinmux.h>
@@ -36,13 +37,6 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
-enum {
-	/* UARTs which we can enable */
-	UARTA	= 1 << 0,
-	UARTB	= 1 << 1,
-	UARTD	= 1 << 3,
-};
-
 const struct tegra2_sysinfo sysinfo = {
 	CONFIG_TEGRA2_BOARD_STRING
 };
@@ -56,56 +50,6 @@ int timer_init(void)
 	return 0;
 }
 
-static void enable_uart(enum periph_id pid)
-{
-	/* Assert UART reset and enable clock */
-	reset_set_enable(pid, 1);
-	clock_enable(pid);
-	clock_ll_set_source(pid, 0);	/* UARTx_CLK_SRC = 00, PLLP_OUT0 */
-
-	/* wait for 2us */
-	udelay(2);
-
-	/* De-assert reset to UART */
-	reset_set_enable(pid, 0);
-}
-
-/*
- * Routine: clock_init_uart
- * Description: init clock for the UART(s)
- */
-static void clock_init_uart(int uart_ids)
-{
-	if (uart_ids & UARTA)
-		enable_uart(PERIPH_ID_UART1);
-	if (uart_ids & UARTB)
-		enable_uart(PERIPH_ID_UART2);
-	if (uart_ids & UARTD)
-		enable_uart(PERIPH_ID_UART4);
-}
-
-/*
- * Routine: pin_mux_uart
- * Description: setup the pin muxes/tristate values for the UART(s)
- */
-static void pin_mux_uart(int uart_ids)
-{
-	if (uart_ids & UARTA) {
-		pinmux_set_func(PINGRP_IRRX, PMUX_FUNC_UARTA);
-		pinmux_set_func(PINGRP_IRTX, PMUX_FUNC_UARTA);
-		pinmux_tristate_disable(PINGRP_IRRX);
-		pinmux_tristate_disable(PINGRP_IRTX);
-	}
-	if (uart_ids & UARTB) {
-		pinmux_set_func(PINGRP_UAD, PMUX_FUNC_IRDA);
-		pinmux_tristate_disable(PINGRP_UAD);
-	}
-	if (uart_ids & UARTD) {
-		pinmux_set_func(PINGRP_GMC, PMUX_FUNC_UARTD);
-		pinmux_tristate_disable(PINGRP_GMC);
-	}
-}
-
 /*
  * Routine: board_init
  * Description: Early hardware init.
@@ -131,23 +75,7 @@ int board_init(void)
 #ifdef CONFIG_BOARD_EARLY_INIT_F
 int board_early_init_f(void)
 {
-	int uart_ids = 0;	/* bit mask of which UART ids to enable */
-
-#ifdef CONFIG_TEGRA2_ENABLE_UARTA
-	uart_ids |= UARTA;
-#endif
-#ifdef CONFIG_TEGRA2_ENABLE_UARTB
-	uart_ids |= UARTB;
-#endif
-#ifdef CONFIG_TEGRA2_ENABLE_UARTD
-	uart_ids |= UARTD;
-#endif
-
-	/* Initialize UART clocks */
-	clock_init_uart(uart_ids);
-
-	/* Initialize periph pinmuxes */
-	pin_mux_uart(uart_ids);
+	board_init_uart_f();
 
 	/* Initialize periph GPIOs */
 #ifdef CONFIG_SPI_UART_SWITCH
-- 
1.7.3.1

[U-Boot] [PATCH 6/6] tegra: Fix build error in plutux, medcom

[Simon Glass]

We need to define CONFIG_ENV_IS_NOWHERE to avoid this error:

cmd_nvedit.c:69:3: error: #error Define one of CONFIG_ENV_IS_IN_{EEPROM|FLASH|DATAFLASH|ONENAND|SPI_FLASH|MG_DISK|NVRAM|MMC} or CONFIG_ENV_IS_NOWHERE

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 include/configs/medcom.h |    2 ++
 include/configs/plutux.h |    2 ++
 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/include/configs/medcom.h b/include/configs/medcom.h
index 6a54925..2dc3507 100644
--- a/include/configs/medcom.h
+++ b/include/configs/medcom.h
@@ -41,6 +41,8 @@
 
 #define CONFIG_BOARD_EARLY_INIT_F
 
+#define CONFIG_ENV_IS_NOWHERE
+
 /* SD/MMC */
 #define CONFIG_MMC
 #define CONFIG_GENERIC_MMC
diff --git a/include/configs/plutux.h b/include/configs/plutux.h
index 033eda0..f869191 100644
--- a/include/configs/plutux.h
+++ b/include/configs/plutux.h
@@ -41,6 +41,8 @@
 
 #define CONFIG_BOARD_EARLY_INIT_F
 
+#define CONFIG_ENV_IS_NOWHERE
+
 /* SD/MMC */
 #define CONFIG_MMC
 #define CONFIG_GENERIC_MMC
-- 
1.7.3.1

[U-Boot] [PATCH 1/6] tegra: Move cpu_init_cp15() to arch_cpu_init()

[Mike Frysinger]

On Wednesday 23 November 2011 17:59:03 Simon Glass wrote:
> --- a/arch/arm/cpu/armv7/tegra2/board.c
> +++ b/arch/arm/cpu/armv7/tegra2/board.c
> @@ -80,6 +80,9 @@ int arch_cpu_init(void)
>  {
>  	/* Fire up the Cortex A9 */
>  	tegra2_start();
> +
> +	/* We didn't do this init in start.S, so do it now */
> +	cpu_init_cp15();
>  	return 0;
>  }

probably a pain since you've got a bunch of little patches, but there should 
be a blank line before that return statement.  it was semi-OK because before 
there were only two statements in this func, but now that there are more, the 
return should be by itself.

i'll leave it up to you/tegra-maintainer if this really needs to get fixed in 
all the patches, or just the last one.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20111123/1b9d9934/attachment.pgp>

[U-Boot] [PATCH 3/6] tegra: Add a function mux feature

[Mike Frysinger]

On Wednesday 23 November 2011 17:59:05 Simon Glass wrote:
> --- /dev/null
> +++ b/arch/arm/cpu/armv7/tegra2/funcmux.c
>
> +void funcmux_select(enum periph_id id, int func)
> +{
> +	switch (id) {
> +	case PERIPH_ID_UART1:
> +		pinmux_set_func(PINGRP_IRRX, PMUX_FUNC_UARTA);
> +		pinmux_set_func(PINGRP_IRTX, PMUX_FUNC_UARTA);
> +		pinmux_tristate_disable(PINGRP_IRRX);
> +		pinmux_tristate_disable(PINGRP_IRTX);
> +		break;
> +
> +	case PERIPH_ID_UART2:
> +		pinmux_set_func(PINGRP_UAD, PMUX_FUNC_IRDA);
> +		pinmux_tristate_disable(PINGRP_UAD);
> +		break;
> +
> +	case PERIPH_ID_UART4:
> +		pinmux_set_func(PINGRP_GMC, PMUX_FUNC_UARTD);
> +		pinmux_tristate_disable(PINGRP_GMC);
> +		break;
> +
> +	default:
> +		debug("%s: invalid periph_id %d", __func__, id);
> +		break;
> +	}
> +}

i don't know anything about the tegra pinmux details, but the way we setup the 
defines in the Blackfin code was to encode all of the bits.  thus we didn't need 
to switch() on any of the id's, we just unpacked the values at runtime with 
bitshifts/masks.

> --- /dev/null
> +++ b/arch/arm/include/asm/arch-tegra2/funcmux.h

needs ifdef protection against multiple inclusion
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20111123/66625749/attachment.pgp>

[U-Boot] [PATCH 4/6] tegra: Add support for UART init in cpu board.c

[Mike Frysinger]

On Wednesday 23 November 2011 17:59:06 Simon Glass wrote:
> --- a/arch/arm/cpu/armv7/tegra2/board.c
> +++ b/arch/arm/cpu/armv7/tegra2/board.c
>
> +static void setup_uarts(int uart_ids)
> +{
> +	static enum periph_id id_for_uart[UART_COUNT] = {
> +		PERIPH_ID_UART1,
> +		PERIPH_ID_UART2,
> +		PERIPH_ID_UART3,
> +		PERIPH_ID_UART4,
> +	};

does this need the [UART_COUNT] ?  what if you used "[]" ...

> +	int i;

size_t

> +	for (i = 0; i < UART_COUNT; i++) {

... and then did ARRAY_SIZE(id_for_uart) ?

> +void board_init_uart_f(void)
> +{
> +	int uart_ids = 0;	/* bit mask of which UART ids to enable */
> +
> +#ifdef CONFIG_TEGRA2_ENABLE_UARTA
> +	uart_ids |= UARTA;
> +#endif
> +#ifdef CONFIG_TEGRA2_ENABLE_UARTB
> +	uart_ids |= UARTB;
> +#endif
> +#ifdef CONFIG_TEGRA2_ENABLE_UARTD
> +	uart_ids |= UARTD;
> +#endif
> +	setup_uarts(uart_ids);
> +}

we added a func in the Blackfin pinmux API where you could pass it a list of 
peripherals to mux.  so you could do:
	static const unsigned pin_list[] = {
	#ifdef CONFIG_TEGRA2_ENABLE_UARTA
		UART1,
	#endif
		0,
	};
	pinmux_request_list(pin_list);

and the list version would just walk the 0-terminated array calling 
pinmux_request() automatically
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20111123/4d270357/attachment.pgp>

[U-Boot] [PATCH 6/6] tegra: Fix build error in plutux, medcom

[Mike Frysinger]

On Wednesday 23 November 2011 17:59:08 Simon Glass wrote:
> We need to define CONFIG_ENV_IS_NOWHERE to avoid this error:
> 
> cmd_nvedit.c:69:3: error: #error Define one of
> CONFIG_ENV_IS_IN_{EEPROM|FLASH|DATAFLASH|ONENAND|SPI_FLASH|MG_DISK|NVRAM|M
> MC} or CONFIG_ENV_IS_NOWHERE

i'd truncate/abbreviate that #error string
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20111123/de34ff05/attachment.pgp>

[U-Boot] [PATCH 6/6] tegra: Fix build error in plutux, medcom

[Thierry Reding]

* Simon Glass wrote:
> We need to define CONFIG_ENV_IS_NOWHERE to avoid this error:
> 
> cmd_nvedit.c:69:3: error: #error Define one of CONFIG_ENV_IS_IN_{EEPROM|FLASH|DATAFLASH|ONENAND|SPI_FLASH|MG_DISK|NVRAM|MMC} or CONFIG_ENV_IS_NOWHERE

Odd. I haven't seen this before when building both boards. Perhaps this was
introduced by some other patch I don't have applied. Anyway, thanks for
fixing this.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20111124/d21a6c12/attachment.pgp>

[U-Boot] [PATCH 1/6] tegra: Move cpu_init_cp15() to arch_cpu_init()

[Stephen Warren]

On 11/23/2011 03:59 PM, Simon Glass wrote:
> This call is more of an architecture requirement than a board
> one, so move it there.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Acked-by: Stephen Warren <swarren@nvidia.com>

-- 
nvpublic

[U-Boot] [PATCH 2/6] tegra: Move clock_early_init() to arch_cpu_init()

[Stephen Warren]

On 11/23/2011 03:59 PM, Simon Glass wrote:
> The clock init is not board specific, so move it into
> the cpu code.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Acked-by: Stephen Warren <swarren@nvidia.com>

-- 
nvpublic

[U-Boot] [PATCH 3/6] tegra: Add a function mux feature

[Stephen Warren]

On 11/23/2011 03:59 PM, Simon Glass wrote:
> funcmux permits selection of config options for particular peripherals,
> such as the pins that are used for that peripheral, if there are several
> options.
> 
> Add UART selection to start with.

> +static void enable_uart(enum periph_id pid)
> +{
> +	/* Assert UART reset and enable clock */
> +	reset_set_enable(pid, 1);
> +	clock_enable(pid);
> +	clock_ll_set_source(pid, 0);	/* UARTx_CLK_SRC = 00, PLLP_OUT0 */
> +
> +	/* wait for 2us */
> +	udelay(2);
> +
> +	/* De-assert reset to UART */
> +	reset_set_enable(pid, 0);
> +}

That doesn't seem like anything to do with function muxing.

> +void funcmux_select(enum periph_id id, int func)

Parameter "func" doesn't appear to be used. Is it to support e.g. UART1
being routed to different sets of pins based on board design? If so, the
values of "func" should be defined by this patch too, and validated in
the code below, so that people don't start passing bogus data without
issue now, then suddenly get hit when we see boards with different
pinmux configurations.

> +{
> +	switch (id) {
> +	case PERIPH_ID_UART1:
> +		pinmux_set_func(PINGRP_IRRX, PMUX_FUNC_UARTA);
> +		pinmux_set_func(PINGRP_IRTX, PMUX_FUNC_UARTA);
> +		pinmux_tristate_disable(PINGRP_IRRX);
> +		pinmux_tristate_disable(PINGRP_IRTX);
> +		break;
> +
> +	case PERIPH_ID_UART2:
> +		pinmux_set_func(PINGRP_UAD, PMUX_FUNC_IRDA);
> +		pinmux_tristate_disable(PINGRP_UAD);
> +		break;
> +
> +	case PERIPH_ID_UART4:
> +		pinmux_set_func(PINGRP_GMC, PMUX_FUNC_UARTD);
> +		pinmux_tristate_disable(PINGRP_GMC);
> +		break;
> +
> +	default:
> +		debug("%s: invalid periph_id %d", __func__, id);
> +		break;
> +	}
> +}

I'm not entirely convinced that centralizing this in a function rather
than putting the board-specific muxing into the per-board files is the
right way to go. What's wrong with the kernel's approach of a single
table describing each board's complete pinmux settings? Eventually, all
this will come from DT anyway, won't it?

-- 
nvpublic

[U-Boot] [PATCH 6/6] tegra: Fix build error in plutux, medcom

[Stephen Warren]

On 11/23/2011 03:59 PM, Simon Glass wrote:
> We need to define CONFIG_ENV_IS_NOWHERE to avoid this error:
> 
> cmd_nvedit.c:69:3: error: #error Define one of CONFIG_ENV_IS_IN_{EEPROM|FLASH|DATAFLASH|ONENAND|SPI_FLASH|MG_DISK|NVRAM|MMC} or CONFIG_ENV_IS_NOWHERE

Seems like you need to wrap that.

Acked-by: Stephen Warren <swarren@nvidia.com>

-- 
nvpublic

[U-Boot] [PATCH 1/6] tegra: Move cpu_init_cp15() to arch_cpu_init()

[Simon Glass]

Hi Mike,

On Wed, Nov 23, 2011 at 3:34 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Wednesday 23 November 2011 17:59:03 Simon Glass wrote:
>> --- a/arch/arm/cpu/armv7/tegra2/board.c
>> +++ b/arch/arm/cpu/armv7/tegra2/board.c
>> @@ -80,6 +80,9 @@ int arch_cpu_init(void)
>> ?{
>> ? ? ? /* Fire up the Cortex A9 */
>> ? ? ? tegra2_start();
>> +
>> + ? ? /* We didn't do this init in start.S, so do it now */
>> + ? ? cpu_init_cp15();
>> ? ? ? return 0;
>> ?}
>
> probably a pain since you've got a bunch of little patches, but there should
> be a blank line before that return statement. ?it was semi-OK because before
> there were only two statements in this func, but now that there are more, the
> return should be by itself.

No pain, thanks for picking this up.

>
> i'll leave it up to you/tegra-maintainer if this really needs to get fixed in
> all the patches, or just the last one.
> -mike
>

Regards,
Simon

[U-Boot] [PATCH 3/6] tegra: Add a function mux feature

[Simon Glass]

Hi Stephen,

On Mon, Nov 28, 2011 at 10:17 AM, Stephen Warren <swarren@nvidia.com> wrote:
> On 11/23/2011 03:59 PM, Simon Glass wrote:
>> funcmux permits selection of config options for particular peripherals,
>> such as the pins that are used for that peripheral, if there are several
>> options.

Thanks for looking at this.

>>
>> Add UART selection to start with.
>
>> +static void enable_uart(enum periph_id pid)
>> +{
>> + ? ? /* Assert UART reset and enable clock */
>> + ? ? reset_set_enable(pid, 1);
>> + ? ? clock_enable(pid);
>> + ? ? clock_ll_set_source(pid, 0); ? ?/* UARTx_CLK_SRC = 00, PLLP_OUT0 */
>> +
>> + ? ? /* wait for 2us */
>> + ? ? udelay(2);
>> +
>> + ? ? /* De-assert reset to UART */
>> + ? ? reset_set_enable(pid, 0);
>> +}
>
> That doesn't seem like anything to do with function muxing.

My thoughts initially were that funcmux_enable_f() might support other
devices. Really the correct thing to do is call
clock_start_periph_pll() but this can't be used prior to relocation.
We are trying to provide a simple function to enable a UART which is
why I thought funcmux might be a good place (since its aim is to
enable things to make functions work).

One option is to move this function into clock.c and call it
clock_ll_start_periph_pll() perhaps:

void clock_ll_start_periph_pll(enum periph_id periph_id, int source)

where source is a plain number (in this case always 0). But it's a bit
uglier in that callers must pass 0, otherwise the UART won't work. So
I am leaning towards something like:

void clock_ll_start_uart(enum periph_id periph_id)

Thoughts?

>
>> +void funcmux_select(enum periph_id id, int func)
>
> Parameter "func" doesn't appear to be used. Is it to support e.g. UART1
> being routed to different sets of pins based on board design? If so, the

Yes that's right, will be used for i2c and MMC also.

> values of "func" should be defined by this patch too, and validated in
> the code below, so that people don't start passing bogus data without
> issue now, then suddenly get hit when we see boards with different
> pinmux configurations.

Yes, will fix.

>
>> +{
>> + ? ? switch (id) {
>> + ? ? case PERIPH_ID_UART1:
>> + ? ? ? ? ? ? pinmux_set_func(PINGRP_IRRX, PMUX_FUNC_UARTA);
>> + ? ? ? ? ? ? pinmux_set_func(PINGRP_IRTX, PMUX_FUNC_UARTA);
>> + ? ? ? ? ? ? pinmux_tristate_disable(PINGRP_IRRX);
>> + ? ? ? ? ? ? pinmux_tristate_disable(PINGRP_IRTX);
>> + ? ? ? ? ? ? break;
>> +
>> + ? ? case PERIPH_ID_UART2:
>> + ? ? ? ? ? ? pinmux_set_func(PINGRP_UAD, PMUX_FUNC_IRDA);
>> + ? ? ? ? ? ? pinmux_tristate_disable(PINGRP_UAD);
>> + ? ? ? ? ? ? break;
>> +
>> + ? ? case PERIPH_ID_UART4:
>> + ? ? ? ? ? ? pinmux_set_func(PINGRP_GMC, PMUX_FUNC_UARTD);
>> + ? ? ? ? ? ? pinmux_tristate_disable(PINGRP_GMC);
>> + ? ? ? ? ? ? break;
>> +
>> + ? ? default:
>> + ? ? ? ? ? ? debug("%s: invalid periph_id %d", __func__, id);
>> + ? ? ? ? ? ? break;
>> + ? ? }
>> +}
>
> I'm not entirely convinced that centralizing this in a function rather
> than putting the board-specific muxing into the per-board files is the
> right way to go. What's wrong with the kernel's approach of a single
> table describing each board's complete pinmux settings? Eventually, all
> this will come from DT anyway, won't it?

I agree what you seem to be implying (that all Tegra boards will use DT).

I would like to reduce code duplication. Yes, when the DT supports
pinmux we can call a function to set it up. But even then the code
will not be in the board files - only the function call to some sort
of pinmux library will be there.

At present we have code like this in the boards:

	/* SDMMC4: config 3, x8 on 2nd set of pins */
	pinmux_set_func(PINGRP_ATB, PMUX_FUNC_SDIO4);
	pinmux_set_func(PINGRP_GMA, PMUX_FUNC_SDIO4);
	pinmux_set_func(PINGRP_GME, PMUX_FUNC_SDIO4);

	pinmux_tristate_disable(PINGRP_ATB);
	pinmux_tristate_disable(PINGRP_GMA);
	pinmux_tristate_disable(PINGRP_GME);

which I want to move into funcmux. The only parameters are the
peripheral ID and the function option. When we have pinmux in the DT
then these functions can look up the DT to get their info.

Also with U-Boot we don't necessarily want to enable everything at
start of day in U-Boot. So it may still be required that we init
(e.g.) the MMC only when an mmc command is executed.

Obviously this needs to be in review. As things change with the kernel
we can react. But in any case we want the affected code to be in once
place, hence funcmux.

Thoughts?

Regards,
Simon

>
> --
> nvpublic
>

[U-Boot] [PATCH 3/6] tegra: Add a function mux feature

[Stephen Warren]

On 11/28/2011 12:19 PM, Simon Glass wrote:
> Hi Stephen,
> 
> On Mon, Nov 28, 2011 at 10:17 AM, Stephen Warren <swarren@nvidia.com> wrote:
>> On 11/23/2011 03:59 PM, Simon Glass wrote:
>>> funcmux permits selection of config options for particular peripherals,
>>> such as the pins that are used for that peripheral, if there are several
>>> options.
> 
> Thanks for looking at this.
> 
>>>
>>> Add UART selection to start with.
>>
>>> +static void enable_uart(enum periph_id pid)
>>> +{
>>> +     /* Assert UART reset and enable clock */
>>> +     reset_set_enable(pid, 1);
>>> +     clock_enable(pid);
>>> +     clock_ll_set_source(pid, 0);    /* UARTx_CLK_SRC = 00, PLLP_OUT0 */
>>> +
>>> +     /* wait for 2us */
>>> +     udelay(2);
>>> +
>>> +     /* De-assert reset to UART */
>>> +     reset_set_enable(pid, 0);
>>> +}
>>
>> That doesn't seem like anything to do with function muxing.
> 
> My thoughts initially were that funcmux_enable_f() might support other
> devices. Really the correct thing to do is call
> clock_start_periph_pll() but this can't be used prior to relocation.
> We are trying to provide a simple function to enable a UART which is
> why I thought funcmux might be a good place (since its aim is to
> enable things to make functions work).
> 
> One option is to move this function into clock.c and call it
> clock_ll_start_periph_pll() perhaps:
> 
> void clock_ll_start_periph_pll(enum periph_id periph_id, int source)
> 
> where source is a plain number (in this case always 0). But it's a bit
> uglier in that callers must pass 0, otherwise the UART won't work. So
> I am leaning towards something like:
> 
> void clock_ll_start_uart(enum periph_id periph_id)

Yes, clock.c and that prototype seem reasonable.

...
>>> +{
>>> +     switch (id) {
>>> +     case PERIPH_ID_UART1:
>>> +             pinmux_set_func(PINGRP_IRRX, PMUX_FUNC_UARTA);
>>> +             pinmux_set_func(PINGRP_IRTX, PMUX_FUNC_UARTA);
>>> +             pinmux_tristate_disable(PINGRP_IRRX);
>>> +             pinmux_tristate_disable(PINGRP_IRTX);
>>> +             break;
>>> +
>>> +     case PERIPH_ID_UART2:
>>> +             pinmux_set_func(PINGRP_UAD, PMUX_FUNC_IRDA);
>>> +             pinmux_tristate_disable(PINGRP_UAD);
>>> +             break;
>>> +
>>> +     case PERIPH_ID_UART4:
>>> +             pinmux_set_func(PINGRP_GMC, PMUX_FUNC_UARTD);
>>> +             pinmux_tristate_disable(PINGRP_GMC);
>>> +             break;
>>> +
>>> +     default:
>>> +             debug("%s: invalid periph_id %d", __func__, id);
>>> +             break;
>>> +     }
>>> +}
>>
>> I'm not entirely convinced that centralizing this in a function rather
>> than putting the board-specific muxing into the per-board files is the
>> right way to go. What's wrong with the kernel's approach of a single
>> table describing each board's complete pinmux settings? Eventually, all
>> this will come from DT anyway, won't it?
> 
> I agree what you seem to be implying (that all Tegra boards will use DT).
> 
> I would like to reduce code duplication. Yes, when the DT supports
> pinmux we can call a function to set it up. But even then the code
> will not be in the board files - only the function call to some sort
> of pinmux library will be there.
> 
> At present we have code like this in the boards:
> 
> 	/* SDMMC4: config 3, x8 on 2nd set of pins */
> 	pinmux_set_func(PINGRP_ATB, PMUX_FUNC_SDIO4);
> 	pinmux_set_func(PINGRP_GMA, PMUX_FUNC_SDIO4);
> 	pinmux_set_func(PINGRP_GME, PMUX_FUNC_SDIO4);
> 
> 	pinmux_tristate_disable(PINGRP_ATB);
> 	pinmux_tristate_disable(PINGRP_GMA);
> 	pinmux_tristate_disable(PINGRP_GME);
> 
> which I want to move into funcmux. The only parameters are the
> peripheral ID and the function option. When we have pinmux in the DT
> then these functions can look up the DT to get their info.

The values for "func" can't come from DT; it's a U-Boot specific SW
concept, not a HW concept. I expect DT will have a table (possibly per
device or for the whole board; not really clear yet) that for each pin
group lists function, tristate, pullup/down, ... parameters directly; no
indirection through a "selected configuration"

So yes you're right these functions should be elided once we switch to
device tree. I guess this is OK to remove some cut/paste code. I just
don't want to see the "func" value definitions become some kind of
long-term concept.

> Also with U-Boot we don't necessarily want to enable everything at
> start of day in U-Boot. So it may still be required that we init
> (e.g.) the MMC only when an mmc command is executed.

If the DT pinmux bindings end up being per device, then yes this is
possible. But really, writing the whole pinmux setup is not many
register writes, and strictly within a bus within the SoC, so probably
isn't worth losing any sleep over skipping those that aren't strictly
necessary for U-boot to operate minimally.

> Obviously this needs to be in review. As things change with the kernel
> we can react. But in any case we want the affected code to be in once
> place, hence funcmux.

-- 
nvpublic

[U-Boot] [PATCH 4/6] tegra: Add support for UART init in cpu board.c

[Simon Glass]

Hi Mike,

On Wed, Nov 23, 2011 at 3:40 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Wednesday 23 November 2011 17:59:06 Simon Glass wrote:
>> --- a/arch/arm/cpu/armv7/tegra2/board.c
>> +++ b/arch/arm/cpu/armv7/tegra2/board.c
>>
>> +static void setup_uarts(int uart_ids)
>> +{
>> + ? ? static enum periph_id id_for_uart[UART_COUNT] = {
>> + ? ? ? ? ? ? PERIPH_ID_UART1,
>> + ? ? ? ? ? ? PERIPH_ID_UART2,
>> + ? ? ? ? ? ? PERIPH_ID_UART3,
>> + ? ? ? ? ? ? PERIPH_ID_UART4,
>> + ? ? };
>
> does this need the [UART_COUNT] ? ?what if you used "[]" ...

No, it is purely trying to provoke a compiler warning if it is wrong.
But that won't work if UART_COUNT is larger anyway. I'll remove it.

>
>> + ? ? int i;
>
> size_t

Ick. OK.

>
>> + ? ? for (i = 0; i < UART_COUNT; i++) {
>
> ... and then did ARRAY_SIZE(id_for_uart) ?
>
>> +void board_init_uart_f(void)
>> +{
>> + ? ? int uart_ids = 0; ? ? ? /* bit mask of which UART ids to enable */
>> +
>> +#ifdef CONFIG_TEGRA2_ENABLE_UARTA
>> + ? ? uart_ids |= UARTA;
>> +#endif
>> +#ifdef CONFIG_TEGRA2_ENABLE_UARTB
>> + ? ? uart_ids |= UARTB;
>> +#endif
>> +#ifdef CONFIG_TEGRA2_ENABLE_UARTD
>> + ? ? uart_ids |= UARTD;
>> +#endif
>> + ? ? setup_uarts(uart_ids);
>> +}
>
> we added a func in the Blackfin pinmux API where you could pass it a list of
> peripherals to mux. ?so you could do:
> ? ? ? ?static const unsigned pin_list[] = {
> ? ? ? ?#ifdef CONFIG_TEGRA2_ENABLE_UARTA
> ? ? ? ? ? ? ? ?UART1,
> ? ? ? ?#endif
> ? ? ? ? ? ? ? ?0,
> ? ? ? ?};
> ? ? ? ?pinmux_request_list(pin_list);
>
> and the list version would just walk the 0-terminated array calling
> pinmux_request() automatically
> -mike
>

Well yes, but funcmux isn't about pins, it's about whole functions. We
could provide a new function to enable a list of functions, but for
now there really wouldn't be any callers. UART1 is just our own number
- the callee requires a peripheral ID.

Regards,
Simon

[U-Boot] [PATCH 3/6] tegra: Add a function mux feature

[Simon Glass]

Hi Stephen,

On Mon, Nov 28, 2011 at 11:42 AM, Stephen Warren <swarren@nvidia.com> wrote:
> On 11/28/2011 12:19 PM, Simon Glass wrote:
>> Hi Stephen,
>>
>> On Mon, Nov 28, 2011 at 10:17 AM, Stephen Warren <swarren@nvidia.com> wrote:
>>> On 11/23/2011 03:59 PM, Simon Glass wrote:
>>>> funcmux permits selection of config options for particular peripherals,
>>>> such as the pins that are used for that peripheral, if there are several
>>>> options.
>>
>> Thanks for looking at this.
>>
>>>>
>>>> Add UART selection to start with.
>>>
>>>> +static void enable_uart(enum periph_id pid)
>>>> +{
>>>> + ? ? /* Assert UART reset and enable clock */
>>>> + ? ? reset_set_enable(pid, 1);
>>>> + ? ? clock_enable(pid);
>>>> + ? ? clock_ll_set_source(pid, 0); ? ?/* UARTx_CLK_SRC = 00, PLLP_OUT0 */
>>>> +
>>>> + ? ? /* wait for 2us */
>>>> + ? ? udelay(2);
>>>> +
>>>> + ? ? /* De-assert reset to UART */
>>>> + ? ? reset_set_enable(pid, 0);
>>>> +}
>>>
>>> That doesn't seem like anything to do with function muxing.
>>
>> My thoughts initially were that funcmux_enable_f() might support other
>> devices. Really the correct thing to do is call
>> clock_start_periph_pll() but this can't be used prior to relocation.
>> We are trying to provide a simple function to enable a UART which is
>> why I thought funcmux might be a good place (since its aim is to
>> enable things to make functions work).
>>
>> One option is to move this function into clock.c and call it
>> clock_ll_start_periph_pll() perhaps:
>>
>> void clock_ll_start_periph_pll(enum periph_id periph_id, int source)
>>
>> where source is a plain number (in this case always 0). But it's a bit
>> uglier in that callers must pass 0, otherwise the UART won't work. So
>> I am leaning towards something like:
>>
>> void clock_ll_start_uart(enum periph_id periph_id)
>
> Yes, clock.c and that prototype seem reasonable.

OK, I will update it, I agree it makes more sense particularly as UART
pre-reloc is a special case.

>
> ...
>>>> +{
>>>> + ? ? switch (id) {
>>>> + ? ? case PERIPH_ID_UART1:
>>>> + ? ? ? ? ? ? pinmux_set_func(PINGRP_IRRX, PMUX_FUNC_UARTA);
>>>> + ? ? ? ? ? ? pinmux_set_func(PINGRP_IRTX, PMUX_FUNC_UARTA);
>>>> + ? ? ? ? ? ? pinmux_tristate_disable(PINGRP_IRRX);
>>>> + ? ? ? ? ? ? pinmux_tristate_disable(PINGRP_IRTX);
>>>> + ? ? ? ? ? ? break;
>>>> +
>>>> + ? ? case PERIPH_ID_UART2:
>>>> + ? ? ? ? ? ? pinmux_set_func(PINGRP_UAD, PMUX_FUNC_IRDA);
>>>> + ? ? ? ? ? ? pinmux_tristate_disable(PINGRP_UAD);
>>>> + ? ? ? ? ? ? break;
>>>> +
>>>> + ? ? case PERIPH_ID_UART4:
>>>> + ? ? ? ? ? ? pinmux_set_func(PINGRP_GMC, PMUX_FUNC_UARTD);
>>>> + ? ? ? ? ? ? pinmux_tristate_disable(PINGRP_GMC);
>>>> + ? ? ? ? ? ? break;
>>>> +
>>>> + ? ? default:
>>>> + ? ? ? ? ? ? debug("%s: invalid periph_id %d", __func__, id);
>>>> + ? ? ? ? ? ? break;
>>>> + ? ? }
>>>> +}
>>>
>>> I'm not entirely convinced that centralizing this in a function rather
>>> than putting the board-specific muxing into the per-board files is the
>>> right way to go. What's wrong with the kernel's approach of a single
>>> table describing each board's complete pinmux settings? Eventually, all
>>> this will come from DT anyway, won't it?
>>
>> I agree what you seem to be implying (that all Tegra boards will use DT).
>>
>> I would like to reduce code duplication. Yes, when the DT supports
>> pinmux we can call a function to set it up. But even then the code
>> will not be in the board files - only the function call to some sort
>> of pinmux library will be there.
>>
>> At present we have code like this in the boards:
>>
>> ? ? ? /* SDMMC4: config 3, x8 on 2nd set of pins */
>> ? ? ? pinmux_set_func(PINGRP_ATB, PMUX_FUNC_SDIO4);
>> ? ? ? pinmux_set_func(PINGRP_GMA, PMUX_FUNC_SDIO4);
>> ? ? ? pinmux_set_func(PINGRP_GME, PMUX_FUNC_SDIO4);
>>
>> ? ? ? pinmux_tristate_disable(PINGRP_ATB);
>> ? ? ? pinmux_tristate_disable(PINGRP_GMA);
>> ? ? ? pinmux_tristate_disable(PINGRP_GME);
>>
>> which I want to move into funcmux. The only parameters are the
>> peripheral ID and the function option. When we have pinmux in the DT
>> then these functions can look up the DT to get their info.
>
> The values for "func" can't come from DT; it's a U-Boot specific SW
> concept, not a HW concept. I expect DT will have a table (possibly per
> device or for the whole board; not really clear yet) that for each pin
> group lists function, tristate, pullup/down, ... parameters directly; no
> indirection through a "selected configuration"

Since the DT pinmux operates with individual pins or pin groups, it
means that a function like MMC needs to adjust several pinmux
settings. This means that several DT properties or nodes need to
change depending on the setting that the board uses.

I really don't want to get into this discussion as it is too early.
Let's see how things turn out in the kernel.

>
> So yes you're right these functions should be elided once we switch to
> device tree. I guess this is OK to remove some cut/paste code. I just
> don't want to see the "func" value definitions become some kind of
> long-term concept.

Well arguably it has value (board vendors just select from a number of
SOC-provided options for each function), but I would rather just
follow what the kernel does as you say.

>
>> Also with U-Boot we don't necessarily want to enable everything at
>> start of day in U-Boot. So it may still be required that we init
>> (e.g.) the MMC only when an mmc command is executed.
>
> If the DT pinmux bindings end up being per device, then yes this is
> possible. But really, writing the whole pinmux setup is not many
> register writes, and strictly within a bus within the SoC, so probably
> isn't worth losing any sleep over skipping those that aren't strictly
> necessary for U-boot to operate minimally.

This doesn't quite fit with U-Boot philosophy (init what you need) but
I agree we might get away with it.

>
>> Obviously this needs to be in review. As things change with the kernel
>> we can react. But in any case we want the affected code to be in once
>> place, hence funcmux.
>
> --
> nvpublic
>

Regards,
Simon

[U-Boot] [PATCH 4/6] tegra: Add support for UART init in cpu board.c

[Mike Frysinger]

On Monday 28 November 2011 15:14:40 Simon Glass wrote:
> On Wed, Nov 23, 2011 at 3:40 PM, Mike Frysinger wrote:
> > On Wednesday 23 November 2011 17:59:06 Simon Glass wrote:
> >> +     int i;
> > 
> > size_t
> 
> Ick. OK.

why ick ? :)

> >> +     for (i = 0; i < UART_COUNT; i++) {
> > 
> > ... and then did ARRAY_SIZE(id_for_uart) ?
> > 
> >> +void board_init_uart_f(void)
> >> +{
> >> +     int uart_ids = 0;       /* bit mask of which UART ids to enable */
> >> +
> >> +#ifdef CONFIG_TEGRA2_ENABLE_UARTA
> >> +     uart_ids |= UARTA;
> >> +#endif
> >> +#ifdef CONFIG_TEGRA2_ENABLE_UARTB
> >> +     uart_ids |= UARTB;
> >> +#endif
> >> +#ifdef CONFIG_TEGRA2_ENABLE_UARTD
> >> +     uart_ids |= UARTD;
> >> +#endif
> >> +     setup_uarts(uart_ids);
> >> +}
> > 
> > we added a func in the Blackfin pinmux API where you could pass it a list
> > of peripherals to mux.  so you could do:
> >        static const unsigned pin_list[] = {
> >        #ifdef CONFIG_TEGRA2_ENABLE_UARTA
> >                UART1,
> >        #endif
> >                0,
> >        };
> >        pinmux_request_list(pin_list);
> > 
> > and the list version would just walk the 0-terminated array calling
> > pinmux_request() automatically
> 
> Well yes, but funcmux isn't about pins, it's about whole functions. We
> could provide a new function to enable a list of functions, but for
> now there really wouldn't be any callers. UART1 is just our own number
> - the callee requires a peripheral ID.

the Blackfin API is given an ID that encodes both the requested function and 
the pin.  i meant doing that instead of bit packing and unpacking ids that 
later get expanded.

although with the pinmux framework getting merged into Linux, this is probably 
now a moot point.  stick with whatever you have working as it'll all get 
thrown out once we migrate to that in u-boot :).
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20111128/4aeeb14f/attachment.pgp>

[U-Boot] [PATCH 4/6] tegra: Add support for UART init in cpu board.c

[Simon Glass]

Hi Mike,

On Mon, Nov 28, 2011 at 3:06 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Monday 28 November 2011 15:14:40 Simon Glass wrote:
>> On Wed, Nov 23, 2011 at 3:40 PM, Mike Frysinger wrote:
>> > On Wednesday 23 November 2011 17:59:06 Simon Glass wrote:
>> >> + ? ? int i;
>> >
>> > size_t
>>
>> Ick. OK.
>
> why ick ? :)

I hardly ever get to write 'int' these days...

>
>> >> + ? ? for (i = 0; i < UART_COUNT; i++) {
>> >
>> > ... and then did ARRAY_SIZE(id_for_uart) ?
>> >
>> >> +void board_init_uart_f(void)
>> >> +{
>> >> + ? ? int uart_ids = 0; ? ? ? /* bit mask of which UART ids to enable */
>> >> +
>> >> +#ifdef CONFIG_TEGRA2_ENABLE_UARTA
>> >> + ? ? uart_ids |= UARTA;
>> >> +#endif
>> >> +#ifdef CONFIG_TEGRA2_ENABLE_UARTB
>> >> + ? ? uart_ids |= UARTB;
>> >> +#endif
>> >> +#ifdef CONFIG_TEGRA2_ENABLE_UARTD
>> >> + ? ? uart_ids |= UARTD;
>> >> +#endif
>> >> + ? ? setup_uarts(uart_ids);
>> >> +}
>> >
>> > we added a func in the Blackfin pinmux API where you could pass it a list
>> > of peripherals to mux. ?so you could do:
>> > ? ? ? ?static const unsigned pin_list[] = {
>> > ? ? ? ?#ifdef CONFIG_TEGRA2_ENABLE_UARTA
>> > ? ? ? ? ? ? ? ?UART1,
>> > ? ? ? ?#endif
>> > ? ? ? ? ? ? ? ?0,
>> > ? ? ? ?};
>> > ? ? ? ?pinmux_request_list(pin_list);
>> >
>> > and the list version would just walk the 0-terminated array calling
>> > pinmux_request() automatically
>>
>> Well yes, but funcmux isn't about pins, it's about whole functions. We
>> could provide a new function to enable a list of functions, but for
>> now there really wouldn't be any callers. UART1 is just our own number
>> - the callee requires a peripheral ID.
>
> the Blackfin API is given an ID that encodes both the requested function and
> the pin. ?i meant doing that instead of bit packing and unpacking ids that
> later get expanded.
>

OK I see

> although with the pinmux framework getting merged into Linux, this is probably
> now a moot point. ?stick with whatever you have working as it'll all get
> thrown out once we migrate to that in u-boot :).
> -mike
>

Yes I really don't want to design anything until everything lands, so
will stick with this for now as you say.

Regards,
Simon