Disable LEDs just before resetting the MT7530 to avoid
situations where the ESW_P4_LED_0 and ESW_P3_LED_0 pin
states may cause an unintended external crystal frequency
to be selected.
The HT_XTAL_FSEL (External Crystal Frequency Selection)
field of HWTRAP (the Hardware Trap register) stores a
2-bit value that represents the state of the ESW_P4_LED_0
and ESW_P4_LED_0 pins (seemingly) sampled just after the
MT7530 has been reset, as:
ESW_P4_LED_0 ESW_P3_LED_0 Frequency
-----------------------------------------
0 1 20MHz
1 0 40MHz
1 1 25MHz
The value of HT_XTAL_FSEL is bootstrapped by pulling
ESW_P4_LED_0 and ESW_P3_LED_0 up or down accordingly,
but:
if a 40MHz crystal has been selected and
the ESW_P3_LED_0 pin is high during reset,
or a 20MHz crystal has been selected and
the ESW_P4_LED_0 pin is high during reset,
then the value of HT_XTAL_FSEL will indicate
that a 25MHz crystal is present.
By default, the state of the LED pins is PHY controlled
to reflect the link state.
To illustrate, if a board has:
5 ports with active low LED control,
and HT_XTAL_FSEL bootstrapped for 40MHz.
When the MT7530 is powered up without any external
connection, only the LED associated with Port 3 is
illuminated as ESW_P3_LED_0 is low.
In this state, directly after mt7530_setup()'s reset
is performed, the HWTRAP register (0x7800) reflects
the intended HT_XTAL_FSEL (HWTRAP bits 10:9) of 40MHz:
mt7530-mdio mdio-bus:1f: mt7530_read: 00007800 == 00007dcf
>>> bin(0x7dcf >> 9 & 0b11)
'0b10'
But if a cable is connected to Port 3 and the link
is active before mt7530_setup()'s reset takes place,
then HT_XTAL_FSEL seems to be set for 25MHz:
mt7530-mdio mdio-bus:1f: mt7530_read: 00007800 == 00007fcf
>>> bin(0x7fcf >> 9 & 0b11)
'0b11'
Once HT_XTAL_FSEL reflects 25MHz, none of the ports
are functional until the MT7621 (or MT7530 itself)
is reset.
By disabling the LED pins just before reset, the chance
of an unintended HT_XTAL_FSEL value is reduced.
Signed-off-by: Justin Swartz <[email protected]>
---
drivers/net/dsa/mt7530.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 3c1f65759..8fa113126 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2238,6 +2238,12 @@ mt7530_setup(struct dsa_switch *ds)
}
}
+ /* Disable LEDs before reset to prevent the MT7530 sampling a
+ * potentially incorrect HT_XTAL_FSEL value.
+ */
+ mt7530_write(priv, MT7530_LED_EN, 0);
+ usleep_range(1000, 1100);
+
/* Reset whole chip through gpio pin or memory-mapped registers for
* different type of hardware
*/
--
Hey Justin.
I couldn't find anything on the MT7621 Giga Switch Programming Guide v0.3
document regarding which pin corresponds to which bit on the HWTRAP
register. There's only this mention on the LED controller section,
"hardware traps and LEDs share the same pins in GSW". But page 16 of the
schematics document for Banana Pi BPI-R2 [1] fully documents this.
The HWTRAP register is populated right after power comes back after the
switch chip is reset [2]. This means any active link before the reset will
go away so the high/low state of the pins will go back to being dictated by
the bootstrapping design of the board. The HWTRAP register will be
populated before a link can be set up.
In conclusion, I don't see any need to disable the LED controller before
resetting the switch chip.
[1] https://wiki.banana-pi.org/Banana_Pi_BPI-R2#Documents
[2] I've tested it on my MT7621AT board with a 40MHz XTAL frequency and a
board with standalone MT7530 with 25MHz XTAL frequency.
While the kernel was booting, before the DSA subdriver kicks in:
- For the board with 40 MHz XTAL: I've connected a Vcc pin to ESW_P3_LED_0
to set it high.
- For the board with 25 MHz XTAL: I've connected a GND pin to ESW_P3_LED_0
to set it low.
Board with 40 MHz XTAL:
[ 2.359428] mt7530-mdio mdio-bus:1f: MT7530 adapts as multi-chip module
[ 2.374918] mt7530-mdio mdio-bus:1f: xtal is 25MHz
Board with 25 MHz XTAL:
[ 4.324672] mt7530-mdio mdio-bus:1f: xtal is 40MHz
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 51d7b816dd02..beab5e5558d0 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2216,6 +2216,15 @@ mt7530_setup(struct dsa_switch *ds)
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");
+
id = mt7530_read(priv, MT7530_CREV);
id >>= CHIP_NAME_SHIFT;
if (id != MT7530_ID) {
Arınç
On 5.03.2024 07:39, Justin Swartz wrote:
> Disable LEDs just before resetting the MT7530 to avoid
> situations where the ESW_P4_LED_0 and ESW_P3_LED_0 pin
> states may cause an unintended external crystal frequency
> to be selected.
>
> The HT_XTAL_FSEL (External Crystal Frequency Selection)
> field of HWTRAP (the Hardware Trap register) stores a
> 2-bit value that represents the state of the ESW_P4_LED_0
> and ESW_P4_LED_0 pins (seemingly) sampled just after the
> MT7530 has been reset, as:
>
> ESW_P4_LED_0 ESW_P3_LED_0 Frequency
> -----------------------------------------
> 0 1 20MHz
> 1 0 40MHz
> 1 1 25MHz
>
> The value of HT_XTAL_FSEL is bootstrapped by pulling
> ESW_P4_LED_0 and ESW_P3_LED_0 up or down accordingly,
> but:
>
> if a 40MHz crystal has been selected and
> the ESW_P3_LED_0 pin is high during reset,
>
> or a 20MHz crystal has been selected and
> the ESW_P4_LED_0 pin is high during reset,
>
> then the value of HT_XTAL_FSEL will indicate
> that a 25MHz crystal is present.
>
> By default, the state of the LED pins is PHY controlled
> to reflect the link state.
>
> To illustrate, if a board has:
>
> 5 ports with active low LED control,
> and HT_XTAL_FSEL bootstrapped for 40MHz.
>
> When the MT7530 is powered up without any external
> connection, only the LED associated with Port 3 is
> illuminated as ESW_P3_LED_0 is low.
>
> In this state, directly after mt7530_setup()'s reset
> is performed, the HWTRAP register (0x7800) reflects
> the intended HT_XTAL_FSEL (HWTRAP bits 10:9) of 40MHz:
>
> mt7530-mdio mdio-bus:1f: mt7530_read: 00007800 == 00007dcf
>
> >>> bin(0x7dcf >> 9 & 0b11)
> '0b10'
>
> But if a cable is connected to Port 3 and the link
> is active before mt7530_setup()'s reset takes place,
> then HT_XTAL_FSEL seems to be set for 25MHz:
>
> mt7530-mdio mdio-bus:1f: mt7530_read: 00007800 == 00007fcf
>
> >>> bin(0x7fcf >> 9 & 0b11)
> '0b11'
>
> Once HT_XTAL_FSEL reflects 25MHz, none of the ports
> are functional until the MT7621 (or MT7530 itself)
> is reset.
>
> By disabling the LED pins just before reset, the chance
> of an unintended HT_XTAL_FSEL value is reduced.
>
> Signed-off-by: Justin Swartz <[email protected]>
> ---
> drivers/net/dsa/mt7530.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 3c1f65759..8fa113126 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2238,6 +2238,12 @@ mt7530_setup(struct dsa_switch *ds)
> }
> }
>
> + /* Disable LEDs before reset to prevent the MT7530 sampling a
> + * potentially incorrect HT_XTAL_FSEL value.
> + */
> + mt7530_write(priv, MT7530_LED_EN, 0);
> + usleep_range(1000, 1100);
> +
> /* Reset whole chip through gpio pin or memory-mapped registers for
> * different type of hardware
> */
On Fri, Mar 08, 2024 at 12:51:15PM +0300, Arınç ÜNAL wrote:
> Hey Justin.
>
> I couldn't find anything on the MT7621 Giga Switch Programming Guide v0.3
> document regarding which pin corresponds to which bit on the HWTRAP
> register. There's only this mention on the LED controller section,
> "hardware traps and LEDs share the same pins in GSW". But page 16 of the
> schematics document for Banana Pi BPI-R2 [1] fully documents this.
>
> The HWTRAP register is populated right after power comes back after the
> switch chip is reset [2]. This means any active link before the reset will
> go away so the high/low state of the pins will go back to being dictated by
> the bootstrapping design of the board. The HWTRAP register will be
> populated before a link can be set up.
>
> In conclusion, I don't see any need to disable the LED controller before
> resetting the switch chip.
>
> [1] https://wiki.banana-pi.org/Banana_Pi_BPI-R2#Documents
>
> [2] I've tested it on my MT7621AT board with a 40MHz XTAL frequency and a
> board with standalone MT7530 with 25MHz XTAL frequency.
How many times did you repeat this test to conclude that there is no
need to disable LEDs before reset?
As Justin is decribing a probabilistic phenomenon ("[...] chance of an
unintended HT_XTAL_FSEL value is reduced.") I believe running a single
test is not enough to conlcude anything.
I have a hard time believing that he was working on this patch without
any reason to do so...
>
> While the kernel was booting, before the DSA subdriver kicks in:
> - For the board with 40 MHz XTAL: I've connected a Vcc pin to ESW_P3_LED_0
> to set it high.
> - For the board with 25 MHz XTAL: I've connected a GND pin to ESW_P3_LED_0
> to set it low.
>
> Board with 40 MHz XTAL:
> [ 2.359428] mt7530-mdio mdio-bus:1f: MT7530 adapts as multi-chip module
> [ 2.374918] mt7530-mdio mdio-bus:1f: xtal is 25MHz
>
> Board with 25 MHz XTAL:
> [ 4.324672] mt7530-mdio mdio-bus:1f: xtal is 40MHz
>
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 51d7b816dd02..beab5e5558d0 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2216,6 +2216,15 @@ mt7530_setup(struct dsa_switch *ds)
> 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");
> +
> id = mt7530_read(priv, MT7530_CREV);
> id >>= CHIP_NAME_SHIFT;
> if (id != MT7530_ID) {
>
> Arınç
>
> On 5.03.2024 07:39, Justin Swartz wrote:
> > Disable LEDs just before resetting the MT7530 to avoid
> > situations where the ESW_P4_LED_0 and ESW_P3_LED_0 pin
> > states may cause an unintended external crystal frequency
> > to be selected.
> >
> > The HT_XTAL_FSEL (External Crystal Frequency Selection)
> > field of HWTRAP (the Hardware Trap register) stores a
> > 2-bit value that represents the state of the ESW_P4_LED_0
> > and ESW_P4_LED_0 pins (seemingly) sampled just after the
> > MT7530 has been reset, as:
> >
> > ESW_P4_LED_0 ESW_P3_LED_0 Frequency
> > -----------------------------------------
> > 0 1 20MHz
> > 1 0 40MHz
> > 1 1 25MHz
> >
> > The value of HT_XTAL_FSEL is bootstrapped by pulling
> > ESW_P4_LED_0 and ESW_P3_LED_0 up or down accordingly,
> > but:
> >
> > if a 40MHz crystal has been selected and
> > the ESW_P3_LED_0 pin is high during reset,
> >
> > or a 20MHz crystal has been selected and
> > the ESW_P4_LED_0 pin is high during reset,
> >
> > then the value of HT_XTAL_FSEL will indicate
> > that a 25MHz crystal is present.
> >
> > By default, the state of the LED pins is PHY controlled
> > to reflect the link state.
> >
> > To illustrate, if a board has:
> >
> > 5 ports with active low LED control,
> > and HT_XTAL_FSEL bootstrapped for 40MHz.
> >
> > When the MT7530 is powered up without any external
> > connection, only the LED associated with Port 3 is
> > illuminated as ESW_P3_LED_0 is low.
> >
> > In this state, directly after mt7530_setup()'s reset
> > is performed, the HWTRAP register (0x7800) reflects
> > the intended HT_XTAL_FSEL (HWTRAP bits 10:9) of 40MHz:
> >
> > mt7530-mdio mdio-bus:1f: mt7530_read: 00007800 == 00007dcf
> >
> > >>> bin(0x7dcf >> 9 & 0b11)
> > '0b10'
> >
> > But if a cable is connected to Port 3 and the link
> > is active before mt7530_setup()'s reset takes place,
> > then HT_XTAL_FSEL seems to be set for 25MHz:
> >
> > mt7530-mdio mdio-bus:1f: mt7530_read: 00007800 == 00007fcf
> >
> > >>> bin(0x7fcf >> 9 & 0b11)
> > '0b11'
> >
> > Once HT_XTAL_FSEL reflects 25MHz, none of the ports
> > are functional until the MT7621 (or MT7530 itself)
> > is reset.
> >
> > By disabling the LED pins just before reset, the chance
> > of an unintended HT_XTAL_FSEL value is reduced.
> >
> > Signed-off-by: Justin Swartz <[email protected]>
> > ---
> > drivers/net/dsa/mt7530.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> > index 3c1f65759..8fa113126 100644
> > --- a/drivers/net/dsa/mt7530.c
> > +++ b/drivers/net/dsa/mt7530.c
> > @@ -2238,6 +2238,12 @@ mt7530_setup(struct dsa_switch *ds)
> > }
> > }
> > + /* Disable LEDs before reset to prevent the MT7530 sampling a
> > + * potentially incorrect HT_XTAL_FSEL value.
> > + */
> > + mt7530_write(priv, MT7530_LED_EN, 0);
> > + usleep_range(1000, 1100);
> > +
> > /* Reset whole chip through gpio pin or memory-mapped registers for
> > * different type of hardware
> > */
Hello:
This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <[email protected]>:
On Tue, 5 Mar 2024 06:39:51 +0200 you wrote:
> Disable LEDs just before resetting the MT7530 to avoid
> situations where the ESW_P4_LED_0 and ESW_P3_LED_0 pin
> states may cause an unintended external crystal frequency
> to be selected.
>
> The HT_XTAL_FSEL (External Crystal Frequency Selection)
> field of HWTRAP (the Hardware Trap register) stores a
> 2-bit value that represents the state of the ESW_P4_LED_0
> and ESW_P4_LED_0 pins (seemingly) sampled just after the
> MT7530 has been reset, as:
>
> [...]
Here is the summary with links:
- net: dsa: mt7530: disable LEDs before reset
https://git.kernel.org/netdev/net-next/c/2920dd92b980
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
Why was this applied? I already explained it did not achieve anything.
Arınç
On 12.03.2024 00:10, [email protected] wrote:
> Hello:
>
> This patch was applied to netdev/net-next.git (main)
> by Jakub Kicinski <[email protected]>:
>
> On Tue, 5 Mar 2024 06:39:51 +0200 you wrote:
>> Disable LEDs just before resetting the MT7530 to avoid
>> situations where the ESW_P4_LED_0 and ESW_P3_LED_0 pin
>> states may cause an unintended external crystal frequency
>> to be selected.
>>
>> The HT_XTAL_FSEL (External Crystal Frequency Selection)
>> field of HWTRAP (the Hardware Trap register) stores a
>> 2-bit value that represents the state of the ESW_P4_LED_0
>> and ESW_P4_LED_0 pins (seemingly) sampled just after the
>> MT7530 has been reset, as:
>>
>> [...]
>
> Here is the summary with links:
> - net: dsa: mt7530: disable LEDs before reset
> https://git.kernel.org/netdev/net-next/c/2920dd92b980
>
> You are awesome, thank you!
On Tue, 12 Mar 2024 00:22:48 +0300 Arınç ÜNAL wrote:
> Why was this applied? I already explained it did not achieve anything.
Daniel disagreed and you didn't reply to him.
Please don't top post.
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/[email protected]/#25753421
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, ...).
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.
>
> Arınç
>
> On 12.03.2024 00:10, [email protected] wrote:
> > Hello:
> >
> > This patch was applied to netdev/net-next.git (main)
> > by Jakub Kicinski <[email protected]>:
> >
> > On Tue, 5 Mar 2024 06:39:51 +0200 you wrote:
> > > Disable LEDs just before resetting the MT7530 to avoid
> > > situations where the ESW_P4_LED_0 and ESW_P3_LED_0 pin
> > > states may cause an unintended external crystal frequency
> > > to be selected.
> > >
> > > The HT_XTAL_FSEL (External Crystal Frequency Selection)
> > > field of HWTRAP (the Hardware Trap register) stores a
> > > 2-bit value that represents the state of the ESW_P4_LED_0
> > > and ESW_P4_LED_0 pins (seemingly) sampled just after the
> > > MT7530 has been reset, as:
> > >
> > > [...]
> >
> > Here is the summary with links:
> > - net: dsa: mt7530: disable LEDs before reset
> > https://git.kernel.org/netdev/net-next/c/2920dd92b980
> >
> > You are awesome, thank you!
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/[email protected]/#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?
Do you agree that the LED controller starts manipulating the state of the
pins used for LEDs and bootstrapping after a link is established?
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?
Do you agree that with power given back, the HWTRAP register will be
populated before a link is established?
>
> 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.
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.
Arınç
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/[email protected]/#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**.
>
> 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.
>
> >
> > 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.
Hi Arınç
This reply will be best read with a fixed-width font.
On 2024-03-08 11:51, Arınç ÜNAL wrote:
> Hey Justin.
>
> I couldn't find anything on the MT7621 Giga Switch Programming Guide
> v0.3
> document regarding which pin corresponds to which bit on the HWTRAP
> register. There's only this mention on the LED controller section,
> "hardware traps and LEDs share the same pins in GSW". But page 16 of
> the
> schematics document for Banana Pi BPI-R2 [1] fully documents this.
There is also a table in the "MT7530 Giga Switch Programming
Guide" that describes which pins correspond to the bits of
HWTRAP, but beware of typos.
> The HWTRAP register is populated right after power comes back after the
> switch chip is reset [2]. This means any active link before the reset
> will
> go away so the high/low state of the pins will go back to being
> dictated by
> the bootstrapping design of the board. The HWTRAP register will be
> populated before a link can be set up.
> In conclusion, I don't see any need to disable the LED controller
> before
> resetting the switch chip.
I should have included more evidence to support my claim.
> [1] https://wiki.banana-pi.org/Banana_Pi_BPI-R2#Documents
>
> [2] I've tested it on my MT7621AT board with a 40MHz XTAL frequency and
> a
> board with standalone MT7530 with 25MHz XTAL frequency.
>
> While the kernel was booting, before the DSA subdriver kicks in:
> - For the board with 40 MHz XTAL: I've connected a Vcc pin to
> ESW_P3_LED_0
> to set it high.
My board has a 40MHz crystal between XPTL_XI and XPTL_XO,
ESW_P4_LED_0 has a 4.7k pull up to 3.3V, and ESW_P3_LED_0
has a 4.7k pull down to GND.
For testing, I'm relying on the MT7530 itself to change the
ESW_P3_LED_0 level according to the link state.
> - For the board with 25 MHz XTAL: I've connected a GND pin to
> ESW_P3_LED_0
> to set it low.
>
> Board with 40 MHz XTAL:
> [ 2.359428] mt7530-mdio mdio-bus:1f: MT7530 adapts as multi-chip
> module
> [ 2.374918] mt7530-mdio mdio-bus:1f: xtal is 25MHz
>
> Board with 25 MHz XTAL:
> [ 4.324672] mt7530-mdio mdio-bus:1f: xtal is 40MHz
>
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 51d7b816dd02..beab5e5558d0 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2216,6 +2216,15 @@ mt7530_setup(struct dsa_switch *ds)
> 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");
> +
> id = mt7530_read(priv, MT7530_CREV);
> id >>= CHIP_NAME_SHIFT;
> if (id != MT7530_ID) {
>
> Arınç
I took a similar approach, with calls to dev_info()
throughout mt7530_setup() plus mt7530_write(), mt7530_read()
and mt7530_rmw() to get an idea of the flow of execution and
which registers were being manipulated.
Calls to dev_info() affected timing, so I switched to using
trace_printk() and then read the output from the debugfs's
tracing/trace file instead of from the console.
I attached a logic analyzer to ESW_P4_LED_0 and ESW_P3_LED_0,
and manually triggered sampling as soon as execution of the
kernel was reported on UART1.
-- Test #1 -----------------------------------------------------------
For the sake of maximal reproducability, I removed the delay
between the assertion and deassertion of reset and added
HWTRAP value tracing:
---%---
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2243,7 +2243,6 @@ mt7530_setup(struct dsa_switch *ds)
*/
if (priv->mcm) {
reset_control_assert(priv->rstc);
- usleep_range(1000, 1100);
reset_control_deassert(priv->rstc);
} else {
gpiod_set_value_cansleep(priv->reset, 0);
@@ -2260,6 +2259,10 @@ mt7530_setup(struct dsa_switch *ds)
return ret;
}
+ static char ht_xtal_fsel[4][6] = { "?????", "20MHz", "40MHz", "25MHz"
};
+ trace_printk("HWTRAP = %x, HT_XTAL_FSEL = %s\n",
+ val, ht_xtal_fsel[(val & HWTRAP_XTAL_MASK) >> 9]);
+
id = mt7530_read(priv, MT7530_CREV);
id >>= CHIP_NAME_SHIFT;
if (id != MT7530_ID) {
---%---
(a) Without a cable connected to Port 3 (lan4) before reset, the
correct external crystal frequency is selected:
[3] [4] [6]
: : :
_________ ______________________________________
ESW_P4_LED_0 |_______|
_______
ESW_P3_LED_0 _________| |______________________________________
: :
[5]...:
[3] Port 4 LED Off. Port 3 LED On.
[4] Signals inverted. (reset_control_assert; reset_control_deassert)
[5] Period of 310 usec.
[6] Signals reflect the bootstrapped configuration.
~ # sed -n -e '$s/^.*\. //p' /sys/kernel/debug/tracing/trace
2.432646: mt7530_setup: HWTRAP = 7dcf, HT_XTAL_FSEL = 40MHz
(b) When a cable attached to an active peer is connected to
Port 3 (lan4) before reset, the incorrect crystal frequency
selection (0b11 = 25MHz) always occurs:
[7] [8] [10] [12]
: : : :
_________ ______________________________________
ESW_P4_LED_0 |_______|
_________ _______
ESW_P3_LED_0 |_______| |______________________________
: : : :
[9]...: [11]..:
[7] Port 4 LED Off. Port 3 LED Off.
[8] Signals inverted. (reset_control_assert; reset_control_deassert)
[9] Period of 320 usec.
[10] Signals inverted.
[11] Period of 300 usec.
[12] Signals reflect the bootstrapped configuration.
~ # sed -n -e '$s/^.*\. //p' /sys/kernel/debug/tracing/trace
2.432884: mt7530_setup: HWTRAP = 7fcf, HT_XTAL_FSEL = 25MHz
-- Test #2 -----------------------------------------------------------
To attempt to determine which transitions are associated
with asserting and deasserting reset, I performed another
test with a delay of what I intended to be a 1 sec period
where the MT7530 is held in reset:
---%---
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2243,7 +2243,7 @@ mt7530_setup(struct dsa_switch *ds)
*/
if (priv->mcm) {
reset_control_assert(priv->rstc);
- usleep_range(1000, 1100);
+ usleep_range(1000000, 1000000);
reset_control_deassert(priv->rstc);
} else {
gpiod_set_value_cansleep(priv->reset, 0);
@@ -2260,6 +2260,10 @@ mt7530_setup(struct dsa_switch *ds)
return ret;
}
+ static char ht_xtal_fsel[4][6] = { "?????", "20MHz", "40MHz", "25MHz"
};
+ trace_printk("HWTRAP = %x, HT_XTAL_FSEL = %s\n",
+ val, ht_xtal_fsel[(val & HWTRAP_XTAL_MASK) >> 9]);
+
id = mt7530_read(priv, MT7530_CREV);
id >>= CHIP_NAME_SHIFT;
if (id != MT7530_ID) {
---%---
(a) Without a cable connected to Port 3 (lan4) before reset, the
correct external crystal frequency is again selected:
[13] [14] [16]
: : :
_________ ______________________________________
ESW_P4_LED_0 |_______|
_______
ESW_P3_LED_0 _________| |______________________________________
: :
[15]..:
[13] Port 4 LED Off. Port 3 LED On.
[14] Signals inverted. (reset_control_deassert?)
[15] Period of 310 usec.
[16] Signals reflect the bootstrapped configuration.
~ # sed -n -e '$s/^.*\. //p' /sys/kernel/debug/tracing/trace
3.437461: mt7530_setup: HWTRAP = 7dcf, HT_XTAL_FSEL = 40MHz
No difference is apparent in the timing diagram, compared
to the result from Test #1a, but it is obvious that the code
which follows the reset was executed about 1 second later.
(b) With a cable from an active peer connected to Port 3
(lan4) before reset, the correct crystal frequency
(0b10 = 40MHz) is selected:
[17] [18] [20] [22]
: : : :
______________________ \ \ ______ _______________
ESW_P4_LED_0 / / |______|
_________ \ \ ______
ESW_P3_LED_0 |____________ / / ______| |_______________
\ \
: : : :
[19]..................: [21].:
[17] Port 4 LED Off. Port 3 LED Off.
[18] ESW_P3_LED_0 set low. (reset_control_assert?)
[19] Period of 1000.325 msec.
[20] Signals inverted. (reset_control_deassert?)
[21] Period of 310 usec.
[22] Signals reflect the bootstrapped configuration.
~ # sed -n -e '$s/^.*\. //p' /sys/kernel/debug/tracing/trace
3.433235: mt7530_setup: HWTRAP = 7dcf, HT_XTAL_FSEL = 40MHz
So it appears that when reset is deasserted, the ESW_P4_LED_0
and ESW_P3_LED_0 levels are inverted for a period of about
300 - 310 usec in Test #1a, #1b, #2a, and #2b.
Co-incidentally, these inverted levels are the active low
representation of what ends up in HT_XTAL_FSEL. And in all
four examples, have been the inversion of what immediately
preceded reset_control_deassert().
-- Test #3 -----------------------------------------------------------
Now it seems that there is a signature that can be used
to identify when reset_control_deassert() is executed,
the time of reset_control_assert()'s execution should be
between (at most) 1100 and (at least) 1000 usec prior
based on the unmodified reset logic.
So this patch only includes the HT_XTAL_FSEL trace.
---%---
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2260,6 +2260,10 @@ mt7530_setup(struct dsa_switch *ds)
return ret;
}
+ static char ht_xtal_fsel[4][6] = { "?????", "20MHz", "40MHz",
"25MHz" };
+ trace_printk("HWTRAP = %x, HT_XTAL_FSEL = %s\n",
+ val, ht_xtal_fsel[(val & HWTRAP_XTAL_MASK) >> 9]);
+
id = mt7530_read(priv, MT7530_CREV);
id >>= CHIP_NAME_SHIFT;
if (id != MT7530_ID) {
---%---
The purpose of the following examples are to show the
variance in how long it takes for ESW_P3_LED_0 to go low.
(a) With a cable from an active peer connected to Port 3
(lan4) before reset, the correct crystal frequency
(0b10 = 40MHz) is selected.
[23] [24] [26] [28] [30]
: : : : :
_____________________________ __________________
ESW_P4_LED_0 |_______|
___________________ _______
ESW_P3_LED_0 |_________| |__________________
: : : : :
: [27]....: [29]..:
[25]...............:
[23] Port 4 LED Off. Port 3 LED Off.
[24] Approximate point of reset_control_assert?
[25] Period of approximately 1000 - 1100 usec.
[26] ESW_P3_LED_0 finally goes low.
[27] Period of 415 usec.
[28] Signals inverted. (reset_control_deassert?)
[29] Period of 310 usec.
[30] Signals reflect the bootstrapped configuration.
(b) With a cable from an active peer connected to Port 3
(lan4) before reset, the correct crystal frequency
(0b10 = 40MHz) is selected.
[31] [32] [34] [36] [38]
: : : : :
_____________________________ __________________
ESW_P4_LED_0 |_______|
___________________________ _______
ESW_P3_LED_0 |_| |__________________
: : : :
: [35] [37]..:
: :
[33]...............:
[31] Port 4 LED Off. Port 3 LED Off.
[32] Approximate point of reset_control_assert?
[33] Period of approximately 1000 - 1100 usec.
[34] ESW_P3_LED_0 finally goes low.
[35] Period of 50 usec.
[36] Signals inverted. (reset_control_deassert?)
[37] Period of 310 usec.
[38] Signals reflect the bootstrapped configuration.
(c) With a cable from an active peer connected to Port 3
(lan4) before reset, an incorrect crystal frequency
(0b11 = 25MHz) is selected.
[45] [46] [48] [50]
: : : :
_____________________________ __________________
ESW_P4_LED_0 |_______|
_____________________________
ESW_P3_LED_0 |__________________________
: : : :
: : [49]..:
: :
[47]...............:
[45] Port 4 LED Off. Port 3 LED Off.
[46] Approximate point of reset_control_assert?
[47] Period of approximately 1000 - 1100 usec.
[48] Signals inverted. (reset_control_deassert?)
[49] Period of 315 usec.
[50] Signals reflect the bootstrapped configuration.
~ # sed -n -e '$s/^.*\. //p' /sys/kernel/debug/tracing/trace
2.617819: mt7530_setup: HWTRAP = 7fcf, HT_XTAL_FSEL = 25MHz
~ # cat /proc/cmdline
console=ttyS0,57600 loglevel=7 printk.time=1 root=/dev/ram0 debug
rdinit=/linuxrc
-- End of Tests ------------------------------------------------------
It seems that the incorrect crystal frequency is selected more
often when debugging messages are present (such as printk()
based approaches) and especially when loglevel=7 and printk.time=1
are specified on the command line.
For the sake of reference: I disabled ethernet support in my build
of (mainline) U-boot, and my kernel configuration is a very lean
selection of options suited for IP routing and a few peripherals
on the I2C and SPI buses.
My userland is confined to an initramfs built around busybox.
I also disable gmac1 because I need a few of the rgmii2 pins for
modem control signalling.
Regards
Justin
PS: Yes, I only have access to MT7621AT SoCs.
> On 5.03.2024 07:39, Justin Swartz wrote:
>> Disable LEDs just before resetting the MT7530 to avoid
>> situations where the ESW_P4_LED_0 and ESW_P3_LED_0 pin
>> states may cause an unintended external crystal frequency
>> to be selected.
>>
>> The HT_XTAL_FSEL (External Crystal Frequency Selection)
>> field of HWTRAP (the Hardware Trap register) stores a
>> 2-bit value that represents the state of the ESW_P4_LED_0
>> and ESW_P4_LED_0 pins (seemingly) sampled just after the
>> MT7530 has been reset, as:
>>
>> ESW_P4_LED_0 ESW_P3_LED_0 Frequency
>> -----------------------------------------
>> 0 1 20MHz
>> 1 0 40MHz
>> 1 1 25MHz
>>
>> The value of HT_XTAL_FSEL is bootstrapped by pulling
>> ESW_P4_LED_0 and ESW_P3_LED_0 up or down accordingly,
>> but:
>>
>> if a 40MHz crystal has been selected and
>> the ESW_P3_LED_0 pin is high during reset,
>>
>> or a 20MHz crystal has been selected and
>> the ESW_P4_LED_0 pin is high during reset,
>>
>> then the value of HT_XTAL_FSEL will indicate
>> that a 25MHz crystal is present.
>>
>> By default, the state of the LED pins is PHY controlled
>> to reflect the link state.
>>
>> To illustrate, if a board has:
>>
>> 5 ports with active low LED control,
>> and HT_XTAL_FSEL bootstrapped for 40MHz.
>>
>> When the MT7530 is powered up without any external
>> connection, only the LED associated with Port 3 is
>> illuminated as ESW_P3_LED_0 is low.
>>
>> In this state, directly after mt7530_setup()'s reset
>> is performed, the HWTRAP register (0x7800) reflects
>> the intended HT_XTAL_FSEL (HWTRAP bits 10:9) of 40MHz:
>>
>> mt7530-mdio mdio-bus:1f: mt7530_read: 00007800 == 00007dcf
>>
>> >>> bin(0x7dcf >> 9 & 0b11)
>> '0b10'
>>
>> But if a cable is connected to Port 3 and the link
>> is active before mt7530_setup()'s reset takes place,
>> then HT_XTAL_FSEL seems to be set for 25MHz:
>>
>> mt7530-mdio mdio-bus:1f: mt7530_read: 00007800 == 00007fcf
>>
>> >>> bin(0x7fcf >> 9 & 0b11)
>> '0b11'
>>
>> Once HT_XTAL_FSEL reflects 25MHz, none of the ports
>> are functional until the MT7621 (or MT7530 itself)
>> is reset.
>>
>> By disabling the LED pins just before reset, the chance
>> of an unintended HT_XTAL_FSEL value is reduced.
>>
>> Signed-off-by: Justin Swartz <[email protected]>
>> ---
>> drivers/net/dsa/mt7530.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index 3c1f65759..8fa113126 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -2238,6 +2238,12 @@ mt7530_setup(struct dsa_switch *ds)
>> }
>> }
>> + /* Disable LEDs before reset to prevent the MT7530 sampling a
>> + * potentially incorrect HT_XTAL_FSEL value.
>> + */
>> + mt7530_write(priv, MT7530_LED_EN, 0);
>> + usleep_range(1000, 1100);
>> +
>> /* Reset whole chip through gpio pin or memory-mapped registers for
>> * different type of hardware
>> */
> (b) When a cable attached to an active peer is connected to
> Port 3 (lan4) before reset, the incorrect crystal frequency
> selection (0b11 = 25MHz) always occurs:
>
> [7] [8] [10] [12]
> : : : :
> _________ ______________________________________
> ESW_P4_LED_0 |_______|
> _________ _______
> ESW_P3_LED_0 |_______| |______________________________
>
> : : : :
> [9]...: [11]..:
>
> [7] Port 4 LED Off. Port 3 LED Off.
> [8] Signals inverted. (reset_control_assert; reset_control_deassert)
> [9] Period of 320 usec.
> [10] Signals inverted.
> [11] Period of 300 usec.
> [12] Signals reflect the bootstrapped configuration.
Shame the patch has already been accepted. The text in this email
would of made a cool commit message.
Andrew
On 12.03.2024 03:07, Justin Swartz wrote:
> I took a similar approach, with calls to dev_info()
> throughout mt7530_setup() plus mt7530_write(), mt7530_read()
> and mt7530_rmw() to get an idea of the flow of execution and
> which registers were being manipulated.
>
> Calls to dev_info() affected timing, so I switched to using
> trace_printk() and then read the output from the debugfs's
> tracing/trace file instead of from the console.
>
> I attached a logic analyzer to ESW_P4_LED_0 and ESW_P3_LED_0,
> and manually triggered sampling as soon as execution of the
> kernel was reported on UART1.
>
>
> -- Test #1 -----------------------------------------------------------
>
> For the sake of maximal reproducability, I removed the delay
> between the assertion and deassertion of reset and added
> HWTRAP value tracing:
>
> ---%---
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2243,7 +2243,6 @@ mt7530_setup(struct dsa_switch *ds)
> */
> if (priv->mcm) {
> reset_control_assert(priv->rstc);
> - usleep_range(1000, 1100);
> reset_control_deassert(priv->rstc);
> } else {
> gpiod_set_value_cansleep(priv->reset, 0);
> @@ -2260,6 +2259,10 @@ mt7530_setup(struct dsa_switch *ds)
> return ret;
> }
>
> + static char ht_xtal_fsel[4][6] = { "?????", "20MHz", "40MHz", "25MHz" };
> + trace_printk("HWTRAP = %x, HT_XTAL_FSEL = %s\n",
> + val, ht_xtal_fsel[(val & HWTRAP_XTAL_MASK) >> 9]);
> +
> id = mt7530_read(priv, MT7530_CREV);
> id >>= CHIP_NAME_SHIFT;
> if (id != MT7530_ID) {
> ---%---
>
> (a) Without a cable connected to Port 3 (lan4) before reset, the
> correct external crystal frequency is selected:
>
> [3] [4] [6]
> : : :
> _________ ______________________________________
> ESW_P4_LED_0 |_______|
> _______
> ESW_P3_LED_0 _________| |______________________________________
>
> : :
> [5]...:
>
> [3] Port 4 LED Off. Port 3 LED On.
> [4] Signals inverted. (reset_control_assert; reset_control_deassert)
> [5] Period of 310 usec.
> [6] Signals reflect the bootstrapped configuration.
>
>
> ~ # sed -n -e '$s/^.*\. //p' /sys/kernel/debug/tracing/trace
> 2.432646: mt7530_setup: HWTRAP = 7dcf, HT_XTAL_FSEL = 40MHz
>
>
> (b) When a cable attached to an active peer is connected to
> Port 3 (lan4) before reset, the incorrect crystal frequency
> selection (0b11 = 25MHz) always occurs:
>
> [7] [8] [10] [12]
> : : : :
> _________ ______________________________________
> ESW_P4_LED_0 |_______|
> _________ _______
> ESW_P3_LED_0 |_______| |______________________________
>
> : : : :
> [9]...: [11]..:
>
> [7] Port 4 LED Off. Port 3 LED Off.
> [8] Signals inverted. (reset_control_assert; reset_control_deassert)
> [9] Period of 320 usec.
> [10] Signals inverted.
> [11] Period of 300 usec.
> [12] Signals reflect the bootstrapped configuration.
>
> ~ # sed -n -e '$s/^.*\. //p' /sys/kernel/debug/tracing/trace
> 2.432884: mt7530_setup: HWTRAP = 7fcf, HT_XTAL_FSEL = 25MHz
>
>
> -- Test #2 -----------------------------------------------------------
>
> To attempt to determine which transitions are associated
> with asserting and deasserting reset, I performed another
> test with a delay of what I intended to be a 1 sec period
> where the MT7530 is held in reset:
>
> ---%---
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2243,7 +2243,7 @@ mt7530_setup(struct dsa_switch *ds)
> */
> if (priv->mcm) {
> reset_control_assert(priv->rstc);
> - usleep_range(1000, 1100);
> + usleep_range(1000000, 1000000);
> reset_control_deassert(priv->rstc);
> } else {
> gpiod_set_value_cansleep(priv->reset, 0);
> @@ -2260,6 +2260,10 @@ mt7530_setup(struct dsa_switch *ds)
> return ret;
> }
>
> + static char ht_xtal_fsel[4][6] = { "?????", "20MHz", "40MHz", "25MHz" };
> + trace_printk("HWTRAP = %x, HT_XTAL_FSEL = %s\n",
> + val, ht_xtal_fsel[(val & HWTRAP_XTAL_MASK) >> 9]);
> +
> id = mt7530_read(priv, MT7530_CREV);
> id >>= CHIP_NAME_SHIFT;
> if (id != MT7530_ID) {
> ---%---
>
> (a) Without a cable connected to Port 3 (lan4) before reset, the
> correct external crystal frequency is again selected:
>
> [13] [14] [16]
> : : :
> _________ ______________________________________
> ESW_P4_LED_0 |_______|
> _______
> ESW_P3_LED_0 _________| |______________________________________
>
> : :
> [15]..:
>
> [13] Port 4 LED Off. Port 3 LED On.
> [14] Signals inverted. (reset_control_deassert?)
> [15] Period of 310 usec.
> [16] Signals reflect the bootstrapped configuration.
>
>
> ~ # sed -n -e '$s/^.*\. //p' /sys/kernel/debug/tracing/trace
> 3.437461: mt7530_setup: HWTRAP = 7dcf, HT_XTAL_FSEL = 40MHz
>
>
> No difference is apparent in the timing diagram, compared
> to the result from Test #1a, but it is obvious that the code
> which follows the reset was executed about 1 second later.
>
>
> (b) With a cable from an active peer connected to Port 3
> (lan4) before reset, the correct crystal frequency
> (0b10 = 40MHz) is selected:
>
> [17] [18] [20] [22]
> : : : :
> ______________________ \ \ ______ _______________
> ESW_P4_LED_0 / / |______|
> _________ \ \ ______
> ESW_P3_LED_0 |____________ / / ______| |_______________
> \ \
> : : : :
> [19]..................: [21].:
>
> [17] Port 4 LED Off. Port 3 LED Off.
> [18] ESW_P3_LED_0 set low. (reset_control_assert?)
> [19] Period of 1000.325 msec.
> [20] Signals inverted. (reset_control_deassert?)
> [21] Period of 310 usec.
> [22] Signals reflect the bootstrapped configuration.
>
>
> ~ # sed -n -e '$s/^.*\. //p' /sys/kernel/debug/tracing/trace
> 3.433235: mt7530_setup: HWTRAP = 7dcf, HT_XTAL_FSEL = 40MHz
>
>
> So it appears that when reset is deasserted, the ESW_P4_LED_0
> and ESW_P3_LED_0 levels are inverted for a period of about
> 300 - 310 usec in Test #1a, #1b, #2a, and #2b.
>
> Co-incidentally, these inverted levels are the active low
> representation of what ends up in HT_XTAL_FSEL. And in all
> four examples, have been the inversion of what immediately
> preceded reset_control_deassert().
>
>
> -- Test #3 -----------------------------------------------------------
>
> Now it seems that there is a signature that can be used
> to identify when reset_control_deassert() is executed,
> the time of reset_control_assert()'s execution should be
> between (at most) 1100 and (at least) 1000 usec prior
> based on the unmodified reset logic.
>
> So this patch only includes the HT_XTAL_FSEL trace.
>
> ---%---
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2260,6 +2260,10 @@ mt7530_setup(struct dsa_switch *ds)
> return ret;
> }
>
> + static char ht_xtal_fsel[4][6] = { "?????", "20MHz", "40MHz", "25MHz" };
> + trace_printk("HWTRAP = %x, HT_XTAL_FSEL = %s\n",
> + val, ht_xtal_fsel[(val & HWTRAP_XTAL_MASK) >> 9]);
> +
> id = mt7530_read(priv, MT7530_CREV);
> id >>= CHIP_NAME_SHIFT;
> if (id != MT7530_ID) {
> ---%---
>
> The purpose of the following examples are to show the
> variance in how long it takes for ESW_P3_LED_0 to go low.
>
> (a) With a cable from an active peer connected to Port 3
> (lan4) before reset, the correct crystal frequency
> (0b10 = 40MHz) is selected.
>
> [23] [24] [26] [28] [30]
> : : : : :
> _____________________________ __________________
> ESW_P4_LED_0 |_______|
> ___________________ _______
> ESW_P3_LED_0 |_________| |__________________
>
> : : : : :
> : [27]....: [29]..:
> [25]...............:
>
> [23] Port 4 LED Off. Port 3 LED Off.
> [24] Approximate point of reset_control_assert?
> [25] Period of approximately 1000 - 1100 usec.
> [26] ESW_P3_LED_0 finally goes low.
> [27] Period of 415 usec.
> [28] Signals inverted. (reset_control_deassert?)
> [29] Period of 310 usec.
> [30] Signals reflect the bootstrapped configuration.
>
>
> (b) With a cable from an active peer connected to Port 3
> (lan4) before reset, the correct crystal frequency
> (0b10 = 40MHz) is selected.
>
> [31] [32] [34] [36] [38]
> : : : : :
> _____________________________ __________________
> ESW_P4_LED_0 |_______|
> ___________________________ _______
> ESW_P3_LED_0 |_| |__________________
>
> : : : :
> : [35] [37]..:
> : :
> [33]...............:
>
> [31] Port 4 LED Off. Port 3 LED Off.
> [32] Approximate point of reset_control_assert?
> [33] Period of approximately 1000 - 1100 usec.
> [34] ESW_P3_LED_0 finally goes low.
> [35] Period of 50 usec.
> [36] Signals inverted. (reset_control_deassert?)
> [37] Period of 310 usec.
> [38] Signals reflect the bootstrapped configuration.
>
>
> (c) With a cable from an active peer connected to Port 3
> (lan4) before reset, an incorrect crystal frequency
> (0b11 = 25MHz) is selected.
>
> [45] [46] [48] [50]
> : : : :
> _____________________________ __________________
> ESW_P4_LED_0 |_______|
> _____________________________
> ESW_P3_LED_0 |__________________________
>
> : : : :
> : : [49]..:
> : :
> [47]...............:
>
> [45] Port 4 LED Off. Port 3 LED Off.
> [46] Approximate point of reset_control_assert?
> [47] Period of approximately 1000 - 1100 usec.
> [48] Signals inverted. (reset_control_deassert?)
> [49] Period of 315 usec.
> [50] Signals reflect the bootstrapped configuration.
>
> ~ # sed -n -e '$s/^.*\. //p' /sys/kernel/debug/tracing/trace
> 2.617819: mt7530_setup: HWTRAP = 7fcf, HT_XTAL_FSEL = 25MHz
>
> ~ # cat /proc/cmdline
> console=ttyS0,57600 loglevel=7 printk.time=1 root=/dev/ram0 debug rdinit=/linuxrc
>
>
> -- End of Tests ------------------------------------------------------
>
> It seems that the incorrect crystal frequency is selected more
> often when debugging messages are present (such as printk()
> based approaches) and especially when loglevel=7 and printk.time=1
> are specified on the command line.
>
> For the sake of reference: I disabled ethernet support in my build
> of (mainline) U-boot, and my kernel configuration is a very lean
> selection of options suited for IP routing and a few peripherals
> on the I2C and SPI buses.
>
> My userland is confined to an initramfs built around busybox.
>
> I also disable gmac1 because I need a few of the rgmii2 pins for
> modem control signalling.
>
> Regards
> Justin
>
> PS: Yes, I only have access to MT7621AT SoCs.
Thanks for the detailed presentation. I've simplified it to this, from what
I understood.
--- Currently -------------------------------------------------------------
With a cable from an active peer connected to Port 3 (lan4) before
reset:
Test 1, the correct crystal frequency (0b10 = 40MHz) is selected.
[1] [2-1] [3] [5]
: : : :
_____________________________ __________________
ESW_P4_LED_0 |_______|
___________________ _______
ESW_P3_LED_0 |_________| |__________________
: : : : :
: [2-2]...: [4]...:
[2]................:
[1] reset_control_assert.
[2] Period of 1000 - 1100 usec.
[2-1] ESW_P3_LED_0 goes low.
[2-2] Period of 415 usec.
[3] reset_control_deassert.
[4] Period of 310 usec. HWTRAP register is populated with bootstrapped
XTAL frequency.
[5] Signals reflect the bootstrapped configuration.
Test 2, an incorrect crystal frequency (0b11 = 25MHz) is selected.
[2] [4] [6]
: : :
_____________________________ __________________
ESW_P4_LED_0 |_______|
_____________________________
ESW_P3_LED_0 |__________________________
: : : :
: : [5]...:
: :
[3]................:
[1] reset_control_assert.
[2] Period of 1000 - 1100 usec.
[3] reset_control_deassert.
[5] Period of 315 usec. HWTRAP register is populated with incorrect
XTAL frequency.
[6] Signals reflect the bootstrapped configuration.
--- 1 second delay between assert and deassert ----------------------------
With a cable from an active peer connected to Port 3 (lan4) before
reset, the correct crystal frequency (0b10 = 40MHz) is selected:
[1] [2-1] [3] [5]
: : : :
_____________________________ __________________
ESW_P4_LED_0 |_______|
___________________ _______
ESW_P3_LED_0 |_________| |__________________
: : : : :
: [2-2]...: [4]...:
[2]................:
[1] reset_control_assert.
[3] Period of 1000.325 msec.
[2-1] ESW_P3_LED_0 goes low.
[2-2] Remaining period of 1000.325 msec.
[3] reset_control_deassert.
[4] Period of 310 usec. HWTRAP register is populated with bootstrapped
XTAL frequency.
[5] Signals reflect the bootstrapped configuration.
---
Wouldn't it be a better idea to increase the delay between
reset_control_assert() and reset_control_deassert(), and
gpiod_set_value_cansleep(priv->reset, 0) and
gpiod_set_value_cansleep(priv->reset, 1) instead of disabling the LED
controller and delaying before reset?
One MT7531 pin used for an LED is also used to decide the crystal frequency
between 40MHz and 25Mhz. We should implement this there as well.
Arınç
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/[email protected]/#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ç
On 12.03.2024 00:10, [email protected] wrote:
> Hello:
>
> This patch was applied to netdev/net-next.git (main)
> by Jakub Kicinski <[email protected]>:
>
> On Tue, 5 Mar 2024 06:39:51 +0200 you wrote:
>> Disable LEDs just before resetting the MT7530 to avoid
>> situations where the ESW_P4_LED_0 and ESW_P3_LED_0 pin
>> states may cause an unintended external crystal frequency
>> to be selected.
>>
>> The HT_XTAL_FSEL (External Crystal Frequency Selection)
>> field of HWTRAP (the Hardware Trap register) stores a
>> 2-bit value that represents the state of the ESW_P4_LED_0
>> and ESW_P4_LED_0 pins (seemingly) sampled just after the
>> MT7530 has been reset, as:
>>
>> [...]
>
> Here is the summary with links:
> - net: dsa: mt7530: disable LEDs before reset
> https://git.kernel.org/netdev/net-next/c/2920dd92b980
>
> You are awesome, thank you!
I am once again calling for this patch to be reverted on the net-next tree
on the basis of:
- This patch did not go through a proper reviewing process. There are
proposed changes on the code it changes regarding the scope and the
method of the patch, and improvements to be made on the patch log.
- This patch should be backported to stable releases, therefore it
shouldn't be on the net-next tree and should be submitted to the net tree
instead.
Arınç
On Tue, 2024-03-12 at 11:38 +0300, Arınç ÜNAL wrote:
> On 12.03.2024 00:10, [email protected] wrote:
> > Hello:
> >
> > This patch was applied to netdev/net-next.git (main)
> > by Jakub Kicinski <[email protected]>:
> >
> > On Tue, 5 Mar 2024 06:39:51 +0200 you wrote:
> > > Disable LEDs just before resetting the MT7530 to avoid
> > > situations where the ESW_P4_LED_0 and ESW_P3_LED_0 pin
> > > states may cause an unintended external crystal frequency
> > > to be selected.
> > >
> > > The HT_XTAL_FSEL (External Crystal Frequency Selection)
> > > field of HWTRAP (the Hardware Trap register) stores a
> > > 2-bit value that represents the state of the ESW_P4_LED_0
> > > and ESW_P4_LED_0 pins (seemingly) sampled just after the
> > > MT7530 has been reset, as:
> > >
> > > [...]
> >
> > Here is the summary with links:
> > - net: dsa: mt7530: disable LEDs before reset
> > https://git.kernel.org/netdev/net-next/c/2920dd92b980
> >
> > You are awesome, thank you!
>
> I am once again calling for this patch to be reverted on the net-next tree
> on the basis of:
>
> - This patch did not go through a proper reviewing process. There are
> proposed changes on the code it changes regarding the scope and the
> method of the patch, and improvements to be made on the patch log.
>
> - This patch should be backported to stable releases, therefore it
> shouldn't be on the net-next tree and should be submitted to the net tree
> instead.
The net-next pull request is out:
https://lore.kernel.org/netdev/[email protected]/
at this point I believe we can't retract it unless there is a very
serious regression affecting most/all users. This does not look such
case.
I think the better option is follow-up on net with follow-up fixes if
any.
All the relevant patches could be sent to the stable tree later:
https://elixir.bootlin.com/linux/latest/source/Documentation/process/stable-kernel-rules.rst#L47
To try to reduce the possibilities of this kind of situation in the
future, may I kindly ask you to invest some more little time to help
the reviewers and the maintainers? e.g. trimming the replies explicitly
cutting all the unneeded parts in the quoted code/text would make the
whole conversation much easier to follow (at least to me). The netdev
volume is insane, it's very easy to get lost in a given thread and miss
relevant part of it.
Cheers,
Paolo
On 12.03.2024 13:46, Paolo Abeni wrote:
> On Tue, 2024-03-12 at 11:38 +0300, Arınç ÜNAL wrote:
>> I am once again calling for this patch to be reverted on the net-next tree
>> on the basis of:
>>
>> - This patch did not go through a proper reviewing process. There are
>> proposed changes on the code it changes regarding the scope and the
>> method of the patch, and improvements to be made on the patch log.
>>
>> - This patch should be backported to stable releases, therefore it
>> shouldn't be on the net-next tree and should be submitted to the net tree
>> instead.
>
> The net-next pull request is out:
>
> https://lore.kernel.org/netdev/[email protected]/
>
> at this point I believe we can't retract it unless there is a very
> serious regression affecting most/all users. This does not look such
> case.
It seems so. This patch was not tested on standalone MT7530 at submission.
Whilst the patch is useless for standalone MT7530, it doesn't seem to break
anything either, from my simple test on a board with it.
>
> I think the better option is follow-up on net with follow-up fixes if
> any.
>
> All the relevant patches could be sent to the stable tree later:
>
> https://elixir.bootlin.com/linux/latest/source/Documentation/process/stable-kernel-rules.rst#L47
>
> To try to reduce the possibilities of this kind of situation in the
> future, may I kindly ask you to invest some more little time to help
> the reviewers and the maintainers? e.g. trimming the replies explicitly
> cutting all the unneeded parts in the quoted code/text would make the
> whole conversation much easier to follow (at least to me). The netdev
> volume is insane, it's very easy to get lost in a given thread and miss
> relevant part of it.
I already try to do this. Here's my proposal that would not reduce but
completely avoid this kind of situation in the future, and at the same time
reduce the workload of the netdev maintainers. Do not apply patches without
ACKs. Ask for reviews at least once if the patch had been stale for a
while, and wait a bit for reviews. Only then apply the patch with/without
ACKs.
Arınç
Arınç
On 2024-03-12 04:41, Arınç ÜNAL wrote:
> On 12.03.2024 03:07, Justin Swartz wrote:
>> I took a similar approach, with calls to dev_info()
>> throughout mt7530_setup() plus mt7530_write(), mt7530_read()
>> and mt7530_rmw() to get an idea of the flow of execution and
>> which registers were being manipulated.
>>
>> Calls to dev_info() affected timing, so I switched to using
>> trace_printk() and then read the output from the debugfs's
>> tracing/trace file instead of from the console.
>>
>> I attached a logic analyzer to ESW_P4_LED_0 and ESW_P3_LED_0,
>> and manually triggered sampling as soon as execution of the
>> kernel was reported on UART1.
>>
>>
>> -- Test #1 -----------------------------------------------------------
>>
>> For the sake of maximal reproducability, I removed the delay
>> between the assertion and deassertion of reset and added
>> HWTRAP value tracing:
>>
>> ---%---
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -2243,7 +2243,6 @@ mt7530_setup(struct dsa_switch *ds)
>> */
>> if (priv->mcm) {
>> reset_control_assert(priv->rstc);
>> - usleep_range(1000, 1100);
>> reset_control_deassert(priv->rstc);
>> } else {
>> gpiod_set_value_cansleep(priv->reset, 0);
>> @@ -2260,6 +2259,10 @@ mt7530_setup(struct dsa_switch *ds)
>> return ret;
>> }
>>
>> + static char ht_xtal_fsel[4][6] = { "?????", "20MHz", "40MHz",
>> "25MHz" };
>> + trace_printk("HWTRAP = %x, HT_XTAL_FSEL = %s\n",
>> + val, ht_xtal_fsel[(val & HWTRAP_XTAL_MASK) >> 9]);
>> +
>> id = mt7530_read(priv, MT7530_CREV);
>> id >>= CHIP_NAME_SHIFT;
>> if (id != MT7530_ID) {
>> ---%---
>>
>> (a) Without a cable connected to Port 3 (lan4) before reset, the
>> correct external crystal frequency is selected:
>>
>> [3] [4] [6]
>> : : :
>> _________
>> ______________________________________
>> ESW_P4_LED_0 |_______|
>> _______
>> ESW_P3_LED_0 _________| |______________________________________
>>
>> : :
>> [5]...:
>>
>> [3] Port 4 LED Off. Port 3 LED On.
>> [4] Signals inverted. (reset_control_assert; reset_control_deassert)
>> [5] Period of 310 usec.
>> [6] Signals reflect the bootstrapped configuration.
>>
>>
>> ~ # sed -n -e '$s/^.*\. //p' /sys/kernel/debug/tracing/trace
>> 2.432646: mt7530_setup: HWTRAP = 7dcf, HT_XTAL_FSEL = 40MHz
>>
>>
>> (b) When a cable attached to an active peer is connected to
>> Port 3 (lan4) before reset, the incorrect crystal frequency
>> selection (0b11 = 25MHz) always occurs:
>>
>> [7] [8] [10] [12]
>> : : : :
>> _________
>> ______________________________________
>> ESW_P4_LED_0 |_______|
>> _________ _______
>> ESW_P3_LED_0 |_______| |______________________________
>>
>> : : : :
>> [9]...: [11]..:
>>
>> [7] Port 4 LED Off. Port 3 LED Off.
>> [8] Signals inverted. (reset_control_assert; reset_control_deassert)
>> [9] Period of 320 usec.
>> [10] Signals inverted.
>> [11] Period of 300 usec.
>> [12] Signals reflect the bootstrapped configuration.
>>
>> ~ # sed -n -e '$s/^.*\. //p' /sys/kernel/debug/tracing/trace
>> 2.432884: mt7530_setup: HWTRAP = 7fcf, HT_XTAL_FSEL = 25MHz
>>
>>
>> -- Test #2 -----------------------------------------------------------
>>
>> To attempt to determine which transitions are associated
>> with asserting and deasserting reset, I performed another
>> test with a delay of what I intended to be a 1 sec period
>> where the MT7530 is held in reset:
>>
>> ---%---
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -2243,7 +2243,7 @@ mt7530_setup(struct dsa_switch *ds)
>> */
>> if (priv->mcm) {
>> reset_control_assert(priv->rstc);
>> - usleep_range(1000, 1100);
>> + usleep_range(1000000, 1000000);
>> reset_control_deassert(priv->rstc);
>> } else {
>> gpiod_set_value_cansleep(priv->reset, 0);
>> @@ -2260,6 +2260,10 @@ mt7530_setup(struct dsa_switch *ds)
>> return ret;
>> }
>>
>> + static char ht_xtal_fsel[4][6] = { "?????", "20MHz", "40MHz",
>> "25MHz" };
>> + trace_printk("HWTRAP = %x, HT_XTAL_FSEL = %s\n",
>> + val, ht_xtal_fsel[(val & HWTRAP_XTAL_MASK) >> 9]);
>> +
>> id = mt7530_read(priv, MT7530_CREV);
>> id >>= CHIP_NAME_SHIFT;
>> if (id != MT7530_ID) {
>> ---%---
>>
>> (a) Without a cable connected to Port 3 (lan4) before reset, the
>> correct external crystal frequency is again selected:
>>
>> [13] [14] [16]
>> : : :
>> _________
>> ______________________________________
>> ESW_P4_LED_0 |_______|
>> _______
>> ESW_P3_LED_0 _________| |______________________________________
>>
>> : :
>> [15]..:
>>
>> [13] Port 4 LED Off. Port 3 LED On.
>> [14] Signals inverted. (reset_control_deassert?)
>> [15] Period of 310 usec.
>> [16] Signals reflect the bootstrapped configuration.
>>
>>
>> ~ # sed -n -e '$s/^.*\. //p' /sys/kernel/debug/tracing/trace
>> 3.437461: mt7530_setup: HWTRAP = 7dcf, HT_XTAL_FSEL = 40MHz
>>
>>
>> No difference is apparent in the timing diagram, compared
>> to the result from Test #1a, but it is obvious that the code
>> which follows the reset was executed about 1 second later.
>>
>>
>> (b) With a cable from an active peer connected to Port 3
>> (lan4) before reset, the correct crystal frequency
>> (0b10 = 40MHz) is selected:
>>
>> [17] [18] [20] [22]
>> : : : :
>> ______________________ \ \ ______
>> _______________
>> ESW_P4_LED_0 / / |______|
>> _________ \ \ ______
>> ESW_P3_LED_0 |____________ / / ______| |_______________
>> \ \
>> : : : :
>> [19]..................: [21].:
>>
>> [17] Port 4 LED Off. Port 3 LED Off.
>> [18] ESW_P3_LED_0 set low. (reset_control_assert?)
>> [19] Period of 1000.325 msec.
>> [20] Signals inverted. (reset_control_deassert?)
>> [21] Period of 310 usec.
>> [22] Signals reflect the bootstrapped configuration.
>>
>>
>> ~ # sed -n -e '$s/^.*\. //p' /sys/kernel/debug/tracing/trace
>> 3.433235: mt7530_setup: HWTRAP = 7dcf, HT_XTAL_FSEL = 40MHz
>>
>>
>> So it appears that when reset is deasserted, the ESW_P4_LED_0
>> and ESW_P3_LED_0 levels are inverted for a period of about
>> 300 - 310 usec in Test #1a, #1b, #2a, and #2b.
>>
>> Co-incidentally, these inverted levels are the active low
>> representation of what ends up in HT_XTAL_FSEL. And in all
>> four examples, have been the inversion of what immediately
>> preceded reset_control_deassert().
>>
>>
>> -- Test #3 -----------------------------------------------------------
>>
>> Now it seems that there is a signature that can be used
>> to identify when reset_control_deassert() is executed,
>> the time of reset_control_assert()'s execution should be
>> between (at most) 1100 and (at least) 1000 usec prior
>> based on the unmodified reset logic.
>>
>> So this patch only includes the HT_XTAL_FSEL trace.
>>
>> ---%---
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -2260,6 +2260,10 @@ mt7530_setup(struct dsa_switch *ds)
>> return ret;
>> }
>>
>> + static char ht_xtal_fsel[4][6] = { "?????", "20MHz", "40MHz",
>> "25MHz" };
>> + trace_printk("HWTRAP = %x, HT_XTAL_FSEL = %s\n",
>> + val, ht_xtal_fsel[(val & HWTRAP_XTAL_MASK) >> 9]);
>> +
>> id = mt7530_read(priv, MT7530_CREV);
>> id >>= CHIP_NAME_SHIFT;
>> if (id != MT7530_ID) {
>> ---%---
>>
>> The purpose of the following examples are to show the
>> variance in how long it takes for ESW_P3_LED_0 to go low.
>>
>> (a) With a cable from an active peer connected to Port 3
>> (lan4) before reset, the correct crystal frequency
>> (0b10 = 40MHz) is selected.
>>
>> [23] [24] [26] [28] [30]
>> : : : : :
>> _____________________________
>> __________________
>> ESW_P4_LED_0 |_______|
>> ___________________ _______
>> ESW_P3_LED_0 |_________| |__________________
>>
>> : : : : :
>> : [27]....: [29]..:
>> [25]...............:
>>
>> [23] Port 4 LED Off. Port 3 LED Off.
>> [24] Approximate point of reset_control_assert?
>> [25] Period of approximately 1000 - 1100 usec.
>> [26] ESW_P3_LED_0 finally goes low.
>> [27] Period of 415 usec.
>> [28] Signals inverted. (reset_control_deassert?)
>> [29] Period of 310 usec.
>> [30] Signals reflect the bootstrapped configuration.
>>
>>
>> (b) With a cable from an active peer connected to Port 3
>> (lan4) before reset, the correct crystal frequency
>> (0b10 = 40MHz) is selected.
>>
>> [31] [32] [34] [36] [38]
>> : : : : :
>> _____________________________
>> __________________
>> ESW_P4_LED_0 |_______|
>> ___________________________ _______
>> ESW_P3_LED_0 |_| |__________________
>>
>> : : : :
>> : [35] [37]..:
>> : :
>> [33]...............:
>>
>> [31] Port 4 LED Off. Port 3 LED Off.
>> [32] Approximate point of reset_control_assert?
>> [33] Period of approximately 1000 - 1100 usec.
>> [34] ESW_P3_LED_0 finally goes low.
>> [35] Period of 50 usec.
>> [36] Signals inverted. (reset_control_deassert?)
>> [37] Period of 310 usec.
>> [38] Signals reflect the bootstrapped configuration.
>>
>>
>> (c) With a cable from an active peer connected to Port 3
>> (lan4) before reset, an incorrect crystal frequency
>> (0b11 = 25MHz) is selected.
>>
>> [45] [46] [48] [50]
>> : : : :
>> _____________________________
>> __________________
>> ESW_P4_LED_0 |_______|
>> _____________________________
>> ESW_P3_LED_0 |__________________________
>>
>> : : : :
>> : : [49]..:
>> : :
>> [47]...............:
>>
>> [45] Port 4 LED Off. Port 3 LED Off.
>> [46] Approximate point of reset_control_assert?
>> [47] Period of approximately 1000 - 1100 usec.
>> [48] Signals inverted. (reset_control_deassert?)
>> [49] Period of 315 usec.
>> [50] Signals reflect the bootstrapped configuration.
>>
>> ~ # sed -n -e '$s/^.*\. //p' /sys/kernel/debug/tracing/trace
>> 2.617819: mt7530_setup: HWTRAP = 7fcf, HT_XTAL_FSEL = 25MHz
>>
>> ~ # cat /proc/cmdline
>> console=ttyS0,57600 loglevel=7 printk.time=1 root=/dev/ram0 debug
>> rdinit=/linuxrc
>>
>>
>> -- End of Tests ------------------------------------------------------
>>
>> It seems that the incorrect crystal frequency is selected more
>> often when debugging messages are present (such as printk()
>> based approaches) and especially when loglevel=7 and printk.time=1
>> are specified on the command line.
>>
>> For the sake of reference: I disabled ethernet support in my build
>> of (mainline) U-boot, and my kernel configuration is a very lean
>> selection of options suited for IP routing and a few peripherals
>> on the I2C and SPI buses.
>>
>> My userland is confined to an initramfs built around busybox.
>>
>> I also disable gmac1 because I need a few of the rgmii2 pins for
>> modem control signalling.
>>
>> Regards
>> Justin
>>
>> PS: Yes, I only have access to MT7621AT SoCs.
>
> Thanks for the detailed presentation. I've simplified it to this, from
> what
> I understood.
>
> --- Currently
> -------------------------------------------------------------
>
> With a cable from an active peer connected to Port 3 (lan4) before
> reset:
>
> Test 1, the correct crystal frequency (0b10 = 40MHz) is selected.
>
> [1] [2-1] [3] [5]
> : : : :
> _____________________________ __________________
> ESW_P4_LED_0 |_______|
> ___________________ _______
> ESW_P3_LED_0 |_________| |__________________
>
> : : : : :
> : [2-2]...: [4]...:
> [2]................:
>
> [1] reset_control_assert.
> [2] Period of 1000 - 1100 usec.
> [2-1] ESW_P3_LED_0 goes low.
> [2-2] Period of 415 usec.
> [3] reset_control_deassert.
> [4] Period of 310 usec. HWTRAP register is populated with bootstrapped
> XTAL frequency.
> [5] Signals reflect the bootstrapped configuration.
>
> Test 2, an incorrect crystal frequency (0b11 = 25MHz) is selected.
>
> [2] [4] [6]
> : : :
> _____________________________ __________________
> ESW_P4_LED_0 |_______|
> _____________________________
> ESW_P3_LED_0 |__________________________
>
> : : : :
> : : [5]...:
> : :
> [3]................:
>
> [1] reset_control_assert.
> [2] Period of 1000 - 1100 usec.
> [3] reset_control_deassert.
> [5] Period of 315 usec. HWTRAP register is populated with incorrect
> XTAL frequency.
> [6] Signals reflect the bootstrapped configuration.
>
> --- 1 second delay between assert and deassert
> ----------------------------
>
> With a cable from an active peer connected to Port 3 (lan4) before
> reset, the correct crystal frequency (0b10 = 40MHz) is selected:
>
> [1] [2-1] [3] [5]
> : : : :
> _____________________________ __________________
> ESW_P4_LED_0 |_______|
> ___________________ _______
> ESW_P3_LED_0 |_________| |__________________
>
> : : : : :
> : [2-2]...: [4]...:
> [2]................:
>
> [1] reset_control_assert.
> [3] Period of 1000.325 msec.
> [2-1] ESW_P3_LED_0 goes low.
> [2-2] Remaining period of 1000.325 msec.
> [3] reset_control_deassert.
> [4] Period of 310 usec. HWTRAP register is populated with bootstrapped
> XTAL frequency.
> [5] Signals reflect the bootstrapped configuration.
>
> ---
>
> Wouldn't it be a better idea to increase the delay between
> reset_control_assert() and reset_control_deassert(), and
> gpiod_set_value_cansleep(priv->reset, 0) and
> gpiod_set_value_cansleep(priv->reset, 1) instead of disabling the LED
> controller and delaying before reset?
I've done some additional tests to see what the difference would be
between increasing the reset hold period vs. disabling the LEDs then
sleeping before reset.
I wanted to know:
When an active link is present on Port 3 at boot, what are the
minimum, maximum and average periods between ESW_P3_LED_0
going low and the signal inversion that seems to co-incide with
reset_control_deassert() for each approach?
I created two patches:
WITH INCREASED RESET DELAY
As I submitted a patch that added an intended 1000 - 1100 usec delay
before reset, I thought it would be fair to increase the reset hold
period by the same value.
---%---
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2243,11 +2243,11 @@ mt7530_setup(struct dsa_switch *ds)
*/
if (priv->mcm) {
reset_control_assert(priv->rstc);
- usleep_range(1000, 1100);
+ usleep_range(2000, 2200);
reset_control_deassert(priv->rstc);
} else {
gpiod_set_value_cansleep(priv->reset, 0);
- usleep_range(1000, 1100);
+ usleep_range(2000, 2200);
gpiod_set_value_cansleep(priv->reset, 1);
}
---%---
DISABLE LEDS BEFORE RESET
Reset hold period unchanged from the intended 1000 - 1100 usec.
---%---
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2238,6 +2238,12 @@ mt7530_setup(struct dsa_switch *ds)
}
}
+ /* Disable LEDs before reset to prevent the MT7530 sampling a
+ * potentially incorrect HT_XTAL_FSEL value.
+ */
+ mt7530_write(priv, MT7530_LED_EN, 0);
+ usleep_range(1000, 1100);
+
/* Reset whole chip through gpio pin or memory-mapped registers
for
* different type of hardware
*/
---%---
I ran 20 tests per patch, applied exclusively. 40 tests in total.
<-- ESW_P3_LED_0 Low Period before Reset Deassertion -->
TEST WITH INCREASED RESET DELAY DISABLE LEDS BEFORE RESET
# (usec) (usec)
-------------------------------------------------------------------
1 182 4790
2 370 3470
3 240 4635
4 1475 4850
5 70 4775
6 2730 4575
7 3180 4565
8 265 5650
9 270 4690
10 1525 4450
11 3210 4735
12 120 4690
13 185 4625
14 305 7020
15 2975 4720
16 245 4675
17 350 4740
18 80 17920
19 150 17665
20 100 4620
Min 70 3470
Max 3210 17920
Mean 270 4720
Avg 923.421 6161.579
Every test resulted in a 40MHz HT_XTAL_FSEL, but after seeing 70 usec
and 80 usec periods I wondered how many more tests it may take before
an 25MHz HT_XTAL_FSEL appears.
I was also surprised by the 17920 usec and 17665 usec periods listed
under the DISABLED LEDS BEFORE RESET column. Nothing unusual seemed
to be happening, at least as far as the kernel message output was
concerned.
What do you make of these results?
> One MT7531 pin used for an LED is also used to decide the crystal
> frequency
> between 40MHz and 25Mhz. We should implement this there as well.
>
> Arınç
On 12.03.2024 15:01, Justin Swartz wrote:
> Arınç
>
> On 2024-03-12 04:41, Arınç ÜNAL wrote:
>> Wouldn't it be a better idea to increase the delay between
>> reset_control_assert() and reset_control_deassert(), and
>> gpiod_set_value_cansleep(priv->reset, 0) and
>> gpiod_set_value_cansleep(priv->reset, 1) instead of disabling the LED
>> controller and delaying before reset?
>
> I've done some additional tests to see what the difference would be
> between increasing the reset hold period vs. disabling the LEDs then
> sleeping before reset.
>
>
> I wanted to know:
>
> When an active link is present on Port 3 at boot, what are the
> minimum, maximum and average periods between ESW_P3_LED_0
> going low and the signal inversion that seems to co-incide with
> reset_control_deassert() for each approach?
>
>
> I created two patches:
>
> WITH INCREASED RESET DELAY
>
> As I submitted a patch that added an intended 1000 - 1100 usec delay
> before reset, I thought it would be fair to increase the reset hold
> period by the same value.
>
> ---%---
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2243,11 +2243,11 @@ mt7530_setup(struct dsa_switch *ds)
> */
> if (priv->mcm) {
> reset_control_assert(priv->rstc);
> - usleep_range(1000, 1100);
> + usleep_range(2000, 2200);
> reset_control_deassert(priv->rstc);
> } else {
> gpiod_set_value_cansleep(priv->reset, 0);
> - usleep_range(1000, 1100);
> + usleep_range(2000, 2200);
> gpiod_set_value_cansleep(priv->reset, 1);
> }
> ---%---
>
>
> DISABLE LEDS BEFORE RESET
>
> Reset hold period unchanged from the intended 1000 - 1100 usec.
>
> ---%---
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2238,6 +2238,12 @@ mt7530_setup(struct dsa_switch *ds)
> }
> }
>
> + /* Disable LEDs before reset to prevent the MT7530 sampling a
> + * potentially incorrect HT_XTAL_FSEL value.
> + */
> + mt7530_write(priv, MT7530_LED_EN, 0);
> + usleep_range(1000, 1100);
> +
> /* Reset whole chip through gpio pin or memory-mapped registers for
> * different type of hardware
> */
> ---%---
>
>
> I ran 20 tests per patch, applied exclusively. 40 tests in total.
>
> <-- ESW_P3_LED_0 Low Period before Reset Deassertion -->
>
> TEST WITH INCREASED RESET DELAY DISABLE LEDS BEFORE RESET
> # (usec) (usec)
> -------------------------------------------------------------------
> 1 182 4790
> 2 370 3470
> 3 240 4635
> 4 1475 4850
> 5 70 4775
> 6 2730 4575
> 7 3180 4565
> 8 265 5650
> 9 270 4690
> 10 1525 4450
> 11 3210 4735
> 12 120 4690
> 13 185 4625
> 14 305 7020
> 15 2975 4720
> 16 245 4675
> 17 350 4740
> 18 80 17920
> 19 150 17665
> 20 100 4620
>
> Min 70 3470
> Max 3210 17920
>
> Mean 270 4720
> Avg 923.421 6161.579
>
>
> Every test resulted in a 40MHz HT_XTAL_FSEL, but after seeing 70 usec
> and 80 usec periods I wondered how many more tests it may take before
> an 25MHz HT_XTAL_FSEL appears.
>
> I was also surprised by the 17920 usec and 17665 usec periods listed
> under the DISABLED LEDS BEFORE RESET column. Nothing unusual seemed
> to be happening, at least as far as the kernel message output was
> concerned.
>
> What do you make of these results?
What I see is setting ESW_P3_LED_0 low via reset assertion is much more
efficient than doing so by setting the LED controller off. So I'd prefer
increasing the delay between assertion and reassertion. For example, the
Realtek DSA subdriver has a much more generous delay. 25ms after reset
assertion [1].
Looking at your results, 2000 - 2200 usec delay still seems too close, so
let's agree on an amount that will ensure that boards in any circumstances
will have these pins back to the bootstrapping configuration before reset
deassertion. What about 5000 - 5100 usec?
[1] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/realtek/rtl83xx.c#n205
Arınç
On 2024-03-12 16:06, Arınç ÜNAL wrote:
> On 12.03.2024 15:01, Justin Swartz wrote:
>> Arınç
>>
>> On 2024-03-12 04:41, Arınç ÜNAL wrote:
>>> Wouldn't it be a better idea to increase the delay between
>>> reset_control_assert() and reset_control_deassert(), and
>>> gpiod_set_value_cansleep(priv->reset, 0) and
>>> gpiod_set_value_cansleep(priv->reset, 1) instead of disabling the LED
>>> controller and delaying before reset?
>>
>> I've done some additional tests to see what the difference would be
>> between increasing the reset hold period vs. disabling the LEDs then
>> sleeping before reset.
>>
>>
>> I wanted to know:
>>
>> When an active link is present on Port 3 at boot, what are the
>> minimum, maximum and average periods between ESW_P3_LED_0
>> going low and the signal inversion that seems to co-incide with
>> reset_control_deassert() for each approach?
>>
>>
>> I created two patches:
>>
>> WITH INCREASED RESET DELAY
>>
>> As I submitted a patch that added an intended 1000 - 1100 usec delay
>> before reset, I thought it would be fair to increase the reset hold
>> period by the same value.
>>
>> ---%---
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -2243,11 +2243,11 @@ mt7530_setup(struct dsa_switch *ds)
>> */
>> if (priv->mcm) {
>> reset_control_assert(priv->rstc);
>> - usleep_range(1000, 1100);
>> + usleep_range(2000, 2200);
>> reset_control_deassert(priv->rstc);
>> } else {
>> gpiod_set_value_cansleep(priv->reset, 0);
>> - usleep_range(1000, 1100);
>> + usleep_range(2000, 2200);
>> gpiod_set_value_cansleep(priv->reset, 1);
>> }
>> ---%---
>>
>>
>> DISABLE LEDS BEFORE RESET
>>
>> Reset hold period unchanged from the intended 1000 - 1100 usec.
>>
>> ---%---
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -2238,6 +2238,12 @@ mt7530_setup(struct dsa_switch *ds)
>> }
>> }
>>
>> + /* Disable LEDs before reset to prevent the MT7530 sampling a
>> + * potentially incorrect HT_XTAL_FSEL value.
>> + */
>> + mt7530_write(priv, MT7530_LED_EN, 0);
>> + usleep_range(1000, 1100);
>> +
>> /* Reset whole chip through gpio pin or memory-mapped
>> registers for
>> * different type of hardware
>> */
>> ---%---
>>
>>
>> I ran 20 tests per patch, applied exclusively. 40 tests in total.
>>
>> <-- ESW_P3_LED_0 Low Period before Reset Deassertion -->
>>
>> TEST WITH INCREASED RESET DELAY DISABLE LEDS BEFORE RESET
>> # (usec) (usec)
>> -------------------------------------------------------------------
>> 1 182 4790
>> 2 370 3470
>> 3 240 4635
>> 4 1475 4850
>> 5 70 4775
>> 6 2730 4575
>> 7 3180 4565
>> 8 265 5650
>> 9 270 4690
>> 10 1525 4450
>> 11 3210 4735
>> 12 120 4690
>> 13 185 4625
>> 14 305 7020
>> 15 2975 4720
>> 16 245 4675
>> 17 350 4740
>> 18 80 17920
>> 19 150 17665
>> 20 100 4620
>>
>> Min 70 3470
>> Max 3210 17920
>>
>> Mean 270 4720
-1s/ Mean/Median/
>> Avg 923.421 6161.579
>>
>>
>> Every test resulted in a 40MHz HT_XTAL_FSEL, but after seeing 70 usec
>> and 80 usec periods I wondered how many more tests it may take before
>> an 25MHz HT_XTAL_FSEL appears.
>>
>> I was also surprised by the 17920 usec and 17665 usec periods listed
>> under the DISABLED LEDS BEFORE RESET column. Nothing unusual seemed
>> to be happening, at least as far as the kernel message output was
>> concerned.
>>
>> What do you make of these results?
>
> What I see is setting ESW_P3_LED_0 low via reset assertion is much more
> efficient than doing so by setting the LED controller off. So I'd
> prefer
> increasing the delay between assertion and reassertion. For example,
> the
> Realtek DSA subdriver has a much more generous delay. 25ms after reset
> assertion [1].
>
> Looking at your results, 2000 - 2200 usec delay still seems too close,
> so
> let's agree on an amount that will ensure that boards in any
> circumstances
> will have these pins back to the bootstrapping configuration before
> reset
> deassertion. What about 5000 - 5100 usec?
TEST ESW_P3_LED_0 LOW PERIOD
# (usec)
----------------------------------
1 5410
2 5440
3 4375
4 5490
5 5475
6 4335
7 4370
8 5435
9 4205
10 4335
11 3750
12 3170
13 4395
14 4375
15 3515
16 4335
17 4220
18 4175
19 4175
20 4350
Min 3170
Max 5490
Median 4342.500
Avg 4466.500
Looks reasonable to me.
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/realtek/rtl83xx.c#n205
>
> Arınç
On 2024-03-12 17:25, Justin Swartz wrote:
> On 2024-03-12 16:06, Arınç ÜNAL wrote:
>> On 12.03.2024 15:01, Justin Swartz wrote:
>>> Arınç
>>>
>>> On 2024-03-12 04:41, Arınç ÜNAL wrote:
>>>> Wouldn't it be a better idea to increase the delay between
>>>> reset_control_assert() and reset_control_deassert(), and
>>>> gpiod_set_value_cansleep(priv->reset, 0) and
>>>> gpiod_set_value_cansleep(priv->reset, 1) instead of disabling the
>>>> LED
>>>> controller and delaying before reset?
>>>
>>> I've done some additional tests to see what the difference would be
>>> between increasing the reset hold period vs. disabling the LEDs then
>>> sleeping before reset.
>>>
>>>
>>> I wanted to know:
>>>
>>> When an active link is present on Port 3 at boot, what are the
>>> minimum, maximum and average periods between ESW_P3_LED_0
>>> going low and the signal inversion that seems to co-incide with
>>> reset_control_deassert() for each approach?
>>>
>>>
>>> I created two patches:
>>>
>>> WITH INCREASED RESET DELAY
>>>
>>> As I submitted a patch that added an intended 1000 - 1100 usec delay
>>> before reset, I thought it would be fair to increase the reset hold
>>> period by the same value.
>>>
>>> ---%---
>>> --- a/drivers/net/dsa/mt7530.c
>>> +++ b/drivers/net/dsa/mt7530.c
>>> @@ -2243,11 +2243,11 @@ mt7530_setup(struct dsa_switch *ds)
>>> */
>>> if (priv->mcm) {
>>> reset_control_assert(priv->rstc);
>>> - usleep_range(1000, 1100);
>>> + usleep_range(2000, 2200);
>>> reset_control_deassert(priv->rstc);
>>> } else {
>>> gpiod_set_value_cansleep(priv->reset, 0);
>>> - usleep_range(1000, 1100);
>>> + usleep_range(2000, 2200);
>>> gpiod_set_value_cansleep(priv->reset, 1);
>>> }
>>> ---%---
>>>
>>>
>>> DISABLE LEDS BEFORE RESET
>>>
>>> Reset hold period unchanged from the intended 1000 - 1100 usec.
>>>
>>> ---%---
>>> --- a/drivers/net/dsa/mt7530.c
>>> +++ b/drivers/net/dsa/mt7530.c
>>> @@ -2238,6 +2238,12 @@ mt7530_setup(struct dsa_switch *ds)
>>> }
>>> }
>>>
>>> + /* Disable LEDs before reset to prevent the MT7530 sampling a
>>> + * potentially incorrect HT_XTAL_FSEL value.
>>> + */
>>> + mt7530_write(priv, MT7530_LED_EN, 0);
>>> + usleep_range(1000, 1100);
>>> +
>>> /* Reset whole chip through gpio pin or memory-mapped
>>> registers for
>>> * different type of hardware
>>> */
>>> ---%---
>>>
>>>
>>> I ran 20 tests per patch, applied exclusively. 40 tests in total.
>>>
>>> <-- ESW_P3_LED_0 Low Period before Reset Deassertion -->
>>>
>>> TEST WITH INCREASED RESET DELAY DISABLE LEDS BEFORE RESET
>>> # (usec) (usec)
>>> -------------------------------------------------------------------
>>> 1 182 4790
>>> 2 370 3470
>>> 3 240 4635
>>> 4 1475 4850
>>> 5 70 4775
>>> 6 2730 4575
>>> 7 3180 4565
>>> 8 265 5650
>>> 9 270 4690
>>> 10 1525 4450
>>> 11 3210 4735
>>> 12 120 4690
>>> 13 185 4625
>>> 14 305 7020
>>> 15 2975 4720
>>> 16 245 4675
>>> 17 350 4740
>>> 18 80 17920
>>> 19 150 17665
>>> 20 100 4620
>>>
>>> Min 70 3470
>>> Max 3210 17920
>>>
>>> Mean 270 4720
> -1s/ Mean/Median/
>
>>> Avg 923.421 6161.579
>>>
>>>
>>> Every test resulted in a 40MHz HT_XTAL_FSEL, but after seeing 70 usec
>>> and 80 usec periods I wondered how many more tests it may take before
>>> an 25MHz HT_XTAL_FSEL appears.
>>>
>>> I was also surprised by the 17920 usec and 17665 usec periods listed
>>> under the DISABLED LEDS BEFORE RESET column. Nothing unusual seemed
>>> to be happening, at least as far as the kernel message output was
>>> concerned.
>>>
>>> What do you make of these results?
>>
>> What I see is setting ESW_P3_LED_0 low via reset assertion is much
>> more
>> efficient than doing so by setting the LED controller off. So I'd
>> prefer
>> increasing the delay between assertion and reassertion. For example,
>> the
>> Realtek DSA subdriver has a much more generous delay. 25ms after reset
>> assertion [1].
>>
>> Looking at your results, 2000 - 2200 usec delay still seems too close,
>> so
>> let's agree on an amount that will ensure that boards in any
>> circumstances
>> will have these pins back to the bootstrapping configuration before
>> reset
>> deassertion. What about 5000 - 5100 usec?
>
> TEST ESW_P3_LED_0 LOW PERIOD
> # (usec)
> ----------------------------------
> 1 5410
> 2 5440
> 3 4375
> 4 5490
> 5 5475
> 6 4335
> 7 4370
> 8 5435
> 9 4205
> 10 4335
> 11 3750
> 12 3170
> 13 4395
> 14 4375
> 15 3515
> 16 4335
> 17 4220
> 18 4175
> 19 4175
> 20 4350
>
> Min 3170
> Max 5490
>
> Median 4342.500
> Avg 4466.500
>
> Looks reasonable to me.
I had messed up the Median/Average calculation with the data
I had pasted earlier after copying and pasting formulas without
double checking how they had been affected.
So here's the table again, now with the 5000 - 5100 usec test
run tacked on for easier comparison:
2000 usec 5000 usec
- 2200 usec DISABLE LEDS - 5100 usec
TEST RESET HOLD BEFORE RESET RESET HOLD
# (usec) (usec) (usec)
--------------------------------------------------------
1 182 4790 5410
2 370 3470 5440
3 240 4635 4375
4 1475 4850 5490
5 70 4775 5475
6 2730 4575 4335
7 3180 4565 4370
8 265 5650 5435
9 270 4690 4205
10 1525 4450 4335
11 3210 4735 3750
12 120 4690 3170
13 185 4625 4395
14 305 7020 4375
15 2975 4720 3515
16 245 4675 4335
17 350 4740 4220
18 80 17920 4175
19 150 17665 4175
20 100 4620 4350
Min 70 3470 3170
Max 3210 17920 5490
Median 267.500 4705 4342.500
Avg 901.350 6093 4466.500
>> [1]
>> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/realtek/rtl83xx.c#n205
>>
>> Arınç
Increase the MT7530 reset hold period from 1000-1100 usec
to 5000-5100 usec.
This should reduce the likelihood of an incorrect external
crystal frequency selection which may occur when reset is
deasserted too early under certain link conditions.
Signed-off-by: Justin Swartz <[email protected]>
---
drivers/net/dsa/mt7530.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 3c1f65759..5e9e1381a 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2243,11 +2243,11 @@ mt7530_setup(struct dsa_switch *ds)
*/
if (priv->mcm) {
reset_control_assert(priv->rstc);
- usleep_range(1000, 1100);
+ usleep_range(5000, 5100);
reset_control_deassert(priv->rstc);
} else {
gpiod_set_value_cansleep(priv->reset, 0);
- usleep_range(1000, 1100);
+ usleep_range(5000, 5100);
gpiod_set_value_cansleep(priv->reset, 1);
}
--
On 12.03.2024 22:21, Justin Swartz wrote:
> Increase the MT7530 reset hold period from 1000-1100 usec
> to 5000-5100 usec.
>
> This should reduce the likelihood of an incorrect external
> crystal frequency selection which may occur when reset is
> deasserted too early under certain link conditions.
>
> Signed-off-by: Justin Swartz <[email protected]>
> ---
> drivers/net/dsa/mt7530.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 3c1f65759..5e9e1381a 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2243,11 +2243,11 @@ mt7530_setup(struct dsa_switch *ds)
> */
> if (priv->mcm) {
> reset_control_assert(priv->rstc);
> - usleep_range(1000, 1100);
> + usleep_range(5000, 5100);
> reset_control_deassert(priv->rstc);
> } else {
> gpiod_set_value_cansleep(priv->reset, 0);
> - usleep_range(1000, 1100);
> + usleep_range(5000, 5100);
> gpiod_set_value_cansleep(priv->reset, 1);
> }
>
Again, the patch subject is missing the indication of a tree. Read the
networking subsystem (netdev) development page [1].
This ship has sailed anyway. Now the changes the first patch did must be
reverted too. I will deal with this from now on, you can stop sending
patches regarding this.
[1] https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html
Arınç
On 2024-03-13 10:59, Arınç ÜNAL wrote:
> On 12.03.2024 22:21, Justin Swartz wrote:
>> Increase the MT7530 reset hold period from 1000-1100 usec
>> to 5000-5100 usec.
>>
>> This should reduce the likelihood of an incorrect external
>> crystal frequency selection which may occur when reset is
>> deasserted too early under certain link conditions.
>>
>> Signed-off-by: Justin Swartz <[email protected]>
>> ---
>> drivers/net/dsa/mt7530.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index 3c1f65759..5e9e1381a 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -2243,11 +2243,11 @@ mt7530_setup(struct dsa_switch *ds)
>> */
>> if (priv->mcm) {
>> reset_control_assert(priv->rstc);
>> - usleep_range(1000, 1100);
>> + usleep_range(5000, 5100);
>> reset_control_deassert(priv->rstc);
>> } else {
>> gpiod_set_value_cansleep(priv->reset, 0);
>> - usleep_range(1000, 1100);
>> + usleep_range(5000, 5100);
>> gpiod_set_value_cansleep(priv->reset, 1);
>> }
>>
>
> Again, the patch subject is missing the indication of a tree. Read the
> networking subsystem (netdev) development page [1].
Thanks for that pointer.
> This ship has sailed anyway. Now the changes the first patch did must
> be
> reverted too. I will deal with this from now on, you can stop sending
> patches regarding this.
At least if the first patch isn't reverted, the approach used is
less likely to result in the problem occuring, IMHO.
The delay between deliberately switching the LEDs off, instead of
only waiting on chip reset logic to handle that, and the reset
assertion could be considered a "reset setup" period to complement
the original reset hold period.
Increasing the hold period to what should be 5000 - 5100 usec,
definitely made the problem go away my testing, but the previous
approach is (if nothing else) more explicit in its intent.
> [1]
> https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html
>
> Arınç
On 13.03.2024 14:52, Justin Swartz wrote:
>
> On 2024-03-13 10:59, Arınç ÜNAL wrote:
>> This ship has sailed anyway. Now the changes the first patch did must be
>> reverted too. I will deal with this from now on, you can stop sending
>> patches regarding this.
>
> At least if the first patch isn't reverted, the approach used is
> less likely to result in the problem occuring, IMHO.
I disagree that the previous approach is less likely to result in the
problem occurring. If you don't like the delay amount we agreed on, feel
free to express a higher amount.
I also disagree on introducing a solution that is in addition to another
solution, both of which fix the same problem.
>
> The delay between deliberately switching the LEDs off, instead of
> only waiting on chip reset logic to handle that, and the reset
> assertion could be considered a "reset setup" period to complement
> the original reset hold period.
>
> Increasing the hold period to what should be 5000 - 5100 usec,
> definitely made the problem go away my testing, but the previous
> approach is (if nothing else) more explicit in its intent.
I don't want any unnecessary complications on the code I'm maintaining. I
already gave a clear intent on the patch log that introduces a simpler and
more efficient approach, it doesn't need to be on the code.
Arınç
On 2024-03-13 14:06, Arınç ÜNAL wrote:
> On 13.03.2024 14:52, Justin Swartz wrote:
>>
>> On 2024-03-13 10:59, Arınç ÜNAL wrote:
>>> This ship has sailed anyway. Now the changes the first patch did must
>>> be
>>> reverted too. I will deal with this from now on, you can stop sending
>>> patches regarding this.
>>
>> At least if the first patch isn't reverted, the approach used is
>> less likely to result in the problem occuring, IMHO.
>
> I disagree that the previous approach is less likely to result in the
> problem occurring. If you don't like the delay amount we agreed on,
> feel
> free to express a higher amount.
I created and tested a patch to entertain your input about what you
thought could be a suitable hold period to address the problem, and it
appeared to work. The criteria being that the crystal frequency
selection
was correct over 20 tests.
So if only the reset hold period is going to change, I'm good with what
you had suggested: 5000 - 5100 usec.
Ultimately the selection of this period should be guided by the timing
information provided in a datasheet or design guide from the
manufacturer.
If you, or anyone else, has such a document that provides this
information
and is able to confirm or deny speculation about any/all timing periods
related to reset, please do so.
> I also disagree on introducing a solution that is in addition to
> another
> solution, both of which fix the same problem.
I'm not sure I understand why you say that. These patches were intended
to be applied exclusively, or in other words: in isolation - not
together.
Although if they were applied together, it wouldn't really matter.
For the record, I've only continued to keep this thread alive in the
hope that some solution to this problem will make it into mainline
eventually.
I don't care if it was my original patch, the subsequent patch, or a
later patch provided by you or someone else. :)
>>
>> The delay between deliberately switching the LEDs off, instead of
>> only waiting on chip reset logic to handle that, and the reset
>> assertion could be considered a "reset setup" period to complement
>> the original reset hold period.
>>
>> Increasing the hold period to what should be 5000 - 5100 usec,
>> definitely made the problem go away my testing, but the previous
>> approach is (if nothing else) more explicit in its intent.
>
> I don't want any unnecessary complications on the code I'm maintaining.
> I
> already gave a clear intent on the patch log that introduces a simpler
> and
> more efficient approach, it doesn't need to be on the code.
>
> Arınç
Kind Regards
Justin
On 13.03.2024 16:13, Justin Swartz wrote:
> On 2024-03-13 14:06, Arınç ÜNAL wrote:
>> On 13.03.2024 14:52, Justin Swartz wrote:
>>>
>>> On 2024-03-13 10:59, Arınç ÜNAL wrote:
>>>> This ship has sailed anyway. Now the changes the first patch did must be
>>>> reverted too. I will deal with this from now on, you can stop sending
>>>> patches regarding this.
>>>
>>> At least if the first patch isn't reverted, the approach used is
>>> less likely to result in the problem occuring, IMHO.
>>
>> I disagree that the previous approach is less likely to result in the
>> problem occurring. If you don't like the delay amount we agreed on, feel
>> free to express a higher amount.
>
> I created and tested a patch to entertain your input about what you
> thought could be a suitable hold period to address the problem, and it
> appeared to work. The criteria being that the crystal frequency selection
> was correct over 20 tests.
>
> So if only the reset hold period is going to change, I'm good with what
> you had suggested: 5000 - 5100 usec.
>
> Ultimately the selection of this period should be guided by the timing
> information provided in a datasheet or design guide from the manufacturer.
That's a good point, I agree.
>
> If you, or anyone else, has such a document that provides this information
> and is able to confirm or deny speculation about any/all timing periods
> related to reset, please do so.
These are the documents I use to program this switch family. I did not
stumble upon a page going over this.
MT7621 Giga Switch Programming Guide v0.3:
https://github.com/vschagen/documents/blob/main/MT7621_ProgrammingGuide_GSW_v0_3.pdf
MT7531 switch chip datasheet:
https://wiki.banana-pi.org/Banana_Pi_BPI-R64#Documents
>
>
>> I also disagree on introducing a solution that is in addition to another
>> solution, both of which fix the same problem.
>
> I'm not sure I understand why you say that. These patches were intended
> to be applied exclusively, or in other words: in isolation - not together.
>
> Although if they were applied together, it wouldn't really matter.
>
> For the record, I've only continued to keep this thread alive in the
> hope that some solution to this problem will make it into mainline
> eventually.
>
> I don't care if it was my original patch, the subsequent patch, or a
> later patch provided by you or someone else. :)
I think you've missed that your patch is already applied. And it won't be
reverted for reasons explained by Paolo in this mail thread.
https://git.kernel.org/netdev/net-next/c/2920dd92b980
So if your patch here were to be applied too, the final mt7530.c would have
the LEDs disabled AND before reset deassertion delay increased.
Arınç
On 2024-03-13 17:04, Arınç ÜNAL wrote:
> On 13.03.2024 16:13, Justin Swartz wrote:
>> On 2024-03-13 14:06, Arınç ÜNAL wrote:
>>> On 13.03.2024 14:52, Justin Swartz wrote:
>>>>
>>>> On 2024-03-13 10:59, Arınç ÜNAL wrote:
>>>>> This ship has sailed anyway. Now the changes the first patch did
>>>>> must be
>>>>> reverted too. I will deal with this from now on, you can stop
>>>>> sending
>>>>> patches regarding this.
>>>>
>>>> At least if the first patch isn't reverted, the approach used is
>>>> less likely to result in the problem occuring, IMHO.
>>>
>>> I disagree that the previous approach is less likely to result in the
>>> problem occurring. If you don't like the delay amount we agreed on,
>>> feel
>>> free to express a higher amount.
>>
>> I created and tested a patch to entertain your input about what you
>> thought could be a suitable hold period to address the problem, and it
>> appeared to work. The criteria being that the crystal frequency
>> selection
>> was correct over 20 tests.
>>
>> So if only the reset hold period is going to change, I'm good with
>> what
>> you had suggested: 5000 - 5100 usec.
>>
>> Ultimately the selection of this period should be guided by the timing
>> information provided in a datasheet or design guide from the
>> manufacturer.
>
> That's a good point, I agree.
>
>>
>> If you, or anyone else, has such a document that provides this
>> information
>> and is able to confirm or deny speculation about any/all timing
>> periods
>> related to reset, please do so.
>
> These are the documents I use to program this switch family. I did not
> stumble upon a page going over this.
>
> MT7621 Giga Switch Programming Guide v0.3:
>
> https://github.com/vschagen/documents/blob/main/MT7621_ProgrammingGuide_GSW_v0_3.pdf
>
> MT7531 switch chip datasheet:
>
> https://wiki.banana-pi.org/Banana_Pi_BPI-R64#Documents
Thanks for the links.
Have you encountered this one?
MT7530 Giga Switch Programming Guide v0.1:
https://github.com/libc0607/arduino_mt7530/blob/master/MT7530_programming_guide_V00.pdf
It has some timing diagrams, but nothing I found for reset.
The HWTRAP description has some bits swapped, so I guess those were
typos,
in the case of XTAL_SELECT.
>>> I also disagree on introducing a solution that is in addition to
>>> another
>>> solution, both of which fix the same problem.
>>
>> I'm not sure I understand why you say that. These patches were
>> intended
>> to be applied exclusively, or in other words: in isolation - not
>> together.
>>
>> Although if they were applied together, it wouldn't really matter.
>>
>> For the record, I've only continued to keep this thread alive in the
>> hope that some solution to this problem will make it into mainline
>> eventually.
>>
>> I don't care if it was my original patch, the subsequent patch, or a
>> later patch provided by you or someone else. :)
>
> I think you've missed that your patch is already applied. And it won't
> be
> reverted for reasons explained by Paolo in this mail thread.
>
> https://git.kernel.org/netdev/net-next/c/2920dd92b980
>
> So if your patch here were to be applied too, the final mt7530.c would
> have
> the LEDs disabled AND before reset deassertion delay increased.
Yes, I seem to have missed that. I thought your request for the
patch to be reverted definitely would have been performed, or at
least queued, seeing as you're the maintainer.
On 13.03.2024 18:38, Justin Swartz wrote:
> On 2024-03-13 17:04, Arınç ÜNAL wrote:
>> On 13.03.2024 16:13, Justin Swartz wrote:
>> I think you've missed that your patch is already applied. And it won't be
>> reverted for reasons explained by Paolo in this mail thread.
>>
>> https://git.kernel.org/netdev/net-next/c/2920dd92b980
>>
>> So if your patch here were to be applied too, the final mt7530.c would have
>> the LEDs disabled AND before reset deassertion delay increased.
>
> Yes, I seem to have missed that. I thought your request for the
> patch to be reverted definitely would have been performed, or at
> least queued, seeing as you're the maintainer.
Yeah, one would think. :D Since your patch was applied in a good intent of
not having it miss the current development cycle, it was a bit rushed. So
before I could present a valid reason to revert the patch, the pull request
that included your patch was already submitted to Linus. So unless the
patch is something very bad which it's not, nobody's going to bother
reverting it.
I've sent another patch an hour or so ago that reverts it and implements
what we've discussed here. I will also make sure it is applied to stable
trees.
https://lore.kernel.org/all/20240313-for-netnext-mt7530-better-fix-xtal-frequency-v1-1-5a50df99f51a@arinc9.com/
Arınç