Messages in this thread | | | Date | Thu, 7 Dec 2023 17:00:15 -0600 | From | Andrew Halaney <> | Subject | Re: [PATCH net-next v2] net: stmmac: don't create a MDIO bus if unnecessary |
| |
On Thu, Dec 07, 2023 at 10:30:23PM +0100, Andrew Lunn wrote: > On Wed, Dec 06, 2023 at 05:46:09PM -0600, Andrew Halaney wrote: > > The stmmac_dt_phy() function, which parses the devicetree node of the > > MAC and ultimately causes MDIO bus allocation, misinterprets what > > fixed-link means in relation to the MAC's MDIO bus. This results in > > a MDIO bus being created in situations it need not be. > > Please extend that with something like.... > > This is bad, because .... > > Most 'clean' driver unconditionally create the MDIO bus. But stmmac is > not that clean, and has to keep backwards compatibility to some old > usage. I'm just wondering what this patch actually brings us, and is > it worth it. Is it fixing a real bug, or just an optimisation? > > Andrew >
It is an optimization for speeding up time to link up. I already sent out v3 moments before this arrived, I'll be sure to capture that more clearly in v4 (and wait a little longer before respinning it).
I'm trying to optimize this device configuration (as shown in the commit): """ Here's[1] an example where there is no MDIO bus or fixed-link for the ethernet1 MAC, so no MDIO bus should be created since ethernet0 is the MDIO master for ethernet1's phy:
ðernet0 { phy-mode = "sgmii"; phy-handle = <&sgmii_phy0>;
mdio { compatible = "snps,dwmac-mdio"; sgmii_phy0: phy@8 { compatible = "ethernet-phy-id0141.0dd4"; reg = <0x8>; device_type = "ethernet-phy"; };
sgmii_phy1: phy@a { compatible = "ethernet-phy-id0141.0dd4"; reg = <0xa>; device_type = "ethernet-phy"; }; }; };
ðernet1 { phy-mode = "sgmii"; phy-handle = <&sgmii_phy1>; }; """
I'm seeing that ethernet1 scans the whole MDIO bus created for it, and finds nothing, wasting time in the process. Since there's no mdio bus described (since it's vacant) you get something like this call order:
stmmac_mdio_register() of_mdiobus_register(new_bus, mdio_node) // mdio_node is NULL in this case __of_mdiobus_register(mdio, np, owner) // this doesn't set phy_mask since np == NULL __mdiobus_register(mdio, owner) mdiobus_scan_bus_c22(bus) mdiobus_scan_c22() // Called PHY_MAX_ADDR times, probing an empty bus
Which is causing a good bit of delay in the time to link up, each attempted probe is taking about 5 ms and that's just benchmarking the get_phy_c22_id() calls, if you look at the whole loop it's greater (but I am unsure if that's just scheduling contention or something else going on, once I realized we were doing this work unnecessarily I decided to try and remove it)
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).
Please let me know if you disagree with that logic and have some alternative solutions for optimizing. In either case I think this code needs some cleaning so I'll carry this through. It also seems that unconditional creation of the MDIO bus is something that's biting some stmmac variants that lack an MDIO controller based on Serge's latest comments on v3:
https://lore.kernel.org/netdev/jz6ot44fjkbmwcezi3fkgqd54nurglblbemrchfgxgq6udlhqz@ntepnnzzelta/
Thanks, Andrew
| |