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

[Andrew Halaney]

On Fri, Dec 08, 2023 at 06:07:06PM +0300, Serge Semin wrote:
> On Thu, Dec 07, 2023 at 05:07:24PM -0600, Andrew Halaney wrote:
> > 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.
> > 
> 
> I doubt you could have tested it on all the possible hardware the
> STMMAC driver supports. The problem is that the DT-bindings thing is a
> kind of contract which can't be changed that easily. It's like ABI but
> for the hardware description so the kernel would bootup correctly on
> the platforms with the old DT blobs. But if the change isn't that
> critical, if the device-tree sources in the kernel fit to the updated
> semantics, if the networking subsystem maintainers aren't against it
> and I guess with the Rob, Krzysztof or Conor blessing (at least it
> won't hurt to add them to the Cc-list together with the devicetree
> mailing-list), then it will likely be accepted.

To be clear, I don't think we're violating the dt-binding ABI contract
here. My intention is that all the existing use cases continue to work,
and this just improves one use case. I did a write up
over on v2 about the use cases I see and the current logic vs what
changes with this patch series:

    https://lore.kernel.org/netdev/plvbqgi2bwlv5quvpiwplq7cxx6m5rl3ghnfhuxfx4bpuhyihl@zmydwrtwdeg6/

Please comment if you think I have broken some backwards
compatibility. If I _could_ break compatibility, I'd make everyone
describe their busses entirely... but as you said that's against the
spirit of dt-bindings and would upset a lot of people. That's why I've
left the "!fixed-link && !phy-handle (legacy handling) logic :)

> 
> > 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.
> 
> Right. Synopsys names the MDIO-bus interface as Station Management
> Agent MDIO module.
> 
> > 
> > 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().
> 
> On the other hand why would the MDIO-bus needed in such case? If the
> phy-handle property is specified with no MDIO-bus DT-subnode, then it
> will point out to a PHY residing an external bus. The only case I can
> imagine though is that the DW XPCS device could be still auto-detected
> on the internal SMA-MDIO-bus. But the only driver which currently has
> XPCS auto-detection activated is the Intel glue layer (see
> dwmac-intel.c and has_xpcs flag), but it doesn't use DT interface
> since it handles a PCIe-based device.  So this case is out of
> brackets.

Agreed, I think making the bus is not needed in the driver as is in that
case.

I'd like to think (but am not sure) that when a devicetree based DW XPCS
description comes around it will allow you to describe it exactly
instead of doing auto-detection (i.e. something like phy-handle), but I
am not very familiar with PCS and friends in the stack so that may be a
crude extension from my knowledge of MDIO.

Thanks,
Andrew

> 
> > 
> > > 
> > > > 
> > > > 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@xxxxxxxxx>
> > > 
> > > -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@xxxxxxx/
> 
> Ok. Thanks.
> 
> -Serge(y)
> 
> > 
> > Thanks,
> > Andrew
> > 
>