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

[Stephen Warren]

On 02/05/2013 01:29 PM, Tom Warren wrote:
> 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

linux-next or the Tegra git tree have the latest additions. arm-soc
hopefully will have them merged in the next day or two.

git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
(whatever the latest tag is there)

git://git.kernel.org/pub/scm/linux/kernel/git/swarren/linux-tegra.git
(for-next)

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

I don't think so; at least with the driver as-is, the code appears not
to work without aliases, so not adding them causes a regression. Even
ignoring that, I don't see why the code->DT conversion patch shouldn't
do this for all boards.

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

No, this has nothing to do with U-Boot vs. Linux. The device tree files
are (should eventually be) standard between the two, and indeed hosted
outside U-Boot. Unrelated, common code is common and should be
represented at a common location. In this case, the vendor for this
particular file has already correctly chosen to put the SDHCI nodes in
the common file, so this needs to be maintained.

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

If there is no diff at all, it's even easier.

The third parameter is already documented in the binding documentation.

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

Yes, really. Why would I bother to make this review comment otherwise.
As I have repeatedly specified, the idea is to reduce the diff between
the kernel and U-Boot .dts files so the diff for a node is zero. There's
a big difference between zero (no manual checking required at all) vs.
even something trivial (always requires manual checking every time a
comparison is made).

Note that at some point, all these .dts files are probably going to be
removed from the kernel and U-Boot anyway, and hosted separately, so
unifying them can only help there.

Sometimes I wonder why I even both reviewing things.

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

Perhaps, but as I said before, whole nodes present/missing is a lot
easier to deal with than diffs within nodes.

Right now, I believe your/Simon's policy on DT is to only include in the
U-Boot .dts files what's actually needed for U-Boot. I've asked that
this be done on a per-node basis rather than per-property basis in order
to reduce diffs. If you want to change that, and include nodes that
U-Boot doesn't need, that'd be great and assist unification, but then
I'd recommend simply importing the current kernel .dts files as-is
without any changes, rather than adding things piece-meal.