drivers/net/dsa/mt7530.c | 6 ++++++ 1 file changed, 6 insertions(+)
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 <justin.swartz@risingedge.co.za>
---
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 <justin.swartz@risingedge.co.za> > --- > 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 > */
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 <justin.swartz@risingedge.co.za> >> --- >> 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 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ç
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 <justin.swartz@risingedge.co.za>
---
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 <justin.swartz@risingedge.co.za> > --- > 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 <justin.swartz@risingedge.co.za> >> --- >> 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ç
> (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 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 <justin.swartz@risingedge.co.za> > > --- > > 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 > > */
© 2016 - 2024 Red Hat, Inc.