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

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