| Message ID | cover.1744710099.git.matthias.schiffer@ew.tq-group.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | RGMII mode clarification + am65-cpsw fix | expand |
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.
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.
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.
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(-)