Introduce function mux library

Quoted 11 lines · On Wednesday 23 November 2011 17:59:03 Simon Glass wrote:

arch/arm/cpu/armv7/tegra2/board.c +0 -0

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

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

Signed-off-by Simon Glass <[email protected]>

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

Signed-off-by Simon Glass <[email protected]>

arch/arm/cpu/armv7/tegra2/funcmux.c new file +15 -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;
+

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.

arch/arm/cpu/armv7/tegra2/funcmux.c new file +3 -0

+static void enable_uart(enum periph_id pid)
+{
+	/* Assert UART reset and enable clock */

+void funcmux_select(enum periph_id id, int func)

arch/arm/cpu/armv7/tegra2/funcmux.c new file +14 -0

+{
+	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;
+

Quoted 3 lines · 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.

Quoted 6 lines

Add UART selection to start with.

(quoted diff) +3 -0

+static void enable_uart(enum periph_id pid)
+{
+ ? ? /* Assert UART reset and enable clock */

Review

Quoted 10 lines

(quoted diff) +10 -0

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

Quoted 2 lines

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

Quoted 23 lines

(quoted diff) +22 -0

+{
+ ? ? 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;

Quoted 2 lines

+ ? ? }
+}

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

Hi Stephen,

Quoted 4 lines · Stephen Warren

Quoted 3 lines · 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.

Thanks for looking at this.

Quoted 18 lines

Quoted 6 lines

Add UART selection to start with.

arch/arm/cpu/armv7/tegra2/funcmux.c new file +3 -0

+static void enable_uart(enum periph_id pid)
+{
+     /* Assert UART reset and enable clock */

arch/arm/cpu/armv7/tegra2/funcmux.c line 39

Quoted 10 lines

arch/arm/cpu/armv7/tegra2/funcmux.c new file +10 -0

+     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)

Quoted 22 lines

Quoted 22 lines

arch/arm/cpu/armv7/tegra2/funcmux.c new file +22 -0

+{
+     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;

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+     }
+}

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.

Quoted 41 lines · Simon Glass

Hi Stephen,

Quoted 4 lines · Stephen Warren

Quoted 3 lines · 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.

Thanks for looking at this.

Quoted 18 lines

Quoted 6 lines

Add UART selection to start with.

(quoted diff) +3 -0

+static void enable_uart(enum periph_id pid)
+{
+ ? ? /* Assert UART reset and enable clock */

Review

Quoted 10 lines

(quoted diff) +10 -0

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

Quoted 5 lines

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.

...

Quoted 45 lines

Quoted 22 lines

Quoted 22 lines

(quoted diff) +22 -0

+{
+ ? ? 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;

Quoted 8 lines

Quoted 2 lines

+ ? ? }
+}

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);

Quoted 7 lines

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

Quoted 4 lines

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.

Quoted 4 lines

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

arch/arm/cpu/armv7/tegra2/board.c +5 -0

+void board_init_uart_f(void)
+{
+	int uart_ids = 0;	/* bit mask of which UART ids to enable */
+
+#ifdef CONFIG_TEGRA2_ENABLE_UARTA

Quoted 3 lines · On Wednesday 23 November 2011 17:59:06 Simon Glass wrote:

arch/arm/cpu/armv7/tegra2/board.c +0 -0

arch/arm/cpu/armv7/tegra2/board.c

Quoted 8 lines

arch/arm/cpu/armv7/tegra2/board.c +8 -0

+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 "[]" ...

Quoted 2 lines

+ ? ? int i;

size_t

Quoted 2 lines

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

... and then did ARRAY_SIZE(id_for_uart) ?

Quoted 5 lines

arch/arm/cpu/armv7/tegra2/board.c +5 -0

+void board_init_uart_f(void)
+{
+ ? ? int uart_ids = 0; ? ? ? /* bit mask of which UART ids to enable */
+
+#ifdef CONFIG_TEGRA2_ENABLE_UARTA

arch/arm/cpu/armv7/tegra2/board.c int arch_cpu_init(void) · line 142

Quoted 10 lines

arch/arm/cpu/armv7/tegra2/board.c +10 -0

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

Quoted 4 lines · Mike Frysinger

Quoted 1 line · On Wednesday 23 November 2011 17:59:06 Simon Glass wrote:

+ int i;

size_t

Ick. OK.

Quoted 30 lines

Quoted 1 line

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

... and then did ARRAY_SIZE(id_for_uart) ?

Quoted 5 lines

arch/arm/cpu/armv7/tegra2/board.c +5 -0

+void board_init_uart_f(void)
+{
+     int uart_ids = 0;       /* bit mask of which UART ids to enable */
+
+#ifdef CONFIG_TEGRA2_ENABLE_UARTA

arch/arm/cpu/armv7/tegra2/board.c int arch_cpu_init(void) · line 142

Quoted 10 lines

arch/arm/cpu/armv7/tegra2/board.c +10 -0

+     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);

Quoted 2 lines

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.

Quoted 7 lines · On Monday 28 November 2011 15:14:40 Simon Glass wrote:

Quoted 4 lines · Mike Frysinger

Quoted 1 line · On Wednesday 23 November 2011 17:59:06 Simon Glass wrote:

+ ? ? int i;

size_t

Ick. OK.

why ick ? :)

Quoted 34 lines

Quoted 33 lines

Quoted 2 lines

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

... and then did ARRAY_SIZE(id_for_uart) ?

Quoted 5 lines

arch/arm/cpu/armv7/tegra2/board.c +5 -0

+void board_init_uart_f(void)
+{
+ ? ? int uart_ids = 0; ? ? ? /* bit mask of which UART ids to enable */
+
+#ifdef CONFIG_TEGRA2_ENABLE_UARTA

arch/arm/cpu/armv7/tegra2/board.c int arch_cpu_init(void) · line 142

Quoted 10 lines

arch/arm/cpu/armv7/tegra2/board.c +10 -0

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

Quoted 4 lines

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

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

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

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