| Message ID | ZIxQIBfO9dH5xFlg@shell.armlinux.org.uk (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add and use helper for PCS negotiation modes | expand |
On Fri, Jun 16, 2023 at 01:05:52PM +0100, Russell King (Oracle) wrote: > Hi, > > Earlier this month, I proposed a helper for deciding whether a PCS > should use inband negotiation modes or not. There was some discussion > around this topic, and I believe there was no disagreement about > providing the helper. > > The initial discussion can be found at: > > https://lore.kernel.org/r/ZGIkGmyL8yL1q1zp@shell.armlinux.org.uk > > Subsequently, I posted a RFC series back in May: > > https://lore.kernel.org/r/ZGzhvePzPjJ0v2En@shell.armlinux.org.uk > > that added a helper, phylink_pcs_neg_mode() which PCS drivers could use > to parse the state, and updated a bunch of drivers to use it. I got > a couple of bits of feedback to it, including some ACKs. > > However, I've decided to take this slightly further and change the > "mode" parameter to both the pcs_config() and pcs_link_up() methods > when a PCS driver opts in to this (by setting "neg_mode" in the > phylink_pcs structure.) If this is not set, we default to the old > behaviour. That said, this series converts all the PCS implementations > I can find currently in net-next. > > Doing this has the added benefit that the negotiation mode parameter > is also available to the pcs_link_up() function, which can now know > whether inband negotiation was in fact enabled or not at pcs_config() > time. > > It has been posted as RFC at: > > https://lore.kernel.org/r/ZIh/CLQ3z89g0Ua0@shell.armlinux.org.uk > > and received one reply, thanks Elad, which is a similar amount of > interest to previous postings. Let's post it as non-RFC and see > whether we get more reaction. Sorry, I was in the process of reviewing the RFC, but I'm not sure I know what to ask to make sure that I understand the motivation :-/ Here's a question that might or might not result in a code change. In the single-patch RFC at: https://lore.kernel.org/all/ZGIkGmyL8yL1q1zp@shell.armlinux.org.uk/ you bring sparx5 and lan966x as a motivation for introducing PHYLINK_PCS_NEG_OUTBAND as separate from PHYLINK_PCS_NEG_INBAND_DISABLED, with both meaning that in-band autoneg isn't used, but in the former case it's not enabled at all, while in the latter it's disabled through ethtool (if I get that right?). I've opened the Sparx5 documentation at: https://ww1.microchip.com/downloads/en/DeviceDoc/SparX-5_Family_L2L3_Enterprise_10G_Ethernet_Switches_Datasheet_00003822B.pdf and also cross-checked with the PCS1G documentation from VSC7514 (Ocelot: https://ww1.microchip.com/downloads/en/DeviceDoc/VMDS-10491.pdf, there's another embedded PDF with registers at page 283), trying to find exactly what the PCS1G_MODE_CFG.SGMII_MODE_ENA field does (which is controlled in sparx5 and lan966x based on the presence or absence of the managed = "in-band-status" property). Do you know for sure what this bit does and whether it makes sense for drivers to even distinguish between OUTBAND and INBAND_DISABLED in the way that this series is proposing? It's hard to know for sure, not having the hardware, but I believe that the bit selects between the SGMII and the 1000Base-X control word format (so, even though there's a dedicated and fully programmable PCS1G_ANEG_CFG.ADV_ABILITY register, the link partner ability is still decoded as per the programmed expected format). The documents talk about using the PCS in "SGMII mode" vs "1000BASE-X SERDES mode". If that's the case, then it is selecting between those 2 based on phylink_autoneg_inband(mode) and irrespective of the phy-mode, i.e.: - enabling the SGMII control word format for phy-mode = "1000base-x" and no managed = "in-band-status", or - enabling the 1000Base-X control word format for phy-mode = "sgmii" and managed = "in-band-status" ...is that a model to follow?
On Fri, Jun 16, 2023 at 06:00:55PM +0300, Vladimir Oltean wrote: > On Fri, Jun 16, 2023 at 01:05:52PM +0100, Russell King (Oracle) wrote: > > Hi, > > > > Earlier this month, I proposed a helper for deciding whether a PCS > > should use inband negotiation modes or not. There was some discussion > > around this topic, and I believe there was no disagreement about > > providing the helper. > > > > The initial discussion can be found at: > > > > https://lore.kernel.org/r/ZGIkGmyL8yL1q1zp@shell.armlinux.org.uk > > > > Subsequently, I posted a RFC series back in May: > > > > https://lore.kernel.org/r/ZGzhvePzPjJ0v2En@shell.armlinux.org.uk > > > > that added a helper, phylink_pcs_neg_mode() which PCS drivers could use > > to parse the state, and updated a bunch of drivers to use it. I got > > a couple of bits of feedback to it, including some ACKs. > > > > However, I've decided to take this slightly further and change the > > "mode" parameter to both the pcs_config() and pcs_link_up() methods > > when a PCS driver opts in to this (by setting "neg_mode" in the > > phylink_pcs structure.) If this is not set, we default to the old > > behaviour. That said, this series converts all the PCS implementations > > I can find currently in net-next. > > > > Doing this has the added benefit that the negotiation mode parameter > > is also available to the pcs_link_up() function, which can now know > > whether inband negotiation was in fact enabled or not at pcs_config() > > time. > > > > It has been posted as RFC at: > > > > https://lore.kernel.org/r/ZIh/CLQ3z89g0Ua0@shell.armlinux.org.uk > > > > and received one reply, thanks Elad, which is a similar amount of > > interest to previous postings. Let's post it as non-RFC and see > > whether we get more reaction. > > Sorry, I was in the process of reviewing the RFC, but I'm not sure I > know what to ask to make sure that I understand the motivation :-/ > Here's a question that might or might not result in a code change. > > In the single-patch RFC at: > https://lore.kernel.org/all/ZGIkGmyL8yL1q1zp@shell.armlinux.org.uk/ > you bring sparx5 and lan966x as a motivation for introducing > PHYLINK_PCS_NEG_OUTBAND as separate from PHYLINK_PCS_NEG_INBAND_DISABLED, > with both meaning that in-band autoneg isn't used, but in the former > case it's not enabled at all, while in the latter it's disabled through > ethtool (if I get that right?). Correct. conf.inband is set true if phylink_autoneg_inband(mode) is true, which equates to MLO_AN_INBAND. conf.autoneg is set true if the ethtool Autoneg flag in the advertising mask is set. That goes through some incomprehensible logic in sparx5_port_pcs_low_set() which I'm not going to even try to unpick because it looks buggy to me, except that conf.autoneg is only looked at if conf.inband were true at some point in the past. So, what I care about here is keeping the behaviour pretty much the same, especially as far as conf.inband. With the new neg_mode: conf.inband is set when we have one of the NEG_INBAND states. These are set when in 1000BASE-X, 2500BASE-X or one of the SGMII family and phylink_autoneg_inband(mode) is true. So, 100% identical when one considers that the driver only supports SGMII, QSGMII, 1000BASE-X and 2500BASE-X for this path. conf.autoneg will only be set when we have NEG_INBAND_ENABLED state, and that is only set when in SGMII mode (irrespective of Autoneg) or in *BASE-X, we're in in-band mode (so conf.inband is set) and the advertising mask has the Autoneg bit set. As this is only looked at if conf.inband was set the _last_ time around (which seems like a bug in the driver...) and we're in 1000BASE-X mode, this is identical logic where it matters. So, conf.inband is 100% identical logic, and conf.autoneg is very similar and for how it's actually used, completely identical. > ... trying to find > exactly what the PCS1G_MODE_CFG.SGMII_MODE_ENA field does (which is > controlled in sparx5 and lan966x based on the presence or absence of the > managed = "in-band-status" property). > > Do you know for sure what this bit does and whether it makes sense for > drivers to even distinguish between OUTBAND and INBAND_DISABLED in the > way that this series is proposing? I have no idea, and I didn't bother investigating - I don't want to go around trying to disect drivers to figure out whether they're buggy or not. However, what I would say is that this is not where these modes came from. They came from me asking myself the question "what would be the logical set of information to give a PCS driver about the negotiation state of the link?" and that's what I came up with _without_ reference to this driver. The states are all documented in the first patch and what they mean. So, no, the Microchip driver code is not the reason why these definitions were chosen. They were chosen because it's the logical set that gives PCS drivers what they need to know. Start from inband + autoneg. Then inband + !autoneg. Then inband possible but not being used. Then "there's no inband possible for this mode". That's the four states. I think having this level of detail is important if we want to think about those pesky inband-AN bypass modes, which make sense for only really the PHYLINK_PCS_NEG_INBAND_DISABLED state and not OUTBAND nor NONE state. Bypass mode doesn't make sense for e.g. SGMII because one needs to know the speed for the link to come up, and if you're getting that through an out-of-band mechanism, you're into forcing the configuration at the PCS end. Makes sense?
On Fri, Jun 16, 2023 at 04:46:39PM +0100, Russell King (Oracle) wrote: > On Fri, Jun 16, 2023 at 06:00:55PM +0300, Vladimir Oltean wrote: > > Do you know for sure what this bit does and whether it makes sense for > > drivers to even distinguish between OUTBAND and INBAND_DISABLED in the > > way that this series is proposing? > > I have no idea, and I didn't bother investigating - I don't want to go > around trying to disect drivers to figure out whether they're buggy or > not. > > However, what I would say is that this is not where these modes came > from. They came from me asking myself the question "what would be the > logical set of information to give a PCS driver about the negotiation > state of the link?" and that's what I came up with _without_ reference > to this driver. The states are all documented in the first patch and > what they mean. > > So, no, the Microchip driver code is not the reason why these > definitions were chosen. They were chosen because it's the logical > set that gives PCS drivers what they need to know. > > Start from inband + autoneg. Then inband + !autoneg. Then inband > possible but not being used. Then "there's no inband possible for this > mode". That's the four states. > > I think having this level of detail is important if we want to think > about those pesky inband-AN bypass modes, which make sense for only > really the PHYLINK_PCS_NEG_INBAND_DISABLED state and not OUTBAND nor > NONE state. Bypass mode doesn't make sense for e.g. SGMII because > one needs to know the speed for the link to come up, and if you're > getting that through an out-of-band mechanism, you're into forcing > the configuration at the PCS end. I should also add... yes, I did _then_ subsequently use the microchip driver as a justification for it. I probably should've explained it without using that as justification. I could also have used the sja1105 driver as well, since: MLO_AN_INBAND => PHYLINK_PCS_NEG_INBAND_ENABLED MLO_AN_FIXED || MLO_AN_PHY => PHYLINK_PCS_NEG_OUTBAND are the conversions done there, which fits with: - if (!phylink_autoneg_inband(mode)) { + if (neg_mode == PHYLINK_PCS_NEG_OUTBAND) { since the opposite of !inband is outband.
On Fri, Jun 16, 2023 at 04:46:39PM +0100, Russell King (Oracle) wrote: > So, no, the Microchip driver code is not the reason why these > definitions were chosen. They were chosen because it's the logical > set that gives PCS drivers what they need to know. > > Start from inband + autoneg. Then inband + !autoneg. Then inband > possible but not being used. Then "there's no inband possible for this > mode". That's the four states. > > I think having this level of detail is important if we want to think > about those pesky inband-AN bypass modes, which make sense for only > really the PHYLINK_PCS_NEG_INBAND_DISABLED state and not OUTBAND nor ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ don't you mean PHYLINK_PCS_NEG_INBAND_ENABLED? I fail to see why would the bypass make any difference for INBAND_DISABLED, where presumably the fiber BMCR of the attached PHY would have BMCR_ANENABLE unset. And in that case, I still don't understand the need for distinguishing between INBAND_DISABLED, OUTBAND, NONE. Sorry, slow-witted :) > NONE state. Bypass mode doesn't make sense for e.g. SGMII because > one needs to know the speed for the link to come up, and if you're > getting that through an out-of-band mechanism, you're into forcing > the configuration at the PCS end. > > Makes sense? I refreshed my memory with this thread https://patchwork.kernel.org/project/netdevbpf/patch/20221118000124.2754581-4-vladimir.oltean@nxp.com/ regarding in-band AN bypass on m88e1011, and the fact that enabling in-band AN bypass with SGMII forces an advertisement of only 1000baseT/Half and 1000baseT/Full on the media side. So.. correct, but I still don't get the overall answer to the question I have, which is "why would drivers want to make any legitimate distinction between INBAND_DISABLED and OUTBAND, when for all intents and purposes, those 2 modes are nothing but the same physical state, reached from 2 different phylink configuration path"?
On Fri, Jun 16, 2023 at 04:52:21PM +0100, Russell King (Oracle) wrote: > I should also add... yes, I did _then_ subsequently use the microchip > driver as a justification for it. I probably should've explained it > without using that as justification. > > I could also have used the sja1105 driver as well, since: > > MLO_AN_INBAND => PHYLINK_PCS_NEG_INBAND_ENABLED Technically this should have been: MLO_AN_INBAND => neg_mode & PHYLINK_PCS_NEG_INBAND, which includes both INBAND_DISABLED and INBAND_ENABLED, right? > MLO_AN_FIXED || MLO_AN_PHY => PHYLINK_PCS_NEG_OUTBAND > > are the conversions done there, which fits with: > > - if (!phylink_autoneg_inband(mode)) { > + if (neg_mode == PHYLINK_PCS_NEG_OUTBAND) { > > since the opposite of !inband is outband. The conversion is correct - no doubt about it. Maybe the SJA1105 and its use of the XPCS is also not the best example, because it doesn't support 1000BASE-X. So it doesn't have to handle the INBAND_DISABLED state. If it did, the !phylink_autoneg_inband(mode) check would have been incorrect (insufficient to detect the xpcs state that it's restoring).
Hello: This series was applied to netdev/net-next.git (main) by Jakub Kicinski <kuba@kernel.org>: On Fri, 16 Jun 2023 13:05:52 +0100 you wrote: > Hi, > > Earlier this month, I proposed a helper for deciding whether a PCS > should use inband negotiation modes or not. There was some discussion > around this topic, and I believe there was no disagreement about > providing the helper. > > [...] Here is the summary with links: - [net-next,01/15] net: phylink: add PCS negotiation mode https://git.kernel.org/netdev/net-next/c/f99d471afa03 - [net-next,02/15] net: phylink: convert phylink_mii_c22_pcs_config() to neg_mode https://git.kernel.org/netdev/net-next/c/cdb08aa04737 - [net-next,03/15] net: phylink: pass neg_mode into phylink_mii_c22_pcs_config() https://git.kernel.org/netdev/net-next/c/febf2aaf0564 - [net-next,04/15] net: pcs: xpcs: update PCS driver to use neg_mode https://git.kernel.org/netdev/net-next/c/a3a47cfb88fc - [net-next,05/15] net: pcs: lynxi: update PCS driver to use neg_mode https://git.kernel.org/netdev/net-next/c/3b2de56a146f - [net-next,06/15] net: pcs: lynx: update PCS driver to use neg_mode https://git.kernel.org/netdev/net-next/c/c689a6528c22 - [net-next,07/15] net: lan966x: update PCS driver to use neg_mode https://git.kernel.org/netdev/net-next/c/a0e93cfdac4c - [net-next,08/15] net: mvneta: update PCS driver to use neg_mode https://git.kernel.org/netdev/net-next/c/140d1002e2a3 - [net-next,09/15] net: mvpp2: update PCS driver to use neg_mode https://git.kernel.org/netdev/net-next/c/d5b16264fffe - [net-next,10/15] net: prestera: update PCS driver to use neg_mode https://git.kernel.org/netdev/net-next/c/d5a052993062 - [net-next,11/15] net: qca8k: update PCS driver to use neg_mode https://git.kernel.org/netdev/net-next/c/bfa0a3ac05b6 - [net-next,12/15] net: sparx5: update PCS driver to use neg_mode https://git.kernel.org/netdev/net-next/c/6e5bb3da9842 - [net-next,13/15] net: dsa: b53: update PCS driver to use neg_mode https://git.kernel.org/netdev/net-next/c/772c476dd1d4 - [net-next,14/15] net: dsa: mt7530: update PCS driver to use neg_mode https://git.kernel.org/netdev/net-next/c/6c1e4eca0b4e - [net-next,15/15] net: macb: update PCS driver to use neg_mode https://git.kernel.org/netdev/net-next/c/f40df95d375d You are awesome, thank you!