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

[Tom Warren]

Stephen,

On Tue, Feb 5, 2013 at 12:54 PM, Stephen Warren <swarren at wwwdotorg.org> wrote:
> 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?

The clock source used for this periph. I removed it in the I2C DT
files - I'll remove it here, too, because it's up to the driver to
choose that based on the freq.

>
> 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.

I used the kernel DTS files you pointed to in our internal 'Initialize
SDHCI from device tree' bug:
repo git://git.kernel.org/pub/scm/linux/kernel/git/arm/arm-soc.git
branch for-next

The order matches arch/arm/boot/dts/tegra20.dtsi. I don't see a clocks
property in that kernel file, either. I do see that the interrupts
(that were already in tegra20.dtsi before I edited it) don't match the
kernel dtsi for most peripherals (i2c, i2s, sdhci, gpio). I'll fix the
SDHCI ones in the next patchset, though.

>
>> 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?

If there was already an alias property in the DT file, then I tried to
be consistent and add one for mmc. But adding aliases to
other-than-NVIDIA boards should be up to the board
maintainer/manufacturer, IMO.

>
>> +     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.

Saw that, but I didn't replicate it here because, well, U-Boot's Not
Linux, and IMO each board file (dts) should have its periph nodes
called out explicitly. If they all happen to be exactly the same for
each board, then I think the manufacturer/board maintainer can do that
'optimization' if they wish - they know better than I if they're
coming out with a new board that may differ in its SDHCI properties
(GPIOs, for instance).

>
>
> 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.

Diffing the DTS files against the kernel isn't really that big a deal
with a decent diff program (kompare, etc.). That GPIO comment is
useful if you need to understand the 3rd param for the pwr-gpios, for
instance (the cd and wp gpios almost always are input/low). And it
only appears once in each DTS file, not "in every single place a GPIO
is used".
>
>> +             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..
Really? That seems a little nit-picky. :/
My whitespace is consistent through-out the DTS file, and with how I
always space comments on the end of a line of 'code'.

>
>> +             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...

These are left-over from Seaboard's original DTS file (with which I
did my original development). It isn't used anywhere in U-Boot that I
can see. I can remove it.

>
>> +     };
>
>> 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.
It's in the kernel DTS for Ventana. Won't that screw up your diff? ;)
I'll remove it.

Tom