[U-Boot] [PATCH 55/60] i2c: tegra: move pinmux setup to board files

[Simon Glass]

Hi Stephen,

On 27 April 2016 at 11:16, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 04/27/2016 10:58 AM, Simon Glass wrote:
>>
>> Hi Stephen,
>>
>> On 27 April 2016 at 10:24, Stephen Warren <swarren at wwwdotorg.org> wrote:
>>>
>>> On 04/27/2016 09:12 AM, Simon Glass wrote:
>>>>
>>>>
>>>> Hi Stephen,
>>>>
>>>> On 19 April 2016 at 14:59, Stephen Warren <swarren at wwwdotorg.org> wrote:
>>>>>
>>>>>
>>>>> From: Stephen Warren <swarren at nvidia.com>
>>>>>
>>>>> Remove funcmux calls from the Tegra I2C driver. Knowledge of pinmux
>>>>> setup must come from either board files or DT; it should not be
>>>>> embedded
>>>>> into board-agnostic driver code. The DT pinmux bindings do not allow
>>>>> drivers to derive funcmux-style information, since the DT bindings are
>>>>> pin-based whereas funcmux is controller-based, so there's no good way
>>>>> to
>>>>> call the existing funcmux APIs from drivers. Converting drivers to use
>>>>> a
>>>>> new (as yet non-existent in U-Boot) API that pulls pinmux information
>>>>> from
>>>>> DT isn't useful for Tegra, since Tegra's DT files don't contain any
>>>>> per-device pinmux tables, so this would simply be extra code that has
>>>>> no
>>>>> effect; doesn't actually set up the pinmux. We are left with moving the
>>>>> pinmux setup functionality into board files. In theory the board files
>>>>> could be converted later to use DT, but that would be a separate
>>>>> change.
>>>>>
>>>>> Signed-off-by: Stephen Warren <swarren at nvidia.com>
>>>>> ---
>>>>>    board/avionic-design/common/tamonten.c  |  5 +++++
>>>>>    board/nvidia/seaboard/seaboard.c        |  3 +++
>>>>>    board/nvidia/whistler/whistler.c        |  1 +
>>>>>    board/toradex/colibri_t20/colibri_t20.c |  3 +++
>>>>>    drivers/i2c/tegra_i2c.c                 | 19 -------------------
>>>>>    5 files changed, 12 insertions(+), 19 deletions(-)
>>>>
>>>>
>>>>
>>>> This should use driver model, which handles pinmux automatically if
>>>> you have a pinctrl driver.
>>>
>>>
>>>
>>> Can you define "automatic"? I don't understand exactly which benefit
>>> you're
>>> describing there.
>>
>>
>> When you probe a device, its pinmux is set up automatically, so the
>> explicit funcmux calls can go away.
>
>
> So the device here would be the pin controller device itself, not individual
> I2C/SPI/SD/... devices.
>
> That's because the Tegra HW does not support[1] dynamic or partial pinmux
> configuration. As such, we need to program the entire pinmux at once early
> during boot, not piece-by-piece as the individual U-Boot devices that use
> individual pins are probed. This is why for example the kernel DT contains a
> single pinmux table that the pin controller driver sets up at boot (with
> identical configuration to what U-Boot sets up, so it's a no-op), rather
> than splitting it into per-device chunks.

Is this the big table that I see in device tree files?

pinmux at 0,70000868 {
pinctrl-names = "default";
pinctrl-0 = <&pinmux_default>;

pinmux_default: common {
clk_32k_out_pa0 {
nvidia,pins = "clk_32k_out_pa0";
nvidia,pull = <TEGRA_PIN_PULL_NONE>;
nvidia,tristate = <TEGRA_PIN_DISABLE>;
nvidia,enable-input = <TEGRA_PIN_ENABLE>;
};
...

>
> As such, there isn't any need for, say, an I2C device probe to call into
> pinmux at all; the pinmux would already have been set up entirely during
> early boot, and hence no I2C-specific actions would be needed later.

OK. As you know I don't like this monolithic pinmux stuff - at least
in early boot it is useful to enable (say) just the UART. Anyway I
still think a driver is useful and will provide a better base for the
future.

>
> So I'm not sure what benefit conversion to DM pinctrl has here. Sure it
> means things get set up the same way as with any other pinctrl device or
> SoC, but this is early SoC-specific configuration, without any interaction
> with common or driver code besides being implemented via some standard
> core->board callbacks/hooks. It seems reasonable to just program the pinmux
> directly using SoC-specific APIs rather than having to add a layer of
> abstraction on top of it just so we can route it through DM. In other words,
> what's already done by this patch series.
>
> Besides, I believe the programming happens before a DM pinctrl device would
> be ready, doesn't it, given it happens from tegra_board_early_init_f()? Or,
> would we be able to fully probe a DM device at that point? The UART console
> setup is even earlier, in SPL pre-T210, where I don't think we even have DM
> enabled.

DM is enabled pretty early, before board_early_init_f(). But if you
hit a problem we can look at it. There is no point in setting up
pinmux before driver model, since it is the drivers that use it.

Worst case, how about creating a pinctrl driver where everything
happens in the probe() method? Ideally you could do the minimum
necessary before relocation, and then the full table load afterwards.

>
> [1] Yes, the HW registers can in practice be programmed bit-by-bit, simply
> because there are a number of registers and the SoC doesn't have a way to
> physically force SW to write to each of those registers. "Support" here
> refers to what the ASIC team will guarantee will work correctly without
> causing glitches or similar issues. There are a few limited exceptions, e.g.
> console UART muxing on its own has been at least partially thought out
> (although there are still conflicts in some cases on older chips), and IO
> controllers that may contain boot media are generally OK to mux on their
> own. However, for anything else, i.e. the majority of cases, the supported
> model is to program everything up-front in one go, and not change it later.
> Sticking to that general model in absolutely all cases removes special cases
> and simplifies the code.

Every SoC has its limitations in terms of pinmux. The point here is to
have a sensible API on top of this stuff. At least then people know
where to find the code, and all the pinmux hacking is in one place.
Yes Tegra has some special features, but Linux has a pinctrl driver
for Tegra. Can't U-Boot?

Regards,
Simon