Hi,
On Tue, Feb 5, 2013 at 12:49 PM, Stephen Warren swarren@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@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@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 U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot