On 06/23/2013 07:38 AM, Chris Desjardins wrote:

A patch description would be useful; e.g. a brief description of the
CARMA board.

+/dts-v1/;

All the other *.dts files have a blank line separating the line above
here and below.

+#include "tegra30.dtsi"

+/**
+ * This file contains common DT entry for Carma
+ */

I don't think that comment applies; I suspect it's copied from the
Cardhu file, where there actually are separate common and
version-specific files, but I don't think there will be separate files here.

+/ {
+	model = "NVIDIA Tegra30 Carma evaluation board";

This board is an actual product, so I'd remove the word "evaluation" here.

+	pcie-controller {
+		status = "okay";
+		pex-clk-supply = <&pex_hvdd_3v3_reg>;
+		vdd-supply = <&ldo1_reg>;
+		avdd-supply = <&ldo2_reg>;
+
+		pci@1,0 {
+			nvidia,num-lanes = <4>;
+		};
+
+		pci@2,0 {
+			nvidia,num-lanes = <1>;
+		};
+
+		pci@3,0 {
+			status = "okay";
+			nvidia,num-lanes = <1>;
+		};
+	};

I would split the PCIe node out into a separate patch. I can take the
main patch to add Carma support directly into the linux-tegra tree, and
Thierry can carry the patch that adds the PCIe node in his tree, until
his PCIe driver makes it upstream.

A note on node ordering: I'd like the nodes in the following order:

1) Any nodes that existing in #included files, in the order they existed
in the #included files.

2) Any new nodes with a reg property, sorted in order of reg value.

3) Any new nodes without a reg property, sorted alphabetically.

So, this PCIe node should go between memory and pinmux.

+	serial@70006200 {
+		compatible = "ns16550";

You shouldn't need to set the compatible value for this node;
tegra30.dtsi has already set it.

+		tps62361 {

...

+			regulator-min-microvolt = <500000>;
+			regulator-max-microvolt = <1500000>;

...

+		pmic: tps65911@2d {

...

+			regulators {
+				vdd1_reg: vdd1 {
+					regulator-name = "vddio_ddr_1v2";
+					regulator-min-microvolt = <1200000>;
+					regulator-max-microvolt = <1200000>;

Have you validated all these voltage levels, and *-supply properties
against the schematic or other documentation for the board? I want to
avoid accepting a DT file that sets up any of the voltages incorrectly,
which could potentially cause damage to the board/components.

+	ahub {
+		i2s@70080400 {
+			status = "okay";
+		};
+	};

I don't see any "sound" or "audio" node. As such, I don't believe any of
that "ahub" node is required.

+	clocks {
+		compatible = "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		clk32k_in: clock {
+			compatible = "fixed-clock";
+			reg=<0>;
+			#clock-cells = <0>;
+			clock-frequency = <32768>;
+		};
+	};

That doesn't seem to be used anywhere.

+	regulators {

...

+		vdd_ac_bat_reg: regulator@0 {

...

+		};
+
+
+		cp_5v_reg: regulator@2 {

There's an extra blank line there, and there's also no regulator with
reg=<1>. Is there a reason for the numbering gap?

+	};
+
+
+};

There are a couple extra blank lines there.

Out of curiosity, do you have upstream U-Boot support for Carma, or are
you using our binary bootloader, or a downstream U-Boot?