| Message ID | 20210212172341.3489046-2-olteanv@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Let phylink manage in-band AN for the PHY | expand |
On Fri, Feb 12, 2021 at 11:40:59PM +0100, Michael Walle wrote: > Fun fact, now it may be the other way around. If the bootloader doesn't > configure it and the PHY isn't reset by the hardware, it won't work in > the bootloader after a reboot ;) If we start messing around with the configuration of PHYs in that regard, we could be opening ourselves up for a world of pain... > If you disable aneg between MAC and PHY, what would be the actual speed > setting/duplex mode then? I guess it have to match the external speed? That is a function of the interface mode and the PHY capabilities. 1) if the PHY supports rate adaption, and is programmed for that, then the PHY link normally operates at a fixed speed (e.g. 1G for SGMII) and the PHY converts to the appropriate speed. We don't actually support this per se, since the parameters we give to the MAC via mac_link_up() are the media side parameters, not the link parameters. 2) if the PHY does not support rate adaption, then the MAC to PHY link needs to follow the media speed and duplex. phylink will be in "PHY" mode, where it passes the media side negotiation results to the MAC just like phylib would, and the MAC should be programmed appropriately. In the case of a SGMII link, the link needs to be programmed to do the appropriate symbol repetition for 100M and 10M speeds. The PHY /should/ do that automatically, but if it doesn't, then the PHY also needs to be programmed to conform. (since if there's no rate adaption in the PHY, the MAC side and the media side must match.) Hope that helps.
On Fri, Feb 12, 2021 at 11:40:59PM +0100, Michael Walle wrote: > Am 2021-02-12 18:23, schrieb Vladimir Oltean: > > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > > > Currently Linux has no control over whether a MAC-to-PHY interface uses > > in-band signaling or not, even though phylink has the > > managed = "in-band-status"; > > property which denotes that the MAC expects in-band signaling to be > > used. > > > > The problem is really that if the in-band signaling is configurable in > > both the PHY and the MAC, there is a risk that they are out of sync > > unless phylink manages them both. Most if not all in-band autoneg state > > machines follow IEEE 802.3 clause 37, which means that they will not > > change the operating mode of the SERDES lane from control to data mode > > unless in-band AN completed successfully. Therefore traffic will not > > work. > > > > It is particularly unpleasant that currently, we assume that PHYs which > > have configurable in-band AN come pre-configured from a prior boot stage > > such as U-Boot, because once the bootloader changes, all bets are off. > > Fun fact, now it may be the other way around. If the bootloader doesn't > configure it and the PHY isn't reset by the hardware, it won't work in > the bootloader after a reboot ;) My understanding is that this is precisely the reason why the U-Boot people don't want to support booting from RAM, and want to assume that the nothing else ran between Power On Reset and the bootloader: https://www.denx.de/wiki/view/DULG/CanUBootBeConfiguredSuchThatItCanBeStartedInRAM [ that does make me wonder what they think about ARM TF-A ] > > Let's introduce a new PHY driver method for configuring in-band autoneg, > > and make phylink be its first user. The main PHY library does not call > > phy_config_inband_autoneg, because it does not know what to configure it > > to. Presumably, non-phylink drivers can also call > > phy_config_inband_autoneg > > individually. > > If you disable aneg between MAC and PHY, what would be the actual speed > setting/duplex mode then? I guess it have to match the external speed? > > I'm trying this on the AT8031. I've removed 'managed = "in-band-status";' > for the PHY. Confirmed that it won't work and then I've implemented your > new callback. That will disable the SGMII aneg (which is done via the > BMCR of fiber page if I'm not entirely mistaken); ethernet will then > work again. But only for gigabit. I presume because the speed setting > of the SGMII link is set to gigabit. Which MAC driver are you testing on? Are you saying that it doesn't force the link to the speed resolved over MDIO and passed to .phylink_mac_link_up, or that the speed communicated to it is incorrect?
On Fri, Feb 12, 2021 at 07:23:40PM +0200, Vladimir Oltean wrote: > + ret = phy_config_inband_aneg(phy, > + (pl->cur_link_an_mode == MLO_AN_INBAND)); Please use phylink_autoneg_inband(pl->cur_link_an_mode) here. > + if (ret && ret != -EOPNOTSUPP) { > + phylink_warn(pl, "failed to configure PHY in-band autoneg: %d\n", > + ret); Please use %pe and ERR_PTR(ret) so we can get a symbolic errno value. As mentioned in this thread, we have at least one PHY which is unable to provide the inband signalling in any mode (BCM84881). Currently, phylink detects this PHY on a SFP (in phylink_phy_no_inband()) and adjusts not to use inband mode. This would need to be addressed if we are creating an alterative way to discover whether the PHY supports inband mode or not. Also, there needs to be consideration of PHYs that dynamically change their interface type, and whether they support inband signalling. For example, a PHY may support a mode where it dynamically selects between 10GBASE-R, 5GBASE-R, 2500BASE-X and SGMII, where the SGMII mode may have inband signalling enabled or disabled. This is not a theoretical case; we have a PHY like that supported in the kernel and boards use it. What would the semantics of your new call be for a PHY that performs this? Should we also have a phydev->inband tristate, taking values "unknown, enabled, disabled" which the PHY driver is required to update in their read_status callback if they dynamically change their interface type? (Although then phylink will need to figure out how to deal with that.) -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Sun, Feb 14, 2021 at 10:35:29AM +0000, Russell King - ARM Linux admin wrote: > > + if (ret && ret != -EOPNOTSUPP) { > > + phylink_warn(pl, "failed to configure PHY in-band autoneg: %d\n", > > + ret); > > Please use %pe and ERR_PTR(ret) so we can get a symbolic errno value. I didn't know that was possible, thanks for the hint. > As mentioned in this thread, we have at least one PHY which is unable > to provide the inband signalling in any mode (BCM84881). Currently, > phylink detects this PHY on a SFP (in phylink_phy_no_inband()) and > adjusts not to use inband mode. This would need to be addressed if we > are creating an alterative way to discover whether the PHY supports > inband mode or not. So I haven't studied the SFP code path too deeply, but I think part of the issue is the order in which things are done. It's almost as if there should be a validation phase for PHY inband abilities too. phylink_sfp_connect_phy -> phylink_sfp_config: -> first this checks if phylink_phy_no_inband -> then this changes pl->link_config.interface and pl->cur_link_an_mode -> phylink_bringup_phy: -> this is where I'm adding the new phy_config_inband_aneg function If we were to use only my phy_config_inband_aneg function, it would need to be moved upwards in the code path, to be precise where phylink_phy_no_inband currently is. Then we'd have to try MLO_AN_INBAND first, with a fallback to MLO_AN_PHY if that fails. I think this would unnecessarily complicate the code. Alternatively, I could create a second PHY driver method, simply for validation of supported inband modes. The validation can be done in place of the current phylink_phy_noinband(), and I can still have the phy_config_inband_aneg() where I put it now, since we shouldn't have any surprises w.r.t. supported operating mode, and there should be no reason to repeat the attempt as there would be with a single PHY driver method. Thoughts? > Also, there needs to be consideration of PHYs that dynamically change > their interface type, and whether they support inband signalling. > For example, a PHY may support a mode where it dynamically selects > between 10GBASE-R, 5GBASE-R, 2500BASE-X and SGMII, where the SGMII > mode may have inband signalling enabled or disabled. This is not a > theoretical case; we have a PHY like that supported in the kernel and > boards use it. What would the semantics of your new call be for a PHY > that performs this? > > Should we also have a phydev->inband tristate, taking values "unknown, > enabled, disabled" which the PHY driver is required to update in their > read_status callback if they dynamically change their interface type? > (Although then phylink will need to figure out how to deal with that.) I don't have such PHY to test with, but I think the easiest way would be to just call the validation method again, after we change the phydev->interface value. The PHY driver can easily take phydev->interface into consideration when answering the question "is inband aneg supported or not". I don't think that making phydev->inband a stateful value is going to be as useful as making it a function, since as you say, we will be required to keep it up to date from generic PHY driver methods, but only phylink will use it.
On Sun, Feb 14, 2021 at 01:10:14PM +0200, Vladimir Oltean wrote: > On Sun, Feb 14, 2021 at 10:35:29AM +0000, Russell King - ARM Linux admin wrote: > > As mentioned in this thread, we have at least one PHY which is unable > > to provide the inband signalling in any mode (BCM84881). Currently, > > phylink detects this PHY on a SFP (in phylink_phy_no_inband()) and > > adjusts not to use inband mode. This would need to be addressed if we > > are creating an alterative way to discover whether the PHY supports > > inband mode or not. > > So I haven't studied the SFP code path too deeply, but I think part of > the issue is the order in which things are done. It's almost as if there > should be a validation phase for PHY inband abilities too. > > phylink_sfp_connect_phy > -> phylink_sfp_config: > -> first this checks if phylink_phy_no_inband > -> then this changes pl->link_config.interface and pl->cur_link_an_mode > -> phylink_bringup_phy: > -> this is where I'm adding the new phy_config_inband_aneg function > > If we were to use only my phy_config_inband_aneg function, it would need > to be moved upwards in the code path, to be precise where phylink_phy_no_inband > currently is. Then we'd have to try MLO_AN_INBAND first, with a fallback > to MLO_AN_PHY if that fails. I think this would unnecessarily complicate > the code. There's another possibility - we could postpone the actual configuration on the MAC side to always be in phylink_sfp_module_start(), essentially moving the phylink_mac_initial_config() call to that point while preserving the current locations where we compute the initial interface mode. We can then defer the AN mode selection. > Alternatively, I could create a second PHY driver method, simply for > validation of supported inband modes. The validation can be done in > place of the current phylink_phy_noinband(), and I can still have the > phy_config_inband_aneg() where I put it now, since we shouldn't have any > surprises w.r.t. supported operating mode, and there should be no reason > to repeat the attempt as there would be with a single PHY driver method. > Thoughts? That also sounds like it should work, and would probably be more flexible. > > Also, there needs to be consideration of PHYs that dynamically change > > their interface type, and whether they support inband signalling. > > For example, a PHY may support a mode where it dynamically selects > > between 10GBASE-R, 5GBASE-R, 2500BASE-X and SGMII, where the SGMII > > mode may have inband signalling enabled or disabled. This is not a > > theoretical case; we have a PHY like that supported in the kernel and > > boards use it. What would the semantics of your new call be for a PHY > > that performs this? > > > > Should we also have a phydev->inband tristate, taking values "unknown, > > enabled, disabled" which the PHY driver is required to update in their > > read_status callback if they dynamically change their interface type? > > (Although then phylink will need to figure out how to deal with that.) > > I don't have such PHY to test with, but I think the easiest way would be > to just call the validation method again, after we change the > phydev->interface value. I'm not sure I follow. It is the PHY driver that is in charge of changing phydev->interface dynamically, since that is precisely what the hardware is doing. 88x3310 hardware, when connected using a single lane serdes without rate adaption will switch the MAC side interface between 10GBASE-R, 5GBASE-R, 2500BASE-X and SGMII depending on the media side speed (10G, 5G, 2.5G, 1G, 100M, 10M.) BCM84881 does the same, switching dynamically between 10GBASE-R, 2500BASE-X and SGMII (never with inband signalling - it's not supported in hardware) for 10G, 2.5G, 1G and 100M speeds. 10M is not supported. With both these PHYs, you don't get to say "I want you to operate in _this_ single interface mode", with the exception of 88x3310 with rate adaption, they aren't designed for that. As soon as there is link on the media side, the PHYs automatically reconfigure their MAC side with no intervention from MDIO. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index fdb914b5b857..d6c63c54943e 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -748,6 +748,18 @@ static int phy_check_link_status(struct phy_device *phydev) return 0; } +int phy_config_inband_aneg(struct phy_device *phydev, bool enabled) +{ + if (!phydev->drv) + return -EIO; + + if (!phydev->drv->config_inband_aneg) + return -EOPNOTSUPP; + + return phydev->drv->config_inband_aneg(phydev, enabled); +} +EXPORT_SYMBOL(phy_config_inband_aneg); + /** * phy_start_aneg - start auto-negotiation for this PHY device * @phydev: the phy_device struct diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index 84f6e197f965..ef3e947d5019 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -978,6 +978,14 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy, return ret; } + ret = phy_config_inband_aneg(phy, + (pl->cur_link_an_mode == MLO_AN_INBAND)); + if (ret && ret != -EOPNOTSUPP) { + phylink_warn(pl, "failed to configure PHY in-band autoneg: %d\n", + ret); + return ret; + } + phy->phylink = pl; phy->phy_link_change = phylink_phy_change; diff --git a/include/linux/phy.h b/include/linux/phy.h index c130788306c8..2260b512ffbf 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -746,6 +746,13 @@ struct phy_driver { */ int (*config_aneg)(struct phy_device *phydev); + /** + * @config_inband_aneg: Enable or disable in-band auto-negotiation for + * the system-side interface if the PHY operates in a mode that + * requires it: (Q)SGMII, USXGMII, 1000Base-X, etc. + */ + int (*config_inband_aneg)(struct phy_device *phydev, bool enabled); + /** @aneg_done: Determines the auto negotiation result */ int (*aneg_done)(struct phy_device *phydev); @@ -1394,6 +1401,7 @@ void phy_detach(struct phy_device *phydev); void phy_start(struct phy_device *phydev); void phy_stop(struct phy_device *phydev); int phy_start_aneg(struct phy_device *phydev); +int phy_config_inband_aneg(struct phy_device *phydev, bool enabled); int phy_aneg_done(struct phy_device *phydev); int phy_speed_down(struct phy_device *phydev, bool sync); int phy_speed_up(struct phy_device *phydev);