mt7530 doesn't work on bpi-r3 if mtk_eth is built as module

[ejka]

When both mt7530 and mtk_eth are built as modules, probing for mt7530 fails with error:

mtk_soc_eth 15100000.ethernet: mdio: MDIO timeout
mdio_bus mdio-bus: failed to read mt7530 register
mtk_soc_eth 15100000.ethernet: mdio: MDIO timeout
mdio_bus mdio-bus: failed to read mt7530 register
mt7530 mdio-bus:1f: chip ffff can't be supported

If mtk_eth is changed to built-in, everything works fine.
Tested on 6.1-main and 6.3-rc branches.
Any ideas how to debug this further?

[frank-w]

You can add printk in code,but why do you want eth and switch driver as module? Device is useless when you have no real rootfs (initrd).

Else you should report this on kernel mailinglist...at least if 6.3-rc1 (without additional patches) is affected too

[ejka]

I want to build debian-style all-modules kernel image for several different devices. I run debian installed to emmc so having drivers in modules is not a problem.

[ejka]

I have found the root cause of the issue. Clock gates in ethsys can't be enabled, once they have been disabled and mtk_eth doesn't work. I've sent a mail describing problem to linux-mediatek. Not sure if it have reached the list yet, as I can't subscribe to it.
For now I use the following patch locally to prevent kernel from disabling the clocks:

diff --git a/drivers/clk/mediatek/clk-mt7986-eth.c b/drivers/clk/mediatek/clk-mt7986-eth.c
index 703872239ecc..a368426eddd7 100644
--- a/drivers/clk/mediatek/clk-mt7986-eth.c
+++ b/drivers/clk/mediatek/clk-mt7986-eth.c
@@ -62,19 +62,20 @@ static const struct mtk_gate_regs eth_cg_regs = {
 	.sta_ofs = 0x30,
 };
 
-#define GATE_ETH(_id, _name, _parent, _shift)                                  \
+#define GATE_ETH(_id, _name, _parent, _shift, _flags)	                        \
 	{                                                                      \
 		.id = _id, .name = _name, .parent_name = _parent,              \
 		.regs = &eth_cg_regs, .shift = _shift,                         \
 		.ops = &mtk_clk_gate_ops_no_setclr_inv,                        \
+		.flags = _flags,                                               \
 	}
 
 static const struct mtk_gate eth_clks[] __initconst = {
-	GATE_ETH(CLK_ETH_FE_EN, "eth_fe_en", "netsys_2x_sel", 6),
-	GATE_ETH(CLK_ETH_GP2_EN, "eth_gp2_en", "sgm_325m_sel", 7),
-	GATE_ETH(CLK_ETH_GP1_EN, "eth_gp1_en", "sgm_325m_sel", 8),
-	GATE_ETH(CLK_ETH_WOCPU1_EN, "eth_wocpu1_en", "netsys_mcu_sel", 14),
-	GATE_ETH(CLK_ETH_WOCPU0_EN, "eth_wocpu0_en", "netsys_mcu_sel", 15),
+	GATE_ETH(CLK_ETH_FE_EN, "eth_fe_en", "netsys_2x_sel", 6, CLK_IS_CRITICAL),
+	GATE_ETH(CLK_ETH_GP2_EN, "eth_gp2_en", "sgm_325m_sel", 7, CLK_IS_CRITICAL),
+	GATE_ETH(CLK_ETH_GP1_EN, "eth_gp1_en", "sgm_325m_sel", 8, CLK_IS_CRITICAL),
+	GATE_ETH(CLK_ETH_WOCPU1_EN, "eth_wocpu1_en", "netsys_mcu_sel", 14, CLK_IS_CRITICAL),
+	GATE_ETH(CLK_ETH_WOCPU0_EN, "eth_wocpu0_en", "netsys_mcu_sel", 15, CLK_IS_CRITICAL),
 };
 
 static void __init mtk_sgmiisys_0_init(struct device_node *node)

Not sure if it is correct way though as I don't have hardware documentation.

[frank-w]

thx, but please send this to linux kernel mailinglist (scripts/get_maintainer.pl drivers/clk/mediatek/clk-mt7986-eth.c) and let discussion begin :)

[frank-w]

@ejka have you send patch to mailing-list? looks like this is really a mt7986-only problem, so i guess you found the right solution

code seems to be different now, so i cannot apply the patch on 6.6-rc1, params for GATE_ETH are passed to external macro GATE_MTK...maybe we can set clocks to critical in another way

maybe we can use GATE_MTK_FLAGS instead of GATE_MTK defined both in drivers/clk/mediatek/clk-gate.h

as you use critical for all clocks we can set this in macro...have you tried with only some of them defined critical?

something like this:

diff --git a/drivers/clk/mediatek/clk-mt7986-eth.c b/drivers/clk/mediatek/clk-mt7986-eth.c
index 7ab78e0f49a1..bcb90e78fb26 100644
--- a/drivers/clk/mediatek/clk-mt7986-eth.c
+++ b/drivers/clk/mediatek/clk-mt7986-eth.c
@@ -53,7 +53,7 @@ static const struct mtk_gate_regs eth_cg_regs = {
 };
 
 #define GATE_ETH(_id, _name, _parent, _shift)                  \
-       GATE_MTK(_id, _name, _parent, &eth_cg_regs, _shift, &mtk_clk_gate_ops_no_setclr_inv)
+       GATE_MTK_FLAGS(_id, _name, _parent, &eth_cg_regs, _shift, &mtk_clk_gate_ops_no_setclr_inv, CLK_IS_CRITICAL)
 
 static const struct mtk_gate eth_clks[] = {
        GATE_ETH(CLK_ETH_FE_EN, "eth_fe_en", "netsys_2x_sel", 6),

[jcdutton]

Ah, if this is due to clocks being disabled after boot, there is a boot param to stop them being disabled. One can use the boot param until the driver supports enabling the clock it needs. So, I don't think one really needs to add critical to them.
Boot param is “clk_ignore_unused”.