回复: [PATCH] net:stmmac:stmmac_platform:Add snps,xpcs devicetree parsing

[Serge Semin]

On Tue, Dec 12, 2023 at 10:50:53AM +0000, Jiangfeng Ma wrote:
> 
> 
> > -----邮件原件-----
> > 发件人: Serge Semin <fancer.lancer at gmail.com>
> > 发送时间: Friday, December 8, 2023 10:28 PM
> > 收件人: Maxime Chevallier <maxime.chevallier at bootlin.com>; Jiangfeng Ma <jiama at synopsys.com>
> > 抄送: Jiangfeng Ma <jiama at synopsys.com>; Alexandre Torgue <alexandre.torgue at foss.st.com>; Jose
> > Abreu <joabreu at synopsys.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>; Simon Horman <horms at kernel.org>; Andrew
> > Halaney <ahalaney at redhat.com>; Bartosz Golaszewski <bartosz.golaszewski at linaro.org>; Shenwei
> > Wang <shenwei.wang at nxp.com>; Johannes Zink <j.zink at pengutronix.de>; Russell King (Oracle
> > <rmk+kernel at armlinux.org.uk>; Jochen Henneberg <jh at henneberg-systemdesign.com>; open
> > list:STMMAC ETHERNET DRIVER <netdev at vger.kernel.org>; moderated list:ARM/STM32
> > ARCHITECTURE <linux-stm32 at st-md-mailman.stormreply.com>; moderated list:ARM/STM32
> > ARCHITECTURE <linux-arm-kernel at lists.infradead.org>; open list <linux-kernel at vger.kernel.org>; James
> > Li <lijames at synopsys.com>; Martin McKenny <mmckenny at synopsys.com>
> > 主题: Re: [PATCH] net:stmmac:stmmac_platform:Add snps,xpcs devicetree parsing
> > 
> Hi Maxime, Serge
> Thanks for your review!
> 
> > Hi Maxime, Jiangfeng
> > 
> > On Fri, Dec 08, 2023 at 09:14:08AM +0100, Maxime Chevallier wrote:
> > > Hello,
> > >
> > > On Fri, 8 Dec 2023 07:02:19 +0000
> > > Jiangfeng Ma <Jiangfeng.Ma at synopsys.com> wrote:
> > >
> > > > In order to setup xpcs, has_xpcs must be set to a non-zero value.
> > > > Add new optional devicetree properties representing this.
> > > >
> > > > If has_xpcs is set to true, then xpcs_an_inband should preferably be
> > > > consistent with it, Otherwise, some errors may occur when starting
> > > > the network, For example, the phy interface that xpcs already supports,
> > > > but link up fails.
> > >
> > > Can you elaborate on why you need this, and on which platform
> > > especially ? Usually drivers for the various stmmac variants know if
> > > they have XPCS or not, or can guess it based on some info such as the
> > > configured PHY mode (dwmac-intel).
> 
> There is no specific platform here. I utilize the dwmcac-generic platform,
> and xpcs is utilized as the MDIO device or it can be seen as a C45 PHY.
> While it's sometimes possible to deduce the presence of xpcs based on information
> such as the phy mode (dwmac-intel), this is not always a definitive indicator.
> For instance, the support of SGMII by XPCS doesn't imply
> that all SGMII-supporting PHYs include XPCS. But as Serge mentioned, using pcs-handle,
> or pcs-handle-name might be a more effective approach.
> > >
> > > Besides that, there are a few issues with your submission. If DT is the
> > > way to go (and I don't say it is), you would also need to update the
> > > bindings to document that property.
> > >
> > > > The types of has_xpcs and xpcs_an_inband are unsigned int,
> > > > and generally used as flags. So it may be more reasonable to set them to
> > > > bool type. This can also be confirmed from the type of @ovr_an_inband.
> > >
> > > And this part would go into a separate patch.
> Sorry for this issue, I will create the patch separately later.
> > 
> > In addition to what Maxime already said having DT-bindings adjusted to
> > fit to the pattern implemented in the software part is a wrong way to
> > go. The best choice in this case is to add the DW XPCS DT-node to the
> > DW MAC MDIO/MI bus and then use the "pcs-handle" to inform the MAC
> > (mainly it's driver) of what PCS-device is actually attached to it.
> > The series I submitted on this week is exactly about that:
> > https://urldefense.com/v3/__https://lore.kernel.org/netdev/20231205103559.9605-1-fancer.lancer@g
> > mail.com/__;!!A4F2R9G_pg!Y6R3WZWHeBdrkZklbqrAQARbHnQ-g_Tbb6r5IqcsSHMQ_l4rOzLLgZvLPl6YP
> > BYferbjrbjZA6_XvSSSvkV35eo2jWPz$
> > I guess I'll need about a month or so to settle all the comments, but
> > the solution implemented there will be better than this one really.
> >

> Yes, I agree that binding the xpcs via the "pcs-handle" DT firmware node 
> is a better way. but the current method of binding xpcs through scanning 
> addresses still relies on mdio_bus_data->has_xpcs.

It doesn't matter on what the code relies. What matters is to
correctly describe the hardware. Adding the 'snps,xpcs' property would
just be a workround so to make things working because the driver was
designed that way.

> The 16th patch in your patchset also mentions the difficulty of 
> obtaining has_xpcs. Therefore, can we add parsing of pcs-handle-names 
> in the platform to determine if the xpcs exists, like this:
> 
> if (plat->mdio_bus_data) {
> 	rc = of_property_match_string(np, "pcs-handle-names", "dw-xpcs");
> 	if (rc >= 0) {
> 		plat->mdio_bus_data->has_xpcs = true;
> 		plat->mdio_bus_data->xpcs_an_inband = true;
> 	}
> }

It won't make sense. 'pcs-handle' would already point out to the
MDIO-device. Since it's doubtfully there is DW XGMAC connected to more
than one PCS device, then there is no need in the named handle
property. Moreover your way of bindings violates bindings rule that
the 'pcs-handle-names' array should always be specified together with
the phandles array:
Documentation/devicetree/bindings/net/ethernet-controller.yaml

Please be patient. After my patchset is merged in, the only thing what
you would need is to do something like this:

xgmac: ethernet at 1f054000 {
	compatible = "snps,dwxgmac";
	reg = <0 0x1f054000 0 0x4000>;

	...

	pcs-handle = <&xgmac_pcs>;

	xgmac_mdio: mdio {
		compatible = "snps,dwmac-mdio";
		#address-cells = <1>;
		#size-cells = <0>;

		xgmac_pcs: ethernet-pcs at 0 {
			compatible = "snps,dw-xpcs";
			reg = <0>;
		};
	};
};

If no XPCS available, just omit the 'pcs-handle' property and the
respective MDIO-bus sub-node.

-Serge(y)

> 
> Thanks,
> Jiangfeng
> 
> > -Serge(y)
> > 
> > >
> > > Thanks,
> > >
> > > Maxime
> > >