Message ID | 1559330285-30246-5-git-send-email-hancock@sedsystems.ca |
---|---|
State | Changes Requested |
Delegated to: | David Miller |
Headers | show |
Series | [net-next] net: phy: phylink: support using device PHY in fixed or 802.3z mode | expand |
So many changes to the same file that seem somehow related. Therefore, why didn't you put this stuff into a formal patch series?
On 2019-05-31 1:29 p.m., David Miller wrote: > > So many changes to the same file that seem somehow related. > > Therefore, why didn't you put this stuff into a formal patch series? The phy/phydev patches I submitted should all be independent changes that aren't directly related or go in any particular order. (Though I did notice I somehow managed to make some of them be reply chained to others - whoops..) They could potentially be a series though.
From: Robert Hancock <hancock@sedsystems.ca> Date: Fri, 31 May 2019 13:41:05 -0600 > On 2019-05-31 1:29 p.m., David Miller wrote: >> >> So many changes to the same file that seem somehow related. >> >> Therefore, why didn't you put this stuff into a formal patch series? > > The phy/phydev patches I submitted should all be independent changes > that aren't directly related or go in any particular order. (Though I > did notice I somehow managed to make some of them be reply chained to > others - whoops..) > > They could potentially be a series though. If they are truly independent I guess it's ok.
On Fri, May 31, 2019 at 01:18:05PM -0600, Robert Hancock wrote: > The Xilinx AXI Ethernet controller supports SFP modules in 1000BaseX > mode in a somewhat unusual manner: it still exposes a PHY device which > needs some PHY-level initialization for the PCS/PMA layer to work properly, > and which provides some link status/control information. > > In this case, we want to use the phylink layer to support proper > communication with the SFP module, but in most other respects we want to > use the PHY attached to the controller. > > Currently the phylink driver does not initialize or use a controller PHY > even if it exists for fixed-link or 802.3z PHY modes, and doesn't > support SFP module attachment in those modes. Sorry, I'm having a hard time following this description. Please draw an ASCII diagram of the setup you have - a picture is worth 1000 words, and I think that is very much the case here. We do have boards where the SFP is connected to a real PHY, where the real PHY offers a RJ45 copper socket and a fiber interface, automatically switching between the two. In this case, we do not use phylink to represent the link between the PHY and the SFP cage, but instead the PHY binds directly to the SFP cage. > This change allows it to > utilize a controller PHY if it is defined, and allows SFP module > attachment/initialization but does not connect the PHY device to the > controller (to allow the controller PHY to be used for link state > tracking). > > Fully supporting this setup would probably require initializing and > tracking the state of both PHYs, which is a much more complex change and > doesn't appear to be required for this use case. > > Signed-off-by: Robert Hancock <hancock@sedsystems.ca> > --- > drivers/net/phy/phylink.c | 29 +++++++++++++++++++---------- > 1 file changed, 19 insertions(+), 10 deletions(-) > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c > index 4fd72c2..9362aca 100644 > --- a/drivers/net/phy/phylink.c > +++ b/drivers/net/phy/phylink.c > @@ -819,12 +819,6 @@ int phylink_of_phy_connect(struct phylink *pl, struct device_node *dn, > struct phy_device *phy_dev; > int ret; > > - /* Fixed links and 802.3z are handled without needing a PHY */ > - if (pl->link_an_mode == MLO_AN_FIXED || > - (pl->link_an_mode == MLO_AN_INBAND && > - phy_interface_mode_is_8023z(pl->link_interface))) > - return 0; > - This looks to me like it will break existing users. > phy_node = of_parse_phandle(dn, "phy-handle", 0); > if (!phy_node) > phy_node = of_parse_phandle(dn, "phy", 0); > @@ -1697,9 +1691,6 @@ static int phylink_sfp_module_insert(void *upstream, > phy_modes(config.interface), > __ETHTOOL_LINK_MODE_MASK_NBITS, support); > > - if (phy_interface_mode_is_8023z(iface) && pl->phydev) > - return -EINVAL; > - > changed = !bitmap_equal(pl->supported, support, > __ETHTOOL_LINK_MODE_MASK_NBITS); > if (changed) { > @@ -1751,12 +1742,30 @@ static int phylink_sfp_connect_phy(void *upstream, struct phy_device *phy) > { > struct phylink *pl = upstream; > > + /* In fixed mode, or in in-band mode with 802.3z PHY interface mode, > + * ignore the SFP PHY and just use the PHY attached to the MAC. > + */ > + if (pl->link_an_mode == MLO_AN_FIXED || > + (pl->link_an_mode == MLO_AN_INBAND && > + phy_interface_mode_is_8023z(pl->link_config.interface))) > + return 0; > + > return __phylink_connect_phy(upstream, phy, pl->link_config.interface); > } > > static void phylink_sfp_disconnect_phy(void *upstream) > { > - phylink_disconnect_phy(upstream); > + struct phylink *pl = upstream; > + > + /* In fixed mode, or in in-band mode with 802.3z PHY interface mode, > + * ignore the SFP PHY and just use the PHY attached to the MAC. > + */ > + if (pl->link_an_mode == MLO_AN_FIXED || > + (pl->link_an_mode == MLO_AN_INBAND && > + phy_interface_mode_is_8023z(pl->link_config.interface))) > + return; Fixed link mode is mutually exclusive with there being a PHY present. Please see Documentation/devicetree/bindings/net/fixed-link.txt Fixed links are not used to fix a declared PHY to a specific mode. > + > + phylink_disconnect_phy(pl); > } > > static const struct sfp_upstream_ops sfp_phylink_ops = { Overall, I think you need to better describe what your setup is, and what you are trying to achieve: * Are you merely trying to support copper SFP modules where the PHY defaults to 1000base-X mode rather than SGMII? * Are you trying to support a network controller that doesn't support SGMII mode? If the former, then I'm pretty certain you're going about it the wrong way - as I've said before, there is nothing in the EEPROM that indicates definitively what format the control word is (and therefore whether it is SGMII or 1000base-X.) Some network controllers may be able to tell the difference, but that is not true of all controllers. The only way I can see to support such modules would be to have a table of quirks to set the interface mode accordingly, and not try this "lets pick one, try to validate it with the network controller, otherwise try the other." In any case, the format of the connection between the SFP module and the network controller isn't one that should appear in the ethtool link modes - I view what you've done there as a hack rather than proper design. If, on the other hand it is the latter, what you do you expect to happen if you plug a copper SFP module that only supports SGMII into a network controller that only supports 1000baseX ? The PHY on some of these modules won't pass data unless the SGMII handshake with the network controller completes, which it may or may not do depending on the 1000baseX implementation - but the network controller won't interpret the bits correctly, and certainly won't be able to deal with it when the link switches to 100M or 10M mode, which it will do depending on the results of the copper side negotiation.
On 2019-05-31 2:31 p.m., Russell King - ARM Linux admin wrote: > On Fri, May 31, 2019 at 01:18:05PM -0600, Robert Hancock wrote: >> The Xilinx AXI Ethernet controller supports SFP modules in 1000BaseX >> mode in a somewhat unusual manner: it still exposes a PHY device which >> needs some PHY-level initialization for the PCS/PMA layer to work properly, >> and which provides some link status/control information. >> >> In this case, we want to use the phylink layer to support proper >> communication with the SFP module, but in most other respects we want to >> use the PHY attached to the controller. >> >> Currently the phylink driver does not initialize or use a controller PHY >> even if it exists for fixed-link or 802.3z PHY modes, and doesn't >> support SFP module attachment in those modes. > > Sorry, I'm having a hard time following this description. Please draw > an ASCII diagram of the setup you have - a picture is worth 1000 words, > and I think that is very much the case here. > > We do have boards where the SFP is connected to a real PHY, where the > real PHY offers a RJ45 copper socket and a fiber interface, > automatically switching between the two. In this case, we do not > use phylink to represent the link between the PHY and the SFP cage, > but instead the PHY binds directly to the SFP cage. > It sounds like the setup you're describing has the PHY being smarter and doing more of the SFP module handling internally. In our setup, the situation is something like this: Xilinx MAC I2C GPIO | |GMII | | | | | Xilinx PHY | | | | | |1000BaseX | | | | | SFP ----------------------------- So in this case the Xilinx PHY is just really doing PCS/PMA, etc. conversions. The I2C and GPIO lines for the SFP socket are routed back to the host separately and the Xilinx PHY has no interaction with them (other than that I believe the LOS signal from the SFP socket is connected into the PHY to provide some link status indication back to it). In this setup, phylink is basically allowing us to communicate with the SFP module over I2C and manage the LOS, TX enable, etc. control lines properly, but the Xilinx PHY does need to be initialized as well for the actual link traffic to pass through. >> This change allows it to >> utilize a controller PHY if it is defined, and allows SFP module >> attachment/initialization but does not connect the PHY device to the >> controller (to allow the controller PHY to be used for link state >> tracking). >> >> Fully supporting this setup would probably require initializing and >> tracking the state of both PHYs, which is a much more complex change and >> doesn't appear to be required for this use case. >> >> Signed-off-by: Robert Hancock <hancock@sedsystems.ca> >> --- >> drivers/net/phy/phylink.c | 29 +++++++++++++++++++---------- >> 1 file changed, 19 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c >> index 4fd72c2..9362aca 100644 >> --- a/drivers/net/phy/phylink.c >> +++ b/drivers/net/phy/phylink.c >> @@ -819,12 +819,6 @@ int phylink_of_phy_connect(struct phylink *pl, struct device_node *dn, >> struct phy_device *phy_dev; >> int ret; >> >> - /* Fixed links and 802.3z are handled without needing a PHY */ >> - if (pl->link_an_mode == MLO_AN_FIXED || >> - (pl->link_an_mode == MLO_AN_INBAND && >> - phy_interface_mode_is_8023z(pl->link_interface))) >> - return 0; >> - > > This looks to me like it will break existing users. > >> phy_node = of_parse_phandle(dn, "phy-handle", 0); >> if (!phy_node) >> phy_node = of_parse_phandle(dn, "phy", 0); >> @@ -1697,9 +1691,6 @@ static int phylink_sfp_module_insert(void *upstream, >> phy_modes(config.interface), >> __ETHTOOL_LINK_MODE_MASK_NBITS, support); >> >> - if (phy_interface_mode_is_8023z(iface) && pl->phydev) >> - return -EINVAL; >> - >> changed = !bitmap_equal(pl->supported, support, >> __ETHTOOL_LINK_MODE_MASK_NBITS); >> if (changed) { >> @@ -1751,12 +1742,30 @@ static int phylink_sfp_connect_phy(void *upstream, struct phy_device *phy) >> { >> struct phylink *pl = upstream; >> >> + /* In fixed mode, or in in-band mode with 802.3z PHY interface mode, >> + * ignore the SFP PHY and just use the PHY attached to the MAC. >> + */ >> + if (pl->link_an_mode == MLO_AN_FIXED || >> + (pl->link_an_mode == MLO_AN_INBAND && >> + phy_interface_mode_is_8023z(pl->link_config.interface))) >> + return 0; >> + >> return __phylink_connect_phy(upstream, phy, pl->link_config.interface); >> } >> >> static void phylink_sfp_disconnect_phy(void *upstream) >> { >> - phylink_disconnect_phy(upstream); >> + struct phylink *pl = upstream; >> + >> + /* In fixed mode, or in in-band mode with 802.3z PHY interface mode, >> + * ignore the SFP PHY and just use the PHY attached to the MAC. >> + */ >> + if (pl->link_an_mode == MLO_AN_FIXED || >> + (pl->link_an_mode == MLO_AN_INBAND && >> + phy_interface_mode_is_8023z(pl->link_config.interface))) >> + return; > > Fixed link mode is mutually exclusive with there being a PHY present. > Please see Documentation/devicetree/bindings/net/fixed-link.txt > > Fixed links are not used to fix a declared PHY to a specific mode. I agree it would likely make sense to not include fixed mode in this case. > >> + >> + phylink_disconnect_phy(pl); >> } >> >> static const struct sfp_upstream_ops sfp_phylink_ops = { > > Overall, I think you need to better describe what your setup is, and > what you are trying to achieve: > > * Are you merely trying to support copper SFP modules where the PHY > defaults to 1000base-X mode rather than SGMII? > * Are you trying to support a network controller that doesn't support > SGMII mode? In our case the controller is supporting 1000BaseX only and is mainly intended for fiber modules. We do want to be able to get copper modules to work - obviously only ones that are set up for 1000BaseX mode are possible. > > If the former, then I'm pretty certain you're going about it the wrong > way - as I've said before, there is nothing in the EEPROM that > indicates definitively what format the control word is (and therefore > whether it is SGMII or 1000base-X.) > > Some network controllers may be able to tell the difference, but that > is not true of all controllers. > > The only way I can see to support such modules would be to have a table > of quirks to set the interface mode accordingly, and not try this "lets > pick one, try to validate it with the network controller, otherwise try > the other." > > In any case, the format of the connection between the SFP module and > the network controller isn't one that should appear in the ethtool link > modes - I view what you've done there as a hack rather than proper > design. > > If, on the other hand it is the latter, what you do you expect to > happen if you plug a copper SFP module that only supports SGMII into > a network controller that only supports 1000baseX ? The PHY on some > of these modules won't pass data unless the SGMII handshake with the > network controller completes, which it may or may not do depending on > the 1000baseX implementation - but the network controller won't > interpret the bits correctly, and certainly won't be able to deal > with it when the link switches to 100M or 10M mode, which it will do > depending on the results of the copper side negotiation. >
On Fri, May 31, 2019 at 06:33:32PM -0600, Robert Hancock wrote: > On 2019-05-31 2:31 p.m., Russell King - ARM Linux admin wrote: > > On Fri, May 31, 2019 at 01:18:05PM -0600, Robert Hancock wrote: > >> The Xilinx AXI Ethernet controller supports SFP modules in 1000BaseX > >> mode in a somewhat unusual manner: it still exposes a PHY device which > >> needs some PHY-level initialization for the PCS/PMA layer to work properly, > >> and which provides some link status/control information. > >> > >> In this case, we want to use the phylink layer to support proper > >> communication with the SFP module, but in most other respects we want to > >> use the PHY attached to the controller. > >> > >> Currently the phylink driver does not initialize or use a controller PHY > >> even if it exists for fixed-link or 802.3z PHY modes, and doesn't > >> support SFP module attachment in those modes. > > > > Sorry, I'm having a hard time following this description. Please draw > > an ASCII diagram of the setup you have - a picture is worth 1000 words, > > and I think that is very much the case here. > > > > We do have boards where the SFP is connected to a real PHY, where the > > real PHY offers a RJ45 copper socket and a fiber interface, > > automatically switching between the two. In this case, we do not > > use phylink to represent the link between the PHY and the SFP cage, > > but instead the PHY binds directly to the SFP cage. > > > > It sounds like the setup you're describing has the PHY being smarter and > doing more of the SFP module handling internally. In our setup, the > situation is something like this: > > Xilinx MAC I2C GPIO > | > |GMII | | > | | | > Xilinx PHY | | > | | | > |1000BaseX | | > | | | > SFP ----------------------------- That is very similar, except the Marvell 88x3310 uses a 10GBASE-R protocol to a SFP+ module, but can be switched to either SGMII or 1000BASE-X mode (neither of which we currently support, but work is in progress, if it turns out that these boards with strong pullups can work with SFP modules.) With the 88x3310, I have a couple of patches that "bolt on" to the PHY driver, so we end up with this setup from the DT, kernel and hardware point of view: ,-----> Copper RJ45 MAC -----> PHY `-----> SFP Hence, the PHY driver is responsible for registering itself as an "upstream" of the SFP cage without involving phylink - phylink gets used for the MAC <-> PHY part of the connection. There's an awkward problem though: the PHY driver doesn't really have much clue whether the network interface is up or down, which SFP really needs to know so it can control whether the SFP module's laser is emitting or not. One of the patches tweaks the phylink code to pass this information over to the SFP cage, around phylib, but the proper solution would be for phylib to propagate that information to the phylib driver, so that it can in turn pass that on to the SFP cage. However, there's a slightly bigger problem looming here, which is that phylib and the network layers in general do _not_ support having two PHYs actively bound to one network interface, and without radically reworking phylib and how phylib is bolted into the network layer, that is not easy to get around. > So in this case the Xilinx PHY is just really doing PCS/PMA, etc. > conversions. The I2C and GPIO lines for the SFP socket are routed back > to the host separately and the Xilinx PHY has no interaction with them > (other than that I believe the LOS signal from the SFP socket is > connected into the PHY to provide some link status indication back to it). So, very similar situation as on the Macchiatobin with the 88x3310 PHYs. > In this setup, phylink is basically allowing us to communicate with the > SFP module over I2C and manage the LOS, TX enable, etc. control lines > properly, but the Xilinx PHY does need to be initialized as well for the > actual link traffic to pass through. I think what you're missing is that the design is a layered one. All the SFP cage stuff is interfaced through the sfp layer, and is made accessible independently via the sfp-bus layer (which is needed to avoid sfp being a hard dependency for the MAC driver - especially important when we have SoCs such as Armada 8040 where one hardware module provides multiple network ports.) phylink provides a mechanism to separate PHYs from the MAC driver such that we can hot-plug PHYs (necessary for the PHYs on SFP modules), and deal with dynamically reconfiguring the MAC's hardware interface mode according to what the module supports. It isn't intended to always be closely bound to the SFP cage side. One of the reasons we have this design is because the early boards I had access to when designing this setup were direct MAC to SFP cage setups - there was no intermediate PHY. Then came the Macchiatobin board which does have a PHY, which brings with it additional complexities, but various hardware problems have stood in the way of having SFP modules in the 10G slots. > In our case the controller is supporting 1000BaseX only and is mainly > intended for fiber modules. We do want to be able to get copper modules > to work - obviously only ones that are set up for 1000BaseX mode are > possible. So, what I say below applies: > > If the former, then I'm pretty certain you're going about it the wrong > > way - as I've said before, there is nothing in the EEPROM that > > indicates definitively what format the control word is (and therefore > > whether it is SGMII or 1000base-X.) > > > > Some network controllers may be able to tell the difference, but that > > is not true of all controllers. > > > > The only way I can see to support such modules would be to have a table > > of quirks to set the interface mode accordingly, and not try this "lets > > pick one, try to validate it with the network controller, otherwise try > > the other." > > > > In any case, the format of the connection between the SFP module and > > the network controller isn't one that should appear in the ethtool link > > modes - I view what you've done there as a hack rather than proper > > design. I do have the beginnings of a quirk system for the sfp-bus layer, since I've been conversing with someone with a GPON module that does appear to follow the SFP MSA, in particular with regard to the time it takes the module to start responding on I2C, and in regard of the speeds it actually supports (basically, the EEPROM is misleading.) So, that should be useful for you as well. http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=phy is my playground of patches for SFP, which are in various stages of maturity, some which have been posted for inclusion (and merged) others that have been waiting some time.
On 2019-06-01 2:10 p.m., Russell King - ARM Linux admin wrote: >>> Sorry, I'm having a hard time following this description. Please draw >>> an ASCII diagram of the setup you have - a picture is worth 1000 words, >>> and I think that is very much the case here. >>> >>> We do have boards where the SFP is connected to a real PHY, where the >>> real PHY offers a RJ45 copper socket and a fiber interface, >>> automatically switching between the two. In this case, we do not >>> use phylink to represent the link between the PHY and the SFP cage, >>> but instead the PHY binds directly to the SFP cage. >>> >> >> It sounds like the setup you're describing has the PHY being smarter and >> doing more of the SFP module handling internally. In our setup, the >> situation is something like this: >> >> Xilinx MAC I2C GPIO >> | >> |GMII | | >> | | | >> Xilinx PHY | | >> | | | >> |1000BaseX | | >> | | | >> SFP ----------------------------- > > That is very similar, except the Marvell 88x3310 uses a 10GBASE-R > protocol to a SFP+ module, but can be switched to either SGMII or > 1000BASE-X mode (neither of which we currently support, but work is > in progress, if it turns out that these boards with strong pullups > can work with SFP modules.) > > With the 88x3310, I have a couple of patches that "bolt on" to the > PHY driver, so we end up with this setup from the DT, kernel and > hardware point of view: > > ,-----> Copper RJ45 > MAC -----> PHY > `-----> SFP > > Hence, the PHY driver is responsible for registering itself as an > "upstream" of the SFP cage without involving phylink - phylink gets > used for the MAC <-> PHY part of the connection. Looking at the patches you have on your branch, it looks like a similar sort of approach could work in our case. One difference however, is that the Marvell driver has its own internal PHY driver that knows about the SFP cage attachment, whereas axienet doesnt (right now we are using the generic PHY driver). Would it make sense for that SFP support to be added into the generic PHY layer? > > There's an awkward problem though: the PHY driver doesn't really have > much clue whether the network interface is up or down, which SFP > really needs to know so it can control whether the SFP module's laser > is emitting or not. One of the patches tweaks the phylink code to > pass this information over to the SFP cage, around phylib, but the > proper solution would be for phylib to propagate that information to > the phylib driver, so that it can in turn pass that on to the SFP cage. > > However, there's a slightly bigger problem looming here, which is that > phylib and the network layers in general do _not_ support having two > PHYs actively bound to one network interface, and without radically > reworking phylib and how phylib is bolted into the network layer, that > is not easy to get around.> >> So in this case the Xilinx PHY is just really doing PCS/PMA, etc. >> conversions. The I2C and GPIO lines for the SFP socket are routed back >> to the host separately and the Xilinx PHY has no interaction with them >> (other than that I believe the LOS signal from the SFP socket is >> connected into the PHY to provide some link status indication back to it). > > So, very similar situation as on the Macchiatobin with the 88x3310 > PHYs. > >> In this setup, phylink is basically allowing us to communicate with the >> SFP module over I2C and manage the LOS, TX enable, etc. control lines >> properly, but the Xilinx PHY does need to be initialized as well for the >> actual link traffic to pass through. > > I think what you're missing is that the design is a layered one. > All the SFP cage stuff is interfaced through the sfp layer, and is > made accessible independently via the sfp-bus layer (which is needed > to avoid sfp being a hard dependency for the MAC driver - especially > important when we have SoCs such as Armada 8040 where one hardware > module provides multiple network ports.) > > phylink provides a mechanism to separate PHYs from the MAC driver > such that we can hot-plug PHYs (necessary for the PHYs on SFP modules), > and deal with dynamically reconfiguring the MAC's hardware interface > mode according to what the module supports. It isn't intended to > always be closely bound to the SFP cage side. > > One of the reasons we have this design is because the early boards I > had access to when designing this setup were direct MAC to SFP cage > setups - there was no intermediate PHY. Then came the Macchiatobin > board which does have a PHY, which brings with it additional > complexities, but various hardware problems have stood in the way of > having SFP modules in the 10G slots. > >> In our case the controller is supporting 1000BaseX only and is mainly >> intended for fiber modules. We do want to be able to get copper modules >> to work - obviously only ones that are set up for 1000BaseX mode are >> possible. > > So, what I say below applies: > >>> If the former, then I'm pretty certain you're going about it the wrong >>> way - as I've said before, there is nothing in the EEPROM that >>> indicates definitively what format the control word is (and therefore >>> whether it is SGMII or 1000base-X.) >>> >>> Some network controllers may be able to tell the difference, but that >>> is not true of all controllers. >>> >>> The only way I can see to support such modules would be to have a table >>> of quirks to set the interface mode accordingly, and not try this "lets >>> pick one, try to validate it with the network controller, otherwise try >>> the other." >>> >>> In any case, the format of the connection between the SFP module and >>> the network controller isn't one that should appear in the ethtool link >>> modes - I view what you've done there as a hack rather than proper >>> design. > > I do have the beginnings of a quirk system for the sfp-bus layer, > since I've been conversing with someone with a GPON module that > does appear to follow the SFP MSA, in particular with regard to the > time it takes the module to start responding on I2C, and in regard > of the speeds it actually supports (basically, the EEPROM is > misleading.) So, that should be useful for you as well. > > http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=phy > > is my playground of patches for SFP, which are in various stages of > maturity, some which have been posted for inclusion (and merged) > others that have been waiting some time. >
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index 4fd72c2..9362aca 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -819,12 +819,6 @@ int phylink_of_phy_connect(struct phylink *pl, struct device_node *dn, struct phy_device *phy_dev; int ret; - /* Fixed links and 802.3z are handled without needing a PHY */ - if (pl->link_an_mode == MLO_AN_FIXED || - (pl->link_an_mode == MLO_AN_INBAND && - phy_interface_mode_is_8023z(pl->link_interface))) - return 0; - phy_node = of_parse_phandle(dn, "phy-handle", 0); if (!phy_node) phy_node = of_parse_phandle(dn, "phy", 0); @@ -1697,9 +1691,6 @@ static int phylink_sfp_module_insert(void *upstream, phy_modes(config.interface), __ETHTOOL_LINK_MODE_MASK_NBITS, support); - if (phy_interface_mode_is_8023z(iface) && pl->phydev) - return -EINVAL; - changed = !bitmap_equal(pl->supported, support, __ETHTOOL_LINK_MODE_MASK_NBITS); if (changed) { @@ -1751,12 +1742,30 @@ static int phylink_sfp_connect_phy(void *upstream, struct phy_device *phy) { struct phylink *pl = upstream; + /* In fixed mode, or in in-band mode with 802.3z PHY interface mode, + * ignore the SFP PHY and just use the PHY attached to the MAC. + */ + if (pl->link_an_mode == MLO_AN_FIXED || + (pl->link_an_mode == MLO_AN_INBAND && + phy_interface_mode_is_8023z(pl->link_config.interface))) + return 0; + return __phylink_connect_phy(upstream, phy, pl->link_config.interface); } static void phylink_sfp_disconnect_phy(void *upstream) { - phylink_disconnect_phy(upstream); + struct phylink *pl = upstream; + + /* In fixed mode, or in in-band mode with 802.3z PHY interface mode, + * ignore the SFP PHY and just use the PHY attached to the MAC. + */ + if (pl->link_an_mode == MLO_AN_FIXED || + (pl->link_an_mode == MLO_AN_INBAND && + phy_interface_mode_is_8023z(pl->link_config.interface))) + return; + + phylink_disconnect_phy(pl); } static const struct sfp_upstream_ops sfp_phylink_ops = {
The Xilinx AXI Ethernet controller supports SFP modules in 1000BaseX mode in a somewhat unusual manner: it still exposes a PHY device which needs some PHY-level initialization for the PCS/PMA layer to work properly, and which provides some link status/control information. In this case, we want to use the phylink layer to support proper communication with the SFP module, but in most other respects we want to use the PHY attached to the controller. Currently the phylink driver does not initialize or use a controller PHY even if it exists for fixed-link or 802.3z PHY modes, and doesn't support SFP module attachment in those modes. This change allows it to utilize a controller PHY if it is defined, and allows SFP module attachment/initialization but does not connect the PHY device to the controller (to allow the controller PHY to be used for link state tracking). Fully supporting this setup would probably require initializing and tracking the state of both PHYs, which is a much more complex change and doesn't appear to be required for this use case. Signed-off-by: Robert Hancock <hancock@sedsystems.ca> --- drivers/net/phy/phylink.c | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-)