* [PATCH net-next v2 0/2] Broadcom PHY driver updates
@ 2021-02-13 2:18 Robert Hancock
2021-02-13 2:18 ` [PATCH net-next v2 1/2] net: phy: broadcom: Set proper 1000BaseX/SGMII interface mode for BCM54616S Robert Hancock
2021-02-13 2:18 ` [PATCH net-next v2 2/2] net: phy: broadcom: Do not modify LED configuration for SFP module PHYs Robert Hancock
0 siblings, 2 replies; 10+ messages in thread
From: Robert Hancock @ 2021-02-13 2:18 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.
Changed since v1:
-Reversed conditional to reduce indentation
-Added missing setting of MII_BCM54XX_AUXCTL_MISC_WREN in
MII_BCM54XX_AUXCTL_SHDWSEL_MISC register
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 | 110 ++++++++++++++++++++++++++++++-------
include/linux/brcmphy.h | 4 ++
2 files changed, 93 insertions(+), 21 deletions(-)
--
2.27.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH net-next v2 1/2] net: phy: broadcom: Set proper 1000BaseX/SGMII interface mode for BCM54616S
2021-02-13 2:18 [PATCH net-next v2 0/2] Broadcom PHY driver updates Robert Hancock
@ 2021-02-13 2:18 ` Robert Hancock
2021-02-13 2:53 ` Florian Fainelli
2021-02-13 2:18 ` [PATCH net-next v2 2/2] net: phy: broadcom: Do not modify LED configuration for SFP module PHYs Robert Hancock
1 sibling, 1 reply; 10+ messages in thread
From: Robert Hancock @ 2021-02-13 2:18 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 | 84 ++++++++++++++++++++++++++++++++------
include/linux/brcmphy.h | 4 ++
2 files changed, 76 insertions(+), 12 deletions(-)
diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 0472b3470c59..484791ac236b 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -64,6 +64,64 @@ 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)
+ return 0;
+
+ /* 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;
+ val |= MII_BCM54XX_AUXCTL_MISC_WREN;
+ 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);
+}
+
static int bcm54xx_config_clock_delay(struct phy_device *phydev)
{
int rc, val;
@@ -283,15 +341,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 +359,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 +446,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 +458,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] 10+ messages in thread
* [PATCH net-next v2 2/2] net: phy: broadcom: Do not modify LED configuration for SFP module PHYs
2021-02-13 2:18 [PATCH net-next v2 0/2] Broadcom PHY driver updates Robert Hancock
2021-02-13 2:18 ` [PATCH net-next v2 1/2] net: phy: broadcom: Set proper 1000BaseX/SGMII interface mode for BCM54616S Robert Hancock
@ 2021-02-13 2:18 ` Robert Hancock
2021-02-13 2:54 ` Florian Fainelli
` (2 more replies)
1 sibling, 3 replies; 10+ messages in thread
From: Robert Hancock @ 2021-02-13 2:18 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 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;
}
--
2.27.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net-next v2 1/2] net: phy: broadcom: Set proper 1000BaseX/SGMII interface mode for BCM54616S
2021-02-13 2:18 ` [PATCH net-next v2 1/2] net: phy: broadcom: Set proper 1000BaseX/SGMII interface mode for BCM54616S Robert Hancock
@ 2021-02-13 2:53 ` Florian Fainelli
0 siblings, 0 replies; 10+ messages in thread
From: Florian Fainelli @ 2021-02-13 2:53 UTC (permalink / raw)
To: Robert Hancock, andrew, hkallweit1, davem, kuba
Cc: linux, bcm-kernel-feedback-list, netdev
On 2/12/2021 6:18 PM, Robert Hancock 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>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next v2 2/2] net: phy: broadcom: Do not modify LED configuration for SFP module PHYs
2021-02-13 2:18 ` [PATCH net-next v2 2/2] net: phy: broadcom: Do not modify LED configuration for SFP module PHYs Robert Hancock
@ 2021-02-13 2:54 ` Florian Fainelli
2021-02-13 10:45 ` Russell King - ARM Linux admin
2021-02-13 16:49 ` Andrew Lunn
2 siblings, 0 replies; 10+ messages in thread
From: Florian Fainelli @ 2021-02-13 2:54 UTC (permalink / raw)
To: Robert Hancock, andrew, hkallweit1, davem, kuba
Cc: linux, bcm-kernel-feedback-list, netdev
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>
--
Florian
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next v2 2/2] net: phy: broadcom: Do not modify LED configuration for SFP module PHYs
2021-02-13 2:18 ` [PATCH net-next v2 2/2] net: phy: broadcom: Do not modify LED configuration for SFP module PHYs Robert Hancock
2021-02-13 2:54 ` Florian Fainelli
@ 2021-02-13 10:45 ` Russell King - ARM Linux admin
2021-02-16 16:52 ` Robert Hancock
2021-02-13 16:49 ` Andrew Lunn
2 siblings, 1 reply; 10+ messages in thread
From: Russell King - ARM Linux admin @ 2021-02-13 10:45 UTC (permalink / raw)
To: Robert Hancock
Cc: andrew, hkallweit1, davem, kuba, f.fainelli,
bcm-kernel-feedback-list, netdev
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.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next v2 2/2] net: phy: broadcom: Do not modify LED configuration for SFP module PHYs
2021-02-13 2:18 ` [PATCH net-next v2 2/2] net: phy: broadcom: Do not modify LED configuration for SFP module PHYs Robert Hancock
2021-02-13 2:54 ` Florian Fainelli
2021-02-13 10:45 ` Russell King - ARM Linux admin
@ 2021-02-13 16:49 ` Andrew Lunn
2 siblings, 0 replies; 10+ messages in thread
From: Andrew Lunn @ 2021-02-13 16:49 UTC (permalink / raw)
To: Robert Hancock
Cc: hkallweit1, davem, kuba, f.fainelli, linux,
bcm-kernel-feedback-list, netdev
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next v2 2/2] net: phy: broadcom: Do not modify LED configuration for SFP module PHYs
2021-02-13 10:45 ` Russell King - ARM Linux admin
@ 2021-02-16 16:52 ` Robert Hancock
2021-02-16 17:04 ` Russell King - ARM Linux admin
0 siblings, 1 reply; 10+ messages in thread
From: Robert Hancock @ 2021-02-16 16:52 UTC (permalink / raw)
To: linux
Cc: bcm-kernel-feedback-list, hkallweit1, davem, f.fainelli, kuba,
andrew, netdev
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?
>
--
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
www.calian.com
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next v2 2/2] net: phy: broadcom: Do not modify LED configuration for SFP module PHYs
2021-02-16 16:52 ` Robert Hancock
@ 2021-02-16 17:04 ` Russell King - ARM Linux admin
2021-02-16 17:05 ` Florian Fainelli
0 siblings, 1 reply; 10+ messages in thread
From: Russell King - ARM Linux admin @ 2021-02-16 17:04 UTC (permalink / raw)
To: Robert Hancock
Cc: bcm-kernel-feedback-list, hkallweit1, davem, f.fainelli, kuba,
andrew, netdev
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.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next v2 2/2] net: phy: broadcom: Do not modify LED configuration for SFP module PHYs
2021-02-16 17:04 ` Russell King - ARM Linux admin
@ 2021-02-16 17:05 ` Florian Fainelli
0 siblings, 0 replies; 10+ messages in thread
From: Florian Fainelli @ 2021-02-16 17:05 UTC (permalink / raw)
To: Russell King - ARM Linux admin, Robert Hancock
Cc: bcm-kernel-feedback-list, hkallweit1, davem, f.fainelli, kuba,
andrew, netdev
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.
--
Florian
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-02-16 17:06 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-13 2:18 [PATCH net-next v2 0/2] Broadcom PHY driver updates Robert Hancock
2021-02-13 2:18 ` [PATCH net-next v2 1/2] net: phy: broadcom: Set proper 1000BaseX/SGMII interface mode for BCM54616S Robert Hancock
2021-02-13 2:53 ` Florian Fainelli
2021-02-13 2:18 ` [PATCH net-next v2 2/2] net: phy: broadcom: Do not modify LED configuration for SFP module PHYs Robert Hancock
2021-02-13 2:54 ` Florian Fainelli
2021-02-13 10:45 ` Russell King - ARM Linux admin
2021-02-16 16:52 ` Robert Hancock
2021-02-16 17:04 ` Russell King - ARM Linux admin
2021-02-16 17:05 ` Florian Fainelli
2021-02-13 16:49 ` Andrew Lunn
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).