All am65-cpsw controllers have a fixed TX delay, so the PHY interface
mode must be fixed up to account for this.
Modes that claim to a delay on the PCB can't actually work. Warn people
to update their Device Trees if one of the unsupported modes is specified.
Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
drivers/net/ethernet/ti/am65-cpsw-nuss.c | 27 ++++++++++++++++++++++--
1 file changed, 25 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index f20d1ff192efe..519757e618ad0 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -2602,6 +2602,7 @@ static int am65_cpsw_nuss_init_slave_ports(struct am65_cpsw_common *common)
return -ENOENT;
for_each_child_of_node(node, port_np) {
+ phy_interface_t phy_if;
struct am65_cpsw_port *port;
u32 port_id;
@@ -2667,14 +2668,36 @@ static int am65_cpsw_nuss_init_slave_ports(struct am65_cpsw_common *common)
/* get phy/link info */
port->slave.port_np = of_node_get(port_np);
- ret = of_get_phy_mode(port_np, &port->slave.phy_if);
+ ret = of_get_phy_mode(port_np, &phy_if);
if (ret) {
dev_err(dev, "%pOF read phy-mode err %d\n",
port_np, ret);
goto of_node_put;
}
- ret = phy_set_mode_ext(port->slave.ifphy, PHY_MODE_ETHERNET, port->slave.phy_if);
+ /* CPSW controllers supported by this driver have a fixed
+ * internal TX delay in RGMII mode. Fix up PHY mode to account
+ * for this and warn about Device Trees that claim to have a TX
+ * delay on the PCB.
+ */
+ switch (phy_if) {
+ case PHY_INTERFACE_MODE_RGMII_ID:
+ phy_if = PHY_INTERFACE_MODE_RGMII_RXID;
+ break;
+ case PHY_INTERFACE_MODE_RGMII_TXID:
+ phy_if = PHY_INTERFACE_MODE_RGMII;
+ break;
+ case PHY_INTERFACE_MODE_RGMII:
+ case PHY_INTERFACE_MODE_RGMII_RXID:
+ dev_warn(dev,
+ "RGMII mode without internal TX delay unsupported; please fix your Device Tree\n");
+ break;
+ default:
+ break;
+ }
+
+ port->slave.phy_if = phy_if;
+ ret = phy_set_mode_ext(port->slave.ifphy, PHY_MODE_ETHERNET, phy_if);
if (ret)
goto of_node_put;
--
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/
Hi, On Tue Jun 24, 2025 at 12:53 PM CEST, Matthias Schiffer wrote: > All am65-cpsw controllers have a fixed TX delay, so the PHY interface > mode must be fixed up to account for this. > > Modes that claim to a delay on the PCB can't actually work. Warn people > to update their Device Trees if one of the unsupported modes is specified. > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> > Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com> > Reviewed-by: Andrew Lunn <andrew@lunn.ch> For whatever reason, this patch is breaking network on our board (just transmission). We have rgmii-id in our devicetree which is now modified to be just rgmii-rxid. The board has a TI AM67A (J722S) with a Broadcom BCM54210E PHY. I'm not sure, if AM67A MAC doesn't add any delay or if it's too small. I'll need to ask around if there are any measurements but my colleague doing the measurements is on holiday at the moment. -michael
On Mon, Jul 14, 2025 at 12:01:22PM +0200, Michael Walle wrote: > Hi, > > On Tue Jun 24, 2025 at 12:53 PM CEST, Matthias Schiffer wrote: > > All am65-cpsw controllers have a fixed TX delay, so the PHY interface > > mode must be fixed up to account for this. > > > > Modes that claim to a delay on the PCB can't actually work. Warn people > > to update their Device Trees if one of the unsupported modes is specified. > > > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> > > Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com> > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > For whatever reason, this patch is breaking network on our board > (just transmission). We have rgmii-id in our devicetree which is now > modified to be just rgmii-rxid. The board has a TI AM67A (J722S) with a > Broadcom BCM54210E PHY. I'm not sure, if AM67A MAC doesn't add any > delay or if it's too small. I'll need to ask around if there are any > measurements but my colleague doing the measurements is on holiday > at the moment. I agree, we need to see if this is a AM65 vs AM67 issue. rgmii-id would be correct if the MAC is not adding delays. Do you have access to the datasheets for both? Can you do a side by side comparison for the section which describes the fixed TX delay? Andrew
Hi, On Mon Jul 14, 2025 at 3:09 PM CEST, Andrew Lunn wrote: > On Mon, Jul 14, 2025 at 12:01:22PM +0200, Michael Walle wrote: > > On Tue Jun 24, 2025 at 12:53 PM CEST, Matthias Schiffer wrote: > > > All am65-cpsw controllers have a fixed TX delay, so the PHY interface > > > mode must be fixed up to account for this. > > > > > > Modes that claim to a delay on the PCB can't actually work. Warn people > > > to update their Device Trees if one of the unsupported modes is specified. > > > > > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> > > > Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com> > > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > > > For whatever reason, this patch is breaking network on our board > > (just transmission). We have rgmii-id in our devicetree which is now > > modified to be just rgmii-rxid. The board has a TI AM67A (J722S) with a > > Broadcom BCM54210E PHY. I'm not sure, if AM67A MAC doesn't add any > > delay or if it's too small. I'll need to ask around if there are any > > measurements but my colleague doing the measurements is on holiday > > at the moment. > > I agree, we need to see if this is a AM65 vs AM67 issue. rgmii-id > would be correct if the MAC is not adding delays. > > Do you have access to the datasheets for both? Can you do a side by > side comparison for the section which describes the fixed TX delay? The datasheets and TRMs of the SoC are public of the SoC. According to the AM67A TRM the delay should be 1.2ns if I'm reading it correctly. The BCM PHY requires a setup time of -0.9ns (min). So, is should work (?), but it doesn't. I'm also not aware of any routing skew between the signals. But as I said, I'll have to check with my colleague next week. -michael
On Tue, Jun 24, 2025 at 12:53:33PM +0200, Matthias Schiffer wrote:
Hello Matthias,
> All am65-cpsw controllers have a fixed TX delay, so the PHY interface
> mode must be fixed up to account for this.
>
> Modes that claim to a delay on the PCB can't actually work. Warn people
> to update their Device Trees if one of the unsupported modes is specified.
>
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> ---
> drivers/net/ethernet/ti/am65-cpsw-nuss.c | 27 ++++++++++++++++++++++--
> 1 file changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> index f20d1ff192efe..519757e618ad0 100644
> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> @@ -2602,6 +2602,7 @@ static int am65_cpsw_nuss_init_slave_ports(struct am65_cpsw_common *common)
> return -ENOENT;
>
> for_each_child_of_node(node, port_np) {
> + phy_interface_t phy_if;
> struct am65_cpsw_port *port;
> u32 port_id;
>
> @@ -2667,14 +2668,36 @@ static int am65_cpsw_nuss_init_slave_ports(struct am65_cpsw_common *common)
>
> /* get phy/link info */
> port->slave.port_np = of_node_get(port_np);
> - ret = of_get_phy_mode(port_np, &port->slave.phy_if);
> + ret = of_get_phy_mode(port_np, &phy_if);
> if (ret) {
> dev_err(dev, "%pOF read phy-mode err %d\n",
> port_np, ret);
> goto of_node_put;
> }
>
> - ret = phy_set_mode_ext(port->slave.ifphy, PHY_MODE_ETHERNET, port->slave.phy_if);
> + /* CPSW controllers supported by this driver have a fixed
> + * internal TX delay in RGMII mode. Fix up PHY mode to account
> + * for this and warn about Device Trees that claim to have a TX
> + * delay on the PCB.
> + */
> + switch (phy_if) {
> + case PHY_INTERFACE_MODE_RGMII_ID:
> + phy_if = PHY_INTERFACE_MODE_RGMII_RXID;
> + break;
> + case PHY_INTERFACE_MODE_RGMII_TXID:
> + phy_if = PHY_INTERFACE_MODE_RGMII;
> + break;
> + case PHY_INTERFACE_MODE_RGMII:
> + case PHY_INTERFACE_MODE_RGMII_RXID:
> + dev_warn(dev,
> + "RGMII mode without internal TX delay unsupported; please fix your Device Tree\n");
Existing users designed boards and enabled Ethernet functionality using
"rgmii-rxid" in the device-tree and implementing the PCB traces in a
way that they interpret "rgmii-rxid". So their (mis)interpretation of
it is being challenged by the series. While it is true that we are updating
the bindings and driver to move towards the correct definition, I believe that
the above message would cause confusion. Would it be alright to update it to
something similar to:
"Interpretation of RGMII delays has been corrected; no functional impact; please fix your Device Tree"
Regards,
Siddharth.
On Thu, Jun 26, 2025 at 03:10:50PM +0530, Siddharth Vadapalli wrote:
> On Tue, Jun 24, 2025 at 12:53:33PM +0200, Matthias Schiffer wrote:
>
> Hello Matthias,
>
> > All am65-cpsw controllers have a fixed TX delay, so the PHY interface
> > mode must be fixed up to account for this.
> >
> > Modes that claim to a delay on the PCB can't actually work. Warn people
> > to update their Device Trees if one of the unsupported modes is specified.
> >
> > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> > Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> > ---
> > drivers/net/ethernet/ti/am65-cpsw-nuss.c | 27 ++++++++++++++++++++++--
> > 1 file changed, 25 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> > index f20d1ff192efe..519757e618ad0 100644
> > --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> > +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> > @@ -2602,6 +2602,7 @@ static int am65_cpsw_nuss_init_slave_ports(struct am65_cpsw_common *common)
> > return -ENOENT;
> >
> > for_each_child_of_node(node, port_np) {
> > + phy_interface_t phy_if;
> > struct am65_cpsw_port *port;
> > u32 port_id;
> >
> > @@ -2667,14 +2668,36 @@ static int am65_cpsw_nuss_init_slave_ports(struct am65_cpsw_common *common)
> >
> > /* get phy/link info */
> > port->slave.port_np = of_node_get(port_np);
> > - ret = of_get_phy_mode(port_np, &port->slave.phy_if);
> > + ret = of_get_phy_mode(port_np, &phy_if);
> > if (ret) {
> > dev_err(dev, "%pOF read phy-mode err %d\n",
> > port_np, ret);
> > goto of_node_put;
> > }
> >
> > - ret = phy_set_mode_ext(port->slave.ifphy, PHY_MODE_ETHERNET, port->slave.phy_if);
> > + /* CPSW controllers supported by this driver have a fixed
> > + * internal TX delay in RGMII mode. Fix up PHY mode to account
> > + * for this and warn about Device Trees that claim to have a TX
> > + * delay on the PCB.
> > + */
> > + switch (phy_if) {
> > + case PHY_INTERFACE_MODE_RGMII_ID:
> > + phy_if = PHY_INTERFACE_MODE_RGMII_RXID;
> > + break;
> > + case PHY_INTERFACE_MODE_RGMII_TXID:
> > + phy_if = PHY_INTERFACE_MODE_RGMII;
> > + break;
> > + case PHY_INTERFACE_MODE_RGMII:
> > + case PHY_INTERFACE_MODE_RGMII_RXID:
> > + dev_warn(dev,
> > + "RGMII mode without internal TX delay unsupported; please fix your Device Tree\n");
>
> Existing users designed boards and enabled Ethernet functionality using
> "rgmii-rxid" in the device-tree and implementing the PCB traces in a
> way that they interpret "rgmii-rxid". So their (mis)interpretation of
> it is being challenged by the series. While it is true that we are updating
> the bindings and driver to move towards the correct definition, I believe that
> the above message would cause confusion. Would it be alright to update it to
> something similar to:
>
> "Interpretation of RGMII delays has been corrected; no functional impact; please fix your Device Tree"
It is dev_warn() not dev_err(), so it should be read as a warning. And
the device will continue to probe and work. So I think the message is
O.K. What we don't want is DT developers thinking they can just ignore
it. So i would keep it reasonably strongly worded.
Andrew
On Thu, Jun 26, 2025 at 01:58:07PM +0200, Andrew Lunn wrote:
> On Thu, Jun 26, 2025 at 03:10:50PM +0530, Siddharth Vadapalli wrote:
> > On Tue, Jun 24, 2025 at 12:53:33PM +0200, Matthias Schiffer wrote:
> >
> > Hello Matthias,
> >
> > > All am65-cpsw controllers have a fixed TX delay, so the PHY interface
> > > mode must be fixed up to account for this.
> > >
> > > Modes that claim to a delay on the PCB can't actually work. Warn people
> > > to update their Device Trees if one of the unsupported modes is specified.
> > >
> > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> > > Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > > Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> > > ---
> > > drivers/net/ethernet/ti/am65-cpsw-nuss.c | 27 ++++++++++++++++++++++--
> > > 1 file changed, 25 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> > > index f20d1ff192efe..519757e618ad0 100644
> > > --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> > > +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> > > @@ -2602,6 +2602,7 @@ static int am65_cpsw_nuss_init_slave_ports(struct am65_cpsw_common *common)
> > > return -ENOENT;
> > >
> > > for_each_child_of_node(node, port_np) {
> > > + phy_interface_t phy_if;
> > > struct am65_cpsw_port *port;
> > > u32 port_id;
> > >
> > > @@ -2667,14 +2668,36 @@ static int am65_cpsw_nuss_init_slave_ports(struct am65_cpsw_common *common)
> > >
> > > /* get phy/link info */
> > > port->slave.port_np = of_node_get(port_np);
> > > - ret = of_get_phy_mode(port_np, &port->slave.phy_if);
> > > + ret = of_get_phy_mode(port_np, &phy_if);
> > > if (ret) {
> > > dev_err(dev, "%pOF read phy-mode err %d\n",
> > > port_np, ret);
> > > goto of_node_put;
> > > }
> > >
> > > - ret = phy_set_mode_ext(port->slave.ifphy, PHY_MODE_ETHERNET, port->slave.phy_if);
> > > + /* CPSW controllers supported by this driver have a fixed
> > > + * internal TX delay in RGMII mode. Fix up PHY mode to account
> > > + * for this and warn about Device Trees that claim to have a TX
> > > + * delay on the PCB.
> > > + */
> > > + switch (phy_if) {
> > > + case PHY_INTERFACE_MODE_RGMII_ID:
> > > + phy_if = PHY_INTERFACE_MODE_RGMII_RXID;
> > > + break;
> > > + case PHY_INTERFACE_MODE_RGMII_TXID:
> > > + phy_if = PHY_INTERFACE_MODE_RGMII;
> > > + break;
> > > + case PHY_INTERFACE_MODE_RGMII:
> > > + case PHY_INTERFACE_MODE_RGMII_RXID:
> > > + dev_warn(dev,
> > > + "RGMII mode without internal TX delay unsupported; please fix your Device Tree\n");
> >
> > Existing users designed boards and enabled Ethernet functionality using
> > "rgmii-rxid" in the device-tree and implementing the PCB traces in a
> > way that they interpret "rgmii-rxid". So their (mis)interpretation of
> > it is being challenged by the series. While it is true that we are updating
> > the bindings and driver to move towards the correct definition, I believe that
> > the above message would cause confusion. Would it be alright to update it to
> > something similar to:
> >
> > "Interpretation of RGMII delays has been corrected; no functional impact; please fix your Device Tree"
>
> It is dev_warn() not dev_err(), so it should be read as a warning. And
> the device will continue to probe and work. So I think the message is
> O.K. What we don't want is DT developers thinking they can just ignore
> it. So i would keep it reasonably strongly worded.
Thank you for the clarification. I have no further concerns and the
patch looks good to me.
Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com>
Regards,
Siddharth.
© 2016 - 2025 Red Hat, Inc.