[EXT] Re: [PATCH] net: stmmac: dwmac-imx: pause the TXC clock in fixed-link

[Russell King (Oracle)]

On Wed, Jul 26, 2023 at 03:59:38PM +0000, Shenwei Wang wrote:
> > -----Original Message-----
> > From: Russell King <linux at armlinux.org.uk>
> > Sent: Wednesday, July 26, 2023 10:29 AM
> > To: Shenwei Wang <shenwei.wang at nxp.com>
> > Cc: Vladimir Oltean <olteanv at gmail.com>; David S. Miller
> > <davem at davemloft.net>; Eric Dumazet <edumazet at google.com>; Jakub
> > Kicinski <kuba at kernel.org>; Paolo Abeni <pabeni at redhat.com>; Maxime
> > Coquelin <mcoquelin.stm32 at gmail.com>; Shawn Guo <shawnguo at kernel.org>;
> > dl-linux-imx <linux-imx at nxp.com>; Giuseppe Cavallaro
> > <peppe.cavallaro at st.com>; Alexandre Torgue <alexandre.torgue at foss.st.com>;
> > Jose Abreu <joabreu at synopsys.com>; Sascha Hauer <s.hauer at pengutronix.de>;
> > Pengutronix Kernel Team <kernel at pengutronix.de>; Fabio Estevam
> > <festevam at gmail.com>; netdev at vger.kernel.org; linux-stm32 at st-md-
> > mailman.stormreply.com; linux-arm-kernel at lists.infradead.org;
> > imx at lists.linux.dev; Frank Li <frank.li at nxp.com>
> > Subject: Re: [EXT] Re: [PATCH] net: stmmac: dwmac-imx: pause the TXC clock in
> > fixed-link
> >
> > Caution: This is an external email. Please take care when clicking links or
> > opening attachments. When in doubt, report the message using the 'Report this
> > email' button
> >
> >
> > On Wed, Jul 26, 2023 at 03:10:19PM +0000, Shenwei Wang wrote:
> > > > if (of_phy_is_fixed_link(dwmac->dev->of_node)) {
> > > >
> > >
> > > This does not help in this case. What I need to determine is if the PHY currently
> > in use is a fixed-link.
> > > The dwmac DTS node may have multiple PHY nodes defined, including both
> > fixed-link and real PHYs.
> >
> > ... and this makes me wonder what DT node structure you think would describe a
> > fixed-link.
> >
> > A valid ethernet device node would be:
> >
> >         dwmac-node {
> >                 phy-handle = <&phy1>;
> >         };
> >
> > In this case:
> >         dwmac->dev->of_node points at "dwmac-node"
> >         plat->phylink_node points at "dwmac-node"
> >         plat->phy_node points at "phy1"
> >         Your "dn" is NULL.
> >         Therefore, your imx_dwmac_is_fixed_link() returns false.
> >
> >         dwmac-node {
> >                 fixed-link {
> >                         speed = <...>;
> >                         full-duplex;
> >                 };
> >         };
> >
> > In this case:
> >         dwmac->dev->of_node points at "dwmac-node"
> >         plat->phylink_node points at "dwmac-node"
> >         plat->phy_node is NULL
> >         Your "dn" points at the "fixed-link" node.
> >         Therefore, your imx_dwmac_is_fixed_link() also returns false.
> >
> > Now, as far as your comment "What I need to determine is if the PHY currently
> > in use is a fixed-link." I'm just going "Eh? What?" at that, because it makes zero
> > sense to me.
> >
> > stmmac uses phylink. phylink doesn't use a PHY for fixed-links, unlike the old
> > phylib-based fixed-link implementation that software-emulated a clause-22 PHY.
> > With phylink, when fixed-link is specified, there is _no_ PHY.
> 
> So you mean the fixed-link node will always be the highest priority to
> be used in the phylink use case?

Yes, because that is how all network drivers have behaved. If you look
at the function that Vladimir pointed out, then you will notice that
the mere presence of a fixed-link node makes it a "fixed link".

> If so, I just need to check if there is a fixed-link node as Vladimir pointed out, right?

You could, but that is grossly inefficient, and I will NAK it because
by doing so, it makes this messy driver even worse.

> > There is no need to do any of this poking about to determine if the link that is
> > being brought up is a fixed-link or not, because phylink's callbacks into the MAC
> > driver already contain this information in the "mode" argument. However, that
> > is not passed to the driver's internal
> > priv->plat->fix_mac_speed() method - but this is the information you
> > need.
> >
> 
> Yes, you are right. The best way is to change the fix_mac_speed prototype
> but it will change several other platforms. That's why I didn't go that way.

Why is that a problem?

I really don't get this "I can't get at information I need without
changing a driver internal interface, so I'll write some really
inefficient code to work around the problem and make the driver
even more messy" attitude.

It's not like you're changing a publicly visible API - it's a
driver private API and all the users of it are in the kernel tree.

A standard part of open source development is not to bodge around
existing code, but to implement efficient solutions to problems.

As phylink *already* tells stmmac_mac_link_up() whether it is
operating with a PHY, fixed-link, or in-band mode, the stmmac
layer has the information you need, but doesn't pass this into
the fix_mac_speed() function.

The best solution to this is *not* to bodge around it by trying
to second-guess what's going on and thus creating messy code.

Given that we have the full source available which we can modify,
then changing things like this function pointer prototype is
absolutely acceptable, and in this case is the correct way to
address the issue you have.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!