Message-ID: <20231219162803.kmb3jb6aevef3kj3@skbuf>
Date: Tue, 19 Dec 2023 18:28:03 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Serge Semin <fancer.lancer@...il.com>
Cc: Andrew Lunn <andrew@...n.ch>, Russell King <linux@...linux.org.uk>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Alexandre Torgue <alexandre.torgue@...s.st.com>,
	Jose Abreu <joabreu@...opsys.com>,
	Jose Abreu <Jose.Abreu@...opsys.com>,
	Maxime Chevallier <maxime.chevallier@...tlin.com>,
	Tomer Maimon <tmaimon77@...il.com>,
	Rob Herring <robh+dt@...nel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
	Conor Dooley <conor+dt@...nel.org>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	openbmc@...ts.ozlabs.org, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 06/16] net: pcs: xpcs: Avoid creating dummy XPCS
 MDIO device

On Tue, Dec 19, 2023 at 06:48:09PM +0300, Serge Semin wrote:
> > > Sorry, because the commit log lost me at the "context presentation" stage,
> > > I failed to understand the "what"s and the "why"s.
> > > 
> > > Are you basically trying to add xpcs support on top of an mdio_device
> > > where the mdio_device_create() call was made externally to the xpcs code,
> > > through mdiobus_register_board_info() and mdiobus_setup_mdiodev_from_board_info()?
> > 
> > Basically yes, but there is more of it. The main idea is to convert
> > the XPCS driver to using the already created non-PHY MDIO-devices
> > instead of manually creating a 'dummy'/'redundant' one. From my point
> > of view there are several reasons of doing so:
> > 
> > 1. mdiobus_register_board_info() provides a way to assign the device
> > platform data to being registered afterwards device. Thus we can pass
> > some custom data to the XPCS-device driver (whether it's just an
> > xpcs_create_*() call or a fully functional MDIO-device driver
> > registered by the mdio_driver_register() method). For instance it can
> > be utilized to drop the fake PHYSIDs implementation from
> > drivers/net/dsa/sja1105/sja1105_mdio.c .

Ok. Seeing an alternative to the NXP_SJA1110_XPCS_ID hack will be interesting.

FWIW, I'm looking at reworking the dsa_loop probing to use software nodes.
Since dsa_loop is the only current user of mdiobus_register_board_info(),
maybe that will lead to its deletion. It appears a matter of timing, but
the mechanism looks promising.

Maybe we can also use it somehow to add compatibility with existing
lynx-pcs device trees where there is no compatible string, so a struct
phy_device gets created. Device tree breakage was the fundamental reason
why Sean Anderson's patch set couldn't make forward progress.
https://patchwork.kernel.org/project/netdevbpf/cover/20221103210650.2325784-1-sean.anderson@seco.com/

> > 2. The MDIO-devices actually registered on the MDIO-bus will be
> > visible in sysfs with for instance useful IO statistics provided by
> > the MDIO-bus. Potentially (if it is required) at some point we'll be
> > able to convert the DW XPCS driver to being true MDIO-device driver
> > (bindable to the DW XPCS device) with less efforts.

Ok.

> > 3. Having an MDIO-device registered that way would make the DW XPCS
> > IO-device implementation unified after the fwnode-based XPCS
> > descriptor creation support is added in one of the subsequent patches.

Unified how? You mean that "XPCS will always operate as a driver bound
to an mdio_device"?

You're not planning to unify the mdio_device and MMIO register handling
by using regmap, right?

> > So based on the listed above I've got a question. Do you think all of
> > that is worth to be implemented? Andrew, Russell?
> > 
> > I am asking because the patchset advance depends on your answers. If
> > you do I'll need to fix the problem described in my first message,
> > implement some new mdiobus_register_board_info()-like but
> > MDIO-bus-specific interface function (so MDIO-device infos would be
> > attached to the allocated MDIO-bus and then used to register the
> > respective MDIO-devices on the MDIO-bus registration), then convert
> > the sja1105 and wangxun txgbe drivers to using it. If you don't I'll
> > get back the xpcs_create_mdiodev() implementation and just provide a
> > fwnode-based version of one.
> 
> Folks, this is the only issue left to settle so I could move on with
> the series fixing up. So the question is: taking my comment above into
> account is it worth to convert the xpcs_create_mdiodev() method to
> re-using the already registered MDIO-device instance instead of
> always creating a stub-like MDIO-device?

I can't exactly say "yes, this is worth it", because it also depends on
what the phylib/phylink maintainers say. So I haven't said anything.
But I also don't have any objection, as long as the conversion doesn't
break existing setups (in new ways; see the "unbind MDIO bus driver"
case which is already problematic).

Powered by blists - more mailing lists