[PATCH 1/1] ARM: tegra: Add basic support for carma devkit

[PATCH 1/1] ARM: tegra: Add basic support for carma devkit

[Chris Desjardins]

Signed-off-by: Chris Desjardins <chris.desjardins-8HJrC8Or5ylBDgjK7y7TUQ@public.gmane.org>
---
 arch/arm/boot/dts/tegra30-carma.dts |  461 +++++++++++++++++++++++++++++++++++
 1 file changed, 461 insertions(+)
 create mode 100644 arch/arm/boot/dts/tegra30-carma.dts

diff --git a/arch/arm/boot/dts/tegra30-carma.dts b/arch/arm/boot/dts/tegra30-carma.dts
new file mode 100644
index 0000000..e88134e
--- /dev/null
+++ b/arch/arm/boot/dts/tegra30-carma.dts
@@ -0,0 +1,461 @@
+/dts-v1/;
+#include "tegra30.dtsi"
+
+/**
+ * This file contains common DT entry for Carma
+ */
+
+/ {
+	model = "NVIDIA Tegra30 Carma evaluation board";
+	compatible = "nvidia,carma", "nvidia,tegra30";
+
+	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>;
+		};
+	};
+
+	memory {
+		reg = <0x80000000 0x40000000>;
+	};
+
+	pinmux {
+		pinctrl-names = "default";
+		pinctrl-0 = <&state_default>;
+
+		state_default: pinmux {
+			sdmmc1_clk_pz0 {
+				nvidia,pins = "sdmmc1_clk_pz0";
+				nvidia,function = "sdmmc1";
+				nvidia,pull = <0>;
+				nvidia,tristate = <0>;
+			};
+			sdmmc1_cmd_pz1 {
+				nvidia,pins =	"sdmmc1_cmd_pz1",
+						"sdmmc1_dat0_py7",
+						"sdmmc1_dat1_py6",
+						"sdmmc1_dat2_py5",
+						"sdmmc1_dat3_py4";
+				nvidia,function = "sdmmc1";
+				nvidia,pull = <2>;
+				nvidia,tristate = <0>;
+			};
+			sdmmc3_clk_pa6 {
+				nvidia,pins = "sdmmc3_clk_pa6";
+				nvidia,function = "sdmmc3";
+				nvidia,pull = <0>;
+				nvidia,tristate = <0>;
+			};
+			sdmmc3_cmd_pa7 {
+				nvidia,pins =	"sdmmc3_cmd_pa7",
+						"sdmmc3_dat0_pb7",
+						"sdmmc3_dat1_pb6",
+						"sdmmc3_dat2_pb5",
+						"sdmmc3_dat3_pb4";
+				nvidia,function = "sdmmc3";
+				nvidia,pull = <2>;
+				nvidia,tristate = <0>;
+			};
+			sdmmc4_clk_pcc4 {
+				nvidia,pins =	"sdmmc4_clk_pcc4",
+						"sdmmc4_rst_n_pcc3";
+				nvidia,function = "sdmmc4";
+				nvidia,pull = <0>;
+				nvidia,tristate = <0>;
+			};
+			sdmmc4_dat0_paa0 {
+				nvidia,pins =	"sdmmc4_dat0_paa0",
+						"sdmmc4_dat1_paa1",
+						"sdmmc4_dat2_paa2",
+						"sdmmc4_dat3_paa3",
+						"sdmmc4_dat4_paa4",
+						"sdmmc4_dat5_paa5",
+						"sdmmc4_dat6_paa6",
+						"sdmmc4_dat7_paa7";
+				nvidia,function = "sdmmc4";
+				nvidia,pull = <2>;
+				nvidia,tristate = <0>;
+			};
+			dap2_fs_pa2 {
+				nvidia,pins =	"dap2_fs_pa2",
+						"dap2_sclk_pa3",
+						"dap2_din_pa4",
+						"dap2_dout_pa5";
+				nvidia,function = "i2s1";
+				nvidia,pull = <0>;
+				nvidia,tristate = <0>;
+			};
+			sdio3 {
+				nvidia,pins = "drive_sdio3";
+				nvidia,high-speed-mode = <0>;
+				nvidia,schmitt = <0>;
+				nvidia,pull-down-strength = <46>;
+				nvidia,pull-up-strength = <42>;
+				nvidia,slew-rate-rising = <1>;
+				nvidia,slew-rate-falling = <1>;
+			};
+			uart3_txd_pw6 {
+				nvidia,pins =	"uart3_txd_pw6",
+						"uart3_cts_n_pa1",
+						"uart3_rts_n_pc0",
+						"uart3_rxd_pw7";
+				nvidia,function = "uartc";
+				nvidia,pull = <0>;
+				nvidia,tristate = <0>;
+			};
+		};
+	};
+
+	serial@70006200 {
+		compatible = "ns16550";
+		status = "okay";
+	};
+
+	rgbddc: i2c@7000c000 {
+		status = "okay";
+		clock-frequency = <100000>;
+	};
+
+	i2c@7000c400 {
+		status = "okay";
+		clock-frequency = <100000>;
+	};
+
+	i2c@7000c500 {
+		status = "okay";
+		clock-frequency = <100000>;
+	};
+
+	hdmiddc: i2c@7000c700 {
+		status = "okay";
+		clock-frequency = <100000>;
+	};
+
+	i2c@7000d000 {
+		status = "okay";
+		clock-frequency = <100000>;
+
+		tps62361 {
+			compatible = "ti,tps62361";
+			reg = <0x60>;
+
+			regulator-name = "tps62361-vout";
+			regulator-min-microvolt = <500000>;
+			regulator-max-microvolt = <1500000>;
+			regulator-boot-on;
+			regulator-always-on;
+			ti,vsel0-state-high;
+			ti,vsel1-state-high;
+		};
+
+		pmic: tps65911@2d {
+			compatible = "ti,tps65911";
+			reg = <0x2d>;
+
+			interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>;
+			#interrupt-cells = <2>;
+			interrupt-controller;
+
+			ti,system-power-controller;
+
+			#gpio-cells = <2>;
+			gpio-controller;
+
+			vcc1-supply = <&vdd_ac_bat_reg>;
+			vcc2-supply = <&vdd_ac_bat_reg>;
+			vcc3-supply = <&vio_reg>;
+			vcc4-supply = <&vdd_5v0_reg>;
+			vcc5-supply = <&vdd_ac_bat_reg>;
+			vcc6-supply = <&vdd2_reg>;
+			vcc7-supply = <&vdd_ac_bat_reg>;
+			vccio-supply = <&vdd_ac_bat_reg>;
+
+			regulators {
+				vdd1_reg: vdd1 {
+					regulator-name = "vddio_ddr_1v2";
+					regulator-min-microvolt = <1200000>;
+					regulator-max-microvolt = <1200000>;
+					regulator-always-on;
+				};
+
+				vdd2_reg: vdd2 {
+					regulator-name = "vdd_1v5_gen";
+					regulator-min-microvolt = <1500000>;
+					regulator-max-microvolt = <1500000>;
+					regulator-always-on;
+				};
+
+				vddctrl_reg: vddctrl {
+					regulator-name = "vdd_cpu,vdd_sys";
+					regulator-min-microvolt = <1000000>;
+					regulator-max-microvolt = <1000000>;
+					regulator-always-on;
+				};
+
+				vio_reg: vio {
+					regulator-name = "vdd_1v8_gen";
+					regulator-min-microvolt = <1800000>;
+					regulator-max-microvolt = <1800000>;
+					regulator-always-on;
+				};
+
+				ldo1_reg: ldo1 {
+					regulator-name = "vdd_pexa,vdd_pexb";
+					regulator-min-microvolt = <1050000>;
+					regulator-max-microvolt = <1050000>;
+				};
+
+				ldo2_reg: ldo2 {
+					regulator-name = "vdd_sata,avdd_plle";
+					regulator-min-microvolt = <1050000>;
+					regulator-max-microvolt = <1050000>;
+				};
+
+				/* LDO3 is not connected to anything */
+
+				ldo4_reg: ldo4 {
+					regulator-name = "vdd_rtc";
+					regulator-min-microvolt = <1200000>;
+					regulator-max-microvolt = <1200000>;
+					regulator-always-on;
+				};
+
+				ldo5_reg: ldo5 {
+					regulator-name = "vddio_sdmmc,avdd_vdac";
+					regulator-min-microvolt = <3300000>;
+					regulator-max-microvolt = <3300000>;
+					regulator-always-on;
+				};
+
+				ldo6_reg: ldo6 {
+					regulator-name = "avdd_dsi_csi,pwrdet_mipi";
+					regulator-min-microvolt = <1200000>;
+					regulator-max-microvolt = <1200000>;
+				};
+
+				ldo7_reg: ldo7 {
+					regulator-name = "vdd_pllm,x,u,a_p_c_s";
+					regulator-min-microvolt = <1200000>;
+					regulator-max-microvolt = <1200000>;
+					regulator-always-on;
+				};
+
+				ldo8_reg: ldo8 {
+					regulator-name = "vdd_ddr_hs";
+					regulator-min-microvolt = <1000000>;
+					regulator-max-microvolt = <1000000>;
+					regulator-always-on;
+				};
+			};
+		};
+	};
+
+	spi@7000da00 {
+		status = "okay";
+		spi-max-frequency = <25000000>;
+		spi-flash@1 {
+			compatible = "winbond,w25q32";
+			reg = <1>;
+			spi-max-frequency = <20000000>;
+		};
+	};
+
+	ahub {
+		i2s@70080400 {
+			status = "okay";
+		};
+	};
+
+	pmc {
+		status = "okay";
+		nvidia,invert-interrupt;
+		nvidia,suspend-mode = <2>;
+		nvidia,cpu-pwr-good-time = <2000>;
+		nvidia,cpu-pwr-off-time = <200>;
+		nvidia,core-pwr-good-time = <3845 3845>;
+		nvidia,core-pwr-off-time = <0>;
+		nvidia,core-power-req-active-high;
+		nvidia,sys-clock-req-active-high;
+	};
+
+	sdhci@78000000 {
+		status = "okay";
+		cd-gpios = <&gpio TEGRA_GPIO(I, 5) GPIO_ACTIVE_LOW>;
+		wp-gpios = <&gpio TEGRA_GPIO(T, 3) GPIO_ACTIVE_HIGH>;
+		power-gpios = <&gpio TEGRA_GPIO(D, 7) GPIO_ACTIVE_HIGH>;
+		bus-width = <4>;
+	};
+
+	sdhci@78000400 {
+		status = "okay";
+		power-gpios = <&gpio TEGRA_GPIO(D, 3) GPIO_ACTIVE_HIGH>;
+		bus-width = <4>;
+		keep-power-in-suspend;
+	};
+
+	sdhci@78000600 {
+		status = "okay";
+		bus-width = <8>;
+		non-removable;
+	};
+
+	clocks {
+		compatible = "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		clk32k_in: clock {
+			compatible = "fixed-clock";
+			reg=<0>;
+			#clock-cells = <0>;
+			clock-frequency = <32768>;
+		};
+	};
+
+	regulators {
+		compatible = "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		vdd_ac_bat_reg: regulator@0 {
+			compatible = "regulator-fixed";
+			reg = <0>;
+			regulator-name = "vdd_ac_bat";
+			regulator-min-microvolt = <5000000>;
+			regulator-max-microvolt = <5000000>;
+			regulator-always-on;
+		};
+
+
+		cp_5v_reg: regulator@2 {
+			compatible = "regulator-fixed";
+			reg = <2>;
+			regulator-name = "cp_5v";
+			regulator-min-microvolt = <5000000>;
+			regulator-max-microvolt = <5000000>;
+			regulator-boot-on;
+			regulator-always-on;
+			enable-active-high;
+			gpio = <&pmic 0 GPIO_ACTIVE_HIGH>;
+		};
+
+		modem_3v3_reg: regulator@4 {
+			compatible = "regulator-fixed";
+			reg = <4>;
+			regulator-name = "modem_3v3";
+			regulator-min-microvolt = <3300000>;
+			regulator-max-microvolt = <3300000>;
+			enable-active-high;
+			gpio = <&gpio TEGRA_GPIO(D, 6) GPIO_ACTIVE_HIGH>;
+		};
+
+		pex_hvdd_3v3_reg: regulator@5 {
+			compatible = "regulator-fixed";
+			reg = <5>;
+			regulator-name = "pex_hvdd_3v3";
+			regulator-min-microvolt = <3300000>;
+			regulator-max-microvolt = <3300000>;
+			enable-active-high;
+			gpio = <&gpio TEGRA_GPIO(L, 7) GPIO_ACTIVE_HIGH>;
+			vin-supply = <&vdd_ac_bat_reg>;
+		};
+
+		ddr_reg: regulator@100 {
+			compatible = "regulator-fixed";
+			regulator-name = "ddr";
+			reg = <100>;
+			regulator-min-microvolt = <1500000>;
+			regulator-max-microvolt = <1500000>;
+			regulator-always-on;
+			regulator-boot-on;
+			enable-active-high;
+			gpio = <&pmic 7 GPIO_ACTIVE_HIGH>;
+		};
+
+		sys_3v3_reg: regulator@101 {
+			compatible = "regulator-fixed";
+			reg = <101>;
+			regulator-name = "sys_3v3";
+			regulator-min-microvolt = <3300000>;
+			regulator-max-microvolt = <3300000>;
+			regulator-always-on;
+			regulator-boot-on;
+			enable-active-high;
+			gpio = <&pmic 6 GPIO_ACTIVE_HIGH>;
+		};
+
+		usb1_vbus_reg: regulator@102 {
+			compatible = "regulator-fixed";
+			reg = <102>;
+			regulator-name = "usb1_vbus";
+			regulator-min-microvolt = <5000000>;
+			regulator-max-microvolt = <5000000>;
+			enable-active-high;
+			gpio = <&gpio TEGRA_GPIO(DD, 6) GPIO_ACTIVE_HIGH>;
+			gpio-open-drain;
+			vin-supply = <&vdd_ac_bat_reg>;
+		};
+
+		usb3_vbus_reg: regulator@103 {
+			compatible = "regulator-fixed";
+			reg = <103>;
+			regulator-name = "usb3_vbus";
+			regulator-min-microvolt = <5000000>;
+			regulator-max-microvolt = <5000000>;
+			enable-active-high;
+			gpio = <&gpio TEGRA_GPIO(DD, 4) GPIO_ACTIVE_HIGH>;
+			gpio-open-drain;
+			vin-supply = <&vdd_ac_bat_reg>;
+		};
+
+		vdd_5v0_reg: regulator@104 {
+			compatible = "regulator-fixed";
+			reg = <104>;
+			regulator-name = "5v0";
+			regulator-min-microvolt = <5000000>;
+			regulator-max-microvolt = <5000000>;
+			enable-active-high;
+			gpio = <&pmic 8 GPIO_ACTIVE_HIGH>;
+		};
+
+		vdd_bl_reg: regulator@105 {
+			compatible = "regulator-fixed";
+			reg = <105>;
+			regulator-name = "vdd_bl";
+			regulator-min-microvolt = <5000000>;
+			regulator-max-microvolt = <5000000>;
+			regulator-always-on;
+			regulator-boot-on;
+			enable-active-high;
+			gpio = <&gpio TEGRA_GPIO(DD, 2) GPIO_ACTIVE_HIGH>;
+		};
+
+		vdd_bl2_reg: regulator@106 {
+			compatible = "regulator-fixed";
+			reg = <106>;
+			regulator-name = "vdd_bl2";
+			regulator-min-microvolt = <5000000>;
+			regulator-max-microvolt = <5000000>;
+			regulator-always-on;
+			regulator-boot-on;
+			enable-active-high;
+			gpio = <&gpio TEGRA_GPIO(DD, 0) GPIO_ACTIVE_HIGH>;
+		};
+	};
+
+
+};
-- 
1.7.10.4

Re: [PATCH 1/1] ARM: tegra: Add basic support for carma devkit

[Chris Desjardins]

Chris Desjardins <chris.desjardins@...> writes:

> 
> 
> Signed-off-by: Chris Desjardins <chris.desjardins@...>
> ---


I just wanted to say a couple of things about this patch:
1) Ethernet is pretty flaky, it works, but when you really start to put a 
heavy load on it the e1000e driver starts saying that the adapter has reset.
2) There is some stuff missing for sure, and probably there is some stuff 
that is not correct, but this is a starting point to just get the board 
running with a mainline kernel. 

Thanks,
    Chris D.

Re: [PATCH 1/1] ARM: tegra: Add basic support for carma devkit

[Stephen Warren]

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.

> diff --git a/arch/arm/boot/dts/tegra30-carma.dts b/arch/arm/boot/dts/tegra30-carma.dts

> +/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?

Re: [PATCH 1/1] ARM: tegra: Add basic support for carma devkit

[Chris Desjardins]

Stephen Warren <swarren@...> writes:

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

> 
> > +	serial <at> 70006200 {
> > +		compatible = "ns16550";
> 
> You shouldn't need to set the compatible value for this node;
> tegra30.dtsi has already set it.

True, but it is set to "nvidia,tegra30-uart", "nvidia,tegra20-uart" which 
causes the nvidia high speed uart driver to be loaded, which sucks when you 
are debugging over serial (which is my normal mode of operation) and half 
way through the boot process your serial port goes away because the ns16550 
driver you were using was unloaded and the high speed uart driver was loaded 
instead... If it is all the same I would rather just keep the old reliable 
driver loaded on this port (it is the only exposed serial port on the carma 
devkit), and I suspect other developers will feel the same... 

> 
> > +		tps62361 {
> ...
> > +			regulator-min-microvolt = <500000>;
> > +			regulator-max-microvolt = <1500000>;
> ...
> > +		pmic: tps65911 <at> 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.

I have not, I will ask Seco if they can supply such documents or verify the 
settings themselves, but the documentation they supply does not include any 
information in this area. I can say that my board runs fine with these 
settings...


> > +	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.
> 

The clk32k_in is used in tegra30.dtsi...

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

The u-boot that was supplied by nvidia works fine for me, so I am just using 
it. It is old, but it works and I don't have any reason to fiddle with it, 
they also provided the source, so I can tweak it if necessary, but so far I 
haven't had to touch it.

Stand by for 2 updated patches. 

Thanks,
    Chris D.

Re: [PATCH 1/1] ARM: tegra: Add basic support for carma devkit

[Stephen Warren]

On 06/25/2013 09:47 AM, Chris Desjardins wrote:
> Stephen Warren <swarren@...> writes:
> 
>>
>> On 06/23/2013 07:38 AM, Chris Desjardins wrote:
>>
> 
>>
>>> +	serial <at> 70006200 {
>>> +		compatible = "ns16550";
>>
>> You shouldn't need to set the compatible value for this node;
>> tegra30.dtsi has already set it.
> 
> True, but it is set to "nvidia,tegra30-uart", "nvidia,tegra20-uart" which 
> causes the nvidia high speed uart driver to be loaded, which sucks when you 

That compatible value ends up selecting the regular serial driver. Any
board that wants the HSUART driver needs to override the compatible
value to "nvidia,tegra30-hsuart", "nvidia,tegra20-hsuart" instead (which
IIRC none do at present).

So, to be consistent with all other Tegra boards, I still believe this
compatible value should be removed.

(And as an aside, selecting "ns16550" here is going to make the UART
driver not know that it's dealing with a Tegra port, and hence not apply
the Tegra-specific WARs/quirks it needs to, which will likely make the
port unreliable in some circumstances.)

>>> +		tps62361 {
>> ...
>>> +			regulator-min-microvolt = <500000>;
>>> +			regulator-max-microvolt = <1500000>;
>> ...
>>> +		pmic: tps65911 <at> 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.
> 
> I have not, I will ask Seco if they can supply such documents or verify the 
> settings themselves, but the documentation they supply does not include any 
> information in this area. I can say that my board runs fine with these 
> settings...

That may appear to happen even if the settings aren't all correct.
Perhaps you can validate the values against whatever downstream kernel
Seco supplies; matching that would be enough.

>>> +	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.
> 
> The clk32k_in is used in tegra30.dtsi...

Yes, indeed.

Re: [PATCH 1/1] ARM: tegra: Add basic support for carma devkit

[Eric Brower]

On 06/23/2013 06:38 AM, Chris Desjardins wrote:
>
> Signed-off-by: Chris Desjardins <chris.desjardins-8HJrC8Or5ylBDgjK7y7TUQ@public.gmane.org>
> ---
>   arch/arm/boot/dts/tegra30-carma.dts |  461 +++++++++++++++++++++++++++++++++++
>   1 file changed, 461 insertions(+)
>   create mode 100644 arch/arm/boot/dts/tegra30-carma.dts
>
> diff --git a/arch/arm/boot/dts/tegra30-carma.dts b/arch/arm/boot/dts/tegra30-carma.dts
> new file mode 100644
> index 0000000..e88134e
> --- /dev/null
> +++ b/arch/arm/boot/dts/tegra30-carma.dts
> @@ -0,0 +1,461 @@
> +/dts-v1/;
> +#include "tegra30.dtsi"
> +
> +/**
> + * This file contains common DT entry for Carma
> + */
> +
> +/ {
> +       model = "NVIDIA Tegra30 Carma evaluation board";
> +       compatible = "nvidia,carma", "nvidia,tegra30";

Chris,

This is not an NVIDIA board, it's a Seco board.  I'd suggest:

model = "Seco Tegra30 Carma development board";
compatible = "seco,carma", "nvidia,tegra30";

But you might want to check with Seco regarding preference of prefix for 
"seco,carma" above.  I see no precedent for it after a quick check.

Thanks,
Eric

Re: [PATCH 1/1] ARM: tegra: Add basic support for carma devkit

[Stephen Warren]

On 06/27/2013 10:59 AM, Eric Brower wrote:
> On 06/23/2013 06:38 AM, Chris Desjardins wrote:
>>
>> Signed-off-by: Chris Desjardins <chris.desjardins-8HJrC8Or5ylBDgjK7y7TUQ@public.gmane.org>
>> ---
>>   arch/arm/boot/dts/tegra30-carma.dts |  461
>> +++++++++++++++++++++++++++++++++++
>>   1 file changed, 461 insertions(+)
>>   create mode 100644 arch/arm/boot/dts/tegra30-carma.dts
>>
>> diff --git a/arch/arm/boot/dts/tegra30-carma.dts
>> b/arch/arm/boot/dts/tegra30-carma.dts
>> new file mode 100644
>> index 0000000..e88134e
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/tegra30-carma.dts
>> @@ -0,0 +1,461 @@
>> +/dts-v1/;
>> +#include "tegra30.dtsi"
>> +
>> +/**
>> + * This file contains common DT entry for Carma
>> + */
>> +
>> +/ {
>> +       model = "NVIDIA Tegra30 Carma evaluation board";
>> +       compatible = "nvidia,carma", "nvidia,tegra30";
> 
> Chris,
> 
> This is not an NVIDIA board, it's a Seco board.  I'd suggest:
> 
> model = "Seco Tegra30 Carma development board";
> compatible = "seco,carma", "nvidia,tegra30";
> 
> But you might want to check with Seco regarding preference of prefix for
> "seco,carma" above.  I see no precedent for it after a quick check.

That's a good point. The seco prefix should be added to
Documentation/devicetree/bindings/vendor-prefixes.txt. I believe the
rule for vendor prefixes is to use the company's stock ticker symbol if
there is one and it makes sense, else fall back to name. "seco" is short
enough it's probably fine either way.

Re: [PATCH 1/1] ARM: tegra: Add basic support for carma devkit

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2160 bytes --]

On Thu, Jun 27, 2013 at 11:51:20AM -0600, Stephen Warren wrote:
> On 06/27/2013 10:59 AM, Eric Brower wrote:
> > On 06/23/2013 06:38 AM, Chris Desjardins wrote:
> >>
> >> Signed-off-by: Chris Desjardins <chris.desjardins-8HJrC8Or5ylBDgjK7y7TUQ@public.gmane.org>
> >> ---
> >>   arch/arm/boot/dts/tegra30-carma.dts |  461
> >> +++++++++++++++++++++++++++++++++++
> >>   1 file changed, 461 insertions(+)
> >>   create mode 100644 arch/arm/boot/dts/tegra30-carma.dts
> >>
> >> diff --git a/arch/arm/boot/dts/tegra30-carma.dts
> >> b/arch/arm/boot/dts/tegra30-carma.dts
> >> new file mode 100644
> >> index 0000000..e88134e
> >> --- /dev/null
> >> +++ b/arch/arm/boot/dts/tegra30-carma.dts
> >> @@ -0,0 +1,461 @@
> >> +/dts-v1/;
> >> +#include "tegra30.dtsi"
> >> +
> >> +/**
> >> + * This file contains common DT entry for Carma
> >> + */
> >> +
> >> +/ {
> >> +       model = "NVIDIA Tegra30 Carma evaluation board";
> >> +       compatible = "nvidia,carma", "nvidia,tegra30";
> > 
> > Chris,
> > 
> > This is not an NVIDIA board, it's a Seco board.  I'd suggest:
> > 
> > model = "Seco Tegra30 Carma development board";
> > compatible = "seco,carma", "nvidia,tegra30";
> > 
> > But you might want to check with Seco regarding preference of prefix for
> > "seco,carma" above.  I see no precedent for it after a quick check.
> 
> That's a good point. The seco prefix should be added to
> Documentation/devicetree/bindings/vendor-prefixes.txt. I believe the
> rule for vendor prefixes is to use the company's stock ticker symbol if
> there is one and it makes sense, else fall back to name. "seco" is short
> enough it's probably fine either way.

Seco is a privately held company so there's no stock ticker symbol. Also
the canonical spelling seems to be "SECO" and their website lists the
device as "CARMA DevKit" or "CUDA on ARM DevKit", so perhaps model
should be something like:

	model = "SECO CARMA DevKit";

or

	model = "SECO CUDA on ARM DevKit";

The fact that it's based on Tegra30 becomes clear by looking at the
compatible and doesn't necessarily need to go into the model string.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/1] ARM: tegra: Add basic support for carma devkit

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 598 bytes --]

On Sun, Jun 23, 2013 at 03:38:05PM +0200, Chris Desjardins wrote:
> 
> Signed-off-by: Chris Desjardins <chris.desjardins-8HJrC8Or5ylBDgjK7y7TUQ@public.gmane.org>
> ---
>  arch/arm/boot/dts/tegra30-carma.dts |  461 +++++++++++++++++++++++++++++++++++
>  1 file changed, 461 insertions(+)
>  create mode 100644 arch/arm/boot/dts/tegra30-carma.dts

Hi Chris,

The new PCIe driver is going to be merged in 3.12, so it would be nice
to see this patch merged as well as given that it is one of the few
users of PCIe.

Have you had a chance to look at the comments you received?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Fwd: [PATCH 1/1] ARM: tegra: Add basic support for carma devkit

[Chris Desjardins]

Thierry, I have looked at the comments, and I have also talked to
SECO, and they aren't willing to give me the docs necessary to really
do the job correctly (even if they did give me the docs, I am not 100%
sure I would be able to do it correctly without a direct line to their
hardware engineers), they also are unwilling to do it themselves. They
did say that they will give me feedback on a dts file, but the
feedback I got from them on the original one was little more than:
this doesn't appear to be correct. To be honest, we have decided to go
with another platform with better support in the mainline kernel (4th
gen x86, because it is powerful, low power, and well supported), and
as a result my seco board is collecting dust.

For reference here is the full "technical" feedback I got from seco:

we noticed some inconsistencies on dts file you provided: with respect
to pcie-controller, ldo2 usage is not correct and PCIe lanes
declaration doesn't look coeherent with actual hardware.

Not really enough for me to make changes with confidence... I think I
have already mentioned this, but just to be sure, comparing and
contrasting the regulator/pci settings from kernel they provide
(pre-device tree) and the mainline is like comparing apples and
oranges. Most of the names have changed, and even in the few cases
where names seem to correlate it would still take me a huge amount of
time to really figure out what I am looking at in their
source/sysfs/etc and translate it to .dts from with confidence... and
I really have no idea how I would verify it (other than looking at
sysfs or something, but to me, looking at what sysfs says about
regulator configuration and verifying that a regulator is configured
correctly are two different things).

Thanks for following up on this, sorry I won't be able to help, feel
free to take my .dts file and run with it if you want, or pressure
seco into updating it correctly if you can.

Thanks again,
    Chris D.




On 27 August 2013 09:56, Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
> On Sun, Jun 23, 2013 at 03:38:05PM +0200, Chris Desjardins wrote:
> >
> > Signed-off-by: Chris Desjardins <chris.desjardins-8HJrC8Or5ylBDgjK7y7TUQ@public.gmane.org>
> > ---
> >  arch/arm/boot/dts/tegra30-carma.dts |  461 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 461 insertions(+)
> >  create mode 100644 arch/arm/boot/dts/tegra30-carma.dts
>
> Hi Chris,
>
> The new PCIe driver is going to be merged in 3.12, so it would be nice
> to see this patch merged as well as given that it is one of the few
> users of PCIe.
>
> Have you had a chance to look at the comments you received?
>
> Thierry

Re: [PATCH 1/1] ARM: tegra: Add basic support for carma devkit

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3221 bytes --]

On Tue, Aug 27, 2013 at 11:15:12AM +0200, Chris Desjardins wrote:
> Thierry, I have looked at the comments, and I have also talked to SECO, and
> they aren't willing to give me the docs necessary to really do the job
> correctly (even if they did give me the docs, I am not 100% sure I would be
> able to do it correctly without a direct line to their hardware engineers),
> they also are unwilling to do it themselves. They did say that they will
> give me feedback on a dts file, but the feedback I got from them on the
> original one was little more than: this doesn't appear to be correct. To be
> honest, we have decided to go with another platform with better support in
> the mainline kernel (4th gen x86, because it is powerful, low power, and
> well supported), and as a result my seco board is collecting dust.

I'm sorry to hear that.

> For reference here is the full "technical" feedback I got from seco:
> 
> we noticed some inconsistencies on dts file you provided: with respect to
> pcie-controller, ldo2 usage is not correct and PCIe lanes declaration
> doesn't look coeherent with actual hardware.
> 
> Not really enough for me to make changes with confidence... I think I have
> already mentioned this, but just to be sure, comparing and contrasting the
> regulator/pci settings from kernel they provide (pre-device tree) and the
> mainline is like comparing apples and oranges. Most of the names have
> changed, and even in the few cases where names seem to correlate it would
> still take me a huge amount of time to really figure out what I am looking
> at in their source/sysfs/etc and translate it to .dts from with
> confidence... and I really have no idea how I would verify it (other than
> looking at sysfs or something, but to me, looking at what sysfs says about
> regulator configuration and verifying that a regulator is configured
> correctly are two different things).

Indeed. It takes quite a bit of effort to successfully correlate the two
datasets.

> Thanks for following up on this, sorry I won't be able to help, feel free
> to take my .dts file and run with it if you want, or pressure seco into
> updating it correctly if you can.

I doubt that I'll be able to exert any more pressure than you did. Given
my lack of hardware I won't be able to drive that effort myself, but
perhaps someone else will pick it up.

Thanks for investigating!

Thierry

> On 27 August 2013 09:56, Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> > On Sun, Jun 23, 2013 at 03:38:05PM +0200, Chris Desjardins wrote:
> > >
> > > Signed-off-by: Chris Desjardins <chris.desjardins-8HJrC8Or5ylBDgjK7y7TUQ@public.gmane.org>
> > > ---
> > >  arch/arm/boot/dts/tegra30-carma.dts |  461
> > +++++++++++++++++++++++++++++++++++
> > >  1 file changed, 461 insertions(+)
> > >  create mode 100644 arch/arm/boot/dts/tegra30-carma.dts
> >
> > Hi Chris,
> >
> > The new PCIe driver is going to be merged in 3.12, so it would be nice
> > to see this patch merged as well as given that it is one of the few
> > users of PCIe.
> >
> > Have you had a chance to look at the comments you received?
> >
> > Thierry
> >

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]