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