diff mbox series

[RFC,net-next,08/22] net: dsa: mt7530: change p{5,6}_interface to p{5,6}_configured

Message ID 20230421143648.87889-9-arinc.unal@arinc9.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: dsa: MT7530, MT7531, and MT7988 improvements | expand

Checks

Context Check Description
netdev/series_format fail Series longer than 15 patches (and no cover letter)
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/cc_maintainers success CCed 16 of 16 maintainers
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 81 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Arınç ÜNAL April 21, 2023, 2:36 p.m. UTC 
From: Arınç ÜNAL <arinc.unal@arinc9.com>

The idea of p5_interface and p6_interface pointers is to prevent
mt753x_mac_config() from running twice for MT7531, as it's already run with
mt753x_cpu_port_enable() from mt7531_setup_common(), if the port is used as
a CPU port.

Change p5_interface and p6_interface to p5_configured and p6_configured.
Make them boolean.

Do not set them for any other reason.

The priv->p5_intf_sel check is useless as in this code path, it will always
be P5_INTF_SEL_GMAC5.

There was also no need to set priv->p5_interface and priv->p6_interface to
PHY_INTERFACE_MODE_NA on mt7530_setup() and mt7531_setup() as they would
already be set to that when "priv" is allocated. The pointers were of the
phy_interface_t enumeration type, and the first element of the enum is
PHY_INTERFACE_MODE_NA. There was nothing in between that would change this
beforehand.

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

Comments

Daniel Golle April 21, 2023, 5:28 p.m. UTC | #1 
On Fri, Apr 21, 2023 at 05:36:34PM +0300, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> The idea of p5_interface and p6_interface pointers is to prevent
> mt753x_mac_config() from running twice for MT7531, as it's already run with
> mt753x_cpu_port_enable() from mt7531_setup_common(), if the port is used as
> a CPU port.
> 
> Change p5_interface and p6_interface to p5_configured and p6_configured.
> Make them boolean.
> 
> Do not set them for any other reason.
> 
> The priv->p5_intf_sel check is useless as in this code path, it will always
> be P5_INTF_SEL_GMAC5.
> 
> There was also no need to set priv->p5_interface and priv->p6_interface to
> PHY_INTERFACE_MODE_NA on mt7530_setup() and mt7531_setup() as they would
> already be set to that when "priv" is allocated. The pointers were of the
> phy_interface_t enumeration type, and the first element of the enum is
> PHY_INTERFACE_MODE_NA. There was nothing in between that would change this
> beforehand.
> 
> Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>

NACK. This assumes that a user port is configured exactly once.
However, interface mode may change because of mode-changing PHYs (e.g.
often using Cisco SGMII for 10M/100M/1000M but using 2500Base-X for
2500M, ie. depending on actual link speed).

Also when using SFP modules (which can be hotplugged) the interface
mode may change after initially setting up the driver, e.g. when SFP
driver is loaded or a module is plugged or replaced.

> ---
>  drivers/net/dsa/mt7530.c | 19 ++++---------------
>  drivers/net/dsa/mt7530.h | 10 ++++++----
>  2 files changed, 10 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index bac2388319a3..2f670e512415 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2237,8 +2237,6 @@ mt7530_setup(struct dsa_switch *ds)
>  	val |= MHWTRAP_MANUAL;
>  	mt7530_write(priv, MT7530_MHWTRAP, val);
>  
> -	priv->p6_interface = PHY_INTERFACE_MODE_NA;
> -
>  	/* Enable and reset MIB counters */
>  	mt7530_mib_reset(ds);
>  
> @@ -2460,10 +2458,6 @@ mt7531_setup(struct dsa_switch *ds)
>  	mt7530_rmw(priv, MT7531_GPIO_MODE0, MT7531_GPIO0_MASK,
>  		   MT7531_GPIO0_INTERRUPT);
>  
> -	/* Let phylink decide the interface later. */
> -	priv->p5_interface = PHY_INTERFACE_MODE_NA;
> -	priv->p6_interface = PHY_INTERFACE_MODE_NA;
> -
>  	/* Enable PHY core PLL, since phy_device has not yet been created
>  	 * provided for phy_[read,write]_mmd_indirect is called, we provide
>  	 * our own mt7531_ind_mmd_phy_[read,write] to complete this
> @@ -2733,25 +2727,20 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>  			goto unsupported;
>  		break;
>  	case 5: /* Port 5, can be used as a CPU port. */
> -		if (priv->p5_interface == state->interface)
> +		if (priv->p5_configured)
>  			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: /* Port 6, can be used as a CPU port. */
> -		if (priv->p6_interface == state->interface)
> +		if (priv->p6_configured)
>  			break;
>  
>  		mt753x_pad_setup(ds, state);
>  
>  		if (mt753x_mac_config(ds, port, mode, state) < 0)
>  			goto unsupported;
> -
> -		priv->p6_interface = state->interface;
>  		break;
>  	default:
>  unsupported:
> @@ -2859,12 +2848,12 @@ mt7531_cpu_port_config(struct dsa_switch *ds, int port)
>  		else
>  			interface = PHY_INTERFACE_MODE_2500BASEX;
>  
> -		priv->p5_interface = interface;
> +		priv->p5_configured = true;
>  		break;
>  	case 6:
>  		interface = PHY_INTERFACE_MODE_2500BASEX;
>  
> -		priv->p6_interface = interface;
> +		priv->p6_configured = true;
>  		break;
>  	default:
>  		return -EINVAL;
> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
> index f58828577520..c3a37a0f4843 100644
> --- a/drivers/net/dsa/mt7530.h
> +++ b/drivers/net/dsa/mt7530.h
> @@ -745,8 +745,10 @@ struct mt753x_info {
>   * @ports:		Holding the state among ports
>   * @reg_mutex:		The lock for protecting among process accessing
>   *			registers
> - * @p6_interface:	Holding the current port 6 interface
> - * @p5_interface:	Holding the current port 5 interface
> + * @p6_configured:	Flag for distinguishing if port 6 of the MT7531 switch
> + *			is already configured
> + * @p5_configured:	Flag for distinguishing if port 5 of the MT7531 switch
> + *			is already configured
>   * @p5_intf_sel:	Holding the current port 5 interface select
>   * @p5_sgmii:		Flag for distinguishing if port 5 of the MT7531 switch
>   *			has got SGMII
> @@ -767,8 +769,8 @@ struct mt7530_priv {
>  	const struct mt753x_info *info;
>  	unsigned int		id;
>  	bool			mcm;
> -	phy_interface_t		p6_interface;
> -	phy_interface_t		p5_interface;
> +	bool			p6_configured;
> +	bool			p5_configured;
>  	p5_interface_select	p5_intf_sel;
>  	bool			p5_sgmii;
>  	u8			mirror_rx;
> -- 
> 2.37.2
>
Arınç ÜNAL April 21, 2023, 6:17 p.m. UTC | #2 
On 21.04.2023 20:28, Daniel Golle wrote:
> On Fri, Apr 21, 2023 at 05:36:34PM +0300, arinc9.unal@gmail.com wrote:
>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>
>> The idea of p5_interface and p6_interface pointers is to prevent
>> mt753x_mac_config() from running twice for MT7531, as it's already run with
>> mt753x_cpu_port_enable() from mt7531_setup_common(), if the port is used as
>> a CPU port.
>>
>> Change p5_interface and p6_interface to p5_configured and p6_configured.
>> Make them boolean.
>>
>> Do not set them for any other reason.
>>
>> The priv->p5_intf_sel check is useless as in this code path, it will always
>> be P5_INTF_SEL_GMAC5.
>>
>> There was also no need to set priv->p5_interface and priv->p6_interface to
>> PHY_INTERFACE_MODE_NA on mt7530_setup() and mt7531_setup() as they would
>> already be set to that when "priv" is allocated. The pointers were of the
>> phy_interface_t enumeration type, and the first element of the enum is
>> PHY_INTERFACE_MODE_NA. There was nothing in between that would change this
>> beforehand.
>>
>> Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> NACK. This assumes that a user port is configured exactly once.
> However, interface mode may change because of mode-changing PHYs (e.g.
> often using Cisco SGMII for 10M/100M/1000M but using 2500Base-X for
> 2500M, ie. depending on actual link speed).
> 
> Also when using SFP modules (which can be hotplugged) the interface
> mode may change after initially setting up the driver, e.g. when SFP
> driver is loaded or a module is plugged or replaced.

I'm not sure I understand. pX_configured would be set to true only when 
the port is used as a CPU port. mt753x_mac_config() should run for user 
or DSA ports more than once, if needed.

Arınç
Arınç ÜNAL April 21, 2023, 6:20 p.m. UTC | #3 
On 21.04.2023 21:17, Arınç ÜNAL wrote:
> On 21.04.2023 20:28, Daniel Golle wrote:
>> On Fri, Apr 21, 2023 at 05:36:34PM +0300, arinc9.unal@gmail.com wrote:
>>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>>
>>> The idea of p5_interface and p6_interface pointers is to prevent
>>> mt753x_mac_config() from running twice for MT7531, as it's already 
>>> run with
>>> mt753x_cpu_port_enable() from mt7531_setup_common(), if the port is 
>>> used as
>>> a CPU port.
>>>
>>> Change p5_interface and p6_interface to p5_configured and p6_configured.
>>> Make them boolean.
>>>
>>> Do not set them for any other reason.
>>>
>>> The priv->p5_intf_sel check is useless as in this code path, it will 
>>> always
>>> be P5_INTF_SEL_GMAC5.
>>>
>>> There was also no need to set priv->p5_interface and 
>>> priv->p6_interface to
>>> PHY_INTERFACE_MODE_NA on mt7530_setup() and mt7531_setup() as they would
>>> already be set to that when "priv" is allocated. The pointers were of 
>>> the
>>> phy_interface_t enumeration type, and the first element of the enum is
>>> PHY_INTERFACE_MODE_NA. There was nothing in between that would change 
>>> this
>>> beforehand.
>>>
>>> Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>>
>> NACK. This assumes that a user port is configured exactly once.
>> However, interface mode may change because of mode-changing PHYs (e.g.
>> often using Cisco SGMII for 10M/100M/1000M but using 2500Base-X for
>> 2500M, ie. depending on actual link speed).
>>
>> Also when using SFP modules (which can be hotplugged) the interface
>> mode may change after initially setting up the driver, e.g. when SFP
>> driver is loaded or a module is plugged or replaced.
> 
> I'm not sure I understand. pX_configured would be set to true only when 
> the port is used as a CPU port. mt753x_mac_config() should run for user 
> or DSA ports more than once, if needed.

Looking at this again, once pX_interface is true, the check will prevent 
even user or DSA ports to be configured again. What about setting 
pX_interface to false after mt753x_mac_config() is run?

Arınç
Arınç ÜNAL April 21, 2023, 6:25 p.m. UTC | #4 
On 21.04.2023 21:20, Arınç ÜNAL wrote:
> On 21.04.2023 21:17, Arınç ÜNAL wrote:
>> On 21.04.2023 20:28, Daniel Golle wrote:
>>> On Fri, Apr 21, 2023 at 05:36:34PM +0300, arinc9.unal@gmail.com wrote:
>>>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>>>
>>>> The idea of p5_interface and p6_interface pointers is to prevent
>>>> mt753x_mac_config() from running twice for MT7531, as it's already 
>>>> run with
>>>> mt753x_cpu_port_enable() from mt7531_setup_common(), if the port is 
>>>> used as
>>>> a CPU port.
>>>>
>>>> Change p5_interface and p6_interface to p5_configured and 
>>>> p6_configured.
>>>> Make them boolean.
>>>>
>>>> Do not set them for any other reason.
>>>>
>>>> The priv->p5_intf_sel check is useless as in this code path, it will 
>>>> always
>>>> be P5_INTF_SEL_GMAC5.
>>>>
>>>> There was also no need to set priv->p5_interface and 
>>>> priv->p6_interface to
>>>> PHY_INTERFACE_MODE_NA on mt7530_setup() and mt7531_setup() as they 
>>>> would
>>>> already be set to that when "priv" is allocated. The pointers were 
>>>> of the
>>>> phy_interface_t enumeration type, and the first element of the enum is
>>>> PHY_INTERFACE_MODE_NA. There was nothing in between that would 
>>>> change this
>>>> beforehand.
>>>>
>>>> Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>>>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>>>
>>> NACK. This assumes that a user port is configured exactly once.
>>> However, interface mode may change because of mode-changing PHYs (e.g.
>>> often using Cisco SGMII for 10M/100M/1000M but using 2500Base-X for
>>> 2500M, ie. depending on actual link speed).
>>>
>>> Also when using SFP modules (which can be hotplugged) the interface
>>> mode may change after initially setting up the driver, e.g. when SFP
>>> driver is loaded or a module is plugged or replaced.
>>
>> I'm not sure I understand. pX_configured would be set to true only 
>> when the port is used as a CPU port. mt753x_mac_config() should run 
>> for user or DSA ports more than once, if needed.
> 
> Looking at this again, once pX_interface is true, the check will prevent 
> even user or DSA ports to be configured again. What about setting 
> pX_interface to false after mt753x_mac_config() is run?

On a third thought, pX_interface will never be true for the port if it's 
a user or DSA port so this should not be a problem at all.

Arınç
Daniel Golle April 21, 2023, 7:03 p.m. UTC | #5 
On Fri, Apr 21, 2023 at 09:25:39PM +0300, Arınç ÜNAL wrote:
> 
> 
> On 21.04.2023 21:20, Arınç ÜNAL wrote:
> > On 21.04.2023 21:17, Arınç ÜNAL wrote:
> > > On 21.04.2023 20:28, Daniel Golle wrote:
> > > > On Fri, Apr 21, 2023 at 05:36:34PM +0300, arinc9.unal@gmail.com wrote:
> > > > > From: Arınç ÜNAL <arinc.unal@arinc9.com>
> > > > > 
> > > > > The idea of p5_interface and p6_interface pointers is to prevent
> > > > > mt753x_mac_config() from running twice for MT7531, as it's
> > > > > already run with
> > > > > mt753x_cpu_port_enable() from mt7531_setup_common(), if the
> > > > > port is used as
> > > > > a CPU port.
> > > > > 
> > > > > Change p5_interface and p6_interface to p5_configured and
> > > > > p6_configured.
> > > > > Make them boolean.
> > > > > 
> > > > > Do not set them for any other reason.
> > > > > 
> > > > > The priv->p5_intf_sel check is useless as in this code path,
> > > > > it will always
> > > > > be P5_INTF_SEL_GMAC5.
> > > > > 
> > > > > There was also no need to set priv->p5_interface and
> > > > > priv->p6_interface to
> > > > > PHY_INTERFACE_MODE_NA on mt7530_setup() and mt7531_setup()
> > > > > as they would
> > > > > already be set to that when "priv" is allocated. The
> > > > > pointers were of the
> > > > > phy_interface_t enumeration type, and the first element of the enum is
> > > > > PHY_INTERFACE_MODE_NA. There was nothing in between that
> > > > > would change this
> > > > > beforehand.
> > > > > 
> > > > > Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> > > > > Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> > > > 
> > > > NACK. This assumes that a user port is configured exactly once.
> > > > However, interface mode may change because of mode-changing PHYs (e.g.
> > > > often using Cisco SGMII for 10M/100M/1000M but using 2500Base-X for
> > > > 2500M, ie. depending on actual link speed).
> > > > 
> > > > Also when using SFP modules (which can be hotplugged) the interface
> > > > mode may change after initially setting up the driver, e.g. when SFP
> > > > driver is loaded or a module is plugged or replaced.
> > > 
> > > I'm not sure I understand. pX_configured would be set to true only
> > > when the port is used as a CPU port. mt753x_mac_config() should run
> > > for user or DSA ports more than once, if needed.
> > 
> > Looking at this again, once pX_interface is true, the check will prevent
> > even user or DSA ports to be configured again. What about setting
> > pX_interface to false after mt753x_mac_config() is run?
> 
> On a third thought, pX_interface will never be true for the port if it's a
> user or DSA port so this should not be a problem at all.

I also followed the individual codepaths and conclude that you are
right. I've also tested this by now on several boards with MT753x,
incl. BPi-R3 and SerDes interface mode switching is anyway handled in
the PCS driver (I should have rembered that...)

Hence
Acked-by: Daniel Golle <daniel@makrotopia.org>

> 
> Arınç
diff mbox series

Patch

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index bac2388319a3..2f670e512415 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2237,8 +2237,6 @@  mt7530_setup(struct dsa_switch *ds)
 	val |= MHWTRAP_MANUAL;
 	mt7530_write(priv, MT7530_MHWTRAP, val);
 
-	priv->p6_interface = PHY_INTERFACE_MODE_NA;
-
 	/* Enable and reset MIB counters */
 	mt7530_mib_reset(ds);
 
@@ -2460,10 +2458,6 @@  mt7531_setup(struct dsa_switch *ds)
 	mt7530_rmw(priv, MT7531_GPIO_MODE0, MT7531_GPIO0_MASK,
 		   MT7531_GPIO0_INTERRUPT);
 
-	/* Let phylink decide the interface later. */
-	priv->p5_interface = PHY_INTERFACE_MODE_NA;
-	priv->p6_interface = PHY_INTERFACE_MODE_NA;
-
 	/* Enable PHY core PLL, since phy_device has not yet been created
 	 * provided for phy_[read,write]_mmd_indirect is called, we provide
 	 * our own mt7531_ind_mmd_phy_[read,write] to complete this
@@ -2733,25 +2727,20 @@  mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 			goto unsupported;
 		break;
 	case 5: /* Port 5, can be used as a CPU port. */
-		if (priv->p5_interface == state->interface)
+		if (priv->p5_configured)
 			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: /* Port 6, can be used as a CPU port. */
-		if (priv->p6_interface == state->interface)
+		if (priv->p6_configured)
 			break;
 
 		mt753x_pad_setup(ds, state);
 
 		if (mt753x_mac_config(ds, port, mode, state) < 0)
 			goto unsupported;
-
-		priv->p6_interface = state->interface;
 		break;
 	default:
 unsupported:
@@ -2859,12 +2848,12 @@  mt7531_cpu_port_config(struct dsa_switch *ds, int port)
 		else
 			interface = PHY_INTERFACE_MODE_2500BASEX;
 
-		priv->p5_interface = interface;
+		priv->p5_configured = true;
 		break;
 	case 6:
 		interface = PHY_INTERFACE_MODE_2500BASEX;
 
-		priv->p6_interface = interface;
+		priv->p6_configured = true;
 		break;
 	default:
 		return -EINVAL;
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index f58828577520..c3a37a0f4843 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -745,8 +745,10 @@  struct mt753x_info {
  * @ports:		Holding the state among ports
  * @reg_mutex:		The lock for protecting among process accessing
  *			registers
- * @p6_interface:	Holding the current port 6 interface
- * @p5_interface:	Holding the current port 5 interface
+ * @p6_configured:	Flag for distinguishing if port 6 of the MT7531 switch
+ *			is already configured
+ * @p5_configured:	Flag for distinguishing if port 5 of the MT7531 switch
+ *			is already configured
  * @p5_intf_sel:	Holding the current port 5 interface select
  * @p5_sgmii:		Flag for distinguishing if port 5 of the MT7531 switch
  *			has got SGMII
@@ -767,8 +769,8 @@  struct mt7530_priv {
 	const struct mt753x_info *info;
 	unsigned int		id;
 	bool			mcm;
-	phy_interface_t		p6_interface;
-	phy_interface_t		p5_interface;
+	bool			p6_configured;
+	bool			p5_configured;
 	p5_interface_select	p5_intf_sel;
 	bool			p5_sgmii;
 	u8			mirror_rx;