[U-Boot] [PATCH 7/8] Tegra124: Venice2: fdt: Add device-tree files

[Stephen Warren]

On 10/07/2013 04:42 PM, Tom Warren wrote:
> These are fairly complete, and near-clones of T114 Venice,
> with an additional I2C port, and MMC address changes for T124.

> diff --git a/arch/arm/dts/tegra124.dtsi b/arch/arm/dts/tegra124.dtsi

> +	tegra_car: clock {
> +		compatible = "nvidia,tegra114-car";

I strongly doubt that the Tegra124 CAR is backwards-compatible with the
Tegra114 CAR. I think that should be:

		compatible = "nvidia,tegra124-car";

Have you validated all the compatible values in this file to make sure
they're accurate? I'd tend towards leaving out DT nodes that aren't
useful yet to reduce the need to verify this, unless you intend to add
all the drivers very quickly.

> +		reg = <0x60006000 0x1000>;

There's been a DT rule change/clarification. All DT nodes that contain a
reg property must contain a unit address in their node name. DT nodes
without a reg property must not contain a unit address in their node
name. As such, this should be "clock at 60006000" not "clock". I'd like to
see this rule applied to DTs for all new SoCs going forward, even if we
haven't yet thought through fixing up all the existing DTs to comply
with the rule.

> +	apbdma: dma {
> +		compatible = "nvidia,tegra114-apbdma", "nvidia,tegra30-apbdma";
> +		reg = <0x6000a000 0x1400>;
> +		interrupts = <0 104 0x04

It'd be nice to finally import include/dt-bindings from the kernel so we
could use named constants instead of magic numbers for the "0x04" here...

> +		status = "disable";

"disabled" not "disable".

> +/* This table has USB timing parameters for each Oscillator frequency we
> + * support. There are four sets of values:

This table should be part of the driver, not DT. Hence, all the
usbparams nodes should be removed.

> +	usb at 7d000000 {
> +		compatible = "nvidia,tegra114-ehci", "nvidia,tegra30-ehci";
> +		reg = <0x7d000000 0x4000>;
> +		interrupts = < 52 >;
> +		phy_type = "utmi";
> +		clocks = <&tegra_car 22>;	/* PERIPH_ID_USBD */
> +		status = "disabled";
> +	};

These don't conform to the latest USB bindings, which have split EHCI
controller and PHY nodes.

> diff --git a/board/nvidia/dts/tegra124-venice2.dts b/board/nvidia/dts/tegra124-venice2.dts

> +/include/ "tegra124.dtsi"
> +#ifdef CONFIG_CHROMEOS
> +/include/ "flashmap-tegra-ro.dtsi"
> +/include/ "flashmap-tegra-4mb-rw.dtsi"
> +/include/ "chromeos-tegra.dtsi"
> +/include/ "crostegra-common.dtsi"
> +#endif

CONFIG_CHROMEOS doesn't exist upstream; I think that should be removed.

> +	config {
> +		hwid = "NVIDIA Venice2";
> +	};

That looks like another non-standard ChromeOS-ism.

> +	i2c at 7000c000 {
> +		status = "okay";
> +		clock-frequency = <100000>;
> +		nvidia,use-repeat-start;

That's not a standard property.

> +	spi at 7000d400 {
> +		status = "okay";
> +		spi-max-frequency = <25000000>;
> +		spi-deactivate-delay = <100>;

That's not a standard property.

> +		cros-ec {
> +			compatible = "google,cros-ec";
> +			spi-half-duplex;
> +			spi-frame-header = <0xec>;
> +			spi-max-frequency = <4000000>;
> +			ec-interrupt = <&gpio 23 1>; /* PC7, KBC_IRQ_L */
> +			reg = <0>;
> +		};

I don't think that conforms to the binding in the kernel's
Documentation/devicetree/bindings/mfd/cros-ec.txt. At least the
compatible value doesn't match.

> +	sdhci at 700b0400 {
> +		cd-gpios = <&gpio 170 0>; /* gpio PV2 */
> +		power-gpios = <&gpio 136 0>; /* gpio PR0 */

power-gpios hasn't been part of the binding for a long time. That should
be an xxx-supply property.

> +		bus-width = <4>;
> +		status = "okay";
> +		nvidia,removable = <1>;

That's not a standard property.