Date: Wed, 13 Dec 2023 14:06:56 +0200
From: Vladimir Oltean <vladimir.oltean@....com>
To: Luiz Angelo Daros de Luca <luizluca@...il.com>
Cc: Alvin Šipraga <ALSI@...g-olufsen.dk>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Andrew Lunn <andrew@...n.ch>,
	Florian Fainelli <f.fainelli@...il.com>,
	Madhuri Sripada <madhuri.sripada@...rochip.com>,
	Marcin Wojtas <mw@...ihalf.com>,
	Linus Walleij <linus.walleij@...aro.org>,
	Tobias Waldekranz <tobias@...dekranz.com>,
	Arun Ramadoss <arun.ramadoss@...rochip.com>,
	"Russell King (Oracle)" <rmk+kernel@...linux.org.uk>,
	Jonathan Corbet <corbet@....net>
Subject: Re: [PATCH net 3/4] docs: net: dsa: update user MDIO bus
 documentation

On Wed, Dec 13, 2023 at 02:30:48AM -0300, Luiz Angelo Daros de Luca wrote:
> Hello Vladimir,
> 
> Sorry for my lack of understanding but I still didn't get it.
> 
> You are telling us we should not use user_mii_bus when the user MDIO
> is described in the OF. Is it only about the "generic DSA mii" or also
> about the custom ones the current drivers have? If it is the latter, I
> don't know how to connect the dots between phy_read/write functions
> and the port.

It's about both. It has to do with the role that ds->user_mii_bus has
(see more below), not about who allocates it.

When the user_mii_bus is allocated by the driver, ds->ops->phy_read()
and ds->ops->phy_write() are not needed. They are only needed for DSA
to implement the ops of the generic user_mii_bus - see dsa_user_mii_bus_init().

> We have some drivers that define ds->user_mii_bus (not the "generic
> DSA mii") with the MDIO described in OF. Are they wrong?

This right here is the core ds->user_mii_bus functionality:

	ret = phylink_of_phy_connect(dp->pl, port_dn, phy_flags);
	if (ret == -ENODEV && ds->user_mii_bus) {
		/* We could not connect to a designated PHY or SFP, so try to
		 * use the switch internal MDIO bus instead
		 */
		ret = dsa_user_phy_connect(user_dev, dp->index, phy_flags);
	}

So, the ds->user_mii_bus is only used if we cant't do phylink_of_phy_connect(),
which follows the "phy-handle" reference.

The reasons (that I can see) why we can't do phylink_of_phy_connect() are:
(a) port_dn is NULL (probing on platform_data and not OF)
(b) port_dn exists, but has no phy-handle
(c) port_dn exists and has a phy-handle to a PHY that doesn't respond
    (wrong address, ?!)

Out of those, it only makes practical sense to design for (a) and (b).

If the ds->user_mii_bus has an OF node, it means that we are not in case
(a). We are in case (b).

Normally to be in case (b), it means that the device tree looks like this:

	switch {
		ports {
			port@0 {
				reg = <0>;
			};
		};
	};

aka port@0 is a user port with an internal PHY, not described in OF.

But to combine case (b) with the additional constraint that ds->user_mii_bus
has an OF node means to have this device tree:

	switch {
		ports {
			port@0 {
				reg = <0>;
				// no phy-handle to <&phy0>
			};
		};

		mdio {
			phy0: ethernet-phy@0 {
			};
		};
	};

Which is simply a broken device tree which should not be like that [1].
If the MDIO bus appears in OF, then _all_ its PHYs must appear in OF [2].
And if all PHYs appear in OF, then you must have a phy-handle to
reference them.

[1] There exists an exception, which is the hack added by Florian in
    commit 771089c2a485 ("net: dsa: bcm_sf2: Ensure that MDIO diversion
    is used"). There, he starts with a valid phy-handle in the device
    tree, but the DSA driver removes it. This makes phylink_of_phy_connect()
    fail, and "diverts" the code to dsa_user_phy_connect(), where the
    mii_bus read and write ops are in control of the DSA driver. Hence
    the name "diversion to ds->user_mii_bus". It's a huge hack, make no
    mistake about it.

[2] This is because __of_mdiobus_register() does this:

	/* Mask out all PHYs from auto probing.  Instead the PHYs listed in
	 * the device tree are populated after the bus has been registered */
	mdio->phy_mask = ~0;

> Alvin asked if we should store the mii_bus internally and not in the
> ds->user_mii_bus but I don't think you answered it. Is it about where
> we store the bus (for dropping user_mii_bus in the future)?

The ds->user_mii_bus is not just a dropbox, a pointer for random storage
that the DSA core generously offers... It would have had a void * type
if it was that, and DSA wouldn't have used it.

When a driver populates ds->user_mii_bus, it opts into its core functionality,
which I just described above. If you don't need it, don't use it. It's
as simple as that. Use your own private pointer to a struct mii_bus,
which stays out of dsa_user_phy_connect()'s business.

It's very unlikely that ds->user_mii_bus will be dropped though, unless
we resolve all other situations that need dsa_user_phy_connect() in some
other way. One of those situations is case (b) described above, aka
device trees with no phy-handle, which we don't want to break (for the
old drivers where that used to work).

I cannot make a blanket comment on whether all drivers that populate
ds->user_mii_bus with an OF-aware MDIO bus "are wrong". Probably out
of sheer ignorance, they connected and tangled together 2 logically
isolated code paths by using ds->user_mii_bus as dumb storage.

What was written carelessly now takes an expert to untangle. You now
have as much information as I do, so you can judge for yourself whether
the behavior given by dsa_user_phy_connect() is needed. My only ask is
to stop proliferating this monkey-see-monkey-do. If, on top of that,
we could eliminate the gratuitous uses of ds->user_mii_bus as plain
storage, that would be just great.

> 
> You now also mention using the MFD model (shouldn't it be cited in the
> docs too?) but I couldn't identify a DSA driver example that uses that
> model, with mdio outside the switch. Do we have one already? Would the
> OF compatible with the MDF model be something like this?

I did mention it already in the docs.

"But an internal microprocessor may have a very different view of the switch
address space (which is MMIO), and may have discrete Linux drivers for each
peripheral. In 2023, the ``ocelot_ext`` driver was added, which deviated from
the traditional DSA driver architecture. Rather than probing on the entire
``spi_device``, it created a multi-function device (MFD) top-level driver for
it (associated with the SoC at large), and the switching IP is only one of the
children of the MFD (it is a platform device with regmaps provided by its
parent). The drivers for each peripheral in this switch SoC are the same when
controlled over SPI and when controlled by the internal processor."

> 
> my_mfd {
>     compatible "aaa";
>     switch {
>         compatible = "bbb";
>         ports {
>             port@0: {
>               phy-handle = <&ethphy0>;
>             }
>         }
>     }
>     mdio {
>          compatible = "ccc";
>          ethphy0: ethernet-phy@0 {
>          }
>     }
> }
> 
> And for MDIO-connected switches, something like this?
> 
> eth0 {
>      mdio {
>          my_mfd {
>             switch{...}
>             mdio{...}
>          }
>      }
> }

Follow the clue given by the "ocelot_ext" reference. Search for the "mscc,vsc7512-switch"
compatible string, which leads you to the Documentation/devicetree/bindings/mfd/mscc,ocelot.yaml
which is the schema for the entire SPI-connected SoC.

> 
> Is it only a suggestion on how to write a new driver or should we
> change the existing ones to fit both models?

First and foremost it is for new drivers. Read the actual patch:

"Authors of new switch drivers that use DSA are encouraged to have a wider view
of the peripherals on the chip that they are controlling, and to use the MFD
model to separate the components whenever possible. The general direction for
the DSA core is to focus only on the Ethernet switching IP and its ports.
``CONFIG_NET_DSA_HWMON`` was removed in 2017. Adding new methods to ``struct
dsa_switch_ops`` which are outside of DSA's core focus on Ethernet is strongly
discouraged."

For changing existing drivers, on one hand I would be glad to see effort
put there, but on the other, I need to be realistic and say that I also
tried to convert the sja1105 driver to the MFD model, and it's really,
really hard to be compatible with both device tree formats. So I'm not
holding my breath that I'll ever see conversion patches.

Powered by blists - more mailing lists