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

[Lucas Stach]

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.

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?

Thanks for the review,
Lucas