Messages in this thread | | | Date | Thu, 8 Feb 2024 22:35:53 +0300 | Subject | Re: [PATCH netnext 8/8] net: dsa: mt7530: simplify link operations and force link down on all ports | From | Arınç ÜNAL <> |
| |
On 8.02.2024 19:22, Daniel Golle wrote: > On Thu, Feb 08, 2024 at 08:51:36AM +0300, Arınç ÜNAL via B4 Relay wrote: >> From: Arınç ÜNAL <arinc.unal@arinc9.com> >> >> Currently, the link operations for switch MACs are scattered across >> port_enable, port_disable, phylink_mac_config, phylink_mac_link_up, and >> phylink_mac_link_down. >> >> port_enable and port_disable clears the link settings. Move that to >> mt7530_setup() and mt7531_setup_common() which set up the switches. This >> way, the link settings are cleared on all ports at setup, and then only >> once with phylink_mac_link_down() when a link goes down. >> >> Enable force mode at setup to apply the force part of the link settings. >> This ensures that only active ports will have their link up. >> >> Now that the bit for setting the port on force mode is done on >> mt7530_setup() and mt7531_setup_common(), get rid of PMCR_FORCE_MODE_ID() >> which helped determine which bit to use for the switch model. >> >> The "MT7621 Giga Switch Programming Guide v0.3", "MT7531 Reference Manual >> for Development Board v1.0", and "MT7988A Wi-Fi 7 Generation Router >> Platform: Datasheet (Open Version) v0.1" documents show that these bits are >> enabled at reset: >> >> PMCR_IFG_XMIT(1) (not part of PMCR_LINK_SETTINGS_MASK) >> PMCR_MAC_MODE (not part of PMCR_LINK_SETTINGS_MASK) >> PMCR_TX_EN >> PMCR_RX_EN >> PMCR_BACKOFF_EN (not part of PMCR_LINK_SETTINGS_MASK) >> PMCR_BACKPR_EN (not part of PMCR_LINK_SETTINGS_MASK) >> PMCR_TX_FC_EN >> PMCR_RX_FC_EN >> >> These bits also don't exist on the MT7530_PMCR_P(6) register of the switch >> on the MT7988 SoC: >> >> PMCR_IFG_XMIT() >> PMCR_MAC_MODE >> PMCR_BACKOFF_EN >> PMCR_BACKPR_EN >> >> Remove the setting of the bits not part of PMCR_LINK_SETTINGS_MASK on >> phylink_mac_config as they're already set. >> >> Suggested-by: Vladimir Oltean <olteanv@gmail.com> >> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com> >> --- >> drivers/net/dsa/mt7530.c | 26 +++++++++++++------------- >> drivers/net/dsa/mt7530.h | 2 -- >> 2 files changed, 13 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c >> index 5c8ad41ce8cd..f67db577d1c0 100644 >> --- a/drivers/net/dsa/mt7530.c >> +++ b/drivers/net/dsa/mt7530.c >> @@ -1018,7 +1018,6 @@ mt7530_port_enable(struct dsa_switch *ds, int port, >> priv->ports[port].enable = true; >> mt7530_rmw(priv, MT7530_PCR_P(port), PCR_MATRIX_MASK, >> priv->ports[port].pm); >> - mt7530_clear(priv, MT7530_PMCR_P(port), PMCR_LINK_SETTINGS_MASK); >> >> mutex_unlock(&priv->reg_mutex); >> >> @@ -1038,7 +1037,6 @@ mt7530_port_disable(struct dsa_switch *ds, int port) >> priv->ports[port].enable = false; >> mt7530_rmw(priv, MT7530_PCR_P(port), PCR_MATRIX_MASK, >> PCR_MATRIX_CLR); >> - mt7530_clear(priv, MT7530_PMCR_P(port), PMCR_LINK_SETTINGS_MASK); >> >> mutex_unlock(&priv->reg_mutex); >> } >> @@ -2257,6 +2255,12 @@ mt7530_setup(struct dsa_switch *ds) >> mt7530_mib_reset(ds); >> >> for (i = 0; i < MT7530_NUM_PORTS; i++) { >> + /* Clear link settings and enable force mode to force link down >> + * on all ports until they're enabled later. >> + */ >> + mt7530_clear(priv, MT7530_PMCR_P(i), PMCR_LINK_SETTINGS_MASK); >> + mt7530_set(priv, MT7530_PMCR_P(i), PMCR_FORCE_MODE); > > Any reason to not combine the two lines above into a single call: > > mt7530_rmw(priv, MT7530_PMCR_P(i), > PMCR_LINK_SETTINGS_MASK | PMCR_FORCE_MODE, > PMCR_FORCE_MODE);
No reason whatsoever, I'll do this. Thanks!
Arınç
| |