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
The order matches arch/arm/boot/dts/tegra20.dtsi. I don't see a clocks property in that kernel file, either. I do see that the interrupts (that were already in tegra20.dtsi before I edited it) don't match the kernel dtsi for most peripherals (i2c, i2s, sdhci, gpio). I'll fix the SDHCI ones in the next patchset, though.
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.
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).
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".
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'.
bus-width = <4>;removable = <1>;What is "removable"? That's not in the binding documentation. There is a "non-removable" property defined in the kernel's Documentation/devicetree/bindings/mmc/mmc.txt though...
These are left-over from Seaboard's original DTS file (with which I did my original development). It isn't used anywhere in U-Boot that I can see. I can remove it.
};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.
Tom