arm64: dts: marvell: 8040-mcbin: Enable 1GB Ethernet

PATCH New Marc Zyngier 4 participants 14 messages Jun 5, 2017 – Jun 6, 2017

Subsystems (3)

Device tree bindings Arm/Marvell Kirkwood and Armada 370, 375, 38x, 39x, XP, 3700, 7K/8K, CN9130 SoC support Marvell macchiatobin support

lore

Share & tools

lore

Marc Zyngier

author

2017-06-05 20:01 UTC #0 lore raw

PATCH 1/1 arm64: dts: marvell: 8040-mcbin: Enable 1GB Ethernet

Enable the 1GB Ethernet interface that lives on the slave CP110, with its corresponding phy (that oddly lives on the master CP110).

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts +16 -0

index f7bb0cc03147..1eb3cd5332c1 100644
@@ -100,6 +100,12 @@
 	status = "okay";
 };
 
+&cpm_mdio {
+	phy0: ethernet-phy at 0 {
+		reg = <0>;
+	};
+};
+
 &cpm_sata0 {
 	/* CPM Lane 0 - U29 */
 	status = "okay";

@@ -115,6 +121,16 @@
 	status = "okay";
 };
 
+&cps_ethernet {
+	status = "okay";
+};
+
+&cps_eth1 {
+	status = "okay";
+	phy = <&phy0>;
+	phy-mode = "sgmii";
+};
+
 &cps_sata0 {
 	/* CPS Lane 1 - U32 */
 	/* CPS Lane 3 - U31 */

Signature · 1 line

2.11.0

Andrew Lunn

maintainer

2017-06-05 21:16 UTC #1 lore raw

PATCH 1/1 arm64: dts: marvell: 8040-mcbin: Enable 1GB Ethernet

arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts line 132

Quoted 5 lines

+&cps_eth1 {
arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts +4 -0
+	status = "okay";
+	phy = <&phy0>;
+	phy-mode = "sgmii";
+};

Hi Marc

Documentation/devicetree/bindings/net/ethernet.txt says:

- phy-handle: phandle, specifies a reference to a node representing a PHY device; this property is described in ePAPR and so preferred;
- phy: the same as "phy-handle" property, not recommended for new bindings.

Andrew

Marc Zyngier

author

2017-06-06 08:21 UTC #2 lore raw

PATCH 1/1 arm64: dts: marvell: 8040-mcbin: Enable 1GB Ethernet

Hi Andrew,

Quoted 8 lines · Andrew Lunn

arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts line 132
Quoted 5 lines
+&cps_eth1 {
arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts +4 -0
+	status = "okay";
+	phy = <&phy0>;
+	phy-mode = "sgmii";
+};
Hi Marc

Quoted 5 lines

Documentation/devicetree/bindings/net/ethernet.txt says:
- phy-handle: phandle, specifies a reference to a node representing a PHY device; this property is described in ePAPR and so preferred;
- phy: the same as "phy-handle" property, not recommended for new bindings.

Happy to amend the patch, though armada-8040-db.dts is already using "phy" (and not "phy-handle"). Should that one be fixed as well in the name of consistency? Also, Documentation/devicetree/bindings/net/marvell-pp2.txt only mentions the "phy" property. If feels a bit odd not to at least recommend the new property in the binding documentation.

Thanks,

M.

Signature · 1 line

Jazz is not dead. It just smells funny...

Andrew Lunn

maintainer

2017-06-06 12:10 UTC #3 lore raw

PATCH 1/1 arm64: dts: marvell: 8040-mcbin: Enable 1GB Ethernet

Quoted 17 lines · Marc Zyngier

Hi Andrew,
Quoted 8 lines · Andrew Lunn
arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts line 132
Quoted 5 lines
+&cps_eth1 {
arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts +4 -0
+	status = "okay";
+	phy = <&phy0>;
+	phy-mode = "sgmii";
+};
Hi Marc
Quoted 5 lines
Documentation/devicetree/bindings/net/ethernet.txt says:
- phy-handle: phandle, specifies a reference to a node representing a PHY device; this property is described in ePAPR and so preferred;
- phy: the same as "phy-handle" property, not recommended for new bindings.

Quoted 6 lines

Happy to amend the patch, though armada-8040-db.dts is already using "phy" (and not "phy-handle"). Should that one be fixed as well in the name of consistency? Also, Documentation/devicetree/bindings/net/marvell-pp2.txt only mentions the "phy" property. If feels a bit odd not to at least recommend the new property in the binding documentation.

Ah. As Russell pointed out, parsing this property is done by the driver, not a central helper. mvpp2 only supports phy, not phy-handle. This is not a new binding, it has been around since the middle of 2014. So "phy" is O.K.

Sometime in the future, it would be nice to centralise the parsing of this and phy-mode.

Andrew

Thomas Petazzoni

2017-06-06 09:02 UTC #4 lore raw

PATCH 1/1 arm64: dts: marvell: 8040-mcbin: Enable 1GB Ethernet

In reply to Marc Zyngier

Hello,

Quoted 4 lines · Marc Zyngier

Enable the 1GB Ethernet interface that lives on the slave CP110, with its corresponding phy (that oddly lives on the master CP110).
Signed-off-by Marc Zyngier <[email protected]>

This patch is somewhat redundant with
https://patchwork.kernel.org/patch/9728207/, which despite the flaming discussion that followed, does the same thing (and more).

Best regards,

Thomas

Signature · 3 lines

Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Thomas Petazzoni

2017-06-06 09:06 UTC #5 lore raw

PATCH 1/1 arm64: dts: marvell: 8040-mcbin: Enable 1GB Ethernet

Hello,

Quoted 3 lines · Thomas Petazzoni

This patch is somewhat redundant with
https://patchwork.kernel.org/patch/9728207/, which despite the flaming discussion that followed, does the same thing (and more).

That being said, the patch from Marcin was also enabling SDHCI, which has already been enabled in the mean time by a patch from Russell.

So perhaps we should indeed keep Marc's patch, enabling just the 1GB Ethernet interface.

Best regards,

Thomas

Signature · 3 lines

Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Marc Zyngier

author

2017-06-06 09:18 UTC #6 lore raw

PATCH 1/1 arm64: dts: marvell: 8040-mcbin: Enable 1GB Ethernet

Quoted 8 lines · Thomas Petazzoni

Hello,
Quoted 3 lines · Thomas Petazzoni
This patch is somewhat redundant with
https://patchwork.kernel.org/patch/9728207/, which despite the flaming discussion that followed, does the same thing (and more).

Quoted 5 lines

That being said, the patch from Marcin was also enabling SDHCI, which has already been enabled in the mean time by a patch from Russell.
So perhaps we should indeed keep Marc's patch, enabling just the 1GB Ethernet interface.

Whichever patch we merge, Andrew's comment remains. Should we stick with "phy", or switch to "phy-handle" everywhere?

Thanks,

M.

Signature · 1 line

Jazz is not dead, it just smell funny.

Thomas Petazzoni

2017-06-06 09:36 UTC #7 lore raw

PATCH 1/1 arm64: dts: marvell: 8040-mcbin: Enable 1GB Ethernet

Hello,

Quoted 5 lines · Marc Zyngier

Quoted 2 lines
So perhaps we should indeed keep Marc's patch, enabling just the 1GB Ethernet interface.
Whichever patch we merge, Andrew's comment remains. Should we stick with "phy", or switch to "phy-handle" everywhere?

Since the binding says to use phy-handle, we should use phy-handle for your patch. Of course, progressively, we should migrate all our bindings to this new property name, but I don't think this should be a prerequisite for your patch to go in.

Side note: I don't understand this change from "phy" to "phy-handle". It really doesn't make sense to me to encode the type of the property in the name of the property. Are we going to use:

  phy-mode-string = "sgmii";
  pinctrl-0-handle = <&foo>;
  reg-u32s = <0xdeadbeef 0xbadcafe>;

is there a pointer to the reasoning for this change?

Best regards,

Thomas

Signature · 3 lines

Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Russell King - ARM Linux

maintainer

2017-06-06 09:57 UTC #8 lore raw

PATCH 1/1 arm64: dts: marvell: 8040-mcbin: Enable 1GB Ethernet

Quoted 3 lines · Thomas Petazzoni

Side note: I don't understand this change from "phy" to "phy-handle". It really doesn't make sense to me to encode the type of the property in the name of the property. Are we going to use:

ePAPR 1.1 has specified phy-handle, which as the binding document says is the preferred version (probably because it's in ePAPR as the official name of the property.)

We've ended up with "phy" and "phy-handle" because there was never a central binding document for ethernet devices until 2014, so drivers randomly picked between the two property names. Which one should be used is driver dependent, since drivers parse this property. Many drivers parse just one of the names and not the other, so you have to use the right one for the driver.

Signature · 3 lines

RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Marc Zyngier

author

2017-06-06 09:16 UTC #9 lore raw

PATCH 1/1 arm64: dts: marvell: 8040-mcbin: Enable 1GB Ethernet

In reply to Thomas Petazzoni

Quoted 11 lines · Thomas Petazzoni

Hello,
Quoted 4 lines · Marc Zyngier
Enable the 1GB Ethernet interface that lives on the slave CP110, with its corresponding phy (that oddly lives on the master CP110).
Signed-off-by Marc Zyngier <[email protected]>
This patch is somewhat redundant with
https://patchwork.kernel.org/patch/9728207/, which despite the flaming discussion that followed, does the same thing (and more).

It's great that there is a patch for that already. How far is that from being merged (given how much controversy seem to surround this)?

Thanks,

Signature · 1 line

Jazz is not dead, it just smell funny.

Thomas Petazzoni

2017-06-06 09:19 UTC #10 lore raw

PATCH 1/1 arm64: dts: marvell: 8040-mcbin: Enable 1GB Ethernet

Hello,

Quoted 6 lines · Marc Zyngier

Quoted 3 lines
This patch is somewhat redundant with
https://patchwork.kernel.org/patch/9728207/, which despite the flaming discussion that followed, does the same thing (and more).
It's great that there is a patch for that already. How far is that from being merged (given how much controversy seem to surround this)?

As I said in my second reply, I believe a v2 of your patch would in fact be better.

Marcin's patch is no longer applicable, and was doing too many things at once. So let's merge your patch instead.

Best regards,

Thomas

Signature · 3 lines

Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Thomas Petazzoni

2017-06-06 12:52 UTC #11 lore raw

PATCH 1/1 arm64: dts: marvell: 8040-mcbin: Enable 1GB Ethernet

In reply to Marc Zyngier

Hello,

Quoted 4 lines · Marc Zyngier

Enable the 1GB Ethernet interface that lives on the slave CP110, with its corresponding phy (that oddly lives on the master CP110).
Signed-off-by Marc Zyngier <[email protected]>

Tested-by Thomas Petazzoni <[email protected]>

Signature · 3 lines

Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Russell King - ARM Linux

maintainer

2017-06-06 13:34 UTC #12 lore raw

PATCH 1/1 arm64: dts: marvell: 8040-mcbin: Enable 1GB Ethernet

In reply to Marc Zyngier

Quoted 13 lines · Marc Zyngier

Enable the 1GB Ethernet interface that lives on the slave CP110, with its corresponding phy (that oddly lives on the master CP110).
Signed-off-by Marc Zyngier <[email protected]>
---
 arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts +0 -0
index f7bb0cc03147..1eb3cd5332c1 100644

arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts line 104

Quoted 5 lines

arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts +2 -0
@@ -100,6 +100,12 @@
 	status = "okay";
 };
 
+&cpm_mdio {
+	phy0: ethernet-phy at 0 {

As the board has multiple phys, can we use a more descriptive label here, such as "ge_phy" (for gigabit ethernet phy) to distinugish it from the 10G PHYs?

arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts line 128

Quoted 5 lines

arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts +5 -0
+&cps_ethernet {
+	status = "okay";
+};
+
+&cps_eth1 {

Labelling up the connector as is done elsewhere in the file would be nice:

	/* CPS Lane 0 - J5 (Gigabit RJ45) */

Quoted 3 lines

arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts +3 -0
+	status = "okay";
+	phy = <&phy0>;
+	phy-mode = "sgmii";

Signature · 3 lines

RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Marc Zyngier

author

2017-06-06 13:48 UTC #13 lore raw

PATCH 1/1 arm64: dts: marvell: 8040-mcbin: Enable 1GB Ethernet

Quoted 33 lines · Russell King - ARM Linux

Quoted 13 lines · Marc Zyngier
Enable the 1GB Ethernet interface that lives on the slave CP110, with its corresponding phy (that oddly lives on the master CP110).
Signed-off-by Marc Zyngier <[email protected]>
---
 arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts +0 -0
index f7bb0cc03147..1eb3cd5332c1 100644
arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts line 104
Quoted 5 lines
arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts +2 -0
@@ -100,6 +100,12 @@
 	status = "okay";
 };
 
+&cpm_mdio {
+	phy0: ethernet-phy at 0 {
As the board has multiple phys, can we use a more descriptive label here, such as "ge_phy" (for gigabit ethernet phy) to distinugish it from the 10G PHYs?
arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts line 128
Quoted 5 lines
arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts +5 -0
+&cps_ethernet {
+	status = "okay";
+};
+
+&cps_eth1 {
Labelling up the connector as is done elsewhere in the file would be nice:

Quoted 7 lines

	/* CPS Lane 0 - J5 (Gigabit RJ45) */
arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts line 131
Quoted 3 lines
arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts +3 -0
+	status = "okay";
+	phy = <&phy0>;
+	phy-mode = "sgmii";

Yup, agreed on both counts. I'll update the patch.

Thanks,

M.

Signature · 1 line

Jazz is not dead. It just smells funny...