Hello!
This patch series simplifies the MT7530 DSA subdriver and improves the
logic of the support for MT7530, MT7531, and the switch on the MT7988 SoC.
I have done a simple ping test to confirm basic communication on all switch
ports on MCM and standalone MT7530, and MT7531 switch with this patch
series applied.
MT7621 Unielec, MCM MT7530:
rgmii-only-gmac0-mt7621-unielec-u7621-06-16m.dtb
gmac0-and-gmac1-mt7621-unielec-u7621-06-16m.dtb
tftpboot 0x80008000 mips-uzImage.bin; tftpboot 0x83000000 mips-rootfs.cpio.uboot; tftpboot 0x83f00000 $dtb; bootm 0x80008000 0x83000000 0x83f00000
MT7622 Bananapi, MT7531:
gmac0-and-gmac1-mt7622-bananapi-bpi-r64.dtb
tftpboot 0x40000000 arm64-Image; tftpboot 0x45000000 arm64-rootfs.cpio.uboot; tftpboot 0x4a000000 $dtb; booti 0x40000000 0x45000000 0x4a000000
MT7623 Bananapi, standalone MT7530:
rgmii-only-gmac0-mt7623n-bananapi-bpi-r2.dtb
gmac0-and-gmac1-mt7623n-bananapi-bpi-r2.dtb
tftpboot 0x80008000 arm-zImage; tftpboot 0x83000000 arm-rootfs.cpio.uboot; tftpboot 0x83f00000 $dtb; bootz 0x80008000 0x83000000 0x83f00000
This patch series is the continuation of the patch series linked below.
https://lore.kernel.org/netdev/[email protected]/
v1: https://lore.kernel.org/netdev/[email protected]/
Arınç
v2:
- Shorten the patch series, include only 7 patches.
- Add the reviewed-by tags given.
- Patch 1
- Change variable ordering of mt753x_conduit_state_change().
- Define the mask variable and store BIT(cpu_dp->index) on it.
- Disable CPU_EN if priv->active_cpu_ports is 0.
- Patch 5
- On the patch log, take the irrelevant information about the
mt7530_setup_port5() call from mt7530_setup() out.
- Patch 6
- Change the patch log to reflect correct information.
Arınç ÜNAL (7):
net: dsa: mt7530: always trap frames to active CPU port on MT7530
net: dsa: mt7530: use p5_interface_select as data type for p5_intf_sel
net: dsa: mt7530: store port 5 SGMII capability of MT7531
net: dsa: mt7530: improve comments regarding port 5 and 6
net: dsa: mt7530: improve code path for setting up port 5
net: dsa: mt7530: do not set priv->p5_interface on mt7530_setup_port5()
net: dsa: mt7530: do not run mt7530_setup_port5() if port 5 is disabled
drivers/net/dsa/mt7530-mdio.c | 7 +-
drivers/net/dsa/mt7530.c | 139 +++++++++++++++++++++----------------
drivers/net/dsa/mt7530.h | 16 +++--
3 files changed, 91 insertions(+), 71 deletions(-)
On the MT7530 switch, the CPU_PORT field indicates which CPU port to trap
frames to, regardless of the affinity of the inbound user port.
When multiple CPU ports are in use, if the DSA conduit interface is down,
trapped frames won't be passed to the conduit interface.
To make trapping frames work including this case, implement
ds->ops->master_state_change() on this subdriver and set the CPU_PORT field
to the numerically smallest CPU port which the DSA conduit interface its
affine to is up. Introduce the active_cpu_ports field to store the
information of the active CPU ports. Correct the macros, CPU_PORT is bits 4
through 6 of the register.
Add a comment to explain frame trapping for this switch.
Currently, the driver doesn't support the use of multiple CPU ports so this
is not necessarily a bug fix.
Suggested-by: Vladimir Oltean <[email protected]>
Suggested-by: Russell King (Oracle) <[email protected]>
Signed-off-by: Arınç ÜNAL <[email protected]>
---
drivers/net/dsa/mt7530.c | 37 +++++++++++++++++++++++++++++++++----
drivers/net/dsa/mt7530.h | 6 ++++--
2 files changed, 37 insertions(+), 6 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 391c4dbdff42..436d5c311be0 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -1035,10 +1035,6 @@ mt753x_cpu_port_enable(struct dsa_switch *ds, int port)
mt7530_set(priv, MT7530_MFC, BC_FFP(BIT(port)) | UNM_FFP(BIT(port)) |
UNU_FFP(BIT(port)));
- /* Set CPU port number */
- if (priv->id == ID_MT7530 || priv->id == ID_MT7621)
- mt7530_rmw(priv, MT7530_MFC, CPU_MASK, CPU_EN | CPU_PORT(port));
-
/* Add the CPU port to the CPU port bitmap for MT7531 and the switch on
* the MT7988 SoC. Trapped frames will be forwarded to the CPU port that
* is affine to the inbound user port.
@@ -3075,6 +3071,38 @@ static int mt753x_set_mac_eee(struct dsa_switch *ds, int port,
return 0;
}
+static void
+mt753x_conduit_state_change(struct dsa_switch *ds,
+ const struct net_device *conduit,
+ bool operational)
+{
+ struct dsa_port *cpu_dp = conduit->dsa_ptr;
+ struct mt7530_priv *priv = ds->priv;
+ u8 mask;
+ int val = 0;
+
+ /* Set the CPU port to trap frames to for MT7530. Trapped frames will be
+ * forwarded to the numerically smallest CPU port which the DSA conduit
+ * interface its affine to is up.
+ */
+ if (priv->id != ID_MT7530 && priv->id != ID_MT7621)
+ return;
+
+ mask = BIT(cpu_dp->index);
+
+ if (operational)
+ priv->active_cpu_ports |= mask;
+ else
+ priv->active_cpu_ports &= ~mask;
+
+ if (priv->active_cpu_ports)
+ val =
+ CPU_EN |
+ CPU_PORT(__ffs((unsigned long)priv->active_cpu_ports));
+
+ mt7530_rmw(priv, MT7530_MFC, CPU_EN | CPU_PORT_MASK, val);
+}
+
static int mt7988_pad_setup(struct dsa_switch *ds, phy_interface_t interface)
{
return 0;
@@ -3130,6 +3158,7 @@ const struct dsa_switch_ops mt7530_switch_ops = {
.phylink_mac_link_up = mt753x_phylink_mac_link_up,
.get_mac_eee = mt753x_get_mac_eee,
.set_mac_eee = mt753x_set_mac_eee,
+ .conduit_state_change = mt753x_conduit_state_change,
};
EXPORT_SYMBOL_GPL(mt7530_switch_ops);
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 17e42d30fff4..ebfb3a7acfcd 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -41,8 +41,8 @@ enum mt753x_id {
#define UNU_FFP(x) (((x) & 0xff) << 8)
#define UNU_FFP_MASK UNU_FFP(~0)
#define CPU_EN BIT(7)
-#define CPU_PORT(x) ((x) << 4)
-#define CPU_MASK (0xf << 4)
+#define CPU_PORT_MASK GENMASK(6, 4)
+#define CPU_PORT(x) FIELD_PREP(CPU_PORT_MASK, x)
#define MIRROR_EN BIT(3)
#define MIRROR_PORT(x) ((x) & 0x7)
#define MIRROR_MASK 0x7
@@ -760,6 +760,7 @@ struct mt753x_info {
* @irq_domain: IRQ domain of the switch irq_chip
* @irq_enable: IRQ enable bits, synced to SYS_INT_EN
* @create_sgmii: Pointer to function creating SGMII PCS instance(s)
+ * @active_cpu_ports: Holding the active CPU ports
*/
struct mt7530_priv {
struct device *dev;
@@ -786,6 +787,7 @@ struct mt7530_priv {
struct irq_domain *irq_domain;
u32 irq_enable;
int (*create_sgmii)(struct mt7530_priv *priv, bool dual_sgmii);
+ u8 active_cpu_ports;
};
struct mt7530_hw_vlan_entry {
--
2.40.1
Introduce the p5_sgmii field to store the information for whether port 5
has got SGMII or not. Instead of reading the MT7531_TOP_SIG_SR register
multiple times, the register will be read once and the value will be
stored on the p5_sgmii field. This saves unnecessary reads of the
register.
Move the comment about MT7531AE and MT7531BE to mt7531_setup(), where the
switch is identified.
Get rid of mt7531_dual_sgmii_supported() now that priv->p5_sgmii stores the
information. Address the code where mt7531_dual_sgmii_supported() is used.
Get rid of mt7531_is_rgmii_port() which just prints the opposite of
priv->p5_sgmii.
Instead of calling mt7531_pll_setup() then returning, do not call it if
port 5 is SGMII.
Remove P5_INTF_SEL_GMAC5_SGMII. The p5_interface_select enum is supposed to
represent the mode that port 5 is being used in, not the hardware
information of port 5. Set p5_intf_sel to P5_INTF_SEL_GMAC5 instead, if
port 5 is not dsa_is_unused_port().
Signed-off-by: Arınç ÜNAL <[email protected]>
Acked-by: Daniel Golle <[email protected]>
Reviewed-by: Vladimir Oltean <[email protected]>
---
drivers/net/dsa/mt7530-mdio.c | 7 ++---
drivers/net/dsa/mt7530.c | 48 ++++++++++++-----------------------
drivers/net/dsa/mt7530.h | 6 +++--
3 files changed, 22 insertions(+), 39 deletions(-)
diff --git a/drivers/net/dsa/mt7530-mdio.c b/drivers/net/dsa/mt7530-mdio.c
index 088533663b83..fa3ee85a99c1 100644
--- a/drivers/net/dsa/mt7530-mdio.c
+++ b/drivers/net/dsa/mt7530-mdio.c
@@ -81,17 +81,14 @@ static const struct regmap_bus mt7530_regmap_bus = {
};
static int
-mt7531_create_sgmii(struct mt7530_priv *priv, bool dual_sgmii)
+mt7531_create_sgmii(struct mt7530_priv *priv)
{
struct regmap_config *mt7531_pcs_config[2] = {};
struct phylink_pcs *pcs;
struct regmap *regmap;
int i, ret = 0;
- /* MT7531AE has two SGMII units for port 5 and port 6
- * MT7531BE has only one SGMII unit for port 6
- */
- for (i = dual_sgmii ? 0 : 1; i < 2; i++) {
+ for (i = priv->p5_sgmii ? 0 : 1; i < 2; i++) {
mt7531_pcs_config[i] = devm_kzalloc(priv->dev,
sizeof(struct regmap_config),
GFP_KERNEL);
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 436d5c311be0..ab9b291c7a5f 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -487,15 +487,6 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
return 0;
}
-static bool mt7531_dual_sgmii_supported(struct mt7530_priv *priv)
-{
- u32 val;
-
- val = mt7530_read(priv, MT7531_TOP_SIG_SR);
-
- return (val & PAD_DUAL_SGMII_EN) != 0;
-}
-
static int
mt7531_pad_setup(struct dsa_switch *ds, phy_interface_t interface)
{
@@ -510,9 +501,6 @@ mt7531_pll_setup(struct mt7530_priv *priv)
u32 xtal;
u32 val;
- if (mt7531_dual_sgmii_supported(priv))
- return;
-
val = mt7530_read(priv, MT7531_CREV);
top_sig = mt7530_read(priv, MT7531_TOP_SIG_SR);
hwstrap = mt7530_read(priv, MT7531_HWTRAP);
@@ -920,8 +908,6 @@ static const char *p5_intf_modes(unsigned int p5_interface)
return "PHY P4";
case P5_INTF_SEL_GMAC5:
return "GMAC5";
- case P5_INTF_SEL_GMAC5_SGMII:
- return "GMAC5_SGMII";
default:
return "unknown";
}
@@ -2470,6 +2456,12 @@ mt7531_setup(struct dsa_switch *ds)
return -ENODEV;
}
+ /* MT7531AE has got two SGMII units. One for port 5, one for port 6.
+ * MT7531BE has got only one SGMII unit which is for port 6.
+ */
+ val = mt7530_read(priv, MT7531_TOP_SIG_SR);
+ priv->p5_sgmii = !!(val & PAD_DUAL_SGMII_EN);
+
/* 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);
@@ -2479,21 +2471,18 @@ mt7531_setup(struct dsa_switch *ds)
SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
SYS_CTRL_REG_RST);
- mt7531_pll_setup(priv);
-
- if (mt7531_dual_sgmii_supported(priv)) {
- priv->p5_intf_sel = P5_INTF_SEL_GMAC5_SGMII;
-
+ if (!priv->p5_sgmii) {
+ mt7531_pll_setup(priv);
+ } else {
/* Let ds->user_mii_bus be able to access external phy. */
mt7530_rmw(priv, MT7531_GPIO_MODE1, MT7531_GPIO11_RG_RXD2_MASK,
MT7531_EXT_P_MDC_11);
mt7530_rmw(priv, MT7531_GPIO_MODE1, MT7531_GPIO12_RG_RXD3_MASK,
MT7531_EXT_P_MDIO_12);
- } else {
- priv->p5_intf_sel = P5_INTF_SEL_GMAC5;
}
- dev_dbg(ds->dev, "P5 support %s interface\n",
- p5_intf_modes(priv->p5_intf_sel));
+
+ if (!dsa_is_unused_port(ds, 5))
+ priv->p5_intf_sel = P5_INTF_SEL_GMAC5;
mt7530_rmw(priv, MT7531_GPIO_MODE0, MT7531_GPIO0_MASK,
MT7531_GPIO0_INTERRUPT);
@@ -2553,11 +2542,6 @@ static void mt7530_mac_port_get_caps(struct dsa_switch *ds, int port,
}
}
-static bool mt7531_is_rgmii_port(struct mt7530_priv *priv, u32 port)
-{
- return (port == 5) && (priv->p5_intf_sel != P5_INTF_SEL_GMAC5_SGMII);
-}
-
static void mt7531_mac_port_get_caps(struct dsa_switch *ds, int port,
struct phylink_config *config)
{
@@ -2570,7 +2554,7 @@ static void mt7531_mac_port_get_caps(struct dsa_switch *ds, int port,
break;
case 5: /* 2nd cpu port supports either rgmii or sgmii/8023z */
- if (mt7531_is_rgmii_port(priv, port)) {
+ if (!priv->p5_sgmii) {
phy_interface_set_rgmii(config->supported_interfaces);
break;
}
@@ -2637,7 +2621,7 @@ static int mt7531_rgmii_setup(struct mt7530_priv *priv, u32 port,
{
u32 val;
- if (!mt7531_is_rgmii_port(priv, port)) {
+ if (priv->p5_sgmii) {
dev_err(priv->dev, "RGMII mode is not available for port %d\n",
port);
return -EINVAL;
@@ -2881,7 +2865,7 @@ mt7531_cpu_port_config(struct dsa_switch *ds, int port)
switch (port) {
case 5:
- if (mt7531_is_rgmii_port(priv, port))
+ if (!priv->p5_sgmii)
interface = PHY_INTERFACE_MODE_RGMII;
else
interface = PHY_INTERFACE_MODE_2500BASEX;
@@ -3033,7 +3017,7 @@ mt753x_setup(struct dsa_switch *ds)
mt7530_free_irq_common(priv);
if (priv->create_sgmii) {
- ret = priv->create_sgmii(priv, mt7531_dual_sgmii_supported(priv));
+ ret = priv->create_sgmii(priv);
if (ret && priv->irq)
mt7530_free_irq(priv);
}
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 9cbf18efa416..80060cc740d2 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -687,7 +687,6 @@ enum p5_interface_select {
P5_INTF_SEL_PHY_P0,
P5_INTF_SEL_PHY_P4,
P5_INTF_SEL_GMAC5,
- P5_INTF_SEL_GMAC5_SGMII,
};
struct mt7530_priv;
@@ -756,6 +755,8 @@ struct mt753x_info {
* registers
* @p6_interface Holding the current port 6 interface
* @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
* @irq: IRQ number of the switch
* @irq_domain: IRQ domain of the switch irq_chip
* @irq_enable: IRQ enable bits, synced to SYS_INT_EN
@@ -777,6 +778,7 @@ struct mt7530_priv {
phy_interface_t p6_interface;
phy_interface_t p5_interface;
enum p5_interface_select p5_intf_sel;
+ bool p5_sgmii;
u8 mirror_rx;
u8 mirror_tx;
struct mt7530_port ports[MT7530_NUM_PORTS];
@@ -786,7 +788,7 @@ struct mt7530_priv {
int irq;
struct irq_domain *irq_domain;
u32 irq_enable;
- int (*create_sgmii)(struct mt7530_priv *priv, bool dual_sgmii);
+ int (*create_sgmii)(struct mt7530_priv *priv);
u8 active_cpu_ports;
};
--
2.40.1
Use the p5_interface_select enumeration as the data type for the
p5_intf_sel field. This ensures p5_intf_sel can only take the values
defined in the p5_interface_select enumeration.
Remove the explicit assignment of 0 to P5_DISABLED as the first enum item
is automatically assigned 0.
Signed-off-by: Arınç ÜNAL <[email protected]>
Acked-by: Daniel Golle <[email protected]>
Reviewed-by: Vladimir Oltean <[email protected]>
---
drivers/net/dsa/mt7530.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index ebfb3a7acfcd..9cbf18efa416 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -683,7 +683,7 @@ struct mt7530_port {
/* Port 5 interface select definitions */
enum p5_interface_select {
- P5_DISABLED = 0,
+ P5_DISABLED,
P5_INTF_SEL_PHY_P0,
P5_INTF_SEL_PHY_P4,
P5_INTF_SEL_GMAC5,
@@ -776,7 +776,7 @@ struct mt7530_priv {
bool mcm;
phy_interface_t p6_interface;
phy_interface_t p5_interface;
- unsigned int p5_intf_sel;
+ enum p5_interface_select p5_intf_sel;
u8 mirror_rx;
u8 mirror_tx;
struct mt7530_port ports[MT7530_NUM_PORTS];
--
2.40.1
priv->p5_interface and priv->p6_interface are for use on the MT7531 switch.
They prevent the CPU ports of MT7531 to be configured again. They are
useless for MT7530. Therefore, remove setting priv->p5_interface for
MT7530.
Signed-off-by: Arınç ÜNAL <[email protected]>
---
drivers/net/dsa/mt7530.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index bf6c59ecc489..07c5f1c6d036 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -978,8 +978,6 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
dev_dbg(ds->dev, "Setup P5, HWTRAP=0x%x, intf_sel=%s, phy-mode=%s\n",
val, p5_intf_modes(priv->p5_intf_sel), phy_modes(interface));
- priv->p5_interface = interface;
-
unlock_exit:
mutex_unlock(&priv->reg_mutex);
}
--
2.40.1
There's no logic to numerically order the CPU ports. State the port number
and its capability of being used as a CPU port instead.
Remove the irrelevant PHY muxing information from
mt7530_mac_port_get_caps(). Explain the supported MII modes instead.
Remove the out of place PHY muxing information from
mt753x_phylink_mac_config(). The function is for MT7530, MT7531, and the
switch on the MT7988 SoC but there's no PHY muxing on MT7531 or the switch
on the MT7988 SoC.
These comments were gradually introduced with the commits below.
ca366d6c889b ("net: dsa: mt7530: Convert to PHYLINK API")
38f790a80560 ("net: dsa: mt7530: Add support for port 5")
88bdef8be9f6 ("net: dsa: mt7530: Extend device data ready for adding a new
hardware")
c288575f7810 ("net: dsa: mt7530: Add the support of MT7531 switch")
Signed-off-by: Arınç ÜNAL <[email protected]>
Acked-by: Daniel Golle <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
---
drivers/net/dsa/mt7530.c | 32 ++++++++++++++++++++++----------
1 file changed, 22 insertions(+), 10 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index ab9b291c7a5f..558784f830c2 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2520,12 +2520,16 @@ static void mt7530_mac_port_get_caps(struct dsa_switch *ds, int port,
struct phylink_config *config)
{
switch (port) {
- case 0 ... 4: /* Internal phy */
+ /* Internal PHY */
+ case 0 ... 4:
__set_bit(PHY_INTERFACE_MODE_GMII,
config->supported_interfaces);
break;
- case 5: /* 2nd cpu port with phy of port 0 or 4 / external phy */
+ /* Port 5 which can be used as a CPU port supports rgmii with delays,
+ * mii, and gmii.
+ */
+ case 5:
phy_interface_set_rgmii(config->supported_interfaces);
__set_bit(PHY_INTERFACE_MODE_MII,
config->supported_interfaces);
@@ -2533,7 +2537,8 @@ static void mt7530_mac_port_get_caps(struct dsa_switch *ds, int port,
config->supported_interfaces);
break;
- case 6: /* 1st cpu port */
+ /* Port 6 which can be used as a CPU port supports rgmii and trgmii. */
+ case 6:
__set_bit(PHY_INTERFACE_MODE_RGMII,
config->supported_interfaces);
__set_bit(PHY_INTERFACE_MODE_TRGMII,
@@ -2548,19 +2553,24 @@ static void mt7531_mac_port_get_caps(struct dsa_switch *ds, int port,
struct mt7530_priv *priv = ds->priv;
switch (port) {
- case 0 ... 4: /* Internal phy */
+ /* Internal PHY */
+ case 0 ... 4:
__set_bit(PHY_INTERFACE_MODE_GMII,
config->supported_interfaces);
break;
- case 5: /* 2nd cpu port supports either rgmii or sgmii/8023z */
+ /* Port 5 which can be used as a CPU port supports rgmii with delays on
+ * MT7531BE, sgmii/802.3z on MT7531AE.
+ */
+ case 5:
if (!priv->p5_sgmii) {
phy_interface_set_rgmii(config->supported_interfaces);
break;
}
fallthrough;
- case 6: /* 1st cpu port supports sgmii/8023z only */
+ /* Port 6 which can be used as a CPU port supports sgmii/802.3z. */
+ case 6:
__set_bit(PHY_INTERFACE_MODE_SGMII,
config->supported_interfaces);
__set_bit(PHY_INTERFACE_MODE_1000BASEX,
@@ -2579,11 +2589,13 @@ static void mt7988_mac_port_get_caps(struct dsa_switch *ds, int port,
phy_interface_zero(config->supported_interfaces);
switch (port) {
- case 0 ... 4: /* Internal phy */
+ /* Internal PHY */
+ case 0 ... 4:
__set_bit(PHY_INTERFACE_MODE_INTERNAL,
config->supported_interfaces);
break;
+ /* Port 6 which can be used as a CPU port is an internal 10G port. */
case 6:
__set_bit(PHY_INTERFACE_MODE_INTERNAL,
config->supported_interfaces);
@@ -2747,12 +2759,12 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
u32 mcr_cur, mcr_new;
switch (port) {
- case 0 ... 4: /* Internal phy */
+ case 0 ... 4:
if (state->interface != PHY_INTERFACE_MODE_GMII &&
state->interface != PHY_INTERFACE_MODE_INTERNAL)
goto unsupported;
break;
- case 5: /* 2nd cpu port with phy of port 0 or 4 / external phy */
+ case 5:
if (priv->p5_interface == state->interface)
break;
@@ -2762,7 +2774,7 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
if (priv->p5_intf_sel != P5_DISABLED)
priv->p5_interface = state->interface;
break;
- case 6: /* 1st cpu port */
+ case 6:
if (priv->p6_interface == state->interface)
break;
--
2.40.1
There's no need to run all the code on mt7530_setup_port5() if port 5 is
disabled. The only case for calling mt7530_setup_port5() from
mt7530_setup() is when PHY muxing is enabled. That is because port 5 is not
defined as a port on the devicetree, therefore, it cannot be controlled by
phylink.
Because of this, run mt7530_setup_port5() if priv->p5_intf_sel is
P5_INTF_SEL_PHY_P0 or P5_INTF_SEL_PHY_P4. Remove the P5_DISABLED case from
mt7530_setup_port5().
Stop initialising the interface variable as the remaining cases will always
call mt7530_setup_port5() with it initialised.
Signed-off-by: Arınç ÜNAL <[email protected]>
Reviewed-by: Vladimir Oltean <[email protected]>
---
drivers/net/dsa/mt7530.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 07c5f1c6d036..0c3b2b75f106 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -942,9 +942,6 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
/* MT7530_P5_MODE_GMAC: P5 -> External phy or 2nd GMAC */
val &= ~MHWTRAP_P5_DIS;
break;
- case P5_DISABLED:
- interface = PHY_INTERFACE_MODE_NA;
- break;
default:
dev_err(ds->dev, "Unsupported p5_intf_sel %d\n",
priv->p5_intf_sel);
@@ -2313,8 +2310,6 @@ mt7530_setup(struct dsa_switch *ds)
* Set priv->p5_intf_sel to the appropriate value if PHY muxing
* is detected.
*/
- interface = PHY_INTERFACE_MODE_NA;
-
for_each_child_of_node(dn, mac_np) {
if (!of_device_is_compatible(mac_np,
"mediatek,eth-mac"))
@@ -2346,7 +2341,9 @@ mt7530_setup(struct dsa_switch *ds)
break;
}
- mt7530_setup_port5(ds, interface);
+ if (priv->p5_intf_sel == P5_INTF_SEL_PHY_P0 ||
+ priv->p5_intf_sel == P5_INTF_SEL_PHY_P4)
+ mt7530_setup_port5(ds, interface);
}
#ifdef CONFIG_GPIOLIB
--
2.40.1
There're two code paths for setting up port 5:
mt7530_setup()
-> mt7530_setup_port5()
mt753x_phylink_mac_config()
-> mt753x_mac_config()
-> mt7530_mac_config()
-> mt7530_setup_port5()
Currently mt7530_setup_port5() from mt7530_setup() always runs. If port 5
is used as a CPU, DSA, or user port, mt7530_setup_port5() from
mt753x_phylink_mac_config() won't run. That is because priv->p5_interface
set on mt7530_setup_port5() will match state->interface on
mt753x_phylink_mac_config() which will stop running mt7530_setup_port5()
again.
Therefore, mt7530_setup_port5() will never run from
mt753x_phylink_mac_config().
Address this by not running mt7530_setup_port5() from mt7530_setup() if
port 5 is used as a CPU, DSA, or user port. This driver isn't in the
dsa_switches_apply_workarounds[] array so phylink will always be present.
To keep the cases where port 5 isn't controlled by phylink working as
before, preserve the mt7530_setup_port5() call from mt7530_setup().
Do not set priv->p5_intf_sel to P5_DISABLED. It is already set to that when
"priv" is allocated.
Move setting the interface to a more specific location. It's supposed to be
overwritten if PHY muxing is detected.
Improve the comment which explains the process.
Signed-off-by: Arınç ÜNAL <[email protected]>
---
drivers/net/dsa/mt7530.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 558784f830c2..bf6c59ecc489 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2308,16 +2308,15 @@ mt7530_setup(struct dsa_switch *ds)
return ret;
/* Setup port 5 */
- priv->p5_intf_sel = P5_DISABLED;
- interface = PHY_INTERFACE_MODE_NA;
-
if (!dsa_is_unused_port(ds, 5)) {
priv->p5_intf_sel = P5_INTF_SEL_GMAC5;
- ret = of_get_phy_mode(dsa_to_port(ds, 5)->dn, &interface);
- if (ret && ret != -ENODEV)
- return ret;
} else {
- /* Scan the ethernet nodes. look for GMAC1, lookup used phy */
+ /* Scan the ethernet nodes. Look for GMAC1, lookup the used PHY.
+ * Set priv->p5_intf_sel to the appropriate value if PHY muxing
+ * is detected.
+ */
+ interface = PHY_INTERFACE_MODE_NA;
+
for_each_child_of_node(dn, mac_np) {
if (!of_device_is_compatible(mac_np,
"mediatek,eth-mac"))
@@ -2348,6 +2347,8 @@ mt7530_setup(struct dsa_switch *ds)
of_node_put(phy_node);
break;
}
+
+ mt7530_setup_port5(ds, interface);
}
#ifdef CONFIG_GPIOLIB
@@ -2358,8 +2359,6 @@ mt7530_setup(struct dsa_switch *ds)
}
#endif /* CONFIG_GPIOLIB */
- mt7530_setup_port5(ds, interface);
-
/* Flush the FDB table */
ret = mt7530_fdb_cmd(priv, MT7530_FDB_FLUSH, NULL);
if (ret < 0)
--
2.40.1
On Wed, 27 Dec 2023 07:43:40 +0300 Arınç ÜNAL wrote:
> This patch series simplifies the MT7530 DSA subdriver and improves the
> logic of the support for MT7530, MT7531, and the switch on the MT7988 SoC.
>
> I have done a simple ping test to confirm basic communication on all switch
> ports on MCM and standalone MT7530, and MT7531 switch with this patch
> series applied.
Posted during shutdown, rebase & repost, please:
https://lore.kernel.org/all/[email protected]/
On Wed, Dec 27, 2023 at 07:43:41AM +0300, Arınç ÜNAL wrote:
> On the MT7530 switch, the CPU_PORT field indicates which CPU port to trap
> frames to, regardless of the affinity of the inbound user port.
>
> When multiple CPU ports are in use, if the DSA conduit interface is down,
> trapped frames won't be passed to the conduit interface.
>
> To make trapping frames work including this case, implement
> ds->ops->master_state_change() on this subdriver and set the CPU_PORT field
conduit_state_change()
> to the numerically smallest CPU port which the DSA conduit interface its
> affine to is up. Introduce the active_cpu_ports field to store the
> information of the active CPU ports. Correct the macros, CPU_PORT is bits 4
> through 6 of the register.
>
> Add a comment to explain frame trapping for this switch.
>
> Currently, the driver doesn't support the use of multiple CPU ports so this
> is not necessarily a bug fix.
>
> Suggested-by: Vladimir Oltean <[email protected]>
> Suggested-by: Russell King (Oracle) <[email protected]>
> Signed-off-by: Arınç ÜNAL <[email protected]>
> ---
> drivers/net/dsa/mt7530.c | 37 +++++++++++++++++++++++++++++++++----
> drivers/net/dsa/mt7530.h | 6 ++++--
> 2 files changed, 37 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 391c4dbdff42..436d5c311be0 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -1035,10 +1035,6 @@ mt753x_cpu_port_enable(struct dsa_switch *ds, int port)
> mt7530_set(priv, MT7530_MFC, BC_FFP(BIT(port)) | UNM_FFP(BIT(port)) |
> UNU_FFP(BIT(port)));
>
> - /* Set CPU port number */
> - if (priv->id == ID_MT7530 || priv->id == ID_MT7621)
> - mt7530_rmw(priv, MT7530_MFC, CPU_MASK, CPU_EN | CPU_PORT(port));
> -
> /* Add the CPU port to the CPU port bitmap for MT7531 and the switch on
> * the MT7988 SoC. Trapped frames will be forwarded to the CPU port that
> * is affine to the inbound user port.
> @@ -3075,6 +3071,38 @@ static int mt753x_set_mac_eee(struct dsa_switch *ds, int port,
> return 0;
> }
>
> +static void
> +mt753x_conduit_state_change(struct dsa_switch *ds,
> + const struct net_device *conduit,
> + bool operational)
> +{
> + struct dsa_port *cpu_dp = conduit->dsa_ptr;
> + struct mt7530_priv *priv = ds->priv;
> + u8 mask;
> + int val = 0;
Longest line first.
> +
> + /* Set the CPU port to trap frames to for MT7530. Trapped frames will be
> + * forwarded to the numerically smallest CPU port which the DSA conduit
> + * interface its affine to is up.
"first CPU port whose conduit interface is up"
> + */
> + if (priv->id != ID_MT7530 && priv->id != ID_MT7621)
> + return;
> +
> + mask = BIT(cpu_dp->index);
> +
> + if (operational)
> + priv->active_cpu_ports |= mask;
> + else
> + priv->active_cpu_ports &= ~mask;
> +
> + if (priv->active_cpu_ports)
> + val =
> + CPU_EN |
> + CPU_PORT(__ffs((unsigned long)priv->active_cpu_ports));
I don't think the type cast is necessary (implicit type promotion takes place).
Also, it is customary to put {} for multi-line "if" blocks, even if they are
made up of a single expression.
But without the type cast, it could look like this.
if (priv->active_cpu_ports)
val = CPU_EN | CPU_PORT(__ffs(priv->active_cpu_ports));
> +
> + mt7530_rmw(priv, MT7530_MFC, CPU_EN | CPU_PORT_MASK, val);
> +}
On Wed, Dec 27, 2023 at 07:43:44AM +0300, Arınç ÜNAL wrote:
> There's no logic to numerically order the CPU ports. State the port number
> and its capability of being used as a CPU port instead.
>
> Remove the irrelevant PHY muxing information from
> mt7530_mac_port_get_caps(). Explain the supported MII modes instead.
>
> Remove the out of place PHY muxing information from
> mt753x_phylink_mac_config(). The function is for MT7530, MT7531, and the
> switch on the MT7988 SoC but there's no PHY muxing on MT7531 or the switch
> on the MT7988 SoC.
>
> These comments were gradually introduced with the commits below.
> ca366d6c889b ("net: dsa: mt7530: Convert to PHYLINK API")
> 38f790a80560 ("net: dsa: mt7530: Add support for port 5")
> 88bdef8be9f6 ("net: dsa: mt7530: Extend device data ready for adding a new
> hardware")
> c288575f7810 ("net: dsa: mt7530: Add the support of MT7531 switch")
>
> Signed-off-by: Arınç ÜNAL <[email protected]>
> Acked-by: Daniel Golle <[email protected]>
> Reviewed-by: Andrew Lunn <[email protected]>
> ---
Reviewed-by: Vladimir Oltean <[email protected]>
On Wed, Dec 27, 2023 at 07:43:45AM +0300, Arınç ÜNAL wrote:
> There're two code paths for setting up port 5:
>
> mt7530_setup()
> -> mt7530_setup_port5()
>
> mt753x_phylink_mac_config()
> -> mt753x_mac_config()
> -> mt7530_mac_config()
> -> mt7530_setup_port5()
>
> Currently mt7530_setup_port5() from mt7530_setup() always runs. If port 5
> is used as a CPU, DSA, or user port, mt7530_setup_port5() from
> mt753x_phylink_mac_config() won't run. That is because priv->p5_interface
> set on mt7530_setup_port5() will match state->interface on
> mt753x_phylink_mac_config() which will stop running mt7530_setup_port5()
> again.
>
> Therefore, mt7530_setup_port5() will never run from
> mt753x_phylink_mac_config().
>
> Address this by not running mt7530_setup_port5() from mt7530_setup() if
> port 5 is used as a CPU, DSA, or user port. This driver isn't in the
> dsa_switches_apply_workarounds[] array so phylink will always be present.
>
> To keep the cases where port 5 isn't controlled by phylink working as
> before, preserve the mt7530_setup_port5() call from mt7530_setup().
>
> Do not set priv->p5_intf_sel to P5_DISABLED. It is already set to that when
> "priv" is allocated.
>
> Move setting the interface to a more specific location. It's supposed to be
> overwritten if PHY muxing is detected.
>
> Improve the comment which explains the process.
>
> Signed-off-by: Arınç ÜNAL <[email protected]>
> ---
Reviewed-by: Vladimir Oltean <[email protected]>
On Wed, Dec 27, 2023 at 07:43:46AM +0300, Arınç ÜNAL wrote:
> priv->p5_interface and priv->p6_interface are for use on the MT7531 switch.
> They prevent the CPU ports of MT7531 to be configured again. They are
> useless for MT7530. Therefore, remove setting priv->p5_interface for
> MT7530.
>
> Signed-off-by: Arınç ÜNAL <[email protected]>
> ---
What makes priv->p5_interface and priv->p6_interface useless for MT7530
as you say? This code in mt753x_phylink_mac_config() seems executed
regardless of switch family:
case 5:
if (priv->p5_interface == state->interface)
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:
if (priv->p6_interface == state->interface)
break;
mt753x_pad_setup(ds, state);
if (mt753x_mac_config(ds, port, mode, state) < 0)
goto unsupported;
priv->p6_interface = state->interface;
break;
On 4.01.2024 18:22, Vladimir Oltean wrote:
> On Wed, Dec 27, 2023 at 07:43:41AM +0300, Arınç ÜNAL wrote:
>> @@ -3075,6 +3071,38 @@ static int mt753x_set_mac_eee(struct dsa_switch *ds, int port,
>> return 0;
>> }
>>
>> +static void
>> +mt753x_conduit_state_change(struct dsa_switch *ds,
>> + const struct net_device *conduit,
>> + bool operational)
>> +{
>> + struct dsa_port *cpu_dp = conduit->dsa_ptr;
>> + struct mt7530_priv *priv = ds->priv;
>> + u8 mask;
>> + int val = 0;
>
> Longest line first.
>
>> +
>> + /* Set the CPU port to trap frames to for MT7530. Trapped frames will be
>> + * forwarded to the numerically smallest CPU port which the DSA conduit
>> + * interface its affine to is up.
>
> "first CPU port whose conduit interface is up"
>
>> + */
>> + if (priv->id != ID_MT7530 && priv->id != ID_MT7621)
>> + return;
>> +
>> + mask = BIT(cpu_dp->index);
>> +
>> + if (operational)
>> + priv->active_cpu_ports |= mask;
>> + else
>> + priv->active_cpu_ports &= ~mask;
>> +
>> + if (priv->active_cpu_ports)
>> + val =
>> + CPU_EN |
>> + CPU_PORT(__ffs((unsigned long)priv->active_cpu_ports));
>
> I don't think the type cast is necessary (implicit type promotion takes place).
>
> Also, it is customary to put {} for multi-line "if" blocks, even if they are
> made up of a single expression.
>
> But without the type cast, it could look like this.
>
> if (priv->active_cpu_ports)
> val = CPU_EN | CPU_PORT(__ffs(priv->active_cpu_ports));
Will do. Thanks for dealing with my rookie mistakes. :)
Arınç
On 4.01.2024 18:42, Vladimir Oltean wrote:
> On Wed, Dec 27, 2023 at 07:43:46AM +0300, Arınç ÜNAL wrote:
>> priv->p5_interface and priv->p6_interface are for use on the MT7531 switch.
>> They prevent the CPU ports of MT7531 to be configured again. They are
>> useless for MT7530. Therefore, remove setting priv->p5_interface for
>> MT7530.
>>
>> Signed-off-by: Arınç ÜNAL <[email protected]>
>> ---
>
> What makes priv->p5_interface and priv->p6_interface useless for MT7530
> as you say? This code in mt753x_phylink_mac_config() seems executed
> regardless of switch family:
>
> case 5:
> if (priv->p5_interface == state->interface)
> 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:
> if (priv->p6_interface == state->interface)
> break;
>
> mt753x_pad_setup(ds, state);
>
> if (mt753x_mac_config(ds, port, mode, state) < 0)
> goto unsupported;
>
> priv->p6_interface = state->interface;
> break;
This is also useless for non-MT7531 switches in the sense that it
unnecessarily prevents port 5 and 6 from being reconfigured. There's
nothing wrong with configuring them multiple times. These are the remains
of before phylink was implemented on this driver so the thought of changing
phy_interface_t on the fly was non existent. At that time, it was probably
made to apply to all switch models for convenience, as port 5 and 6 are CPU
ports so they're highly likely to be fixed links.
The reason I don't deal with this code block now is because I will get rid
of priv->p5_interface and priv->p6_interface when I also get rid of
priv->info->cpu_port_config() with a later patch.
Arınç
On 6.01.2024 18:00, Arınç ÜNAL wrote:
> On 4.01.2024 18:42, Vladimir Oltean wrote:
>> On Wed, Dec 27, 2023 at 07:43:46AM +0300, Arınç ÜNAL wrote:
>>> priv->p5_interface and priv->p6_interface are for use on the MT7531 switch.
>>> They prevent the CPU ports of MT7531 to be configured again. They are
>>> useless for MT7530. Therefore, remove setting priv->p5_interface for
>>> MT7530.
>>>
>>> Signed-off-by: Arınç ÜNAL <[email protected]>
>>> ---
>>
>> What makes priv->p5_interface and priv->p6_interface useless for MT7530
>> as you say? This code in mt753x_phylink_mac_config() seems executed
>> regardless of switch family:
>>
>> case 5:
>> if (priv->p5_interface == state->interface)
>> 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:
>> if (priv->p6_interface == state->interface)
>> break;
>>
>> mt753x_pad_setup(ds, state);
>>
>> if (mt753x_mac_config(ds, port, mode, state) < 0)
>> goto unsupported;
>>
>> priv->p6_interface = state->interface;
>> break;
>
> This is also useless for non-MT7531 switches in the sense that it
> unnecessarily prevents port 5 and 6 from being reconfigured. There's
> nothing wrong with configuring them multiple times. These are the remains
> of before phylink was implemented on this driver so the thought of changing
> phy_interface_t on the fly was non existent. At that time, it was probably
> made to apply to all switch models for convenience, as port 5 and 6 are CPU
> ports so they're highly likely to be fixed links.
>
> The reason I don't deal with this code block now is because I will get rid
> of priv->p5_interface and priv->p6_interface when I also get rid of
> priv->info->cpu_port_config() with a later patch.
Sorry, I couldn't give a straight answer. Here's the exact answer to this
patch.
Running mt7530_setup_port5() from mt7530_setup() handles all cases of
configuring port 5, including phylink.
Setting priv->p5_interface under mt7530_setup_port5() makes sure that
mt7530_setup_port5() from mt753x_phylink_mac_config() won't run.
The ("net: dsa: mt7530: improve code path for setting up port 5") patch
makes so that mt7530_setup_port5() from mt7530_setup() runs only on
non-phylink cases.
So we get rid of setting priv->p5_interface under mt7530_setup_port5() as
port 5 phylink configuration will be done by running mt7530_setup_port5()
from mt753x_phylink_mac_config() now.
Running mt7530_setup_port5() from mt753x_phylink_mac_config() multiple
times is being prevented which shouldn't be done. That's because of a
different reason involving MT7531. I will deal with this with a later
patch.
I intend to put this on the patch log.
Arınç
On Tue, Jan 09, 2024 at 02:42:45PM +0300, Arınç ÜNAL wrote:
> Running mt7530_setup_port5() from mt753x_phylink_mac_config() multiple
> times is being prevented which shouldn't be done. That's because of a
> different reason involving MT7531. I will deal with this with a later
> patch.
>
> I intend to put this on the patch log.
Still not clear why it shouldn't be done. Ideally you could come up with
a plausible hypothesis as to why it's there in the first place, and why
it's not needed.
On 9.01.2024 14:51, Vladimir Oltean wrote:
> On Tue, Jan 09, 2024 at 02:42:45PM +0300, Arınç ÜNAL wrote:
>> Running mt7530_setup_port5() from mt753x_phylink_mac_config() multiple
>> times is being prevented which shouldn't be done. That's because of a
>> different reason involving MT7531. I will deal with this with a later
>> patch.
>>
>> I intend to put this on the patch log.
>
> Still not clear why it shouldn't be done. Ideally you could come up with
> a plausible hypothesis as to why it's there in the first place, and why
> it's not needed.
I can't tell why it's there. Russell did analyse why it's not needed [1]
and my test [2] confirms it.
This patch is about removing the unnecessary setting of priv->p5_interface
on mt7530_setup_port5() which I believe I have already explained. I would
like to get into the details of priv->p5_interface and priv->p6_interface
when I remove them along with cpu_port_config().
That said, I will refrain from including the last paragraph on the patch
log.
[1] https://lore.kernel.org/netdev/[email protected]/
[2] https://lore.kernel.org/netdev/[email protected]/
Arınç