[U-Boot] [PATCH 1/2] Tegra: fdt: Add/enhance sdhci (mmc) nodes for all T20 DT files
Simon Glass
sjg at chromium.org
Wed Feb 6 05:56:59 CET 2013
Hi,
On Tue, Feb 5, 2013 at 12:49 PM, Stephen Warren <swarren at wwwdotorg.org> wrote:
> 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.
I have to say that within reason I like the idea of bring in the DT
from the kernel as is, limited perhaps to the nodes that U-Boot
actually uses.
A separate repo for the DT files seems like something that should
happen, but I have seen little progress on that front. Still, when it
happens, it would be nice it we could drop U-Boot's files and just use
the kernel's. That will be a lot easier if we head in that direction
now.
Regards,
Simon
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
More information about the U-Boot
mailing list