Received-SPF: pass (google.com: domain of linux-kernel+bounces-58629-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <2a6a8d93-beae-4bec-a792-0e093b4c4ecd@arinc9.com>
Date: Thu, 8 Feb 2024 22:35:53 +0300
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH netnext 8/8] net: dsa: mt7530: simplify link operations
 and force link down on all ports
To: Daniel Golle <daniel@makrotopia.org>
Cc: DENG Qingfang <dqfext@gmail.com>, Sean Wang <sean.wang@mediatek.com>,
 Andrew Lunn <andrew@lunn.ch>, Florian Fainelli <f.fainelli@gmail.com>,
 Vladimir Oltean <olteanv@gmail.com>, "David S. Miller"
 <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>,
 Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
 Matthias Brugger <matthias.bgg@gmail.com>,
 AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>,
 Russell King <linux@armlinux.org.uk>, mithat.guner@xeront.com,
 erkin.bozoglu@xeront.com, Bartel Eerdekens <bartel.eerdekens@constell8.be>,
 netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
 linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org
References: <20240208-for-netnext-mt7530-improvements-3-v1-0-d7c1cfd502ca@arinc9.com>
 <20240208-for-netnext-mt7530-improvements-3-v1-8-d7c1cfd502ca@arinc9.com>
 <ZcT_r68mStRAC3Uk@makrotopia.org>
Content-Language: en-US
From: =?UTF-8?B?QXLEsW7DpyDDnE5BTA==?= <arinc.unal@arinc9.com>
In-Reply-To: <ZcT_r68mStRAC3Uk@makrotopia.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

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ç