mvebu: Network devicetree pin muxing

Dear Ezequiel Garcia,

arch/arm/boot/dts/armada-370.dtsi line 160

Quoted 1 line · Ezequiel Garcia

arch/arm/boot/dts/armada-370.dtsi +1 -0

+				ge0_pins: ge0_pins {

Would it make sense to name that ge0_rgmii_pins ? And ditto for ge1 ?

This commit adds the required pin muxing for the two network interfaces and the MDIO interface, to the Armada 370 SoC.

Signed-off-by Ezequiel Garcia <[email protected]>

---
 arch/arm/boot/dts/armada-370.dtsi | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

arch/arm/boot/dts/armada-370.dtsi +0 -0

index 21b588b..b0700bf6 100644

arch/arm/boot/dts/armada-370.dtsi +15 -0

+				};
+
+				ge0_pins: ge0_pins {
+					marvell,pins = "mpp5", "mpp6", "mpp7", "mpp8",
+						       "mpp9", "mpp10", "mpp11", "mpp12",
+						       "mpp13", "mpp14", "mpp15", "mpp16";
+					marvell,function = "ge0";
+				};
+
+				ge1_pins: ge1_pins {
+					marvell,pins = "mpp19", "mpp20", "mpp21", "mpp22",
+						       "mpp23", "mpp24", "mpp25", "mpp26",
+						       "mpp27", "mpp28", "mpp29", "mpp30";
+					marvell,function = "ge1";
+				};
 			};
 
 			gpio0: gpio at 18100 {

Quoted 13 lines · Ezequiel Garcia

This commit adds the required pin muxing for the two network interfaces and the MDIO interface, to the Armada 370 SoC.

Signed-off-by Ezequiel Garcia <[email protected]>

---
 arch/arm/boot/dts/armada-370.dtsi | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

arch/arm/boot/dts/armada-370.dtsi +0 -0

index 21b588b..b0700bf6 100644

arch/arm/boot/dts/armada-370.dtsi line 155

Quoted 5 lines

arch/arm/boot/dts/armada-370.dtsi +2 -0

@@ -151,6 +151,25 @@
 						       "mpp62", "mpp60", "mpp58";
 					marvell,function = "audio";
 				};
+
+				mdio_pins: mdio_pins {

Ezequiel,

please keep me posted next time ;)

Above should be: mdio_pins: mdio-pins {
same for the ones below.

arch/arm/boot/dts/armada-370.dtsi line 157

Quoted 2 lines

arch/arm/boot/dts/armada-370.dtsi +2 -0

+					marvell,pins = "mpp17", "mpp18";
+					marvell,function = "ge";

As we haven't used it before, we could still rename "ge" function to more approprate "mdio"... but it is ok.

Besides the comments,

Acked-by Sebastian Hesselbarth <[email protected]>

This commit adds the required pin muxing for the two network interfaces and the MDIO interface, to the Armada 370 SoC.

Signed-off-by Ezequiel Garcia <[email protected]>

---
 arch/arm/boot/dts/armada-370.dtsi | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

arch/arm/boot/dts/armada-370.dtsi +0 -0

index 21b588b..b0700bf6 100644

arch/arm/boot/dts/armada-370.dtsi +12 -0

+					marvell,pins = "mpp5", "mpp6", "mpp7", "mpp8",
+						       "mpp9", "mpp10", "mpp11", "mpp12",
+						       "mpp13", "mpp14", "mpp15", "mpp16";
+					marvell,function = "ge0";
+				};
+
+				ge1_pins: ge1_pins {
+					marvell,pins = "mpp19", "mpp20", "mpp21", "mpp22",
+						       "mpp23", "mpp24", "mpp25", "mpp26",
+						       "mpp27", "mpp28", "mpp29", "mpp30";
+					marvell,function = "ge1";
+				};
 			};
 
 			gpio0: gpio at 18100 {

This commit adds the required pin muxing for the network interfaces and the MDIO interface to be properly initialized. For instance, this makes it possible for a bootloader to initialize and access the network interfaces.

Signed-off-by Ezequiel Garcia <[email protected]>

---
 arch/arm/boot/dts/armada-370-mirabox.dts | 6 ++++++
 1 file changed, 6 insertions(+)

arch/arm/boot/dts/armada-370-mirabox.dts +2 -0

index 097df7d8..c745256 100644
@@ -91,6 +91,8 @@
 			};
 
 			mdio {
+				pinctrl-0 = <&mdio_pins>;
+				pinctrl-names = "default";
 				phy0: ethernet-phy at 0 {
 					reg = <0>;
 				};

Quoted 23 lines · Ezequiel Garcia

This commit adds the required pin muxing for the network interfaces and the MDIO interface to be properly initialized. For instance, this makes it possible for a bootloader to initialize and access the network interfaces.

Signed-off-by Ezequiel Garcia <[email protected]>

---
 arch/arm/boot/dts/armada-370-mirabox.dts | 6 ++++++
 1 file changed, 6 insertions(+)

arch/arm/boot/dts/armada-370-mirabox.dts +2 -0

index 097df7d8..c745256 100644
@@ -91,6 +91,8 @@
 			};
 
 			mdio {
+				pinctrl-0 = <&mdio_pins>;
+				pinctrl-names = "default";
 				phy0: ethernet-phy at 0 {
 					reg = <0>;
 				};

arch/arm/boot/dts/armada-370-mirabox.dts line 106

Quoted 5 lines

arch/arm/boot/dts/armada-370-mirabox.dts +2 -0

@@ -100,11 +102,15 @@
 				};
 			};
 			ethernet at 70000 {
+				pinctrl-0 = <&ge0_pins>;
+				pinctrl-names = "default";

Hi Ezequiel

Could you put these one level higher in the DT hierarchy? i.e. once in armada-370.dtsi and not in every board .dts file?

Quoted 32 lines · Andrew Lunn

Quoted 23 lines · Ezequiel Garcia

This commit adds the required pin muxing for the network interfaces and the MDIO interface to be properly initialized. For instance, this makes it possible for a bootloader to initialize and access the network interfaces.

Signed-off-by Ezequiel Garcia <[email protected]>

---
 arch/arm/boot/dts/armada-370-mirabox.dts | 6 ++++++
 1 file changed, 6 insertions(+)

arch/arm/boot/dts/armada-370-mirabox.dts +2 -0

index 097df7d8..c745256 100644
@@ -91,6 +91,8 @@
 			};
 
 			mdio {
+				pinctrl-0 = <&mdio_pins>;
+				pinctrl-names = "default";
 				phy0: ethernet-phy at 0 {
 					reg = <0>;
 				};

arch/arm/boot/dts/armada-370-mirabox.dts line 106

Quoted 5 lines

arch/arm/boot/dts/armada-370-mirabox.dts +2 -0

@@ -100,11 +102,15 @@
 				};
 			};
 			ethernet at 70000 {
+				pinctrl-0 = <&ge0_pins>;
+				pinctrl-names = "default";

Hi Ezequiel

Quoted 3 lines

Could you put these one level higher in the DT hierarchy? i.e. once in armada-370.dtsi and not in every board .dts file?

Are you sure that's the correct approach?

As explained on the cover letter, these MAC MPP mux are not required in the SGMII case. Moreover, this is not the only way to wire the MAC0 and MAC1 controllers, so I even considered pushing down the pinmux to the dts files!

The devicetree location of the pinmux is something that still confuses me, so I'm more than open to directions here.

Quoted 37 lines · Ezequiel Garcia

Quoted 32 lines · Andrew Lunn

Quoted 23 lines · Ezequiel Garcia

This commit adds the required pin muxing for the network interfaces and the MDIO interface to be properly initialized. For instance, this makes it possible for a bootloader to initialize and access the network interfaces.

Signed-off-by Ezequiel Garcia <[email protected]>

---
 arch/arm/boot/dts/armada-370-mirabox.dts | 6 ++++++
 1 file changed, 6 insertions(+)

arch/arm/boot/dts/armada-370-mirabox.dts +2 -0

index 097df7d8..c745256 100644
@@ -91,6 +91,8 @@
 			};
 
 			mdio {
+				pinctrl-0 = <&mdio_pins>;
+				pinctrl-names = "default";
 				phy0: ethernet-phy at 0 {
 					reg = <0>;
 				};

arch/arm/boot/dts/armada-370-mirabox.dts line 106

Quoted 5 lines

arch/arm/boot/dts/armada-370-mirabox.dts +2 -0

@@ -100,11 +102,15 @@
 				};
 			};
 			ethernet at 70000 {
+				pinctrl-0 = <&ge0_pins>;
+				pinctrl-names = "default";

Hi Ezequiel

Quoted 3 lines

Could you put these one level higher in the DT hierarchy? i.e. once in armada-370.dtsi and not in every board .dts file?

Quoted 5 lines

Are you sure that's the correct approach?

As explained on the cover letter, these MAC MPP mux are not required in the SGMII case. Moreover, this is not the only way to wire the MAC0 and MAC1 controllers, so I even considered pushing down the pinmux to the dts files!

Hi Ezequiel

At least at the moment, it seems like RGMII is the norm, and SGMII is the exception. So having the default as RGMII probably makes sense. Boards which don't require it can then override this in there .dts file. If we see this changes with time, we can swap it around.

Quoted 2 lines

The devicetree location of the pinmux is something that still confuses me, so I'm more than open to directions here.

Patch #1 comment is not really correct. It defines a number of pin configuration nodes. These nodes can then be used via phandles. What might help is naming these differently. Name them ge[01]_rgmii_pins. It is then clear when they should be used. You can also add ge[0]_sgmii_pins, which would be an empty set. You can then use ge[01]_rgmii_pins as the default in armada-370.dtsi and override them with ge[0]_sgmii_pins for the 370 RD.

Experience from Kirkwood is that it also has multiple ways to wire up MAC0 and MAC1, but in practice, nobody ever does. And if they did, it can be overridden in the .dts file.

At least at the moment, it seems like RGMII is the norm, and SGMII is the exception. So having the default as RGMII probably makes sense. Boards which don't require it can then override this in there .dts file. If we see this changes with time, we can swap it around.

Quoted 4 lines · Andrew Lunn

At least at the moment, it seems like RGMII is the norm, and SGMII is the exception. So having the default as RGMII probably makes sense. Boards which don't require it can then override this in there .dts file. If we see this changes with time, we can swap it around.

I personally disagree with this approach. armada-370.dtsi is here to describe what happens at the *SoC* level, not to factorize some random board specific details that appear to be common between a certain number of boards (but not all).

So I really, really, would prefer to keep the board-specific details such as which pin muxing is done for Ethernet in each individual .dts file.

.dtsi to describe the SoC, .dts to describe the board. It's clear and simple for everyone to understand, especially for new comers.

Dear Andrew Lunn,

Quoted 4 lines · Andrew Lunn

At least at the moment, it seems like RGMII is the norm, and SGMII is the exception. So having the default as RGMII probably makes sense. Boards which don't require it can then override this in there .dts file. If we see this changes with time, we can swap it around.

I personally disagree with this approach. armada-370.dtsi is here to describe what happens at the *SoC* level, not to factorize some random board specific details that appear to be common between a certain number of boards (but not all).

So I really, really, would prefer to keep the board-specific details such as which pin muxing is done for Ethernet in each individual .dts file.

.dtsi to describe the SoC, .dts to describe the board. It's clear and simple for everyone to understand, especially for new comers.

Quoted 4 lines

I personally disagree with this approach. armada-370.dtsi is here to describe what happens at the *SoC* level, not to factorize some random board specific details that appear to be common between a certain number of boards (but not all).

It's my understanding that *possible* pinmux configurations are a characteristic of the SoC. Which configurations are used is an artifact of the board.

I don't see the need to list all possible configurations in the SoC dtsi file, but just the ones we have actually seen used.

Quoted 3 lines

So I really, really, would prefer to keep the board-specific details such as which pin muxing is done for Ethernet in each individual .dts file.

So, wouldn't this be the phandle selection?

Quoted 2 lines

.dtsi to describe the SoC, .dts to describe the board. It's clear and simple for everyone to understand, especially for new comers.

If you're strongly opposed to describing possible pinmux configurations in the SoC dtsi file, how about an SoC-pinmux.dtsi? I just see the "Thar be dragons ahead" sign by describing pinmux in the board files.

However, Andrew was suggesting to move the phandle selection to the .dtsi file, with the argument that most boards use RGMII, so it should be the default. That's what I disagreed with.