[U-Boot] [PATCH 0/7] tegra: Enhance funcmux to support I2C and MMC

[Simon Glass]

Hi Stephen,

On Mon, Jan 9, 2012 at 3:46 PM, Stephen Warren <swarren at nvidia.com> wrote:
> On 01/09/2012 04:36 PM, Simon Glass wrote:
>> Hi Stephen,
>>
>> On Mon, Jan 9, 2012 at 3:11 PM, Stephen Warren <swarren at nvidia.com> wrote:
>>> On 01/09/2012 03:53 PM, Simon Glass wrote:
>>>> This series expands funcmux_select() to support configs other than 0, and
>>>> to support options associated with a config.
>>>>
>>>> This permits introduction of I2C support using multiple config options.
>>>>
>>>> The options parameter is used by MMC to select standard (4-bit) or 8-bit
>>>> operation.
>>>
>>> The unification in this series basically seems fine.
>>>
>>> Why not consider bus width part of the "config" though, rather the
>>> complicating things with an extra parameter? As an example, for SDMMC4,
>>> you'd have say:
>>>
>>> 0: ATC + ATD 8 bit
>>> 1: ATB + GMA 4 bit
>>> 2: ATB + GMA + GME 8 bit
>>>
>>> ... and no option values.
>>
>> I am thinking ahead a little to where we have more peripherals with
>> several options. If we imagine a situation where the SOC has 3
>> different pin configs each of which can be 1-bit, 4-bit or 8-bit, then
>> it is nice to have the options broken out separately.
>
> On Tegra20, the pin mux is controlled in groups, so you're mostly
> picking which groups to use for the function, which then determines the
> bus width. In other words, its often unlikely that you can pick bus
> width as an independent option from the set of pins/groups used.

I wasn't really saying it was independent, just that some configs
offer different variations and it would be nice to make this explicit
rather than flattening the config + options information into a single
value. For now the benefit is marginal so I will remove it,
particularly as you say it will be no use on Tegra30:

>
> On Tegra30, the situation is about the same except that the mux function
> is picked on a per-pin basis instead of in groups of pins, which takes
> the same argument even further; for a 4-bit bus you'd simply remove 4
> pins from the list of pins being used, so it doesn't make sense to refer
> to 4-bit as an option of an 8-bit setup with some pins unused, because
> the unused pins are set to some other mux function.

IMO it does make sense for the user - remember I am trying to simplify
pinmux use for boards. Exposing all the different pins is almost
exactly what I am trying to avoid :-)

>
>> Also, we can also use the options for something else, like Tegra 3's
>> drive strength and slew rate control (and perhaps other things I
>> understand even less).
>
> There are far far too many options for that to be represented by a
> single U32, or even a small number of them. When boards start needing to
> set up drive strengths etc., we'll probably need individual API calls
> for each config option, since each board's characterization will trigger
> a combination of options that's extremely likely to be unique.

My reading of the Tegra30 registers suggested this was possible, but
it seems I assumed too much!

>
>>> Also, we should probably define names for the config values, at least in
>>> the cases where 0 isn't the only option. Hard-coding 0 or 1 at the call
>>> sites isn't very meaningful.
>>
>> I can certainly do that - it was in the back of my mind. But the only
>> thing I could think of was to create an enum with the pingroup
>> assignments, like:
>>
>> enum {
>>     UART1_IRRX_IRTX   = 0,
>>     UART2_UAD             = 0,
>> ...
>> };
>>
>> Seems a bit ugly?
>
> I don't agree. The options are just completely arbitrary IDs for a
> particular set of pins/groups that are being used. Well, arbitrary
> within the set of possibilities given the wiring of Tegra's pinmux HW
> anyway. Hence, giving those IDs names based on which pins/groups are
> being used makes sense to me.

OK ugly long enums are on their way :-) - will send a new series.
Thanks again for the helpful review.

Regards,
Simon

>
> --
> nvpublic