Skip to content

mediatek: fix DTS defining mt7530 switch phys but not referencing them #15141

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

dangowrt
Copy link
Member

@dangowrt dangowrt commented Apr 12, 2024

mediatek: fix DTS defining mt7530 switch phys but not referencing them

The upstream solution to define the MDIO bus in DT is a bit more strict than our previous downstream solution doing the same thing and now requires switch PHYs to be referenced in DT as well.

Arınç Ünal told us in #15141:
"With [the now upstream patch written by him which we backported], the switch MDIO bus won't be assigned to ds->user_mii_bus when the switch MDIO bus is defined on the device tree anymore. This was not the case with the downstream patch.
When ds->user_mii_bus is populated, DSA will 1:1 map the port with PHY. Meaning port with address 1 will be mapped to PHY with address 1.
Because that ds->user_mii_bus is not populated when the switch MDIO bus is defined on the device tree, on every port node, the PHY address must be supplied by the phy-handle property."

Add those phy-handles to affected devices' DT.

Fixes: 4354b34 ("generic: 6.6: sync mt7530 DSA driver with upstream")
Fixes: 401a6cc ("generic: 6.1: sync mt7530 DSA driver with upstream")
Signed-off-by: Daniel Golle daniel@makrotopia.org

@dangowrt dangowrt requested a review from blocktrron April 12, 2024 13:06
@github-actions github-actions bot added the target/mediatek pull request/issue for mediatek target label Apr 12, 2024
@arinc9
Copy link
Contributor

arinc9 commented Apr 12, 2024

Looks like the upstream solution to define the MDIO bus in DT is a bit more strict than our previous downstream solution doing the same thing and now requires switch PHYs to be referenced in DT as well.

I don't think this is the case.

Read Andrew's comment:

Arınç:

Like you said, if the mdio node is not defined, the driver will assume 1:1
mapping. If not, it will need all the PHYs to be defined on the mdio node
along with on the ports node.

Andrew:

The problem is __of_mdiobus_register() :-(

If there is a node in DT, the scanning of all addresses is
disabled. It looks at just the addresses listed in DT. Plain
mdiobus_register() by default looks at all addresses to discover if a
PHY is there.

Hence when slave.c tries to connect to the PHY, only the PHY you
listed in DT has been found.

I don't see a way around this, at least not without adding a new
variant of of_mdiobus_register which combined DT with a bus scan.

https://patchwork.kernel.org/comment/25318259/

Edit: I will compile an OpenWrt snapshot to get to the bottom of this.

@arinc9
Copy link
Contributor

arinc9 commented Apr 12, 2024

With my patch, the switch MDIO bus won't be assigned to ds->user_mii_bus when the switch MDIO bus is defined on the device tree anymore. This was not the case with the downstream patch.

When ds->user_mii_bus is populated, DSA will 1:1 map the port with PHY. Meaning port with address 1 will be mapped to PHY with address 1. Because that ds->user_mii_bus is not populated when the switch MDIO bus is defined on the device tree, on every port node, the PHY address must be supplied by the phy-handle property.

Daniel, you can add above to your patch log here.

dangowrt added a commit to dangowrt/openwrt that referenced this pull request Apr 12, 2024
The upstream solution to define the MDIO bus in DT is a bit
more strict than our previous downstream solution doing the same thing
and now requires switch PHYs to be referenced in DT as well.

Arınç Ünal told us in openwrt#15141:
"With [the now upstream patch written by him which we backported], the
switch MDIO bus won't be assigned to ds->user_mii_bus when the switch
MDIO bus is defined on the device tree anymore. This was not the case
with the downstream patch.

When ds->user_mii_bus is populated, DSA will 1:1 map the port with
PHY. Meaning port with address 1 will be mapped to PHY with address 1.
Because that ds->user_mii_bus is not populated when the switch MDIO
bus is defined on the device tree, on every port node, the PHY address
must be supplied by the phy-handle property."

Add those phy-handles to affected devices' DT.

Fixes: 4354b34 ("generic: 6.6: sync mt7530 DSA driver with upstream")
Fixes: 401a6cc ("generic: 6.1: sync mt7530 DSA driver with upstream")
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
@dangowrt dangowrt force-pushed the mt7530-fix-switch-phy-without-handle branch from 7ff21c8 to 4f14e8d Compare April 12, 2024 17:47
@Gingernut1978
Copy link
Contributor

Gingernut1978 commented Apr 13, 2024

Working perfect. Tested on Asus TUF-AX4200.

Sat Apr 13 08:18:11 2024 kern.info kernel: [    2.766557] mt7530-mdio mdio-bus:1f: configuring for fixed/2500base-x link mode
Sat Apr 13 08:18:11 2024 kern.info kernel: [    2.775534] mt7530-mdio mdio-bus:1f: Link is Up - 2.5Gbps/Full - flow control rx/tx
Sat Apr 13 08:18:11 2024 kern.warn kernel: [    2.784651] MediaTek MT7531 PHY mt7530-0:01: Configure LED registers (num=14)
Sat Apr 13 08:18:11 2024 kern.info kernel: [    2.799289] mt7530-mdio mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7531 PHY] (irq=POLL)
Sat Apr 13 08:18:11 2024 kern.warn kernel: [    2.819393] MediaTek MT7531 PHY mt7530-0:02: Configure LED registers (num=14)
Sat Apr 13 08:18:11 2024 kern.info kernel: [    2.833935] mt7530-mdio mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7531 PHY] (irq=POLL)
Sat Apr 13 08:18:11 2024 kern.warn kernel: [    2.853718] MediaTek MT7531 PHY mt7530-0:03: Configure LED registers (num=14)
Sat Apr 13 08:18:11 2024 kern.info kernel: [    2.868250] mt7530-mdio mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7531 PHY] (irq=POLL)
Sat Apr 13 08:18:11 2024 kern.warn kernel: [    2.888025] MediaTek MT7531 PHY mt7530-0:04: Configure LED registers (num=14)
Sat Apr 13 08:18:11 2024 kern.info kernel: [    2.902562] mt7530-mdio mdio-bus:1f lan4 (uninitialized): PHY [mt7530-0:04] driver [MediaTek MT7531 PHY] (irq=POLL)

A big thanks as always.

The upstream solution to define the MDIO bus in DT is a bit
more strict than our previous downstream solution doing the same thing
and now requires switch PHYs to be referenced in DT as well.

Arınç Ünal told us in openwrt#15141:
"With [the now upstream patch written by him which we backported], the
switch MDIO bus won't be assigned to ds->user_mii_bus when the switch
MDIO bus is defined on the device tree anymore. This was not the case
with the downstream patch.

When ds->user_mii_bus is populated, DSA will 1:1 map the port with
PHY. Meaning port with address 1 will be mapped to PHY with address 1.
Because that ds->user_mii_bus is not populated when the switch MDIO
bus is defined on the device tree, on every port node, the PHY address
must be supplied by the phy-handle property."

Add those phy-handles to affected devices' DT.

Fixes: 4354b34 ("generic: 6.6: sync mt7530 DSA driver with upstream")
Fixes: 401a6cc ("generic: 6.1: sync mt7530 DSA driver with upstream")
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
@dangowrt dangowrt force-pushed the mt7530-fix-switch-phy-without-handle branch from 4f14e8d to eb13076 Compare April 13, 2024 17:49
@openwrt-bot openwrt-bot merged commit eb13076 into openwrt:main Apr 13, 2024
3 checks passed
@dangowrt dangowrt deleted the mt7530-fix-switch-phy-without-handle branch April 13, 2024 17:49
Vladdrako pushed a commit to Vladdrako/openwrt that referenced this pull request Apr 28, 2024
The upstream solution to define the MDIO bus in DT is a bit
more strict than our previous downstream solution doing the same thing
and now requires switch PHYs to be referenced in DT as well.

Arınç Ünal told us in openwrt#15141:
"With [the now upstream patch written by him which we backported], the
switch MDIO bus won't be assigned to ds->user_mii_bus when the switch
MDIO bus is defined on the device tree anymore. This was not the case
with the downstream patch.

When ds->user_mii_bus is populated, DSA will 1:1 map the port with
PHY. Meaning port with address 1 will be mapped to PHY with address 1.
Because that ds->user_mii_bus is not populated when the switch MDIO
bus is defined on the device tree, on every port node, the PHY address
must be supplied by the phy-handle property."

Add those phy-handles to affected devices' DT.

Fixes: 4354b34 ("generic: 6.6: sync mt7530 DSA driver with upstream")
Fixes: 401a6cc ("generic: 6.1: sync mt7530 DSA driver with upstream")
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
target/mediatek pull request/issue for mediatek target
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants