priv->p5_interface and priv->p6_interface are for use on the MT7531 switch.
They prevent the CPU ports of MT7531 to be configured again. They are
useless for MT7530. Therefore, remove setting priv->p5_interface for
MT7530.

Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
 drivers/net/dsa/mt7530.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index bf6c59ecc489..07c5f1c6d036 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -978,8 +978,6 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
 	dev_dbg(ds->dev, "Setup P5, HWTRAP=0x%x, intf_sel=%s, phy-mode=%s\n",
 		val, p5_intf_modes(priv->p5_intf_sel), phy_modes(interface));
 
-	priv->p5_interface = interface;
-
 unlock_exit:
 	mutex_unlock(&priv->reg_mutex);
 }
-- 
2.40.1

On Wed, Dec 27, 2023 at 07:43:46AM +0300, Arınç ÜNAL wrote:
> priv->p5_interface and priv->p6_interface are for use on the MT7531 switch.
> They prevent the CPU ports of MT7531 to be configured again. They are
> useless for MT7530. Therefore, remove setting priv->p5_interface for
> MT7530.
> 
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---

What makes priv->p5_interface and priv->p6_interface useless for MT7530
as you say? This code in mt753x_phylink_mac_config() seems executed
regardless of switch family:

	case 5:
		if (priv->p5_interface == state->interface)
			break;

		if (mt753x_mac_config(ds, port, mode, state) < 0)
			goto unsupported;

		if (priv->p5_intf_sel != P5_DISABLED)
			priv->p5_interface = state->interface;
		break;
	case 6:
		if (priv->p6_interface == state->interface)
			break;

		mt753x_pad_setup(ds, state);

		if (mt753x_mac_config(ds, port, mode, state) < 0)
			goto unsupported;

		priv->p6_interface = state->interface;
		break;

On 4.01.2024 18:42, Vladimir Oltean wrote:
> On Wed, Dec 27, 2023 at 07:43:46AM +0300, Arınç ÜNAL wrote:
>> priv->p5_interface and priv->p6_interface are for use on the MT7531 switch.
>> They prevent the CPU ports of MT7531 to be configured again. They are
>> useless for MT7530. Therefore, remove setting priv->p5_interface for
>> MT7530.
>>
>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>> ---
> 
> What makes priv->p5_interface and priv->p6_interface useless for MT7530
> as you say? This code in mt753x_phylink_mac_config() seems executed
> regardless of switch family:
> 
> 	case 5:
> 		if (priv->p5_interface == state->interface)
> 			break;
> 
> 		if (mt753x_mac_config(ds, port, mode, state) < 0)
> 			goto unsupported;
> 
> 		if (priv->p5_intf_sel != P5_DISABLED)
> 			priv->p5_interface = state->interface;
> 		break;
> 	case 6:
> 		if (priv->p6_interface == state->interface)
> 			break;
> 
> 		mt753x_pad_setup(ds, state);
> 
> 		if (mt753x_mac_config(ds, port, mode, state) < 0)
> 			goto unsupported;
> 
> 		priv->p6_interface = state->interface;
> 		break;

This is also useless for non-MT7531 switches in the sense that it
unnecessarily prevents port 5 and 6 from being reconfigured. There's
nothing wrong with configuring them multiple times. These are the remains
of before phylink was implemented on this driver so the thought of changing
phy_interface_t on the fly was non existent. At that time, it was probably
made to apply to all switch models for convenience, as port 5 and 6 are CPU
ports so they're highly likely to be fixed links.

The reason I don't deal with this code block now is because I will get rid
of priv->p5_interface and priv->p6_interface when I also get rid of
priv->info->cpu_port_config() with a later patch.

Arınç

On 6.01.2024 18:00, Arınç ÜNAL wrote:
> On 4.01.2024 18:42, Vladimir Oltean wrote:
>> On Wed, Dec 27, 2023 at 07:43:46AM +0300, Arınç ÜNAL wrote:
>>> priv->p5_interface and priv->p6_interface are for use on the MT7531 switch.
>>> They prevent the CPU ports of MT7531 to be configured again. They are
>>> useless for MT7530. Therefore, remove setting priv->p5_interface for
>>> MT7530.
>>>
>>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>>> ---
>>
>> What makes priv->p5_interface and priv->p6_interface useless for MT7530
>> as you say? This code in mt753x_phylink_mac_config() seems executed
>> regardless of switch family:
>>
>>     case 5:
>>         if (priv->p5_interface == state->interface)
>>             break;
>>
>>         if (mt753x_mac_config(ds, port, mode, state) < 0)
>>             goto unsupported;
>>
>>         if (priv->p5_intf_sel != P5_DISABLED)
>>             priv->p5_interface = state->interface;
>>         break;
>>     case 6:
>>         if (priv->p6_interface == state->interface)
>>             break;
>>
>>         mt753x_pad_setup(ds, state);
>>
>>         if (mt753x_mac_config(ds, port, mode, state) < 0)
>>             goto unsupported;
>>
>>         priv->p6_interface = state->interface;
>>         break;
> 
> This is also useless for non-MT7531 switches in the sense that it
> unnecessarily prevents port 5 and 6 from being reconfigured. There's
> nothing wrong with configuring them multiple times. These are the remains
> of before phylink was implemented on this driver so the thought of changing
> phy_interface_t on the fly was non existent. At that time, it was probably
> made to apply to all switch models for convenience, as port 5 and 6 are CPU
> ports so they're highly likely to be fixed links.
> 
> The reason I don't deal with this code block now is because I will get rid
> of priv->p5_interface and priv->p6_interface when I also get rid of
> priv->info->cpu_port_config() with a later patch.

Sorry, I couldn't give a straight answer. Here's the exact answer to this
patch.

Running mt7530_setup_port5() from mt7530_setup() handles all cases of
configuring port 5, including phylink.

Setting priv->p5_interface under mt7530_setup_port5() makes sure that
mt7530_setup_port5() from mt753x_phylink_mac_config() won't run.

The ("net: dsa: mt7530: improve code path for setting up port 5") patch
makes so that mt7530_setup_port5() from mt7530_setup() runs only on
non-phylink cases.

So we get rid of setting priv->p5_interface under mt7530_setup_port5() as
port 5 phylink configuration will be done by running mt7530_setup_port5()
from mt753x_phylink_mac_config() now.

Running mt7530_setup_port5() from mt753x_phylink_mac_config() multiple
times is being prevented which shouldn't be done. That's because of a
different reason involving MT7531. I will deal with this with a later
patch.

I intend to put this on the patch log.

Arınç

On Tue, Jan 09, 2024 at 02:42:45PM +0300, Arınç ÜNAL wrote:
> Running mt7530_setup_port5() from mt753x_phylink_mac_config() multiple
> times is being prevented which shouldn't be done. That's because of a
> different reason involving MT7531. I will deal with this with a later
> patch.
> 
> I intend to put this on the patch log.

Still not clear why it shouldn't be done. Ideally you could come up with
a plausible hypothesis as to why it's there in the first place, and why
it's not needed.

On 9.01.2024 14:51, Vladimir Oltean wrote:
> On Tue, Jan 09, 2024 at 02:42:45PM +0300, Arınç ÜNAL wrote:
>> Running mt7530_setup_port5() from mt753x_phylink_mac_config() multiple
>> times is being prevented which shouldn't be done. That's because of a
>> different reason involving MT7531. I will deal with this with a later
>> patch.
>>
>> I intend to put this on the patch log.
> 
> Still not clear why it shouldn't be done. Ideally you could come up with
> a plausible hypothesis as to why it's there in the first place, and why
> it's not needed.

I can't tell why it's there. Russell did analyse why it's not needed [1]
and my test [2] confirms it.

This patch is about removing the unnecessary setting of priv->p5_interface
on mt7530_setup_port5() which I believe I have already explained. I would
like to get into the details of priv->p5_interface and priv->p6_interface
when I remove them along with cpu_port_config().

That said, I will refrain from including the last paragraph on the patch
log.

[1] https://lore.kernel.org/netdev/ZHy2jQLesdYFMQtO@shell.armlinux.org.uk/
[2] https://lore.kernel.org/netdev/9308fa1a-6de3-490b-9aeb-eb207b0432df@arinc9.com/

Arınç