| Message ID | ZvF0er+vyciwy3Nx@shell.armlinux.org.uk (mailing list archive) |
|---|---|
| Headers | show |
| Series | net: pcs: xpcs: cleanups batch 1 | expand |
Hi Russell, On Mon, 23 Sep 2024 15:00:26 +0100 "Russell King (Oracle)" <linux@armlinux.org.uk> wrote: > Hi, > > First, sorry for the bland series subject - this is the first in a > number of cleanup series to the XPCS driver. This series has some > functional changes beyond merely cleanups, notably the first patch. FWIW I've reviewed the whole series, to the best of my knowledge this looks fine, nice improvements. I don't have any means to test however. Thanks, Maxime
Hi Russell, On Mon, Sep 23, 2024 at 03:00:26PM +0100, Russell King (Oracle) wrote: > First, sorry for the bland series subject - this is the first in a > number of cleanup series to the XPCS driver. I presume you intend to remove the rest of the exported xpcs functions as well, in further "batches". Could you share in advance some details about what you plan to do with xpcs_get_an_mode() as used in stmmac? if (xpcs_get_an_mode(priv->hw->xpcs, mode) != DW_AN_C73)) I'm interested because I actually have some downstream NXP patches which introduce an entirely new MLO_AN_C73 negotiating mode in phylink (though they don't convert XPCS to it, sadly). Just wondering where this is going in your view.
On Wed, Sep 25, 2024 at 04:43:37PM +0300, Vladimir Oltean wrote: > Hi Russell, > > On Mon, Sep 23, 2024 at 03:00:26PM +0100, Russell King (Oracle) wrote: > > First, sorry for the bland series subject - this is the first in a > > number of cleanup series to the XPCS driver. > > I presume you intend to remove the rest of the exported xpcs functions > as well, in further "batches". Could you share in advance some details > about what you plan to do with xpcs_get_an_mode() as used in stmmac? I've been concentrating more on the sja1105 and wangxun users with this cleanup, as changing stmmac is going to be quite painful - so I've left this as something for the future. stmmac already stores a phylink_pcs pointer, but we can't re-use that for XPCS because stmmac needs to know that it's an XPCS vs some other PCS due to the direct calls such as xpcs_get_an_mode() and xpcs_config_eee(). When I was working on EEE support at phylink level, I did try to figure out what xpcs_config_eee() is all about, what it's trying to do, why, and how it would fit into any phylink-based EEE scheme, but I never got very far with that due to lack of documentation. So, at the moment I have no plans to touch the prototypes of xpcs_get_an_mode(), xpcs_config_eee() nor xpcs_get_interfaces(). With the entire patch series being so large already, I'm in no hurry to add patches for this - which would need yet more work on stmmac that I'm no longer willing to do. > if (xpcs_get_an_mode(priv->hw->xpcs, mode) != DW_AN_C73)) > > I'm interested because I actually have some downstream NXP patches which > introduce an entirely new MLO_AN_C73 negotiating mode in phylink (though > they don't convert XPCS to it, sadly). Just wondering where this is going > in your view. To give a flavour of what remains: net: pcs: xpcs: move Wangxun VR_XS_PCS_DIG_CTRL1 configuration net: pcs: xpcs: correctly place DW_VR_MII_DIG_CTRL1_2G5_EN net: pcs: xpcs: use dev_*() to print messages net: pcs: xpcs: convert to use read_poll_timeout() net: pcs: xpcs: add _modify() accessors net: pcs: xpcs: use FIELD_PREP() and FIELD_GET() net: pcs: xpcs: convert to use linkmode_adv_to_c73() net: pcs: xpcs: add xpcs_linkmode_supported() net: mdio: add linkmode_adv_to_c73() net: pcs: xpcs: move searching ID list out of line net: pcs: xpcs: rename xpcs_get_id() net: pcs: xpcs: move definition of struct dw_xpcs to private header net: pcs: xpcs: provide a helper to get the phylink pcs given xpcs net: pcs: xpcs: pass xpcs instead of xpcs->id to xpcs_find_compat() net: pcs: xpcs: don't use array for interface net: pcs: xpcs: remove dw_xpcs_compat enum which looks like this on the diffstat: drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c | 2 +- drivers/net/pcs/pcs-xpcs-nxp.c | 24 +- drivers/net/pcs/pcs-xpcs-wx.c | 51 +-- drivers/net/pcs/pcs-xpcs.c | 521 +++++++++------------- drivers/net/pcs/pcs-xpcs.h | 42 +- include/linux/mdio.h | 40 ++ include/linux/pcs/pcs-xpcs.h | 19 +- 7 files changed, 303 insertions(+), 396 deletions(-)
On Thu, Sep 26, 2024 at 12:41:27PM +0100, Russell King (Oracle) wrote: > On Wed, Sep 25, 2024 at 04:43:37PM +0300, Vladimir Oltean wrote: > > Hi Russell, > > > > On Mon, Sep 23, 2024 at 03:00:26PM +0100, Russell King (Oracle) wrote: > > > First, sorry for the bland series subject - this is the first in a > > > number of cleanup series to the XPCS driver. > > > > I presume you intend to remove the rest of the exported xpcs functions > > as well, in further "batches". Could you share in advance some details > > about what you plan to do with xpcs_get_an_mode() as used in stmmac? > > I've been concentrating more on the sja1105 and wangxun users with this > cleanup, as changing stmmac is going to be quite painful - so I've left > this as something for the future. stmmac already stores a phylink_pcs > pointer, but we can't re-use that for XPCS because stmmac needs to know > that it's an XPCS vs some other PCS due to the direct calls such as > xpcs_get_an_mode() and xpcs_config_eee(). > > When I was working on EEE support at phylink level, I did try to figure > out what xpcs_config_eee() is all about, what it's trying to do, why, > and how it would fit into any phylink-based EEE scheme, but I never got > very far with that due to lack of documentation. > > So, at the moment I have no plans to touch the prototypes of > xpcs_get_an_mode(), xpcs_config_eee() nor xpcs_get_interfaces(). With > the entire patch series being so large already, I'm in no hurry to add > patches for this - which would need yet more work on stmmac that I'm > no longer willing to do. Ok, but I guess that the (very) long-term plan still is that direct calls from the MAC driver into symbols exported by the PCS are no longer going to be a thing, right? > > if (xpcs_get_an_mode(priv->hw->xpcs, mode) != DW_AN_C73)) > > > > I'm interested because I actually have some downstream NXP patches which > > introduce an entirely new MLO_AN_C73 negotiating mode in phylink (though > > they don't convert XPCS to it, sadly). Just wondering where this is going > > in your view. > > To give a flavour of what remains: > > net: pcs: xpcs: move Wangxun VR_XS_PCS_DIG_CTRL1 configuration > net: pcs: xpcs: correctly place DW_VR_MII_DIG_CTRL1_2G5_EN > net: pcs: xpcs: use dev_*() to print messages > net: pcs: xpcs: convert to use read_poll_timeout() > net: pcs: xpcs: add _modify() accessors > net: pcs: xpcs: use FIELD_PREP() and FIELD_GET() > net: pcs: xpcs: convert to use linkmode_adv_to_c73() > net: pcs: xpcs: add xpcs_linkmode_supported() > net: mdio: add linkmode_adv_to_c73() > net: pcs: xpcs: move searching ID list out of line > net: pcs: xpcs: rename xpcs_get_id() > net: pcs: xpcs: move definition of struct dw_xpcs to private header > net: pcs: xpcs: provide a helper to get the phylink pcs given xpcs > net: pcs: xpcs: pass xpcs instead of xpcs->id to xpcs_find_compat() > net: pcs: xpcs: don't use array for interface > net: pcs: xpcs: remove dw_xpcs_compat enum > > which looks like this on the diffstat: > > drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c | 2 +- > drivers/net/pcs/pcs-xpcs-nxp.c | 24 +- > drivers/net/pcs/pcs-xpcs-wx.c | 51 +-- > drivers/net/pcs/pcs-xpcs.c | 521 +++++++++------------- > drivers/net/pcs/pcs-xpcs.h | 42 +- > include/linux/mdio.h | 40 ++ > include/linux/pcs/pcs-xpcs.h | 19 +- > 7 files changed, 303 insertions(+), 396 deletions(-) Ok, I don't see anything major on the clause 73 autoneg front. Which I guess is good? (because at least there aren't competing ideas in flight about phylink's role for this operating mode) The bad part is that some user-visible functional changes in xpcs will most likely be in order. So will probably not be able to be converted without someone with both access to the 10G hardware and the motivation to do so (and this might make the conversion unreachable for me too). For example, without having seen the content of your patch list, I can only assume linkmode_adv_to_c73() is based on the ethtool link modes that _xpcs_config_aneg_c73(), mii_c73_mod_linkmode() and phylink_c73_priority_resolution[] treat. But I'm already objecting that 2500baseX shouldn't be in those tables. There should have been a new 2500base-KX ethtool mode, which is one of the amendments to 802.3-2018 called 802.3cb-2018. I also have other objections to XPCS's implementation of C73, but I don't think this is the right context to point them all out. The gist is that at least for this area, I don't think it would be a good idea at all to base core phylink support based on what XPCS has/does. I guess what I'm saying is that taking a break from stmmac until the groundwork in the core has been laid out through some other vector also seems like the best idea to me. Would you be interested in seeing an alternative implementation of clause 73 support (for the Lynx PCS), this time centered around phylink_pcs, even if it doesn't touch stmmac/xpcs? As a side effect of that work, it would maybe provide a long-term avenue of avoiding the xpcs_get_interfaces() and xpcs_get_an_mode() direct calls, as well as a more consolidated framework for C73 in XPCS to be reimplemented by somebody. (warning, this implementation will be quite large, so the question is also about your time/energy availability in the near future).