| Message ID | 20210216225454.2944373-1-robert.hancock@calian.com |
|---|---|
| Headers | show |
| Series | Broadcom PHY driver updates | expand |
Hello: This series was applied to netdev/net-next.git (refs/heads/master): On Tue, 16 Feb 2021 16:54:51 -0600 you wrote: > Updates to the Broadcom PHY driver related to use with copper SFP modules. > > Changed since v3: > -fixed kerneldoc error > > Changed since v2: > -Create flag for PHY on SFP module and use that rather than accessing > attached_dev directly in PHY driver > > [...] Here is the summary with links: - [net-next,v4,1/3] net: phy: broadcom: Set proper 1000BaseX/SGMII interface mode for BCM54616S https://git.kernel.org/netdev/net-next/c/3afd0218992a - [net-next,v4,2/3] net: phy: Add is_on_sfp_module flag and phy_on_sfp helper https://git.kernel.org/netdev/net-next/c/b834489bcecc - [net-next,v4,3/3] net: phy: broadcom: Do not modify LED configuration for SFP module PHYs https://git.kernel.org/netdev/net-next/c/b5d007e2aac8 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
On Tue, Feb 16, 2021 at 04:54:53PM -0600, Robert Hancock wrote: > Add a flag and helper function to indicate that a PHY device is part of > an SFP module, which is set on attach. This can be used by PHY drivers > to handle SFP-specific quirks or behavior. > > Signed-off-by: Robert Hancock <robert.hancock@calian.com> > --- > drivers/net/phy/phy_device.c | 2 ++ > include/linux/phy.h | 11 +++++++++++ > 2 files changed, 13 insertions(+) > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index 05261698bf74..d6ac3ed38197 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -1377,6 +1377,8 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev, > > if (phydev->sfp_bus_attached) > dev->sfp_bus = phydev->sfp_bus; > + else if (dev->sfp_bus) > + phydev->is_on_sfp_module = true; I get lost here. It this correct? We have setups with two PHY. The marvell10g PHY can play the role of media converter. It converts the signal from the MAC into signals which can be connected to an SFP cage. And then inside the cage, we can have a copper module with a second PHY. It is this second PHY which we need to indicate is_on_sfp_module true. We probably want to media convert PHYs LEDs to work, since those possible are connected to the front panel. So this needs to be specific to the SFP module PHY, and i'm not sure it is. Russell? Andrew
On Wed, Feb 17, 2021 at 04:58:26AM +0100, Andrew Lunn wrote: > On Tue, Feb 16, 2021 at 04:54:53PM -0600, Robert Hancock wrote: > > Add a flag and helper function to indicate that a PHY device is part of > > an SFP module, which is set on attach. This can be used by PHY drivers > > to handle SFP-specific quirks or behavior. > > > > Signed-off-by: Robert Hancock <robert.hancock@calian.com> > > --- > > drivers/net/phy/phy_device.c | 2 ++ > > include/linux/phy.h | 11 +++++++++++ > > 2 files changed, 13 insertions(+) > > > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > > index 05261698bf74..d6ac3ed38197 100644 > > --- a/drivers/net/phy/phy_device.c > > +++ b/drivers/net/phy/phy_device.c > > @@ -1377,6 +1377,8 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev, > > > > if (phydev->sfp_bus_attached) > > dev->sfp_bus = phydev->sfp_bus; > > + else if (dev->sfp_bus) > > + phydev->is_on_sfp_module = true; > > I get lost here. It this correct? > > We have setups with two PHY. The marvell10g PHY can play the role of > media converter. It converts the signal from the MAC into signals > which can be connected to an SFP cage. > > And then inside the cage, we can have a copper module with a second > PHY. It is this second PHY which we need to indicate is_on_sfp_module > true. We don't support that setup, at least at the moment. There's no support for stacking PHYs precisely because we have the net_device structure containing a pointer to the PHY (which will be the first PHY in the chain.) That has the effect of making the second PHY inaccessible to the normal network APIs. > We probably want to media convert PHYs LEDs to work, since those > possible are connected to the front panel. So this needs to be > specific to the SFP module PHY, and i'm not sure it is. Russell? The main reason I'm hessitant with using the net_device structure to detect this is that we know that phylink has been converted to work without the net_device structure - it will be NULL. This includes attaching the "primary" PHY - phylib will be given a NULL net_device. The good news is that if a SFP cage is attempted to be attached in that situation, phylink will oops in phylink_sfp_attach(). So it isn't something that we support. However, the point is that we can end up in situations where phylib has a NULL net_device. Florian mentioned in response to one of my previous emails that he's working on sorting out the phylib dev_flags - I think we should wait for that to be resolved first, so we can allocate a dev_flag (or maybe we can do that already today?) that says "this PHY is part of a SFP module". That will give us a clearly defined positive condition that is more maintainable into the future. I'm just worrying that if we sort out phylink_sfp_*tach() (which could be trivial), if we introduce new dependencies into phylib on the network device, we're moving backwards. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On 2/17/2021 2:34 AM, Russell King - ARM Linux admin wrote: > On Wed, Feb 17, 2021 at 04:58:26AM +0100, Andrew Lunn wrote: >> On Tue, Feb 16, 2021 at 04:54:53PM -0600, Robert Hancock wrote: >>> Add a flag and helper function to indicate that a PHY device is part of >>> an SFP module, which is set on attach. This can be used by PHY drivers >>> to handle SFP-specific quirks or behavior. >>> >>> Signed-off-by: Robert Hancock <robert.hancock@calian.com> >>> --- >>> drivers/net/phy/phy_device.c | 2 ++ >>> include/linux/phy.h | 11 +++++++++++ >>> 2 files changed, 13 insertions(+) >>> >>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c >>> index 05261698bf74..d6ac3ed38197 100644 >>> --- a/drivers/net/phy/phy_device.c >>> +++ b/drivers/net/phy/phy_device.c >>> @@ -1377,6 +1377,8 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev, >>> >>> if (phydev->sfp_bus_attached) >>> dev->sfp_bus = phydev->sfp_bus; >>> + else if (dev->sfp_bus) >>> + phydev->is_on_sfp_module = true; >> >> I get lost here. It this correct? >> >> We have setups with two PHY. The marvell10g PHY can play the role of >> media converter. It converts the signal from the MAC into signals >> which can be connected to an SFP cage. >> >> And then inside the cage, we can have a copper module with a second >> PHY. It is this second PHY which we need to indicate is_on_sfp_module >> true. > > We don't support that setup, at least at the moment. There's no support > for stacking PHYs precisely because we have the net_device structure > containing a pointer to the PHY (which will be the first PHY in the > chain.) That has the effect of making the second PHY inaccessible to the > normal network APIs. > >> We probably want to media convert PHYs LEDs to work, since those >> possible are connected to the front panel. So this needs to be >> specific to the SFP module PHY, and i'm not sure it is. Russell? > > The main reason I'm hessitant with using the net_device structure > to detect this is that we know that phylink has been converted to > work without the net_device structure - it will be NULL. This includes > attaching the "primary" PHY - phylib will be given a NULL net_device. > > The good news is that if a SFP cage is attempted to be attached in > that situation, phylink will oops in phylink_sfp_attach(). So it > isn't something that we support. However, the point is that we can > end up in situations where phylib has a NULL net_device. > > Florian mentioned in response to one of my previous emails that he's > working on sorting out the phylib dev_flags - I think we should wait > for that to be resolved first, so we can allocate a dev_flag (or > maybe we can do that already today?) that says "this PHY is part of > a SFP module". That will give us a clearly defined positive condition > that is more maintainable into the future. We could be allocating a generic flag today starting from bit 31 and down and that would certainly be fine without necessarily making my rework any harder. The existing issues with phydev->dev_flags as I am sure you are all aware is that the flags are not name-spaced per driver yet all Ethernet MAC drivers make assumptions (tg3.c, bcmgenet.c, etc.) and it is not possible to pass arbitrary configuration settings down the PHY driver, etc. I would not hold my breath on this rework as I keep getting sucked into various issues at work. FWIW, this series appears to have already been applied. -- Florian
Updates to the Broadcom PHY driver related to use with copper SFP modules. Changed since v3: -fixed kerneldoc error Changed since v2: -Create flag for PHY on SFP module and use that rather than accessing attached_dev directly in PHY driver 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 (3): net: phy: broadcom: Set proper 1000BaseX/SGMII interface mode for BCM54616S net: phy: Add is_on_sfp_module flag and phy_on_sfp helper net: phy: broadcom: Do not modify LED configuration for SFP module PHYs drivers/net/phy/broadcom.c | 108 ++++++++++++++++++++++++++++------- drivers/net/phy/phy_device.c | 2 + include/linux/brcmphy.h | 4 ++ include/linux/phy.h | 11 ++++ 4 files changed, 104 insertions(+), 21 deletions(-)