[PATCH] arm64: dts: marvell: espressobin: Add ethernet switch aliases

[PATCH] arm64: dts: marvell: espressobin: Add ethernet switch aliases

[Pali Rohár]

Espressobin boards have 3 ethernet ports and some of them got assigned more
then one MAC address. MAC addresses are stored in U-Boot environment.

Since commit a2c7023f7075c ("net: dsa: read mac address from DT for slave
device") kernel can use MAC addresses from DT for particular DSA port.

Currently Espressobin DTS file contains alias just for ethernet0.

This patch defines additional ethernet aliases in Espressobin DTS files, so
bootloader can fill correct MAC address for DSA switch ports if more MAC
addresses were specified.

DT alias ethernet1 is used for wan port, DT aliases ethernet2 and ethernet3
are used for lan ports for both Espressobin revisions (V5 and V7).

Fixes: 5253cb8c00a6f ("arm64: dts: marvell: espressobin: add ethernet alias")
Signed-off-by: Pali Rohár <pali@kernel.org>
---
 .../dts/marvell/armada-3720-espressobin-v7-emmc.dts  | 10 ++++++++--
 .../boot/dts/marvell/armada-3720-espressobin-v7.dts  | 10 ++++++++--
 .../boot/dts/marvell/armada-3720-espressobin.dtsi    | 12 ++++++++----
 3 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/boot/dts/marvell/armada-3720-espressobin-v7-emmc.dts b/arch/arm64/boot/dts/marvell/armada-3720-espressobin-v7-emmc.dts
index 03733fd92732..215d2f702623 100644
--- a/arch/arm64/boot/dts/marvell/armada-3720-espressobin-v7-emmc.dts
+++ b/arch/arm64/boot/dts/marvell/armada-3720-espressobin-v7-emmc.dts
@@ -20,17 +20,23 @@
 	compatible = "globalscale,espressobin-v7-emmc", "globalscale,espressobin-v7",
 		     "globalscale,espressobin", "marvell,armada3720",
 		     "marvell,armada3710";
+
+	aliases {
+		/* ethernet1 is wan port */
+		ethernet1 = &switch0port3;
+		ethernet3 = &switch0port1;
+	};
 };
 
 &switch0 {
 	ports {
-		port@1 {
+		switch0port1: port@1 {
 			reg = <1>;
 			label = "lan1";
 			phy-handle = <&switch0phy0>;
 		};
 
-		port@3 {
+		switch0port3: port@3 {
 			reg = <3>;
 			label = "wan";
 			phy-handle = <&switch0phy2>;
diff --git a/arch/arm64/boot/dts/marvell/armada-3720-espressobin-v7.dts b/arch/arm64/boot/dts/marvell/armada-3720-espressobin-v7.dts
index 8570c5f47d7d..b6f4af8ebafb 100644
--- a/arch/arm64/boot/dts/marvell/armada-3720-espressobin-v7.dts
+++ b/arch/arm64/boot/dts/marvell/armada-3720-espressobin-v7.dts
@@ -19,17 +19,23 @@
 	model = "Globalscale Marvell ESPRESSOBin Board V7";
 	compatible = "globalscale,espressobin-v7", "globalscale,espressobin",
 		     "marvell,armada3720", "marvell,armada3710";
+
+	aliases {
+		/* ethernet1 is wan port */
+		ethernet1 = &switch0port3;
+		ethernet3 = &switch0port1;
+	};
 };
 
 &switch0 {
 	ports {
-		port@1 {
+		switch0port1: port@1 {
 			reg = <1>;
 			label = "lan1";
 			phy-handle = <&switch0phy0>;
 		};
 
-		port@3 {
+		switch0port3: port@3 {
 			reg = <3>;
 			label = "wan";
 			phy-handle = <&switch0phy2>;
diff --git a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi
index b97218c72727..0775c16e0ec8 100644
--- a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi
@@ -13,6 +13,10 @@
 / {
 	aliases {
 		ethernet0 = &eth0;
+		/* for dsa slave device */
+		ethernet1 = &switch0port1;
+		ethernet2 = &switch0port2;
+		ethernet3 = &switch0port3;
 		serial0 = &uart0;
 		serial1 = &uart1;
 	};
@@ -120,7 +124,7 @@
 			#address-cells = <1>;
 			#size-cells = <0>;
 
-			port@0 {
+			switch0port0: port@0 {
 				reg = <0>;
 				label = "cpu";
 				ethernet = <&eth0>;
@@ -131,19 +135,19 @@
 				};
 			};
 
-			port@1 {
+			switch0port1: port@1 {
 				reg = <1>;
 				label = "wan";
 				phy-handle = <&switch0phy0>;
 			};
 
-			port@2 {
+			switch0port2: port@2 {
 				reg = <2>;
 				label = "lan0";
 				phy-handle = <&switch0phy1>;
 			};
 
-			port@3 {
+			switch0port3: port@3 {
 				reg = <3>;
 				label = "lan1";
 				phy-handle = <&switch0phy2>;
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: dts: marvell: espressobin: Add ethernet switch aliases

[Andrew Lunn]

On Mon, Sep 07, 2020 at 01:27:17PM +0200, Pali Rohár wrote:
> Espressobin boards have 3 ethernet ports and some of them got assigned more
> then one MAC address. MAC addresses are stored in U-Boot environment.
> 
> Since commit a2c7023f7075c ("net: dsa: read mac address from DT for slave
> device") kernel can use MAC addresses from DT for particular DSA port.
> 
> Currently Espressobin DTS file contains alias just for ethernet0.
> 
> This patch defines additional ethernet aliases in Espressobin DTS files, so
> bootloader can fill correct MAC address for DSA switch ports if more MAC
> addresses were specified.
> 
> DT alias ethernet1 is used for wan port, DT aliases ethernet2 and ethernet3
> are used for lan ports for both Espressobin revisions (V5 and V7).
> 
> Fixes: 5253cb8c00a6f ("arm64: dts: marvell: espressobin: add ethernet alias")
> Signed-off-by: Pali Rohár <pali@kernel.org>

I'm not sure a Fixes: is appropriate here. What is actually broken?
This just seems like a new feature.

    Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: dts: marvell: espressobin: Add ethernet switch aliases

[Pali Rohár]

On Monday 07 September 2020 16:42:28 Andrew Lunn wrote:
> On Mon, Sep 07, 2020 at 01:27:17PM +0200, Pali Rohár wrote:
> > Espressobin boards have 3 ethernet ports and some of them got assigned more
> > then one MAC address. MAC addresses are stored in U-Boot environment.
> > 
> > Since commit a2c7023f7075c ("net: dsa: read mac address from DT for slave
> > device") kernel can use MAC addresses from DT for particular DSA port.
> > 
> > Currently Espressobin DTS file contains alias just for ethernet0.
> > 
> > This patch defines additional ethernet aliases in Espressobin DTS files, so
> > bootloader can fill correct MAC address for DSA switch ports if more MAC
> > addresses were specified.
> > 
> > DT alias ethernet1 is used for wan port, DT aliases ethernet2 and ethernet3
> > are used for lan ports for both Espressobin revisions (V5 and V7).
> > 
> > Fixes: 5253cb8c00a6f ("arm64: dts: marvell: espressobin: add ethernet alias")
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> 
> I'm not sure a Fixes: is appropriate here. What is actually broken?
> This just seems like a new feature.

Hello Andrew! With "fixes" I mean that this patch fixes aliases list in
that mentioned commit as it was incomplete. Probably better would be
"related" or "extended" keyword in this case. But I do not know.

I would not say it is a "new feature". But rather that patch in this
email fixes issue that Linux kernel did not set correct MAC address for
DSA slave ports. I think it is something which could be backported also
to stable releases as "ignoring" vendor/factory MAC address is not
correct behavior.

If you have an idea how to reformulate commit description I will do it.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: dts: marvell: espressobin: Add ethernet switch aliases

[Andrew Lunn]

> I would not say it is a "new feature". But rather that patch in this
> email fixes issue that Linux kernel did not set correct MAC address for
> DSA slave ports. I think it is something which could be backported also
> to stable releases as "ignoring" vendor/factory MAC address is not
> correct behavior.

Hi Pali

The rules for stable are here:

https://www.kernel.org/doc/html/v5.8/process/stable-kernel-rules.html

Do you think it fits?

   Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: dts: marvell: espressobin: Add ethernet switch aliases

[Pali Rohár]

On Monday 07 September 2020 17:43:53 Andrew Lunn wrote:
> > I would not say it is a "new feature". But rather that patch in this
> > email fixes issue that Linux kernel did not set correct MAC address for
> > DSA slave ports. I think it is something which could be backported also
> > to stable releases as "ignoring" vendor/factory MAC address is not
> > correct behavior.
> 
> Hi Pali
> 
> The rules for stable are here:
> 
> https://www.kernel.org/doc/html/v5.8/process/stable-kernel-rules.html
> 
> Do you think it fits?
> 
>    Andrew

Hello Andrew! I think it fits into those rules. As I wrote it fixes real
bug that Linux kernel does not use correct MAC address for particular
DSA slaves / ethernet ports. But if you or other people have opposite
opinion I will of course respect it.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: dts: marvell: espressobin: Add ethernet switch aliases

[Andre Heider]

Hi Pali,

On 07/09/2020 13:27, Pali Rohár wrote:
> Espressobin boards have 3 ethernet ports and some of them got assigned more
> then one MAC address. MAC addresses are stored in U-Boot environment.
> 
> Since commit a2c7023f7075c ("net: dsa: read mac address from DT for slave
> device") kernel can use MAC addresses from DT for particular DSA port.
> 
> Currently Espressobin DTS file contains alias just for ethernet0.
> 
> This patch defines additional ethernet aliases in Espressobin DTS files, so
> bootloader can fill correct MAC address for DSA switch ports if more MAC
> addresses were specified.
> 
> DT alias ethernet1 is used for wan port, DT aliases ethernet2 and ethernet3
> are used for lan ports for both Espressobin revisions (V5 and V7).
> 
> Fixes: 5253cb8c00a6f ("arm64: dts: marvell: espressobin: add ethernet alias")
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>   .../dts/marvell/armada-3720-espressobin-v7-emmc.dts  | 10 ++++++++--
>   .../boot/dts/marvell/armada-3720-espressobin-v7.dts  | 10 ++++++++--
>   .../boot/dts/marvell/armada-3720-espressobin.dtsi    | 12 ++++++++----
>   3 files changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/marvell/armada-3720-espressobin-v7-emmc.dts b/arch/arm64/boot/dts/marvell/armada-3720-espressobin-v7-emmc.dts
> index 03733fd92732..215d2f702623 100644
> --- a/arch/arm64/boot/dts/marvell/armada-3720-espressobin-v7-emmc.dts
> +++ b/arch/arm64/boot/dts/marvell/armada-3720-espressobin-v7-emmc.dts
> @@ -20,17 +20,23 @@
>   	compatible = "globalscale,espressobin-v7-emmc", "globalscale,espressobin-v7",
>   		     "globalscale,espressobin", "marvell,armada3720",
>   		     "marvell,armada3710";
> +
> +	aliases {
> +		/* ethernet1 is wan port */
> +		ethernet1 = &switch0port3;
> +		ethernet3 = &switch0port1;
> +	};
>   };
>   
>   &switch0 {
>   	ports {
> -		port@1 {
> +		switch0port1: port@1 {
>   			reg = <1>;
>   			label = "lan1";
>   			phy-handle = <&switch0phy0>;
>   		};
>   
> -		port@3 {
> +		switch0port3: port@3 {
>   			reg = <3>;
>   			label = "wan";
>   			phy-handle = <&switch0phy2>;

My dts-foo is a little rusty, but now that you labeled the ports in the 
.dtsi, can this whole "switch0" block reduced to something like:

&switch0port1 {
	label = "lan1";
};

&switch0port3 {
	label = "wan";
};

?

> diff --git a/arch/arm64/boot/dts/marvell/armada-3720-espressobin-v7.dts b/arch/arm64/boot/dts/marvell/armada-3720-espressobin-v7.dts
> index 8570c5f47d7d..b6f4af8ebafb 100644
> --- a/arch/arm64/boot/dts/marvell/armada-3720-espressobin-v7.dts
> +++ b/arch/arm64/boot/dts/marvell/armada-3720-espressobin-v7.dts
> @@ -19,17 +19,23 @@
>   	model = "Globalscale Marvell ESPRESSOBin Board V7";
>   	compatible = "globalscale,espressobin-v7", "globalscale,espressobin",
>   		     "marvell,armada3720", "marvell,armada3710";
> +
> +	aliases {
> +		/* ethernet1 is wan port */
> +		ethernet1 = &switch0port3;
> +		ethernet3 = &switch0port1;
> +	};
>   };
>   
>   &switch0 {
>   	ports {
> -		port@1 {
> +		switch0port1: port@1 {
>   			reg = <1>;
>   			label = "lan1";
>   			phy-handle = <&switch0phy0>;
>   		};
>   
> -		port@3 {
> +		switch0port3: port@3 {
>   			reg = <3>;
>   			label = "wan";
>   			phy-handle = <&switch0phy2>;
> diff --git a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi
> index b97218c72727..0775c16e0ec8 100644
> --- a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi
> +++ b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi
> @@ -13,6 +13,10 @@
>   / {
>   	aliases {
>   		ethernet0 = &eth0;
> +		/* for dsa slave device */
> +		ethernet1 = &switch0port1;
> +		ethernet2 = &switch0port2;
> +		ethernet3 = &switch0port3;
>   		serial0 = &uart0;
>   		serial1 = &uart1;
>   	};
> @@ -120,7 +124,7 @@
>   			#address-cells = <1>;
>   			#size-cells = <0>;
>   
> -			port@0 {
> +			switch0port0: port@0 {

This label is unused it seems.

Regards,
Andre

>   				reg = <0>;
>   				label = "cpu";
>   				ethernet = <&eth0>;
> @@ -131,19 +135,19 @@
>   				};
>   			};
>   
> -			port@1 {
> +			switch0port1: port@1 {
>   				reg = <1>;
>   				label = "wan";
>   				phy-handle = <&switch0phy0>;
>   			};
>   
> -			port@2 {
> +			switch0port2: port@2 {
>   				reg = <2>;
>   				label = "lan0";
>   				phy-handle = <&switch0phy1>;
>   			};
>   
> -			port@3 {
> +			switch0port3: port@3 {
>   				reg = <3>;
>   				label = "lan1";
>   				phy-handle = <&switch0phy2>;
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: dts: marvell: espressobin: Add ethernet switch aliases

[Andrew Lunn]

> My dts-foo is a little rusty, but now that you labeled the ports in the
> .dtsi, can this whole "switch0" block reduced to something like:
> 
> &switch0port1 {
> 	label = "lan1";
> };
> 
> &switch0port3 {
> 	label = "wan";
> };

Probably yes.

But that is definitely too much for stable.

    Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: dts: marvell: espressobin: Add ethernet switch aliases

[Andrew Lunn]

On Mon, Sep 07, 2020 at 06:13:16PM +0200, Pali Rohár wrote:
> On Monday 07 September 2020 17:43:53 Andrew Lunn wrote:
> > > I would not say it is a "new feature". But rather that patch in this
> > > email fixes issue that Linux kernel did not set correct MAC address for
> > > DSA slave ports. I think it is something which could be backported also
> > > to stable releases as "ignoring" vendor/factory MAC address is not
> > > correct behavior.
> > 
> > Hi Pali
> > 
> > The rules for stable are here:
> > 
> > https://www.kernel.org/doc/html/v5.8/process/stable-kernel-rules.html
> > 
> > Do you think it fits?
> > 
> >    Andrew
> 
> Hello Andrew! I think it fits into those rules. As I wrote it fixes real
> bug that Linux kernel does not use correct MAC address for particular
> DSA slaves / ethernet ports.

O.K, then:

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: dts: marvell: espressobin: Add ethernet switch aliases

[Pali Rohár]

On Monday 07 September 2020 19:23:03 Andrew Lunn wrote:
> > My dts-foo is a little rusty, but now that you labeled the ports in the
> > .dtsi, can this whole "switch0" block reduced to something like:
> > 
> > &switch0port1 {
> > 	label = "lan1";
> > };
> > 
> > &switch0port3 {
> > 	label = "wan";
> > };
> 
> Probably yes.
> 
> But that is definitely too much for stable.

Yes, this suggested change is not for stable, but looks like a nice
cleanup. So it could be done in followup patch.

Andre, are you going to prepare and test this followup change?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: dts: marvell: espressobin: Add ethernet switch aliases

[Pali Rohár]

On Monday 07 September 2020 19:13:41 Andre Heider wrote:
> > @@ -120,7 +124,7 @@
> >   			#address-cells = <1>;
> >   			#size-cells = <0>;
> > -			port@0 {
> > +			switch0port0: port@0 {
> 
> This label is unused it seems.

Yes, it is unused, but I defined labels for all ports so it would be
clean that ports are indexed from zero and not from one. Also it looks
inconsistent if some of DSA ports have labels and some does not.

> 
> >   				reg = <0>;
> >   				label = "cpu";
> >   				ethernet = <&eth0>;
> > @@ -131,19 +135,19 @@
> >   				};
> >   			};
> > -			port@1 {
> > +			switch0port1: port@1 {
> >   				reg = <1>;
> >   				label = "wan";
> >   				phy-handle = <&switch0phy0>;
> >   			};
> > -			port@2 {
> > +			switch0port2: port@2 {
> >   				reg = <2>;
> >   				label = "lan0";
> >   				phy-handle = <&switch0phy1>;
> >   			};
> > -			port@3 {
> > +			switch0port3: port@3 {
> >   				reg = <3>;
> >   				label = "lan1";
> >   				phy-handle = <&switch0phy2>;
> > 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: dts: marvell: espressobin: Add ethernet switch aliases

[Andre Heider]

On 07/09/2020 19:35, Pali Rohár wrote:
> On Monday 07 September 2020 19:23:03 Andrew Lunn wrote:
>>> My dts-foo is a little rusty, but now that you labeled the ports in the
>>> .dtsi, can this whole "switch0" block reduced to something like:
>>>
>>> &switch0port1 {
>>> 	label = "lan1";
>>> };
>>>
>>> &switch0port3 {
>>> 	label = "wan";
>>> };
>>
>> Probably yes.
>>
>> But that is definitely too much for stable.
> 
> Yes, this suggested change is not for stable, but looks like a nice
> cleanup. So it could be done in followup patch.
> 
> Andre, are you going to prepare and test this followup change?

I can prep the patch if you like, but the suggested cleanup only affects 
the v7 dts files. I don't have that hardware version to test it, so 
could only send an untested patch.

Regards,
Andre


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: dts: marvell: espressobin: Add ethernet switch aliases

[Andre Heider]

On 07/09/2020 19:42, Pali Rohár wrote:
> On Monday 07 September 2020 19:13:41 Andre Heider wrote:
>>> @@ -120,7 +124,7 @@
>>>    			#address-cells = <1>;
>>>    			#size-cells = <0>;
>>> -			port@0 {
>>> +			switch0port0: port@0 {
>>
>> This label is unused it seems.
> 
> Yes, it is unused, but I defined labels for all ports so it would be
> clean that ports are indexed from zero and not from one. Also it looks
> inconsistent if some of DSA ports have labels and some does not.

Alright, sounds good:
Reviewed-by: Andre Heider <a.heider@gmail.com>

> 
>>
>>>    				reg = <0>;
>>>    				label = "cpu";
>>>    				ethernet = <&eth0>;
>>> @@ -131,19 +135,19 @@
>>>    				};
>>>    			};
>>> -			port@1 {
>>> +			switch0port1: port@1 {
>>>    				reg = <1>;
>>>    				label = "wan";
>>>    				phy-handle = <&switch0phy0>;
>>>    			};
>>> -			port@2 {
>>> +			switch0port2: port@2 {
>>>    				reg = <2>;
>>>    				label = "lan0";
>>>    				phy-handle = <&switch0phy1>;
>>>    			};
>>> -			port@3 {
>>> +			switch0port3: port@3 {
>>>    				reg = <3>;
>>>    				label = "lan1";
>>>    				phy-handle = <&switch0phy2>;
>>>
>>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: dts: marvell: espressobin: Add ethernet switch aliases

[Pali Rohár]

On Monday 07 September 2020 19:43:08 Andre Heider wrote:
> On 07/09/2020 19:35, Pali Rohár wrote:
> > On Monday 07 September 2020 19:23:03 Andrew Lunn wrote:
> > > > My dts-foo is a little rusty, but now that you labeled the ports in the
> > > > .dtsi, can this whole "switch0" block reduced to something like:
> > > > 
> > > > &switch0port1 {
> > > > 	label = "lan1";
> > > > };
> > > > 
> > > > &switch0port3 {
> > > > 	label = "wan";
> > > > };
> > > 
> > > Probably yes.
> > > 
> > > But that is definitely too much for stable.
> > 
> > Yes, this suggested change is not for stable, but looks like a nice
> > cleanup. So it could be done in followup patch.
> > 
> > Andre, are you going to prepare and test this followup change?
> 
> I can prep the patch if you like, but the suggested cleanup only affects the
> v7 dts files. I don't have that hardware version to test it, so could only
> send an untested patch.

As a result of this cleanup should be binary DTB file for V7 with same
structure as DTB file without such cleanup patch, right? And this test
(structure / content of compiled file) does not need particular hardware.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: dts: marvell: espressobin: Add ethernet switch aliases

[Andre Heider]

On 07/09/2020 19:47, Pali Rohár wrote:
> On Monday 07 September 2020 19:43:08 Andre Heider wrote:
>> On 07/09/2020 19:35, Pali Rohár wrote:
>>> On Monday 07 September 2020 19:23:03 Andrew Lunn wrote:
>>>>> My dts-foo is a little rusty, but now that you labeled the ports in the
>>>>> .dtsi, can this whole "switch0" block reduced to something like:
>>>>>
>>>>> &switch0port1 {
>>>>> 	label = "lan1";
>>>>> };
>>>>>
>>>>> &switch0port3 {
>>>>> 	label = "wan";
>>>>> };
>>>>
>>>> Probably yes.
>>>>
>>>> But that is definitely too much for stable.
>>>
>>> Yes, this suggested change is not for stable, but looks like a nice
>>> cleanup. So it could be done in followup patch.
>>>
>>> Andre, are you going to prepare and test this followup change?
>>
>> I can prep the patch if you like, but the suggested cleanup only affects the
>> v7 dts files. I don't have that hardware version to test it, so could only
>> send an untested patch.
> 
> As a result of this cleanup should be binary DTB file for V7 with same
> structure as DTB file without such cleanup patch, right? And this test
> (structure / content of compiled file) does not need particular hardware.

Ok, will do.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: dts: marvell: espressobin: Add ethernet switch aliases

[Andrew Lunn]

> As a result of this cleanup should be binary DTB file for V7 with same
> structure as DTB file without such cleanup patch, right?

Should be. If need be, you can decompile the DTB back to a DTS and
make sure it looks correct.

     Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: dts: marvell: espressobin: Add ethernet switch aliases

[Pali Rohár]

On Monday 07 September 2020 19:23:45 Andrew Lunn wrote:
> On Mon, Sep 07, 2020 at 06:13:16PM +0200, Pali Rohár wrote:
> > On Monday 07 September 2020 17:43:53 Andrew Lunn wrote:
> > > > I would not say it is a "new feature". But rather that patch in this
> > > > email fixes issue that Linux kernel did not set correct MAC address for
> > > > DSA slave ports. I think it is something which could be backported also
> > > > to stable releases as "ignoring" vendor/factory MAC address is not
> > > > correct behavior.
> > > 
> > > Hi Pali
> > > 
> > > The rules for stable are here:
> > > 
> > > https://www.kernel.org/doc/html/v5.8/process/stable-kernel-rules.html
> > > 
> > > Do you think it fits?
> > > 
> > >    Andrew
> > 
> > Hello Andrew! I think it fits into those rules. As I wrote it fixes real
> > bug that Linux kernel does not use correct MAC address for particular
> > DSA slaves / ethernet ports.
> 
> O.K, then:
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> 
>     Andrew

Ok! Andrew, I would like to ask another question, how to correctly
define that this patch depends on a2c7023f7075c? I specified it in
human-readable part of commit description, but for backporting it would
also need some machine-readable format. So patch would not be
occasionally backported to older/stable kernel where a2c7023f7075c is
not available.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: dts: marvell: espressobin: Add ethernet switch aliases

[Gregory CLEMENT]

Hello Pali,

> Espressobin boards have 3 ethernet ports and some of them got assigned more
> then one MAC address. MAC addresses are stored in U-Boot environment.
>
> Since commit a2c7023f7075c ("net: dsa: read mac address from DT for slave
> device") kernel can use MAC addresses from DT for particular DSA port.
>
> Currently Espressobin DTS file contains alias just for ethernet0.
>
> This patch defines additional ethernet aliases in Espressobin DTS files, so
> bootloader can fill correct MAC address for DSA switch ports if more MAC
> addresses were specified.
>
> DT alias ethernet1 is used for wan port, DT aliases ethernet2 and ethernet3
> are used for lan ports for both Espressobin revisions (V5 and V7).
>
> Fixes: 5253cb8c00a6f ("arm64: dts: marvell: espressobin: add ethernet alias")
> Signed-off-by: Pali Rohár <pali@kernel.org>


Applied on mvebu/fixes

Thanks,

Gregory

> ---
>  .../dts/marvell/armada-3720-espressobin-v7-emmc.dts  | 10 ++++++++--
>  .../boot/dts/marvell/armada-3720-espressobin-v7.dts  | 10 ++++++++--
>  .../boot/dts/marvell/armada-3720-espressobin.dtsi    | 12 ++++++++----
>  3 files changed, 24 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/marvell/armada-3720-espressobin-v7-emmc.dts b/arch/arm64/boot/dts/marvell/armada-3720-espressobin-v7-emmc.dts
> index 03733fd92732..215d2f702623 100644
> --- a/arch/arm64/boot/dts/marvell/armada-3720-espressobin-v7-emmc.dts
> +++ b/arch/arm64/boot/dts/marvell/armada-3720-espressobin-v7-emmc.dts
> @@ -20,17 +20,23 @@
>  	compatible = "globalscale,espressobin-v7-emmc", "globalscale,espressobin-v7",
>  		     "globalscale,espressobin", "marvell,armada3720",
>  		     "marvell,armada3710";
> +
> +	aliases {
> +		/* ethernet1 is wan port */
> +		ethernet1 = &switch0port3;
> +		ethernet3 = &switch0port1;
> +	};
>  };
>  
>  &switch0 {
>  	ports {
> -		port@1 {
> +		switch0port1: port@1 {
>  			reg = <1>;
>  			label = "lan1";
>  			phy-handle = <&switch0phy0>;
>  		};
>  
> -		port@3 {
> +		switch0port3: port@3 {
>  			reg = <3>;
>  			label = "wan";
>  			phy-handle = <&switch0phy2>;
> diff --git a/arch/arm64/boot/dts/marvell/armada-3720-espressobin-v7.dts b/arch/arm64/boot/dts/marvell/armada-3720-espressobin-v7.dts
> index 8570c5f47d7d..b6f4af8ebafb 100644
> --- a/arch/arm64/boot/dts/marvell/armada-3720-espressobin-v7.dts
> +++ b/arch/arm64/boot/dts/marvell/armada-3720-espressobin-v7.dts
> @@ -19,17 +19,23 @@
>  	model = "Globalscale Marvell ESPRESSOBin Board V7";
>  	compatible = "globalscale,espressobin-v7", "globalscale,espressobin",
>  		     "marvell,armada3720", "marvell,armada3710";
> +
> +	aliases {
> +		/* ethernet1 is wan port */
> +		ethernet1 = &switch0port3;
> +		ethernet3 = &switch0port1;
> +	};
>  };
>  
>  &switch0 {
>  	ports {
> -		port@1 {
> +		switch0port1: port@1 {
>  			reg = <1>;
>  			label = "lan1";
>  			phy-handle = <&switch0phy0>;
>  		};
>  
> -		port@3 {
> +		switch0port3: port@3 {
>  			reg = <3>;
>  			label = "wan";
>  			phy-handle = <&switch0phy2>;
> diff --git a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi
> index b97218c72727..0775c16e0ec8 100644
> --- a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi
> +++ b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi
> @@ -13,6 +13,10 @@
>  / {
>  	aliases {
>  		ethernet0 = &eth0;
> +		/* for dsa slave device */
> +		ethernet1 = &switch0port1;
> +		ethernet2 = &switch0port2;
> +		ethernet3 = &switch0port3;
>  		serial0 = &uart0;
>  		serial1 = &uart1;
>  	};
> @@ -120,7 +124,7 @@
>  			#address-cells = <1>;
>  			#size-cells = <0>;
>  
> -			port@0 {
> +			switch0port0: port@0 {
>  				reg = <0>;
>  				label = "cpu";
>  				ethernet = <&eth0>;
> @@ -131,19 +135,19 @@
>  				};
>  			};
>  
> -			port@1 {
> +			switch0port1: port@1 {
>  				reg = <1>;
>  				label = "wan";
>  				phy-handle = <&switch0phy0>;
>  			};
>  
> -			port@2 {
> +			switch0port2: port@2 {
>  				reg = <2>;
>  				label = "lan0";
>  				phy-handle = <&switch0phy1>;
>  			};
>  
> -			port@3 {
> +			switch0port3: port@3 {
>  				reg = <3>;
>  				label = "lan1";
>  				phy-handle = <&switch0phy2>;
> -- 
> 2.20.1
>

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: dts: marvell: espressobin: Add ethernet switch aliases

[Pali Rohár]

On Tuesday 08 September 2020 09:47:33 Pali Rohár wrote:
> On Monday 07 September 2020 19:23:45 Andrew Lunn wrote:
> > On Mon, Sep 07, 2020 at 06:13:16PM +0200, Pali Rohár wrote:
> > > On Monday 07 September 2020 17:43:53 Andrew Lunn wrote:
> > > > > I would not say it is a "new feature". But rather that patch in this
> > > > > email fixes issue that Linux kernel did not set correct MAC address for
> > > > > DSA slave ports. I think it is something which could be backported also
> > > > > to stable releases as "ignoring" vendor/factory MAC address is not
> > > > > correct behavior.
> > > > 
> > > > Hi Pali
> > > > 
> > > > The rules for stable are here:
> > > > 
> > > > https://www.kernel.org/doc/html/v5.8/process/stable-kernel-rules.html
> > > > 
> > > > Do you think it fits?
> > > > 
> > > >    Andrew
> > > 
> > > Hello Andrew! I think it fits into those rules. As I wrote it fixes real
> > > bug that Linux kernel does not use correct MAC address for particular
> > > DSA slaves / ethernet ports.
> > 
> > O.K, then:
> > 
> > Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> > 
> >     Andrew
> 
> Ok! Andrew, I would like to ask another question, how to correctly
> define that this patch depends on a2c7023f7075c? I specified it in
> human-readable part of commit description, but for backporting it would
> also need some machine-readable format. So patch would not be
> occasionally backported to older/stable kernel where a2c7023f7075c is
> not available.

Based on stable-kernel-rules.html document I think that following line
should define this dependency in machine readable format:

Cc: <stable@vger.kernel.org> # a2c7023f7075c: dsa: read mac address

Gregory, if it is correct, would you add that line into commit sign-off
area where is existing Fixes: line?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: dts: marvell: espressobin: Add ethernet switch aliases

[Gregory CLEMENT]

Hi Pali,

> On Tuesday 08 September 2020 09:47:33 Pali Rohár wrote:
>> On Monday 07 September 2020 19:23:45 Andrew Lunn wrote:
>> > On Mon, Sep 07, 2020 at 06:13:16PM +0200, Pali Rohár wrote:
>> > > On Monday 07 September 2020 17:43:53 Andrew Lunn wrote:
>> > > > > I would not say it is a "new feature". But rather that patch in this
>> > > > > email fixes issue that Linux kernel did not set correct MAC address for
>> > > > > DSA slave ports. I think it is something which could be backported also
>> > > > > to stable releases as "ignoring" vendor/factory MAC address is not
>> > > > > correct behavior.
>> > > > 
>> > > > Hi Pali
>> > > > 
>> > > > The rules for stable are here:
>> > > > 
>> > > > https://www.kernel.org/doc/html/v5.8/process/stable-kernel-rules.html
>> > > > 
>> > > > Do you think it fits?
>> > > > 
>> > > >    Andrew
>> > > 
>> > > Hello Andrew! I think it fits into those rules. As I wrote it fixes real
>> > > bug that Linux kernel does not use correct MAC address for particular
>> > > DSA slaves / ethernet ports.
>> > 
>> > O.K, then:
>> > 
>> > Reviewed-by: Andrew Lunn <andrew@lunn.ch>
>> > 
>> >     Andrew
>> 
>> Ok! Andrew, I would like to ask another question, how to correctly
>> define that this patch depends on a2c7023f7075c? I specified it in
>> human-readable part of commit description, but for backporting it would
>> also need some machine-readable format. So patch would not be
>> occasionally backported to older/stable kernel where a2c7023f7075c is
>> not available.
>
> Based on stable-kernel-rules.html document I think that following line
> should define this dependency in machine readable format:
>
> Cc: <stable@vger.kernel.org> # a2c7023f7075c: dsa: read mac address
>
> Gregory, if it is correct, would you add that line into commit sign-off
> area where is existing Fixes: line?

I amended the commit log with this change.

Thanks,

Gregory

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel