| Message ID | 20240516204847.171029-1-linux@fw-web.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: dts: mt7622: fix switch probe on bananapi-r64 | expand |
On 16/05/2024 23:48, Frank Wunderlich wrote: > From: Frank Wunderlich <frank-w@public-files.de> > > After commit 868ff5f4944a > ("net: dsa: mt7530-mdio: read PHY address of switch from device tree") > the mt7531 switch on Bananapi-R64 was not detected. > > mt7530-mdio mdio-bus:00: reset timeout > mt7530-mdio mdio-bus:00: probe with driver mt7530-mdio failed with error -110 > > Fix this by adding phy address in devicetree. > > Signed-off-by: Frank Wunderlich <frank-w@public-files.de> I don't like the mention of the Linux kernel driver on the patch log. What you're fixing is the incorrect description of the switch's PHY address on the DTS file. Whether or not any driver from any project is actually reading it from the DTS file is irrelevant to this patch. That said, I already have a patch series I've been meaning to send the next version of that already addresses this. Please wait for that. Arınç
Am 17. Mai 2024 04:17:47 MESZ schrieb "Arınç ÜNAL" <arinc.unal@arinc9.com>: >On 16/05/2024 23:48, Frank Wunderlich wrote: >> From: Frank Wunderlich <frank-w@public-files.de> >> >> After commit 868ff5f4944a >> ("net: dsa: mt7530-mdio: read PHY address of switch from device tree") >> the mt7531 switch on Bananapi-R64 was not detected. >> >> mt7530-mdio mdio-bus:00: reset timeout >> mt7530-mdio mdio-bus:00: probe with driver mt7530-mdio failed with error -110 >> >> Fix this by adding phy address in devicetree. >> >> Signed-off-by: Frank Wunderlich <frank-w@public-files.de> > >I don't like the mention of the Linux kernel driver on the patch log. What >you're fixing is the incorrect description of the switch's PHY address on >the DTS file. Whether or not any driver from any project is actually >reading it from the DTS file is irrelevant to this patch. That said, I >already have a patch series I've been meaning to send the next version of >that already addresses this. Please wait for that. > >Arınç Hi arinc, From my PoV it is a regression in next/6.10 because the driver change was merged (without "broadcast" fallback) and the dts patch [1] is not. I agree that my patch can be dropped because there was already one. regards Frank [1] https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240314-for-mediatek-mt7531-phy-address-v1-1-52f58db01acd@arinc9.com/
On Thu, 16 May 2024 22:48:47 +0200, Frank Wunderlich wrote: > From: Frank Wunderlich <frank-w@public-files.de> > > After commit 868ff5f4944a > ("net: dsa: mt7530-mdio: read PHY address of switch from device tree") > the mt7531 switch on Bananapi-R64 was not detected. > > mt7530-mdio mdio-bus:00: reset timeout > mt7530-mdio mdio-bus:00: probe with driver mt7530-mdio failed with error -110 > > Fix this by adding phy address in devicetree. > > Signed-off-by: Frank Wunderlich <frank-w@public-files.de> > --- > arch/arm64/boot/dts/mediatek/mt7622-bananapi-bpi-r64.dts | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > My bot found new DTB warnings on the .dts files added or changed in this series. Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings are fixed by another series. Ultimately, it is up to the platform maintainer whether these warnings are acceptable or not. No need to reply unless the platform maintainer has comments. If you already ran DT checks and didn't see these error(s), then make sure dt-schema is up to date: pip3 install dtschema --upgrade New warnings running 'make CHECK_DTBS=y mediatek/mt7622-bananapi-bpi-r64.dtb' for 20240516204847.171029-1-linux@fw-web.de: arch/arm64/boot/dts/mediatek/mt7622-bananapi-bpi-r64.dtb: switch@1f: 'interrupts' is a dependency of 'interrupt-controller' from schema $id: http://devicetree.org/schemas/net/dsa/mediatek,mt7530.yaml#
[CCing the regression list, as it should be in the loop for regressions: https://docs.kernel.org/admin-guide/reporting-regressions.html] On 17.05.24 08:27, Frank Wunderlich wrote: > Am 17. Mai 2024 04:17:47 MESZ schrieb "Arınç ÜNAL" <arinc.unal@arinc9.com>: >> On 16/05/2024 23:48, Frank Wunderlich wrote: >>> From: Frank Wunderlich <frank-w@public-files.de> >>> >>> After commit 868ff5f4944a >>> ("net: dsa: mt7530-mdio: read PHY address of switch from device tree") >>> the mt7531 switch on Bananapi-R64 was not detected. >>> >>> mt7530-mdio mdio-bus:00: reset timeout >>> mt7530-mdio mdio-bus:00: probe with driver mt7530-mdio failed with error -110 >>> >>> Fix this by adding phy address in devicetree. >>> >>> Signed-off-by: Frank Wunderlich <frank-w@public-files.de> >> >> I don't like the mention of the Linux kernel driver on the patch log. What >> you're fixing is the incorrect description of the switch's PHY address on >> the DTS file. Whether or not any driver from any project is actually >> reading it from the DTS file is irrelevant to this patch. That said, I >> already have a patch series I've been meaning to send the next version of >> that already addresses this. Please wait for that. Did you sent this? Because from what I see with my limited experience in this subsystem... > From my PoV it is a regression in next/6.10 ...I have to agree with Frank here. So it would be good to get this fixed before -rc1 is out to prevent more people from running into this. > because the driver change was merged (without "broadcast" fallback) and the dts patch [1] is not. > > I agree that my patch can be dropped because there was already one. > > regards Frank > > [1] https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240314-for-mediatek-mt7531-phy-address-v1-1-52f58db01acd@arinc9.com/ Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) -- Everything you wanna know about Linux kernel regression tracking: https://linux-regtracking.leemhuis.info/about/#tldr If I did something stupid, please tell me, as explained on that page.
[adding Paolo, who committed the culprit] On 23.05.24 12:44, Linux regression tracking (Thorsten Leemhuis) wrote: > On 17.05.24 08:27, Frank Wunderlich wrote: >> Am 17. Mai 2024 04:17:47 MESZ schrieb "Arınç ÜNAL" <arinc.unal@arinc9.com>: >>> On 16/05/2024 23:48, Frank Wunderlich wrote: >>>> From: Frank Wunderlich <frank-w@public-files.de> >>>> >>>> After commit 868ff5f4944a >>>> ("net: dsa: mt7530-mdio: read PHY address of switch from device tree") >>>> the mt7531 switch on Bananapi-R64 was not detected. >>>> >>>> mt7530-mdio mdio-bus:00: reset timeout >>>> mt7530-mdio mdio-bus:00: probe with driver mt7530-mdio failed with error -110 >>>> >>>> Fix this by adding phy address in devicetree. >>>> Frank, am I right assuming the regression is still present in mainline? As from here it looks like for two weeks now there was no progress at all to fix this (or I missed it, which is quite possible). Makes me wonder if the maintainers should revert the culprit or if the arm64 dts folks should accept your fix despite Arınç ÜNAL's (who from a quick look on lore hasn't posted anything for two weeks now) comment. Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) -- Everything you wanna know about Linux kernel regression tracking: https://linux-regtracking.leemhuis.info/about/#tldr If I did something stupid, please tell me, as explained on that page. #regzbot poke >>> I don't like the mention of the Linux kernel driver on the patch log. What >>> you're fixing is the incorrect description of the switch's PHY address on >>> the DTS file. Whether or not any driver from any project is actually >>> reading it from the DTS file is irrelevant to this patch. That said, I >>> already have a patch series I've been meaning to send the next version of >>> that already addresses this. Please wait for that. > > Did you sent this? Because from what I see with my limited experience in > this subsystem... > >> From my PoV it is a regression in next/6.10 > > ...I have to agree with Frank here. So it would be good to get this > fixed before -rc1 is out to prevent more people from running into this. > >> because the driver change was merged (without "broadcast" fallback) and the dts patch [1] is not. >> >> I agree that my patch can be dropped because there was already one. >> >> regards Frank >> >> [1] https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240314-for-mediatek-mt7531-phy-address-v1-1-52f58db01acd@arinc9.com/ > > Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) > -- > Everything you wanna know about Linux kernel regression tracking: > https://linux-regtracking.leemhuis.info/about/#tldr > If I did something stupid, please tell me, as explained on that page. >
On 31/05/2024 08.40, Thorsten Leemhuis wrote: > [adding Paolo, who committed the culprit] > > On 23.05.24 12:44, Linux regression tracking (Thorsten Leemhuis) wrote: >> On 17.05.24 08:27, Frank Wunderlich wrote: >>> Am 17. Mai 2024 04:17:47 MESZ schrieb "Arınç ÜNAL" <arinc.unal@arinc9.com>: >>>> On 16/05/2024 23:48, Frank Wunderlich wrote: >>>>> From: Frank Wunderlich <frank-w@public-files.de> >>>>> >>>>> After commit 868ff5f4944a >>>>> ("net: dsa: mt7530-mdio: read PHY address of switch from device tree") >>>>> the mt7531 switch on Bananapi-R64 was not detected. >>>>> >>>>> mt7530-mdio mdio-bus:00: reset timeout >>>>> mt7530-mdio mdio-bus:00: probe with driver mt7530-mdio failed with error -110 >>>>> >>>>> Fix this by adding phy address in devicetree. >>>>> > > Frank, am I right assuming the regression is still present in mainline? > As from here it looks like for two weeks now there was no progress at > all to fix this (or I missed it, which is quite possible). > > Makes me wonder if the maintainers should revert the culprit or if the > arm64 dts folks should accept your fix despite Arınç ÜNAL's (who from a > quick look on lore hasn't posted anything for two weeks now) comment. I'm not against the patch. I'm against the logic it entails on the patch log. I had already submitted a patch series that would've prevented this issue back in 14 March 2024 [1]. I've asked numerous times for the patch series to be applied [2][3][4][5]. Eventually Daniel asked for some changes [6]. But I won't have time to do that anytime soon and I think the patch series is good enough to be applied as is. [1] https://lore.kernel.org/all/20240314-for-mediatek-mt7531-phy-address-v1-0-52f58db01acd@arinc9.com/ [2] https://lore.kernel.org/all/ff196055-ecd8-4563-bc01-ff2533a07109@arinc9.com/ [3] https://lore.kernel.org/all/a60fc16d-4236-427c-b4a8-ec6fdf62d9f0@arinc9.com/ [4] https://lore.kernel.org/all/facb8204-c2b3-4084-a2e3-4fbf3a3fdc9d@arinc9.com/ [5] https://lore.kernel.org/all/44e366ea-964a-4515-9027-2a2edfe12512@arinc9.com/ [6] https://lore.kernel.org/all/ZixU287DdhvRyZBe@makrotopia.org/ Arınç
On 17/05/2024 09.27, Frank Wunderlich wrote: > Am 17. Mai 2024 04:17:47 MESZ schrieb "Arınç ÜNAL" <arinc.unal@arinc9.com>: >> On 16/05/2024 23:48, Frank Wunderlich wrote: >>> From: Frank Wunderlich <frank-w@public-files.de> >>> >>> After commit 868ff5f4944a >>> ("net: dsa: mt7530-mdio: read PHY address of switch from device tree") >>> the mt7531 switch on Bananapi-R64 was not detected. >>> >>> mt7530-mdio mdio-bus:00: reset timeout >>> mt7530-mdio mdio-bus:00: probe with driver mt7530-mdio failed with error -110 >>> >>> Fix this by adding phy address in devicetree. >>> >>> Signed-off-by: Frank Wunderlich <frank-w@public-files.de> >> >> I don't like the mention of the Linux kernel driver on the patch log. What >> you're fixing is the incorrect description of the switch's PHY address on >> the DTS file. Whether or not any driver from any project is actually >> reading it from the DTS file is irrelevant to this patch. That said, I >> already have a patch series I've been meaning to send the next version of >> that already addresses this. Please wait for that. >> >> Arınç > > Hi arinc, > > From my PoV it is a regression in next/6.10 because the driver change was merged (without "broadcast" fallback) and the dts patch [1] is not. What is a broadcast fallback? 0x1f is just another PHY address. Arınç
Am 31. Mai 2024 08:12:06 MESZ schrieb "Arınç ÜNAL" <arinc.unal@arinc9.com>: >On 17/05/2024 09.27, Frank Wunderlich wrote: >> Am 17. Mai 2024 04:17:47 MESZ schrieb "Arınç ÜNAL" <arinc.unal@arinc9.com>: >>> On 16/05/2024 23:48, Frank Wunderlich wrote: >>>> From: Frank Wunderlich <frank-w@public-files.de> >>>> >>>> After commit 868ff5f4944a >>>> ("net: dsa: mt7530-mdio: read PHY address of switch from device tree") >>>> the mt7531 switch on Bananapi-R64 was not detected. >>>> >>>> mt7530-mdio mdio-bus:00: reset timeout >>>> mt7530-mdio mdio-bus:00: probe with driver mt7530-mdio failed with error -110 >>>> >>>> Fix this by adding phy address in devicetree. >>>> >>>> Signed-off-by: Frank Wunderlich <frank-w@public-files.de> >>> >>> I don't like the mention of the Linux kernel driver on the patch log. What >>> you're fixing is the incorrect description of the switch's PHY address on >>> the DTS file. Whether or not any driver from any project is actually >>> reading it from the DTS file is irrelevant to this patch. That said, I >>> already have a patch series I've been meaning to send the next version of >>> that already addresses this. Please wait for that. >>> >>> Arınç >> >> Hi arinc, >> >> From my PoV it is a regression in next/6.10 because the driver change was merged (without "broadcast" fallback) and the dts patch [1] is not. > >What is a broadcast fallback? 0x1f is just another PHY address. Afaik 0x0 is some kind of broadcast address if real phy address is not known. The driver change seems not allow this 0x0 adress and forces devicetree to have the real address. Thats what i mean with broadcast fallback. Maybe the naming is wrong. >Arınç @thorsten i have not tested again,but i have not seen any further fix for it. regards Frank
On 31/05/2024 09.18, Frank Wunderlich wrote: > Am 31. Mai 2024 08:12:06 MESZ schrieb "Arınç ÜNAL" <arinc.unal@arinc9.com>: >> On 17/05/2024 09.27, Frank Wunderlich wrote: >>> Am 17. Mai 2024 04:17:47 MESZ schrieb "Arınç ÜNAL" <arinc.unal@arinc9.com>: >>>> On 16/05/2024 23:48, Frank Wunderlich wrote: >>>>> From: Frank Wunderlich <frank-w@public-files.de> >>>>> >>>>> After commit 868ff5f4944a >>>>> ("net: dsa: mt7530-mdio: read PHY address of switch from device tree") >>>>> the mt7531 switch on Bananapi-R64 was not detected. >>>>> >>>>> mt7530-mdio mdio-bus:00: reset timeout >>>>> mt7530-mdio mdio-bus:00: probe with driver mt7530-mdio failed with error -110 >>>>> >>>>> Fix this by adding phy address in devicetree. >>>>> >>>>> Signed-off-by: Frank Wunderlich <frank-w@public-files.de> >>>> >>>> I don't like the mention of the Linux kernel driver on the patch log. What >>>> you're fixing is the incorrect description of the switch's PHY address on >>>> the DTS file. Whether or not any driver from any project is actually >>>> reading it from the DTS file is irrelevant to this patch. That said, I >>>> already have a patch series I've been meaning to send the next version of >>>> that already addresses this. Please wait for that. >>>> >>>> Arınç >>> >>> Hi arinc, >>> >>> From my PoV it is a regression in next/6.10 because the driver change was merged (without "broadcast" fallback) and the dts patch [1] is not. >> >> What is a broadcast fallback? 0x1f is just another PHY address. > > Afaik 0x0 is some kind of broadcast address if real phy address is not known. The driver change seems not allow this 0x0 adress and forces devicetree to have the real address. That's not true. 0x0 is just another PHY address. The driver change actually allows reading 0x0 from the device tree and using that PHY address to control the switch registers, instead of using 0x1f which was hardcoded on the driver. But on the hardware, the switch actually listens on 0x1f. Arınç
diff --git a/arch/arm64/boot/dts/mediatek/mt7622-bananapi-bpi-r64.dts b/arch/arm64/boot/dts/mediatek/mt7622-bananapi-bpi-r64.dts index 224bb289660c..811b227d6f50 100644 --- a/arch/arm64/boot/dts/mediatek/mt7622-bananapi-bpi-r64.dts +++ b/arch/arm64/boot/dts/mediatek/mt7622-bananapi-bpi-r64.dts @@ -149,9 +149,9 @@ mdio: mdio-bus { #address-cells = <1>; #size-cells = <0>; - switch@0 { + switch@1f { compatible = "mediatek,mt7531"; - reg = <0>; + reg = <0x1f>; interrupt-controller; #interrupt-cells = <1>; interrupts-extended = <&pio 53 IRQ_TYPE_LEVEL_HIGH>;