[PATCH 3/3] ARM: mvebu: Add Armada 385 General Purpose Development Board support
From: Thomas Petazzoni <hidden>
Date: 2014-12-31 10:32:50
Also in:
linux-devicetree
Dear Gregory CLEMENT, On Sat, 27 Dec 2014 12:00:35 +0100, Gregory CLEMENT wrote:
quoted hunk
+ spi-flash at 0 { + #address-cells = <1>; + #size-cells = <1>; + compatible = "st,m25p128"; + reg = <0>; /* Chip select 0 */ + spi-max-frequency = <50000000>;
According to the m25p128 datasheet, the max frequency is 54 Mhz.
quoted hunk
+ i2c at 11000 { + status = "okay"; + clock-frequency = <100000>; + + pca9555_0: pca9555 at 20 { + compatible = "nxp,pca9555"; + pinctrl-names = "default"; + pinctrl-0 = <&pca0_pins>; + interrupt-parent = <&gpio0>; + interrupts = <18 IRQ_TYPE_EDGE_FALLING>; + gpio-controller; + #gpio-cells = <2>; + interrupt-controller; + #interrupt-cells = <2>; + reg = <0x20>; + }; + + pca9555_1: pca9555 at 21 { + compatible = "nxp,pca9555"; + pinctrl-names = "default"; + interrupt-parent = <&gpio0>; + interrupts = <18 IRQ_TYPE_EDGE_FALLING>; + + gpio-controller; + #gpio-cells = <2>; + interrupt-controller; + #interrupt-cells = <2>; + + reg = <0x21>; + };
It would be good to align both description in terms of empty new lines. Also, in the second controller, you have pinctrl-names, but no pinctrl-0, so it doesn't make much sense.
quoted hunk
+ ethernet at 70000 { + status = "okay"; + phy = <&phy1>; + phy-mode = "rgmii-id"; + }; + +
One too many empty new line.
quoted hunk
+ mdio at 72004 { + phy0: ethernet-phy at 0 { + reg = <0>; + }; + + phy1: ethernet-phy at 1 { + reg = <1>; + }; + };
Maybe this is confusing, but it seems like the port 0 (i.e the one at
0x70000) is connected to the PHY at address 1, while the port 1 (i.e
the one at 0x30000) is connected to the PHY at address 0. According to
U-Boot:
| egiga0 | RGMII | 0x01 |
| egiga1 | RGMII | 0x00 |
So maybe we should have:
ethernet at 30000 {
phy = <&phy1>;
};
ethernet at 70000 {
phy = <&phy0>;
};
mdio at 72004 {
phy0: ethernet-phy at 1 {
reg = <1>;
};
phy1: ethernet-phy at 0 {
reg = <0>;
};
};
To reflect this, no?
quoted hunk
+ sata at a8000 { + nr-ports = <2>; + status = "okay"; + #address-cells = <1>; + #size-cells = <0>; + + sata0: sata-port at 0 { + reg = <0>; + target-supply = <®_5v_sata0>; + }; + + sata1: sata-port at 1 { + reg = <1>; + target-supply = <®_5v_sata1>; + };
Doesn't this part depends on the patches you have submitted to the AHCI driver? If so, it would be good to state this in the cover letter, so that the dependencies are known. Or for the moment, to not handle the SATA part, until the DT binding for describing per-SATA port regulators is sorted out (I saw that the feedback from Mark Brown on your patches indicate that some rework will be needed). Also, having a reg property into a child node that isn't part of a bus looks wrong.
quoted hunk
+ }; + + sata at e0000 { + nr-ports = <2>; + status = "okay"; + #address-cells = <1>; + #size-cells = <0>; + + sata2: sata-port at 0 { + reg = <0>; + target-supply = <®_5v_sata2>; + }; + + sata3: sata-port at 1 { + reg = <1>; + target-supply = <®_5v_sata3>; + }; + };
Ditto.
+ sdhci at d8000 {
+ clock-frequency = <200000000>;Why? There is already a 'clocks' property in armada-38x.dtsi. However, the clock is actually 250 Mhz, not 200 Mhz. Any reason why you forced 200 Mhz here?
quoted hunk
+ cd-gpios = <&pca9555_0 5 GPIO_ACTIVE_LOW>; + no-1-8-v; + wp-inverted;
Why do you have a wp-inverted property, with no wp-gpios property? If I understand the DT binding correctly, wp-inverted only makes sense when wp-gpios is used. Thanks, Thomas -- Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com