| Message ID | 20210212172341.3489046-2-olteanv@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Let phylink manage in-band AN for the PHY | 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 6 of 6 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: 616 this patch: 616 |
| 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, 52 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 532 this patch: 532 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
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 ;) > 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. -michael
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?
Am 2021-02-13 01:18, schrieb Russell King - ARM Linux admin: > 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.) Thanks, but I'm not sure I understand the difference between "rate adaption" and symbol repetition. The SGMII link is always 1.25Gb, right? If the media side is 100Mbit it will repeat the symbol 10 times or 100 times in case of 10Mbit. What is "rate adaption" then?
Am 2021-02-13 01:36, schrieb Vladimir Oltean: > 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 ] It isn't that easy sometimes. Eg. there might be boards without a proper reset of the peripherals, maybe the SoC will just generate an internal reset, whatever. One might conisder that a broken board. But, for example, on the kontron sl28, we deliberatly chose not to do a PHY reset (well it is actually configurable) because this will also prevent WoL by the PHY. >> > 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? enetc > 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? That seem to work: [ 5313.852406] fsl_enetc 0000:00:00.0 gbe0: phy link down sgmii/Unknown/Unknown [ 5313.852414] fsl_enetc 0000:00:00.0 gbe0: Link is Down [ 5315.900687] fsl_enetc 0000:00:00.0 gbe0: phy link up sgmii/100Mbps/Full [ 5315.916816] fsl_enetc 0000:00:00.0 gbe0: Link is Up - 100Mbps/Full - flow control rx/tx But the Atheros PHY seems to have a problem with the SGMII link if there is no autoneg. No matter what I do, I can't get any traffic though if its not gigabit on the copper side. Unfortunately, I don't have access to an oscilloscope right now to see whats going on on the SGMII link. -michael
> Thanks, but I'm not sure I understand the difference between "rate > adaption" and symbol repetition. The SGMII link is always 1.25Gb, > right? If the media side is 100Mbit it will repeat the symbol 10 > times or 100 times in case of 10Mbit. What is "rate adaption" then? Hi Michael Some multiG PHYs fix their host side interface to say 10Gbps, independent of what the media side is doing. The PHY adapts the 10Gbps stream it gets from the host down to 10Mbps etc as needed, dropping frames if its internal buffers overflow. Andrew
On Sat, Feb 13, 2021 at 05:41:55PM +0100, Michael Walle wrote: > Am 2021-02-13 01:18, schrieb Russell King - ARM Linux admin: > > 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.) > > Thanks, but I'm not sure I understand the difference between "rate > adaption" and symbol repetition. The SGMII link is always 1.25Gb, > right? If the media side is 100Mbit it will repeat the symbol 10 > times or 100 times in case of 10Mbit. What is "rate adaption" then? You are correct about SGMII. Bear in mind that there is very little difference between SGMII and 1000base-X when you don't have AN. SGMII can be forced to do the symbol repetition. Symbol repetition does not exist in 1000base-X. Some PHYs, particularly 10G multi-speed PHYs, have a "rate adaption" block that can be used when the media side is operating at slower- than-10G speed when the media side is fixed at 10G. What this means is the MAC side always operates at full 10G speed, but the PHY queues packets for transmission at the media rate. Received packets are received by the PHY and then forwarded to the MAC at 10G speed. To add to that, some PHYs have the ability to send pause frames when their internal buffer fills up. Others stipulate that the MAC needs to rate-limit the transmitted packets to match the media side speed. Whether such a feature exists in 1G PHYs is unknown. If a PHY is capable of 1G/100M/10M speeds, but if SGMII AN is disabled and there is no way to force the symbol repetition, the SGMII side will be fixed at 1G speed. The only way this can work with a slower media speed is if the PHY performs rate adaption internally, converting between the 1G speed on the host side and whatever speed the media side is operating. So, for example, if the PHY was to have rate adaption, the media side would operate at 100M but the MAC side would continue operating at 1G speed without symbol repetition.
Am 2021-02-13 17:53, schrieb Michael Walle: > Am 2021-02-13 01:36, schrieb Vladimir Oltean: > But the Atheros PHY seems to have a problem with the SGMII link > if there is no autoneg. > No matter what I do, I can't get any traffic though if its not > gigabit on the copper side. Unfortunately, I don't have access > to an oscilloscope right now to see whats going on on the SGMII > link. Scrap that. It will work if I set the speed/duplex mode in BMCR correctly. (I tried that before, but I shifted one bit. doh). So that will work, but when will it be done? There is no callback to configure the PCS side of the PHY if a link up is detected. -michael
On Sat, Feb 13, 2021 at 06:09:13PM +0100, Michael Walle wrote: > Am 2021-02-13 17:53, schrieb Michael Walle: > > Am 2021-02-13 01:36, schrieb Vladimir Oltean: > > But the Atheros PHY seems to have a problem with the SGMII link > > if there is no autoneg. > > No matter what I do, I can't get any traffic though if its not > > gigabit on the copper side. Unfortunately, I don't have access > > to an oscilloscope right now to see whats going on on the SGMII > > link. > > Scrap that. It will work if I set the speed/duplex mode in BMCR > correctly. (I tried that before, but I shifted one bit. doh). > > So that will work, but when will it be done? There is no > callback to configure the PCS side of the PHY if a link up is > detected. That's interesting/odd, on VSC8514 there is no need to force the speed of the system side to what was negotiated on media side. I took a quick look through the AR8033 datasheet and there isn't any mentioning the ability to program the SGMII link according to internal state as opposed to register settings, but it's equally possible that I'm simply not seeing it. On the other hand, I never meant for the inband autoneg setting to only be configurable both ways. I expect some PHYs are not able to operate using noinband mode, and for those I guess you should simply return -EINVAL, allowing the system designer to know that the configuration will not work and why. I think you could hook into .config_aneg_done, for the autoneg=true case, and into .config_aneg for autoneg=false (I'm talking about autoneg on media side here), but honestly I think the PHY is pretty broken for requiring external coordination between the clause 28 and the clause 37 PCS. So unless there is a real need to configure noinband mode, I would probably not bother.
Am 2021-02-13 19:56, schrieb Vladimir Oltean: > On Sat, Feb 13, 2021 at 06:09:13PM +0100, Michael Walle wrote: >> Am 2021-02-13 17:53, schrieb Michael Walle: >> > Am 2021-02-13 01:36, schrieb Vladimir Oltean: >> > But the Atheros PHY seems to have a problem with the SGMII link >> > if there is no autoneg. >> > No matter what I do, I can't get any traffic though if its not >> > gigabit on the copper side. Unfortunately, I don't have access >> > to an oscilloscope right now to see whats going on on the SGMII >> > link. >> >> Scrap that. It will work if I set the speed/duplex mode in BMCR >> correctly. (I tried that before, but I shifted one bit. doh). >> >> So that will work, but when will it be done? There is no >> callback to configure the PCS side of the PHY if a link up is >> detected. > > That's interesting/odd, on VSC8514 there is no need to force the speed > of the system side to what was negotiated on media side. I took a quick > look through the AR8033 datasheet and there isn't any mentioning the > ability to program the SGMII link according to internal state as > opposed > to register settings, but it's equally possible that I'm simply not > seeing it. I couldn't find anything in the AR8031 datasheet either. > On the other hand, I never meant for the inband autoneg setting to only > be configurable both ways. Then why is there a "bool enabled"? > I expect some PHYs are not able to operate > using noinband mode, and for those I guess you should simply return > -EINVAL, allowing the system designer to know that the configuration > will not work and why. You mean like this: static int at803x_config_inband_aneg(struct phy_device *phydev, bool enabled) { if (!enabled) return -EINVAL; /* enable SGMII autoneg */ return phy_write_paged(...); } But then why bother with config_inband_aneg() at all and just enable it unconditionally in config_init(). [and maybe keep the return -EINVAL]. Which then begs the question, does it makes sense on (Q)SGMII links at all? > I think you could hook into .config_aneg_done, for the autoneg=true > case, and into .config_aneg for autoneg=false (I'm talking about > autoneg > on media side here), but honestly I think the PHY is pretty broken for > requiring external coordination between the clause 28 and the clause 37 > PCS. So unless there is a real need to configure noinband mode, I would > probably not bother. Probably not. -michael
On Sat, Feb 13, 2021 at 08:57:46PM +0100, Michael Walle wrote: > > On the other hand, I never meant for the inband autoneg setting to only > > be configurable both ways. > > Then why is there a "bool enabled"? Let me stress the word _only_ both ways. The whole point of the "bool enabled" is to attempt coordination with the 'managed = "in-band-status"' property device tree property of the MAC, or to error out if it is not possible. > > I expect some PHYs are not able to operate > > using noinband mode, and for those I guess you should simply return > > -EINVAL, allowing the system designer to know that the configuration > > will not work and why. > > You mean like this: > > static int at803x_config_inband_aneg(struct phy_device *phydev, bool > enabled) > { > if (!enabled) > return -EINVAL; > /* enable SGMII autoneg */ > return phy_write_paged(...); > } > > But then why bother with config_inband_aneg() at all and just enable > it unconditionally in config_init(). [and maybe keep the return -EINVAL]. Because .config_init() is generic code, while .config_inband_autoneg() is phylink-specific. Generally I don't want to make any assumption about the state in which a PHY driver used to operate prior to this series. If you are sure that at803x.c user relies on a prior bootloader stage having disabled in-band AN, then sure, I suppose you can enable it unconditionally in .config_init(). For VSC8514 I put the configuration deliberately in a phylink-specific callback since I trust that at least the MAC-side drivers were reviewed for proper use of MLO_AN_PHY vs MLO_AN_INBAND. > Which then begs the question, does it makes sense on (Q)SGMII links at > all? See above. In the general case we need to assume a wild world where the same PHY driver operates as inband on some platforms and noinband on others (and most importantly, it works on both, which we'd like to preserve at least). I would be glad if we didn't need to make that assumption though.
On Sat, Feb 13, 2021 at 08:57:46PM +0100, Michael Walle wrote: > But then why bother with config_inband_aneg() at all and just enable > it unconditionally in config_init(). [and maybe keep the return -EINVAL]. > Which then begs the question, does it makes sense on (Q)SGMII links at > all? There are three cases. There are PHYs which operate in SGMII mode: 1) with in-band signalling enabled or disabled. 2) only with in-band signalling enabled. 3) with no in-band signalling capability. Most Marvell PHYs can be configured and fall into class (1), although changing that configuration is disruptive to the entire PHY. There is at least one Broadcom PHY we support that falls into class (3) and it appears on SFP modules. It sounds like AR803x fall into class (2).
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.)
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.
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);