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 <[email protected]>
---
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
Hello,
On Fri, 8 Dec 2023 07:02:19 +0000
Jiangfeng Ma <[email protected]> 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
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 <[email protected]> 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/[email protected]/
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
>
> -----邮件原件-----
> 发件人: Serge Semin <[email protected]>
> 发送时间: Friday, December 8, 2023 10:28 PM
> 收件人: Maxime Chevallier <[email protected]>; Jiangfeng Ma <[email protected]>
> 抄送: Jiangfeng Ma <[email protected]>; Alexandre Torgue <[email protected]>; Jose
> Abreu <[email protected]>; David S. Miller <[email protected]>; Eric Dumazet
> <[email protected]>; Jakub Kicinski <[email protected]>; Paolo Abeni <[email protected]>;
> Maxime Coquelin <[email protected]>; Simon Horman <[email protected]>; Andrew
> Halaney <[email protected]>; Bartosz Golaszewski <[email protected]>; Shenwei
> Wang <[email protected]>; Johannes Zink <[email protected]>; Russell King (Oracle
> <[email protected]>; Jochen Henneberg <[email protected]>; open
> list:STMMAC ETHERNET DRIVER <[email protected]>; moderated list:ARM/STM32
> ARCHITECTURE <[email protected]>; moderated list:ARM/STM32
> ARCHITECTURE <[email protected]>; open list <[email protected]>; James
> Li <[email protected]>; Martin McKenny <[email protected]>
> 主题: 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 <[email protected]> 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
> >
> -----Original Message-----
> From: Jiangfeng Ma
> Sent: Tuesday, December 12, 2023 6:51 PM
> To: Serge Semin <[email protected]>; Maxime Chevallier <[email protected]>
> Cc: Alexandre Torgue <[email protected]>; Jose Abreu <[email protected]>; David
> S. Miller <[email protected]>; Eric Dumazet <[email protected]>; Jakub Kicinski
> <[email protected]>; Paolo Abeni <[email protected]>; Maxime Coquelin
> <[email protected]>; Simon Horman <[email protected]>; Andrew Halaney
> <[email protected]>; Bartosz Golaszewski <[email protected]>; Shenwei Wang
> <[email protected]>; Johannes Zink <[email protected]>; Russell King (Oracle
> <[email protected]>; Jochen Henneberg <[email protected]>; open
> list:STMMAC ETHERNET DRIVER <[email protected]>; moderated list:ARM/STM32
> ARCHITECTURE <[email protected]>; moderated list:ARM/STM32
> ARCHITECTURE <[email protected]>; open list <[email protected]>;
> James Li <[email protected]>; Martin McKenny <[email protected]>
> 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 <[email protected]>
> > Sent: Friday, December 8, 2023 10:28 PM
> > To: Maxime Chevallier <[email protected]>; Jiangfeng Ma
> <[email protected]>
> > Cc: Jiangfeng Ma <[email protected]>; Alexandre Torgue <[email protected]>;
> Jose
> > Abreu <[email protected]>; David S. Miller <[email protected]>; Eric Dumazet
> > <[email protected]>; Jakub Kicinski <[email protected]>; Paolo Abeni <[email protected]>;
> > Maxime Coquelin <[email protected]>; Simon Horman <[email protected]>; Andrew
> > Halaney <[email protected]>; Bartosz Golaszewski <[email protected]>; Shenwei
> > Wang <[email protected]>; Johannes Zink <[email protected]>; Russell King (Oracle
> > <[email protected]>; Jochen Henneberg <[email protected]>; open
> > list:STMMAC ETHERNET DRIVER <[email protected]>; moderated list:ARM/STM32
> > ARCHITECTURE <[email protected]>; moderated list:ARM/STM32
> > ARCHITECTURE <[email protected]>; open list <[email protected]>;
> James
> > Li <[email protected]>; Martin McKenny <[email protected]>
> > 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 <[email protected]> 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
> > >
On Tue, Dec 12, 2023 at 10:50:53AM +0000, Jiangfeng Ma wrote:
>
>
> > -----邮件原件-----
> > 发件人: Serge Semin <[email protected]>
> > 发送时间: Friday, December 8, 2023 10:28 PM
> > 收件人: Maxime Chevallier <[email protected]>; Jiangfeng Ma <[email protected]>
> > 抄送: Jiangfeng Ma <[email protected]>; Alexandre Torgue <[email protected]>; Jose
> > Abreu <[email protected]>; David S. Miller <[email protected]>; Eric Dumazet
> > <[email protected]>; Jakub Kicinski <[email protected]>; Paolo Abeni <[email protected]>;
> > Maxime Coquelin <[email protected]>; Simon Horman <[email protected]>; Andrew
> > Halaney <[email protected]>; Bartosz Golaszewski <[email protected]>; Shenwei
> > Wang <[email protected]>; Johannes Zink <[email protected]>; Russell King (Oracle
> > <[email protected]>; Jochen Henneberg <[email protected]>; open
> > list:STMMAC ETHERNET DRIVER <[email protected]>; moderated list:ARM/STM32
> > ARCHITECTURE <[email protected]>; moderated list:ARM/STM32
> > ARCHITECTURE <[email protected]>; open list <[email protected]>; James
> > Li <[email protected]>; Martin McKenny <[email protected]>
> > 主题: 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 <[email protected]> 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
> > >