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(-)
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
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
>
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ç
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ç
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ç
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ç
© 2016 - 2024 Red Hat, Inc.