Re: [PATCH] net: dsa: mt7530: disable LEDs before reset
From: Arınç ÜNAL
Date: Mon Mar 11 2024 - 23:17:59 EST
On 12.03.2024 02:43, Daniel Golle wrote:
On Tue, Mar 12, 2024 at 02:27:25AM +0300, Arınç ÜNAL wrote:
On 12.03.2024 00:58, Daniel Golle wrote:
On Tue, Mar 12, 2024 at 12:22:48AM +0300, Arınç ÜNAL wrote:
Why was this applied? I already explained it did not achieve anything.
I agree that we were still debating about it, however, I do believe
Justin that he truely observed this problem and the fix seemed
appropriate to me.
I've explained this in my previous email which you did not notice
or at least haven't repied to:
https://patchwork.kernel.org/project/netdevbpf/patch/20240305043952.21590-1-justin.swartz@xxxxxxxxxxxxxxxx/#25753421
I did read that and I did not respond because you did not argue over any of
the technical points I've made. All you said was did I repeat the test
enough, on a technical matter that I consider adding two and two together
and expecting a result other than four.
How I interpreted your response was: I don't know much about this, maybe
you're wrong. Justin must've made this patch for a reason so let's have
them elaborate further.
In the end it probably depends on the electric capacity of the circuit
connecting each LED, so it may not be reproducible on all boards and/or
under all circumstances (temperature, humindity, ...).
I'm sorry, this makes no sense to me. I simply fail to see how this fits
here. Could you base your argument over my points please?
Sure, will happily do so.
Do you agree that the LED controller starts manipulating the state of the
pins used for LEDs and bootstrapping after a link is established?
Yes. But a reset may happen while a link is already up because the switch IC
was initialized and in use by the bootloader. And hence LED may be powered
by the LED controller in that moment **just before reset**.
Yes.
Do you agree that after power is cut from the switch IC and then given
back, any active link from before will go away, meaning the pins will go
back to the state that is being dictated by the bootstrapping design of the
board?
I don't see how this could be related. We are not talking about power cuts
here, but rather use of a RESET signal (typically a GPIO on standalone MT753x
or reset controller of the CPU-part of the MCM).
Ok that makes sense. Thanks for clearing that up for me.
Do you agree that with power given back, the HWTRAP register will be
populated before a link is established?
Yes sure, but see above.
Disabling the LEDs and waiting for around 1mS before reset seems like
a sensible thing to do, and I'm glad Justin took care of it.
Let's ask Justin if they tested this on a standalone MT7530. Because I did.
The switch chip won't even be powered on before the switch chip reset
operation is done. So the operation this patch brings does not do anything
at all for standalone MT7530.
This is not true in case the bootloader has already powered on the
switch in order to load firmware via TFTP. In this case the link may
be up (and hence LEDs may be powered on) at the moment the reset
triggerd by probe of the DSA driver kicks in.
This diff works on MCM MT7530 on MT7621AT. Not on standalone MT7530 on
MT7623NI SoC. Also, I believe the LEDs turn off at the first mt7530_probe()
which returns -EPROBE_DEFER.
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index b012cbb249e4..f1a5673baa55 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2247,6 +2247,24 @@ mt7530_setup(struct dsa_switch *ds)
}
}
+ /* Waiting for MT7530 got to stable */
+ INIT_MT7530_DUMMY_POLL(&p, priv, MT7530_HWTRAP);
+ ret = readx_poll_timeout(_mt7530_read, &p, val, val != 0,
+ 20, 1000000);
+ if (ret < 0) {
+ dev_err(priv->dev, "reset timeout\n");
+ return ret;
+ }
+
+ if ((val & HWTRAP_XTAL_MASK) == HWTRAP_XTAL_25MHZ)
+ dev_info(priv->dev, "xtal is 25MHz\n");
+
+ if ((val & HWTRAP_XTAL_MASK) == HWTRAP_XTAL_40MHZ)
+ dev_info(priv->dev, "xtal is 40MHz\n");
+
+ if ((val & HWTRAP_XTAL_MASK) == HWTRAP_XTAL_20MHZ)
+ dev_info(priv->dev, "xtal is 20MHz\n");
+
/* Reset whole chip through gpio pin or memory-mapped registers for
* different type of hardware
*/
[ 5.319534] mt7530-mdio mdio-bus:1f: reset timeout
[ 5.324371] mt7530-mdio: probe of mdio-bus:1f failed with error -110
[ 5.330803] ------------[ cut here ]------------
[ 5.335427] WARNING: CPU: 2 PID: 36 at drivers/regulator/core.c:2398 _regulator_put+0x15c/0x164
[ 5.344180] Modules linked in:
[ 5.347248] CPU: 2 PID: 36 Comm: kworker/u19:0 Not tainted 6.8.0-rc7-next-20240306+ #36
[ 5.355264] Hardware name: Mediatek Cortex-A7 (Device Tree)
[ 5.360841] Workqueue: events_unbound deferred_probe_work_func
[ 5.366694] Call trace:
[ 5.366710] unwind_backtrace from show_stack+0x10/0x14
[ 5.374487] show_stack from dump_stack_lvl+0x54/0x68
[ 5.379551] dump_stack_lvl from __warn+0x94/0xc0
[ 5.384272] __warn from warn_slowpath_fmt+0x184/0x18c
[ 5.389431] warn_slowpath_fmt from _regulator_put+0x15c/0x164
[ 5.395283] _regulator_put from regulator_put+0x1c/0x2c
[ 5.400611] regulator_put from devres_release_all+0x98/0xfc
[ 5.406290] devres_release_all from device_unbind_cleanup+0xc/0x60
[ 5.412577] device_unbind_cleanup from really_probe+0x25c/0x3f4
[ 5.418603] really_probe from __driver_probe_device+0x9c/0x130
[ 5.424543] __driver_probe_device from driver_probe_device+0x30/0xc0
[ 5.431003] driver_probe_device from __device_attach_driver+0xa8/0x120
[ 5.437639] __device_attach_driver from bus_for_each_drv+0x90/0xe4
[ 5.443927] bus_for_each_drv from __device_attach+0xa8/0x1d4
[ 5.449692] __device_attach from bus_probe_device+0x88/0x8c
[ 5.455370] bus_probe_device from deferred_probe_work_func+0x8c/0xd4
[ 5.461829] deferred_probe_work_func from process_one_work+0x158/0x2e4
[ 5.468465] process_one_work from worker_thread+0x264/0x488
[ 5.474142] worker_thread from kthread+0xd4/0xd8
[ 5.478865] kthread from ret_from_fork+0x14/0x28
[ 5.483583] Exception stack(0xf08bdfb0 to 0xf08bdff8)
[ 5.488642] dfa0: 00000000 00000000 00000000 00000000
[ 5.496829] dfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[ 5.505015] dfe0: 00000000 00000000 00000000 00000000 00000013 00000000
[ 5.511688] ---[ end trace 0000000000000000 ]---
[ 5.516493] ------------[ cut here ]------------
[ 5.521153] WARNING: CPU: 2 PID: 36 at drivers/regulator/core.c:2398 _regulator_put+0x15c/0x164
[ 5.529904] Modules linked in:
[ 5.532970] CPU: 2 PID: 36 Comm: kworker/u19:0 Tainted: G W 6.8.0-rc7-next-20240306+ #36
[ 5.542462] Hardware name: Mediatek Cortex-A7 (Device Tree)
[ 5.548038] Workqueue: events_unbound deferred_probe_work_func
[ 5.553890] Call trace:
[ 5.553900] unwind_backtrace from show_stack+0x10/0x14
[ 5.561673] show_stack from dump_stack_lvl+0x54/0x68
[ 5.566737] dump_stack_lvl from __warn+0x94/0xc0
[ 5.571457] __warn from warn_slowpath_fmt+0x184/0x18c
[ 5.576615] warn_slowpath_fmt from _regulator_put+0x15c/0x164
[ 5.582465] _regulator_put from regulator_put+0x1c/0x2c
[ 5.587794] regulator_put from devres_release_all+0x98/0xfc
[ 5.593471] devres_release_all from device_unbind_cleanup+0xc/0x60
[ 5.599757] device_unbind_cleanup from really_probe+0x25c/0x3f4
[ 5.605784] really_probe from __driver_probe_device+0x9c/0x130
[ 5.611724] __driver_probe_device from driver_probe_device+0x30/0xc0
[ 5.618184] driver_probe_device from __device_attach_driver+0xa8/0x120
[ 5.624819] __device_attach_driver from bus_for_each_drv+0x90/0xe4
[ 5.631106] bus_for_each_drv from __device_attach+0xa8/0x1d4
[ 5.636871] __device_attach from bus_probe_device+0x88/0x8c
[ 5.642549] bus_probe_device from deferred_probe_work_func+0x8c/0xd4
[ 5.649009] deferred_probe_work_func from process_one_work+0x158/0x2e4
[ 5.655644] process_one_work from worker_thread+0x264/0x488
[ 5.661321] worker_thread from kthread+0xd4/0xd8
[ 5.666042] kthread from ret_from_fork+0x14/0x28
[ 5.670759] Exception stack(0xf08bdfb0 to 0xf08bdff8)
[ 5.675818] dfa0: 00000000 00000000 00000000 00000000
[ 5.684004] dfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[ 5.692189] dfe0: 00000000 00000000 00000000 00000000 00000013 00000000
[ 5.698858] ---[ end trace 0000000000000000 ]---
My conclusion to this patch is Justin tested this only on an MCM MT7530
where the switch IC still has power before the DSA subdriver kicks in. And
assumed that disabling the LED controller before switch chip reset would
"reduce" the possibility of having these pins continue being manipulated by
the LED controller AFTER power is cut off and given back to the switch
chip, where the state of these pins would be back to being dictated by the
bootstrapping design of the board.
Jakub, please revert this. And please next time do not apply any patch that
modifies this driver without my approval if I've already made an argument
against it. I'm actively maintaining this driver, if there's a need to
respond, I will do so.
This patch did not have any ACKs. It also did not have the tree described
on the subject. More reasons as to why this shouldn't have been applied in
its current state.
It was clearly recognizable as a fix.
I see that it was applied to the net-next tree[1], not net[2]. Looks like
it wasn't clear enough. Let's follow the rules.
However, I agree that applying it after Ack from an active maintainer
would have been better.
I don't see a need to revert it before this debate (which starts to
look like an argument over authority) has concluded.
Let's hope this patch makes its way to the net tree.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/log/drivers/net/dsa/mt7530.c
[2] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/log/drivers/net/dsa/mt7530.c
Arınç