Messages in this thread | | | Date | Thu, 7 Dec 2023 17:07:24 -0600 | From | Andrew Halaney <> | Subject | Re: [PATCH net-next v3] net: stmmac: don't create a MDIO bus if unnecessary |
| |
On Fri, Dec 08, 2023 at 01:16:12AM +0300, Serge Semin wrote: > On Thu, Dec 07, 2023 at 03:12:40PM -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. > > > > Currently a MDIO bus is created if the description is either: > > > > 1. Not fixed-link > > 2. fixed-link but contains a MDIO bus as well > > > > The "1" case above isn't always accurate. If there's a phy-handle, > > it could be referencing a phy on another MDIO controller's bus[1]. In > > this case currently the MAC will make a MDIO bus and scan it all > > anyways unnecessarily. > > > > There's also a lot of upstream devicetrees[2] that expect a MDIO bus to > > be created and scanned for a phy. This case can also be inferred from > > the platform description by not having a phy-handle && not being > > fixed-link. This hits case "1" in the current driver's logic. > > > > Let's improve the logic to create a MDIO bus if either: > > > > > - Devicetree contains a MDIO bus > > - !fixed-link && !phy-handle (legacy handling) > > If what you suggest here is a free from regressions semantics change > (really hope it is) I will be with both my hands for it. This will > solve the problem we have with one of our device which doesn't have > SMA interface (hardware designers decided to save ~4K gates of the > chip area) but has a PHY externally attached to the DW XGMAC<->XPCS > interface. PHY is accessible via a GPIO-based MDIO bus. BTW having no > SMA interface available on a DW *MAC device but creating the MDIO-bus > on top of the non-existent SMA CSRs anyway causes having _32_ dummy > PHYs created with zero IDs.
I hope it is regression free! I have tested both the [1] and [2] cases (I hacked up the devicetree for [1] to make it look like [2]) without any issue.
Sorry, I don't have any docs for stmmac hardware so this might be answered in there (or just common net knowledge that I can't find online)... what's SMA stand for? I assume it's the MDIO interface.
I agree though, if you have a phy-handle and no mdio node in your devicetree this patch series should bail out without registering a bus in stmmac_mdio_register().
> > > > > Below upstream devicetree snippets can be found that explain some of > > the cases above more concretely.
<snip>
> > - if (mdio) { > > - plat->mdio_bus_data = > > - devm_kzalloc(dev, sizeof(struct stmmac_mdio_bus_data), > > - GFP_KERNEL); > > > + /* Legacy devicetrees allowed for no MDIO bus description and expect > > + * the bus to be scanned for devices. If there's no phy or fixed-link > > + * described assume this is the case since there must be something > > + * connected to the MAC. > > + */ > > + legacy_mdio = !of_phy_is_fixed_link(np) && !plat->phy_node; > > + if (legacy_mdio) > > + dev_info(dev, "Deprecated MDIO bus assumption used\n"); > > + > > + if (plat->mdio_node || legacy_mdio) { > > + plat->mdio_bus_data = devm_kzalloc(dev, > > Special thanks for adding the comment above this code. It will really > save time of figuring out why MDIO-bus needs to be created anyway. > > > + sizeof(struct stmmac_mdio_bus_data), > > Should v4 is required I would suggest to change this to > sizeof(*plat->mdio_bus_data). > > Anyway feel free to add: > Reviewed-by: Serge Semin <fancer.lancer@gmail.com> > > -Serge(y)
Sure I will spin v4 to pick that up, thanks for catching it. I'll also improve the motivation in the commit message a hair more per Andrew Lunn's request over here on v2 (and will hold off a little bit just to make sure reviews come in before a respin):
https://lore.kernel.org/netdev/e64b14c3-4b80-4120-8cc4-9baa40cdcb75@lunn.ch/
Thanks, Andrew
| |