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

[Arınç ÜNAL]

Thanks for looking over this patch series Jakub!

On 29/02/2024 03:49, Jakub Kicinski wrote:
> On Fri, 16 Feb 2024 14:05:36 +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.
>
> I don't know phylink so some basic questions..
>
> What's "mode" in this case?

The mode here represents whether we will configure properties of the link
manually (force mode), or by polling the PHY. The driver is supposed to
configure the properties so we enable force mode on the port MAC registers.

MT7530 has a single bit for this, PMCR_FORCE_MODE. MT7531 has multiple
bits, MT7531_FORCE_MODE, each of them enabling the force mode for a certain
property of the link.

> > 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.
>
> MT7531_FORCE_MODE also includes MT7531_FORCE_LNK, doesn't that mean
> the link will be up?

No, that one enables force mode for the link status property. The bit for
setting the link status is PMCR_FORCE_LNK.

I know the current naming of the bits is confusing. I have patch for this
on my next patch series to improve it.

> > 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.
>
> This should be a separate change.

Sure, will do.

> > Suggested-by: Vladimir Oltean <olteanv@xxxxxxxxx>
> > Signed-off-by: Arınç ÜNAL <arinc.unal@xxxxxxxxxx>
>
> > @@ -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
>
> "Clear link settings to force link down" makes sense.
> Since I don't know what the mode is, the "and enable force mode"
> sounds possibly out of place. If you're only combining this
> for the convenience of RMW, keep the reasoning separate.

No. Force mode must be enabled so that we can manually set the link state.
Then, clearing the link settings achieves setting the link status to down.

Arınç