Re: [PATCH netnext 8/8] net: dsa: mt7530: simplify link operations and force link down on all ports

[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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Signed-off-by: Arınç ÜNAL <arinc.unal@xxxxxxxxxx>
> > ---
> >   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ç