[U-Boot] [PATCH v2] tegra: add Colibri T20 board support

[Stephen Warren <swarren@wwwdotorg.org>]

On 10/01/2012 10:48 AM, Lucas Stach wrote:
> Am Montag, den 01.10.2012, 10:33 -0600 schrieb Stephen Warren:
>> On 09/29/2012 02:03 PM, Lucas Stach wrote:
>>> This adds board support for the Toradex Colibri T20 module.
>>>
>>> Working functions:
>>> - SD card boot
>>> - USB boot
>>> - Network
>>> - NAND environment
>>
>>> diff --git a/board/toradex/colibri_t20/Makefile b/board/toradex/colibri_t20/Makefile
>> ...
>>> +#########################################################################
>>> \ No newline at end of file
>>
>> I assume that's a mistake.
>>
> Another one...
> 
>>> diff --git a/board/toradex/colibri_t20/colibri_t20.c b/board/toradex/colibri_t20/colibri_t20.c
>>
>>> +#ifdef CONFIG_USB_EHCI_TEGRA
>>> +void pin_mux_usb(void)
>>> +{
>>> +	/* USB 1 aka Tegra USB port 3 */
>>> +	pinmux_tristate_disable(PINGRP_SPIG);
>>
>> I don't think that's muxing USB itself, but rather muxing perhaps the
>> VBUS GPIO?
>>
> That's right. I'll do a comment to make this more obvious.
> 
>>> +#ifdef CONFIG_TEGRA_MMC
>>> +int board_mmc_init(bd_t *bd)
>>> +{
>>> +	funcmux_select(PERIPH_ID_SDMMC4, FUNCMUX_SDMMC4_ATB_GMA_4_BIT);
>>> +	pinmux_tristate_disable(PINGRP_GMB);
>>
>> It might be useful to comment the tristate call like other boards, e.g.:
>>
>>         /* For power GPIO PI6 */
>>         pinmux_tristate_disable(PINGRP_ATA);
>>
>> so it's obvious why the call isn't done inside funcmux_select().
>>
>>> diff --git a/board/toradex/dts/tegra20-colibri_t20.dts b/board/toradex/dts/tegra20-colibri_t20.dts
>>
>>> +	usb at c5008000 {
>>> +		nvidia,vbus-gpio = <&gpio 178 1>; /* PW2 low-active */
>>> +	};
>>
>> As an FYI, although the GPIO bindings do specify that the last cell
>> there is for GPIO flags, I'm not sure that we should rely on it. Not all
>> GPIO bindings actually have the ability to specify flags there, so if a
>> given board's GPIO is provided by some device whose GPIO binding doesn't
>> allow for flags, then it won't be possible to specify an active-low
>> GPIO, and this won't work.
>>
>> The kernel certainly doesn't actually do anything with the flags
>> argument in the EHCI driver, IIRC.
>>
>> Either we should forcibly change all GPIO bindings in the kernel to
>> require that they allow flags to be specified (probably very hard), or
>> remove the flags from the Tegra GPIO binding, and use a separate
>> property such as nvidia,vbus-gpio-active-low for this purpose.
>> Certainly, the latter form of approach has been taken in other places
>> (such as fixed regulator IIRC).
>
> You mentioned that your plan is to bring over the regulator thing from
> the kernel to u-boot. So IMHO the correct approach would be to just use
> a fixed regulator for VBUS, where we already have the active-low
> property in the binding.

Yes, that's true.

> So can we just let this sit as it is now, and agree to remove the GPIO
> active-low flag from the Tegra GPIO binding and every use of it as soon
> as we have proper regulators in u-boot?

Hmmm. I guess. I wonder how likely it is that anyone is going to get
around to cleaning up U-Boot's DT usage to match the kernel's though. At
least I can file a bug for this once the kernel is update though, so
hopefully it will happen. I do wish that U-Boot would start using
generally agreed upon bindings rather than going its own way first and
then having to be cleaned up though, although admittedly that comment
doesn't directly apply to this case.