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

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