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@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
