Message-ID: <slq3uvpe424lxksgc2ho4q5apon6yqyvmq3ubpmx3z5ln5yhqf@klnkglqb6o6r>
Date:   Mon, 11 Dec 2023 13:48:47 -0600
From:   Andrew Halaney <ahalaney@...hat.com>
To:     Serge Semin <fancer.lancer@...il.com>
Cc:     Alexandre Torgue <alexandre.torgue@...s.st.com>,
        Jose Abreu <joabreu@...opsys.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        netdev@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [PATCH net-next v3] net: stmmac: don't create a MDIO bus if
 unnecessary

On Mon, Dec 11, 2023 at 08:27:46PM +0300, Serge Semin wrote:
> On Fri, Dec 08, 2023 at 10:50:29AM -0600, Andrew Halaney wrote:
> > 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.
> 
> To shortly sum up so I didn't miss something. Current semantics of the
> MDIO-bus registration is:
> if !fixed-link || mdio_node_present
>     create MDIO-bus
> and the semantics of the PHY auto-probe (legacy) is:
> if !(fixed-link || mdio_node_present || phy_node_present)
>     auto-probe PHY

I think phy_node_present doesn't belong in the current view of the
semantics for PHY auto-probe (legacy). This devicetree would trigger a
PHY auto-probe/scan on ethernet0's MAC's MDIO bus:

	random-mdio-bus {
		rgmii_phy: phy@0 {
		};
	};

	ethernet0 {
		phy-handle = <&rgmii_phy>;
	};

The assumption I make in this patch is that nothing useful could be on
ethernet0's MDIO bus, it certainly at least is not the phy the MAC uses.

> 
> You are changing the MDIO-bus creation semantics to:
> if !(fixed-link || phy_node_present) || mdio_node_present
>     create MDIO-bus
> with no change in the PHY auto-probe semantics:
> if !(fixed-link || mdio_node_present || phy_node_present)
>     auto-probe PHY

Unfortunately as I highlighted above this logic (while accurate to the
patch under review) is a change from the prior logic for the "auto-probe
PHY" case.

> 
> So the change is that if a PHY-handle is specified the MDIO-bus won't
> be created with the rest conditions being the same.
> 
> The only concern I had was the so called legacy case and a possibility
> to have MDIO-bus with other than PHY devices. Based on the pseudo-code
> above the former case won't be affected since having PHY-node
> specified didn't triggered MDIO-bus auto-probe even before your
> change. The later case concerns for instance the DW XPCS devices which

As I've realized in your response here, there is the possibility that
something is on the MDIO bus in the ethernet0 exmpale bus above, and would
probe, in the before handling. So I guess this isn't totally backwards
compatible. Gah, thanks for highlighting.

I'm not sure in practice if anyone out there is really relying on that
or not. I can get away with the "no auto-probe/scan of bus" optimization I
really desire by describing my MDIO bus as disabled in the devicetree
(need to send patches to do that in the dts and handle it gracefully in
stmmac). I'm wondering if I should keep forth with this patch as is, or
if I should keep the same logic but clean it up a bit as is done in the
current patch... I guess probably the latter.

> on some platforms could be found on the DW MAC MDIO bus with not
> having PHY living on that bus. But DW XPCS auto-probing currently is
> only supported by the non-OF platforms (it's Intel). Thus your change
> is supposed to be safe here too.
> 
> So to speak AFAICS from the STMMAC MDIO OF stuff your solution isn't
> supposed to cause regressions and break the current DTs backward
> compatibility indeed.
> 
> Regarding the ideal implementation. What could be much better is to
> implement the next semantics:
> if SMA-capability-detected &&
>    (!mdio_node_present || (mdio_node_present && mdio_node_enabled))
>     create MDIO-bus
> and preserve the PHY auto-probe semantics for backwards compatibility.
> Regarding the SMA-capability flag, it has been presented since DW GMAC
> v3.50, so since very much early DW MAC releases. But even for the
> early devices I think it could be auto-detected by checking the SMA
> CSRs writability. At least DW XGMAC AFAICS has the command CSR not
> writable if SMA is unavailable.
> 
> But I guess it's a matter of another patch.
> 

I like that logic for what it is worth, although would be unsure of
verifying the SMA-capability-detected part of it. But I wouldn't mind
seeing that patch :)

Powered by blists - more mailing lists