Messages in this thread Patch in this message | | | Date | Thu, 8 Feb 2024 16:22:07 +0000 | From | Daniel Golle <> | Subject | Re: [PATCH netnext 8/8] net: dsa: mt7530: simplify link operations and force link down on all ports |
| |
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);
> + > /* Disable forwarding by default on all ports */ > mt7530_rmw(priv, MT7530_PCR_P(i), PCR_MATRIX_MASK, > PCR_MATRIX_CLR); > @@ -2359,6 +2363,12 @@ mt7531_setup_common(struct dsa_switch *ds) > UNU_FFP_MASK); > > 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), MT7531_FORCE_MODE); > +
Same here obviously.
> /* Disable forwarding by default on all ports */ > mt7530_rmw(priv, MT7530_PCR_P(i), PCR_MATRIX_MASK, > PCR_MATRIX_CLR); > @@ -2657,23 +2667,13 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode, > const struct phylink_link_state *state) > { > struct mt7530_priv *priv = ds->priv; > - u32 mcr_cur, mcr_new; > > if ((port == 5 || port == 6) && priv->info->mac_port_config) > priv->info->mac_port_config(ds, port, mode, state->interface); > > - mcr_cur = mt7530_read(priv, MT7530_PMCR_P(port)); > - mcr_new = mcr_cur; > - mcr_new &= ~PMCR_LINK_SETTINGS_MASK; > - mcr_new |= PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | PMCR_BACKOFF_EN | > - PMCR_BACKPR_EN | PMCR_FORCE_MODE_ID(priv->id); > - > /* Are we connected to external phy */ > if (port == 5 && dsa_is_user_port(ds, 5)) > - mcr_new |= PMCR_EXT_PHY; > - > - if (mcr_new != mcr_cur) > - mt7530_write(priv, MT7530_PMCR_P(port), mcr_new); > + mt7530_set(priv, MT7530_PMCR_P(port), PMCR_EXT_PHY); > } > > static void mt753x_phylink_mac_link_down(struct dsa_switch *ds, int port, > diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h > index 8a8144868eaa..a71166e0a7fc 100644 > --- a/drivers/net/dsa/mt7530.h > +++ b/drivers/net/dsa/mt7530.h > @@ -304,8 +304,6 @@ enum mt7530_vlan_port_acc_frm { > MT7531_FORCE_DPX | \ > MT7531_FORCE_RX_FC | \ > MT7531_FORCE_TX_FC) > -#define PMCR_FORCE_MODE_ID(id) ((((id) == ID_MT7531) || ((id) == ID_MT7988)) ? \ > - MT7531_FORCE_MODE : PMCR_FORCE_MODE) > #define PMCR_LINK_SETTINGS_MASK (PMCR_TX_EN | PMCR_FORCE_SPEED_1000 | \ > PMCR_RX_EN | PMCR_FORCE_SPEED_100 | \ > PMCR_TX_FC_EN | PMCR_RX_FC_EN | \ > > -- > 2.40.1 >
| |