Re: [PATCH net-next 1/2] net: phy: broadcom: Set proper 1000BaseX/SGMII interface mode for BCM54616S

On 2/12/2021 5:45 PM, Robert Hancock wrote:

On Fri, 2021-02-12 at 17:26 -0800, Florian Fainelli wrote:

quoted

On 2/12/2021 4:28 PM, 'Robert Hancock' via BCM-KERNEL-FEEDBACK-LIST,PDL
wrote:

quoted

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://urldefense.com/v3/__https://www.belfuse.com/resources/datasheets/powersolutions/ds-bps-sfp-1gbt-05-series.pdf__;!!IOGos0k!20FhZqRHEiz2-qFJ7J8XC4xX2qG-ajZ17Ma1W-VwDgwdQZeIhHEpWKlNldWW8DyFaQo$ 
https://urldefense.com/v3/__https://www.belfuse.com/resources/datasheets/powersolutions/ds-bps-sfp-1gbt-06-series.pdf__;!!IOGos0k!20FhZqRHEiz2-qFJ7J8XC4xX2qG-ajZ17Ma1W-VwDgwdQZeIhHEpWKlNldWW58K3fY4$ 

Signed-off-by: Robert Hancock <redacted>
---
 drivers/net/phy/broadcom.c | 83 ++++++++++++++++++++++++++++++++------
 include/linux/brcmphy.h    |  4 ++
 2 files changed, 75 insertions(+), 12 deletions(-)

diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 0472b3470c59..78542580f2b2 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c

@@ -64,6 +64,63 @@ 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) {

Can you reverse the condition so as to save a level of identation?

Can do.

quoted

quoted

+		/* 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;
+		rc = bcm54xx_auxctl_write(phydev,
MII_BCM54XX_AUXCTL_SHDWSEL_MISC,
+					  val);
+		if (rc < 0)
+			return rc;

I don't think this write is making it through since you are not setting
MII_BCM54XX_AUXCTL_MISC_WREN in val, I know this is an annoying detail,
and we could probably fold that to be within bcm54xx_auxctl_write()
directly, similarly to what bcm_phy_write_shadow() does.

Ah, indeed. I assume that is specific to the MII_BCM54XX_AUXCTL_SHDWSEL_MISC
register? I suppose bcm54xx_auxctl_write could add that automatically for
writes to that register. Not sure if that is too much magic for that function
or not..

Upon closer look it's a bit more subtle than that, as we need to apply
the write enable bit only when targeting the "misc" shadow register, a
million ways to die in the west :)
-- 
Florian