diff mbox series

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

Message ID 20230522121532.86610-13-arinc.unal@arinc9.com (mailing list archive)
State New, archived
Headers show
Series net: dsa: mt7530: improve, trap BPDU & LLDP, and prefer CPU port | expand

Commit Message

Arınç ÜNAL May 22, 2023, 12:15 p.m. UTC 
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(-)

Comments

Andrew Lunn May 23, 2023, 11:40 p.m. UTC | #1 
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
Vladimir Oltean May 24, 2023, 6:15 p.m. UTC | #2 
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
>
Arınç ÜNAL May 25, 2023, 7:04 a.m. UTC | #3 
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ç
diff mbox series

Patch

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 |