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

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@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@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@7000c000 {

• status = "okay";
  

• clock-frequency = <100000>;
  

• nvidia,use-repeat-start;
  

That's not a standard property.

• spi@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@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.