Re: [PATCH 2/2] ARM: dts: Add the ethernet and ethernet PHY to the cygnus core DT.

[Jon Mason]

On Tue, Apr 25, 2017 at 11:23 AM, Sergei Shtylyov
 wrote:
> Hello!
>
> On 04/25/2017 06:15 PM, Jon Mason wrote:
>
 Cygnus has a single amac controller connected to the B53 switch with 2
 PHYs.  On the BCM911360_EP platform, those two PHYs are connected to
 the external ethernet jacks.
>
>
> [...]
>
 Signed-off-by: Eric Anholt 
 ---
  arch/arm/boot/dts/bcm-cygnus.dtsi  | 60
 ++
  arch/arm/boot/dts/bcm911360_entphn.dts |  8 +
  2 files changed, 68 insertions(+)

 diff --git a/arch/arm/boot/dts/bcm-cygnus.dtsi
 b/arch/arm/boot/dts/bcm-cygnus.dtsi
 index 009f1346b817..318899df9972 100644
 --- a/arch/arm/boot/dts/bcm-cygnus.dtsi
 +++ b/arch/arm/boot/dts/bcm-cygnus.dtsi
>
> [...]

 @@ -295,6 +345,16 @@
 status = "disabled";
 };

 +   eth0: enet@18042000 {
 +   compatible = "brcm,amac";
 +   reg = <0x18042000 0x1000>,
 + <0x1811 0x1000>;
 +   reg-names = "amac_base", "idm_base";
>>>
>>>
>>>
>>>I don't think "_base" suffixes are necessary here.
>>
>>
>> 100% necessary, per the driver.  See
>> drivers/net/ethernet/broadcom/bgmac-platform.c
>
>
>I'd recommend to fix the driver/bindings then...

They're already in use in other device trees.  So, we'd need to
support backward compatibility on them, thus removing any real benefit
to changing them.


>
> MBR, Sergei
>

Re: [PATCH 2/2] ARM: dts: Add the ethernet and ethernet PHY to the cygnus core DT.

[Sergei Shtylyov]

Hello!

On 04/25/2017 06:15 PM, Jon Mason wrote:


Cygnus has a single amac controller connected to the B53 switch with 2
PHYs.  On the BCM911360_EP platform, those two PHYs are connected to
the external ethernet jacks.


[...]


Signed-off-by: Eric Anholt 
---
 arch/arm/boot/dts/bcm-cygnus.dtsi  | 60
++
 arch/arm/boot/dts/bcm911360_entphn.dts |  8 +
 2 files changed, 68 insertions(+)

diff --git a/arch/arm/boot/dts/bcm-cygnus.dtsi
b/arch/arm/boot/dts/bcm-cygnus.dtsi
index 009f1346b817..318899df9972 100644
--- a/arch/arm/boot/dts/bcm-cygnus.dtsi
+++ b/arch/arm/boot/dts/bcm-cygnus.dtsi

[...]

@@ -295,6 +345,16 @@
status = "disabled";
};

+   eth0: enet@18042000 {
+   compatible = "brcm,amac";
+   reg = <0x18042000 0x1000>,
+ <0x1811 0x1000>;
+   reg-names = "amac_base", "idm_base";



   I don't think "_base" suffixes are necessary here.


100% necessary, per the driver.  See
drivers/net/ethernet/broadcom/bgmac-platform.c


   I'd recommend to fix the driver/bindings then...

MBR, Sergei

Re: [PATCH 2/2] ARM: dts: Add the ethernet and ethernet PHY to the cygnus core DT.

[Jon Mason]

On Tue, Apr 25, 2017 at 5:40 AM, Sergei Shtylyov
 wrote:
> Hello.
>
> On 4/25/2017 12:50 AM, Eric Anholt wrote:
>
>> Cygnus has a single amac controller connected to the B53 switch with 2
>> PHYs.  On the BCM911360_EP platform, those two PHYs are connected to
>> the external ethernet jacks.
>
>
>My spell checker trips on "amac" and "ethernet" -- perhaps they need
> capitalization?
>
>> Signed-off-by: Eric Anholt 
>> ---
>>  arch/arm/boot/dts/bcm-cygnus.dtsi  | 60
>> ++
>>  arch/arm/boot/dts/bcm911360_entphn.dts |  8 +
>>  2 files changed, 68 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/bcm-cygnus.dtsi
>> b/arch/arm/boot/dts/bcm-cygnus.dtsi
>> index 009f1346b817..318899df9972 100644
>> --- a/arch/arm/boot/dts/bcm-cygnus.dtsi
>> +++ b/arch/arm/boot/dts/bcm-cygnus.dtsi
>> @@ -142,6 +142,56 @@
>> interrupts = <0>;
>> };
>>
>> +   mdio: mdio@18002000 {
>> +   compatible = "brcm,iproc-mdio";
>> +   reg = <0x18002000 0x8>;
>> +   #size-cells = <1>;
>> +   #address-cells = <0>;
>> +
>> +   gphy0: eth-gphy@0 {
>
>
>The node anmes must be generic, the DT spec has standardized
> "ethernet-phy" name for this case.
>
>> +   reg = <0>;
>> +   max-speed = <1000>;
>> +   };
>> +
>> +   gphy1: eth-gphy@1 {
>> +   reg = <1>;
>> +   max-speed = <1000>;
>> +   };
>> +   };
>
> [...]
>>
>> @@ -295,6 +345,16 @@
>> status = "disabled";
>> };
>>
>> +   eth0: enet@18042000 {
>> +   compatible = "brcm,amac";
>> +   reg = <0x18042000 0x1000>,
>> + <0x1811 0x1000>;
>> +   reg-names = "amac_base", "idm_base";
>
>
>I don't think "_base" suffixes are necessary here.

100% necessary, per the driver.  See
drivers/net/ethernet/broadcom/bgmac-platform.c


>
> [...]
>
> MBR, Sergei
>

Re: [PATCH 2/2] ARM: dts: Add the ethernet and ethernet PHY to the cygnus core DT.

[Sergei Shtylyov]

On 4/25/2017 12:50 AM, Eric Anholt wrote:


Cygnus has a single amac controller connected to the B53 switch with 2
PHYs.  On the BCM911360_EP platform, those two PHYs are connected to
the external ethernet jacks.

Signed-off-by: Eric Anholt 
---
 arch/arm/boot/dts/bcm-cygnus.dtsi  | 60 ++
 arch/arm/boot/dts/bcm911360_entphn.dts |  8 +
 2 files changed, 68 insertions(+)

diff --git a/arch/arm/boot/dts/bcm-cygnus.dtsi 
b/arch/arm/boot/dts/bcm-cygnus.dtsi
index 009f1346b817..318899df9972 100644
--- a/arch/arm/boot/dts/bcm-cygnus.dtsi
+++ b/arch/arm/boot/dts/bcm-cygnus.dtsi

[...]

@@ -295,6 +345,16 @@
status = "disabled";
};

+   eth0: enet@18042000 {


   Oh, and this one should be named "ethernet", according to the DT spec.


+   compatible = "brcm,amac";
+   reg = <0x18042000 0x1000>,
+ <0x1811 0x1000>;
+   reg-names = "amac_base", "idm_base";
+   interrupts = ;
+   max-speed = <1000>;
+   status = "disabled";
+   };
+
nand: nand@18046000 {
compatible = "brcm,nand-iproc", "brcm,brcmnand-v6.1";
reg = <0x18046000 0x600>, <0xf8105408 0x600>,

[...]

MBR, Sergei

Re: [PATCH 2/2] ARM: dts: Add the ethernet and ethernet PHY to the cygnus core DT.

[Sergei Shtylyov]

Hello.

On 4/25/2017 12:50 AM, Eric Anholt wrote:


Cygnus has a single amac controller connected to the B53 switch with 2
PHYs.  On the BCM911360_EP platform, those two PHYs are connected to
the external ethernet jacks.


   My spell checker trips on "amac" and "ethernet" -- perhaps they need 
capitalization?



Signed-off-by: Eric Anholt 
---
 arch/arm/boot/dts/bcm-cygnus.dtsi  | 60 ++
 arch/arm/boot/dts/bcm911360_entphn.dts |  8 +
 2 files changed, 68 insertions(+)

diff --git a/arch/arm/boot/dts/bcm-cygnus.dtsi 
b/arch/arm/boot/dts/bcm-cygnus.dtsi
index 009f1346b817..318899df9972 100644
--- a/arch/arm/boot/dts/bcm-cygnus.dtsi
+++ b/arch/arm/boot/dts/bcm-cygnus.dtsi
@@ -142,6 +142,56 @@
interrupts = <0>;
};

+   mdio: mdio@18002000 {
+   compatible = "brcm,iproc-mdio";
+   reg = <0x18002000 0x8>;
+   #size-cells = <1>;
+   #address-cells = <0>;
+
+   gphy0: eth-gphy@0 {


   The node anmes must be generic, the DT spec has standardized 
"ethernet-phy" name for this case.



+   reg = <0>;
+   max-speed = <1000>;
+   };
+
+   gphy1: eth-gphy@1 {
+   reg = <1>;
+   max-speed = <1000>;
+   };
+   };

[...]

@@ -295,6 +345,16 @@
status = "disabled";
};

+   eth0: enet@18042000 {
+   compatible = "brcm,amac";
+   reg = <0x18042000 0x1000>,
+ <0x1811 0x1000>;
+   reg-names = "amac_base", "idm_base";


   I don't think "_base" suffixes are necessary here.

[...]

MBR, Sergei

Re: [PATCH 2/2] ARM: dts: Add the ethernet and ethernet PHY to the cygnus core DT.

[Eric Anholt]

Florian Fainelli  writes:

> On 04/24/2017 02:50 PM, Eric Anholt wrote:
>> Cygnus has a single amac controller connected to the B53 switch with 2
>> PHYs.  On the BCM911360_EP platform, those two PHYs are connected to
>> the external ethernet jacks.
>> 
>> Signed-off-by: Eric Anholt 
>
> This looks fine, just a few nits on the label names:

Thanks.  I've applied all of these (and Andrew's and Scott's
suggestions), and I'll send out a new version once the DT maintainers
have had a chance to look.


signature.asc
Description: PGP signature

Re: [PATCH 2/2] ARM: dts: Add the ethernet and ethernet PHY to the cygnus core DT.

[Eric Anholt]

Andrew Lunn  writes:

>> +mdio: mdio@18002000 {
>> +compatible = "brcm,iproc-mdio";
>> +reg = <0x18002000 0x8>;
>> +#size-cells = <1>;
>> +#address-cells = <0>;
>> +
>> +gphy0: eth-gphy@0 {
>> +reg = <0>;
>> +max-speed = <1000>;
>> +};
>> +
>> +gphy1: eth-gphy@1 {
>> +reg = <1>;
>> +max-speed = <1000>;
>> +};
>> +};
>
> Hi Eric
>
> Do these max-speed properties do anything useful? Is the PHY capable
> of > 1Gbps?

Not sure where I had those copy-and-pasted from, but they don't seem
necessary.  Dropped.


signature.asc
Description: PGP signature

Re: [PATCH 2/2] ARM: dts: Add the ethernet and ethernet PHY to the cygnus core DT.

[Andrew Lunn]

> + mdio: mdio@18002000 {
> + compatible = "brcm,iproc-mdio";
> + reg = <0x18002000 0x8>;
> + #size-cells = <1>;
> + #address-cells = <0>;
> +
> + gphy0: eth-gphy@0 {
> + reg = <0>;
> + max-speed = <1000>;
> + };
> +
> + gphy1: eth-gphy@1 {
> + reg = <1>;
> + max-speed = <1000>;
> + };
> + };

Hi Eric

Do these max-speed properties do anything useful? Is the PHY capable
of > 1Gbps?

   Thanks
Andrew

Re: [PATCH 2/2] ARM: dts: Add the ethernet and ethernet PHY to the cygnus core DT.

[Florian Fainelli]

On 04/24/2017 02:50 PM, Eric Anholt wrote:
> Cygnus has a single amac controller connected to the B53 switch with 2
> PHYs.  On the BCM911360_EP platform, those two PHYs are connected to
> the external ethernet jacks.
> 
> Signed-off-by: Eric Anholt 

This looks fine, just a few nits on the label names:

> ---
>  arch/arm/boot/dts/bcm-cygnus.dtsi  | 60 
> ++
>  arch/arm/boot/dts/bcm911360_entphn.dts |  8 +
>  2 files changed, 68 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/bcm-cygnus.dtsi 
> b/arch/arm/boot/dts/bcm-cygnus.dtsi
> index 009f1346b817..318899df9972 100644
> --- a/arch/arm/boot/dts/bcm-cygnus.dtsi
> +++ b/arch/arm/boot/dts/bcm-cygnus.dtsi
> @@ -142,6 +142,56 @@
>   interrupts = <0>;
>   };
>  
> + mdio: mdio@18002000 {
> + compatible = "brcm,iproc-mdio";
> + reg = <0x18002000 0x8>;
> + #size-cells = <1>;
> + #address-cells = <0>;
> +
> + gphy0: eth-gphy@0 {
> + reg = <0>;
> + max-speed = <1000>;
> + };
> +
> + gphy1: eth-gphy@1 {
> + reg = <1>;
> + max-speed = <1000>;
> + };
> + };
> +
> + dsa: dsa@18007000 {

This would be better named switch: switch@18007000

> + compatible = "brcm,bcm11360-srab", "brcm,cygnus-srab";
> + reg = <0x18007000 0x1000>;
> + status = "disabled";
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port0@0 {

You can probably just put port@0

> + reg = <0>;
> + phy-handle = <>;
> + phy-mode = "rgmii";
> + };
> +
> + port1@1 {

And so on

> + reg = <1>;
> + phy-handle = <>;
> + phy-mode = "rgmii";
> + };
> +
> + port8@8 {

And so forth

> + reg = <8>;
> + label = "cpu";
> + ethernet = <>;
> + fixed-link {
> + speed = <1000>;
> + full-duplex;
> + };
> + };
> + };
> + };
> +
>   i2c0: i2c@18008000 {
>   compatible = "brcm,cygnus-iproc-i2c", "brcm,iproc-i2c";
>   reg = <0x18008000 0x100>;
> @@ -295,6 +345,16 @@
>   status = "disabled";
>   };
>  
> + eth0: enet@18042000 {
> + compatible = "brcm,amac";
> + reg = <0x18042000 0x1000>,
> +   <0x1811 0x1000>;
> + reg-names = "amac_base", "idm_base";
> + interrupts = ;
> + max-speed = <1000>;
> + status = "disabled";
> + };
> +
>   nand: nand@18046000 {
>   compatible = "brcm,nand-iproc", "brcm,brcmnand-v6.1";
>   reg = <0x18046000 0x600>, <0xf8105408 0x600>,
> diff --git a/arch/arm/boot/dts/bcm911360_entphn.dts 
> b/arch/arm/boot/dts/bcm911360_entphn.dts
> index 8b3800f46288..2a1f54ab3574 100644
> --- a/arch/arm/boot/dts/bcm911360_entphn.dts
> +++ b/arch/arm/boot/dts/bcm911360_entphn.dts
> @@ -57,6 +57,14 @@
>   };
>  };
>  
> + {
> + status = "okay";
> +};

And that would be  here then.

With that fixed:

Reviewed-by: Florian Fainelli 

> +
> + {
> + status = "okay";
> +};
> +
>   {
>   status = "okay";
>  };
> 


-- 
Florian

[PATCH 2/2] ARM: dts: Add the ethernet and ethernet PHY to the cygnus core DT.

[Eric Anholt]

Cygnus has a single amac controller connected to the B53 switch with 2
PHYs.  On the BCM911360_EP platform, those two PHYs are connected to
the external ethernet jacks.

Signed-off-by: Eric Anholt 
---
 arch/arm/boot/dts/bcm-cygnus.dtsi  | 60 ++
 arch/arm/boot/dts/bcm911360_entphn.dts |  8 +
 2 files changed, 68 insertions(+)

diff --git a/arch/arm/boot/dts/bcm-cygnus.dtsi 
b/arch/arm/boot/dts/bcm-cygnus.dtsi
index 009f1346b817..318899df9972 100644
--- a/arch/arm/boot/dts/bcm-cygnus.dtsi
+++ b/arch/arm/boot/dts/bcm-cygnus.dtsi
@@ -142,6 +142,56 @@
interrupts = <0>;
};
 
+   mdio: mdio@18002000 {
+   compatible = "brcm,iproc-mdio";
+   reg = <0x18002000 0x8>;
+   #size-cells = <1>;
+   #address-cells = <0>;
+
+   gphy0: eth-gphy@0 {
+   reg = <0>;
+   max-speed = <1000>;
+   };
+
+   gphy1: eth-gphy@1 {
+   reg = <1>;
+   max-speed = <1000>;
+   };
+   };
+
+   dsa: dsa@18007000 {
+   compatible = "brcm,bcm11360-srab", "brcm,cygnus-srab";
+   reg = <0x18007000 0x1000>;
+   status = "disabled";
+
+   ports {
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   port0@0 {
+   reg = <0>;
+   phy-handle = <>;
+   phy-mode = "rgmii";
+   };
+
+   port1@1 {
+   reg = <1>;
+   phy-handle = <>;
+   phy-mode = "rgmii";
+   };
+
+   port8@8 {
+   reg = <8>;
+   label = "cpu";
+   ethernet = <>;
+   fixed-link {
+   speed = <1000>;
+   full-duplex;
+   };
+   };
+   };
+   };
+
i2c0: i2c@18008000 {
compatible = "brcm,cygnus-iproc-i2c", "brcm,iproc-i2c";
reg = <0x18008000 0x100>;
@@ -295,6 +345,16 @@
status = "disabled";
};
 
+   eth0: enet@18042000 {
+   compatible = "brcm,amac";
+   reg = <0x18042000 0x1000>,
+ <0x1811 0x1000>;
+   reg-names = "amac_base", "idm_base";
+   interrupts = ;
+   max-speed = <1000>;
+   status = "disabled";
+   };
+
nand: nand@18046000 {
compatible = "brcm,nand-iproc", "brcm,brcmnand-v6.1";
reg = <0x18046000 0x600>, <0xf8105408 0x600>,
diff --git a/arch/arm/boot/dts/bcm911360_entphn.dts 
b/arch/arm/boot/dts/bcm911360_entphn.dts
index 8b3800f46288..2a1f54ab3574 100644
--- a/arch/arm/boot/dts/bcm911360_entphn.dts
+++ b/arch/arm/boot/dts/bcm911360_entphn.dts
@@ -57,6 +57,14 @@
};
 };
 
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
  {
status = "okay";
 };
-- 
2.11.0