Hi Stephen,
On Mon, Nov 28, 2011 at 11:42 AM, Stephen Warren <swar...@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 <swar...@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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
