Message-ID: <6d601f06-7760-41d7-a190-2f370f7827ba@arinc9.com>
Date: Tue, 12 Mar 2024 06:17:26 +0300
From: Arınç ÜNAL <arinc.unal@...nc9.com>
To: Daniel Golle <daniel@...rotopia.org>
Cc: patchwork-bot+netdevbpf@...nel.org,
 Justin Swartz <justin.swartz@...ingedge.co.za>, dqfext@...il.com,
 sean.wang@...iatek.com, andrew@...n.ch, f.fainelli@...il.com,
 olteanv@...il.com, davem@...emloft.net, edumazet@...gle.com,
 kuba@...nel.org, pabeni@...hat.com, matthias.bgg@...il.com,
 angelogioacchino.delregno@...labora.com, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH] net: dsa: mt7530: disable LEDs before reset

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@risingedge.co.za/#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ç

Powered by blists - more mailing lists