netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] net:stmmac:stmmac_platform:Add snps,xpcs devicetree parsing
@ 2023-12-08  7:02 Jiangfeng Ma
  2023-12-08  8:14 ` Maxime Chevallier
  0 siblings, 1 reply; 6+ messages in thread
From: Jiangfeng Ma @ 2023-12-08  7:02 UTC (permalink / raw)
  To: Alexandre Torgue, Jose Abreu, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Maxime Coquelin, Simon Horman,
	Andrew Halaney, Bartosz Golaszewski, Shenwei Wang, Johannes Zink,
	Russell King  (Oracle, Jiangfeng Ma, Jochen Henneberg
  Cc: open list:STMMAC ETHERNET DRIVER,
	moderated  list:ARM/STM32 ARCHITECTURE,
	moderated list:ARM/STM32 ARCHITECTURE, open list, James Li,
	Martin McKenny

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.

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.

Signed-off-by: Jiangfeng Ma <Jiangfeng.Ma@synopsys.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 5 +++++
 include/linux/stmmac.h                                | 4 ++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index 1ffde55..6ebc2a9 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -324,6 +324,7 @@ static int stmmac_dt_phy(struct plat_stmmacenet_data *plat,
 			 struct device_node *np, struct device *dev)
 {
 	bool mdio = !of_phy_is_fixed_link(np);
+	bool has_xpcs = false;
 	static const struct of_device_id need_mdio_ids[] = {
 		{ .compatible = "snps,dwc-qos-ethernet-4.10" },
 		{},
@@ -345,6 +346,7 @@ static int stmmac_dt_phy(struct plat_stmmacenet_data *plat,
 
 	if (plat->mdio_node) {
 		dev_dbg(dev, "Found MDIO subnode\n");
+		has_xpcs = of_property_read_bool(plat->mdio_node, "snps,xpcs");
 		mdio = true;
 	}
 
@@ -356,6 +358,9 @@ static int stmmac_dt_phy(struct plat_stmmacenet_data *plat,
 			return -ENOMEM;
 
 		plat->mdio_bus_data->needs_reset = true;
+		plat->mdio_bus_data->has_xpcs = has_xpcs;
+		if (plat->mdio_bus_data->has_xpcs)
+			plat->mdio_bus_data->xpcs_an_inband = true;
 	}
 
 	return 0;
diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
index dee5ad6..dea35ee 100644
--- a/include/linux/stmmac.h
+++ b/include/linux/stmmac.h
@@ -82,8 +82,8 @@
 
 struct stmmac_mdio_bus_data {
 	unsigned int phy_mask;
-	unsigned int has_xpcs;
-	unsigned int xpcs_an_inband;
+	bool has_xpcs;
+	bool xpcs_an_inband;
 	int *irqs;
 	int probed_phy_irq;
 	bool needs_reset;
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] net:stmmac:stmmac_platform:Add snps,xpcs devicetree parsing
  2023-12-08  7:02 [PATCH] net:stmmac:stmmac_platform:Add snps,xpcs devicetree parsing Jiangfeng Ma
@ 2023-12-08  8:14 ` Maxime Chevallier
  2023-12-08 14:28   ` Serge Semin
  0 siblings, 1 reply; 6+ messages in thread
From: Maxime Chevallier @ 2023-12-08  8:14 UTC (permalink / raw)
  To: Jiangfeng Ma
  Cc: Alexandre Torgue, Jose Abreu, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Maxime Coquelin, Simon Horman,
	Andrew Halaney, Bartosz Golaszewski, Shenwei Wang, Johannes Zink,
	Russell King  (Oracle, Jochen Henneberg,
	open list:STMMAC ETHERNET DRIVER,
	moderated  list:ARM/STM32 ARCHITECTURE,
	moderated list:ARM/STM32 ARCHITECTURE, open list, James Li,
	Martin McKenny

Hello,

On Fri, 8 Dec 2023 07:02:19 +0000
Jiangfeng Ma <Jiangfeng.Ma@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).

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.

Thanks,

Maxime

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] net:stmmac:stmmac_platform:Add snps,xpcs devicetree parsing
  2023-12-08  8:14 ` Maxime Chevallier
@ 2023-12-08 14:28   ` Serge Semin
  2023-12-12 10:50     ` 回复: " Jiangfeng Ma
  0 siblings, 1 reply; 6+ messages in thread
From: Serge Semin @ 2023-12-08 14:28 UTC (permalink / raw)
  To: Maxime Chevallier, Jiangfeng Ma
  Cc: Jiangfeng Ma, Alexandre Torgue, Jose Abreu, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Maxime Coquelin,
	Simon Horman, Andrew Halaney, Bartosz Golaszewski, Shenwei Wang,
	Johannes Zink, Russell King (Oracle, Jochen Henneberg,
	open list:STMMAC ETHERNET DRIVER,
	moderated  list:ARM/STM32 ARCHITECTURE,
	moderated list:ARM/STM32 ARCHITECTURE, open list, James Li,
	Martin McKenny

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@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).
> 
> 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.

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://lore.kernel.org/netdev/20231205103559.9605-1-fancer.lancer@gmail.com/
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.

-Serge(y)

> 
> Thanks,
> 
> Maxime
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* 回复: [PATCH] net:stmmac:stmmac_platform:Add snps,xpcs devicetree parsing
  2023-12-08 14:28   ` Serge Semin
@ 2023-12-12 10:50     ` Jiangfeng Ma
  2023-12-12 13:15       ` Jiangfeng Ma
  2023-12-19 16:13       ` 回复: " Serge Semin
  0 siblings, 2 replies; 6+ messages in thread
From: Jiangfeng Ma @ 2023-12-12 10:50 UTC (permalink / raw)
  To: Serge Semin, Maxime Chevallier
  Cc: Alexandre Torgue, Jose Abreu, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Maxime Coquelin, Simon Horman,
	Andrew Halaney, Bartosz Golaszewski, Shenwei Wang, Johannes Zink,
	Russell King  (Oracle, Jochen Henneberg,
	open list:STMMAC ETHERNET DRIVER,
	moderated  list:ARM/STM32 ARCHITECTURE,
	moderated list:ARM/STM32  ARCHITECTURE, open list, James Li,
	Martin McKenny



> -----邮件原件-----
> 发件人: Serge Semin <fancer.lancer@gmail.com>
> 发送时间: Friday, December 8, 2023 10:28 PM
> 收件人: Maxime Chevallier <maxime.chevallier@bootlin.com>; Jiangfeng Ma <jiama@synopsys.com>
> 抄送: Jiangfeng Ma <jiama@synopsys.com>; Alexandre Torgue <alexandre.torgue@foss.st.com>; Jose
> Abreu <joabreu@synopsys.com>; David S. Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>;
> Maxime Coquelin <mcoquelin.stm32@gmail.com>; Simon Horman <horms@kernel.org>; Andrew
> Halaney <ahalaney@redhat.com>; Bartosz Golaszewski <bartosz.golaszewski@linaro.org>; Shenwei
> Wang <shenwei.wang@nxp.com>; Johannes Zink <j.zink@pengutronix.de>; Russell King (Oracle
> <rmk+kernel@armlinux.org.uk>; Jochen Henneberg <jh@henneberg-systemdesign.com>; open
> list:STMMAC ETHERNET DRIVER <netdev@vger.kernel.org>; moderated list:ARM/STM32
> ARCHITECTURE <linux-stm32@st-md-mailman.stormreply.com>; moderated list:ARM/STM32
> ARCHITECTURE <linux-arm-kernel@lists.infradead.org>; open list <linux-kernel@vger.kernel.org>; James
> Li <lijames@synopsys.com>; Martin McKenny <mmckenny@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@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. 
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;
	}
}

Thanks,
Jiangfeng

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH] net:stmmac:stmmac_platform:Add snps,xpcs devicetree parsing
  2023-12-12 10:50     ` 回复: " Jiangfeng Ma
@ 2023-12-12 13:15       ` Jiangfeng Ma
  2023-12-19 16:13       ` 回复: " Serge Semin
  1 sibling, 0 replies; 6+ messages in thread
From: Jiangfeng Ma @ 2023-12-12 13:15 UTC (permalink / raw)
  To: Serge Semin, Maxime Chevallier
  Cc: Alexandre Torgue, Jose Abreu, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Maxime Coquelin, Simon Horman,
	Andrew Halaney, Bartosz Golaszewski, Shenwei Wang, Johannes Zink,
	Russell King  (Oracle, Jochen Henneberg,
	open list:STMMAC ETHERNET DRIVER,
	moderated  list:ARM/STM32 ARCHITECTURE,
	moderated list:ARM/STM32  ARCHITECTURE, open list, James Li,
	Martin McKenny



> -----Original Message-----
> From: Jiangfeng Ma
> Sent: Tuesday, December 12, 2023 6:51 PM
> To: Serge Semin <fancer.lancer@gmail.com>; Maxime Chevallier <maxime.chevallier@bootlin.com>
> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>; David
> S. Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub Kicinski
> <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Maxime Coquelin
> <mcoquelin.stm32@gmail.com>; Simon Horman <horms@kernel.org>; Andrew Halaney
> <ahalaney@redhat.com>; Bartosz Golaszewski <bartosz.golaszewski@linaro.org>; Shenwei Wang
> <shenwei.wang@nxp.com>; Johannes Zink <j.zink@pengutronix.de>; Russell King (Oracle
> <rmk+kernel@armlinux.org.uk>; Jochen Henneberg <jh@henneberg-systemdesign.com>; open
> list:STMMAC ETHERNET DRIVER <netdev@vger.kernel.org>; moderated list:ARM/STM32
> ARCHITECTURE <linux-stm32@st-md-mailman.stormreply.com>; moderated list:ARM/STM32
> ARCHITECTURE <linux-arm-kernel@lists.infradead.org>; open list <linux-kernel@vger.kernel.org>;
> James Li <lijames@synopsys.com>; Martin McKenny <mmckenny@synopsys.com>
> Subject: Re: [PATCH] net:stmmac:stmmac_platform:Add snps,xpcs devicetree parsing
> 
Sorry for the problem with my mail settings.
> 
> > -----Original Message-----
> > From: Serge Semin <fancer.lancer@gmail.com>
> > Sent: Friday, December 8, 2023 10:28 PM
> > To: Maxime Chevallier <maxime.chevallier@bootlin.com>; Jiangfeng Ma
> <jiama@synopsys.com>
> > Cc: Jiangfeng Ma <jiama@synopsys.com>; Alexandre Torgue <alexandre.torgue@foss.st.com>;
> Jose
> > Abreu <joabreu@synopsys.com>; David S. Miller <davem@davemloft.net>; Eric Dumazet
> > <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>;
> > Maxime Coquelin <mcoquelin.stm32@gmail.com>; Simon Horman <horms@kernel.org>; Andrew
> > Halaney <ahalaney@redhat.com>; Bartosz Golaszewski <bartosz.golaszewski@linaro.org>; Shenwei
> > Wang <shenwei.wang@nxp.com>; Johannes Zink <j.zink@pengutronix.de>; Russell King (Oracle
> > <rmk+kernel@armlinux.org.uk>; Jochen Henneberg <jh@henneberg-systemdesign.com>; open
> > list:STMMAC ETHERNET DRIVER <netdev@vger.kernel.org>; moderated list:ARM/STM32
> > ARCHITECTURE <linux-stm32@st-md-mailman.stormreply.com>; moderated list:ARM/STM32
> > ARCHITECTURE <linux-arm-kernel@lists.infradead.org>; open list <linux-kernel@vger.kernel.org>;
> James
> > Li <lijames@synopsys.com>; Martin McKenny <mmckenny@synopsys.com>
> > Subject:  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@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.
> 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;
> 	}
> }
> 
> Thanks,
> Jiangfeng
> 
> > -Serge(y)
> >
> > >
> > > Thanks,
> > >
> > > Maxime
> > >

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: 回复: [PATCH] net:stmmac:stmmac_platform:Add snps,xpcs devicetree parsing
  2023-12-12 10:50     ` 回复: " Jiangfeng Ma
  2023-12-12 13:15       ` Jiangfeng Ma
@ 2023-12-19 16:13       ` Serge Semin
  1 sibling, 0 replies; 6+ messages in thread
From: Serge Semin @ 2023-12-19 16:13 UTC (permalink / raw)
  To: Jiangfeng Ma
  Cc: Maxime Chevallier, Alexandre Torgue, Jose Abreu, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Maxime Coquelin,
	Simon Horman, Andrew Halaney, Bartosz Golaszewski, Shenwei Wang,
	Johannes Zink, Russell King (Oracle, Jochen Henneberg,
	open list:STMMAC ETHERNET DRIVER,
	moderated  list:ARM/STM32 ARCHITECTURE,
	moderated list:ARM/STM32 ARCHITECTURE, open list, James Li,
	Martin McKenny

On Tue, Dec 12, 2023 at 10:50:53AM +0000, Jiangfeng Ma wrote:
> 
> 
> > -----邮件原件-----
> > 发件人: Serge Semin <fancer.lancer@gmail.com>
> > 发送时间: Friday, December 8, 2023 10:28 PM
> > 收件人: Maxime Chevallier <maxime.chevallier@bootlin.com>; Jiangfeng Ma <jiama@synopsys.com>
> > 抄送: Jiangfeng Ma <jiama@synopsys.com>; Alexandre Torgue <alexandre.torgue@foss.st.com>; Jose
> > Abreu <joabreu@synopsys.com>; David S. Miller <davem@davemloft.net>; Eric Dumazet
> > <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>;
> > Maxime Coquelin <mcoquelin.stm32@gmail.com>; Simon Horman <horms@kernel.org>; Andrew
> > Halaney <ahalaney@redhat.com>; Bartosz Golaszewski <bartosz.golaszewski@linaro.org>; Shenwei
> > Wang <shenwei.wang@nxp.com>; Johannes Zink <j.zink@pengutronix.de>; Russell King (Oracle
> > <rmk+kernel@armlinux.org.uk>; Jochen Henneberg <jh@henneberg-systemdesign.com>; open
> > list:STMMAC ETHERNET DRIVER <netdev@vger.kernel.org>; moderated list:ARM/STM32
> > ARCHITECTURE <linux-stm32@st-md-mailman.stormreply.com>; moderated list:ARM/STM32
> > ARCHITECTURE <linux-arm-kernel@lists.infradead.org>; open list <linux-kernel@vger.kernel.org>; James
> > Li <lijames@synopsys.com>; Martin McKenny <mmckenny@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@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@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@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
> > >

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2023-12-19 16:14 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-08  7:02 [PATCH] net:stmmac:stmmac_platform:Add snps,xpcs devicetree parsing Jiangfeng Ma
2023-12-08  8:14 ` Maxime Chevallier
2023-12-08 14:28   ` Serge Semin
2023-12-12 10:50     ` 回复: " Jiangfeng Ma
2023-12-12 13:15       ` Jiangfeng Ma
2023-12-19 16:13       ` 回复: " Serge Semin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).