[U-Boot] [PATCH 1/2] Tegra: fdt: Add/enhance sdhci (mmc) nodes for all T20 DT files

[Stephen Warren <swarren@wwwdotorg.org>]

On 02/04/2013 04:48 PM, Tom Warren wrote:
> Linux dts files were used for those boards that didn't already
> have sdhci info populated. If a cd-gpio was present, I set
> "removable = <1>", else removable = <0>.

> diff --git a/arch/arm/dts/tegra20.dtsi b/arch/arm/dts/tegra20.dtsi

>  	sdhci at c8000200 {
>  		compatible = "nvidia,tegra20-sdhci";
>  		reg = <0xc8000200 0x200>;
>  		interrupts = < 47 >;
> +		status = "disabled";
> +		/* PERIPH_ID_SDMMC2, PLLP_OUT? */
> +		clocks = <&tegra_car 9>;
>  	};

What does "PLLP_OUT?" mean?

I'm not entirely convinced it's a good idea to add this comment, since
it creates a diff between the .dts files in the kernel and U-Boot.

Similarly, the status and clocks properties are in the other order in
the kernel .dts files. It'd be good to be consistent to allow minimal
diffs between them.

> diff --git a/board/avionic-design/dts/tegra20-medcom-wide.dts b/board/avionic-design/dts/tegra20-medcom-wide.dts

I suppose that there are no aliases in this file because only one SDHCI
controller is enabled. I wonder if we should add aliases to all .dts
files just to be explicit? Perhaps it's not necessary because this board
really will never ever get another SDHCI controller added (I assume that
any SDHCI controllers the board has are already enabled, although I
wonder about SDIO...), so there doesn't need to be a "hint" that there
should be an alias added too?

> +	sdhci at c8000600 {

In the kernel, this SDHCI node is part of tegra20-tamonten.dtsi, since
its parameters are defined by the carrier board. I think U-Boot .dts
files should match.


The following 3 comments apply to all the .dts files (or at least the
1st and 3rd; not sure about the 2nd):

> +		status = "okay";
> +		/* Parameter 3 bit 0:1=output, 0=input; bit 1:1=high, 0=low */

I don't think that comment is particularly useful. While it's true,
duplicating it every single place a GPIO is used seems wasteful. And the
comment is more re: the GPIO binding that anything to do with SDHCI.
Plus, it makes another diff relative to the kernel.

> +		cd-gpios = <&gpio 58 0>;	/* gpio PH2 */
> +		wp-gpios = <&gpio 59 0>;	/* gpio PH3 */

The kernel appears to have a space before the comment not a TAB, so this
makes another diff..

> +		bus-width = <4>;
> +		removable = <1>;

What is "removable"? That's not in the binding documentation. There is a
"non-removable" property defined in the kernel's
Documentation/devicetree/bindings/mmc/mmc.txt though...

> +	};

> diff --git a/board/nvidia/dts/tegra20-ventana.dts b/board/nvidia/dts/tegra20-ventana.dts

This file doesn't have any aliases added.

> +	sdhci at c8000000 {
> +		status = "okay";
> +		power-gpios = <&gpio 86 0>;	/* gpio PK6 */
> +		bus-width = <4>;
> +		removable = <0>;
> +	};

I think that's the WiFi SDIO port, so you may not need to add it to U-Boot.