[PATCH net-next v4 0/3] Broadcom PHY driver updates

Updates to the Broadcom PHY driver related to use with copper SFP modules.

Changed since v3:
-fixed kerneldoc error

Changed since v2:
-Create flag for PHY on SFP module and use that rather than accessing
 attached_dev directly in PHY driver

Changed since v1:
-Reversed conditional to reduce indentation
-Added missing setting of MII_BCM54XX_AUXCTL_MISC_WREN in
 MII_BCM54XX_AUXCTL_SHDWSEL_MISC register

Robert Hancock (3):
  net: phy: broadcom: Set proper 1000BaseX/SGMII interface mode for
    BCM54616S
  net: phy: Add is_on_sfp_module flag and phy_on_sfp helper
  net: phy: broadcom: Do not modify LED configuration for SFP module
    PHYs

 drivers/net/phy/broadcom.c   | 108 ++++++++++++++++++++++++++++-------
 drivers/net/phy/phy_device.c |   2 +
 include/linux/brcmphy.h      |   4 ++
 include/linux/phy.h          |  11 ++++
 4 files changed, 104 insertions(+), 21 deletions(-)

-- 
2.27.0

Add a flag and helper function to indicate that a PHY device is part of
an SFP module, which is set on attach. This can be used by PHY drivers
to handle SFP-specific quirks or behavior.

Signed-off-by: Robert Hancock <redacted>
---
 drivers/net/phy/phy_device.c |  2 ++
 include/linux/phy.h          | 11 +++++++++++
 2 files changed, 13 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 05261698bf74..d6ac3ed38197 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c

@@ -1377,6 +1377,8 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 
 		if (phydev->sfp_bus_attached)
 			dev->sfp_bus = phydev->sfp_bus;
+		else if (dev->sfp_bus)
+			phydev->is_on_sfp_module = true;
 	}
 
 	/* Some Ethernet drivers try to connect to a PHY device before

diff --git a/include/linux/phy.h b/include/linux/phy.h
index 0d537f59b77f..1a12e4436b5b 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h

@@ -492,6 +492,7 @@ struct macsec_ops;
  * @sysfs_links: Internal boolean tracking sysfs symbolic links setup/removal.
  * @loopback_enabled: Set true if this PHY has been loopbacked successfully.
  * @downshifted_rate: Set true if link speed has been downshifted.
+ * @is_on_sfp_module: Set true if PHY is located on an SFP module.
  * @state: State of the PHY for management purposes
  * @dev_flags: Device-specific flags used by the PHY driver.
  * @irq: IRQ number of the PHY's interrupt (-1 if none)

@@ -565,6 +566,7 @@ struct phy_device {
 	unsigned sysfs_links:1;
 	unsigned loopback_enabled:1;
 	unsigned downshifted_rate:1;
+	unsigned is_on_sfp_module:1;
 
 	unsigned autoneg:1;
 	/* The most recently read link state */

@@ -1296,6 +1298,15 @@ static inline bool phy_is_internal(struct phy_device *phydev)
 	return phydev->is_internal;
 }
 
+/**
+ * phy_on_sfp - Convenience function for testing if a PHY is on an SFP module
+ * @phydev: the phy_device struct
+ */
+static inline bool phy_on_sfp(struct phy_device *phydev)
+{
+	return phydev->is_on_sfp_module;
+}
+
 /**
  * phy_interface_mode_is_rgmii - Convenience function for testing if a
  * PHY interface mode is RGMII (all variants)

-- 
2.27.0

The default configuration for the BCM54616S PHY may not match the desired
mode when using 1000BaseX or SGMII interface modes, such as when it is on
an SFP module. Add code to explicitly set the correct mode using
programming sequences provided by Bel-Fuse:

https://www.belfuse.com/resources/datasheets/powersolutions/ds-bps-sfp-1gbt-05-series.pdf
https://www.belfuse.com/resources/datasheets/powersolutions/ds-bps-sfp-1gbt-06-series.pdf

Signed-off-by: Robert Hancock <redacted>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/broadcom.c | 84 ++++++++++++++++++++++++++++++++------
 include/linux/brcmphy.h    |  4 ++
 2 files changed, 76 insertions(+), 12 deletions(-)

diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 91fbd26c809e..8e7fc3368380 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c

@@ -103,6 +103,64 @@ static int bcm54612e_config_init(struct phy_device *phydev)
 	return 0;
 }
 
+static int bcm54616s_config_init(struct phy_device *phydev)
+{
+	int rc, val;
+
+	if (phydev->interface != PHY_INTERFACE_MODE_SGMII &&
+	    phydev->interface != PHY_INTERFACE_MODE_1000BASEX)
+		return 0;
+
+	/* Ensure proper interface mode is selected. */
+	/* Disable RGMII mode */
+	val = bcm54xx_auxctl_read(phydev, MII_BCM54XX_AUXCTL_SHDWSEL_MISC);
+	if (val < 0)
+		return val;
+	val &= ~MII_BCM54XX_AUXCTL_SHDWSEL_MISC_RGMII_EN;
+	val |= MII_BCM54XX_AUXCTL_MISC_WREN;
+	rc = bcm54xx_auxctl_write(phydev, MII_BCM54XX_AUXCTL_SHDWSEL_MISC,
+				  val);
+	if (rc < 0)
+		return rc;
+
+	/* Select 1000BASE-X register set (primary SerDes) */
+	val = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
+	if (val < 0)
+		return val;
+	val |= BCM54XX_SHD_MODE_1000BX;
+	rc = bcm_phy_write_shadow(phydev, BCM54XX_SHD_MODE, val);
+	if (rc < 0)
+		return rc;
+
+	/* Power down SerDes interface */
+	rc = phy_set_bits(phydev, MII_BMCR, BMCR_PDOWN);
+	if (rc < 0)
+		return rc;
+
+	/* Select proper interface mode */
+	val &= ~BCM54XX_SHD_INTF_SEL_MASK;
+	val |= phydev->interface == PHY_INTERFACE_MODE_SGMII ?
+		BCM54XX_SHD_INTF_SEL_SGMII :
+		BCM54XX_SHD_INTF_SEL_GBIC;
+	rc = bcm_phy_write_shadow(phydev, BCM54XX_SHD_MODE, val);
+	if (rc < 0)
+		return rc;
+
+	/* Power up SerDes interface */
+	rc = phy_clear_bits(phydev, MII_BMCR, BMCR_PDOWN);
+	if (rc < 0)
+		return rc;
+
+	/* Select copper register set */
+	val &= ~BCM54XX_SHD_MODE_1000BX;
+	rc = bcm_phy_write_shadow(phydev, BCM54XX_SHD_MODE, val);
+	if (rc < 0)
+		return rc;
+
+	/* Power up copper interface */
+	return phy_clear_bits(phydev, MII_BMCR, BMCR_PDOWN);
+}
+
 /* Needs SMDSP clock enabled via bcm54xx_phydsp_config() */
 static int bcm50610_a0_workaround(struct phy_device *phydev)
 {

@@ -283,15 +341,17 @@ static int bcm54xx_config_init(struct phy_device *phydev)
 
 	bcm54xx_adjust_rxrefclk(phydev);
 
-	if (BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54210E) {
+	switch (BRCM_PHY_MODEL(phydev)) {
+	case PHY_ID_BCM54210E:
 		err = bcm54210e_config_init(phydev);
-		if (err)
-			return err;
-	} else if (BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54612E) {
+		break;
+	case PHY_ID_BCM54612E:
 		err = bcm54612e_config_init(phydev);
-		if (err)
-			return err;
-	} else if (BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54810) {
+		break;
+	case PHY_ID_BCM54616S:
+		err = bcm54616s_config_init(phydev);
+		break;
+	case PHY_ID_BCM54810:
 		/* For BCM54810, we need to disable BroadR-Reach function */
 		val = bcm_phy_read_exp(phydev,
 				       BCM54810_EXP_BROADREACH_LRE_MISC_CTL);

@@ -299,9 +359,10 @@ static int bcm54xx_config_init(struct phy_device *phydev)
 		err = bcm_phy_write_exp(phydev,
 					BCM54810_EXP_BROADREACH_LRE_MISC_CTL,
 					val);
-		if (err < 0)
-			return err;
+		break;
 	}
+	if (err)
+		return err;
 
 	bcm54xx_phydsp_config(phydev);
 

@@ -390,7 +451,7 @@ struct bcm54616s_phy_priv {
 static int bcm54616s_probe(struct phy_device *phydev)
 {
 	struct bcm54616s_phy_priv *priv;
-	int val, intf_sel;
+	int val;
 
 	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)

@@ -408,8 +469,7 @@ static int bcm54616s_probe(struct phy_device *phydev)
 	 * RGMII-1000Base-X is properly supported, but RGMII-100Base-FX
 	 * support is still missing as of now.
 	 */
-	intf_sel = (val & BCM54XX_SHD_INTF_SEL_MASK) >> 1;
-	if (intf_sel == 1) {
+	if ((val & BCM54XX_SHD_INTF_SEL_MASK) == BCM54XX_SHD_INTF_SEL_RGMII) {
 		val = bcm_phy_read_shadow(phydev, BCM54616S_SHD_100FX_CTRL);
 		if (val < 0)
 			return val;

diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
index 8fe1d55371ac..c2c2147dfeb8 100644
--- a/include/linux/brcmphy.h
+++ b/include/linux/brcmphy.h

@@ -129,6 +129,7 @@
 
 #define MII_BCM54XX_AUXCTL_SHDWSEL_MISC			0x07
 #define MII_BCM54XX_AUXCTL_SHDWSEL_MISC_WIRESPEED_EN	0x0010
+#define MII_BCM54XX_AUXCTL_SHDWSEL_MISC_RGMII_EN	0x0080
 #define MII_BCM54XX_AUXCTL_SHDWSEL_MISC_RGMII_SKEW_EN	0x0100
 #define MII_BCM54XX_AUXCTL_MISC_FORCE_AMDIX		0x0200
 #define MII_BCM54XX_AUXCTL_MISC_WREN			0x8000

@@ -216,6 +217,9 @@
 /* 11111: Mode Control Register */
 #define BCM54XX_SHD_MODE		0x1f
 #define BCM54XX_SHD_INTF_SEL_MASK	GENMASK(2, 1)	/* INTERF_SEL[1:0] */
+#define BCM54XX_SHD_INTF_SEL_RGMII	0x02
+#define BCM54XX_SHD_INTF_SEL_SGMII	0x04
+#define BCM54XX_SHD_INTF_SEL_GBIC	0x06
 #define BCM54XX_SHD_MODE_1000BX		BIT(0)	/* Enable 1000-X registers */
 
 /*

-- 
2.27.0

bcm54xx_config_init was modifying the PHY LED configuration to enable link
and activity indications. However, some SFP modules (such as Bel-Fuse
SFP-1GBT-06) have no LEDs but use the LED outputs to control the SFP LOS
signal, and modifying the LED settings will cause the LOS output to
malfunction. Skip this configuration for PHYs which are bound to an SFP
bus.

Signed-off-by: Robert Hancock <redacted>
---
 drivers/net/phy/broadcom.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 8e7fc3368380..fa0be591ae79 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c

@@ -366,18 +366,24 @@ static int bcm54xx_config_init(struct phy_device *phydev)
 
 	bcm54xx_phydsp_config(phydev);
 
-	/* Encode link speed into LED1 and LED3 pair (green/amber).
+	/* For non-SFP setups, encode link speed into LED1 and LED3 pair
+	 * (green/amber).
 	 * Also flash these two LEDs on activity. This means configuring
 	 * them for MULTICOLOR and encoding link/activity into them.
+	 * Don't do this for devices on an SFP module, since some of these
+	 * use the LED outputs to control the SFP LOS signal, and changing
+	 * these settings will cause LOS to malfunction.
 	 */
-	val = BCM5482_SHD_LEDS1_LED1(BCM_LED_SRC_MULTICOLOR1) |
-		BCM5482_SHD_LEDS1_LED3(BCM_LED_SRC_MULTICOLOR1);
-	bcm_phy_write_shadow(phydev, BCM5482_SHD_LEDS1, val);
-
-	val = BCM_LED_MULTICOLOR_IN_PHASE |
-		BCM5482_SHD_LEDS1_LED1(BCM_LED_MULTICOLOR_LINK_ACT) |
-		BCM5482_SHD_LEDS1_LED3(BCM_LED_MULTICOLOR_LINK_ACT);
-	bcm_phy_write_exp(phydev, BCM_EXP_MULTICOLOR, val);
+	if (!phy_on_sfp(phydev)) {
+		val = BCM5482_SHD_LEDS1_LED1(BCM_LED_SRC_MULTICOLOR1) |
+			BCM5482_SHD_LEDS1_LED3(BCM_LED_SRC_MULTICOLOR1);
+		bcm_phy_write_shadow(phydev, BCM5482_SHD_LEDS1, val);
+
+		val = BCM_LED_MULTICOLOR_IN_PHASE |
+			BCM5482_SHD_LEDS1_LED1(BCM_LED_MULTICOLOR_LINK_ACT) |
+			BCM5482_SHD_LEDS1_LED3(BCM_LED_MULTICOLOR_LINK_ACT);
+		bcm_phy_write_exp(phydev, BCM_EXP_MULTICOLOR, val);
+	}
 
 	return 0;
 }

-- 
2.27.0

Hello:

This series was applied to netdev/net-next.git (refs/heads/master):

On Tue, 16 Feb 2021 16:54:51 -0600 you wrote:

Updates to the Broadcom PHY driver related to use with copper SFP modules.

Changed since v3:
-fixed kerneldoc error

Changed since v2:
-Create flag for PHY on SFP module and use that rather than accessing
 attached_dev directly in PHY driver

[...]

Here is the summary with links:
  - [net-next,v4,1/3] net: phy: broadcom: Set proper 1000BaseX/SGMII interface mode for BCM54616S
    https://git.kernel.org/netdev/net-next/c/3afd0218992a
  - [net-next,v4,2/3] net: phy: Add is_on_sfp_module flag and phy_on_sfp helper
    https://git.kernel.org/netdev/net-next/c/b834489bcecc
  - [net-next,v4,3/3] net: phy: broadcom: Do not modify LED configuration for SFP module PHYs
    https://git.kernel.org/netdev/net-next/c/b5d007e2aac8

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

On Tue, Feb 16, 2021 at 04:54:53PM -0600, Robert Hancock wrote:

I get lost here. It this correct?

We have setups with two PHY. The marvell10g PHY can play the role of
media converter. It converts the signal from the MAC into signals
which can be connected to an SFP cage.

And then inside the cage, we can have a copper module with a second
PHY. It is this second PHY which we need to indicate is_on_sfp_module
true.

We probably want to media convert PHYs LEDs to work, since those
possible are connected to the front panel. So this needs to be
specific to the SFP module PHY, and i'm not sure it is. Russell?

	Andrew

On Wed, Feb 17, 2021 at 04:58:26AM +0100, Andrew Lunn wrote:

On Tue, Feb 16, 2021 at 04:54:53PM -0600, Robert Hancock wrote:

quoted

Add a flag and helper function to indicate that a PHY device is part of
an SFP module, which is set on attach. This can be used by PHY drivers
to handle SFP-specific quirks or behavior.

Signed-off-by: Robert Hancock <redacted>
---
 drivers/net/phy/phy_device.c |  2 ++
 include/linux/phy.h          | 11 +++++++++++
 2 files changed, 13 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 05261698bf74..d6ac3ed38197 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c

@@ -1377,6 +1377,8 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 
 		if (phydev->sfp_bus_attached)
 			dev->sfp_bus = phydev->sfp_bus;
+		else if (dev->sfp_bus)
+			phydev->is_on_sfp_module = true;

I get lost here. It this correct?

We have setups with two PHY. The marvell10g PHY can play the role of
media converter. It converts the signal from the MAC into signals
which can be connected to an SFP cage.

And then inside the cage, we can have a copper module with a second
PHY. It is this second PHY which we need to indicate is_on_sfp_module
true.

We don't support that setup, at least at the moment. There's no support
for stacking PHYs precisely because we have the net_device structure
containing a pointer to the PHY (which will be the first PHY in the
chain.) That has the effect of making the second PHY inaccessible to the
normal network APIs.

We probably want to media convert PHYs LEDs to work, since those
possible are connected to the front panel. So this needs to be
specific to the SFP module PHY, and i'm not sure it is. Russell?

The main reason I'm hessitant with using the net_device structure
to detect this is that we know that phylink has been converted to
work without the net_device structure - it will be NULL. This includes
attaching the "primary" PHY - phylib will be given a NULL net_device.

The good news is that if a SFP cage is attempted to be attached in
that situation, phylink will oops in phylink_sfp_attach(). So it
isn't something that we support. However, the point is that we can
end up in situations where phylib has a NULL net_device.

Florian mentioned in response to one of my previous emails that he's
working on sorting out the phylib dev_flags - I think we should wait
for that to be resolved first, so we can allocate a dev_flag (or
maybe we can do that already today?) that says "this PHY is part of
a SFP module". That will give us a clearly defined positive condition
that is more maintainable into the future.

I'm just worrying that if we sort out phylink_sfp_*tach() (which could
be trivial), if we introduce new dependencies into phylib on the
network device, we're moving backwards.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

On 2/17/2021 2:34 AM, Russell King - ARM Linux admin wrote:

On Wed, Feb 17, 2021 at 04:58:26AM +0100, Andrew Lunn wrote:

quoted

On Tue, Feb 16, 2021 at 04:54:53PM -0600, Robert Hancock wrote:

quoted

Add a flag and helper function to indicate that a PHY device is part of
an SFP module, which is set on attach. This can be used by PHY drivers
to handle SFP-specific quirks or behavior.

Signed-off-by: Robert Hancock <redacted>
---
 drivers/net/phy/phy_device.c |  2 ++
 include/linux/phy.h          | 11 +++++++++++
 2 files changed, 13 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 05261698bf74..d6ac3ed38197 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c

@@ -1377,6 +1377,8 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 
 		if (phydev->sfp_bus_attached)
 			dev->sfp_bus = phydev->sfp_bus;
+		else if (dev->sfp_bus)
+			phydev->is_on_sfp_module = true;

I get lost here. It this correct?

We have setups with two PHY. The marvell10g PHY can play the role of
media converter. It converts the signal from the MAC into signals
which can be connected to an SFP cage.

And then inside the cage, we can have a copper module with a second
PHY. It is this second PHY which we need to indicate is_on_sfp_module
true.

We don't support that setup, at least at the moment. There's no support
for stacking PHYs precisely because we have the net_device structure
containing a pointer to the PHY (which will be the first PHY in the
chain.) That has the effect of making the second PHY inaccessible to the
normal network APIs.

quoted

We probably want to media convert PHYs LEDs to work, since those
possible are connected to the front panel. So this needs to be
specific to the SFP module PHY, and i'm not sure it is. Russell?

The main reason I'm hessitant with using the net_device structure
to detect this is that we know that phylink has been converted to
work without the net_device structure - it will be NULL. This includes
attaching the "primary" PHY - phylib will be given a NULL net_device.

The good news is that if a SFP cage is attempted to be attached in
that situation, phylink will oops in phylink_sfp_attach(). So it
isn't something that we support. However, the point is that we can
end up in situations where phylib has a NULL net_device.

Florian mentioned in response to one of my previous emails that he's
working on sorting out the phylib dev_flags - I think we should wait
for that to be resolved first, so we can allocate a dev_flag (or
maybe we can do that already today?) that says "this PHY is part of
a SFP module". That will give us a clearly defined positive condition
that is more maintainable into the future.

We could be allocating a generic flag today starting from bit 31 and
down and that would certainly be fine without necessarily making my
rework any harder.

The existing issues with phydev->dev_flags as I am sure you are all
aware is that the flags are not name-spaced per driver yet all Ethernet
MAC drivers make assumptions (tg3.c, bcmgenet.c, etc.) and it is not
possible to pass arbitrary configuration settings down the PHY driver,
etc.  I would not hold my breath on this rework as I keep getting sucked
into various issues at work.

FWIW, this series appears to have already been applied.
-- 
Florian