diff mbox series

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

Message ID 20240216-for-netnext-mt7530-improvements-3-v2-8-094cae3ff23b@arinc9.com (mailing list archive)
State New
Headers show
Series MT7530 DSA Subdriver Improvements Act III | expand

Commit Message

Arınç ÜNAL via B4 Relay Feb. 16, 2024, 11:05 a.m. UTC 
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(-)

Comments

Jakub Kicinski Feb. 29, 2024, 1:49 a.m. UTC | #1 
On Fri, 16 Feb 2024 14:05:36 +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.

I don't know phylink so some basic questions..

What's "mode" in this case?

> 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?

> 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.

> Suggested-by: Vladimir Oltean <olteanv@gmail.com>
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>

> @@ -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.
Arınç ÜNAL March 1, 2024, 7:17 a.m. UTC | #2 
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@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.
> 
> 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@gmail.com>
>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
>> @@ -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ç
diff mbox series

Patch

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 5c8ad41ce8cd..1ae11b180d54 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_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_rmw(priv, MT7530_PMCR_P(i), PMCR_LINK_SETTINGS_MASK |
+			   MT7531_FORCE_MODE, MT7531_FORCE_MODE);
+
 		/* 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 | \