Re: [PATCH net-next v2] net: stmmac: don't create a MDIO bus if unnecessary

[Andrew Halaney <ahalaney@redhat.com>]

On Fri, Dec 08, 2023 at 02:14:41PM +0100, Andrew Lunn wrote:
> > I know you said the standard is to make the MDIO bus unconditionally, but
> > to me that feels like a waste, and describing say an empty MDIO bus
> > (which would set the phy_mask for us eventually and not scan a
> > bunch of phys, untested but I think that would work) seems like a bad
> > description in the devicetree (I could definitely see describing an
> > empty MDIO bus getting NACKed, but that's a guess).
> 
> DT describes the hardware. The MDIO bus master exists. So typically
> the SoC .dtsi file would include it in the Ethernet node. At the SoC
> level it is empty, unless there is an integrated PHY in the SoC. The
> board .dts file then adds any PHYs to the node which the board
> actually has.
> 
> So i doubt adding an empty MDIO node to the SoC .dtsi file will get
> NACKed, it correctly describes the hardware.

Agreed, thanks for helping me consider all the cases. In my particular
example it would make sense to have SoC dtsi describe the mdio bus,
leave it disabled, and boards enable it and describe components as
necessary.

So you have let's say these 8 abbreviated cases:

Case 1 (MDIO bus used with phy on bus connected to MAC):

	ethernet {
		status = "okay";
		phy-handle = <&phy0>;
		phy-mode = "rgmii";

		mdio {
			status = "okay";

			phy0: phy@0 {
			};
		};
	};

Case 2 (MDIO bus used but MAC's connect fixed-link):

	ethernet {
		status = "okay";
		phy-mode = "rgmii";

		fixed-link {
		};

		mdio {
			status = "okay";

			switch/unrelated-phy {
			};
		};
	};

Case 3 (MDIO bus used but MAC's connected to another bus's phy):

	ethernet {
		status = "okay";
		phy-handle = <&phy1>;
		phy-mode = "rgmii";

		mdio {
			status = "okay";

			switch/unrelated-phy {
			};
		};
	};

Case 4 (MDIO bus disabled/unused, connected fixed-link):

	ethernet {
		status = "okay";
		phy-mode = "rgmii";

		fixed-link {
		};

		mdio {
			status = "disabled";
		};
	};

Case 5 (MDIO bus disabled/unused, connected to another bus's phy):

	ethernet {
		status = "okay";
		phy-handle = <&phy1>;
		phy-mode = "rgmii";

		mdio {
			status = "disabled";
		};
	};

Case 6 (MDIO bus not described, connected fixed-link):

	ethernet {
		status = "okay";
		phy-mode = "rgmii";

		fixed-link {
		};
	};

Case 7 (MDIO bus not described, connected to a different bus's phy):

	ethernet {
		status = "okay";
		phy-handle = <&phy1>;
		phy-mode = "rgmii";
	};

Case 8 (MDIO bus not described, but phy on MDIO bus is connected to MAC,
        legacy description[2] in my commit message):

	ethernet {
		status = "okay";
	};


If we look at the logic in stmmac today about how to handle the MDIO
bus, you've got basically:

	if !fixed-link || mdio_node_present()
		of_mdiobus_register(np)

Applying current stmmac logic to our cases...

Case 1 (MDIO bus used with phy on bus connected to MAC):
    MDIO bus made, no unnecessary scanning

Case 2 (MDIO bus used but MAC's fixed-link):
    MDIO bus made, no unnecessary scanning

Case 3 (MDIO bus used but MAC's connected to another bus's phy):
    MDIO bus made, no unnecessary scanning

Case 4 (MDIO bus disabled/unused, connected fixed-link):
    MDIO bus attempted to be made, fails -ENODEV due to disabled
    and stmmac returns -ENODEV from probe too

Case 5 (MDIO bus disabled/unused, connected to another bus's phy):
    MDIO bus attempted to be made, fails -ENODEV due to disabled
    and stmmac returns -ENODEV from probe too

Case 6 (MDIO bus not described, connected fixed-link):
    MDIO bus not created

Case 7 (MDIO bus not described, connected to a different bus's phy):
    MDIO bus created, but the whole bus is scanned

Case 8 (MDIO bus not described, but phy on MDIO bus is connected to MAC,
        legacy description[2] in my commit message):
    MDIO bus created, the whole bus is scanned and the undescribed but
    necessary phy is found


The things I note of interest are cases 4, 5, 7, 8. Cases 4/5 are a bug in
stmmac IMO, which breaks the devicetree description you mentioned as
ideal in my case. Case 7 is the one I'm currently working with, and the
devicetree can be updated to match case 5, but right now case 7 makes a
bus and scans it needlessly which isn't great. It _sounds_ like to me
Serge knows of stmmac variants that also *do not* have an MDIO controller,
so they'd fall in this case too and really shouldn't create a bus. Case 8
is the legacy one that I wish didn't exist, but it does, and for that
reason we should continue to make a bus and scan the whole thing if we can't
figure out what the MAC's connected to.

So in my opinion there's 3 changes I want to make based on all the
use cases I could think of:

    1. This patch, which improves the stmmac logic and stops making
       a bus for case 7
    2. A new patch to gracefully handle cases 4/5, where currently if the
       MDIO bus is disabled in the devicetree, as it should be,
       stmmac handles -ENODEV from of_mdiobus_register() as a failure
       instead of gracefully continuing knowing this is fine and dandy.
    3. Clean up the sa8775p dts to have the MDIO bus described in the
       SoC dtsi and left disabled instead of undescribed (a more
       accurate hardware description).

Please let me know if you see any holes in my logic, hopefully the
wall of text is useful in explaining how I got here.

Thanks,
Andrew