Fix 10G PHY interface types

-----Original Message-----
From: [email protected] <[email protected]> On Behalf Of Russell King - ARM Linux admin
Sent: Friday, January 3, 2020 1:51 PM
To: Andrew Lunn <[email protected]>; Florian Fainelli <[email protected]>; Heiner Kallweit <[email protected]>
Cc: David S. Miller <[email protected]>; Jonathan Corbet <[email protected]>; Kishon Vijay Abraham I <[email protected]>; linux- [email protected]; [email protected]
Subject: [PATCH net-next 0/2] Fix 10G PHY interface types

Recent discussion has revealed that our current usage of the 10GKR phy_interface_t is not correct. This is based on a misunderstanding caused in part by the various specifications being difficult to obtain. Now that a better understanding has been reached, we ought to correct this.

This series introduce PHY_INTERFACE_MODE_10GBASER to replace the existing usage of 10GKR mode, and document their differences in the phylib documentation. Then switch PHY, SFP/phylink, the Marvell PP2 network driver, and its associated comphy driver over to use the correct interface mode. None of the existing platform usage was actually using 10GBASE-KR.

In order to maintain compatibility with existing DT files, arrange for the Marvell PP2 driver to rewrite the phy interface mode; this allows other drivers to adopt correct behaviour w.r.t whether the 10G connection conforms to the backplane 10GBASE-KR protocol vs normal 10GBASE-R protocol.

After applying these locally to net-next I've validated that the only places which mention the old PHY_INTERFACE_MODE_10GKR definition are:

Documentation/networking/phy.rst:``PHY_INTERFACE_MODE_10GKR``
drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c: if (phy_mode ==
PHY_INTERFACE_MODE_10GKR)
drivers/net/phy/aquantia_main.c: phydev->interface =
PHY_INTERFACE_MODE_10GKR;
drivers/net/phy/aquantia_main.c: phydev->interface !=
PHY_INTERFACE_MODE_10GKR &&

include/linux/phy.h:    PHY_INTERFACE_MODE_10GKR,
include/linux/phy.h:    case PHY_INTERFACE_MODE_10GKR:

which is as expected. The only users of "10gbase-kr" in DT are:

arch/arm64/boot/dts/marvell/armada-7040-db.dts: phy-mode = "10gbase-kr";
arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts: phy-mode =
"10gbase-kr"; arch/arm64/boot/dts/marvell/armada-8040-db.dts: phy-mode = "10gbase-kr"; arch/arm64/boot/dts/marvell/armada-8040-db.dts: phy-mode = "10gbase-kr";
arch/arm64/boot/dts/marvell/armada-8040-mcbin-singleshot.dts: phy-mode =
"10gbase-kr";
arch/arm64/boot/dts/marvell/armada-8040-mcbin-singleshot.dts: phy-mode =
"10gbase-kr";
arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts: phy-mode =
"10gbase-kr";arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts: phy-
mode = "10gbase-kr";arch/arm64/boot/dts/marvell/cn9130-db.dts: phy-
mode = "10gbase-kr";
arch/arm64/boot/dts/marvell/cn9131-db.dts: phy-mode = "10gbase-kr";
arch/arm64/boot/dts/marvell/cn9132-db.dts: phy-mode = "10gbase-kr";

which all use the mvpp2 driver, and these will be updated in a separate patch to be submitted in the following kernel cycle.

 Documentation/networking/phy.rst                | 18 ++++++++++++++++++
 drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 13 ++++++++-----
 drivers/net/phy/aquantia_main.c                 |  7 +++++--
 drivers/net/phy/bcm84881.c                      |  4 ++--
 drivers/net/phy/marvell10g.c                    | 11 ++++++-----
 drivers/net/phy/phylink.c                       |  1 +
 drivers/net/phy/sfp-bus.c                       |  2 +-
 drivers/phy/marvell/phy-mvebu-cp110-comphy.c    | 20 ++++++++++----------
 include/linux/phy.h                             | 12 ++++++++----
 9 files changed, 59 insertions(+), 29 deletions(-)

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Quoted 76 lines

-----Original Message-----
From: [email protected] <[email protected]> On Behalf Of Russell King - ARM Linux admin
Sent: Friday, January 3, 2020 1:51 PM
To: Andrew Lunn <[email protected]>; Florian Fainelli <[email protected]>; Heiner Kallweit <[email protected]>
Cc: David S. Miller <[email protected]>; Jonathan Corbet <[email protected]>; Kishon Vijay Abraham I <[email protected]>; linux- [email protected]; [email protected]
Subject: [PATCH net-next 0/2] Fix 10G PHY interface types

Recent discussion has revealed that our current usage of the 10GKR phy_interface_t is not correct. This is based on a misunderstanding caused in part by the various specifications being difficult to obtain. Now that a better understanding has been reached, we ought to correct this.

This series introduce PHY_INTERFACE_MODE_10GBASER to replace the existing usage of 10GKR mode, and document their differences in the phylib documentation. Then switch PHY, SFP/phylink, the Marvell PP2 network driver, and its associated comphy driver over to use the correct interface mode. None of the existing platform usage was actually using 10GBASE-KR.

In order to maintain compatibility with existing DT files, arrange for the Marvell PP2 driver to rewrite the phy interface mode; this allows other drivers to adopt correct behaviour w.r.t whether the 10G connection conforms to the backplane 10GBASE-KR protocol vs normal 10GBASE-R protocol.

After applying these locally to net-next I've validated that the only places which mention the old PHY_INTERFACE_MODE_10GKR definition are:

Documentation/networking/phy.rst:``PHY_INTERFACE_MODE_10GKR``
drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c: if (phy_mode ==
PHY_INTERFACE_MODE_10GKR)
drivers/net/phy/aquantia_main.c: phydev->interface =
PHY_INTERFACE_MODE_10GKR;
drivers/net/phy/aquantia_main.c: phydev->interface !=
PHY_INTERFACE_MODE_10GKR &&

include/linux/phy.h:    PHY_INTERFACE_MODE_10GKR,
include/linux/phy.h:    case PHY_INTERFACE_MODE_10GKR:

which is as expected. The only users of "10gbase-kr" in DT are:

arch/arm64/boot/dts/marvell/armada-7040-db.dts: phy-mode = "10gbase-kr";
arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts: phy-mode =
"10gbase-kr"; arch/arm64/boot/dts/marvell/armada-8040-db.dts: phy-mode = "10gbase-kr"; arch/arm64/boot/dts/marvell/armada-8040-db.dts: phy-mode = "10gbase-kr";
arch/arm64/boot/dts/marvell/armada-8040-mcbin-singleshot.dts: phy-mode =
"10gbase-kr";
arch/arm64/boot/dts/marvell/armada-8040-mcbin-singleshot.dts: phy-mode =
"10gbase-kr";
arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts: phy-mode =
"10gbase-kr";arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts: phy-
mode = "10gbase-kr";arch/arm64/boot/dts/marvell/cn9130-db.dts: phy-
mode = "10gbase-kr";
arch/arm64/boot/dts/marvell/cn9131-db.dts: phy-mode = "10gbase-kr";
arch/arm64/boot/dts/marvell/cn9132-db.dts: phy-mode = "10gbase-kr";

which all use the mvpp2 driver, and these will be updated in a separate patch to be submitted in the following kernel cycle.

 Documentation/networking/phy.rst                | 18 ++++++++++++++++++
 drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 13 ++++++++-----
 drivers/net/phy/aquantia_main.c                 |  7 +++++--
 drivers/net/phy/bcm84881.c                      |  4 ++--
 drivers/net/phy/marvell10g.c                    | 11 ++++++-----
 drivers/net/phy/phylink.c                       |  1 +
 drivers/net/phy/sfp-bus.c                       |  2 +-
 drivers/phy/marvell/phy-mvebu-cp110-comphy.c    | 20 ++++++++++----------
 include/linux/phy.h                             | 12 ++++++++----
 9 files changed, 59 insertions(+), 29 deletions(-)

Quoted 5 lines

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Hi,

we should conclude our discussions on the initial thread before we do this. The current use on 10GBASE_KR is not correct, I agree but changing this to 10GBASE-R may not be the way to go. We need to determine if the existing device tree binding corelated type is the one we need to change or maybe a more complex solution is required. There are multiple paths forward:

Extending this enum that has a mix of things in it that are barely related.

For 10G Ethernet there is already an XGMII entry that describes the MII, if this should stick to strict MIIs. This is of little value for chips that have part of the traditional PHY blocks grouped along with the MAC, the XGMII is not exposed outside the RTL (if it even exists there) and the actual visible interfaces are completely different.

Describing the actual interface at chip to chip level (RGMII, SGMII, XAUI, XFI, etc.). This may be incomplete for people trying to configure their HW that supports multiple modes (reminder - device trees describe HW, they do not configure SW). More details would be required and the list would be... eclectic.

Moving to something different altogether, that would not conflict existing device trees but permit a more thorough classification and less ambiguity. We need more work on clearing a path towards that.

Describing the actual interface at chip to chip level (RGMII, SGMII, XAUI, XFI, etc.). This may be incomplete for people trying to configure their HW that supports multiple modes (reminder - device trees describe HW, they do not configure SW). More details would be required and the list would be... eclectic.

-----Original Message-----
From: Andrew Lunn <[email protected]>
Sent: Friday, January 3, 2020 4:18 PM
To: Madalin Bucur (OSS) <[email protected]>
Cc: Russell King - ARM Linux admin <[email protected]>; Florian Fainelli <[email protected]>; Heiner Kallweit <[email protected]>; David S. Miller <[email protected]>; Jonathan Corbet <[email protected]>; Kishon Vijay Abraham I <[email protected]>; [email protected]; [email protected]
Subject: Re: [PATCH net-next 0/2] Fix 10G PHY interface types

Quoted 1 line

Describing the actual interface at chip to chip level (RGMII, SGMII,

XAUI,

Quoted 1 line

XFI, etc.). This may be incomplete for people trying to configure their

HW

Quoted 1 line

that supports multiple modes (reminder - device trees describe HW, they

do

Quoted 1 line

not configure SW). More details would be required and the list would

be...

Quoted 1 line

eclectic.

Hi Madalin

Please forget the existing DT binding for the moment. Please describe what values you need to program into your hardware to make it work. Please don't use the existing DT naming when describing what you need. Maybe use the terms from the reference manual?

Once we have a list, we can figure out what could be generic, what could be vendor specific, and how to describe it in ACPI, DT, etc.

At LPC 2019, Claudiu and Vladimir talked about wanting to describe the eye configuration for some hardware. It would be interesting if there is any overlap. Aquantia also have some values used to configure the SERDES of their PHYs. I think this is a board specific binary blob which is loaded into the PHY as part of the firmware. That then limits their firmware to a specific board, which is not so nice. But does that also indicate that how the MAC is configured might also depend on how the PHY configures its electrical properties?

Andrew

And here is a LS1046A SoC document that describes XFI and 10GBASE_KR:

https://www.mouser.com/pdfdocs/LS1046A.pdf

Recent discussion has revealed that the use of PHY_INTERFACE_MODE_10GKR is incorrect. Add a 10GBASE-R definition, document both the -R and -KR versions, and the fact that 10GKR was used incorrectly.

Signed-off-by Russell King <[email protected]>

Recent discussion has revealed that our current usage of the 10GKR phy_interface_t is not correct. This is based on a misunderstanding caused in part by the various specifications being difficult to obtain. Now that a better understanding has been reached, we ought to correct this.

This series introduce PHY_INTERFACE_MODE_10GBASER to replace the existing usage of 10GKR mode, and document their differences in the phylib documentation. Then switch PHY, SFP/phylink, the Marvell PP2 network driver, and its associated comphy driver over to use the correct interface mode. None of the existing platform usage was actually using 10GBASE-KR.

Quoted 9 lines

For Marvell mvpp2, we detect 10GBASE-KR, and rewrite it to 10GBASE-R for compatibility with existing DT - this is the only network driver at present that makes use of PHY_INTERFACE_MODE_10GKR.

Signed-off-by Russell King <[email protected]>

drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c +0 -0

@@ -5247,6 +5247,9 @@ static int mvpp2_port_probe(struct platform_device *pdev,
 		goto err_free_netdev;
 	}
 

drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c static int mvpp2_port_probe(struct platform_device *pdev, · line 5251

Quoted 2 lines

drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c +2 -0

+	if (phy_mode == PHY_INTERFACE_MODE_10GKR)
+		phy_mode = PHY_INTERFACE_MODE_10GBASER;

Hi Russell

Maybe consider adding a comment here, or suggest readers to read the commit message of the patch that added these two lines to get the full story.

-----Original Message-----
From: Andrew Lunn <[email protected]>
Sent: Friday, January 3, 2020 6:48 PM
To: Madalin Bucur (OSS) <[email protected]>
Cc: Russell King - ARM Linux admin <[email protected]>; Florian Fainelli <[email protected]>; Heiner Kallweit <[email protected]>; David S. Miller <[email protected]>; Jonathan Corbet <[email protected]>; Kishon Vijay Abraham I <[email protected]>; [email protected]; [email protected]
Subject: Re: [PATCH net-next 0/2] Fix 10G PHY interface types

Quoted 3 lines

And here is a LS1046A SoC document that describes XFI and 10GBASE_KR:

https://www.mouser.com/pdfdocs/LS1046A.pdf

I don't see any register descriptions here. So you failed to answer my question. What do you actually need to configure? What needs to go into DT?

You keep avoiding the questions Russell and I pose, which is not helping your argument. We need details, not hand waiving.

Andrew