Skip to contentSkip to conversation

arm64: dts: sun50i: h5: NanoPI Neo 2: phy-mode rgmii-id

PATCH In review Clément Bœsch 4 participants 13 messages Aug 30, 2021 – Sep 6, 2021

Subsystems (2)
Device tree bindings Arm/Allwinner sunXi SoC support
lore
Share & tools
lore

Clément Bœsch

author
#0 lore raw

PATCH 1/1 arm64: dts: sun50i: h5: NanoPI Neo 2: phy-mode rgmii-id

Since commit bbc4d71d6354 ("net: phy: realtek: fix rtl8211e rx/tx delay config") network is broken on the NanoPi Neo 2.

This patch changes the phy-mode to use internal delays both for RX and TX as has been done for other boards affected by the same commit.

Fixes bbc4d71d6354 ("net: phy: realtek: fix rtl8211e rx/tx delay config")
Signed-off-by Clément Bœsch <[email protected]>
---
 arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo2.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo2.dts +1 -1
index 02f8e72f0cad..05486cccee1c 100644
@@ -75,7 +75,7 @@ &emac {
 	pinctrl-0 = <&emac_rgmii_pins>;
 	phy-supply = <&reg_gmac_3v3>;
 	phy-handle = <&ext_rgmii_phy>;
-	phy-mode = "rgmii";
+	phy-mode = "rgmii-id";
 	status = "okay";
 };
Signature · 1 line
2.33.0
maintainer Reviewed-by on v1 1/1
#1 lore raw

Hi!

Dne ponedeljek, 30. avgust 2021 ob 17:16:45 CEST je Clément Bœsch napisal(a):

Quoted 7 lines

Since commit bbc4d71d6354 ("net: phy: realtek: fix rtl8211e rx/tx delay config") network is broken on the NanoPi Neo 2.

This patch changes the phy-mode to use internal delays both for RX and TX as has been done for other boards affected by the same commit.

Fixes bbc4d71d6354 ("net: phy: realtek: fix rtl8211e rx/tx delay config")

This commit fixes DT issue, so "fixes" tag should be:

Fixes 44a94c7ef989 ("arm64: dts: allwinner: H5: Restore EMAC changes")

Here, a node with wrong phy-mode property was added to NanoPi Neo 2 board DT.

Other than that, this patch is fine and once fixes tag is fixed, you can add:

Reviewed-by Jernej Skrabec <[email protected]>

For next version, you should:
- change fixed tag
- add my review-by tag right above your signed-off-by tag
- mark patch as v2 (add "-v2" parameter to git format-patch)
- describe change right under "---" line

Note, if you borked something when sending, you should mark patch or patch series as "RESEND", so recipients don't look for changes in two subsequent e- mails (--subject-prefix="RESEND PATCH").

Thanks for the fix!

Best regards,
Jernej

Quoted 18 lines
Signed-off-by Clément Bœsch <[email protected]>
---
 arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo2.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
(quoted diff) +0 -0

b/arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo2.dts index 02f8e72f0cad..05486cccee1c 100644

arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo2.dts +1 -1
@@ -75,7 +75,7 @@ &emac {
 	pinctrl-0 = <&emac_rgmii_pins>;
 	phy-supply = <&reg_gmac_3v3>;
 	phy-handle = <&ext_rgmii_phy>;
-	phy-mode = "rgmii";
+	phy-mode = "rgmii-id";
 	status = "okay";
 };
author
#2 lore raw
Quoted 2 lines · Jernej Škrabec

Hi!

Hi,

Quoted 7 lines

Dne ponedeljek, 30. avgust 2021 ob 17:16:45 CEST je Clément Bœsch napisal(a):

Quoted 6 lines

Since commit bbc4d71d6354 ("net: phy: realtek: fix rtl8211e rx/tx delay config") network is broken on the NanoPi Neo 2.

This patch changes the phy-mode to use internal delays both for RX and TX as has been done for other boards affected by the same commit.

Quoted 6 lines
Quoted 1 line
Fixes bbc4d71d6354 ("net: phy: realtek: fix rtl8211e rx/tx delay config")

This commit fixes DT issue, so "fixes" tag should be:

Fixes 44a94c7ef989 ("arm64: dts: allwinner: H5: Restore EMAC changes")

Here, a node with wrong phy-mode property was added to NanoPi Neo 2 board DT.

Shouldn't I add it instead of replacing? I followed what I observed in git log --grep bbc4d71d63 where all the commits pretty much follow this
pattern: that commit is the one causing the actual observed regression, while 44a94c7ef989 is much older, and while it's wrong, it wasn't causing an issue in practice.

Or did I misunderstand something?

Quoted 9 lines

Other than that, this patch is fine and once fixes tag is fixed, you can add:

Reviewed-by Jernej Skrabec <[email protected]>

For next version, you should:
- change fixed tag
- add my review-by tag right above your signed-off-by tag
- mark patch as v2 (add "-v2" parameter to git format-patch)
- describe change right under "---" line

Will do.

Quoted 3 lines

Note, if you borked something when sending, you should mark patch or patch series as "RESEND", so recipients don't look for changes in two subsequent e- mails (--subject-prefix="RESEND PATCH").

Not sure I follow you so before I disturb everyone with more noise I'd just like to confirm: you mean a git send-email in-reply-to=[broken patch attempt] (the one where I borked the --cc), right? But with what patch? I'm a bit confused here.

Quoted 1 line

Thanks for the fix!

No problem; I really think a scan of all the other boards would be meaningful though, it looks like a lot of them got fixed but there are many other candidates and the issue feels pretty critical to me (regression, and no network at all).

Signature · 1 line
Clément B.
maintainer
#3 lore raw

Hi!

Dne ponedeljek, 30. avgust 2021 ob 23:25:23 CEST je Clément Bœsch napisal(a):

Quoted 7 lines · Jernej Škrabec
Quoted 2 lines

Hi!

Hi,

Quoted 1 line

Dne ponedeljek, 30. avgust 2021 ob 17:16:45 CEST je Clément Bœsch

napisal(a):

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines

Since commit bbc4d71d6354 ("net: phy: realtek: fix rtl8211e rx/tx delay config") network is broken on the NanoPi Neo 2.

This patch changes the phy-mode to use internal delays both for RX and TX as has been done for other boards affected by the same commit.

Quoted 6 lines
Quoted 6 lines
Quoted 1 line
Fixes bbc4d71d6354 ("net: phy: realtek: fix rtl8211e rx/tx delay config")

This commit fixes DT issue, so "fixes" tag should be:

Fixes 44a94c7ef989 ("arm64: dts: allwinner: H5: Restore EMAC changes")

Here, a node with wrong phy-mode property was added to NanoPi Neo 2 board

DT.

Quoted 8 lines

Shouldn't I add it instead of replacing? I followed what I observed in git log --grep bbc4d71d63 where all the commits pretty much follow this
pattern: that commit is the one causing the actual observed regression, while 44a94c7ef989 is much older, and while it's wrong, it wasn't causing an issue in practice.

Or did I misunderstand something?

With that grep command you limited yourself only to those commits which reference this particular commit. There are others, which also change "rgmii" to "rgmii-id" and reference other commits, like:
544cc3f8573b ("arm64: dts: allwinner: h6: orangepi-one-plus: Fix ethernet") 97a38c1c213b ("arm64: dts: allwinner: beelink-gs1: Enable both RGMII RX/TX delay")
(there are more).

Anyway, let's continue this discussion in Andrew's thread.

Quoted 4 lines
Quoted 4 lines

Other than that, this patch is fine and once fixes tag is fixed, you can add:

Reviewed-by Jernej Skrabec <[email protected]>
Quoted 8 lines
Quoted 6 lines

For next version, you should:
- change fixed tag
- add my review-by tag right above your signed-off-by tag
- mark patch as v2 (add "-v2" parameter to git format-patch)
- describe change right under "---" line

Will do.

Please wait until discussion reaches a conclusion.

Quoted 3 lines
Quoted 3 lines

Note, if you borked something when sending, you should mark patch or patch series as "RESEND", so recipients don't look for changes in two subsequent

e-

Quoted 6 lines
Quoted 1 line

mails (--subject-prefix="RESEND PATCH").

Not sure I follow you so before I disturb everyone with more noise I'd just like to confirm: you mean a git send-email in-reply-to=[broken patch attempt] (the one where I borked the --cc), right? But with what patch? I'm a bit confused here.

That's just for the future reference. No need to do anything now for this patch. If you bork any send attempt in the future, recreate same patch(es) with this additional tag ("RESEND") and send them again.

Quoted 7 lines
Quoted 2 lines

Thanks for the fix!

No problem; I really think a scan of all the other boards would be meaningful though, it looks like a lot of them got fixed but there are many other candidates and the issue feels pretty critical to me (regression, and no network at all).

I guess you speak for all boards, not just those with Allwinner SoC? I fixed as many boards as I have - testing is very desired. On some boards rgmii was changed to rgmii-txid, so it's not as straightforward as search and replace. It could be also deducted from schematics, but at least one Allwinner board has this configured in HW wrong (SW fix was needed for that board) and other boards might not have schematics publicy available.

In short, I wouldn't start mass generating patches for this.

Best regards,
Jernej

Quoted 4 lines
Signature · 2 lines
-- 
Clément B.
#4 lore raw
In reply to Jernej Škrabec
Quoted 9 lines · Jernej Škrabec

Hi!

Dne ponedeljek, 30. avgust 2021 ob 17:16:45 CEST je Clément Bœsch napisal(a):

Quoted 6 lines

Since commit bbc4d71d6354 ("net: phy: realtek: fix rtl8211e rx/tx delay config") network is broken on the NanoPi Neo 2.

This patch changes the phy-mode to use internal delays both for RX and TX as has been done for other boards affected by the same commit.

Quoted 6 lines
Quoted 1 line
Fixes bbc4d71d6354 ("net: phy: realtek: fix rtl8211e rx/tx delay config")

This commit fixes DT issue, so "fixes" tag should be:

Fixes 44a94c7ef989 ("arm64: dts: allwinner: H5: Restore EMAC changes")

Here, a node with wrong phy-mode property was added to NanoPi Neo 2 board DT.

Hi Jernej

I would say, it is debatable. The board broke when the driver started acting on the value, which is commit bbc4d71d6354. It could be argued it was always technically broken, since it has the wrong value, but practically, it was not broken. One of the rules for stable is:

- It must fix a real bug that bothers people (not a, "This could be a problem..." type thing).

So i would argue, anything before bbc4d71d6354 does not fulfil stable requirements, since it does not bother anybody.

Andrew

maintainer
#5 lore raw

Hi Andrew!

Dne ponedeljek, 30. avgust 2021 ob 23:50:42 CEST je Andrew Lunn napisal(a):

Quoted 4 lines · Jernej Škrabec
Quoted 3 lines

Hi!

Dne ponedeljek, 30. avgust 2021 ob 17:16:45 CEST je Clément Bœsch

napisal(a):

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines

Since commit bbc4d71d6354 ("net: phy: realtek: fix rtl8211e rx/tx delay config") network is broken on the NanoPi Neo 2.

This patch changes the phy-mode to use internal delays both for RX and TX as has been done for other boards affected by the same commit.

Quoted 6 lines
Quoted 6 lines
Quoted 1 line
Fixes bbc4d71d6354 ("net: phy: realtek: fix rtl8211e rx/tx delay config")

This commit fixes DT issue, so "fixes" tag should be:

Fixes 44a94c7ef989 ("arm64: dts: allwinner: H5: Restore EMAC changes")

Here, a node with wrong phy-mode property was added to NanoPi Neo 2 board

DT.

Quoted 8 lines

Hi Jernej

I would say, it is debatable. The board broke when the driver started acting on the value, which is commit bbc4d71d6354. It could be argued it was always technically broken, since it has the wrong value, but practically, it was not broken. One of the rules for stable is:

Quoted 5 lines

- It must fix a real bug that bothers people (not a, "This could be a problem..." type thing).

So i would argue, anything before bbc4d71d6354 does not fulfil stable requirements, since it does not bother anybody.

True, but then again, DT is a bit special, since it's not used only by Linux. It's shared at least with BSDs and U-Boot, which most likely have independent driver implementation. That's why it's good to have DT fixes referencing DT related commits. As you said, DT described HW wrong.

Looking at past Allwinner related DT commits regarding this issue, we were not totally consistent. Most of them reference commit where emac node was introduced in DT. But I also found a couple of commits which indeed have fixes tag for bbc4d71d6354.

Best regards,
Jernej

Quoted 3 lines

Andrew

#6 lore raw
Quoted 9 lines

True, but then again, DT is a bit special, since it's not used only by Linux. It's shared at least with BSDs and U-Boot, which most likely have independent driver implementation. That's why it's good to have DT fixes referencing DT related commits. As you said, DT described HW wrong.

Looking at past Allwinner related DT commits regarding this issue, we were not totally consistent. Most of them reference commit where emac node was introduced in DT. But I also found a couple of commits which indeed have fixes tag for bbc4d71d6354.

All true, and it is not a big deal if it does go back further. I don't care, we can let the Allwinner Maintainer decide. Ah, that is you :-)

I just want to try to keep it simple for somebody who is contributing their first Linux kernel patch. If it is good enough, i say accept it, and we can do a bit more education on the second contribution.

Andre

maintainer
#7 lore raw

Dne torek, 31. avgust 2021 ob 22:53:24 CEST je Andrew Lunn napisal(a):

Quoted 1 line
Quoted 1 line

True, but then again, DT is a bit special, since it's not used only by

Linux.

Quoted 1 line
Quoted 1 line

It's shared at least with BSDs and U-Boot, which most likely have

independent

Quoted 4 lines
Quoted 4 lines

driver implementation. That's why it's good to have DT fixes referencing DT related commits. As you said, DT described HW wrong.

Looking at past Allwinner related DT commits regarding this issue, we were

not

Quoted 2 lines
Quoted 2 lines

totally consistent. Most of them reference commit where emac node was introduced in DT. But I also found a couple of commits which indeed have

fixes

Quoted 8 lines
Quoted 1 line

tag for bbc4d71d6354.

All true, and it is not a big deal if it does go back further. I don't care, we can let the Allwinner Maintainer decide. Ah, that is you :-)

I just want to try to keep it simple for somebody who is contributing their first Linux kernel patch. If it is good enough, i say accept it, and we can do a bit more education on the second contribution.

Maxime, Chen-Yu, thoughts?

Since there are both approaches for fixes tag already in, I guess this is also fine.

Best regards,
Jernej

#8 lore raw
Quoted 18 lines · Jernej Škrabec

Dne torek, 31. avgust 2021 ob 22:53:24 CEST je Andrew Lunn napisal(a):

Quoted 1 line
Quoted 1 line

True, but then again, DT is a bit special, since it's not used only by

Linux.

Quoted 1 line
Quoted 1 line

It's shared at least with BSDs and U-Boot, which most likely have

independent

Quoted 4 lines
Quoted 4 lines

driver implementation. That's why it's good to have DT fixes referencing DT related commits. As you said, DT described HW wrong.

Looking at past Allwinner related DT commits regarding this issue, we were

not

Quoted 2 lines
Quoted 2 lines

totally consistent. Most of them reference commit where emac node was introduced in DT. But I also found a couple of commits which indeed have

fixes

Quoted 5 lines
Quoted 1 line

tag for bbc4d71d6354.

All true, and it is not a big deal if it does go back further. I don't care, we can let the Allwinner Maintainer decide. Ah, that is you :-)

Quoted 8 lines
Quoted 3 lines

I just want to try to keep it simple for somebody who is contributing their first Linux kernel patch. If it is good enough, i say accept it, and we can do a bit more education on the second contribution.

Maxime, Chen-Yu, thoughts?

Since there are both approaches for fixes tag already in, I guess this is also fine.

I agree with you, the fixes should point at the initial Device Tree issue. Our DT are used by other projects and even if in Linux it wasn't causing any issue before another commit showed up, it might affect those projects differently.

Maxime

author
#9 lore raw

PATCH v2 1/1 arm64: dts: allwinner: h5: NanoPI Neo 2: Fix ethernet node

In reply to Jernej Škrabec

RX and TX delay are provided by ethernet PHY. Reflect that in ethernet node.

Fixes 44a94c7ef989 ("arm64: dts: allwinner: H5: Restore EMAC changes")
Signed-off-by Clément Bœsch <[email protected]>
Reviewed-by Jernej Skrabec <[email protected]>

---
Reword the commit message and description (in particular, refer to the DT hash instead of the kernel network commit).

---
 arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo2.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo2.dts +1 -1
index 02f8e72f0cad..05486cccee1c 100644
@@ -75,7 +75,7 @@ &emac {
 	pinctrl-0 = <&emac_rgmii_pins>;
 	phy-supply = <&reg_gmac_3v3>;
 	phy-handle = <&ext_rgmii_phy>;
-	phy-mode = "rgmii";
+	phy-mode = "rgmii-id";
 	status = "okay";
 };
Signature · 1 line
2.33.0
Reviewed-by on v1 1/1
#12 lore raw
In reply to Clément Bœsch
Quoted 8 lines · Clément Bœsch

Since commit bbc4d71d6354 ("net: phy: realtek: fix rtl8211e rx/tx delay config") network is broken on the NanoPi Neo 2.

This patch changes the phy-mode to use internal delays both for RX and TX as has been done for other boards affected by the same commit.

Fixes bbc4d71d6354 ("net: phy: realtek: fix rtl8211e rx/tx delay config")
Signed-off-by Clément Bœsch <[email protected]>
Cc [email protected]
Reviewed-by Andrew Lunn <[email protected]>

Andrew