net: dsa: mt7530: improve, trap BPDU & LLDP, and prefer CPU port

[PATCH net-next 12/30] net: dsa: mt7530: move XTAL check to mt7530_setup()

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

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

The crystal frequency concerns the switch core. The frequency should be
checked when the switch is being set up so the driver can reject the
unsupported hardware earlier and without requiring port 6 to be used.

Move it to mt7530_setup().

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

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 049f7be0d790..fa48273269c4 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -408,13 +408,6 @@ mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
 
 	xtal = mt7530_read(priv, MT7530_HWTRAP) & HWTRAP_XTAL_MASK;
 
-	if (xtal == HWTRAP_XTAL_20MHZ) {
-		dev_err(priv->dev,
-			"%s: MT7530 with a 20MHz XTAL is not supported!\n",
-			__func__);
-		return -EINVAL;
-	}
-
 	switch (interface) {
 	case PHY_INTERFACE_MODE_RGMII:
 		trgint = 0;
@@ -2133,7 +2126,7 @@ mt7530_setup(struct dsa_switch *ds)
 	struct mt7530_dummy_poll p;
 	phy_interface_t interface;
 	struct dsa_port *cpu_dp;
-	u32 id, val;
+	u32 id, val, xtal;
 	int ret, i;
 
 	/* The parent node of master netdev which holds the common system
@@ -2203,6 +2196,15 @@ mt7530_setup(struct dsa_switch *ds)
 		return -ENODEV;
 	}
 
+	xtal = mt7530_read(priv, MT7530_HWTRAP) & HWTRAP_XTAL_MASK;
+
+	if (xtal == HWTRAP_XTAL_20MHZ) {
+		dev_err(priv->dev,
+			"%s: MT7530 with a 20MHz XTAL is not supported!\n",
+			__func__);
+		return -EINVAL;
+	}
+
 	/* Reset the switch through internal reset */
 	mt7530_write(priv, MT7530_SYS_CTRL,
 		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
-- 
2.39.2

Re: [PATCH net-next 12/30] net: dsa: mt7530: move XTAL check to mt7530_setup()

Posted by Vladimir Oltean 1 year, 4 months ago

On Mon, May 22, 2023 at 03:15:14PM +0300, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> The crystal frequency concerns the switch core. The frequency should be
> checked when the switch is being set up so the driver can reject the
> unsupported hardware earlier and without requiring port 6 to be used.
> 
> Move it to mt7530_setup().
> 
> Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---

Do you know why a crystal frequency of 20 MHz is not supported?

>  drivers/net/dsa/mt7530.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 049f7be0d790..fa48273269c4 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -408,13 +408,6 @@ mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
>  
>  	xtal = mt7530_read(priv, MT7530_HWTRAP) & HWTRAP_XTAL_MASK;
>  
> -	if (xtal == HWTRAP_XTAL_20MHZ) {
> -		dev_err(priv->dev,
> -			"%s: MT7530 with a 20MHz XTAL is not supported!\n",
> -			__func__);
> -		return -EINVAL;
> -	}
> -
>  	switch (interface) {
>  	case PHY_INTERFACE_MODE_RGMII:
>  		trgint = 0;
> @@ -2133,7 +2126,7 @@ mt7530_setup(struct dsa_switch *ds)
>  	struct mt7530_dummy_poll p;
>  	phy_interface_t interface;
>  	struct dsa_port *cpu_dp;
> -	u32 id, val;
> +	u32 id, val, xtal;
>  	int ret, i;
>  
>  	/* The parent node of master netdev which holds the common system
> @@ -2203,6 +2196,15 @@ mt7530_setup(struct dsa_switch *ds)
>  		return -ENODEV;
>  	}
>  
> +	xtal = mt7530_read(priv, MT7530_HWTRAP) & HWTRAP_XTAL_MASK;
> +
> +	if (xtal == HWTRAP_XTAL_20MHZ) {
> +		dev_err(priv->dev,
> +			"%s: MT7530 with a 20MHz XTAL is not supported!\n",
> +			__func__);

I don't think __func__ brings much value here, it could be dropped in
the process of moving the code.

Also, the HWTRAP register is already read once, here (stored in "val"):

	INIT_MT7530_DUMMY_POLL(&p, priv, MT7530_HWTRAP);
	ret = readx_poll_timeout(_mt7530_read, &p, val, val != 0,
				 20, 1000000);

I wonder if we really need to read it twice.

> +		return -EINVAL;
> +	}
> +
>  	/* Reset the switch through internal reset */
>  	mt7530_write(priv, MT7530_SYS_CTRL,
>  		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
> -- 
> 2.39.2
>

Re: [PATCH net-next 12/30] net: dsa: mt7530: move XTAL check to mt7530_setup()

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

On 24.05.2023 21:15, Vladimir Oltean wrote:
> On Mon, May 22, 2023 at 03:15:14PM +0300, arinc9.unal@gmail.com wrote:
>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>
>> The crystal frequency concerns the switch core. The frequency should be
>> checked when the switch is being set up so the driver can reject the
>> unsupported hardware earlier and without requiring port 6 to be used.
>>
>> Move it to mt7530_setup().
>>
>> Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>> ---
> 
> Do you know why a crystal frequency of 20 MHz is not supported?

I don't know, it just isn't.

https://github.com/BPI-SINOVOIP/BPI-R2-bsp/blob/master/linux-mt/drivers/net/ethernet/mediatek/gsw_mt7623.c#L1076

> 
>>   drivers/net/dsa/mt7530.c | 18 ++++++++++--------
>>   1 file changed, 10 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index 049f7be0d790..fa48273269c4 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -408,13 +408,6 @@ mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
>>   
>>   	xtal = mt7530_read(priv, MT7530_HWTRAP) & HWTRAP_XTAL_MASK;
>>   
>> -	if (xtal == HWTRAP_XTAL_20MHZ) {
>> -		dev_err(priv->dev,
>> -			"%s: MT7530 with a 20MHz XTAL is not supported!\n",
>> -			__func__);
>> -		return -EINVAL;
>> -	}
>> -
>>   	switch (interface) {
>>   	case PHY_INTERFACE_MODE_RGMII:
>>   		trgint = 0;
>> @@ -2133,7 +2126,7 @@ mt7530_setup(struct dsa_switch *ds)
>>   	struct mt7530_dummy_poll p;
>>   	phy_interface_t interface;
>>   	struct dsa_port *cpu_dp;
>> -	u32 id, val;
>> +	u32 id, val, xtal;
>>   	int ret, i;
>>   
>>   	/* The parent node of master netdev which holds the common system
>> @@ -2203,6 +2196,15 @@ mt7530_setup(struct dsa_switch *ds)
>>   		return -ENODEV;
>>   	}
>>   
>> +	xtal = mt7530_read(priv, MT7530_HWTRAP) & HWTRAP_XTAL_MASK;
>> +
>> +	if (xtal == HWTRAP_XTAL_20MHZ) {
>> +		dev_err(priv->dev,
>> +			"%s: MT7530 with a 20MHz XTAL is not supported!\n",
>> +			__func__);
> 
> I don't think __func__ brings much value here, it could be dropped in
> the process of moving the code.

Will do.

> 
> Also, the HWTRAP register is already read once, here (stored in "val"):
> 
> 	INIT_MT7530_DUMMY_POLL(&p, priv, MT7530_HWTRAP);
> 	ret = readx_poll_timeout(_mt7530_read, &p, val, val != 0,
> 				 20, 1000000);
> 
> I wonder if we really need to read it twice.

Likely not, good catch.

Arınç

Re: [PATCH net-next 12/30] net: dsa: mt7530: move XTAL check to mt7530_setup()

Posted by Andrew Lunn 1 year, 4 months ago

On Mon, May 22, 2023 at 03:15:14PM +0300, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> The crystal frequency concerns the switch core. The frequency should be
> checked when the switch is being set up so the driver can reject the
> unsupported hardware earlier and without requiring port 6 to be used.
> 
> Move it to mt7530_setup().
> 
> Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew