| Message ID | 20210213021840.2646187-3-robert.hancock@calian.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Broadcom PHY driver updates | expand |
| 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 |
On 2/12/2021 6:18 PM, Robert Hancock 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> Acked-by: Florian Fainelli <f.fainelli@gmail.com>
On Fri, Feb 12, 2021 at 08:18:40PM -0600, Robert Hancock wrote: > + if (!phydev->sfp_bus && > + (!phydev->attached_dev || !phydev->attached_dev->sfp_bus)) { First, do we want this to be repeated in every driver? Second, are you sure this is the correct condition to be using for this? phydev->sfp_bus is non-NULL when _this_ PHY has a SFP bus connected to its fibre side, it will never be set for a PHY on a SFP. The fact that it is non-NULL or NULL shouldn't have a bearing on whether we configure the LED register.
On Fri, Feb 12, 2021 at 08:18:40PM -0600, Robert Hancock 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. I agree with Russell here. You need to add a quirk to sfp.c which detects this specific SFP, and sets PHY flag before starting the PHY. Andrew
On Sat, 2021-02-13 at 10:45 +0000, Russell King - ARM Linux admin wrote: > On Fri, Feb 12, 2021 at 08:18:40PM -0600, Robert Hancock wrote: > > + if (!phydev->sfp_bus && > > + (!phydev->attached_dev || !phydev->attached_dev->sfp_bus)) { > > First, do we want this to be repeated in every driver? > > Second, are you sure this is the correct condition to be using for > this? phydev->sfp_bus is non-NULL when _this_ PHY has a SFP bus > connected to its fibre side, it will never be set for a PHY on a > SFP. The fact that it is non-NULL or NULL shouldn't have a bearing > on whether we configure the LED register. I think you're correct, the phydev->sfp_bus portion is probably not useful and could be dropped. What we're really concerned about is whether this PHY is on an SFP module or not. I'm not sure that a module-specific quirk makes sense here since there are probably other models which have a similar design where the LED outputs from the PHY are used for other purposes, and there's really no benefit to playing with the LED outputs on SFP modules in any case, so it would be safer to skip the LED reconfiguration for anything on an SFP. So we could either have a condition for "!phydev->attached_dev || !phydev->attached_dev- >sfp_bus" here and anywhere else that needs a similar check, or we do something different, like have a specific flag to indicate that this PHY is on an SFP module? What do people think is best here? >
On Tue, Feb 16, 2021 at 04:52:13PM +0000, Robert Hancock wrote: > On Sat, 2021-02-13 at 10:45 +0000, Russell King - ARM Linux admin wrote: > > On Fri, Feb 12, 2021 at 08:18:40PM -0600, Robert Hancock wrote: > > > + if (!phydev->sfp_bus && > > > + (!phydev->attached_dev || !phydev->attached_dev->sfp_bus)) { > > > > First, do we want this to be repeated in every driver? > > > > Second, are you sure this is the correct condition to be using for > > this? phydev->sfp_bus is non-NULL when _this_ PHY has a SFP bus > > connected to its fibre side, it will never be set for a PHY on a > > SFP. The fact that it is non-NULL or NULL shouldn't have a bearing > > on whether we configure the LED register. > > I think you're correct, the phydev->sfp_bus portion is probably not useful and > could be dropped. What we're really concerned about is whether this PHY is on > an SFP module or not. I'm not sure that a module-specific quirk makes sense > here since there are probably other models which have a similar design where > the LED outputs from the PHY are used for other purposes, and there's really no > benefit to playing with the LED outputs on SFP modules in any case, so it would > be safer to skip the LED reconfiguration for anything on an SFP. So we could > either have a condition for "!phydev->attached_dev || !phydev->attached_dev- > >sfp_bus" here and anywhere else that needs a similar check, or we do something > different, like have a specific flag to indicate that this PHY is on an SFP > module? What do people think is best here? I don't think relying on phydev->attached_dev in any way is a good idea, and I suspect a flag is going to be way better. One of the problems is that phydev->dev_flags are PHY specific at the moment. Not sure if we can do anything about that. In the short term, at the very least, I think we should wrap whatever test we use in a "phy_on_sfp(phydev)" helper so that we have a standard helper for this.
On 2/16/2021 9:04 AM, Russell King - ARM Linux admin wrote: > On Tue, Feb 16, 2021 at 04:52:13PM +0000, Robert Hancock wrote: >> On Sat, 2021-02-13 at 10:45 +0000, Russell King - ARM Linux admin wrote: >>> On Fri, Feb 12, 2021 at 08:18:40PM -0600, Robert Hancock wrote: >>>> + if (!phydev->sfp_bus && >>>> + (!phydev->attached_dev || !phydev->attached_dev->sfp_bus)) { >>> >>> First, do we want this to be repeated in every driver? >>> >>> Second, are you sure this is the correct condition to be using for >>> this? phydev->sfp_bus is non-NULL when _this_ PHY has a SFP bus >>> connected to its fibre side, it will never be set for a PHY on a >>> SFP. The fact that it is non-NULL or NULL shouldn't have a bearing >>> on whether we configure the LED register. >> >> I think you're correct, the phydev->sfp_bus portion is probably not useful and >> could be dropped. What we're really concerned about is whether this PHY is on >> an SFP module or not. I'm not sure that a module-specific quirk makes sense >> here since there are probably other models which have a similar design where >> the LED outputs from the PHY are used for other purposes, and there's really no >> benefit to playing with the LED outputs on SFP modules in any case, so it would >> be safer to skip the LED reconfiguration for anything on an SFP. So we could >> either have a condition for "!phydev->attached_dev || !phydev->attached_dev- >>> sfp_bus" here and anywhere else that needs a similar check, or we do something >> different, like have a specific flag to indicate that this PHY is on an SFP >> module? What do people think is best here? > > I don't think relying on phydev->attached_dev in any way is a good > idea, and I suspect a flag is going to be way better. One of the > problems is that phydev->dev_flags are PHY specific at the moment. > Not sure if we can do anything about that. I have some ideas on how we can improve that and hope to be able to post something by the end of the week. > > In the short term, at the very least, I think we should wrap whatever > test we use in a "phy_on_sfp(phydev)" helper so that we have a standard > helper for this. > Sounds reasonable.
diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c index 484791ac236b..9d73bfe0a1c2 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> @@ -366,18 +367,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(-)