Hi, On Tue, Feb 5, 2013 at 12:49 PM, Stephen Warren <[email protected]> wrote: > On 02/05/2013 01:29 PM, Tom Warren wrote: >> Stephen, >> >> On Tue, Feb 5, 2013 at 12:54 PM, Stephen Warren <[email protected]> >> 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@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@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@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 > [email protected] > http://lists.denx.de/mailman/listinfo/u-boot _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
