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