Hi Thomas,
On 31/12/2014 11:32, Thomas Petazzoni wrote:
> Dear Gregory CLEMENT,
>
> On Sat, 27 Dec 2014 12:00:35 +0100, Gregory CLEMENT wrote:
>
>> + spi-flash@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.
It is the max frequency for the 65nm devices, but the one on the GP board
is a M25P128-VMF6P. As there was no 'B' in the part number then it was a
130nm device which is limited to 50MHz.
>
>> + i2c@11000 {
>> + status = "okay";
>> + clock-frequency = <100000>;
>> +
>> + pca9555_0: pca9555@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@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.
>
Yes it was a left over of from a previous version, I will remove it.
>> + ethernet@70000 {
>> + status = "okay";
>> + phy = <&phy1>;
>> + phy-mode = "rgmii-id";
>> + };
>> +
>> +
>
> One too many empty new line.
>
>> + mdio@72004 {
>> + phy0: ethernet-phy@0 {
>> + reg = <0>;
>> + };
>> +
>> + phy1: ethernet-phy@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@30000 {
> phy = <&phy1>;
> };
>
> ethernet@70000 {
> phy = <&phy0>;
> };
>
> mdio@72004 {
> phy0: ethernet-phy@1 {
> reg = <1>;
> };
>
> phy1: ethernet-phy@0 {
> reg = <0>;
> };
> };
>
> To reflect this, no?
>
>> + sata@a8000 {
>> + nr-ports = <2>;
>> + status = "okay";
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + sata0: sata-port@0 {
>> + reg = <0>;
>> + target-supply = <®_5v_sata0>;
>> + };
>> +
>> + sata1: sata-port@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).
It was the same kind of DT binding which was used for PHY that I used for
the regulator, I didn't imagine that it couldn't be accepted. But I was
wrong DT bindings seems to be really dependent of each maintainers.
I will move the regulator part of the SATA in a different patch.
>
> Also, having a reg property into a child node that isn't part of a bus
> looks wrong.
But according to the binding documentation:
Documentation/devicetree/bindings/ata/ahci-platform.txt, the reg property is a
required property. If it is wrong that means that the bindings should be
changed,
but it also should be stable.
>
>> + };
>> +
>> + sata@e0000 {
>> + nr-ports = <2>;
>> + status = "okay";
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + sata2: sata-port@0 {
>> + reg = <0>;
>> + target-supply = <®_5v_sata2>;
>> + };
>> +
>> + sata3: sata-port@1 {
>> + reg = <1>;
>> + target-supply = <®_5v_sata3>;
>> + };
>> + };
>
> Ditto.
>
>> + sdhci@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?
>
It was copied from the 385 DB dts before the patch "ARM: mvebu: remove
clock-frequency from Armada 38x SDHCI Device Tree node" was applied. I will
remove it
>> + 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.
This part came also from the 385 DB board as the connector was similar. I
thought you had introduced it on purpose so I kept it.
Thanks,
Gregory
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
