[PATCH net-next 0/4] RGMII mode clarification + am65-cpsw fix

[PATCH net-next 0/4] RGMII mode clarification + am65-cpsw fix

[Matthias Schiffer]

As discussed [1], the comments for the different rgmii(-*id) modes do not
accurately describe what these values mean. Update the binding
documentation and fix up the mode to account for the fixed TX delay on
the AM65 CPSW Ethernet controllers, similar to the way the icssg-prueth
does it. For backwards compatibility, the "impossible" modes that claim
to have a delay on the PCB are still accepted, but trigger a warning
message.

As Andrew suggested, I have also added a checkpatch check that requires
a comment for any RGMII mode that is not "rgmii-id".

No Device Trees are updated to avoid the warning for now, to give other
projects syncing the Linux Device Trees some time to fix their drivers
as well. I intend to submit an equivalent change for U-Boot's
am65-cpsw-nuss driver as soon as the changes are accepted for Linux.

[1] https://lore.kernel.org/lkml/d25b1447-c28b-4998-b238-92672434dc28@lunn.ch/

Submitting for net-next for now - I don't know it would make sense to
backport some of these changes to stable.

Note: I have also added the maintainers for the TI K3 SoC families to cc
in addition to the get_maintainers.pl output, to loop in some more of
the relevant people at TI. Should MAINTAINERS be extended to include
some of you for the am65-cpsw* files? At the moment, only the netdev
maintainers are reported for drivers/net/ethernet/ti/am65-cpsw-nuss.c
(except for "authored" lines etc.)

Matthias Schiffer (4):
  dt-bindings: net: ethernet-controller: update descriptions of RGMII
    modes
  dt-bindings: net: ti: k3-am654-cpsw-nuss: update phy-mode in example
  net: ethernet: ti: am65-cpsw: fixup PHY mode for fixed RGMII TX delay
  checkpatch: check for comment explaining rgmii(|-rxid|-txid) PHY modes

 Documentation/dev-tools/checkpatch.rst        |  9 +++++++
 .../bindings/net/ethernet-controller.yaml     | 16 ++++++-----
 .../bindings/net/ti,k3-am654-cpsw-nuss.yaml   |  2 +-
 drivers/net/ethernet/ti/am65-cpsw-nuss.c      | 27 +++++++++++++++++--
 scripts/checkpatch.pl                         | 11 ++++++++
 5 files changed, 55 insertions(+), 10 deletions(-)

-- 
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/

[PATCH net-next 1/4] dt-bindings: net: ethernet-controller: update descriptions of RGMII modes

[Matthias Schiffer]

As discussed [1], the comments for the different rgmii(-*id) modes do not
accurately describe what these values mean.

As the Device Tree is primarily supposed to describe the hardware and not
its configuration, the different modes need to distinguish board designs
(if a delay is built into the PCB using different trace lengths); whether
a delay is added on the MAC or the PHY side when needed should not matter.

Unfortunately, implementation in MAC drivers is somewhat inconsistent
where a delay is fixed or configurable on the MAC side. As a first step
towards sorting this out, improve the documentation.

Link: https://lore.kernel.org/lkml/d25b1447-c28b-4998-b238-92672434dc28@lunn.ch/ [1]
Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 .../bindings/net/ethernet-controller.yaml        | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
index 45819b2358002..2ddc1ce2439a6 100644
--- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
@@ -74,19 +74,21 @@ properties:
       - rev-rmii
       - moca
 
-      # RX and TX delays are added by the MAC when required
+      # RX and TX delays are part of the board design (through PCB traces). MAC
+      # and PHY must not add delays.
       - rgmii
 
-      # RGMII with internal RX and TX delays provided by the PHY,
-      # the MAC should not add the RX or TX delays in this case
+      # RGMII with internal RX and TX delays provided by the MAC or PHY. No
+      # delays are included in the board design; this is the most common case
+      # in modern designs.
       - rgmii-id
 
-      # RGMII with internal RX delay provided by the PHY, the MAC
-      # should not add an RX delay in this case
+      # RGMII with internal RX delay provided by the MAC or PHY. TX delay is
+      # part of the board design.
       - rgmii-rxid
 
-      # RGMII with internal TX delay provided by the PHY, the MAC
-      # should not add an TX delay in this case
+      # RGMII with internal TX delay provided by the MAC or PHY. RX delay is
+      # part of the board design.
       - rgmii-txid
       - rtbi
       - smii
-- 
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/

[PATCH net-next 2/4] dt-bindings: net: ti: k3-am654-cpsw-nuss: update phy-mode in example

[Matthias Schiffer]

k3-am65-cpsw-nuss controllers have a fixed internal TX delay, so RXID
mode is not actually possible and will result in a warning from the
driver going forward.

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 .../devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml          | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml b/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml
index b11894fbaec47..c8128b8ca74fb 100644
--- a/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml
+++ b/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml
@@ -282,7 +282,7 @@ examples:
                     ti,syscon-efuse = <&mcu_conf 0x200>;
                     phys = <&phy_gmii_sel 1>;
 
-                    phy-mode = "rgmii-rxid";
+                    phy-mode = "rgmii-id";
                     phy-handle = <&phy0>;
                 };
             };
-- 
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/

[PATCH net-next 3/4] net: ethernet: ti: am65-cpsw: fixup PHY mode for fixed RGMII TX delay

[Matthias Schiffer]

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>
---
 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 c9fd34787c998..a1d32735c7512 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 = 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/

[PATCH net-next 4/4] checkpatch: check for comment explaining rgmii(|-rxid|-txid) PHY modes

[Matthias Schiffer]

Historially, the RGMII PHY modes specified in Device Trees have been
used inconsistently, often referring to the usage of delays on the PHY
side rather than describing the board; many drivers still implement this
incorrectly.

Require a comment in Devices Trees using these modes (usually mentioning
that the delay is relalized on the PCB), so we can avoid adding more
incorrect uses (or will at least notice which drivers still need to be
fixed).

Suggested-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 Documentation/dev-tools/checkpatch.rst |  9 +++++++++
 scripts/checkpatch.pl                  | 11 +++++++++++
 2 files changed, 20 insertions(+)

diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
index abb3ff6820766..8692d3bc155f1 100644
--- a/Documentation/dev-tools/checkpatch.rst
+++ b/Documentation/dev-tools/checkpatch.rst
@@ -513,6 +513,15 @@ Comments
 
     See: https://lore.kernel.org/lkml/20131006222342.GT19510@leaf/
 
+  **UNCOMMENTED_RGMII_MODE**
+    Historially, the RGMII PHY modes specified in Device Trees have been
+    used inconsistently, often referring to the usage of delays on the PHY
+    side rather than describing the board.
+
+    PHY modes "rgmii", "rgmii-rxid" and "rgmii-txid" modes require the clock
+    signal to be delayed on the PCB; this unusual configuration should be
+    described in a comment. If they are not (meaning that the delay is realized
+    internally in the MAC or PHY), "rgmii-id" is the correct PHY mode.
 
 Commit message
 --------------
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 784912f570e9d..57fcbd4b63ede 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3735,6 +3735,17 @@ sub process {
 			}
 		}
 
+# Check for RGMII phy-mode with delay on PCB
+		if ($realfile =~ /\.dtsi?$/ && $line =~ /^\+\s*(phy-mode|phy-connection-type)\s*=\s*"/ &&
+		    !ctx_has_comment($first_line, $linenr)) {
+			my $prop = $1;
+			my $mode = get_quoted_string($line, $rawline);
+			if ($mode =~ /^"rgmii(?:|-rxid|-txid)"$/) {
+				CHK("UNCOMMENTED_RGMII_MODE",
+				    "$prop $mode without comment -- delays on the PCB should be described, otherwise use \"rgmii-id\"\n" . $herecurr);
+			}
+		}
+
 # check for using SPDX license tag at beginning of files
 		if ($realline == $checklicenseline) {
 			if ($rawline =~ /^[ \+]\s*\#\!\s*\//) {
-- 
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/

Re: [PATCH net-next 1/4] dt-bindings: net: ethernet-controller: update descriptions of RGMII modes

[Siddharth Vadapalli]

On Tue, Apr 15, 2025 at 12:18:01PM +0200, Matthias Schiffer wrote:
> As discussed [1], the comments for the different rgmii(-*id) modes do not
> accurately describe what these values mean.
> 
> As the Device Tree is primarily supposed to describe the hardware and not
> its configuration, the different modes need to distinguish board designs

If the Ethernet-Controller (MAC) is integrated in an SoC (as is the case
with CPSW Ethernet Switch), and, given that "phy-mode" is a property
added within the device-tree node of the MAC, I fail to understand how
the device-tree can continue "describing" hardware for different board
designs using the same SoC (unchanged MAC HW).

How do we handle situations where a given MAC supports various
"phy-modes" in HW? Shouldn't "phy-modes" then be a "list" to technically
descibe the HW? Even if we set aside the "rgmii" variants that this
series is attempting to address, the CPSW MAC supports "sgmii", "qsgmii"
and "usxgmii/xfi" as well.

> (if a delay is built into the PCB using different trace lengths); whether
> a delay is added on the MAC or the PHY side when needed should not matter.
> 
> Unfortunately, implementation in MAC drivers is somewhat inconsistent
> where a delay is fixed or configurable on the MAC side. As a first step
> towards sorting this out, improve the documentation.
> 
> Link: https://lore.kernel.org/lkml/d25b1447-c28b-4998-b238-92672434dc28@lunn.ch/ [1]
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> ---
>  .../bindings/net/ethernet-controller.yaml        | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> index 45819b2358002..2ddc1ce2439a6 100644
> --- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> +++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> @@ -74,19 +74,21 @@ properties:
>        - rev-rmii
>        - moca
>  
> -      # RX and TX delays are added by the MAC when required
> +      # RX and TX delays are part of the board design (through PCB traces). MAC
> +      # and PHY must not add delays.
>        - rgmii
>  
> -      # RGMII with internal RX and TX delays provided by the PHY,
> -      # the MAC should not add the RX or TX delays in this case
> +      # RGMII with internal RX and TX delays provided by the MAC or PHY. No
> +      # delays are included in the board design; this is the most common case
> +      # in modern designs.
>        - rgmii-id
>  
> -      # RGMII with internal RX delay provided by the PHY, the MAC
> -      # should not add an RX delay in this case
> +      # RGMII with internal RX delay provided by the MAC or PHY. TX delay is
> +      # part of the board design.
>        - rgmii-rxid
>  
> -      # RGMII with internal TX delay provided by the PHY, the MAC
> -      # should not add an TX delay in this case
> +      # RGMII with internal TX delay provided by the MAC or PHY. RX delay is
> +      # part of the board design.

Since all of the above is documented in "ethernet-controller.yaml" and
not "ethernet-phy.yaml", describing what the "MAC" should or shouldn't
do seems accurate, and modifying it to describe what the "PHY" should or
shouldn't do seems wrong.

>        - rgmii-txid
>        - rtbi
>        - smii

Regards,
Siddharth.

Re: [PATCH net-next 1/4] dt-bindings: net: ethernet-controller: update descriptions of RGMII modes

[Maxime Chevallier]

Hi Matthias,

On Tue, 15 Apr 2025 12:18:01 +0200
Matthias Schiffer <matthias.schiffer@ew.tq-group.com> wrote:

> As discussed [1], the comments for the different rgmii(-*id) modes do not
> accurately describe what these values mean.
> 
> As the Device Tree is primarily supposed to describe the hardware and not
> its configuration, the different modes need to distinguish board designs
> (if a delay is built into the PCB using different trace lengths); whether
> a delay is added on the MAC or the PHY side when needed should not matter.
> 
> Unfortunately, implementation in MAC drivers is somewhat inconsistent
> where a delay is fixed or configurable on the MAC side. As a first step
> towards sorting this out, improve the documentation.
> 
> Link: https://lore.kernel.org/lkml/d25b1447-c28b-4998-b238-92672434dc28@lunn.ch/ [1]
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>

Thanks for doing that ! I've tried to document that as well in the
past but it didn't go anywhere and was more clumsy. I think your wording
is clear and helpful.

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime

Re: [PATCH net-next 2/4] dt-bindings: net: ti: k3-am654-cpsw-nuss: update phy-mode in example

[Maxime Chevallier]

On Tue, 15 Apr 2025 12:18:02 +0200
Matthias Schiffer <matthias.schiffer@ew.tq-group.com> wrote:

> k3-am65-cpsw-nuss controllers have a fixed internal TX delay, so RXID
> mode is not actually possible and will result in a warning from the
> driver going forward.
> 
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime

Re: [PATCH net-next 3/4] net: ethernet: ti: am65-cpsw: fixup PHY mode for fixed RGMII TX delay

[Maxime Chevallier]

On Tue, 15 Apr 2025 12:18:03 +0200
Matthias Schiffer <matthias.schiffer@ew.tq-group.com> 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>

This looks good to me,

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime

Re: [PATCH net-next 4/4] checkpatch: check for comment explaining rgmii(|-rxid|-txid) PHY modes

[Maxime Chevallier]

On Tue, 15 Apr 2025 12:18:04 +0200
Matthias Schiffer <matthias.schiffer@ew.tq-group.com> wrote:

> Historially, the RGMII PHY modes specified in Device Trees have been
  ^^^^^^^^^^^
  Historically
> used inconsistently, often referring to the usage of delays on the PHY
> side rather than describing the board; many drivers still implement this
> incorrectly.
> 
> Require a comment in Devices Trees using these modes (usually mentioning
> that the delay is relalized on the PCB), so we can avoid adding more
> incorrect uses (or will at least notice which drivers still need to be
> fixed).
> 
> Suggested-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> ---
>  Documentation/dev-tools/checkpatch.rst |  9 +++++++++
>  scripts/checkpatch.pl                  | 11 +++++++++++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
> index abb3ff6820766..8692d3bc155f1 100644
> --- a/Documentation/dev-tools/checkpatch.rst
> +++ b/Documentation/dev-tools/checkpatch.rst
> @@ -513,6 +513,15 @@ Comments
>  
>      See: https://lore.kernel.org/lkml/20131006222342.GT19510@leaf/
>  
> +  **UNCOMMENTED_RGMII_MODE**
> +    Historially, the RGMII PHY modes specified in Device Trees have been
       ^^^^^^^^^^^
      	 Historically
> +    used inconsistently, often referring to the usage of delays on the PHY
> +    side rather than describing the board.
> +
> +    PHY modes "rgmii", "rgmii-rxid" and "rgmii-txid" modes require the clock
> +    signal to be delayed on the PCB; this unusual configuration should be
> +    described in a comment. If they are not (meaning that the delay is realized
> +    internally in the MAC or PHY), "rgmii-id" is the correct PHY mode.
>  
>  Commit message
>  --------------
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 784912f570e9d..57fcbd4b63ede 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3735,6 +3735,17 @@ sub process {
>  			}
>  		}
>  
> +# Check for RGMII phy-mode with delay on PCB
> +		if ($realfile =~ /\.dtsi?$/ && $line =~ /^\+\s*(phy-mode|phy-connection-type)\s*=\s*"/ &&
> +		    !ctx_has_comment($first_line, $linenr)) {
> +			my $prop = $1;
> +			my $mode = get_quoted_string($line, $rawline);
> +			if ($mode =~ /^"rgmii(?:|-rxid|-txid)"$/) {
> +				CHK("UNCOMMENTED_RGMII_MODE",
> +				    "$prop $mode without comment -- delays on the PCB should be described, otherwise use \"rgmii-id\"\n" . $herecurr);
> +			}
> +		}
> +

My Perl-fu isn't good enough for me to review this properly... I think
though that Andrew mentioned something along the lines of 'Comment
should include PCB somewhere', but I don't know if this is easily
doable with checkpatch though.

Maxime

Re: [PATCH net-next 4/4] checkpatch: check for comment explaining rgmii(|-rxid|-txid) PHY modes

[Matthias Schiffer]

On Tue, 2025-04-15 at 13:15 +0200, Maxime Chevallier wrote:
> On Tue, 15 Apr 2025 12:18:04 +0200
> Matthias Schiffer <matthias.schiffer@ew.tq-group.com> wrote:
> 
> > Historially, the RGMII PHY modes specified in Device Trees have been
>   ^^^^^^^^^^^
>   Historically
> > used inconsistently, often referring to the usage of delays on the PHY
> > side rather than describing the board; many drivers still implement this
> > incorrectly.
> > 
> > Require a comment in Devices Trees using these modes (usually mentioning
> > that the delay is relalized on the PCB), so we can avoid adding more
> > incorrect uses (or will at least notice which drivers still need to be
> > fixed).
> > 
> > Suggested-by: Andrew Lunn <andrew@lunn.ch>
> > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> > ---
> >  Documentation/dev-tools/checkpatch.rst |  9 +++++++++
> >  scripts/checkpatch.pl                  | 11 +++++++++++
> >  2 files changed, 20 insertions(+)
> > 
> > diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
> > index abb3ff6820766..8692d3bc155f1 100644
> > --- a/Documentation/dev-tools/checkpatch.rst
> > +++ b/Documentation/dev-tools/checkpatch.rst
> > @@ -513,6 +513,15 @@ Comments
> >  
> >      See: https://lore.kernel.org/lkml/20131006222342.GT19510@leaf/
> >  
> > +  **UNCOMMENTED_RGMII_MODE**
> > +    Historially, the RGMII PHY modes specified in Device Trees have been
>        ^^^^^^^^^^^
>       	 Historically
> > +    used inconsistently, often referring to the usage of delays on the PHY
> > +    side rather than describing the board.
> > +
> > +    PHY modes "rgmii", "rgmii-rxid" and "rgmii-txid" modes require the clock
> > +    signal to be delayed on the PCB; this unusual configuration should be
> > +    described in a comment. If they are not (meaning that the delay is realized
> > +    internally in the MAC or PHY), "rgmii-id" is the correct PHY mode.
> >  
> >  Commit message
> >  --------------
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 784912f570e9d..57fcbd4b63ede 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -3735,6 +3735,17 @@ sub process {
> >  			}
> >  		}
> >  
> > +# Check for RGMII phy-mode with delay on PCB
> > +		if ($realfile =~ /\.dtsi?$/ && $line =~ /^\+\s*(phy-mode|phy-connection-type)\s*=\s*"/ &&
> > +		    !ctx_has_comment($first_line, $linenr)) {
> > +			my $prop = $1;
> > +			my $mode = get_quoted_string($line, $rawline);
> > +			if ($mode =~ /^"rgmii(?:|-rxid|-txid)"$/) {
> > +				CHK("UNCOMMENTED_RGMII_MODE",
> > +				    "$prop $mode without comment -- delays on the PCB should be described, otherwise use \"rgmii-id\"\n" . $herecurr);
> > +			}
> > +		}
> > +
> 
> My Perl-fu isn't good enough for me to review this properly... I think
> though that Andrew mentioned something along the lines of 'Comment
> should include PCB somewhere', but I don't know if this is easily
> doable with checkpatch though.
> 
> Maxime

I think it can be done using ctx_locate_comment instead of ctx_has_comment, but
I decided against it - requiring to have a comment at all should be sufficient
to make people think about the used mode, and a comment with a bad explanation
would hopefully be caught during review.

Thanks,
Matthias



-- 
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/

Re: [PATCH net-next 1/4] dt-bindings: net: ethernet-controller: update descriptions of RGMII modes

[Matthias Schiffer]

On Tue, 2025-04-15 at 16:06 +0530, Siddharth Vadapalli wrote:
> 
> On Tue, Apr 15, 2025 at 12:18:01PM +0200, Matthias Schiffer wrote:
> > As discussed [1], the comments for the different rgmii(-*id) modes do not
> > accurately describe what these values mean.
> > 
> > As the Device Tree is primarily supposed to describe the hardware and not
> > its configuration, the different modes need to distinguish board designs
> 
> If the Ethernet-Controller (MAC) is integrated in an SoC (as is the case
> with CPSW Ethernet Switch), and, given that "phy-mode" is a property
> added within the device-tree node of the MAC, I fail to understand how
> the device-tree can continue "describing" hardware for different board
> designs using the same SoC (unchanged MAC HW).

The setting is part of the MAC node, but it is always set in the board DTS,
together with assigning a PHY to the MAC.

> How do we handle situations where a given MAC supports various
> "phy-modes" in HW? Shouldn't "phy-modes" then be a "list" to technically
> descibe the HW? Even if we set aside the "rgmii" variants that this
> series is attempting to address, the CPSW MAC supports "sgmii", "qsgmii"
> and "usxgmii/xfi" as well.

This is not about PHY mode support of the MAC, but the mode to be used on a
particular board. I would not expect a board to use multiple different
interfaces with a single PHY (and if such cases exist, I consider them out of
scope for this patch series).

> 
> > (if a delay is built into the PCB using different trace lengths); whether
> > a delay is added on the MAC or the PHY side when needed should not matter.
> > 
> > Unfortunately, implementation in MAC drivers is somewhat inconsistent
> > where a delay is fixed or configurable on the MAC side. As a first step
> > towards sorting this out, improve the documentation.
> > 
> > Link: https://lore.kernel.org/lkml/d25b1447-c28b-4998-b238-92672434dc28@lunn.ch/ [1]
> > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> > ---
> >  .../bindings/net/ethernet-controller.yaml        | 16 +++++++++-------
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> > index 45819b2358002..2ddc1ce2439a6 100644
> > --- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> > +++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> > @@ -74,19 +74,21 @@ properties:
> >        - rev-rmii
> >        - moca
> >  
> > -      # RX and TX delays are added by the MAC when required
> > +      # RX and TX delays are part of the board design (through PCB traces). MAC
> > +      # and PHY must not add delays.
> >        - rgmii
> >  
> > -      # RGMII with internal RX and TX delays provided by the PHY,
> > -      # the MAC should not add the RX or TX delays in this case
> > +      # RGMII with internal RX and TX delays provided by the MAC or PHY. No
> > +      # delays are included in the board design; this is the most common case
> > +      # in modern designs.
> >        - rgmii-id
> >  
> > -      # RGMII with internal RX delay provided by the PHY, the MAC
> > -      # should not add an RX delay in this case
> > +      # RGMII with internal RX delay provided by the MAC or PHY. TX delay is
> > +      # part of the board design.
> >        - rgmii-rxid
> >  
> > -      # RGMII with internal TX delay provided by the PHY, the MAC
> > -      # should not add an TX delay in this case
> > +      # RGMII with internal TX delay provided by the MAC or PHY. RX delay is
> > +      # part of the board design.
> 
> Since all of the above is documented in "ethernet-controller.yaml" and
> not "ethernet-phy.yaml", describing what the "MAC" should or shouldn't
> do seems accurate, and modifying it to describe what the "PHY" should or
> shouldn't do seems wrong.

The settings describe the connection between MAC and PHY, thus their explanation
must mention both to make sense. See the linked discussion with Andrew for
details.

Best,
Matthias



> 
> >        - rgmii-txid
> >        - rtbi
> >        - smii
> 
> Regards,
> Siddharth.

-- 
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/

Re: [PATCH net-next 1/4] dt-bindings: net: ethernet-controller: update descriptions of RGMII modes

[Siddharth Vadapalli]

On Tue, Apr 15, 2025 at 01:28:48PM +0200, Matthias Schiffer wrote:
> On Tue, 2025-04-15 at 16:06 +0530, Siddharth Vadapalli wrote:
> > 
> > On Tue, Apr 15, 2025 at 12:18:01PM +0200, Matthias Schiffer wrote:
> > > As discussed [1], the comments for the different rgmii(-*id) modes do not
> > > accurately describe what these values mean.
> > > 
> > > As the Device Tree is primarily supposed to describe the hardware and not
> > > its configuration, the different modes need to distinguish board designs
> > 
> > If the Ethernet-Controller (MAC) is integrated in an SoC (as is the case
> > with CPSW Ethernet Switch), and, given that "phy-mode" is a property
> > added within the device-tree node of the MAC, I fail to understand how
> > the device-tree can continue "describing" hardware for different board
> > designs using the same SoC (unchanged MAC HW).
> 
> The setting is part of the MAC node, but it is always set in the board DTS,
> together with assigning a PHY to the MAC.

The MAC is the same independent of which board it is used in. So are we
really describing the "MAC" or configuring the "MAC"? Isn't it the PHY
along with the PCB lines on a given board that determine how the "MAC"
should be "configured" to make the combination of "MAC" + "PHY"
functional together?

> 
> > How do we handle situations where a given MAC supports various
> > "phy-modes" in HW? Shouldn't "phy-modes" then be a "list" to technically
> > descibe the HW? Even if we set aside the "rgmii" variants that this
> > series is attempting to address, the CPSW MAC supports "sgmii", "qsgmii"
> > and "usxgmii/xfi" as well.
> 
> This is not about PHY mode support of the MAC, but the mode to be used on a
> particular board. I would not expect a board to use multiple different
> interfaces with a single PHY (and if such cases exist, I consider them out of

For a fixed PHY, the MAC will be "configured" to operate in a set of
modes supported by the PHY. The HW description is coming from the PHY
that has been "fixed", and not the MAC. But the "phy-mode" property
resides within the device-tree node of the MAC and not the PHY. So are
we still "describing" the MAC when it is the "PHY" that introduces the
limitation or requires the MAC to be configured for a particular
"phy-mode"?

> scope for this patch series).
> 
> > 
> > > (if a delay is built into the PCB using different trace lengths); whether
> > > a delay is added on the MAC or the PHY side when needed should not matter.
> > > 
> > > Unfortunately, implementation in MAC drivers is somewhat inconsistent
> > > where a delay is fixed or configurable on the MAC side. As a first step
> > > towards sorting this out, improve the documentation.

While this patch is improving the documentation and making it consistent
when it comes to the description of "rgmii" by stating that the "MAC"
shouldn't add a delay, for the remaining cases, as to who adds the delay
and whether or not the MAC should add a delay has been left open.
Existing documentation clarifies what the MAC should do for each case
except "rgmii" which is being fixed by your patch.

> > > 
> > > Link: https://lore.kernel.org/lkml/d25b1447-c28b-4998-b238-92672434dc28@lunn.ch/ [1]
> > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> > > ---
> > >  .../bindings/net/ethernet-controller.yaml        | 16 +++++++++-------
> > >  1 file changed, 9 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> > > index 45819b2358002..2ddc1ce2439a6 100644
> > > --- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> > > +++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> > > @@ -74,19 +74,21 @@ properties:
> > >        - rev-rmii
> > >        - moca
> > >  
> > > -      # RX and TX delays are added by the MAC when required
> > > +      # RX and TX delays are part of the board design (through PCB traces). MAC
> > > +      # and PHY must not add delays.
> > >        - rgmii
> > >  
> > > -      # RGMII with internal RX and TX delays provided by the PHY,
> > > -      # the MAC should not add the RX or TX delays in this case
> > > +      # RGMII with internal RX and TX delays provided by the MAC or PHY. No
> > > +      # delays are included in the board design; this is the most common case
> > > +      # in modern designs.
> > >        - rgmii-id
> > >  
> > > -      # RGMII with internal RX delay provided by the PHY, the MAC
> > > -      # should not add an RX delay in this case
> > > +      # RGMII with internal RX delay provided by the MAC or PHY. TX delay is
> > > +      # part of the board design.
> > >        - rgmii-rxid
> > >  
> > > -      # RGMII with internal TX delay provided by the PHY, the MAC
> > > -      # should not add an TX delay in this case
> > > +      # RGMII with internal TX delay provided by the MAC or PHY. RX delay is
> > > +      # part of the board design.

[...]

Regards,
Siddharth.

Re: [PATCH net-next 4/4] checkpatch: check for comment explaining rgmii(|-rxid|-txid) PHY modes

[Maxime Chevallier]

On Tue, 15 Apr 2025 13:21:25 +0200
Matthias Schiffer <matthias.schiffer@ew.tq-group.com> wrote:

> On Tue, 2025-04-15 at 13:15 +0200, Maxime Chevallier wrote:
> > On Tue, 15 Apr 2025 12:18:04 +0200
> > Matthias Schiffer <matthias.schiffer@ew.tq-group.com> wrote:
> >   
> > > Historially, the RGMII PHY modes specified in Device Trees have been  
> >   ^^^^^^^^^^^
> >   Historically  
> > > used inconsistently, often referring to the usage of delays on the PHY
> > > side rather than describing the board; many drivers still implement this
> > > incorrectly.
> > > 
> > > Require a comment in Devices Trees using these modes (usually mentioning
> > > that the delay is relalized on the PCB), so we can avoid adding more
> > > incorrect uses (or will at least notice which drivers still need to be
> > > fixed).
> > > 
> > > Suggested-by: Andrew Lunn <andrew@lunn.ch>
> > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> > > ---
> > >  Documentation/dev-tools/checkpatch.rst |  9 +++++++++
> > >  scripts/checkpatch.pl                  | 11 +++++++++++
> > >  2 files changed, 20 insertions(+)
> > > 
> > > diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
> > > index abb3ff6820766..8692d3bc155f1 100644
> > > --- a/Documentation/dev-tools/checkpatch.rst
> > > +++ b/Documentation/dev-tools/checkpatch.rst
> > > @@ -513,6 +513,15 @@ Comments
> > >  
> > >      See: https://lore.kernel.org/lkml/20131006222342.GT19510@leaf/
> > >  
> > > +  **UNCOMMENTED_RGMII_MODE**
> > > +    Historially, the RGMII PHY modes specified in Device Trees have been  
> >        ^^^^^^^^^^^
> >       	 Historically  
> > > +    used inconsistently, often referring to the usage of delays on the PHY
> > > +    side rather than describing the board.
> > > +
> > > +    PHY modes "rgmii", "rgmii-rxid" and "rgmii-txid" modes require the clock
> > > +    signal to be delayed on the PCB; this unusual configuration should be
> > > +    described in a comment. If they are not (meaning that the delay is realized
> > > +    internally in the MAC or PHY), "rgmii-id" is the correct PHY mode.
> > >  
> > >  Commit message
> > >  --------------
> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > index 784912f570e9d..57fcbd4b63ede 100755
> > > --- a/scripts/checkpatch.pl
> > > +++ b/scripts/checkpatch.pl
> > > @@ -3735,6 +3735,17 @@ sub process {
> > >  			}
> > >  		}
> > >  
> > > +# Check for RGMII phy-mode with delay on PCB
> > > +		if ($realfile =~ /\.dtsi?$/ && $line =~ /^\+\s*(phy-mode|phy-connection-type)\s*=\s*"/ &&
> > > +		    !ctx_has_comment($first_line, $linenr)) {
> > > +			my $prop = $1;
> > > +			my $mode = get_quoted_string($line, $rawline);
> > > +			if ($mode =~ /^"rgmii(?:|-rxid|-txid)"$/) {
> > > +				CHK("UNCOMMENTED_RGMII_MODE",
> > > +				    "$prop $mode without comment -- delays on the PCB should be described, otherwise use \"rgmii-id\"\n" . $herecurr);
> > > +			}
> > > +		}
> > > +  
> > 
> > My Perl-fu isn't good enough for me to review this properly... I think
> > though that Andrew mentioned something along the lines of 'Comment
> > should include PCB somewhere', but I don't know if this is easily
> > doable with checkpatch though.
> > 
> > Maxime  
> 
> I think it can be done using ctx_locate_comment instead of ctx_has_comment, but
> I decided against it - requiring to have a comment at all should be sufficient
> to make people think about the used mode, and a comment with a bad explanation
> would hopefully be caught during review.

True, and having looked at other stuff in checkpatch, it looks like
there's no other example of rules expecting a specific word in a
comment.

So besides the typo above, I'm OK with this patch :)

Maxime

Re: [PATCH net-next 4/4] checkpatch: check for comment explaining rgmii(|-rxid|-txid) PHY modes

[Andrew Lunn]

> My Perl-fu isn't good enough for me to review this properly... I think
> though that Andrew mentioned something along the lines of 'Comment
> should include PCB somewhere', but I don't know if this is easily
> doable with checkpatch though.

Maybe make it into a patchset, and change a few DTS files to match the
condition. e.g.

arm/boot/dts/nxp/ls/ls1021a-tsn.dts:/* RGMII delays added via PCB traces */
arm/boot/dts/nxp/ls/ls1021a-tsn.dts-&enet2 {
arm/boot/dts/nxp/ls/ls1021a-tsn.dts-    phy-mode = "rgmii";
arm/boot/dts/nxp/ls/ls1021a-tsn.dts-    status = "okay";

This example is not great, but...
arm64/boot/dts/freescale/imx8mp-skov-reva.dtsi-                         phy-mode = "rgmii";
arm64/boot/dts/freescale/imx8mp-skov-reva.dtsi:                         /* 2ns RX delay is implemented on PCB */
arm64/boot/dts/freescale/imx8mp-skov-reva.dtsi-                         tx-internal-delay-ps = <2000>;
arm64/boot/dts/freescale/imx8mp-skov-reva.dtsi-                         rx-internal-delay-ps = <0>;

There is one more i know of somewhere which i cannot find at the
moment which uses rgmii-rxid or rgmii-txid, an has a comment about
delay.

	Andrew

Re: [PATCH net-next 4/4] checkpatch: check for comment explaining rgmii(|-rxid|-txid) PHY modes

[Andrew Lunn]

> +  **UNCOMMENTED_RGMII_MODE**
> +    Historially, the RGMII PHY modes specified in Device Trees have been
> +    used inconsistently, often referring to the usage of delays on the PHY
> +    side rather than describing the board.
> +
> +    PHY modes "rgmii", "rgmii-rxid" and "rgmii-txid" modes require the clock
> +    signal to be delayed on the PCB; this unusual configuration should be
> +    described in a comment. If they are not (meaning that the delay is realized
> +    internally in the MAC or PHY), "rgmii-id" is the correct PHY mode.

It is unclear to me how much ctx_has_comment() will return. Maybe
include an example here of how it should look. I'm assuming:

/* RGMII delays added via PCB traces */
&enet2 {
    phy-mode = "rgmii";
    status = "okay";

fails, but

&enet2 {
    /* RGMII delays added via PCB traces */
    phy-mode = "rgmii";
    status = "okay";

passes?

>  
>  Commit message
>  --------------
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 784912f570e9d..57fcbd4b63ede 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3735,6 +3735,17 @@ sub process {
>  			}
>  		}
>  
> +# Check for RGMII phy-mode with delay on PCB
> +		if ($realfile =~ /\.dtsi?$/ && $line =~ /^\+\s*(phy-mode|phy-connection-type)\s*=\s*"/ &&

I don't grok perl. Is this only looking a dtsi files? .dts files
should also be checked.

Thanks for working on this, it will be very useful.

	Andrew

Re: [PATCH net-next 4/4] checkpatch: check for comment explaining rgmii(|-rxid|-txid) PHY modes

[Matthias Schiffer]

On Tue, 2025-04-15 at 15:20 +0200, Andrew Lunn wrote:
> 
> > +  **UNCOMMENTED_RGMII_MODE**
> > +    Historially, the RGMII PHY modes specified in Device Trees have been
> > +    used inconsistently, often referring to the usage of delays on the PHY
> > +    side rather than describing the board.
> > +
> > +    PHY modes "rgmii", "rgmii-rxid" and "rgmii-txid" modes require the clock
> > +    signal to be delayed on the PCB; this unusual configuration should be
> > +    described in a comment. If they are not (meaning that the delay is realized
> > +    internally in the MAC or PHY), "rgmii-id" is the correct PHY mode.
> 
> It is unclear to me how much ctx_has_comment() will return. Maybe
> include an example here of how it should look. I'm assuming:
> 
> /* RGMII delays added via PCB traces */
> &enet2 {
>     phy-mode = "rgmii";
>     status = "okay";
> 
> fails, but
> 
> &enet2 {
>     /* RGMII delays added via PCB traces */
>     phy-mode = "rgmii";
>     status = "okay";
> 
> passes?

Yes, it works like that. I can't claim to fully understand the checkpatch code
handling comments, but I copied it from other similar checks and tested it on a
few test patches.

One thing to note is that I implemented it as a CHK() and not a WARN() because
that's what is used for other comment checks like DATA_RACE - meaning it will
only trigger with --strict.


> 
> >  
> >  Commit message
> >  --------------
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 784912f570e9d..57fcbd4b63ede 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -3735,6 +3735,17 @@ sub process {
> >  			}
> >  		}
> >  
> > +# Check for RGMII phy-mode with delay on PCB
> > +		if ($realfile =~ /\.dtsi?$/ && $line =~ /^\+\s*(phy-mode|phy-connection-type)\s*=\s*"/ &&
> 
> I don't grok perl. Is this only looking a dtsi files? .dts files
> should also be checked.

It is a regular expression - the ? makes the previous character optional,
matching both .dts and .dtsi files.

Best,
Matthias


> 
> Thanks for working on this, it will be very useful.
> 
> 	Andrew

-- 
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/

Re: [PATCH net-next 4/4] checkpatch: check for comment explaining rgmii(|-rxid|-txid) PHY modes

[Matthias Schiffer]

On Tue, 2025-04-15 at 15:36 +0200, Matthias Schiffer wrote:
> On Tue, 2025-04-15 at 15:20 +0200, Andrew Lunn wrote:
> > 
> > > +  **UNCOMMENTED_RGMII_MODE**
> > > +    Historially, the RGMII PHY modes specified in Device Trees have been
> > > +    used inconsistently, often referring to the usage of delays on the PHY
> > > +    side rather than describing the board.
> > > +
> > > +    PHY modes "rgmii", "rgmii-rxid" and "rgmii-txid" modes require the clock
> > > +    signal to be delayed on the PCB; this unusual configuration should be
> > > +    described in a comment. If they are not (meaning that the delay is realized
> > > +    internally in the MAC or PHY), "rgmii-id" is the correct PHY mode.
> > 
> > It is unclear to me how much ctx_has_comment() will return. Maybe
> > include an example here of how it should look. I'm assuming:
> > 
> > /* RGMII delays added via PCB traces */
> > &enet2 {
> >     phy-mode = "rgmii";
> >     status = "okay";
> > 
> > fails, but
> > 
> > &enet2 {
> >     /* RGMII delays added via PCB traces */
> >     phy-mode = "rgmii";
> >     status = "okay";
> > 
> > passes?
> 
> Yes, it works like that. I can't claim to fully understand the checkpatch code
> handling comments, but I copied it from other similar checks and tested it on a
> few test patches.
> 
> One thing to note is that I implemented it as a CHK() and not a WARN() because
> that's what is used for other comment checks like DATA_RACE - meaning it will
> only trigger with --strict.

Oops, DATA_RACE is actually a WARN(). I must have copied it from some other
comment check that uses CHK(). Let me know which you want me to use.

> 
> 
> > 
> > >  
> > >  Commit message
> > >  --------------
> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > index 784912f570e9d..57fcbd4b63ede 100755
> > > --- a/scripts/checkpatch.pl
> > > +++ b/scripts/checkpatch.pl
> > > @@ -3735,6 +3735,17 @@ sub process {
> > >  			}
> > >  		}
> > >  
> > > +# Check for RGMII phy-mode with delay on PCB
> > > +		if ($realfile =~ /\.dtsi?$/ && $line =~ /^\+\s*(phy-mode|phy-connection-type)\s*=\s*"/ &&
> > 
> > I don't grok perl. Is this only looking a dtsi files? .dts files
> > should also be checked.
> 
> It is a regular expression - the ? makes the previous character optional,
> matching both .dts and .dtsi files.
> 
> Best,
> Matthias
> 
> 
> > 
> > Thanks for working on this, it will be very useful.
> > 
> > 	Andrew
> 

-- 
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/

Re: [PATCH net-next 4/4] checkpatch: check for comment explaining rgmii(|-rxid|-txid) PHY modes

[Joe Perches]

On 2025-04-15 03:18, Matthias Schiffer wrote:
> Historially, the RGMII PHY modes specified in Device Trees have been
> used inconsistently, often referring to the usage of delays on the PHY
> side rather than describing the board; many drivers still implement 
> this
> incorrectly.
> 
> Require a comment in Devices Trees using these modes (usually 
> mentioning
> that the delay is relalized

realized

> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 784912f570e9d..57fcbd4b63ede 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3735,6 +3735,17 @@ sub process {
>  			}
>  		}
> 
> +# Check for RGMII phy-mode with delay on PCB
> +		if ($realfile =~ /\.dtsi?$/ && $line =~ 
> /^\+\s*(phy-mode|phy-connection-type)\s*=\s*"/ &&
> +		    !ctx_has_comment($first_line, $linenr)) {

Not sure where $first_line comes from and unsure if this works on 
patches rather than complete files.

Does it?

Re: [PATCH net-next 1/4] dt-bindings: net: ethernet-controller: update descriptions of RGMII modes

[Matthias Schiffer]

On Tue, 2025-04-15 at 17:25 +0530, Siddharth Vadapalli wrote:
> 
> On Tue, Apr 15, 2025 at 01:28:48PM +0200, Matthias Schiffer wrote:
> > On Tue, 2025-04-15 at 16:06 +0530, Siddharth Vadapalli wrote:
> > > 
> > > On Tue, Apr 15, 2025 at 12:18:01PM +0200, Matthias Schiffer wrote:
> > > > As discussed [1], the comments for the different rgmii(-*id) modes do not
> > > > accurately describe what these values mean.
> > > > 
> > > > As the Device Tree is primarily supposed to describe the hardware and not
> > > > its configuration, the different modes need to distinguish board designs
> > > 
> > > If the Ethernet-Controller (MAC) is integrated in an SoC (as is the case
> > > with CPSW Ethernet Switch), and, given that "phy-mode" is a property
> > > added within the device-tree node of the MAC, I fail to understand how
> > > the device-tree can continue "describing" hardware for different board
> > > designs using the same SoC (unchanged MAC HW).
> > 
> > The setting is part of the MAC node, but it is always set in the board DTS,
> > together with assigning a PHY to the MAC.
> 
> The MAC is the same independent of which board it is used in. So are we
> really describing the "MAC" or configuring the "MAC"? Isn't it the PHY
> along with the PCB lines on a given board that determine how the "MAC"
> should be "configured" to make the combination of "MAC" + "PHY"
> functional together?
> 
> > 
> > > How do we handle situations where a given MAC supports various
> > > "phy-modes" in HW? Shouldn't "phy-modes" then be a "list" to technically
> > > descibe the HW? Even if we set aside the "rgmii" variants that this
> > > series is attempting to address, the CPSW MAC supports "sgmii", "qsgmii"
> > > and "usxgmii/xfi" as well.
> > 
> > This is not about PHY mode support of the MAC, but the mode to be used on a
> > particular board. I would not expect a board to use multiple different
> > interfaces with a single PHY (and if such cases exist, I consider them out of
> 
> For a fixed PHY, the MAC will be "configured" to operate in a set of
> modes supported by the PHY. The HW description is coming from the PHY
> that has been "fixed", and not the MAC. But the "phy-mode" property
> resides within the device-tree node of the MAC and not the PHY. So are
> we still "describing" the MAC when it is the "PHY" that introduces the
> limitation or requires the MAC to be configured for a particular
> "phy-mode"?

The phy-mode property does not describe the MAC, but how MAC and PHY are
connected. The MAC node just happens to be where this information is placed in
the Device Tree (Using graph nodes to describe the connection between MAC and
PHY seems like overkill...)

Also note that (as I understand it) I'm not changing anything, I'm updating the
documentation to reflect what has been the intended behavior already. Please see
the previous discussion with Andrew that I linked, where he convinced me that
this is the correct approach.

> 
> > scope for this patch series).
> > 
> > > 
> > > > (if a delay is built into the PCB using different trace lengths); whether
> > > > a delay is added on the MAC or the PHY side when needed should not matter.
> > > > 
> > > > Unfortunately, implementation in MAC drivers is somewhat inconsistent
> > > > where a delay is fixed or configurable on the MAC side. As a first step
> > > > towards sorting this out, improve the documentation.
> 
> While this patch is improving the documentation and making it consistent
> when it comes to the description of "rgmii" by stating that the "MAC"
> shouldn't add a delay, for the remaining cases, as to who adds the delay
> and whether or not the MAC should add a delay has been left open.
> Existing documentation clarifies what the MAC should do for each case
> except "rgmii" which is being fixed by your patch.

Andrew specifically asked to leave it open in the DT bindings whether MAC or PHY
add the delay, and it might differ between drivers (and different operating
systems using the same Device Tree).

Whether the MAC should add a required delay in cases where it's configurable is
an interesting question - not one of the Device Tree bindings, but of driver
implementation.

On Linux, there currently isn't a way for the MAC driver to query from the PHY
whether it could include the delays itself. My assumption is that most PHYs
either don't have internal delays, or the delays are configurable. If this is
the case, having the MAC add them in internal-delay modes and not adding them on
the PHY side would be the best default (also for PHY-less/fixed-link setups,
which should be handled like a PHY without internal delay capabilities.)

@Andrew, does the above seem correct to you?

Best,
Matthias


> 
> > > > 
> > > > Link: https://lore.kernel.org/lkml/d25b1447-c28b-4998-b238-92672434dc28@lunn.ch/ [1]
> > > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> > > > ---
> > > >  .../bindings/net/ethernet-controller.yaml        | 16 +++++++++-------
> > > >  1 file changed, 9 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> > > > index 45819b2358002..2ddc1ce2439a6 100644
> > > > --- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> > > > +++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> > > > @@ -74,19 +74,21 @@ properties:
> > > >        - rev-rmii
> > > >        - moca
> > > >  
> > > > -      # RX and TX delays are added by the MAC when required
> > > > +      # RX and TX delays are part of the board design (through PCB traces). MAC
> > > > +      # and PHY must not add delays.
> > > >        - rgmii
> > > >  
> > > > -      # RGMII with internal RX and TX delays provided by the PHY,
> > > > -      # the MAC should not add the RX or TX delays in this case
> > > > +      # RGMII with internal RX and TX delays provided by the MAC or PHY. No
> > > > +      # delays are included in the board design; this is the most common case
> > > > +      # in modern designs.
> > > >        - rgmii-id
> > > >  
> > > > -      # RGMII with internal RX delay provided by the PHY, the MAC
> > > > -      # should not add an RX delay in this case
> > > > +      # RGMII with internal RX delay provided by the MAC or PHY. TX delay is
> > > > +      # part of the board design.
> > > >        - rgmii-rxid
> > > >  
> > > > -      # RGMII with internal TX delay provided by the PHY, the MAC
> > > > -      # should not add an TX delay in this case
> > > > +      # RGMII with internal TX delay provided by the MAC or PHY. RX delay is
> > > > +      # part of the board design.
> 
> [...]
> 
> Regards,
> Siddharth.

-- 
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/

Re: [PATCH net-next 4/4] checkpatch: check for comment explaining rgmii(|-rxid|-txid) PHY modes

[Matthias Schiffer]

On Tue, 2025-04-15 at 09:11 -0700, Joe Perches wrote:
> 
> On 2025-04-15 03:18, Matthias Schiffer wrote:
> > Historially, the RGMII PHY modes specified in Device Trees have been
> > used inconsistently, often referring to the usage of delays on the PHY
> > side rather than describing the board; many drivers still implement 
> > this
> > incorrectly.
> > 
> > Require a comment in Devices Trees using these modes (usually 
> > mentioning
> > that the delay is relalized
> 
> realized
> 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 784912f570e9d..57fcbd4b63ede 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -3735,6 +3735,17 @@ sub process {
> >  			}
> >  		}
> > 
> > +# Check for RGMII phy-mode with delay on PCB
> > +		if ($realfile =~ /\.dtsi?$/ && $line =~ 
> > /^\+\s*(phy-mode|phy-connection-type)\s*=\s*"/ &&
> > +		    !ctx_has_comment($first_line, $linenr)) {
> 
> Not sure where $first_line comes from and unsure if this works on 
> patches rather than complete files.
> 
> Does it?

Yes, it works both with patches and full files. I'm using ctx_has_comment() the
same way existing checks do - I think $first_line refers to the start of the
current context for patch files. I have also verified that it only matches on
comments directly above the phy-mode line in question.

Best,
Matthias



-- 
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/

Re: [PATCH net-next 4/4] checkpatch: check for comment explaining rgmii(|-rxid|-txid) PHY modes

[Paolo Abeni]

On 4/15/25 3:37 PM, Matthias Schiffer wrote:
> On Tue, 2025-04-15 at 15:36 +0200, Matthias Schiffer wrote:
>> On Tue, 2025-04-15 at 15:20 +0200, Andrew Lunn wrote:
>>>
>>>> +  **UNCOMMENTED_RGMII_MODE**
>>>> +    Historially, the RGMII PHY modes specified in Device Trees have been
>>>> +    used inconsistently, often referring to the usage of delays on the PHY
>>>> +    side rather than describing the board.
>>>> +
>>>> +    PHY modes "rgmii", "rgmii-rxid" and "rgmii-txid" modes require the clock
>>>> +    signal to be delayed on the PCB; this unusual configuration should be
>>>> +    described in a comment. If they are not (meaning that the delay is realized
>>>> +    internally in the MAC or PHY), "rgmii-id" is the correct PHY mode.
>>>
>>> It is unclear to me how much ctx_has_comment() will return. Maybe
>>> include an example here of how it should look. I'm assuming:
>>>
>>> /* RGMII delays added via PCB traces */
>>> &enet2 {
>>>     phy-mode = "rgmii";
>>>     status = "okay";
>>>
>>> fails, but
>>>
>>> &enet2 {
>>>     /* RGMII delays added via PCB traces */
>>>     phy-mode = "rgmii";
>>>     status = "okay";
>>>
>>> passes?
>>
>> Yes, it works like that. I can't claim to fully understand the checkpatch code
>> handling comments, but I copied it from other similar checks and tested it on a
>> few test patches.
>>
>> One thing to note is that I implemented it as a CHK() and not a WARN() because
>> that's what is used for other comment checks like DATA_RACE - meaning it will
>> only trigger with --strict.
> 
> Oops, DATA_RACE is actually a WARN(). I must have copied it from some other
> comment check that uses CHK(). Let me know which you want me to use.

I think it's better if this will trigger on plain invocation, so that
there are more chances people are going to actually notice/correct the
thing before the actual submission.

Thanks,

Paolo

Re: [PATCH net-next 1/4] dt-bindings: net: ethernet-controller: update descriptions of RGMII modes

[Andrew Lunn]

On Tue, Apr 15, 2025 at 12:18:01PM +0200, Matthias Schiffer wrote:
> As discussed [1], the comments for the different rgmii(-*id) modes do not
> accurately describe what these values mean.
> 
> As the Device Tree is primarily supposed to describe the hardware and not
> its configuration, the different modes need to distinguish board designs
> (if a delay is built into the PCB using different trace lengths); whether
> a delay is added on the MAC or the PHY side when needed should not matter.
> 
> Unfortunately, implementation in MAC drivers is somewhat inconsistent
> where a delay is fixed or configurable on the MAC side. As a first step
> towards sorting this out, improve the documentation.
> 
> Link: https://lore.kernel.org/lkml/d25b1447-c28b-4998-b238-92672434dc28@lunn.ch/ [1]
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> ---
>  .../bindings/net/ethernet-controller.yaml        | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> index 45819b2358002..2ddc1ce2439a6 100644
> --- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> +++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> @@ -74,19 +74,21 @@ properties:
>        - rev-rmii
>        - moca
>  
> -      # RX and TX delays are added by the MAC when required
> +      # RX and TX delays are part of the board design (through PCB traces). MAC
> +      # and PHY must not add delays.
>        - rgmii
>  
> -      # RGMII with internal RX and TX delays provided by the PHY,
> -      # the MAC should not add the RX or TX delays in this case
> +      # RGMII with internal RX and TX delays provided by the MAC or PHY. No
> +      # delays are included in the board design; this is the most common case
> +      # in modern designs.
>        - rgmii-id
>  
> -      # RGMII with internal RX delay provided by the PHY, the MAC
> -      # should not add an RX delay in this case
> +      # RGMII with internal RX delay provided by the MAC or PHY. TX delay is
> +      # part of the board design.
>        - rgmii-rxid
>  
> -      # RGMII with internal TX delay provided by the PHY, the MAC
> -      # should not add an TX delay in this case
> +      # RGMII with internal TX delay provided by the MAC or PHY. RX delay is
> +      # part of the board design.

This looks good to me. There is nothing here which is Linux specific.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 1/4] dt-bindings: net: ethernet-controller: update descriptions of RGMII modes

[Andrew Lunn]

On Tue, Apr 15, 2025 at 04:06:31PM +0530, Siddharth Vadapalli wrote:
> On Tue, Apr 15, 2025 at 12:18:01PM +0200, Matthias Schiffer wrote:
> > As discussed [1], the comments for the different rgmii(-*id) modes do not
> > accurately describe what these values mean.
> > 
> > As the Device Tree is primarily supposed to describe the hardware and not
> > its configuration, the different modes need to distinguish board designs
> 
> If the Ethernet-Controller (MAC) is integrated in an SoC (as is the case
> with CPSW Ethernet Switch), and, given that "phy-mode" is a property
> added within the device-tree node of the MAC, I fail to understand how
> the device-tree can continue "describing" hardware for different board
> designs using the same SoC (unchanged MAC HW).

phy-mode describes the link between the MAC and the PHY. So it either
needs to be in the MAC node, or the PHY node when describing the
board. I don't know the history of why it ended up in the MAC node,
but it did.

> Since all of the above is documented in "ethernet-controller.yaml" and
> not "ethernet-phy.yaml", describing what the "MAC" should or shouldn't
> do seems accurate, and modifying it to describe what the "PHY" should or
> shouldn't do seems wrong.

What we are really saying here is, does the PCB have extra long clock
lines in order to add the delays? If yes, the MAC/PHY pair does
nothing. If no, the MAC/PHY pair needs to add the delay.

DT however says nothing about how the MAC/PHY pair should add the
delay. That would be configuration, and DT should try to not describe
configuration. Hence the binding description needs to be neutral to
MAC and PHY.

Linux has a preference, that the PHY does the delay. However, other
operating systems might have other preferences. And since the binding
should be generic across a range of operating systems, we don't
mention Linux's preference.

	Andrew

Re: [PATCH net-next 2/4] dt-bindings: net: ti: k3-am654-cpsw-nuss: update phy-mode in example

[Andrew Lunn]

On Tue, Apr 15, 2025 at 12:18:02PM +0200, Matthias Schiffer wrote:
> k3-am65-cpsw-nuss controllers have a fixed internal TX delay, so RXID
> mode is not actually possible and will result in a warning from the
> driver going forward.
> 
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> ---
>  .../devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml          | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml b/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml
> index b11894fbaec47..c8128b8ca74fb 100644
> --- a/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml
> +++ b/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml
> @@ -282,7 +282,7 @@ examples:
>                      ti,syscon-efuse = <&mcu_conf 0x200>;
>                      phys = <&phy_gmii_sel 1>;
>  
> -                    phy-mode = "rgmii-rxid";
> +                    phy-mode = "rgmii-id";

It would be good to enforce the phy-modes which are valid, which i
think are:

        case PHY_INTERFACE_MODE_RGMII_ID:
        case PHY_INTERFACE_MODE_RGMII_TXID:
        case PHY_INTERFACE_MODE_RMII:
        case PHY_INTERFACE_MODE_QSGMII:
        case PHY_INTERFACE_MODE_SGMII:
        case PHY_INTERFACE_MODE_USXGMII:

Anyway, this can be a follow up patch, it should not block this
patchset.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 3/4] net: ethernet: ti: am65-cpsw: fixup PHY mode for fixed RGMII TX delay

[Andrew Lunn]

On Tue, Apr 15, 2025 at 12:18:03PM +0200, 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: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 1/4] dt-bindings: net: ethernet-controller: update descriptions of RGMII modes

[Rob Herring (Arm)]

On Tue, 15 Apr 2025 12:18:01 +0200, Matthias Schiffer wrote:
> As discussed [1], the comments for the different rgmii(-*id) modes do not
> accurately describe what these values mean.
> 
> As the Device Tree is primarily supposed to describe the hardware and not
> its configuration, the different modes need to distinguish board designs
> (if a delay is built into the PCB using different trace lengths); whether
> a delay is added on the MAC or the PHY side when needed should not matter.
> 
> Unfortunately, implementation in MAC drivers is somewhat inconsistent
> where a delay is fixed or configurable on the MAC side. As a first step
> towards sorting this out, improve the documentation.
> 
> Link: https://lore.kernel.org/lkml/d25b1447-c28b-4998-b238-92672434dc28@lunn.ch/ [1]
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> ---
>  .../bindings/net/ethernet-controller.yaml        | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 

Acked-by: Rob Herring (Arm) <robh@kernel.org>

Re: [PATCH net-next 2/4] dt-bindings: net: ti: k3-am654-cpsw-nuss: update phy-mode in example

[Rob Herring (Arm)]

On Tue, 15 Apr 2025 12:18:02 +0200, Matthias Schiffer wrote:
> k3-am65-cpsw-nuss controllers have a fixed internal TX delay, so RXID
> mode is not actually possible and will result in a warning from the
> driver going forward.
> 
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> ---
>  .../devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml          | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Acked-by: Rob Herring (Arm) <robh@kernel.org>

Re: [PATCH net-next 1/4] dt-bindings: net: ethernet-controller: update descriptions of RGMII modes

[Russell King (Oracle)]

On Tue, Apr 15, 2025 at 12:18:01PM +0200, Matthias Schiffer wrote:
> diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> index 45819b2358002..2ddc1ce2439a6 100644
> --- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> +++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> @@ -74,19 +74,21 @@ properties:
>        - rev-rmii
>        - moca
>  
> -      # RX and TX delays are added by the MAC when required
> +      # RX and TX delays are part of the board design (through PCB traces). MAC
> +      # and PHY must not add delays.
>        - rgmii
>  
> -      # RGMII with internal RX and TX delays provided by the PHY,
> -      # the MAC should not add the RX or TX delays in this case
> +      # RGMII with internal RX and TX delays provided by the MAC or PHY. No
> +      # delays are included in the board design; this is the most common case
> +      # in modern designs.
>        - rgmii-id
>  
> -      # RGMII with internal RX delay provided by the PHY, the MAC
> -      # should not add an RX delay in this case
> +      # RGMII with internal RX delay provided by the MAC or PHY. TX delay is
> +      # part of the board design.
>        - rgmii-rxid
>  
> -      # RGMII with internal TX delay provided by the PHY, the MAC
> -      # should not add an TX delay in this case
> +      # RGMII with internal TX delay provided by the MAC or PHY. RX delay is
> +      # part of the board design.
>        - rgmii-txid
>        - rtbi
>        - smii

Sorry, but I don't think this wording improves the situation - in fact,
I think it makes the whole thing way more confusing.

Scenario 1: I'm a network device driver author. I read the above, Okay,
I have a RGMII interface, but I need delays to be added. I'll detect
when RGMII-ID is used, and cause the MAC driver to add the delays, but
still pass PHY_INTERFACE_MODE_RGMII_ID to phylib.

Scenario 2: I'm writing a DT file for a board. Hmm, so if I specify
rgmii because the delays are implemented in the traces, but I need to
fine-tune them. However, the documentation says that delays must not
be added by the MAC or the PHY so how do I adjust them. I know, I'll
use rgmii-id because that allows delays!

I suspect neither of these two are really want you mean, but given
this wording, that's exactly where it leads - which is more
confusion and less proper understanding.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 1/4] dt-bindings: net: ethernet-controller: update descriptions of RGMII modes

[Russell King (Oracle)]

On Tue, Apr 15, 2025 at 04:06:31PM +0530, Siddharth Vadapalli wrote:
> On Tue, Apr 15, 2025 at 12:18:01PM +0200, Matthias Schiffer wrote:
> > As discussed [1], the comments for the different rgmii(-*id) modes do not
> > accurately describe what these values mean.
> > 
> > As the Device Tree is primarily supposed to describe the hardware and not
> > its configuration, the different modes need to distinguish board designs
> 
> If the Ethernet-Controller (MAC) is integrated in an SoC (as is the case
> with CPSW Ethernet Switch), and, given that "phy-mode" is a property
> added within the device-tree node of the MAC, I fail to understand how
> the device-tree can continue "describing" hardware for different board
> designs using the same SoC (unchanged MAC HW).
> 
> How do we handle situations where a given MAC supports various
> "phy-modes" in HW? Shouldn't "phy-modes" then be a "list" to technically
> descibe the HW? Even if we set aside the "rgmii" variants that this
> series is attempting to address, the CPSW MAC supports "sgmii", "qsgmii"
> and "usxgmii/xfi" as well.

phy-mode is quite simply the operating mode for the link between the PHY
and the MAC, and depends how the PHY is wired to the MAC.

The list of modes that a MAC supports is dependent on its hardware
design and is generally known by the MAC driver without need to specify
it firmware.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 1/4] dt-bindings: net: ethernet-controller: update descriptions of RGMII modes

[Russell King (Oracle)]

On Tue, Apr 15, 2025 at 05:25:23PM +0530, Siddharth Vadapalli wrote:
> For a fixed PHY,

No such thing in reality. The kernel has an obsolete idea of a fixed PHY
which is a software-emulated PHY to represent a fixed link, but that is
basically dead with phylink (there is now no PHY for fixed links under
phylink).

As I stated, "phy-mode" describes how the MAC needs to configure its
PHY facing interface, whether there is or is not a PHY present. One can
argue that it's misnamed, but it's buried in deep DT history going back
decades, and there's a rule that we don't break backwards compatibility.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 1/4] dt-bindings: net: ethernet-controller: update descriptions of RGMII modes

[Russell King (Oracle)]

On Wed, Apr 16, 2025 at 09:41:57AM +0200, Matthias Schiffer wrote:
> Also note that (as I understand it) I'm not changing anything, I'm updating the
> documentation to reflect what has been the intended behavior already. Please see
> the previous discussion with Andrew that I linked, where he convinced me that
> this is the correct approach.

I think you are as I stated in my email yesterday. The use of "MAC or
PHY" in your new descriptions opens avenues for confusion such as the
scenarios that I described in yesterday's email.

> Andrew specifically asked to leave it open in the DT bindings whether MAC
> or PHY add the delay, and it might differ between drivers (and different
> operating systems using the same Device Tree).

I'm hoping that Andrew will read my email form yesterday and reconsider
because to me this is a backwards step - it doesn't solve the problem
with unclear documentation. I believe it makes the problem worse, and
will lead to more bugs and misunderstandings in this area.

> Whether the MAC should add a required delay in cases where it's configurable
> is an interesting question - not one of the Device Tree bindings, but of
> driver implementation.

Where Andrew gets this from are MAC drivers that detect the rgmii-*id
modes, apply the delay at the MAC, and then convert the value passed to
phylib to PHY_INTERFACE_MODE_RGMII. This is a load of additional special
handling in the MAC driver, and I'd say it's "advanced" usage and takes
more time to review. It's open to mistakes without review by those who
know this "trick", and the chances of phylib maintainers being Cc'd on
MAC drivers is pretty low.

So, I don't think it's something we want to be generally encouraging,
but instead the more normal "phy-mode describes the phy_interface_mode_t
that is passed to phylib" and only allow the "advanced" case in
exceptional cases.

> On Linux, there currently isn't a way for the MAC driver to query from the PHY
> whether it could include the delays itself. My assumption is that most PHYs
> either don't have internal delays, or the delays are configurable.

motorcomm, dp83tg720, icplus, marvell, dp 838678, adin, micrel, tja11xx,
vitesse, dp83822, mscc, at803x, microchip_t1, broadcom, dp83869,
intel-xway, realtek all do handle internal delays. I haven't checked
whether there are PHYs that don't - that's harder because we don't know
whether PHYs that don't mention RGMII in the driver actually support
RGMII or not.

> If this is
> the case, having the MAC add them in internal-delay modes and not adding them on
> the PHY side would be the best default (also for PHY-less/fixed-link setups,
> which should be handled like a PHY without internal delay capabilities.)

See my "advanced" use case above. We do have drivers doing that.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 1/4] dt-bindings: net: ethernet-controller: update descriptions of RGMII modes

[Andrew Lunn]

> I'm hoping that Andrew will read my email form yesterday and reconsider
> because to me this is a backwards step

I will get back to that in a minute.

> > On Linux, there currently isn't a way for the MAC driver to query from the PHY
> > whether it could include the delays itself. My assumption is that most PHYs
> > either don't have internal delays, or the delays are configurable.
> 
> motorcomm, dp83tg720, icplus, marvell, dp 838678, adin, micrel, tja11xx,
> vitesse, dp83822, mscc, at803x, microchip_t1, broadcom, dp83869,
> intel-xway, realtek all do handle internal delays. I haven't checked
> whether there are PHYs that don't - that's harder because we don't know
> whether PHYs that don't mention RGMII in the driver actually support
> RGMII or not.

I did look through this once. There are no PHYs with Linux drivers
which support any of the RGMII without supporting all 4 RGMII
modes. So we should just assume all RGMII PHYs can add the delays.

If i remember the history correctly, Renesas built an RDK with a PHY
which did not support RGMII delays. So they where forced to do the
delays in the MAC. But it seems like mainline support for that PHY
never happened.

> 
> > If this is
> > the case, having the MAC add them in internal-delay modes and not adding them on
> > the PHY side would be the best default (also for PHY-less/fixed-link setups,
> > which should be handled like a PHY without internal delay capabilities.)
> 
> See my "advanced" use case above. We do have drivers doing that.

I agree with Russell here, it is the worse default, not the best
default. It makes it different to nearly every other MAC driver. It
needs extra work in the MAC, which most MAC drivers get wrong. They
also tend not to call out they have done it different to every other
MAC driver in Linux, and so it does not get the needed extra review,
and so is broken. I also think there is some 'vendor SDK' mentality
here. Our MAC can do this, our SDK allows it, the Linux driver must
have it and use it. Pretty much all hardware has features which never
get used, but vendors sometimes have issues with just leaving it
unused.

	Andrew

Re: [PATCH net-next 1/4] dt-bindings: net: ethernet-controller: update descriptions of RGMII modes

[Andrew Lunn]

On Mon, Apr 21, 2025 at 08:20:29PM +0100, Russell King (Oracle) wrote:
> On Tue, Apr 15, 2025 at 12:18:01PM +0200, Matthias Schiffer wrote:
> > diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> > index 45819b2358002..2ddc1ce2439a6 100644
> > --- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> > +++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> > @@ -74,19 +74,21 @@ properties:
> >        - rev-rmii
> >        - moca
> >  
> > -      # RX and TX delays are added by the MAC when required
> > +      # RX and TX delays are part of the board design (through PCB traces). MAC
> > +      # and PHY must not add delays.
> >        - rgmii
> >  
> > -      # RGMII with internal RX and TX delays provided by the PHY,
> > -      # the MAC should not add the RX or TX delays in this case
> > +      # RGMII with internal RX and TX delays provided by the MAC or PHY. No
> > +      # delays are included in the board design; this is the most common case
> > +      # in modern designs.
> >        - rgmii-id
> >  
> > -      # RGMII with internal RX delay provided by the PHY, the MAC
> > -      # should not add an RX delay in this case
> > +      # RGMII with internal RX delay provided by the MAC or PHY. TX delay is
> > +      # part of the board design.
> >        - rgmii-rxid
> >  
> > -      # RGMII with internal TX delay provided by the PHY, the MAC
> > -      # should not add an TX delay in this case
> > +      # RGMII with internal TX delay provided by the MAC or PHY. RX delay is
> > +      # part of the board design.
> >        - rgmii-txid
> >        - rtbi
> >        - smii
> 
> Sorry, but I don't think this wording improves the situation - in fact,
> I think it makes the whole thing way more confusing.
> 
> Scenario 1: I'm a network device driver author. I read the above, Okay,
> I have a RGMII interface, but I need delays to be added. I'll detect
> when RGMII-ID is used, and cause the MAC driver to add the delays, but
> still pass PHY_INTERFACE_MODE_RGMII_ID to phylib.
> 
> Scenario 2: I'm writing a DT file for a board. Hmm, so if I specify
> rgmii because the delays are implemented in the traces, but I need to
> fine-tune them. However, the documentation says that delays must not
> be added by the MAC or the PHY so how do I adjust them. I know, I'll
> use rgmii-id because that allows delays!
> 
> I suspect neither of these two are really want you mean, but given
> this wording, that's exactly where it leads - which is more
> confusion and less proper understanding.

These DT documents are supposed to be normative and OS agnostic. I
wounder what the DT Maintainers will say if we add an Informative
section afterwards giving a detailed description of how Linux actually
implements these normative statements? It will need to open with a
clear statement that it is describing Linux behaviour, and other OSes
can implement the normative part in other ways and still be compliant,
but that Linux has seen a lot of broken implementations and so wants
to add Informative information to guide Linux developers.

   Andrew

Re: [PATCH net-next 1/4] dt-bindings: net: ethernet-controller: update descriptions of RGMII modes

[Russell King (Oracle)]

On Tue, Apr 22, 2025 at 05:00:37PM +0200, Andrew Lunn wrote:
> On Mon, Apr 21, 2025 at 08:20:29PM +0100, Russell King (Oracle) wrote:
> > On Tue, Apr 15, 2025 at 12:18:01PM +0200, Matthias Schiffer wrote:
> > > diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> > > index 45819b2358002..2ddc1ce2439a6 100644
> > > --- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> > > +++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> > > @@ -74,19 +74,21 @@ properties:
> > >        - rev-rmii
> > >        - moca
> > >  
> > > -      # RX and TX delays are added by the MAC when required
> > > +      # RX and TX delays are part of the board design (through PCB traces). MAC
> > > +      # and PHY must not add delays.
> > >        - rgmii
> > >  
> > > -      # RGMII with internal RX and TX delays provided by the PHY,
> > > -      # the MAC should not add the RX or TX delays in this case
> > > +      # RGMII with internal RX and TX delays provided by the MAC or PHY. No
> > > +      # delays are included in the board design; this is the most common case
> > > +      # in modern designs.
> > >        - rgmii-id
> > >  
> > > -      # RGMII with internal RX delay provided by the PHY, the MAC
> > > -      # should not add an RX delay in this case
> > > +      # RGMII with internal RX delay provided by the MAC or PHY. TX delay is
> > > +      # part of the board design.
> > >        - rgmii-rxid
> > >  
> > > -      # RGMII with internal TX delay provided by the PHY, the MAC
> > > -      # should not add an TX delay in this case
> > > +      # RGMII with internal TX delay provided by the MAC or PHY. RX delay is
> > > +      # part of the board design.
> > >        - rgmii-txid
> > >        - rtbi
> > >        - smii
> > 
> > Sorry, but I don't think this wording improves the situation - in fact,
> > I think it makes the whole thing way more confusing.
> > 
> > Scenario 1: I'm a network device driver author. I read the above, Okay,
> > I have a RGMII interface, but I need delays to be added. I'll detect
> > when RGMII-ID is used, and cause the MAC driver to add the delays, but
> > still pass PHY_INTERFACE_MODE_RGMII_ID to phylib.
> > 
> > Scenario 2: I'm writing a DT file for a board. Hmm, so if I specify
> > rgmii because the delays are implemented in the traces, but I need to
> > fine-tune them. However, the documentation says that delays must not
> > be added by the MAC or the PHY so how do I adjust them. I know, I'll
> > use rgmii-id because that allows delays!
> > 
> > I suspect neither of these two are really want you mean, but given
> > this wording, that's exactly where it leads - which is more
> > confusion and less proper understanding.
> 
> These DT documents are supposed to be normative and OS agnostic. I
> wounder what the DT Maintainers will say if we add an Informative
> section afterwards giving a detailed description of how Linux actually
> implements these normative statements? It will need to open with a
> clear statement that it is describing Linux behaviour, and other OSes
> can implement the normative part in other ways and still be compliant,
> but that Linux has seen a lot of broken implementations and so wants
> to add Informative information to guide Linux developers.

Well, looking at ePAPR, the only thing that was defined back then was
the presence of a property to describe the interface type between the
ethernet device and PHY. The values were left to the implementation
to decide upon, but with some recommendations.

What that means is that the values to this property are not part of
the DT standard, but are a matter for the implementation.

However, with the yaml stuff, if that is basically becoming "DT
specification" then it needs to be clearly defined what each value
actually means for the system, and not this vague airy-fairy thing
we have now.

We've learnt the hard way in the kernel where that gets us with
the number of back-compat breaking changes we've had to make where
the RGMII delays have been totally wrongly interpreted and leaving
it vague means that other implementations will suffer the same pain.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 1/4] dt-bindings: net: ethernet-controller: update descriptions of RGMII modes

[Matthias Schiffer]

On Tue, 2025-04-22 at 16:31 +0100, Russell King (Oracle) wrote:
> ********************
> Achtung externe E-Mail: Öffnen Sie Anhänge und Links nur, wenn Sie wissen, dass diese aus einer sicheren Quelle stammen und sicher sind. Leiten Sie die E-Mail im Zweifelsfall zur Prüfung an den IT-Helpdesk weiter.
> Attention external email: Open attachments and links only if you know that they are from a secure source and are safe. In doubt forward the email to the IT-Helpdesk to check it.
> ********************
> 
> On Tue, Apr 22, 2025 at 05:00:37PM +0200, Andrew Lunn wrote:
> > On Mon, Apr 21, 2025 at 08:20:29PM +0100, Russell King (Oracle) wrote:
> > > On Tue, Apr 15, 2025 at 12:18:01PM +0200, Matthias Schiffer wrote:
> > > > diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> > > > index 45819b2358002..2ddc1ce2439a6 100644
> > > > --- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> > > > +++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> > > > @@ -74,19 +74,21 @@ properties:
> > > >        - rev-rmii
> > > >        - moca
> > > >  
> > > > -      # RX and TX delays are added by the MAC when required
> > > > +      # RX and TX delays are part of the board design (through PCB traces). MAC
> > > > +      # and PHY must not add delays.
> > > >        - rgmii
> > > >  
> > > > -      # RGMII with internal RX and TX delays provided by the PHY,
> > > > -      # the MAC should not add the RX or TX delays in this case
> > > > +      # RGMII with internal RX and TX delays provided by the MAC or PHY. No
> > > > +      # delays are included in the board design; this is the most common case
> > > > +      # in modern designs.
> > > >        - rgmii-id
> > > >  
> > > > -      # RGMII with internal RX delay provided by the PHY, the MAC
> > > > -      # should not add an RX delay in this case
> > > > +      # RGMII with internal RX delay provided by the MAC or PHY. TX delay is
> > > > +      # part of the board design.
> > > >        - rgmii-rxid
> > > >  
> > > > -      # RGMII with internal TX delay provided by the PHY, the MAC
> > > > -      # should not add an TX delay in this case
> > > > +      # RGMII with internal TX delay provided by the MAC or PHY. RX delay is
> > > > +      # part of the board design.
> > > >        - rgmii-txid
> > > >        - rtbi
> > > >        - smii
> > > 
> > > Sorry, but I don't think this wording improves the situation - in fact,
> > > I think it makes the whole thing way more confusing.
> > > 
> > > Scenario 1: I'm a network device driver author. I read the above, Okay,
> > > I have a RGMII interface, but I need delays to be added. I'll detect
> > > when RGMII-ID is used, and cause the MAC driver to add the delays, but
> > > still pass PHY_INTERFACE_MODE_RGMII_ID to phylib.
> > > 
> > > Scenario 2: I'm writing a DT file for a board. Hmm, so if I specify
> > > rgmii because the delays are implemented in the traces, but I need to
> > > fine-tune them. However, the documentation says that delays must not
> > > be added by the MAC or the PHY so how do I adjust them. I know, I'll
> > > use rgmii-id because that allows delays!
> > > 
> > > I suspect neither of these two are really want you mean, but given
> > > this wording, that's exactly where it leads - which is more
> > > confusion and less proper understanding.
> > 
> > These DT documents are supposed to be normative and OS agnostic. I
> > wounder what the DT Maintainers will say if we add an Informative
> > section afterwards giving a detailed description of how Linux actually
> > implements these normative statements? It will need to open with a
> > clear statement that it is describing Linux behaviour, and other OSes
> > can implement the normative part in other ways and still be compliant,
> > but that Linux has seen a lot of broken implementations and so wants
> > to add Informative information to guide Linux developers.
> 
> Well, looking at ePAPR, the only thing that was defined back then was
> the presence of a property to describe the interface type between the
> ethernet device and PHY. The values were left to the implementation
> to decide upon, but with some recommendations.
> 
> What that means is that the values to this property are not part of
> the DT standard, but are a matter for the implementation.
> 
> However, with the yaml stuff, if that is basically becoming "DT
> specification" then it needs to be clearly defined what each value
> actually means for the system, and not this vague airy-fairy thing
> we have now.
> 
> We've learnt the hard way in the kernel where that gets us with
> the number of back-compat breaking changes we've had to make where
> the RGMII delays have been totally wrongly interpreted and leaving
> it vague means that other implementations will suffer the same pain.

I agree with Russell that it seems preferable to make it unambiguous whether
delays are added on the MAC or PHY side, in particular for fine-tuning. If
anything is left to the implementation, we should make the range of acceptable
driver behavior very clear in the documentation.

Historically, there appear to exist at least 4 different ways to handle the
RGMII modes in MAC drivers:

(I'm using the terms "id" and "noid" in the following to refer to modes with and
without delays independently of the direction; a single driver may fall into
different categories for the RX and TX side)

1) MAC ignores the mode, does not add delays, passes the mode to the PHY
  - Mode "noid" is used when delays are added by the PCB
  - PHY sees the same interface mode that is specified in the DT
  - Does not match the old wording of the DT binding docs (as the MAC never adds
    delays)

2) MAC adds delays in "noid" mode, passes the mode to the PHY unchanged
  - Delays added by the PCB can only be supported via driver-specific
    fine-tuning properties on the MAC or PHY side
  - Without driver-specific properties, none of the modes allow for delays on
    the PCB; every mode adds delays either on the MAC or the PHY side
  - PHY sees the same interface mode that is specified in the DT
  - Matches the old wording of the DT binding docs (which don't allow for delays
    added by the PCB)

3) MAC has a fixed delay, but also passes the interface mode unchanged to the
  PHY (example: TX delays in TI CPSW AM65)
  - No support for delays on the PCB due to hardware limitation
  - PHY sees the same interface mode that is specified in the DT
  - For the direction in which the delays are added by the MAC, you need to
    specify "noid" in the DT even though there is an internal delay

4) MAC adds delays in "id" mode and fixes it up so "noid" is passed to
  the PHY (example: TX delays in TI ICSSG)
  - No support for delays on the PCB due to hardware limitation
  - PHY does NOT see the same interface mode that is specified in the DT
  - Fine-tuning options may be confusing because of the fixup

Of all of these variants:
- 1 and 2 appear to be most common in existing MAC drivers
- 2 and maybe 3 match the old wording of the DT binding docs
- 1, 2 and 3 pass the interface mode unchanged to the PHY
- 1 and 4 match my proposed new wording of the DT binding docs based on Andrew's
  input
- 1 allows for designs that have a delay on the PCB without driver-specific
  fine-tuning
- 2 and maybe 3 and 4 allow for designs where the PHY can't add delays and none
  exist on the PCB (or there is no PHY - could be two SoCs directly connected
  via RGMII, or a switch IC we can't control)

And of course, things become even more muddled when driver-specific properties
for fine-tuning etc. are involved.

Any change to existing drivers will need to be made in a backwards-compatible
way, meaning that we can't break compatibility with old Device Trees. If we
somehow want to clean up this mess, and also support delays on the PCB, designs
without (configurable) PHY, and unambiguously specify where delays are added, I
don't think the existing rgmii(-*id) modes are going to suffice. I think
something along the lines of the following might be a possible way forward:

- Introduce new DT properties to specify whether delays should be added on the
MAC or PHY side, for RX and TX independently. Could also replace some driver-
specific fine-tuning properties.
- In the presence of the new properties, only "rgmii" can be specified as phy-
mode, the delays are not part of the PHY mode property anymore
- MAC and PHY drivers must reject delay configurations they can't satisfy
- Not specifying the new properties results in a deprecation warning on existing
drivers. New drivers make the new properties mandatory.
- Not specifying the new properties causes drivers to interpret "rgmii(-*id)"
however they have done in the past to maintain backwards compatibility

Implementation of the new properties would need to be done in a way that allows
MAC and PHY drivers to be converted one by one, and to only print deprecation
warnings asking for the DT to be fixed once the conversion has been done.
Attempting to specify the new properties when not both MAC and PHY driver
support them must be rejected, so the drivers gaining support for them can't
result in a breaking change.

I would be very happy to hear some feedback on these ideas - of course,
alternative ideas that are easier to implement would be even more welcome...

Best,
Matthias


-- 
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/

Re: [PATCH net-next 1/4] dt-bindings: net: ethernet-controller: update descriptions of RGMII modes

[Andrew Lunn]

> > However, with the yaml stuff, if that is basically becoming "DT
> > specification" then it needs to be clearly defined what each value
> > actually means for the system, and not this vague airy-fairy thing
> > we have now.

 
> I agree with Russell that it seems preferable to make it unambiguous whether
> delays are added on the MAC or PHY side, in particular for fine-tuning. If
> anything is left to the implementation, we should make the range of acceptable
> driver behavior very clear in the documentation.

I think we should try the "Informative" route first, see what the DT
Maintainers think when we describe in detail how Linux interprets
these values.

I don't think a whole new set of properties will solve anything. I
would say the core of the problem is that there are multiple ways of
getting a working system, many of which don't fit the DT binding. But
DT developers don't care about that, they are just happy when it
works. Adding a different set of properties won't change that.

	Andrew

Re: [PATCH net-next 1/4] dt-bindings: net: ethernet-controller: update descriptions of RGMII modes

[Siddharth Vadapalli]

On Mon, Apr 28, 2025 at 04:08:10PM +0200, Andrew Lunn wrote:
> > > However, with the yaml stuff, if that is basically becoming "DT
> > > specification" then it needs to be clearly defined what each value
> > > actually means for the system, and not this vague airy-fairy thing
> > > we have now.
> 
>  
> > I agree with Russell that it seems preferable to make it unambiguous whether
> > delays are added on the MAC or PHY side, in particular for fine-tuning. If
> > anything is left to the implementation, we should make the range of acceptable
> > driver behavior very clear in the documentation.
> 
> I think we should try the "Informative" route first, see what the DT
> Maintainers think when we describe in detail how Linux interprets
> these values.
> 
> I don't think a whole new set of properties will solve anything. I
> would say the core of the problem is that there are multiple ways of
> getting a working system, many of which don't fit the DT binding. But
> DT developers don't care about that, they are just happy when it
> works. Adding a different set of properties won't change that.

Isn't the ambiguity arising due to an incomplete description wherein we
are not having an accurate description for the PCB Traces?

A complete description might be something like:

mac {
	pcb-traces {
		mac-to-phy-trace-delay = <X>; // Nanoseconds
		phy-to-mac-trace-delay = <Y>; // Nanoseconds
	};
	phy-mode = "rgmii-*";
	phy-handle = <&phy>;
};

In some designs, the "mac-to-phy-trace" and the "phy-to-mac-trace" are
treated as a part of the MAC block for example. Depending on which block
contains the trace, the delay is added accordingly.

Regards,
Siddharth.

Re: [PATCH net-next 1/4] dt-bindings: net: ethernet-controller: update descriptions of RGMII modes

[Andrew Lunn]

> A complete description might be something like:
> 
> mac {
> 	pcb-traces {
> 		mac-to-phy-trace-delay = <X>; // Nanoseconds
> 		phy-to-mac-trace-delay = <Y>; // Nanoseconds
> 	};

> In some designs, the "mac-to-phy-trace" and the "phy-to-mac-trace" are
> treated as a part of the MAC block for example.

PCB traces cannot be part of the MAC block, since they are copper on
the PCB. In fact, such a consideration just adds to the confusion,
because how do you know which designs do and which don't include the
MAC block?

	Andrew

Re: [PATCH net-next 1/4] dt-bindings: net: ethernet-controller: update descriptions of RGMII modes

[Matthias Schiffer]

On Mon, 2025-04-28 at 16:08 +0200, Andrew Lunn wrote:
> 
> > > However, with the yaml stuff, if that is basically becoming "DT
> > > specification" then it needs to be clearly defined what each value
> > > actually means for the system, and not this vague airy-fairy thing
> > > we have now.
> 
>  
> > I agree with Russell that it seems preferable to make it unambiguous whether
> > delays are added on the MAC or PHY side, in particular for fine-tuning. If
> > anything is left to the implementation, we should make the range of acceptable
> > driver behavior very clear in the documentation.
> 
> I think we should try the "Informative" route first, see what the DT
> Maintainers think when we describe in detail how Linux interprets
> these values.

Oh, we should not be Linux-specific. We should describe in detail how *any OS*
must interpret values.


> 
> I don't think a whole new set of properties will solve anything. I
> would say the core of the problem is that there are multiple ways of
> getting a working system, many of which don't fit the DT binding. But
> DT developers don't care about that, they are just happy when it
> works. Adding a different set of properties won't change that.
> 
> 	Andrew

Hmm, considering that

- interpretation of existing properties is inconsistent
- we could like something with a consistent interpretation
- we can't change how existing drivers interpret the properties, as that would
be a breaking change,

I don't think we really have any options but to introduce something new, or keep
the inconsistent status quo.

Best,
Matthias



-- 
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/

Re: [PATCH net-next 1/4] dt-bindings: net: ethernet-controller: update descriptions of RGMII modes

[Andrew Lunn]

On Tue, Apr 29, 2025 at 09:24:49AM +0200, Matthias Schiffer wrote:
> On Mon, 2025-04-28 at 16:08 +0200, Andrew Lunn wrote:
> > 
> > > > However, with the yaml stuff, if that is basically becoming "DT
> > > > specification" then it needs to be clearly defined what each value
> > > > actually means for the system, and not this vague airy-fairy thing
> > > > we have now.
> > 
> >  
> > > I agree with Russell that it seems preferable to make it unambiguous whether
> > > delays are added on the MAC or PHY side, in particular for fine-tuning. If
> > > anything is left to the implementation, we should make the range of acceptable
> > > driver behavior very clear in the documentation.
> > 
> > I think we should try the "Informative" route first, see what the DT
> > Maintainers think when we describe in detail how Linux interprets
> > these values.
> 
> Oh, we should not be Linux-specific. We should describe in detail how *any OS*
> must interpret values.

There is two things here. One is related to delays on the PCB. Those
are OS agnostic and clearly you are describing hardware. But once you
get to implementing the delay in the MAC or the PHY, it is policy if
the PHY does it, or the MAC does it. Different OSes can have different
policy. We cannot force other OSes to do the same as Linux.
 
I drafted some text last night. I need to review it because i often
make typos, and then i will post it.

	Andrew

Re: [PATCH net-next 1/4] dt-bindings: net: ethernet-controller: update descriptions of RGMII modes

[Matthias Schiffer]

On Tue, 2025-04-29 at 14:08 +0200, Andrew Lunn wrote:
> On Tue, Apr 29, 2025 at 09:24:49AM +0200, Matthias Schiffer wrote:
> > On Mon, 2025-04-28 at 16:08 +0200, Andrew Lunn wrote:
> > > 
> > > > > However, with the yaml stuff, if that is basically becoming "DT
> > > > > specification" then it needs to be clearly defined what each value
> > > > > actually means for the system, and not this vague airy-fairy thing
> > > > > we have now.
> > > 
> > >  
> > > > I agree with Russell that it seems preferable to make it unambiguous whether
> > > > delays are added on the MAC or PHY side, in particular for fine-tuning. If
> > > > anything is left to the implementation, we should make the range of acceptable
> > > > driver behavior very clear in the documentation.
> > > 
> > > I think we should try the "Informative" route first, see what the DT
> > > Maintainers think when we describe in detail how Linux interprets
> > > these values.
> > 
> > Oh, we should not be Linux-specific. We should describe in detail how *any OS*
> > must interpret values.
> 
> There is two things here. One is related to delays on the PCB. Those
> are OS agnostic and clearly you are describing hardware. But once you
> get to implementing the delay in the MAC or the PHY, it is policy if
> the PHY does it, or the MAC does it. Different OSes can have different
> policy. We cannot force other OSes to do the same as Linux.

If we want to support fine-tuning properties and other driver-specific
attributes that rely on the specific delay mode used on the MAC or PHY side, we
must make this policy a part of the binding docs.

Also, we make decisions how DT bindings work in Linux all the time, and other OS
must implement them in the same way to be compatible with Device Trees using
these bindings. I don't see how in this case we suddenly can't make such a
decision.

>  
> I drafted some text last night. I need to review it because i often
> make typos, and then i will post it.

Thanks, I'll give it a read later.

Best,
Matthias

-- 
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/

Re: [PATCH net-next 2/4] dt-bindings: net: ti: k3-am654-cpsw-nuss: update phy-mode in example

[Roger Quadros]

Hi Matthias,

On 15/04/2025 13:18, Matthias Schiffer wrote:
> k3-am65-cpsw-nuss controllers have a fixed internal TX delay, so RXID
> mode is not actually possible and will result in a warning from the
> driver going forward.
> 
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> ---
>  .../devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml          | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml b/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml
> index b11894fbaec47..c8128b8ca74fb 100644
> --- a/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml
> +++ b/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml
> @@ -282,7 +282,7 @@ examples:
>                      ti,syscon-efuse = <&mcu_conf 0x200>;
>                      phys = <&phy_gmii_sel 1>;
>  
> -                    phy-mode = "rgmii-rxid";
> +                    phy-mode = "rgmii-id";
>                      phy-handle = <&phy0>;
>                  };
>              };

FYI the following TI boards using this driver are using "rgmii-rxid".
Will you be sending fixes to the device trees files?

arch/arm64/boot/dts/ti
k3-am625-beagleplay.dts:	phy-mode = "rgmii-rxid";
k3-am625-sk.dts:	phy-mode = "rgmii-rxid";
k3-am625-sk.dts.orig:	phy-mode = "rgmii-rxid";
k3-am62a7-sk.dts:	phy-mode = "rgmii-rxid";
k3-am62a-phycore-som.dtsi:	phy-mode = "rgmii-rxid";
k3-am62p5-sk.dts:	phy-mode = "rgmii-rxid";
k3-am62p5-sk.dts:	phy-mode = "rgmii-rxid";
k3-am62-phycore-som.dtsi:	phy-mode = "rgmii-rxid";
k3-am62-verdin-dev.dtsi:	phy-mode = "rgmii-rxid";
k3-am62-verdin.dtsi:	phy-mode = "rgmii-rxid";
k3-am62-verdin-ivy.dtsi:	phy-mode = "rgmii-rxid";
k3-am62x-phyboard-lyra.dtsi:	phy-mode = "rgmii-rxid";
k3-am62x-sk-common.dtsi:	phy-mode = "rgmii-rxid";
k3-am642-evm.dts:	phy-mode = "rgmii-rxid";
k3-am642-evm.dts:	phy-mode = "rgmii-rxid";
k3-am642-sk.dts:	phy-mode = "rgmii-rxid";
k3-am642-sk.dts:	phy-mode = "rgmii-rxid";
k3-am642-tqma64xxl-mbax4xxl.dts:	phy-mode = "rgmii-rxid";
k3-am642-tqma64xxl-mbax4xxl.dts:	/* phy-mode is fixed up to rgmii-rxid by prueth driver to account for
k3-am64-phycore-som.dtsi:	phy-mode = "rgmii-rxid";
k3-am654-base-board.dts:	phy-mode = "rgmii-rxid";
k3-am67a-beagley-ai.dts:	phy-mode = "rgmii-rxid";
k3-am68-sk-base-board.dts:	phy-mode = "rgmii-rxid";
k3-am69-sk.dts:	phy-mode = "rgmii-rxid";
k3-j7200-common-proc-board.dts:	phy-mode = "rgmii-rxid";
k3-j721e-beagleboneai64.dts:	phy-mode = "rgmii-rxid";
k3-j721e-common-proc-board.dts:	phy-mode = "rgmii-rxid";
k3-j721e-evm-gesi-exp-board.dtso:	phy-mode = "rgmii-rxid";
k3-j721e-evm-gesi-exp-board.dtso:	phy-mode = "rgmii-rxid";
k3-j721e-evm-gesi-exp-board.dtso:	phy-mode = "rgmii-rxid";
k3-j721e-evm-gesi-exp-board.dtso:	phy-mode = "rgmii-rxid";
k3-j721e-sk.dts:	phy-mode = "rgmii-rxid";
k3-j721s2-common-proc-board.dts:	phy-mode = "rgmii-rxid";
k3-j721s2-evm-gesi-exp-board.dtso:	phy-mode = "rgmii-rxid";
k3-j722s-evm.dts:	phy-mode = "rgmii-rxid";
k3-j784s4-j742s2-evm-common.dtsi:	phy-mode = "rgmii-rxid";
k3-j784s4-j742s2-evm-common.dtsi:	phy-mode = "rgmii-rxid";

-- 
cheers,
-roger

Re: [PATCH net-next 3/4] net: ethernet: ti: am65-cpsw: fixup PHY mode for fixed RGMII TX delay

[Roger Quadros]

Matthias,

On 15/04/2025 13:18, 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
Could you please help me understand this statement? Which delay? TX or RX?

Isn't this patch forcing the device tree to have TX delay mentioned in it?

> to update their Device Trees if one of the unsupported modes is specified.
> 
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> ---
>  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 c9fd34787c998..a1d32735c7512 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 = 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;
>  

-- 
cheers,
-roger

Re: [PATCH net-next 3/4] net: ethernet: ti: am65-cpsw: fixup PHY mode for fixed RGMII TX delay

[Andrew Lunn]

On Wed, Apr 30, 2025 at 05:56:29PM +0300, Roger Quadros wrote:
> Matthias,
> 
> On 15/04/2025 13:18, 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
> Could you please help me understand this statement? Which delay? TX or RX?

See if this helper:

https://patchwork.kernel.org/project/netdevbpf/patch/20250429-v6-15-rc3-net-rgmii-delays-v1-1-f52664945741@lunn.ch/

There will be a V2 soon.

      Andrew

Re: [PATCH net-next 2/4] dt-bindings: net: ti: k3-am654-cpsw-nuss: update phy-mode in example

[Matthias Schiffer]

On Wed, 2025-04-30 at 17:22 +0300, Roger Quadros wrote:
> 
> Hi Matthias,
> 
> On 15/04/2025 13:18, Matthias Schiffer wrote:
> > k3-am65-cpsw-nuss controllers have a fixed internal TX delay, so RXID
> > mode is not actually possible and will result in a warning from the
> > driver going forward.
> > 
> > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> > ---
> >  .../devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml          | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml b/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml
> > index b11894fbaec47..c8128b8ca74fb 100644
> > --- a/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml
> > +++ b/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml
> > @@ -282,7 +282,7 @@ examples:
> >                      ti,syscon-efuse = <&mcu_conf 0x200>;
> >                      phys = <&phy_gmii_sel 1>;
> >  
> > -                    phy-mode = "rgmii-rxid";
> > +                    phy-mode = "rgmii-id";
> >                      phy-handle = <&phy0>;
> >                  };
> >              };
> 
> FYI the following TI boards using this driver are using "rgmii-rxid".
> Will you be sending fixes to the device trees files?


Hi Roger,

as written in the cover letter, I haven't fixed any DTS for now, so projects
consuming Linux's Device Tree sources (like U-Boot) have some time to update
their driver first (as fixing the Device Trees without updating the driver would
break Ethernet).

Once a fix has been accepted in U-Boot (and preferably after we've come to an
agreement on the open questions...) I can also send a patch to update these
files.

Best,
Matthias



> 
> arch/arm64/boot/dts/ti
> k3-am625-beagleplay.dts:	phy-mode = "rgmii-rxid";
> k3-am625-sk.dts:	phy-mode = "rgmii-rxid";
> k3-am625-sk.dts.orig:	phy-mode = "rgmii-rxid";
> k3-am62a7-sk.dts:	phy-mode = "rgmii-rxid";
> k3-am62a-phycore-som.dtsi:	phy-mode = "rgmii-rxid";
> k3-am62p5-sk.dts:	phy-mode = "rgmii-rxid";
> k3-am62p5-sk.dts:	phy-mode = "rgmii-rxid";
> k3-am62-phycore-som.dtsi:	phy-mode = "rgmii-rxid";
> k3-am62-verdin-dev.dtsi:	phy-mode = "rgmii-rxid";
> k3-am62-verdin.dtsi:	phy-mode = "rgmii-rxid";
> k3-am62-verdin-ivy.dtsi:	phy-mode = "rgmii-rxid";
> k3-am62x-phyboard-lyra.dtsi:	phy-mode = "rgmii-rxid";
> k3-am62x-sk-common.dtsi:	phy-mode = "rgmii-rxid";
> k3-am642-evm.dts:	phy-mode = "rgmii-rxid";
> k3-am642-evm.dts:	phy-mode = "rgmii-rxid";
> k3-am642-sk.dts:	phy-mode = "rgmii-rxid";
> k3-am642-sk.dts:	phy-mode = "rgmii-rxid";
> k3-am642-tqma64xxl-mbax4xxl.dts:	phy-mode = "rgmii-rxid";
> k3-am642-tqma64xxl-mbax4xxl.dts:	/* phy-mode is fixed up to rgmii-rxid by prueth driver to account for
> k3-am64-phycore-som.dtsi:	phy-mode = "rgmii-rxid";
> k3-am654-base-board.dts:	phy-mode = "rgmii-rxid";
> k3-am67a-beagley-ai.dts:	phy-mode = "rgmii-rxid";
> k3-am68-sk-base-board.dts:	phy-mode = "rgmii-rxid";
> k3-am69-sk.dts:	phy-mode = "rgmii-rxid";
> k3-j7200-common-proc-board.dts:	phy-mode = "rgmii-rxid";
> k3-j721e-beagleboneai64.dts:	phy-mode = "rgmii-rxid";
> k3-j721e-common-proc-board.dts:	phy-mode = "rgmii-rxid";
> k3-j721e-evm-gesi-exp-board.dtso:	phy-mode = "rgmii-rxid";
> k3-j721e-evm-gesi-exp-board.dtso:	phy-mode = "rgmii-rxid";
> k3-j721e-evm-gesi-exp-board.dtso:	phy-mode = "rgmii-rxid";
> k3-j721e-evm-gesi-exp-board.dtso:	phy-mode = "rgmii-rxid";
> k3-j721e-sk.dts:	phy-mode = "rgmii-rxid";
> k3-j721s2-common-proc-board.dts:	phy-mode = "rgmii-rxid";
> k3-j721s2-evm-gesi-exp-board.dtso:	phy-mode = "rgmii-rxid";
> k3-j722s-evm.dts:	phy-mode = "rgmii-rxid";
> k3-j784s4-j742s2-evm-common.dtsi:	phy-mode = "rgmii-rxid";
> k3-j784s4-j742s2-evm-common.dtsi:	phy-mode = "rgmii-rxid";
> 

-- 
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/

Re: [PATCH net-next 4/4] checkpatch: check for comment explaining rgmii(|-rxid|-txid) PHY modes

[Matthias Schiffer]

On Tue, 2025-04-15 at 15:12 +0200, Andrew Lunn wrote:
> 
> > My Perl-fu isn't good enough for me to review this properly... I think
> > though that Andrew mentioned something along the lines of 'Comment
> > should include PCB somewhere', but I don't know if this is easily
> > doable with checkpatch though.
> 
> Maybe make it into a patchset, and change a few DTS files to match the
> condition. e.g.
> 
> arm/boot/dts/nxp/ls/ls1021a-tsn.dts:/* RGMII delays added via PCB traces */
> arm/boot/dts/nxp/ls/ls1021a-tsn.dts-&enet2 {
> arm/boot/dts/nxp/ls/ls1021a-tsn.dts-    phy-mode = "rgmii";
> arm/boot/dts/nxp/ls/ls1021a-tsn.dts-    status = "okay";
> 
> This example is not great, but...
> arm64/boot/dts/freescale/imx8mp-skov-reva.dtsi-                         phy-mode = "rgmii";
> arm64/boot/dts/freescale/imx8mp-skov-reva.dtsi:                         /* 2ns RX delay is implemented on PCB */
> arm64/boot/dts/freescale/imx8mp-skov-reva.dtsi-                         tx-internal-delay-ps = <2000>;
> arm64/boot/dts/freescale/imx8mp-skov-reva.dtsi-                         rx-internal-delay-ps = <0>;
> 
> There is one more i know of somewhere which i cannot find at the
> moment which uses rgmii-rxid or rgmii-txid, an has a comment about
> delay.
> 
> 	Andrew

FWIW, I've decided against including the comment changes in this series for now
(going to send a v2 in a moment), as non-functional changes just to improve
style or make checkpatch happy are often frowned upon as unnecessary churn.

Best,
Matthias



-- 
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/