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

[Russell King (Oracle)]

On Wed, Jul 26, 2023 at 03:00:49PM +0000, Shenwei Wang wrote:
> 
> 
> > -----Original Message-----
> > From: Russell King <linux at armlinux.org.uk>
> > Sent: Tuesday, July 25, 2023 4:05 PM
> > To: Shenwei Wang <shenwei.wang at nxp.com>
> > Cc: 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: [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 Tue, Jul 25, 2023 at 02:49:31PM -0500, Shenwei Wang wrote:
> > > +static bool imx_dwmac_is_fixed_link(struct imx_priv_data *dwmac) {
> > > +     struct plat_stmmacenet_data *plat_dat;
> > > +     struct device_node *dn;
> > > +
> > > +     if (!dwmac || !dwmac->plat_dat)
> > > +             return false;
> > > +
> > > +     plat_dat = dwmac->plat_dat;
> > > +     dn = of_get_child_by_name(dwmac->dev->of_node, "fixed-link");
> > > +     if (!dn)
> > > +             return false;
> > > +
> > > +     if (plat_dat->phy_node == dn || plat_dat->phylink_node == dn)
> > > +             return true;
> >
> > Why would the phy_node or the phylink_node ever be pointing at the fixed-link
> > node?
> >
> 
> The logic was learned from the function of stmmac_probe_config_dt, and it normally
> save the phy handle to those two members: phy_node and phylink_node. But seems
> checking phy_node is enough here, right?
> 
>         plat->phy_node = of_parse_phandle(np, "phy-handle", 0);
> 
>         /* PHYLINK automatically parses the phy-handle property */
>         plat->phylink_node = np;

So, plat->phy_node will never ever be equal to your "dn" above.
plat->phylink_node is the same as dwmac->dev->of_node above, and
so plat->phylink_node will never be your "dn" above either.

Those two together means that imx_dwmac_is_fixed_link() will _always_
return false, and thus most of the code you're adding is rather
useless.

It also means the code you're submitting probably hasn't been properly
tested.

Have you confirmed that imx_dwmac_is_fixed_link() will actually return
true in your testing? Under what conditions did your testing reveal a
true return value from this function?

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