From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Warren Date: Tue, 05 Feb 2013 13:49:40 -0700 Subject: [U-Boot] [PATCH 1/2] Tegra: fdt: Add/enhance sdhci (mmc) nodes for all T20 DT files In-Reply-To: References: <1360021735-13286-1-git-send-email-twarren@nvidia.com> <1360021735-13286-2-git-send-email-twarren@nvidia.com> <51116378.2040205@wwwdotorg.org> Message-ID: <51117064.6080806@wwwdotorg.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 02/05/2013 01:29 PM, Tom Warren wrote: > Stephen, > > On Tue, Feb 5, 2013 at 12:54 PM, Stephen Warren 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.