[v1] net: dsa: MT7530, MT7531, and MT7988 improvements

[RFC PATCH net-next 20/22] net: dsa: mt7530: force link-down on MACs before reset on MT7530

Posted by arinc9.unal@gmail.com 1 year, 5 months ago

From: Arınç ÜNAL <arinc.unal@arinc9.com>

Force link-down on all MACs before internal reset. Let's follow suit commit
728c2af6ad8c ("net: mt7531: ensure all MACs are powered down before
reset").

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

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index ac1e3c58aaac..8ece3d0d820c 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2203,6 +2203,10 @@ mt7530_setup(struct dsa_switch *ds)
 		return -EINVAL;
 	}
 
+	/* Force link-down on all MACs before internal reset */
+	for (i = 0; i < MT7530_NUM_PORTS; i++)
+		mt7530_write(priv, MT7530_PMCR_P(i), PMCR_FORCE_LNK);
+
 	/* Reset the switch through internal reset */
 	mt7530_write(priv, MT7530_SYS_CTRL,
 		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
-- 
2.37.2

Re: [RFC PATCH net-next 20/22] net: dsa: mt7530: force link-down on MACs before reset on MT7530

Posted by Daniel Golle 1 year, 5 months ago

On Fri, Apr 21, 2023 at 05:36:46PM +0300, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> Force link-down on all MACs before internal reset. Let's follow suit commit
> 728c2af6ad8c ("net: mt7531: ensure all MACs are powered down before
> reset").
> 
> Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---
>  drivers/net/dsa/mt7530.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index ac1e3c58aaac..8ece3d0d820c 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2203,6 +2203,10 @@ mt7530_setup(struct dsa_switch *ds)
>  		return -EINVAL;
>  	}
>  
> +	/* Force link-down on all MACs before internal reset */
> +	for (i = 0; i < MT7530_NUM_PORTS; i++)
> +		mt7530_write(priv, MT7530_PMCR_P(i), PMCR_FORCE_LNK);
> +

Moving this part to mt753x_setup just before calling priv->info->sw_setup(ds);
is probably better. Though it isn't documented I assume that the requirement
to have the ports in force-link-down may also apply to MT7988, and for sure
it doesn't do any harm.

Hence I suggest to squash this change:
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index a2cb7e296165e..998c4e8930cd3 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2203,10 +2203,6 @@ mt7530_setup(struct dsa_switch *ds)
 		return -EINVAL;
 	}
 
-	/* Force link-down on all MACs before internal reset */
-	for (i = 0; i < MT7530_NUM_PORTS; i++)
-		mt7530_write(priv, MT7530_PMCR_P(i), PMCR_FORCE_LNK);
-
 	/* Reset the switch through internal reset */
 	mt7530_write(priv, MT7530_SYS_CTRL,
 		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
@@ -2423,10 +2419,6 @@ mt7531_setup(struct dsa_switch *ds)
 		dev_info(priv->dev, "found MT7531BE\n");
 	}
 
-	/* all MACs must be forced link-down before sw reset */
-	for (i = 0; i < MT7530_NUM_PORTS; i++)
-		mt7530_write(priv, MT7530_PMCR_P(i), MT7531_FORCE_LNK);
-
 	/* Reset the switch through internal reset */
 	mt7530_write(priv, MT7530_SYS_CTRL,
 		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
@@ -2907,6 +2899,10 @@ mt753x_setup(struct dsa_switch *ds)
 		priv->pcs[i].port = i;
 	}
 
+	/* Force link-down on all MACs before setup */
+	for (i = 0; i < MT7530_NUM_PORTS; i++)
+		mt7530_write(priv, MT7530_PMCR_P(i), PMCR_FORCE_LNK);
+
 	ret = priv->info->sw_setup(ds);
 	if (ret)
 		return ret;

Re: [RFC PATCH net-next 20/22] net: dsa: mt7530: force link-down on MACs before reset on MT7530

Posted by Arınç ÜNAL 1 year, 4 months ago

On 21.04.2023 21:42, Daniel Golle wrote:
> On Fri, Apr 21, 2023 at 05:36:46PM +0300, arinc9.unal@gmail.com wrote:
>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>
>> Force link-down on all MACs before internal reset. Let's follow suit commit
>> 728c2af6ad8c ("net: mt7531: ensure all MACs are powered down before
>> reset").
>>
>> Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>> ---
>>   drivers/net/dsa/mt7530.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index ac1e3c58aaac..8ece3d0d820c 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -2203,6 +2203,10 @@ mt7530_setup(struct dsa_switch *ds)
>>   		return -EINVAL;
>>   	}
>>   
>> +	/* Force link-down on all MACs before internal reset */
>> +	for (i = 0; i < MT7530_NUM_PORTS; i++)
>> +		mt7530_write(priv, MT7530_PMCR_P(i), PMCR_FORCE_LNK);
>> +
> 
> Moving this part to mt753x_setup just before calling priv->info->sw_setup(ds);
> is probably better. Though it isn't documented I assume that the requirement
> to have the ports in force-link-down may also apply to MT7988, and for sure
> it doesn't do any harm.

Now that I'm reading through the programming guide for MT7531 [0] and 
MT7530 [1], I see that the SW_PHY_RST bit on the SYS_CTRL register 
doesn't exist for MT7531. This is likely why forcing link-down on the 
MACs is necessary for MT7531.

I didn't come across any documentation for the switch on the MT7988 SoC. 
Should I assume the registers are identical with MT7531 or have you got 
a document I can look at?

You also don't do system or register reset for the switch on the MT7988 
SoC, what's up with that?

I'm not going to do this change for MT7530 as I think SW_PHY_RST is 
sufficient, and there's no mention to this like on MT7531.

[0] 
https://drive.google.com/file/d/1aVdQz3rbKWjkvdga8-LQ-VFXjmHR8yf9/view?usp=sharing
[1] 
https://github.com/vschagen/documents/blob/main/MT7621_ProgrammingGuide_GSW_v0_3.pdf

Arınç

Re: [RFC PATCH net-next 20/22] net: dsa: mt7530: force link-down on MACs before reset on MT7530

Posted by Arınç ÜNAL 1 year, 5 months ago

On 21.04.2023 21:42, Daniel Golle wrote:
> On Fri, Apr 21, 2023 at 05:36:46PM +0300, arinc9.unal@gmail.com wrote:
>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>
>> Force link-down on all MACs before internal reset. Let's follow suit commit
>> 728c2af6ad8c ("net: mt7531: ensure all MACs are powered down before
>> reset").
>>
>> Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>> ---
>>   drivers/net/dsa/mt7530.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index ac1e3c58aaac..8ece3d0d820c 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -2203,6 +2203,10 @@ mt7530_setup(struct dsa_switch *ds)
>>   		return -EINVAL;
>>   	}
>>   
>> +	/* Force link-down on all MACs before internal reset */
>> +	for (i = 0; i < MT7530_NUM_PORTS; i++)
>> +		mt7530_write(priv, MT7530_PMCR_P(i), PMCR_FORCE_LNK);
>> +
> 
> Moving this part to mt753x_setup just before calling priv->info->sw_setup(ds);
> is probably better. Though it isn't documented I assume that the requirement
> to have the ports in force-link-down may also apply to MT7988, and for sure
> it doesn't do any harm.
> 
> Hence I suggest to squash this change:
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index a2cb7e296165e..998c4e8930cd3 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2203,10 +2203,6 @@ mt7530_setup(struct dsa_switch *ds)
>   		return -EINVAL;
>   	}
>   
> -	/* Force link-down on all MACs before internal reset */
> -	for (i = 0; i < MT7530_NUM_PORTS; i++)
> -		mt7530_write(priv, MT7530_PMCR_P(i), PMCR_FORCE_LNK);
> -
>   	/* Reset the switch through internal reset */
>   	mt7530_write(priv, MT7530_SYS_CTRL,
>   		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
> @@ -2423,10 +2419,6 @@ mt7531_setup(struct dsa_switch *ds)
>   		dev_info(priv->dev, "found MT7531BE\n");
>   	}
>   
> -	/* all MACs must be forced link-down before sw reset */
> -	for (i = 0; i < MT7530_NUM_PORTS; i++)
> -		mt7530_write(priv, MT7530_PMCR_P(i), MT7531_FORCE_LNK);
> -
>   	/* Reset the switch through internal reset */
>   	mt7530_write(priv, MT7530_SYS_CTRL,
>   		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
> @@ -2907,6 +2899,10 @@ mt753x_setup(struct dsa_switch *ds)
>   		priv->pcs[i].port = i;
>   	}
>   
> +	/* Force link-down on all MACs before setup */
> +	for (i = 0; i < MT7530_NUM_PORTS; i++)
> +		mt7530_write(priv, MT7530_PMCR_P(i), PMCR_FORCE_LNK);

MT7531 has got a different bit on the register for this, 
MT7531_FORCE_LNK. Are you sure PMCR_FORCE_LNK would work for MT7531 too?

Arınç

Re: [RFC PATCH net-next 20/22] net: dsa: mt7530: force link-down on MACs before reset on MT7530

Posted by Daniel Golle 1 year, 5 months ago

On Fri, Apr 21, 2023 at 09:47:16PM +0300, Arınç ÜNAL wrote:
> On 21.04.2023 21:42, Daniel Golle wrote:
> > On Fri, Apr 21, 2023 at 05:36:46PM +0300, arinc9.unal@gmail.com wrote:
> > > From: Arınç ÜNAL <arinc.unal@arinc9.com>
> > > 
> > > Force link-down on all MACs before internal reset. Let's follow suit commit
> > > 728c2af6ad8c ("net: mt7531: ensure all MACs are powered down before
> > > reset").
> > > 
> > > Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> > > Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> > > ---
> > >   drivers/net/dsa/mt7530.c | 4 ++++
> > >   1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> > > index ac1e3c58aaac..8ece3d0d820c 100644
> > > --- a/drivers/net/dsa/mt7530.c
> > > +++ b/drivers/net/dsa/mt7530.c
> > > @@ -2203,6 +2203,10 @@ mt7530_setup(struct dsa_switch *ds)
> > >   		return -EINVAL;
> > >   	}
> > > +	/* Force link-down on all MACs before internal reset */
> > > +	for (i = 0; i < MT7530_NUM_PORTS; i++)
> > > +		mt7530_write(priv, MT7530_PMCR_P(i), PMCR_FORCE_LNK);
> > > +
> > 
> > Moving this part to mt753x_setup just before calling priv->info->sw_setup(ds);
> > is probably better. Though it isn't documented I assume that the requirement
> > to have the ports in force-link-down may also apply to MT7988, and for sure
> > it doesn't do any harm.
> > 
> > Hence I suggest to squash this change:
> > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> > index a2cb7e296165e..998c4e8930cd3 100644
> > --- a/drivers/net/dsa/mt7530.c
> > +++ b/drivers/net/dsa/mt7530.c
> > @@ -2203,10 +2203,6 @@ mt7530_setup(struct dsa_switch *ds)
> >   		return -EINVAL;
> >   	}
> > -	/* Force link-down on all MACs before internal reset */
> > -	for (i = 0; i < MT7530_NUM_PORTS; i++)
> > -		mt7530_write(priv, MT7530_PMCR_P(i), PMCR_FORCE_LNK);
> > -
> >   	/* Reset the switch through internal reset */
> >   	mt7530_write(priv, MT7530_SYS_CTRL,
> >   		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
> > @@ -2423,10 +2419,6 @@ mt7531_setup(struct dsa_switch *ds)
> >   		dev_info(priv->dev, "found MT7531BE\n");
> >   	}
> > -	/* all MACs must be forced link-down before sw reset */
> > -	for (i = 0; i < MT7530_NUM_PORTS; i++)
> > -		mt7530_write(priv, MT7530_PMCR_P(i), MT7531_FORCE_LNK);
> > -
> >   	/* Reset the switch through internal reset */
> >   	mt7530_write(priv, MT7530_SYS_CTRL,
> >   		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
> > @@ -2907,6 +2899,10 @@ mt753x_setup(struct dsa_switch *ds)
> >   		priv->pcs[i].port = i;
> >   	}
> > +	/* Force link-down on all MACs before setup */
> > +	for (i = 0; i < MT7530_NUM_PORTS; i++)
> > +		mt7530_write(priv, MT7530_PMCR_P(i), PMCR_FORCE_LNK);
> 
> MT7531 has got a different bit on the register for this, MT7531_FORCE_LNK.
> Are you sure PMCR_FORCE_LNK would work for MT7531 too?

No, I had overlooked that. As the effects of not doing the
force-link-down before the reset are subtle and depend on the
link-partners I may not have cought them in my tests.


> 
> Arınç