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

[Shenwei Wang]

> -----Original Message-----
> From: Russell King <linux at armlinux.org.uk>
> Sent: Wednesday, July 26, 2023 11:30 AM
> 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: 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 04:10:11PM +0000, Shenwei Wang wrote:
> > > 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?
> > >
> >
> > We can't make that assumption. In my testing code, I had trace
> > statements in that section to indicate the code was actually executed. You
> may get some clues from the following DTS:
> >
> > +&eqos {
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&pinctrl_eqos_rgmii>;
> > +       phy-mode = "rgmii-rxid";
> > +       phy-handle = <&fixed0>;
> > +       status = "okay";
> > +
> > +       fixed0: fixed-link {
> > +               speed = <1000>;
> > +               full-duplex;
> > +       };
>
> This is just totally botched up.
>
> "fixed0" is _not_ a PHY, therefore you should NOT be referencing it in phy-
> handle. Please see the DT binding document:
>

If there is a hidden rule here, it should be added to the CHECK_DTBS schema target.
That way, users would get a warning or syntax error when running the tools, reminding
them to follow the undisclosed rule.

Thanks,
Shenwei

>   phy-handle:
>     $ref: /schemas/types.yaml#/definitions/phandle
>     description:
>       Specifies a reference to a node representing a PHY device.
>
>   fixed-link:
>     oneOf:
>       - $ref: /schemas/types.yaml#/definitions/uint32-array
>         deprecated: true
> ...
>       - type: object
>         additionalProperties: false
>         properties:
>           speed:
> ...
>
> As I said, fixed-link is _not_ a PHY, and thus phy-handle must *not* be used to
> point at it.
>
> The mere presence of a node called "fixed-link" will make this "eqos"
> device use that fixed-link node, and the phy-handle will be ignored.
>
> So sorry, but as far as your patch goes, it's a hard NAK from me right now until
> the DT description is actually correct.
>
> --
> RMK's Patch system:
> https://www.ar/
> mlinux.org.uk%2Fdeveloper%2Fpatches%2F&data=05%7C01%7Cshenwei.wang
> %40nxp.com%7C2396bc12c0524d7e006e08db8df58103%7C686ea1d3bc2b4c6f
> a92cd99c5c301635%7C0%7C0%7C638259857794101296%7CUnknown%7CTWF
> pbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6
> Mn0%3D%7C3000%7C%7C%7C&sdata=6VRey8tkgXhSaXSrf%2B0JVhwUivzVFPK
> QDzte0oKrIck%3D&reserved=0
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!