Message-ID: <20240926134952.atabwkzfal44r3lk@skbuf>
Date: Thu, 26 Sep 2024 16:49:52 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
	Alexandre Torgue <alexandre.torgue@...s.st.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Florian Fainelli <f.fainelli@...il.com>,
	Jakub Kicinski <kuba@...nel.org>,
	Jiawen Wu <jiawenwu@...stnetic.com>,
	Jose Abreu <joabreu@...opsys.com>,
	Jose Abreu <Jose.Abreu@...opsys.com>,
	linux-arm-kernel@...ts.infradead.org,
	linux-stm32@...md-mailman.stormreply.com,
	Maxime Coquelin <mcoquelin.stm32@...il.com>,
	Mengyuan Lou <mengyuanlou@...-swift.com>, netdev@...r.kernel.org,
	Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH RFC 00/10] net: pcs: xpcs: cleanups batch 1

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).

Powered by blists - more mailing lists