Message-ID:
 <PAXPR04MB9185108CB4A04C4CD5AE29FC8900A@PAXPR04MB9185.eurprd04.prod.outlook.com>
Date: Wed, 26 Jul 2023 18:47:15 +0000
From: Shenwei Wang <shenwei.wang@....com>
To: Russell King <linux@...linux.org.uk>
CC: Vladimir Oltean <olteanv@...il.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>, Shawn Guo <shawnguo@...nel.org>, dl-linux-imx
	<linux-imx@....com>, Giuseppe Cavallaro <peppe.cavallaro@...com>, Alexandre
 Torgue <alexandre.torgue@...s.st.com>, Jose Abreu <joabreu@...opsys.com>,
	Sascha Hauer <s.hauer@...gutronix.de>, Pengutronix Kernel Team
	<kernel@...gutronix.de>, Fabio Estevam <festevam@...il.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-stm32@...md-mailman.stormreply.com"
	<linux-stm32@...md-mailman.stormreply.com>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "imx@...ts.linux.dev"
	<imx@...ts.linux.dev>, Frank Li <frank.li@....com>
Subject: RE: [EXT] Re: [PATCH] net: stmmac: dwmac-imx: pause the TXC clock in
 fixed-link



> -----Original Message-----
> From: Russell King <linux@...linux.org.uk>
> Sent: Wednesday, July 26, 2023 12:01 PM
> To: Shenwei Wang <shenwei.wang@....com>
> Cc: Vladimir Oltean <olteanv@...il.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>; Shawn Guo <shawnguo@...nel.org>;
> dl-linux-imx <linux-imx@....com>; Giuseppe Cavallaro
> <peppe.cavallaro@...com>; Alexandre Torgue <alexandre.torgue@...s.st.com>;
> Jose Abreu <joabreu@...opsys.com>; Sascha Hauer <s.hauer@...gutronix.de>;
> Pengutronix Kernel Team <kernel@...gutronix.de>; Fabio Estevam
> <festevam@...il.com>; netdev@...r.kernel.org; linux-stm32@...md-
> mailman.stormreply.com; linux-arm-kernel@...ts.infradead.org;
> imx@...ts.linux.dev; Frank Li <frank.li@....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:59:38PM +0000, Shenwei Wang wrote:
> > > -----Original Message-----
> > > From: Russell King <linux@...linux.org.uk>
> > > Sent: Wednesday, July 26, 2023 10:29 AM
> > > To: Shenwei Wang <shenwei.wang@....com>
> > > Cc: Vladimir Oltean <olteanv@...il.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>; Shawn Guo
> > > <shawnguo@...nel.org>; dl-linux-imx <linux-imx@....com>; Giuseppe
> > > Cavallaro <peppe.cavallaro@...com>; Alexandre Torgue
> > > <alexandre.torgue@...s.st.com>; Jose Abreu <joabreu@...opsys.com>;
> > > Sascha Hauer <s.hauer@...gutronix.de>; Pengutronix Kernel Team
> > > <kernel@...gutronix.de>; Fabio Estevam <festevam@...il.com>;
> > > netdev@...r.kernel.org; linux-stm32@...md- mailman.stormreply.com;
> > > linux-arm-kernel@...ts.infradead.org;
> > > imx@...ts.linux.dev; Frank Li <frank.li@....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".
>

Then, the way this phylink driver behaves makes the rest of the discussion kind of pointless
for now, because I don't actually need fix_mac_speed to give me any interface info now.
The basic of_phy_is_fixed_link check does the job for me.

Not sure why you think it's inefficient - could you explain that part?

Thanks,
Shenwei

> > 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.ar/
> mlinux.org.uk%2Fdeveloper%2Fpatches%2F&data=05%7C01%7Cshenwei.wang
> %40nxp.com%7C4a805e10cd20434538f608db8df9edf5%7C686ea1d3bc2b4c6fa
> 92cd99c5c301635%7C0%7C0%7C638259876795921310%7CUnknown%7CTWFp
> bGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6
> Mn0%3D%7C3000%7C%7C%7C&sdata=9A30fzTXCfc6fYU4Ttijm0c7Piy18tFUWW
> yxtyQoJLo%3D&reserved=0
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Powered by blists - more mailing lists