* [PATCH net-next 0/2] Broadcom PHY driver updates
@ 2021-02-13 0:28 Robert Hancock
2021-02-13 0:28 ` [PATCH net-next 1/2] net: phy: broadcom: Set proper 1000BaseX/SGMII interface mode for BCM54616S Robert Hancock
2021-02-13 0:28 ` [PATCH net-next 2/2] net: phy: broadcom: Do not modify LED configuration for SFP module PHYs Robert Hancock
0 siblings, 2 replies; 8+ messages in thread
From: Robert Hancock @ 2021-02-13 0:28 UTC (permalink / raw)
To: andrew, hkallweit1, davem, kuba
Cc: f.fainelli, linux, bcm-kernel-feedback-list, netdev, Robert Hancock
Updates to the Broadcom PHY driver related to use with copper SFP modules.
Robert Hancock (2):
net: phy: broadcom: Set proper 1000BaseX/SGMII interface mode for
BCM54616S
net: phy: broadcom: Do not modify LED configuration for SFP module
PHYs
drivers/net/phy/broadcom.c | 109 ++++++++++++++++++++++++++++++-------
include/linux/brcmphy.h | 4 ++
2 files changed, 92 insertions(+), 21 deletions(-)
--
2.27.0
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH net-next 1/2] net: phy: broadcom: Set proper 1000BaseX/SGMII interface mode for BCM54616S 2021-02-13 0:28 [PATCH net-next 0/2] Broadcom PHY driver updates Robert Hancock @ 2021-02-13 0:28 ` Robert Hancock 2021-02-13 1:26 ` Florian Fainelli 2021-02-13 0:28 ` [PATCH net-next 2/2] net: phy: broadcom: Do not modify LED configuration for SFP module PHYs Robert Hancock 1 sibling, 1 reply; 8+ messages in thread From: Robert Hancock @ 2021-02-13 0:28 UTC (permalink / raw) To: andrew, hkallweit1, davem, kuba Cc: f.fainelli, linux, bcm-kernel-feedback-list, netdev, Robert Hancock 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 <robert.hancock@calian.com> --- 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) { + /* 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; + + /* 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); + } + return 0; +} + static int bcm54xx_config_clock_delay(struct phy_device *phydev) { int rc, val; @@ -283,15 +340,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 +358,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); @@ -385,7 +445,7 @@ static int bcm5481_config_aneg(struct phy_device *phydev) static int bcm54616s_probe(struct phy_device *phydev) { - int val, intf_sel; + int val; val = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE); if (val < 0) @@ -397,8 +457,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 de9430d55c90..b0a73b91d5ba 100644 --- a/include/linux/brcmphy.h +++ b/include/linux/brcmphy.h @@ -137,6 +137,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 @@ -223,6 +224,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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net-next 1/2] net: phy: broadcom: Set proper 1000BaseX/SGMII interface mode for BCM54616S 2021-02-13 0:28 ` [PATCH net-next 1/2] net: phy: broadcom: Set proper 1000BaseX/SGMII interface mode for BCM54616S Robert Hancock @ 2021-02-13 1:26 ` Florian Fainelli 2021-02-13 1:45 ` Robert Hancock 0 siblings, 1 reply; 8+ messages in thread From: Florian Fainelli @ 2021-02-13 1:26 UTC (permalink / raw) To: Robert Hancock, andrew, hkallweit1, davem, kuba Cc: f.fainelli, linux, bcm-kernel-feedback-list, netdev On 2/12/2021 4:28 PM, 'Robert Hancock' via BCM-KERNEL-FEEDBACK-LIST,PDL wrote: > 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 <robert.hancock@calian.com> > --- > 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? > + /* 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. The reset of the sequence and changes looks fine to me. -- Florian ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next 1/2] net: phy: broadcom: Set proper 1000BaseX/SGMII interface mode for BCM54616S 2021-02-13 1:26 ` Florian Fainelli @ 2021-02-13 1:45 ` Robert Hancock 2021-02-13 2:55 ` Florian Fainelli 0 siblings, 1 reply; 8+ messages in thread From: Robert Hancock @ 2021-02-13 1:45 UTC (permalink / raw) To: hkallweit1, davem, f.fainelli, kuba, andrew Cc: linux, netdev, bcm-kernel-feedback-list On Fri, 2021-02-12 at 17:26 -0800, Florian Fainelli wrote: > > On 2/12/2021 4:28 PM, 'Robert Hancock' via BCM-KERNEL-FEEDBACK-LIST,PDL > wrote: > > 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 <robert.hancock@calian.com> > > --- > > 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. > > > + /* 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.. > > The reset of the sequence and changes looks fine to me. -- Robert Hancock Senior Hardware Designer, Calian Advanced Technologies www.calian.com ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next 1/2] net: phy: broadcom: Set proper 1000BaseX/SGMII interface mode for BCM54616S 2021-02-13 1:45 ` Robert Hancock @ 2021-02-13 2:55 ` Florian Fainelli 0 siblings, 0 replies; 8+ messages in thread From: Florian Fainelli @ 2021-02-13 2:55 UTC (permalink / raw) To: Robert Hancock, hkallweit1, davem, kuba, andrew Cc: linux, netdev, bcm-kernel-feedback-list On 2/12/2021 5:45 PM, Robert Hancock wrote: > On Fri, 2021-02-12 at 17:26 -0800, Florian Fainelli wrote: >> >> On 2/12/2021 4:28 PM, 'Robert Hancock' via BCM-KERNEL-FEEDBACK-LIST,PDL >> wrote: >>> 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 <robert.hancock@calian.com> >>> --- >>> 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. > >> >>> + /* 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH net-next 2/2] net: phy: broadcom: Do not modify LED configuration for SFP module PHYs 2021-02-13 0:28 [PATCH net-next 0/2] Broadcom PHY driver updates Robert Hancock 2021-02-13 0:28 ` [PATCH net-next 1/2] net: phy: broadcom: Set proper 1000BaseX/SGMII interface mode for BCM54616S Robert Hancock @ 2021-02-13 0:28 ` Robert Hancock 2021-02-13 1:17 ` Florian Fainelli 1 sibling, 1 reply; 8+ messages in thread From: Robert Hancock @ 2021-02-13 0:28 UTC (permalink / raw) To: andrew, hkallweit1, davem, kuba Cc: f.fainelli, linux, bcm-kernel-feedback-list, netdev, Robert Hancock 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 <robert.hancock@calian.com> --- drivers/net/phy/broadcom.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c index 78542580f2b2..81a5721f732a 100644 --- a/drivers/net/phy/broadcom.c +++ b/drivers/net/phy/broadcom.c @@ -12,6 +12,7 @@ #include "bcm-phy-lib.h" #include <linux/module.h> +#include <linux/netdevice.h> #include <linux/phy.h> #include <linux/brcmphy.h> #include <linux/of.h> @@ -365,18 +366,25 @@ 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 that may be 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 (!phydev->sfp_bus && + (!phydev->attached_dev || !phydev->attached_dev->sfp_bus)) { + 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net-next 2/2] net: phy: broadcom: Do not modify LED configuration for SFP module PHYs 2021-02-13 0:28 ` [PATCH net-next 2/2] net: phy: broadcom: Do not modify LED configuration for SFP module PHYs Robert Hancock @ 2021-02-13 1:17 ` Florian Fainelli 2021-02-13 1:41 ` Vladimir Oltean 0 siblings, 1 reply; 8+ messages in thread From: Florian Fainelli @ 2021-02-13 1:17 UTC (permalink / raw) To: Robert Hancock, andrew, hkallweit1, davem, kuba Cc: f.fainelli, linux, bcm-kernel-feedback-list, netdev On 2/12/2021 4:28 PM, 'Robert Hancock' via BCM-KERNEL-FEEDBACK-LIST,PDL wrote: > 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 <robert.hancock@calian.com> > --- > drivers/net/phy/broadcom.c | 26 +++++++++++++++++--------- > 1 file changed, 17 insertions(+), 9 deletions(-) > > diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c > index 78542580f2b2..81a5721f732a 100644 > --- a/drivers/net/phy/broadcom.c > +++ b/drivers/net/phy/broadcom.c > @@ -12,6 +12,7 @@ > > #include "bcm-phy-lib.h" > #include <linux/module.h> > +#include <linux/netdevice.h> > #include <linux/phy.h> > #include <linux/brcmphy.h> > #include <linux/of.h> > @@ -365,18 +366,25 @@ 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 that may be 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 (!phydev->sfp_bus && > + (!phydev->attached_dev || !phydev->attached_dev->sfp_bus)) { > + 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); Not sure I can come up with a better solution but this should probably be made conditional upon the specific SFP module, or a set of specific SFP modules, whether we can convey those details via a Device Tree property or by doing a SFP ID lookup. Acked-by: Florian Fainelli <f.fainelli@gmail.com> -- Florian ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next 2/2] net: phy: broadcom: Do not modify LED configuration for SFP module PHYs 2021-02-13 1:17 ` Florian Fainelli @ 2021-02-13 1:41 ` Vladimir Oltean 0 siblings, 0 replies; 8+ messages in thread From: Vladimir Oltean @ 2021-02-13 1:41 UTC (permalink / raw) To: Florian Fainelli Cc: Robert Hancock, andrew, hkallweit1, davem, kuba, linux, bcm-kernel-feedback-list, netdev On Fri, Feb 12, 2021 at 05:17:53PM -0800, Florian Fainelli wrote: > On 2/12/2021 4:28 PM, 'Robert Hancock' via BCM-KERNEL-FEEDBACK-LIST,PDL > wrote: > > 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. That would be me, sorry. > > Signed-off-by: Robert Hancock <robert.hancock@calian.com> > > --- > > drivers/net/phy/broadcom.c | 26 +++++++++++++++++--------- > > 1 file changed, 17 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c > > index 78542580f2b2..81a5721f732a 100644 > > --- a/drivers/net/phy/broadcom.c > > +++ b/drivers/net/phy/broadcom.c > > @@ -12,6 +12,7 @@ > > > > #include "bcm-phy-lib.h" > > #include <linux/module.h> > > +#include <linux/netdevice.h> > > #include <linux/phy.h> > > #include <linux/brcmphy.h> > > #include <linux/of.h> > > @@ -365,18 +366,25 @@ 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 that may be 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 (!phydev->sfp_bus && > > + (!phydev->attached_dev || !phydev->attached_dev->sfp_bus)) { > > + 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); > > Not sure I can come up with a better solution but this should probably > be made conditional upon the specific SFP module, or a set of specific > SFP modules, whether we can convey those details via a Device Tree > property or by doing a SFP ID lookup. > > Acked-by: Florian Fainelli <f.fainelli@gmail.com> Not sure when is the last time I saw an SFP module with activity/link LEDs, the patch was intended for a BCM5464R RJ45 twisted pair setup which I still have. What is the last thing that happened in PHY LED territory? I lost track since Marek's RFC for offloading phy_led_triggers that didn't seem to make any progress. I could try to explicitly request the activity LED to blink on my board via device tree, if it helps. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-02-13 3:00 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-13 0:28 [PATCH net-next 0/2] Broadcom PHY driver updates Robert Hancock 2021-02-13 0:28 ` [PATCH net-next 1/2] net: phy: broadcom: Set proper 1000BaseX/SGMII interface mode for BCM54616S Robert Hancock 2021-02-13 1:26 ` Florian Fainelli 2021-02-13 1:45 ` Robert Hancock 2021-02-13 2:55 ` Florian Fainelli 2021-02-13 0:28 ` [PATCH net-next 2/2] net: phy: broadcom: Do not modify LED configuration for SFP module PHYs Robert Hancock 2021-02-13 1:17 ` Florian Fainelli 2021-02-13 1:41 ` Vladimir Oltean
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).