| Message ID | 20210213002825.2557444-3-robert.hancock@calian.com |
|---|---|
| State | New |
| Headers | show |
| Series | Broadcom PHY driver updates | expand |
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>
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.
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; }
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(-)