Re: [PATCH] net: dsa: mt7530: disable LEDs before reset

On Tue, Mar 12, 2024 at 02:27:25AM +0300, Arınç ÜNAL wrote:

On 12.03.2024 00:58, Daniel Golle wrote:

quoted

On Tue, Mar 12, 2024 at 12:22:48AM +0300, Arınç ÜNAL wrote:

quoted

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.

quoted

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**.

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).

Do you agree that with power given back, the HWTRAP register will be
populated before a link is established?

Yes sure, but see above.

quoted

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.

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.

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel