diff mbox series

[net-next,2/2] net: phy: broadcom: Do not modify LED configuration for SFP module PHYs

Message ID 20210213002825.2557444-3-robert.hancock@calian.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Broadcom PHY driver updates | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 41 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

Robert Hancock Feb. 13, 2021, 12:28 a.m. UTC
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(-)

Comments

Florian Fainelli Feb. 13, 2021, 1:17 a.m. UTC | #1
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>
Vladimir Oltean Feb. 13, 2021, 1:41 a.m. UTC | #2
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 mbox series

Patch

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;
 }