| Message ID | 16a08c72ec6cf68bbe55b82d6fb2f12879941f16.1744710099.git.matthias.schiffer@ew.tq-group.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | RGMII mode clarification + am65-cpsw fix | expand |
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
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
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
> 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
> + **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
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
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 >
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?
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
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*\//) {
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(+)