Hello!
This patch series simplifies the code, improves the logic of the switch
hardware support, traps LLDP frames and BPDUs for MT7530, MT7531, and
MT7988 SoC switches, and introduces the preferring local CPU port
operation.
There's also a patch for fixing the port capabilities of the switch on the
MT7988 SoC.
I have done a bidirectional speed test using iperf3 on all ports of the
MT7530 and MT7531 switches with this patch series applied. I have tested
every possible configuration on the MCM and standalone MT7530 and MT7531
switch. I'll let the name of the dtb files speak for themselves.
MT7621 Unielec:
only-gmac0-mt7621-unielec-u7621-06-16m.dtb
rgmii-only-gmac0-mt7621-unielec-u7621-06-16m.dtb
only-gmac1-mt7621-unielec-u7621-06-16m.dtb
gmac0-and-gmac1-mt7621-unielec-u7621-06-16m.dtb
phy0-muxing-mt7621-unielec-u7621-06-16m.dtb
phy4-muxing-mt7621-unielec-u7621-06-16m.dtb
port5-as-user-mt7621-unielec-u7621-06-16m.dtb
tftpboot 0x80008000 mips-uzImage-next-20230519.bin; tftpboot 0x83000000 mips-rootfs.cpio.uboot; tftpboot 0x83f00000 $dtb; bootm 0x80008000 0x83000000 0x83f00000
MT7622 Bananapi:
only-gmac0-mt7622-bananapi-bpi-r64.dtb
gmac0-and-gmac1-mt7622-bananapi-bpi-r64.dtb
port5-as-user-mt7622-bananapi-bpi-r64.dtb
tftpboot 0x40000000 arm64-Image-next-20230519; tftpboot 0x45000000 arm64-rootfs.cpio.uboot; tftpboot 0x4a000000 $dtb; booti 0x40000000 0x45000000 0x4a000000
MT7623 Bananapi:
only-gmac0-mt7623n-bananapi-bpi-r2.dtb
rgmii-only-gmac0-mt7623n-bananapi-bpi-r2.dtb
only-gmac1-mt7623n-bananapi-bpi-r2.dtb
gmac0-and-gmac1-mt7623n-bananapi-bpi-r2.dtb
phy0-muxing-mt7623n-bananapi-bpi-r2.dtb
phy4-muxing-mt7623n-bananapi-bpi-r2.dtb
port5-as-user-mt7623n-bananapi-bpi-r2.dtb
tftpboot 0x80008000 arm-zImage-next-20230519; tftpboot 0x83000000 arm-rootfs.cpio.uboot; tftpboot 0x83f00000 $dtb; bootz 0x80008000 0x83000000 0x83f00000
Arınç
Arınç ÜNAL (30):
net: dsa: mt7530: add missing @p5_interface to mt7530_priv description
net: dsa: mt7530: use p5_interface_select as data type for p5_intf_sel
net: dsa: mt7530: properly support MT7531AE and MT7531BE
net: dsa: mt7530: improve comments regarding port 5 and 6
net: dsa: mt7530: read XTAL value from correct register
net: dsa: mt7530: improve code path for setting up port 5
net: dsa: mt7530: do not run mt7530_setup_port5() if port 5 is disabled
net: dsa: mt7530: change p{5,6}_interface to p{5,6}_configured
net: dsa: mt7530: empty default case on mt7530_setup_port5()
net: dsa: mt7530: call port 6 setup from mt7530_mac_config()
net: dsa: mt7530: remove pad_setup function pointer
net: dsa: mt7530: move XTAL check to mt7530_setup()
net: dsa: mt7530: move enabling port 6 to mt7530_setup_port6()
net: dsa: mt7530: switch to if/else statements on mt7530_setup_port6()
net: dsa: mt7530: set TRGMII RD TAP if trgmii is being used
net: dsa: mt7530: move lowering port 5 RGMII driving to mt7530_setup()
net: dsa: mt7530: fix port capabilities for MT7988
net: dsa: mt7530: remove .mac_port_config for MT7988 and make it optional
net: dsa: mt7530: set interrupt register only for MT7530
net: dsa: mt7530: properly reset MT7531 switch
net: dsa: mt7530: get rid of useless error returns on phylink code path
net: dsa: mt7530: rename p5_intf_sel and use only for MT7530 switch
net: dsa: mt7530: run mt7530_pll_setup() only with 40 MHz XTAL
net: dsa: mt7530: rename MT7530_MFC to MT753X_MFC
net: dsa: mt7530: properly set MT7531_CPU_PMAP
net: dsa: mt7530: properly set MT7530_CPU_PORT
net: dsa: mt7530: introduce BPDU trapping for MT7530 switch
net: dsa: mt7530: introduce LLDP frame trapping
net: dsa: introduce preferred_default_local_cpu_port and use on MT7530
MAINTAINERS: add me as maintainer of MEDIATEK SWITCH DRIVER
MAINTAINERS | 5 +-
drivers/net/dsa/mt7530-mdio.c | 7 +-
drivers/net/dsa/mt7530.c | 470 ++++++++++++++++---------------------
drivers/net/dsa/mt7530.h | 107 +++++----
include/net/dsa.h | 8 +
net/dsa/dsa.c | 24 +-
6 files changed, 289 insertions(+), 332 deletions(-)
From: Arınç ÜNAL <[email protected]>
According to the document MT7531 Reference Manual for Development Board
v1.0, the SW_PHY_RST bit on the SYS_CTRL register doesn't exist for
MT7531. This is likely why forcing link-down on the MACs is necessary for
MT7531.
Therefore, do not set SW_PHY_RST on mt7531_setup().
Tested-by: Arınç ÜNAL <[email protected]>
Signed-off-by: Arınç ÜNAL <[email protected]>
---
drivers/net/dsa/mt7530.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 0c261ef87bee..aafb7415e2ce 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2416,14 +2416,12 @@ mt7531_setup(struct dsa_switch *ds)
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 */
+ /* Force link-down on all MACs before internal reset */
for (i = 0; i < MT7530_NUM_PORTS; i++)
mt7530_write(priv, MT7530_PMCR_P(i), MT7531_FORCE_LNK);
/* Reset the switch through internal reset */
- mt7530_write(priv, MT7530_SYS_CTRL,
- SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
- SYS_CTRL_REG_RST);
+ mt7530_write(priv, MT7530_SYS_CTRL, SYS_CTRL_SW_RST | SYS_CTRL_REG_RST);
if (!priv->p5_sgmii) {
mt7531_pll_setup(priv);
--
2.39.2
From: Arınç ÜNAL <[email protected]>
The code on mt7530_pll_setup() needs to be run only on the MT7530 switch
with a 40 MHz oscillator. Introduce a check to do this.
Link: https://github.com/BPI-SINOVOIP/BPI-R2-bsp/blob/4a5dd143f2172ec97a2872fa29c7c4cd520f45b5/linux-mt/drivers/net/ethernet/mediatek/gsw_mt7623.c#L1039
Tested-by: Arınç ÜNAL <[email protected]>
Signed-off-by: Arınç ÜNAL <[email protected]>
---
drivers/net/dsa/mt7530.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 19afcd914109..9a4d4413287a 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2197,7 +2197,8 @@ mt7530_setup(struct dsa_switch *ds)
SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
SYS_CTRL_REG_RST);
- mt7530_pll_setup(priv);
+ if (xtal == HWTRAP_XTAL_40MHZ)
+ mt7530_pll_setup(priv);
/* Lower P5 RGMII Tx driving, 8mA */
mt7530_write(priv, MT7530_IO_DRV_CR,
--
2.39.2
From: Arınç ÜNAL <[email protected]>
This code is from before this driver was converted to phylink API. Phylink
deals with the unsupported interface cases before mt7530_setup_port6() is
run. Therefore, the default case would never run. However, it must be
defined nonetheless to handle all the remaining enumeration values, the
phy-modes.
Switch to if/else statements which simplifies the code.
Change mt7530_setup_port6() to void now that there're no error cases left.
Tested-by: Arınç ÜNAL <[email protected]>
Signed-off-by: Arınç ÜNAL <[email protected]>
---
drivers/net/dsa/mt7530.c | 35 +++++++++++------------------------
1 file changed, 11 insertions(+), 24 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 47b89193d4cc..744787e38ecc 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -400,26 +400,28 @@ static void mt7530_pll_setup(struct mt7530_priv *priv)
}
/* Setup port 6 interface mode and TRGMII TX circuit */
-static int
+static void
mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
{
struct mt7530_priv *priv = ds->priv;
- u32 ncpo1, ssc_delta, trgint, xtal;
+ u32 ncpo1, ssc_delta, xtal;
mt7530_clear(priv, MT7530_MHWTRAP, MHWTRAP_P6_DIS);
xtal = mt7530_read(priv, MT7530_HWTRAP) & HWTRAP_XTAL_MASK;
- switch (interface) {
- case PHY_INTERFACE_MODE_RGMII:
- trgint = 0;
- break;
- case PHY_INTERFACE_MODE_TRGMII:
- trgint = 1;
+ if (interface == PHY_INTERFACE_MODE_RGMII) {
+ mt7530_rmw(priv, MT7530_P6ECR, P6_INTF_MODE_MASK,
+ P6_INTF_MODE(0));
+ } else {
+ mt7530_rmw(priv, MT7530_P6ECR, P6_INTF_MODE_MASK,
+ P6_INTF_MODE(1));
+
if (xtal == HWTRAP_XTAL_25MHZ)
ssc_delta = 0x57;
else
ssc_delta = 0x87;
+
if (priv->id == ID_MT7621) {
/* PLL frequency: 125MHz: 1.0GBit */
if (xtal == HWTRAP_XTAL_40MHZ)
@@ -432,17 +434,7 @@ mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
if (xtal == HWTRAP_XTAL_25MHZ)
ncpo1 = 0x1400;
}
- break;
- default:
- dev_err(priv->dev, "xMII interface %d not supported\n",
- interface);
- return -EINVAL;
- }
- mt7530_rmw(priv, MT7530_P6ECR, P6_INTF_MODE_MASK,
- P6_INTF_MODE(trgint));
-
- if (trgint) {
/* Disable the MT7530 TRGMII clocks */
core_clear(priv, CORE_TRGMII_GSW_CLK_CG, REG_TRGMIICK_EN);
@@ -464,8 +456,6 @@ mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
/* Enable the MT7530 TRGMII clocks */
core_set(priv, CORE_TRGMII_GSW_CLK_CG, REG_TRGMIICK_EN);
}
-
- return 0;
}
static void
@@ -2563,14 +2553,11 @@ mt7530_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
phy_interface_t interface)
{
struct mt7530_priv *priv = ds->priv;
- int ret;
if (port == 5) {
mt7530_setup_port5(priv->ds, interface);
} else if (port == 6) {
- ret = mt7530_setup_port6(priv->ds, interface);
- if (ret)
- return ret;
+ mt7530_setup_port6(priv->ds, interface);
}
return 0;
--
2.39.2
From: Arınç ÜNAL <[email protected]>
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.
Signed-off-by: Arınç ÜNAL <[email protected]>
Acked-by: Daniel Golle <[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 845f5dd16d83..415d8ea07472 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -675,7 +675,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,
@@ -768,7 +768,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.39.2
From: Arınç ÜNAL <[email protected]>
Add me as a maintainer of the MediaTek MT7530 DSA subdriver.
List maintainers in alphabetical order by first name.
Signed-off-by: Arınç ÜNAL <[email protected]>
---
MAINTAINERS | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index e2fd64c2ebdc..51e8d30651a8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13254,10 +13254,11 @@ F: drivers/memory/mtk-smi.c
F: include/soc/mediatek/smi.h
MEDIATEK SWITCH DRIVER
-M: Sean Wang <[email protected]>
+M: Arınç ÜNAL <[email protected]>
+M: Daniel Golle <[email protected]>
M: Landen Chao <[email protected]>
M: DENG Qingfang <[email protected]>
-M: Daniel Golle <[email protected]>
+M: Sean Wang <[email protected]>
L: [email protected]
S: Maintained
F: drivers/net/dsa/mt7530-mdio.c
--
2.39.2
From: Vladimir Oltean <[email protected]>
When multiple CPU ports are being used, the numerically smallest CPU port
becomes the port all user ports become affine to. This may not be the best
choice for all switches as there may be a numerically greater CPU port with
more bandwidth than the numerically smallest one.
Such switches are MT7530 and MT7531BE, which the MT7530 DSA subdriver
controls. Port 5 of these switches has got RGMII whilst port 6 has got
either TRGMII or SGMII.
Therefore, introduce the preferred_default_local_cpu_port operation to the
DSA subsystem and use it on the MT7530 DSA subdriver to prefer port 6 as
the default CPU port.
To prove the benefit of this operation, I (Arınç) have done a bidirectional
speed test between two DSA user ports on the MT7531BE switch using iperf3.
The user ports are 1 Gbps full duplex and on different networks so the SoC
MAC would have to do 2 Gbps TX and 2 Gbps RX to deliver full speed.
Without preferring port 6:
[ ID][Role] Interval Transfer Bitrate Retr
[ 5][TX-C] 0.00-20.00 sec 374 MBytes 157 Mbits/sec 734 sender
[ 5][TX-C] 0.00-20.00 sec 373 MBytes 156 Mbits/sec receiver
[ 7][RX-C] 0.00-20.00 sec 1.81 GBytes 778 Mbits/sec 0 sender
[ 7][RX-C] 0.00-20.00 sec 1.81 GBytes 777 Mbits/sec receiver
With preferring port 6:
[ ID][Role] Interval Transfer Bitrate Retr
[ 5][TX-C] 0.00-20.00 sec 1.99 GBytes 856 Mbits/sec 273 sender
[ 5][TX-C] 0.00-20.00 sec 1.99 GBytes 855 Mbits/sec receiver
[ 7][RX-C] 0.00-20.00 sec 1.72 GBytes 737 Mbits/sec 15 sender
[ 7][RX-C] 0.00-20.00 sec 1.71 GBytes 736 Mbits/sec receiver
Using one port for WAN and the other ports for LAN is a very popular use
case which is what this test emulates.
This doesn't affect the remaining switches, MT7531AE and the switch on the
MT7988 SoC. Both CPU ports of the MT7531AE switch have got SGMII and there
is only one CPU port on the switch on the MT7988 SoC.
Signed-off-by: Vladimir Oltean <[email protected]>
Signed-off-by: Arınç ÜNAL <[email protected]>
---
drivers/net/dsa/mt7530.c | 15 +++++++++++++++
include/net/dsa.h | 8 ++++++++
net/dsa/dsa.c | 24 +++++++++++++++++++++++-
3 files changed, 46 insertions(+), 1 deletion(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 8f5a8803cb33..8fd23da76169 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -399,6 +399,20 @@ static void mt7530_pll_setup(struct mt7530_priv *priv)
core_set(priv, CORE_TRGMII_GSW_CLK_CG, REG_GSWCK_EN);
}
+/* If port 6 is available as a CPU port, always prefer that as the default,
+ * otherwise don't care.
+ */
+static struct dsa_port *
+mt753x_preferred_default_local_cpu_port(struct dsa_switch *ds)
+{
+ struct dsa_port *cpu_dp = dsa_to_port(ds, 6);
+
+ if (dsa_port_is_cpu(cpu_dp))
+ return cpu_dp;
+
+ return NULL;
+}
+
/* Setup port 6 interface mode and TRGMII TX circuit */
static void
mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
@@ -3000,6 +3014,7 @@ static int mt7988_setup(struct dsa_switch *ds)
const struct dsa_switch_ops mt7530_switch_ops = {
.get_tag_protocol = mtk_get_tag_protocol,
.setup = mt753x_setup,
+ .preferred_default_local_cpu_port = mt753x_preferred_default_local_cpu_port,
.get_strings = mt7530_get_strings,
.get_ethtool_stats = mt7530_get_ethtool_stats,
.get_sset_count = mt7530_get_sset_count,
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 8903053fa5aa..ab0f0a5b0860 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -958,6 +958,14 @@ struct dsa_switch_ops {
struct phy_device *phy);
void (*port_disable)(struct dsa_switch *ds, int port);
+ /*
+ * Compatibility between device trees defining multiple CPU ports and
+ * drivers which are not OK to use by default the numerically smallest
+ * CPU port of a switch for its local ports. This can return NULL,
+ * meaning "don't know/don't care".
+ */
+ struct dsa_port *(*preferred_default_local_cpu_port)(struct dsa_switch *ds);
+
/*
* Port's MAC EEE settings
*/
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index ab1afe67fd18..1afed89e03c0 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -403,6 +403,24 @@ static int dsa_tree_setup_default_cpu(struct dsa_switch_tree *dst)
return 0;
}
+static struct dsa_port *
+dsa_switch_preferred_default_local_cpu_port(struct dsa_switch *ds)
+{
+ struct dsa_port *cpu_dp;
+
+ if (!ds->ops->preferred_default_local_cpu_port)
+ return NULL;
+
+ cpu_dp = ds->ops->preferred_default_local_cpu_port(ds);
+ if (!cpu_dp)
+ return NULL;
+
+ if (WARN_ON(!dsa_port_is_cpu(cpu_dp) || cpu_dp->ds != ds))
+ return NULL;
+
+ return cpu_dp;
+}
+
/* Perform initial assignment of CPU ports to user ports and DSA links in the
* fabric, giving preference to CPU ports local to each switch. Default to
* using the first CPU port in the switch tree if the port does not have a CPU
@@ -410,12 +428,16 @@ static int dsa_tree_setup_default_cpu(struct dsa_switch_tree *dst)
*/
static int dsa_tree_setup_cpu_ports(struct dsa_switch_tree *dst)
{
- struct dsa_port *cpu_dp, *dp;
+ struct dsa_port *preferred_cpu_dp, *cpu_dp, *dp;
list_for_each_entry(cpu_dp, &dst->ports, list) {
if (!dsa_port_is_cpu(cpu_dp))
continue;
+ preferred_cpu_dp = dsa_switch_preferred_default_local_cpu_port(cpu_dp->ds);
+ if (preferred_cpu_dp && preferred_cpu_dp != cpu_dp)
+ continue;
+
/* Prefer a local CPU port */
dsa_switch_for_each_port(dp, cpu_dp->ds) {
/* Prefer the first local CPU port found */
--
2.39.2
From: Arınç ÜNAL <[email protected]>
Add the missing p5_interface field to the mt7530_priv description. Sort out
the description in the process.
Signed-off-by: Arınç ÜNAL <[email protected]>
Acked-by: Daniel Golle <[email protected]>
---
drivers/net/dsa/mt7530.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 5084f48a8869..845f5dd16d83 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -746,7 +746,8 @@ 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
+ * @p6_interface: Holding the current port 6 interface
+ * @p5_interface: Holding the current port 5 interface
* @p5_intf_sel: Holding the current port 5 interface select
* @irq: IRQ number of the switch
* @irq_domain: IRQ domain of the switch irq_chip
--
2.39.2
From: Arınç ÜNAL <[email protected]>
The MT7530_CPU_PORT bits represent the CPU port to trap frames to for the
MT7530 switch. There are two issues with the current way of setting these
bits. ID_MT7530 which is for the standalone MT7530 switch is not included.
When multiple CPU ports are being used, the trapped frames won't be
received when the DSA conduit interface, which the frames are supposed to
be trapped to, is down because it's not affine to any user port. This
requires the DSA conduit interface to be manually set up for the trapped
frames to be received.
Address these issues by implementing ds->ops->master_state_change() on this
subdriver and setting the MT7530_CPU_PORT bits there. Introduce the
active_cpu_ports field to store the information of active CPU ports.
Correct the macros, MT7530_CPU_PORT is bits 4 through 6 of the register.
Any frames set for trapping to CPU port will be trapped to the numerically
smallest CPU port which is affine to the DSA conduit interface that is set
up. To make the understatement obvious, the frames won't necessarily be
trapped to the CPU port the user port, which these frames are received
from, is affine to. This operation is only there to make sure the trapped
frames always reach the CPU.
Tested-by: Arınç ÜNAL <[email protected]>
Co-developed-by: Vladimir Oltean <[email protected]>
Signed-off-by: Vladimir Oltean <[email protected]>
Signed-off-by: Arınç ÜNAL <[email protected]>
---
drivers/net/dsa/mt7530.c | 33 ++++++++++++++++++++++++++++-----
drivers/net/dsa/mt7530.h | 6 ++++--
2 files changed, 32 insertions(+), 7 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 0b513e3628fe..cd16911fcb01 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -958,11 +958,6 @@ mt753x_cpu_port_enable(struct dsa_switch *ds, int port)
mt7530_set(priv, MT753X_MFC, MT753X_BC_FFP(BIT(port)) |
MT753X_UNM_FFP(BIT(port)) | MT753X_UNU_FFP(BIT(port)));
- /* Set CPU port number */
- if (priv->id == ID_MT7621)
- mt7530_rmw(priv, MT753X_MFC, MT7530_CPU_MASK, MT7530_CPU_EN |
- MT7530_CPU_PORT(port));
-
/* Add the CPU port to the CPU port bitmap for MT7531 and the switch on
* the MT7988 SoC. Any frames set for trapping to CPU port will be
* trapped to the CPU port the user port is affine to.
@@ -2947,6 +2942,33 @@ static int mt753x_set_mac_eee(struct dsa_switch *ds, int port,
return 0;
}
+static void
+mt753x_master_state_change(struct dsa_switch *ds,
+ const struct net_device *master,
+ bool operational)
+{
+ struct mt7530_priv *priv = ds->priv;
+ struct dsa_port *cpu_dp = master->dsa_ptr;
+
+ /* Set the CPU port to trap frames to for MT7530. There can be only one
+ * CPU port due to MT7530_CPU_PORT having only 3 bits. Any frames set
+ * for trapping to CPU port will be trapped to the numerically smallest
+ * CPU port which is affine to the DSA conduit interface that is set up.
+ */
+ if (priv->id != ID_MT7530 && priv->id != ID_MT7621)
+ return;
+
+ if (operational)
+ priv->active_cpu_ports |= BIT(cpu_dp->index);
+ else
+ priv->active_cpu_ports &= ~BIT(cpu_dp->index);
+
+ if (priv->active_cpu_ports)
+ mt7530_rmw(priv, MT753X_MFC, MT7530_CPU_EN |
+ MT7530_CPU_PORT_MASK, MT7530_CPU_EN |
+ MT7530_CPU_PORT(__ffs(priv->active_cpu_ports)));
+}
+
static int mt7988_setup(struct dsa_switch *ds)
{
struct mt7530_priv *priv = ds->priv;
@@ -2996,6 +3018,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,
+ .master_state_change = mt753x_master_state_change,
};
EXPORT_SYMBOL_GPL(mt7530_switch_ops);
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index fd2a2f726b8a..52e5d71a04d3 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -41,8 +41,8 @@ enum mt753x_id {
#define MT753X_UNU_FFP(x) (((x) & 0xff) << 8)
#define MT753X_UNU_FFP_MASK MT753X_UNU_FFP(~0)
#define MT7530_CPU_EN BIT(7)
-#define MT7530_CPU_PORT(x) ((x) << 4)
-#define MT7530_CPU_MASK (0xf << 4)
+#define MT7530_CPU_PORT(x) (((x) & 0x7) << 4)
+#define MT7530_CPU_PORT_MASK MT7530_CPU_PORT(~0)
#define MT7530_MIRROR_EN BIT(3)
#define MT7530_MIRROR_PORT(x) ((x) & 0x7)
#define MT7530_MIRROR_MASK 0x7
@@ -753,6 +753,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;
@@ -780,6 +781,7 @@ struct mt7530_priv {
struct irq_domain *irq_domain;
u32 irq_enable;
int (*create_sgmii)(struct mt7530_priv *priv);
+ unsigned long active_cpu_ports;
};
struct mt7530_hw_vlan_entry {
--
2.39.2
From: Arınç ÜNAL <[email protected]>
mt7530_pad_clk_setup() is called if port 6 is enabled. It used to do more
things than setting up port 6. That part was moved to more appropriate
locations, mt7530_setup() and mt7530_pll_setup().
Now that all it does is set up port 6, rename it to mt7530_setup_port6(),
and move it to a more appropriate location, under mt7530_mac_config().
Leave an empty mt7530_pad_clk_setup() to satisfy the pad_setup function
pointer.
This is the code path for setting up the ports before:
mt753x_phylink_mac_config()
-> mt753x_mac_config()
-> mt7530_mac_config()
-> mt7530_setup_port5()
-> mt753x_pad_setup()
-> mt7530_pad_clk_setup()
This is after:
mt753x_phylink_mac_config()
-> mt753x_mac_config()
-> mt7530_mac_config()
-> mt7530_setup_port5()
-> mt7530_setup_port6()
Tested-by: Arınç ÜNAL <[email protected]>
Signed-off-by: Arınç ÜNAL <[email protected]>
---
drivers/net/dsa/mt7530.c | 21 +++++++++++++++------
1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 50f150ff481a..0b0ed1bd2afa 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -401,7 +401,7 @@ static void mt7530_pll_setup(struct mt7530_priv *priv)
/* Setup port 6 interface mode and TRGMII TX circuit */
static int
-mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
+mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
{
struct mt7530_priv *priv = ds->priv;
u32 ncpo1, ssc_delta, trgint, xtal;
@@ -473,6 +473,12 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
return 0;
}
+static int
+mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
+{
+ return 0;
+}
+
static int
mt7531_pad_setup(struct dsa_switch *ds, phy_interface_t interface)
{
@@ -2571,12 +2577,15 @@ mt7530_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
phy_interface_t interface)
{
struct mt7530_priv *priv = ds->priv;
+ int ret;
- /* Only need to setup port5. */
- if (port != 5)
- return 0;
-
- mt7530_setup_port5(priv->ds, interface);
+ if (port == 5) {
+ mt7530_setup_port5(priv->ds, interface);
+ } else if (port == 6) {
+ ret = mt7530_setup_port6(priv->ds, interface);
+ if (ret)
+ return ret;
+ }
return 0;
}
--
2.39.2
From: Arınç ÜNAL <[email protected]>
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.
Tested-by: Arınç ÜNAL <[email protected]>
Signed-off-by: Arınç ÜNAL <[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 b8f159afcd45..710c6622d648 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -929,9 +929,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);
@@ -2279,8 +2276,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"))
@@ -2312,7 +2307,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.39.2
From: Arınç ÜNAL <[email protected]>
Setting this register related to interrupts is only needed for the MT7530
switch. Make an exclusive check to ensure this.
Signed-off-by: Arınç ÜNAL <[email protected]>
Acked-by: Daniel Golle <[email protected]>
Tested-by: Daniel Golle <[email protected]>
---
drivers/net/dsa/mt7530.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 99f5da8b27be..0c261ef87bee 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2029,7 +2029,7 @@ mt7530_setup_irq(struct mt7530_priv *priv)
}
/* This register must be set for MT7530 to properly fire interrupts */
- if (priv->id != ID_MT7531)
+ if (priv->id == ID_MT7530 || priv->id == ID_MT7621)
mt7530_set(priv, MT7530_TOP_SIG_CTRL, TOP_SIG_CTRL_NORMAL);
ret = request_threaded_irq(priv->irq, NULL, mt7530_irq_thread_fn,
--
2.39.2
From: Arınç ÜNAL <[email protected]>
For the switch on the MT7988 SoC, the code in mac_port_config for MT7988 is
not needed as the interface of the CPU port is already handled on
mt7988_mac_port_get_caps().
Make .mac_port_config optional. Before calling
priv->info->mac_port_config(), if there's no mac_port_config member in the
priv->info table, exit mt753x_mac_config() successfully.
Remove mac_port_config from the sanity check as the sanity check requires a
pointer to a mac_port_config function to be non-NULL. This will fail for
MT7988 as mac_port_config won't be a member of its info table.
Co-developed-by: Daniel Golle <[email protected]>
Signed-off-by: Daniel Golle <[email protected]>
Signed-off-by: Arınç ÜNAL <[email protected]>
---
drivers/net/dsa/mt7530.c | 18 ++++--------------
1 file changed, 4 insertions(+), 14 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index f017cc028183..99f5da8b27be 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2614,17 +2614,6 @@ static bool mt753x_is_mac_port(u32 port)
return (port == 5 || port == 6);
}
-static int
-mt7988_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
- phy_interface_t interface)
-{
- if (dsa_is_cpu_port(ds, port) &&
- interface == PHY_INTERFACE_MODE_INTERNAL)
- return 0;
-
- return -EINVAL;
-}
-
static int
mt7531_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
phy_interface_t interface)
@@ -2665,6 +2654,9 @@ mt753x_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
{
struct mt7530_priv *priv = ds->priv;
+ if (!priv->info->mac_port_config)
+ return 0;
+
return priv->info->mac_port_config(ds, port, mode, state->interface);
}
@@ -3108,7 +3100,6 @@ const struct mt753x_info mt753x_table[] = {
.phy_write_c45 = mt7531_ind_c45_phy_write,
.cpu_port_config = mt7988_cpu_port_config,
.mac_port_get_caps = mt7988_mac_port_get_caps,
- .mac_port_config = mt7988_mac_config,
},
};
EXPORT_SYMBOL_GPL(mt753x_table);
@@ -3136,8 +3127,7 @@ mt7530_probe_common(struct mt7530_priv *priv)
* properly.
*/
if (!priv->info->sw_setup || !priv->info->phy_read_c22 ||
- !priv->info->phy_write_c22 || !priv->info->mac_port_get_caps ||
- !priv->info->mac_port_config)
+ !priv->info->phy_write_c22 || !priv->info->mac_port_get_caps)
return -EINVAL;
priv->id = priv->info->id;
--
2.39.2
From: Arınç ÜNAL <[email protected]>
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 both the MT7530 and MT7531
switches but there's no PHY muxing on MT7531.
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]>
---
drivers/net/dsa/mt7530.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 024b853f9558..b28d66a7c0b2 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2499,7 +2499,9 @@ static void mt7530_mac_port_get_caps(struct dsa_switch *ds, int port,
config->supported_interfaces);
break;
- case 5: /* 2nd cpu port with phy of port 0 or 4 / external phy */
+ case 5: /* Port 5 which can be used as a CPU port supports rgmii with
+ * delays, mii, and gmii.
+ */
phy_interface_set_rgmii(config->supported_interfaces);
__set_bit(PHY_INTERFACE_MODE_MII,
config->supported_interfaces);
@@ -2507,7 +2509,9 @@ static void mt7530_mac_port_get_caps(struct dsa_switch *ds, int port,
config->supported_interfaces);
break;
- case 6: /* 1st cpu port */
+ case 6: /* Port 6 which can be used as a CPU port supports rgmii and
+ * trgmii.
+ */
__set_bit(PHY_INTERFACE_MODE_RGMII,
config->supported_interfaces);
__set_bit(PHY_INTERFACE_MODE_TRGMII,
@@ -2527,14 +2531,17 @@ static void mt7531_mac_port_get_caps(struct dsa_switch *ds, int port,
config->supported_interfaces);
break;
- case 5: /* 2nd cpu port supports either rgmii or sgmii/8023z */
+ case 5: /* Port 5 which can be used as a CPU port supports rgmii with
+ * delays on MT7531BE, sgmii/802.3z on MT7531AE.
+ */
if (!priv->p5_sgmii) {
phy_interface_set_rgmii(config->supported_interfaces);
break;
}
fallthrough;
- case 6: /* 1st cpu port supports sgmii/8023z only */
+ case 6: /* Port 6 which can be used as a CPU port supports sgmii/802.3z.
+ */
__set_bit(PHY_INTERFACE_MODE_SGMII,
config->supported_interfaces);
__set_bit(PHY_INTERFACE_MODE_1000BASEX,
@@ -2726,7 +2733,7 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
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: /* Port 5, can be used as a CPU port. */
if (priv->p5_interface == state->interface)
break;
@@ -2736,7 +2743,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: /* Port 6, can be used as a CPU port. */
if (priv->p6_interface == state->interface)
break;
--
2.39.2
From: Arınç ÜNAL <[email protected]>
Every bit of the CPU port bitmap for MT7531 and the switch on the MT7988
SoC represents a CPU port to trap frames to. Currently only the bit that
corresponds to the first found CPU port is set on the bitmap. Introduce the
MT7531_CPU_PMAP macro to individually set the bits of the CPU port bitmap.
Set the CPU port bitmap for MT7531 and the switch on the MT7988 SoC on
mt753x_cpu_port_enable() which runs on a loop for each CPU port. Add
comments to explain this.
According to the document MT7531 Reference Manual for Development Board
v1.0, the MT7531_CPU_PMAP bits are unset after reset so no need to clear it
beforehand. Since there's currently no public document for the switch on
the MT7988 SoC, I assume this is also the case for this switch.
Tested-by: Arınç ÜNAL <[email protected]>
Signed-off-by: Arınç ÜNAL <[email protected]>
---
drivers/net/dsa/mt7530.c | 15 ++++++++-------
drivers/net/dsa/mt7530.h | 3 ++-
2 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 58d8738d94d3..0b513e3628fe 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -963,6 +963,13 @@ mt753x_cpu_port_enable(struct dsa_switch *ds, int port)
mt7530_rmw(priv, MT753X_MFC, MT7530_CPU_MASK, MT7530_CPU_EN |
MT7530_CPU_PORT(port));
+ /* Add the CPU port to the CPU port bitmap for MT7531 and the switch on
+ * the MT7988 SoC. Any frames set for trapping to CPU port will be
+ * trapped to the CPU port the user port is affine to.
+ */
+ if (priv->id == ID_MT7531 || priv->id == ID_MT7988)
+ mt7530_set(priv, MT7531_CFC, MT7531_CPU_PMAP(BIT(port)));
+
/* CPU port gets connected to all user ports of
* the switch.
*/
@@ -2315,15 +2322,9 @@ static int
mt7531_setup_common(struct dsa_switch *ds)
{
struct mt7530_priv *priv = ds->priv;
- struct dsa_port *cpu_dp;
int ret, i;
- /* BPDU to CPU port */
- dsa_switch_for_each_cpu_port(cpu_dp, ds) {
- mt7530_rmw(priv, MT7531_CFC, MT7531_CPU_PMAP_MASK,
- BIT(cpu_dp->index));
- break;
- }
+ /* Trap BPDUs to the CPU port(s) */
mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
MT753X_BPDU_CPU_ONLY);
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 5ebb942b07ef..fd2a2f726b8a 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -53,7 +53,8 @@ enum mt753x_id {
#define MT7531_MIRROR_MASK (0x7 << 16)
#define MT7531_MIRROR_PORT_GET(x) (((x) >> 16) & 0x7)
#define MT7531_MIRROR_PORT_SET(x) (((x) & 0x7) << 16)
-#define MT7531_CPU_PMAP_MASK GENMASK(7, 0)
+#define MT7531_CPU_PMAP(x) ((x) & 0xff)
+#define MT7531_CPU_PMAP_MASK MT7531_CPU_PMAP(~0)
#define MT753X_MIRROR_REG(id) ((((id) == ID_MT7531) || ((id) == ID_MT7988)) ? \
MT7531_CFC : MT753X_MFC)
--
2.39.2
From: Arınç ÜNAL <[email protected]>
The MT7530_MFC register is on MT7530, MT7531, and the switch on the MT7988
SoC. Some bits are for MT7530 only. Call the shared ones MT753X, the
MT7530-specific ones MT7530.
Signed-off-by: Arınç ÜNAL <[email protected]>
---
drivers/net/dsa/mt7530.c | 30 +++++++++++++++------------
drivers/net/dsa/mt7530.h | 44 ++++++++++++++++++++--------------------
2 files changed, 39 insertions(+), 35 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 9a4d4413287a..58d8738d94d3 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -955,12 +955,13 @@ mt753x_cpu_port_enable(struct dsa_switch *ds, int port)
PORT_SPEC_TAG);
/* Enable flooding on the CPU port */
- mt7530_set(priv, MT7530_MFC, BC_FFP(BIT(port)) | UNM_FFP(BIT(port)) |
- UNU_FFP(BIT(port)));
+ mt7530_set(priv, MT753X_MFC, MT753X_BC_FFP(BIT(port)) |
+ MT753X_UNM_FFP(BIT(port)) | MT753X_UNU_FFP(BIT(port)));
/* Set CPU port number */
if (priv->id == ID_MT7621)
- mt7530_rmw(priv, MT7530_MFC, CPU_MASK, CPU_EN | CPU_PORT(port));
+ mt7530_rmw(priv, MT753X_MFC, MT7530_CPU_MASK, MT7530_CPU_EN |
+ MT7530_CPU_PORT(port));
/* CPU port gets connected to all user ports of
* the switch.
@@ -1120,16 +1121,19 @@ mt7530_port_bridge_flags(struct dsa_switch *ds, int port,
flags.val & BR_LEARNING ? 0 : SA_DIS);
if (flags.mask & BR_FLOOD)
- mt7530_rmw(priv, MT7530_MFC, UNU_FFP(BIT(port)),
- flags.val & BR_FLOOD ? UNU_FFP(BIT(port)) : 0);
+ mt7530_rmw(priv, MT753X_MFC, MT753X_UNU_FFP(BIT(port)),
+ flags.val & BR_FLOOD ?
+ MT753X_UNU_FFP(BIT(port)) : 0);
if (flags.mask & BR_MCAST_FLOOD)
- mt7530_rmw(priv, MT7530_MFC, UNM_FFP(BIT(port)),
- flags.val & BR_MCAST_FLOOD ? UNM_FFP(BIT(port)) : 0);
+ mt7530_rmw(priv, MT753X_MFC, MT753X_UNM_FFP(BIT(port)),
+ flags.val & BR_MCAST_FLOOD ?
+ MT753X_UNM_FFP(BIT(port)) : 0);
if (flags.mask & BR_BCAST_FLOOD)
- mt7530_rmw(priv, MT7530_MFC, BC_FFP(BIT(port)),
- flags.val & BR_BCAST_FLOOD ? BC_FFP(BIT(port)) : 0);
+ mt7530_rmw(priv, MT753X_MFC, MT753X_BC_FFP(BIT(port)),
+ flags.val & BR_BCAST_FLOOD ?
+ MT753X_BC_FFP(BIT(port)) : 0);
return 0;
}
@@ -1667,13 +1671,13 @@ mt7530_port_vlan_del(struct dsa_switch *ds, int port,
static int mt753x_mirror_port_get(unsigned int id, u32 val)
{
return (id == ID_MT7531) ? MT7531_MIRROR_PORT_GET(val) :
- MIRROR_PORT(val);
+ MT7530_MIRROR_PORT(val);
}
static int mt753x_mirror_port_set(unsigned int id, u32 val)
{
return (id == ID_MT7531) ? MT7531_MIRROR_PORT_SET(val) :
- MIRROR_PORT(val);
+ MT7530_MIRROR_PORT(val);
}
static int mt753x_port_mirror_add(struct dsa_switch *ds, int port,
@@ -2327,8 +2331,8 @@ mt7531_setup_common(struct dsa_switch *ds)
mt7530_mib_reset(ds);
/* Disable flooding on all ports */
- mt7530_clear(priv, MT7530_MFC, BC_FFP_MASK | UNM_FFP_MASK |
- UNU_FFP_MASK);
+ mt7530_clear(priv, MT753X_MFC, MT753X_BC_FFP_MASK | MT753X_UNM_FFP_MASK
+ | MT753X_UNU_FFP_MASK);
for (i = 0; i < MT7530_NUM_PORTS; i++) {
/* Disable forwarding by default on all ports */
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 216081fb1c12..5ebb942b07ef 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -32,35 +32,35 @@ enum mt753x_id {
#define SYSC_REG_RSTCTRL 0x34
#define RESET_MCM BIT(2)
-/* Registers to mac forward control for unknown frames */
-#define MT7530_MFC 0x10
-#define BC_FFP(x) (((x) & 0xff) << 24)
-#define BC_FFP_MASK BC_FFP(~0)
-#define UNM_FFP(x) (((x) & 0xff) << 16)
-#define UNM_FFP_MASK UNM_FFP(~0)
-#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 MIRROR_EN BIT(3)
-#define MIRROR_PORT(x) ((x) & 0x7)
-#define MIRROR_MASK 0x7
-
-/* Registers for CPU forward control */
+/* Register for MAC forward control */
+#define MT753X_MFC 0x10
+#define MT753X_BC_FFP(x) (((x) & 0xff) << 24)
+#define MT753X_BC_FFP_MASK MT753X_BC_FFP(~0)
+#define MT753X_UNM_FFP(x) (((x) & 0xff) << 16)
+#define MT753X_UNM_FFP_MASK MT753X_UNM_FFP(~0)
+#define MT753X_UNU_FFP(x) (((x) & 0xff) << 8)
+#define MT753X_UNU_FFP_MASK MT753X_UNU_FFP(~0)
+#define MT7530_CPU_EN BIT(7)
+#define MT7530_CPU_PORT(x) ((x) << 4)
+#define MT7530_CPU_MASK (0xf << 4)
+#define MT7530_MIRROR_EN BIT(3)
+#define MT7530_MIRROR_PORT(x) ((x) & 0x7)
+#define MT7530_MIRROR_MASK 0x7
+
+/* Register for CPU forward control */
#define MT7531_CFC 0x4
#define MT7531_MIRROR_EN BIT(19)
-#define MT7531_MIRROR_MASK (MIRROR_MASK << 16)
-#define MT7531_MIRROR_PORT_GET(x) (((x) >> 16) & MIRROR_MASK)
-#define MT7531_MIRROR_PORT_SET(x) (((x) & MIRROR_MASK) << 16)
+#define MT7531_MIRROR_MASK (0x7 << 16)
+#define MT7531_MIRROR_PORT_GET(x) (((x) >> 16) & 0x7)
+#define MT7531_MIRROR_PORT_SET(x) (((x) & 0x7) << 16)
#define MT7531_CPU_PMAP_MASK GENMASK(7, 0)
#define MT753X_MIRROR_REG(id) ((((id) == ID_MT7531) || ((id) == ID_MT7988)) ? \
- MT7531_CFC : MT7530_MFC)
+ MT7531_CFC : MT753X_MFC)
#define MT753X_MIRROR_EN(id) ((((id) == ID_MT7531) || ((id) == ID_MT7988)) ? \
- MT7531_MIRROR_EN : MIRROR_EN)
+ MT7531_MIRROR_EN : MT7530_MIRROR_EN)
#define MT753X_MIRROR_MASK(id) ((((id) == ID_MT7531) || ((id) == ID_MT7988)) ? \
- MT7531_MIRROR_MASK : MIRROR_MASK)
+ MT7531_MIRROR_MASK : MT7530_MIRROR_MASK)
/* Registers for BPDU and PAE frame control*/
#define MT753X_BPC 0x24
--
2.39.2
From: Arınç ÜNAL <[email protected]>
This code sets the Read Data (RD) TAP value to 16 for all TRGMII control
registers.
The for loop iterates over all the TRGMII control registers, and
mt7530_rmw() function is used to perform a read-modify-write operation on
each register's RD_TAP field to set its value to 16.
This operation is used to tune the timing of the read data signal in
TRGMII to match the TX signal of the link partner.
Run this if trgmii is being used. Since this code doesn't lower the
driving, there's no apparent benefit to run this if trgmii is not being
used.
Add a comment to explain the code.
Thanks to 趙皎宏 (Landen Chao) for pointing out what the code does.
Tested-by: Arınç ÜNAL <[email protected]>
Signed-off-by: Arınç ÜNAL <[email protected]>
---
drivers/net/dsa/mt7530.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 744787e38ecc..f2c1aa9cf7f7 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -404,7 +404,7 @@ static void
mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
{
struct mt7530_priv *priv = ds->priv;
- u32 ncpo1, ssc_delta, xtal;
+ u32 ncpo1, ssc_delta, i, xtal;
mt7530_clear(priv, MT7530_MHWTRAP, MHWTRAP_P6_DIS);
@@ -455,6 +455,11 @@ mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
/* Enable the MT7530 TRGMII clocks */
core_set(priv, CORE_TRGMII_GSW_CLK_CG, REG_TRGMIICK_EN);
+
+ /* Set the Read Data TAP value of the MT7530 TRGMII */
+ for (i = 0; i < NUM_TRGMII_CTRL; i++)
+ mt7530_rmw(priv, MT7530_TRGMII_RD(i),
+ RD_TAP_MASK, RD_TAP(16));
}
}
@@ -2209,10 +2214,6 @@ mt7530_setup(struct dsa_switch *ds)
mt7530_write(priv, MT7530_TRGMII_TD_ODT(i),
TD_DM_DRVP(8) | TD_DM_DRVN(8));
- for (i = 0; i < NUM_TRGMII_CTRL; i++)
- mt7530_rmw(priv, MT7530_TRGMII_RD(i),
- RD_TAP_MASK, RD_TAP(16));
-
/* Directly access the PHY registers via C_MDC/C_MDIO. The bit that
* enables modifying the hardware trap must be set for this.
*/
--
2.39.2
From: Arınç ÜNAL <[email protected]>
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.
mt7530_setup_port5() from mt753x_phylink_mac_config() won't run when port 5
is disabled or used for PHY muxing as port 5 won't be defined on the
devicetree.
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. For the cases of PHY muxing or
the port being disabled, call mt7530_setup_port5() from mt7530_setup().
Do not set priv->p5_interface on mt7530_setup_port5(). There won't be a
case where mt753x_phylink_mac_config() runs after mt7530_setup_port5()
anymore.
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.
Tested-by: Arınç ÜNAL <[email protected]>
Signed-off-by: Arınç ÜNAL <[email protected]>
---
drivers/net/dsa/mt7530.c | 19 ++++++++-----------
1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 1a842d6fbc27..b8f159afcd45 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -965,8 +965,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);
}
@@ -2274,16 +2272,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"))
@@ -2314,6 +2311,8 @@ mt7530_setup(struct dsa_switch *ds)
of_node_put(phy_node);
break;
}
+
+ mt7530_setup_port5(ds, interface);
}
#ifdef CONFIG_GPIOLIB
@@ -2324,8 +2323,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.39.2
From: Arınç ÜNAL <[email protected]>
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 <[email protected]>
Signed-off-by: Arınç ÜNAL <[email protected]>
---
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
From: Arınç ÜNAL <[email protected]>
Enable port 6 only when port 6 is being used. Update the comment on
mt7530_setup() with a better explanation. Do not set MHWTRAP_MANUAL on
mt7530_setup_port5() as it's already done on mt7530_setup() beforehand.
Tested-by: Arınç ÜNAL <[email protected]>
Signed-off-by: Arınç ÜNAL <[email protected]>
---
drivers/net/dsa/mt7530.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index fa48273269c4..47b89193d4cc 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -406,6 +406,8 @@ mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
struct mt7530_priv *priv = ds->priv;
u32 ncpo1, ssc_delta, trgint, xtal;
+ mt7530_clear(priv, MT7530_MHWTRAP, MHWTRAP_P6_DIS);
+
xtal = mt7530_read(priv, MT7530_HWTRAP) & HWTRAP_XTAL_MASK;
switch (interface) {
@@ -897,7 +899,7 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
val = mt7530_read(priv, MT7530_MHWTRAP);
- val |= MHWTRAP_MANUAL | MHWTRAP_P5_MAC_SEL | MHWTRAP_P5_DIS;
+ val |= MHWTRAP_P5_MAC_SEL | MHWTRAP_P5_DIS;
val &= ~MHWTRAP_P5_RGMII_MODE & ~MHWTRAP_PHY0_SEL;
switch (priv->p5_intf_sel) {
@@ -2221,9 +2223,11 @@ mt7530_setup(struct dsa_switch *ds)
mt7530_rmw(priv, MT7530_TRGMII_RD(i),
RD_TAP_MASK, RD_TAP(16));
- /* Enable port 6 */
+ /* Directly access the PHY registers via C_MDC/C_MDIO. The bit that
+ * enables modifying the hardware trap must be set for this.
+ */
val = mt7530_read(priv, MT7530_MHWTRAP);
- val &= ~MHWTRAP_P6_DIS & ~MHWTRAP_PHY_ACCESS;
+ val &= ~MHWTRAP_PHY_ACCESS;
val |= MHWTRAP_MANUAL;
mt7530_write(priv, MT7530_MHWTRAP, val);
--
2.39.2
From: Arınç ÜNAL <[email protected]>
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 <[email protected]>
Signed-off-by: Arınç ÜNAL <[email protected]>
Acked-by: Daniel Golle <[email protected]>
---
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 710c6622d648..d837aa20968c 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2234,8 +2234,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);
@@ -2455,10 +2453,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
@@ -2728,25 +2722,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:
@@ -2854,12 +2843,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 2602c95fd3a5..06037be5882c 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;
enum p5_interface_select p5_intf_sel;
bool p5_sgmii;
u8 mirror_rx;
--
2.39.2
From: Arınç ÜNAL <[email protected]>
The pad_setup function pointer was introduced with 88bdef8be9f6 ("net: dsa:
mt7530: Extend device data ready for adding a new hardware"). It was being
used to set up the core clock and port 6 of the MT7530 switch, and pll of
the MT7531 switch.
All of these were moved to more appropriate locations, and it was never
used for the switch on the MT7988 SoC. Therefore, this function pointer
hasn't got a use anymore. Remove it.
Tested-by: Arınç ÜNAL <[email protected]>
Signed-off-by: Arınç ÜNAL <[email protected]>
Acked-by: Daniel Golle <[email protected]>
---
drivers/net/dsa/mt7530.c | 36 ++----------------------------------
drivers/net/dsa/mt7530.h | 3 ---
2 files changed, 2 insertions(+), 37 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 0b0ed1bd2afa..049f7be0d790 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -473,18 +473,6 @@ mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
return 0;
}
-static int
-mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
-{
- return 0;
-}
-
-static int
-mt7531_pad_setup(struct dsa_switch *ds, phy_interface_t interface)
-{
- return 0;
-}
-
static void
mt7531_pll_setup(struct mt7530_priv *priv)
{
@@ -2564,14 +2552,6 @@ static void mt7988_mac_port_get_caps(struct dsa_switch *ds, int port,
}
}
-static int
-mt753x_pad_setup(struct dsa_switch *ds, const struct phylink_link_state *state)
-{
- struct mt7530_priv *priv = ds->priv;
-
- return priv->info->pad_setup(ds, state->interface);
-}
-
static int
mt7530_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
phy_interface_t interface)
@@ -2738,8 +2718,6 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
if (priv->p6_configured)
break;
- mt753x_pad_setup(ds, state);
-
if (mt753x_mac_config(ds, port, mode, state) < 0)
goto unsupported;
break;
@@ -3041,11 +3019,6 @@ static int mt753x_set_mac_eee(struct dsa_switch *ds, int port,
return 0;
}
-static int mt7988_pad_setup(struct dsa_switch *ds, phy_interface_t interface)
-{
- return 0;
-}
-
static int mt7988_setup(struct dsa_switch *ds)
{
struct mt7530_priv *priv = ds->priv;
@@ -3107,7 +3080,6 @@ const struct mt753x_info mt753x_table[] = {
.phy_write_c22 = mt7530_phy_write_c22,
.phy_read_c45 = mt7530_phy_read_c45,
.phy_write_c45 = mt7530_phy_write_c45,
- .pad_setup = mt7530_pad_clk_setup,
.mac_port_get_caps = mt7530_mac_port_get_caps,
.mac_port_config = mt7530_mac_config,
},
@@ -3119,7 +3091,6 @@ const struct mt753x_info mt753x_table[] = {
.phy_write_c22 = mt7530_phy_write_c22,
.phy_read_c45 = mt7530_phy_read_c45,
.phy_write_c45 = mt7530_phy_write_c45,
- .pad_setup = mt7530_pad_clk_setup,
.mac_port_get_caps = mt7530_mac_port_get_caps,
.mac_port_config = mt7530_mac_config,
},
@@ -3131,7 +3102,6 @@ const struct mt753x_info mt753x_table[] = {
.phy_write_c22 = mt7531_ind_c22_phy_write,
.phy_read_c45 = mt7531_ind_c45_phy_read,
.phy_write_c45 = mt7531_ind_c45_phy_write,
- .pad_setup = mt7531_pad_setup,
.cpu_port_config = mt7531_cpu_port_config,
.mac_port_get_caps = mt7531_mac_port_get_caps,
.mac_port_config = mt7531_mac_config,
@@ -3144,7 +3114,6 @@ const struct mt753x_info mt753x_table[] = {
.phy_write_c22 = mt7531_ind_c22_phy_write,
.phy_read_c45 = mt7531_ind_c45_phy_read,
.phy_write_c45 = mt7531_ind_c45_phy_write,
- .pad_setup = mt7988_pad_setup,
.cpu_port_config = mt7988_cpu_port_config,
.mac_port_get_caps = mt7988_mac_port_get_caps,
.mac_port_config = mt7988_mac_config,
@@ -3174,9 +3143,8 @@ mt7530_probe_common(struct mt7530_priv *priv)
/* Sanity check if these required device operations are filled
* properly.
*/
- if (!priv->info->sw_setup || !priv->info->pad_setup ||
- !priv->info->phy_read_c22 || !priv->info->phy_write_c22 ||
- !priv->info->mac_port_get_caps ||
+ if (!priv->info->sw_setup || !priv->info->phy_read_c22 ||
+ !priv->info->phy_write_c22 || !priv->info->mac_port_get_caps ||
!priv->info->mac_port_config)
return -EINVAL;
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 06037be5882c..f7a504e4c17b 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -696,8 +696,6 @@ struct mt753x_pcs {
* @phy_write_c22: Holding the way writing PHY port using C22
* @phy_read_c45: Holding the way reading PHY port using C45
* @phy_write_c45: Holding the way writing PHY port using C45
- * @pad_setup: Holding the way setting up the bus pad for a certain
- * MAC port
* @phy_mode_supported: Check if the PHY type is being supported on a certain
* port
* @mac_port_validate: Holding the way to set addition validate type for a
@@ -718,7 +716,6 @@ struct mt753x_info {
int regnum);
int (*phy_write_c45)(struct mt7530_priv *priv, int port, int devad,
int regnum, u16 val);
- int (*pad_setup)(struct dsa_switch *ds, phy_interface_t interface);
int (*cpu_port_config)(struct dsa_switch *ds, int port);
void (*mac_port_get_caps)(struct dsa_switch *ds, int port,
struct phylink_config *config);
--
2.39.2
From: Arınç ÜNAL <[email protected]>
Remove error returns on the cases where they are already handled with the
function the mac_port_get_caps member points to.
mt7531_mac_config() is also called from mt7531_cpu_port_config() outside of
phylink but the port and interface modes are already handled there.
Change the functions and the mac_port_config function pointer to void now
that there're no error returns anymore.
Remove mt753x_is_mac_port() that used to help the said error returns.
On mt7531_mac_config(), switch to if statements to simplify the code.
Remove internal phy cases from mt753x_phylink_mac_config() as there is no
configuration to be done for them. There's also no need to check the
interface mode as that's already handled with the function the
mac_port_get_caps member points to.
Remove the comments for port 5 and 6 as they're not really useful.
Tested-by: Arınç ÜNAL <[email protected]>
Signed-off-by: Arınç ÜNAL <[email protected]>
Acked-by: Daniel Golle <[email protected]>
Tested-by: Daniel Golle <[email protected]>
---
drivers/net/dsa/mt7530.c | 85 +++++++++-------------------------------
drivers/net/dsa/mt7530.h | 2 +-
2 files changed, 19 insertions(+), 68 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index aafb7415e2ce..996b8c02cb05 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2545,7 +2545,7 @@ static void mt7988_mac_port_get_caps(struct dsa_switch *ds, int port,
}
}
-static int
+static void
mt7530_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
phy_interface_t interface)
{
@@ -2556,22 +2556,14 @@ mt7530_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
} else if (port == 6) {
mt7530_setup_port6(priv->ds, interface);
}
-
- return 0;
}
-static int mt7531_rgmii_setup(struct mt7530_priv *priv, u32 port,
- phy_interface_t interface,
- struct phy_device *phydev)
+static void mt7531_rgmii_setup(struct mt7530_priv *priv, u32 port,
+ phy_interface_t interface,
+ struct phy_device *phydev)
{
u32 val;
- if (priv->p5_sgmii) {
- dev_err(priv->dev, "RGMII mode is not available for port %d\n",
- port);
- return -EINVAL;
- }
-
val = mt7530_read(priv, MT7531_CLKGEN_CTRL);
val |= GP_CLK_EN;
val &= ~GP_MODE_MASK;
@@ -2599,20 +2591,14 @@ static int mt7531_rgmii_setup(struct mt7530_priv *priv, u32 port,
case PHY_INTERFACE_MODE_RGMII_ID:
break;
default:
- return -EINVAL;
+ break;
}
}
- mt7530_write(priv, MT7531_CLKGEN_CTRL, val);
- return 0;
-}
-
-static bool mt753x_is_mac_port(u32 port)
-{
- return (port == 5 || port == 6);
+ mt7530_write(priv, MT7531_CLKGEN_CTRL, val);
}
-static int
+static void
mt7531_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
phy_interface_t interface)
{
@@ -2620,42 +2606,21 @@ mt7531_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
struct phy_device *phydev;
struct dsa_port *dp;
- if (!mt753x_is_mac_port(port)) {
- dev_err(priv->dev, "port %d is not a MAC port\n", port);
- return -EINVAL;
- }
-
- switch (interface) {
- case PHY_INTERFACE_MODE_RGMII:
- case PHY_INTERFACE_MODE_RGMII_ID:
- case PHY_INTERFACE_MODE_RGMII_RXID:
- case PHY_INTERFACE_MODE_RGMII_TXID:
+ if (phy_interface_mode_is_rgmii(interface)) {
dp = dsa_to_port(ds, port);
phydev = dp->slave->phydev;
- return mt7531_rgmii_setup(priv, port, interface, phydev);
- case PHY_INTERFACE_MODE_SGMII:
- case PHY_INTERFACE_MODE_NA:
- case PHY_INTERFACE_MODE_1000BASEX:
- case PHY_INTERFACE_MODE_2500BASEX:
- /* handled in SGMII PCS driver */
- return 0;
- default:
- return -EINVAL;
+ mt7531_rgmii_setup(priv, port, interface, phydev);
}
-
- return -EINVAL;
}
-static int
+static void
mt753x_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
const struct phylink_link_state *state)
{
struct mt7530_priv *priv = ds->priv;
- if (!priv->info->mac_port_config)
- return 0;
-
- return priv->info->mac_port_config(ds, port, mode, state->interface);
+ if (priv->info->mac_port_config)
+ priv->info->mac_port_config(ds, port, mode, state->interface);
}
static struct phylink_pcs *
@@ -2684,30 +2649,18 @@ 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 */
- if (state->interface != PHY_INTERFACE_MODE_GMII &&
- state->interface != PHY_INTERFACE_MODE_INTERNAL)
- goto unsupported;
- break;
- case 5: /* Port 5, can be used as a CPU port. */
+ case 5:
if (priv->p5_configured)
break;
- if (mt753x_mac_config(ds, port, mode, state) < 0)
- goto unsupported;
+ mt753x_mac_config(ds, port, mode, state);
break;
- case 6: /* Port 6, can be used as a CPU port. */
+ case 6:
if (priv->p6_configured)
break;
- if (mt753x_mac_config(ds, port, mode, state) < 0)
- goto unsupported;
+ mt753x_mac_config(ds, port, mode, state);
break;
- default:
-unsupported:
- dev_err(ds->dev, "%s: unsupported %s port: %i\n",
- __func__, phy_modes(state->interface), port);
- return;
}
mcr_cur = mt7530_read(priv, MT7530_PMCR_P(port));
@@ -2800,7 +2753,6 @@ mt7531_cpu_port_config(struct dsa_switch *ds, int port)
struct mt7530_priv *priv = ds->priv;
phy_interface_t interface;
int speed;
- int ret;
switch (port) {
case 5:
@@ -2825,9 +2777,8 @@ mt7531_cpu_port_config(struct dsa_switch *ds, int port)
else
speed = SPEED_1000;
- ret = mt7531_mac_config(ds, port, MLO_AN_FIXED, interface);
- if (ret)
- return ret;
+ mt7531_mac_config(ds, port, MLO_AN_FIXED, interface);
+
mt7530_write(priv, MT7530_PMCR_P(port),
PMCR_CPU_PORT_SETTING(priv->id));
mt753x_phylink_pcs_link_up(&priv->pcs[port].pcs, MLO_AN_FIXED,
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index f7a504e4c17b..b7f80a487073 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -722,7 +722,7 @@ struct mt753x_info {
void (*mac_port_validate)(struct dsa_switch *ds, int port,
phy_interface_t interface,
unsigned long *supported);
- int (*mac_port_config)(struct dsa_switch *ds, int port,
+ void (*mac_port_config)(struct dsa_switch *ds, int port,
unsigned int mode,
phy_interface_t interface);
};
--
2.39.2
From: Arınç ÜNAL <[email protected]>
Move lowering Tx driving of rgmii on port 5 to right before lowering of Tx
driving of trgmii on port 6 on mt7530_setup().
This way, the switch should consume less power regardless of port 5 being
used.
Tested-by: Arınç ÜNAL <[email protected]>
Signed-off-by: Arınç ÜNAL <[email protected]>
---
drivers/net/dsa/mt7530.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index f2c1aa9cf7f7..514e82299537 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -933,10 +933,6 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
/* P5 RGMII TX Clock Control: delay x */
mt7530_write(priv, MT7530_P5RGMIITXCR,
CSR_RGMII_TXC_CFG(0x10 + tx_delay));
-
- /* reduce P5 RGMII Tx driving, 8mA */
- mt7530_write(priv, MT7530_IO_DRV_CR,
- P5_IO_CLK_DRV(1) | P5_IO_DATA_DRV(1));
}
mt7530_write(priv, MT7530_MHWTRAP, val);
@@ -2209,6 +2205,10 @@ mt7530_setup(struct dsa_switch *ds)
mt7530_pll_setup(priv);
+ /* Lower P5 RGMII Tx driving, 8mA */
+ mt7530_write(priv, MT7530_IO_DRV_CR,
+ P5_IO_CLK_DRV(1) | P5_IO_DATA_DRV(1));
+
/* Lower Tx driving for TRGMII path */
for (i = 0; i < NUM_TRGMII_CTRL; i++)
mt7530_write(priv, MT7530_TRGMII_TD_ODT(i),
--
2.39.2
From: Arınç ÜNAL <[email protected]>
Introduce the p5_sgmii field to store the information for whether port 5
has got SGMII or not.
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.
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]>
---
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 9bc54e1348cb..024b853f9558 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -473,15 +473,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)
{
@@ -496,9 +487,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);
@@ -907,8 +895,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";
}
@@ -2444,6 +2430,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);
@@ -2453,21 +2445,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->slave_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);
@@ -2527,11 +2516,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)
{
@@ -2544,7 +2528,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;
}
@@ -2611,7 +2595,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;
@@ -2864,7 +2848,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;
@@ -3023,7 +3007,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 415d8ea07472..2602c95fd3a5 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -679,7 +679,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;
@@ -749,6 +748,8 @@ struct mt753x_info {
* @p6_interface: Holding the current port 6 interface
* @p5_interface: Holding the current port 5 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
@@ -769,6 +770,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];
@@ -778,7 +780,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);
};
struct mt7530_hw_vlan_entry {
--
2.39.2
From: Arınç ÜNAL <[email protected]>
The MT753X switches are capable of trapping certain frames. Introduce
trapping BPDUs to the CPU port for the MT7530 switch.
BPDUs will be trapped to the numerically smallest CPU port which is affine
to the DSA conduit interface that is set up. The BPDUs won't necessarily be
trapped to the CPU port the user port, which these BPDUs are received from,
is affine to.
Tested-by: Arınç ÜNAL <[email protected]>
Signed-off-by: Arınç ÜNAL <[email protected]>
---
drivers/net/dsa/mt7530.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index cd16911fcb01..2fb4b0bc6335 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2223,6 +2223,10 @@ mt7530_setup(struct dsa_switch *ds)
val |= MHWTRAP_MANUAL;
mt7530_write(priv, MT7530_MHWTRAP, val);
+ /* Trap BPDUs to the CPU port */
+ mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
+ MT753X_BPDU_CPU_ONLY);
+
/* Enable and reset MIB counters */
mt7530_mib_reset(ds);
--
2.39.2
From: Arınç ÜNAL <[email protected]>
The MT753X switches are capable of trapping certain frames. Introduce
trapping LLDP frames to the CPU port(s) for the MT753X switches.
For MT7530, LLDP frames will be trapped to the numerically smallest CPU
port which is affine to the DSA conduit interface that is set up. The LLDP
frames won't necessarily be trapped to the CPU port the user port, which
these LLDP frames are received from, is affine to.
For MT7531 and the switch on the MT7988 SoC, LLDP frames will be trapped to
the CPU port the user port is affine to.
The bit for R0E_MANG_FR is 27. When set, the switch regards the frames with
:0E MAC DA as management (LLDP) frames. This bit is set to 1 after reset on
MT7530 and MT7531 according to the documents MT7620 Programming Guide v1.0
and MT7531 Reference Manual for Development Board v1.0, so there's no need
to deal with this bit. Since there's currently no public document for the
switch on the MT7988 SoC, I assume this is also the case for this switch.
Remove the ETHSYS_CLKCFG0 register which doesn't exist on the said
documents, and conflicts with the MT753X_RGAC2 register.
The mt753x_bpdu_port_fw enum is universally used for trapping frames,
therefore rename it and the values in it to mt753x_port_fw.
Tested-by: Arınç ÜNAL <[email protected]>
Signed-off-by: Arınç ÜNAL <[email protected]>
---
drivers/net/dsa/mt7530.c | 12 ++++++++++--
drivers/net/dsa/mt7530.h | 23 ++++++++++++-----------
2 files changed, 22 insertions(+), 13 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 2fb4b0bc6335..8f5a8803cb33 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2225,7 +2225,11 @@ mt7530_setup(struct dsa_switch *ds)
/* Trap BPDUs to the CPU port */
mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
- MT753X_BPDU_CPU_ONLY);
+ MT753X_PORT_FW_CPU_ONLY);
+
+ /* Trap LLDP frames with :0E MAC DA to the CPU port */
+ mt7530_rmw(priv, MT753X_RGAC2, MT753X_R0E_PORT_FW_MASK,
+ MT753X_R0E_PORT_FW(MT753X_PORT_FW_CPU_ONLY));
/* Enable and reset MIB counters */
mt7530_mib_reset(ds);
@@ -2325,7 +2329,11 @@ mt7531_setup_common(struct dsa_switch *ds)
/* Trap BPDUs to the CPU port(s) */
mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
- MT753X_BPDU_CPU_ONLY);
+ MT753X_PORT_FW_CPU_ONLY);
+
+ /* Trap LLDP frames with :0E MAC DA to the CPU port(s) */
+ mt7530_rmw(priv, MT753X_RGAC2, MT753X_R0E_PORT_FW_MASK,
+ MT753X_R0E_PORT_FW(MT753X_PORT_FW_CPU_ONLY));
/* Enable and reset MIB counters */
mt7530_mib_reset(ds);
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 52e5d71a04d3..2664057b3cd2 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -25,10 +25,6 @@ enum mt753x_id {
#define TRGMII_BASE(x) (0x10000 + (x))
-/* Registers to ethsys access */
-#define ETHSYS_CLKCFG0 0x2c
-#define ETHSYS_TRGMII_CLK_SEL362_5 BIT(11)
-
#define SYSC_REG_RSTCTRL 0x34
#define RESET_MCM BIT(2)
@@ -63,16 +59,21 @@ enum mt753x_id {
#define MT753X_MIRROR_MASK(id) ((((id) == ID_MT7531) || ((id) == ID_MT7988)) ? \
MT7531_MIRROR_MASK : MT7530_MIRROR_MASK)
-/* Registers for BPDU and PAE frame control*/
+/* Register for BPDU and PAE frame control */
#define MT753X_BPC 0x24
#define MT753X_BPDU_PORT_FW_MASK GENMASK(2, 0)
-enum mt753x_bpdu_port_fw {
- MT753X_BPDU_FOLLOW_MFC,
- MT753X_BPDU_CPU_EXCLUDE = 4,
- MT753X_BPDU_CPU_INCLUDE = 5,
- MT753X_BPDU_CPU_ONLY = 6,
- MT753X_BPDU_DROP = 7,
+/* Register for :03 and :0E MAC DA frame control */
+#define MT753X_RGAC2 0x2c
+#define MT753X_R0E_PORT_FW(x) (((x) & 0x7) << 16)
+#define MT753X_R0E_PORT_FW_MASK MT753X_R0E_PORT_FW(~0)
+
+enum mt753x_port_fw {
+ MT753X_PORT_FW_FOLLOW_MFC,
+ MT753X_PORT_FW_CPU_EXCLUDE = 4,
+ MT753X_PORT_FW_CPU_INCLUDE = 5,
+ MT753X_PORT_FW_CPU_ONLY = 6,
+ MT753X_PORT_FW_DROP = 7,
};
/* Registers for address table access */
--
2.39.2
On Mon, May 22, 2023 at 03:15:02PM +0300, [email protected] wrote:
> Hello!
>
> This patch series simplifies the code, improves the logic of the switch
> hardware support, traps LLDP frames and BPDUs for MT7530, MT7531, and
> MT7988 SoC switches, and introduces the preferring local CPU port
> operation.
Hi Arınç
https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html
says:
Avoid sending series longer than 15 patches. Larger series takes
longer to review as reviewers will defer looking at it until they
find a large chunk of time. A small series can be reviewed in a
short time, so Maintainers just do it. As a result, a sequence of
smaller series gets merged quicker and with better review coverage.
Given you description above, it sounds like this could easily be split
into smaller patch series.
Andrew
On 22.05.2023 15:25, Andrew Lunn wrote:
> On Mon, May 22, 2023 at 03:15:02PM +0300, [email protected] wrote:
>> Hello!
>>
>> This patch series simplifies the code, improves the logic of the switch
>> hardware support, traps LLDP frames and BPDUs for MT7530, MT7531, and
>> MT7988 SoC switches, and introduces the preferring local CPU port
>> operation.
>
> Hi Arınç
>
> https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html
>
> says:
>
> Avoid sending series longer than 15 patches. Larger series takes
> longer to review as reviewers will defer looking at it until they
> find a large chunk of time. A small series can be reviewed in a
> short time, so Maintainers just do it. As a result, a sequence of
> smaller series gets merged quicker and with better review coverage.
>
> Given you description above, it sounds like this could easily be split
> into smaller patch series.
Later patches require the prior ones to apply properly. I can submit the
first 15 patches, then the remaining once the first submission is applied.
Would that suit you?
Arınç
The 05/22/2023 15:15, [email protected] wrote:
Hi,
>
> Hello!
>
> This patch series simplifies the code, improves the logic of the switch
> hardware support, traps LLDP frames and BPDUs for MT7530, MT7531, and
> MT7988 SoC switches, and introduces the preferring local CPU port
> operation.
>
> There's also a patch for fixing the port capabilities of the switch on the
> MT7988 SoC.
>
I have noticed that in many patches of the series you have:
Tested-by: Arınç ÜNAL <[email protected]>
Where you also have:
Signed-off-by: Arınç ÜNAL <[email protected]>
I think you can drop Tested-by as the SoB will imply that. I think you
got a similar comment some time ago to a different patch series.
--
/Horatiu
On 22/05/2023 17:09, Horatiu Vultur wrote:
> The 05/22/2023 15:15, [email protected] wrote:
>
> Hi,
>
>>
>> Hello!
>>
>> This patch series simplifies the code, improves the logic of the switch
>> hardware support, traps LLDP frames and BPDUs for MT7530, MT7531, and
>> MT7988 SoC switches, and introduces the preferring local CPU port
>> operation.
>>
>> There's also a patch for fixing the port capabilities of the switch on the
>> MT7988 SoC.
>>
>
> I have noticed that in many patches of the series you have:
> Tested-by: Arınç ÜNAL <[email protected]>
>
> Where you also have:
> Signed-off-by: Arınç ÜNAL <[email protected]>
>
> I think you can drop Tested-by as the SoB will imply that. I think you
> got a similar comment some time ago to a different patch series.
Yes, that was for the net, therefore the stable tree. The very first
rule for the patches going to the stable tree is that the patch must be
tested.
I don't see a clear indication of the patches submitted to net-next
being tested on the relevant hardware by the author. Therefore, I
explicitly put my tag to state it. Let me know if you don't agree with this.
Arınç
On Mon, May 22, 2023 at 04:37:28PM +0300, Arınç ÜNAL wrote:
> Later patches require the prior ones to apply properly. I can submit the
> first 15 patches, then the remaining once the first submission is applied.
> Would that suit you?
Please wait for some feedback on (a subset of) what you've submitted thus far.
Thanks.
On Mon, May 22, 2023 at 04:09:17PM +0200, Horatiu Vultur wrote:
> The 05/22/2023 15:15, [email protected] wrote:
>
> Hi,
>
> >
> > Hello!
> >
> > This patch series simplifies the code, improves the logic of the switch
> > hardware support, traps LLDP frames and BPDUs for MT7530, MT7531, and
> > MT7988 SoC switches, and introduces the preferring local CPU port
> > operation.
> >
> > There's also a patch for fixing the port capabilities of the switch on the
> > MT7988 SoC.
> >
>
> I have noticed that in many patches of the series you have:
> Tested-by: Arınç ÜNAL <[email protected]>
>
> Where you also have:
> Signed-off-by: Arınç ÜNAL <[email protected]>
>
> I think you can drop Tested-by as the SoB will imply that. I think you
> got a similar comment some time ago to a different patch series.
Signed-off-by in no way implies a tested-by. Signed-off-by has a very
distinct definition that is in submitting-patches.rst.
Clearly, if one is working on infrastructure where there are numerous
drivers involved, one probably doesn't have all the hardware, and one
may have to send patches that have only been build tested, but never
tested against real hardware.
While we may attempt to elicit testing, most of the time this seems
to be a waste of time and effort - or at least that's my experience.
Even if you Cc people who have recently been active with hardware,
that is no guarantee that there will be any reaction.
That has got to the point now where I just don't bother trying to
elicit help from others to test driver changes. If people want to
test, they need to do so when they see a patch on the mailing list,
preferably before it gets applied. If not, and if it breaks something,
then we'll have to generate a patch to fix the breakage.
So no, please stop thinking that SoB implies that the patch has been
tested.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Mon, 22 May 2023 19:13:31 +0100 Russell King (Oracle) wrote:
> > I have noticed that in many patches of the series you have:
> > Tested-by: Arınç ÜNAL <[email protected]>
> >
> > Where you also have:
> > Signed-off-by: Arınç ÜNAL <[email protected]>
> >
> > I think you can drop Tested-by as the SoB will imply that. I think you
> > got a similar comment some time ago to a different patch series.
>
> Signed-off-by in no way implies a tested-by. Signed-off-by has a very
> distinct definition that is in submitting-patches.rst.
>
> Clearly, if one is working on infrastructure where there are numerous
> drivers involved, one probably doesn't have all the hardware, and one
> may have to send patches that have only been build tested, but never
> tested against real hardware.
>
> While we may attempt to elicit testing, most of the time this seems
> to be a waste of time and effort - or at least that's my experience.
> Even if you Cc people who have recently been active with hardware,
> that is no guarantee that there will be any reaction.
>
> That has got to the point now where I just don't bother trying to
> elicit help from others to test driver changes. If people want to
> test, they need to do so when they see a patch on the mailing list,
> preferably before it gets applied. If not, and if it breaks something,
> then we'll have to generate a patch to fix the breakage.
>
> So no, please stop thinking that SoB implies that the patch has been
> tested.
Dunno, I had the same reaction as Horatiu. Adding "Compile tested only"
in the commit messages of patches which author wasn't able to test seems
more natural than assuming that nothing is tested by default.
It's not a hard requirement, e.g. seems fairly common sense that cross-
-driver work comes with limited testing coverage. But for someone
working on a single driver, assuming not tested by default, again,
to me - feels odd.
On Mon, May 22, 2023 at 03:15:04PM +0300, [email protected] wrote:
> From: Arınç ÜNAL <[email protected]>
>
> 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.
>
> Signed-off-by: Arınç ÜNAL <[email protected]>
> Acked-by: Daniel Golle <[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 845f5dd16d83..415d8ea07472 100644
> --- a/drivers/net/dsa/mt7530.h
> +++ b/drivers/net/dsa/mt7530.h
> @@ -675,7 +675,7 @@ struct mt7530_port {
>
> /* Port 5 interface select definitions */
> enum p5_interface_select {
> - P5_DISABLED = 0,
> + P5_DISABLED,
Is this change important in this context? Maybe add something to the
commit message about it?
On Mon, May 22, 2023 at 03:15:06PM +0300, [email protected] wrote:
> From: Arınç ÜNAL <[email protected]>
>
> 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 both the MT7530 and MT7531
> switches but there's no PHY muxing on MT7531.
>
> 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]>
Andrew
On Mon, May 22, 2023 at 03:15:14PM +0300, [email protected] wrote:
> From: Arınç ÜNAL <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Arınç ÜNAL <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
Andrew
On Mon, May 22, 2023 at 03:15:03PM +0300, [email protected] wrote:
> From: Arınç ÜNAL <[email protected]>
>
> Add the missing p5_interface field to the mt7530_priv description. Sort out
> the description in the process.
>
> Signed-off-by: Arınç ÜNAL <[email protected]>
> Acked-by: Daniel Golle <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
Andrew
On 24.05.2023 02:31, Andrew Lunn wrote:
> On Mon, May 22, 2023 at 03:15:04PM +0300, [email protected] wrote:
>> From: Arınç ÜNAL <[email protected]>
>>
>> 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.
>>
>> Signed-off-by: Arınç ÜNAL <[email protected]>
>> Acked-by: Daniel Golle <[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 845f5dd16d83..415d8ea07472 100644
>> --- a/drivers/net/dsa/mt7530.h
>> +++ b/drivers/net/dsa/mt7530.h
>> @@ -675,7 +675,7 @@ struct mt7530_port {
>>
>> /* Port 5 interface select definitions */
>> enum p5_interface_select {
>> - P5_DISABLED = 0,
>> + P5_DISABLED,
>
> Is this change important in this context? Maybe add something to the
> commit message about it?
Not important, the first element in the enum is given value 0 so this is
merely a style change. I will mention it on the patch log, thanks.
Arınç
On Mon, May 22, 2023 at 03:15:32PM +0300, [email protected] wrote:
> From: Arınç ÜNAL <[email protected]>
>
> Add me as a maintainer of the MediaTek MT7530 DSA subdriver.
>
> List maintainers in alphabetical order by first name.
>
> Signed-off-by: Arınç ÜNAL <[email protected]>
> ---
Patches on the MAINTAINERS file go to the "net" tree, because people
working on stable branches of the kernel (which you also maintain) need
to "see" you as well, to be able to ask you questions.
On Mon, May 22, 2023 at 03:15:06PM +0300, [email protected] wrote:
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 024b853f9558..b28d66a7c0b2 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2499,7 +2499,9 @@ static void mt7530_mac_port_get_caps(struct dsa_switch *ds, int port,
> config->supported_interfaces);
> break;
>
> - case 5: /* 2nd cpu port with phy of port 0 or 4 / external phy */
> + case 5: /* Port 5 which can be used as a CPU port supports rgmii with
> + * delays, mii, and gmii.
> + */
Please put the comments on dedicated lines:
/* Port X can be used as ... */
case X:
On Mon, May 22, 2023 at 03:15:05PM +0300, [email protected] wrote:
> From: Arınç ÜNAL <[email protected]>
>
> Introduce the p5_sgmii field to store the information for whether port 5
> has got SGMII or not.
>
> 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.
>
> 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]>
> ---
Pretty busy patch, and after reading it, I'm not sure:
- why? (this seems to be absent from the commit message)
- how are MT7531AE and MT7531BE supported any more properly after this
change, as the commit title claims
- what is the overall effect, other than just refactoring. If that's
all, what's written on the tin needs to be a better representation of
what's inside.
Pure refactoring is not a bad goal in itself, as long as we're on the
same page that there is a justification which makes the new code better
than the old one.
On Mon, May 22, 2023 at 03:15:10PM +0300, [email protected] wrote:
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 710c6622d648..d837aa20968c 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2728,25 +2722,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;
If you don't replace this bit with anything, who will set priv->p5_configured
for mt7530?
> 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;
Similar question for port 6.
On Mon, May 22, 2023 at 03:15:08PM +0300, [email protected] wrote:
> From: Arınç ÜNAL <[email protected]>
>
> 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.
>
> mt7530_setup_port5() from mt753x_phylink_mac_config() won't run when port 5
> is disabled or used for PHY muxing as port 5 won't be defined on the
> devicetree.
>
> 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. For the cases of PHY muxing or
> the port being disabled, call mt7530_setup_port5() from mt7530_setup().
So TL;DR: mt7530_setup() -> mt7530_setup_port5() short-circuits
mt753x_phylink_mac_config() -> ... -> mt7530_setup_port5() through the
stateful variable priv->p5_interface, such that port 5 is effectively
never configured by phylink, but statically at probe time. The main goal of
the patch is to undo the short-circuit, and let phylink configure port 5.
It is worth stating that we know phylink will always be present, because
mt7530 isn't in the dsa_switches_apply_workarounds[] array. Otherwise
this strategy would have been problematic with some device trees.
> Do not set priv->p5_interface on mt7530_setup_port5(). There won't be a
> case where mt753x_phylink_mac_config() runs after mt7530_setup_port5()
> anymore.
The bulk of the change is difficult enough to follow. I believe this
part could be done through a separate patch, and the rest would still
work.
On Mon, May 22, 2023 at 03:15:12PM +0300, [email protected] wrote:
> From: Arınç ÜNAL <[email protected]>
>
> mt7530_pad_clk_setup() is called if port 6 is enabled. It used to do more
> things than setting up port 6. That part was moved to more appropriate
> locations, mt7530_setup() and mt7530_pll_setup().
>
> Now that all it does is set up port 6, rename it to mt7530_setup_port6(),
> and move it to a more appropriate location, under mt7530_mac_config().
>
> Leave an empty mt7530_pad_clk_setup() to satisfy the pad_setup function
> pointer.
>
> This is the code path for setting up the ports before:
>
> mt753x_phylink_mac_config()
> -> mt753x_mac_config()
> -> mt7530_mac_config()
> -> mt7530_setup_port5()
> -> mt753x_pad_setup()
> -> mt7530_pad_clk_setup()
>
> This is after:
>
> mt753x_phylink_mac_config()
> -> mt753x_mac_config()
> -> mt7530_mac_config()
> -> mt7530_setup_port5()
> -> mt7530_setup_port6()
>
> Tested-by: Arınç ÜNAL <[email protected]>
> Signed-off-by: Arınç ÜNAL <[email protected]>
> ---
Reviewed-by: Vladimir Oltean <[email protected]>
On Mon, May 22, 2023 at 03:15:09PM +0300, [email protected] wrote:
> From: Arınç ÜNAL <[email protected]>
>
> 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.
>
> Tested-by: Arınç ÜNAL <[email protected]>
> Signed-off-by: Arınç ÜNAL <[email protected]>
> ---
Reviewed-by: Vladimir Oltean <[email protected]>
On Mon, May 22, 2023 at 03:15:13PM +0300, [email protected] wrote:
> From: Arınç ÜNAL <[email protected]>
>
> The pad_setup function pointer was introduced with 88bdef8be9f6 ("net: dsa:
> mt7530: Extend device data ready for adding a new hardware"). It was being
> used to set up the core clock and port 6 of the MT7530 switch, and pll of
> the MT7531 switch.
>
> All of these were moved to more appropriate locations, and it was never
> used for the switch on the MT7988 SoC. Therefore, this function pointer
> hasn't got a use anymore. Remove it.
>
> Tested-by: Arınç ÜNAL <[email protected]>
> Signed-off-by: Arınç ÜNAL <[email protected]>
> Acked-by: Daniel Golle <[email protected]>
> ---
Reviewed-by: Vladimir Oltean <[email protected]>
On Mon, May 22, 2023 at 03:15:14PM +0300, [email protected] wrote:
> From: Arınç ÜNAL <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Arınç ÜNAL <[email protected]>
> ---
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
>
On 24.05.2023 17:48, Vladimir Oltean wrote:
> On Mon, May 22, 2023 at 03:15:05PM +0300, [email protected] wrote:
>> From: Arınç ÜNAL <[email protected]>
>>
>> Introduce the p5_sgmii field to store the information for whether port 5
>> has got SGMII or not.
>>
>> 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.
>>
>> 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]>
>> ---
>
> Pretty busy patch, and after reading it, I'm not sure:
>
> - why? (this seems to be absent from the commit message)
> - how are MT7531AE and MT7531BE supported any more properly after this
> change, as the commit title claims
> - what is the overall effect, other than just refactoring. If that's
> all, what's written on the tin needs to be a better representation of
> what's inside.
>
> Pure refactoring is not a bad goal in itself, as long as we're on the
> same page that there is a justification which makes the new code better
> than the old one.
With this patch, 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. I will mention this on the patch log.
This is already being done on the U-Boot driver.
https://github.com/u-boot/u-boot/blob/master/drivers/net/mtk_eth.c#L859
I think "improve MT7531AE and MT7531BE support" will be a better subject.
Arınç
On 24.05.2023 20:35, Vladimir Oltean wrote:
> On Mon, May 22, 2023 at 03:15:08PM +0300, [email protected] wrote:
>> From: Arınç ÜNAL <[email protected]>
>>
>> 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.
>>
>> mt7530_setup_port5() from mt753x_phylink_mac_config() won't run when port 5
>> is disabled or used for PHY muxing as port 5 won't be defined on the
>> devicetree.
>>
>> 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. For the cases of PHY muxing or
>> the port being disabled, call mt7530_setup_port5() from mt7530_setup().
>
> So TL;DR: mt7530_setup() -> mt7530_setup_port5() short-circuits
> mt753x_phylink_mac_config() -> ... -> mt7530_setup_port5() through the
> stateful variable priv->p5_interface, such that port 5 is effectively
> never configured by phylink, but statically at probe time. The main goal of
> the patch is to undo the short-circuit, and let phylink configure port 5.
True.
>
> It is worth stating that we know phylink will always be present, because
> mt7530 isn't in the dsa_switches_apply_workarounds[] array. Otherwise
> this strategy would have been problematic with some device trees.
I will add this to the patch log:
This driver isn't in the dsa_switches_apply_workarounds[] array so
phylink will always be present.
>
>> Do not set priv->p5_interface on mt7530_setup_port5(). There won't be a
>> case where mt753x_phylink_mac_config() runs after mt7530_setup_port5()
>> anymore.
>
> The bulk of the change is difficult enough to follow. I believe this
> part could be done through a separate patch, and the rest would still
> work.
Will do.
Arınç
On 24.05.2023 21:15, Vladimir Oltean wrote:
> On Mon, May 22, 2023 at 03:15:14PM +0300, [email protected] wrote:
>> From: Arınç ÜNAL <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Arınç ÜNAL <[email protected]>
>> ---
>
> 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ç
On 24.05.2023 20:51, Vladimir Oltean wrote:
> On Mon, May 22, 2023 at 03:15:10PM +0300, [email protected] wrote:
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index 710c6622d648..d837aa20968c 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -2728,25 +2722,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;
>
> If you don't replace this bit with anything, who will set priv->p5_configured
> for mt7530?
I intend priv->p5_configured and priv->p6_configured to be only used for
MT7531 as I have stated on the mt7530_priv description.
Arınç
On Thu, May 25, 2023 at 09:49:58AM +0300, Arınç ÜNAL wrote:
> On 24.05.2023 20:51, Vladimir Oltean wrote:
> > On Mon, May 22, 2023 at 03:15:10PM +0300, [email protected] wrote:
> > > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> > > index 710c6622d648..d837aa20968c 100644
> > > --- a/drivers/net/dsa/mt7530.c
> > > +++ b/drivers/net/dsa/mt7530.c
> > > @@ -2728,25 +2722,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;
> >
> > If you don't replace this bit with anything, who will set priv->p5_configured
> > for mt7530?
>
> I intend priv->p5_configured and priv->p6_configured to be only used for
> MT7531 as I have stated on the mt7530_priv description.
Ok, but given the premise of this patch set, that phylink is always available,
does it make sense for mt7531_cpu_port_config() and mt7988_cpu_port_config()
to manually call phylink methods?
On Mon, May 22, 2023 at 03:15:17PM +0300, [email protected] wrote:
> From: Arınç ÜNAL <[email protected]>
>
> This code sets the Read Data (RD) TAP value to 16 for all TRGMII control
> registers.
>
> The for loop iterates over all the TRGMII control registers, and
> mt7530_rmw() function is used to perform a read-modify-write operation on
> each register's RD_TAP field to set its value to 16.
>
> This operation is used to tune the timing of the read data signal in
> TRGMII to match the TX signal of the link partner.
>
> Run this if trgmii is being used. Since this code doesn't lower the
> driving, there's no apparent benefit to run this if trgmii is not being
> used.
>
> Add a comment to explain the code.
>
> Thanks to 趙皎宏 (Landen Chao) for pointing out what the code does.
>
> Tested-by: Arınç ÜNAL <[email protected]>
> Signed-off-by: Arınç ÜNAL <[email protected]>
> ---
Reviewed-by: Vladimir Oltean <[email protected]>
On Mon, May 22, 2023 at 03:15:18PM +0300, [email protected] wrote:
> From: Arınç ÜNAL <[email protected]>
>
> Move lowering Tx driving of rgmii on port 5 to right before lowering of Tx
> driving of trgmii on port 6 on mt7530_setup().
>
> This way, the switch should consume less power regardless of port 5 being
> used.
>
> Tested-by: Arınç ÜNAL <[email protected]>
> Signed-off-by: Arınç ÜNAL <[email protected]>
> ---
This patch assumes that the MAC has its TXC ticking even when PMCR_TX_EN
is unset. Why would it do that?
> drivers/net/dsa/mt7530.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index f2c1aa9cf7f7..514e82299537 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -933,10 +933,6 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
> /* P5 RGMII TX Clock Control: delay x */
> mt7530_write(priv, MT7530_P5RGMIITXCR,
> CSR_RGMII_TXC_CFG(0x10 + tx_delay));
> -
> - /* reduce P5 RGMII Tx driving, 8mA */
> - mt7530_write(priv, MT7530_IO_DRV_CR,
> - P5_IO_CLK_DRV(1) | P5_IO_DATA_DRV(1));
> }
>
> mt7530_write(priv, MT7530_MHWTRAP, val);
> @@ -2209,6 +2205,10 @@ mt7530_setup(struct dsa_switch *ds)
>
> mt7530_pll_setup(priv);
>
> + /* Lower P5 RGMII Tx driving, 8mA */
> + mt7530_write(priv, MT7530_IO_DRV_CR,
> + P5_IO_CLK_DRV(1) | P5_IO_DATA_DRV(1));
If you move code then preserve its alignment to the open parenthesis.
> +
> /* Lower Tx driving for TRGMII path */
> for (i = 0; i < NUM_TRGMII_CTRL; i++)
> mt7530_write(priv, MT7530_TRGMII_TD_ODT(i),
> --
> 2.39.2
>
On Mon, May 22, 2023 at 03:15:16PM +0300, [email protected] wrote:
> From: Arınç ÜNAL <[email protected]>
>
> This code is from before this driver was converted to phylink API. Phylink
> deals with the unsupported interface cases before mt7530_setup_port6() is
> run. Therefore, the default case would never run. However, it must be
> defined nonetheless to handle all the remaining enumeration values, the
> phy-modes.
>
> Switch to if/else statements which simplifies the code.
>
> Change mt7530_setup_port6() to void now that there're no error cases left.
>
> Tested-by: Arınç ÜNAL <[email protected]>
> Signed-off-by: Arınç ÜNAL <[email protected]>
> ---
Reviewed-by: Vladimir Oltean <[email protected]>
On Mon, May 22, 2023 at 03:15:20PM +0300, [email protected] wrote:
> From: Arınç ÜNAL <[email protected]>
>
> For the switch on the MT7988 SoC, the code in mac_port_config for MT7988 is
> not needed as the interface of the CPU port is already handled on
> mt7988_mac_port_get_caps().
>
> Make .mac_port_config optional. Before calling
> priv->info->mac_port_config(), if there's no mac_port_config member in the
> priv->info table, exit mt753x_mac_config() successfully.
>
> Remove mac_port_config from the sanity check as the sanity check requires a
> pointer to a mac_port_config function to be non-NULL. This will fail for
> MT7988 as mac_port_config won't be a member of its info table.
>
> Co-developed-by: Daniel Golle <[email protected]>
> Signed-off-by: Daniel Golle <[email protected]>
> Signed-off-by: Arınç ÜNAL <[email protected]>
> ---
Reviewed-by: Vladimir Oltean <[email protected]>
On Mon, May 22, 2023 at 03:15:21PM +0300, [email protected] wrote:
> From: Arınç ÜNAL <[email protected]>
>
> Setting this register related to interrupts is only needed for the MT7530
> switch. Make an exclusive check to ensure this.
>
> Signed-off-by: Arınç ÜNAL <[email protected]>
> Acked-by: Daniel Golle <[email protected]>
> Tested-by: Daniel Golle <[email protected]>
> ---
Why does it matter? What prompted you to make this change? I guess it's
not needed for MT7988? Or the register is not present? Or?...
> drivers/net/dsa/mt7530.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 99f5da8b27be..0c261ef87bee 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2029,7 +2029,7 @@ mt7530_setup_irq(struct mt7530_priv *priv)
> }
>
> /* This register must be set for MT7530 to properly fire interrupts */
> - if (priv->id != ID_MT7531)
> + if (priv->id == ID_MT7530 || priv->id == ID_MT7621)
> mt7530_set(priv, MT7530_TOP_SIG_CTRL, TOP_SIG_CTRL_NORMAL);
>
> ret = request_threaded_irq(priv->irq, NULL, mt7530_irq_thread_fn,
> --
> 2.39.2
>
On Mon, May 22, 2023 at 03:15:26PM +0300, [email protected] wrote:
> /* Disable flooding on all ports */
> - mt7530_clear(priv, MT7530_MFC, BC_FFP_MASK | UNM_FFP_MASK |
> - UNU_FFP_MASK);
> + mt7530_clear(priv, MT753X_MFC, MT753X_BC_FFP_MASK | MT753X_UNM_FFP_MASK
> + | MT753X_UNU_FFP_MASK);
The preferred coding style is not to start new lines with operators.
> +/* Register for CPU forward control */
> #define MT7531_CFC 0x4
> #define MT7531_MIRROR_EN BIT(19)
> -#define MT7531_MIRROR_MASK (MIRROR_MASK << 16)
> -#define MT7531_MIRROR_PORT_GET(x) (((x) >> 16) & MIRROR_MASK)
> -#define MT7531_MIRROR_PORT_SET(x) (((x) & MIRROR_MASK) << 16)
> +#define MT7531_MIRROR_MASK (0x7 << 16)
minor nitpick: if you express this as GENMASK(18, 16), it will be a bit
easier to cross-check with the datasheet, since both 18 and 16 are more
representative than 0x7.
> +#define MT7531_MIRROR_PORT_GET(x) (((x) >> 16) & 0x7)
also here: (((x) & GENMASK(18, 16)) >> 16)
> +#define MT7531_MIRROR_PORT_SET(x) (((x) & 0x7) << 16)
and here: (((x) << 16) & GENMASK(18, 16))
On Fri, May 26, 2023 at 06:42:58PM +0300, Vladimir Oltean wrote:
> On Mon, May 22, 2023 at 03:15:26PM +0300, [email protected] wrote:
> > /* Disable flooding on all ports */
> > - mt7530_clear(priv, MT7530_MFC, BC_FFP_MASK | UNM_FFP_MASK |
> > - UNU_FFP_MASK);
> > + mt7530_clear(priv, MT753X_MFC, MT753X_BC_FFP_MASK | MT753X_UNM_FFP_MASK
> > + | MT753X_UNU_FFP_MASK);
>
> The preferred coding style is not to start new lines with operators.
>
> > +/* Register for CPU forward control */
> > #define MT7531_CFC 0x4
> > #define MT7531_MIRROR_EN BIT(19)
> > -#define MT7531_MIRROR_MASK (MIRROR_MASK << 16)
> > -#define MT7531_MIRROR_PORT_GET(x) (((x) >> 16) & MIRROR_MASK)
> > -#define MT7531_MIRROR_PORT_SET(x) (((x) & MIRROR_MASK) << 16)
> > +#define MT7531_MIRROR_MASK (0x7 << 16)
>
> minor nitpick: if you express this as GENMASK(18, 16), it will be a bit
> easier to cross-check with the datasheet, since both 18 and 16 are more
> representative than 0x7.
>
> > +#define MT7531_MIRROR_PORT_GET(x) (((x) >> 16) & 0x7)
>
> also here: (((x) & GENMASK(18, 16)) >> 16)
Even better are:
#define MT7531_MIRROR_PORT_GET(x) FIELD_GET(MT7531_MIRROR_MASK, x)
>
> > +#define MT7531_MIRROR_PORT_SET(x) (((x) & 0x7) << 16)
>
> and here: (((x) << 16) & GENMASK(18, 16))
#define MT7531_MIRROR_PORT_SET(x) FIELD_PREP(MT7531_MIRROR_MASK, x)
No need to add parens around "x" in either of these uses as we're not
doing anything with x other than passing it into another macro.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Mon, May 22, 2023 at 03:15:27PM +0300, [email protected] wrote:
> From: Arınç ÜNAL <[email protected]>
>
> Every bit of the CPU port bitmap for MT7531 and the switch on the MT7988
> SoC represents a CPU port to trap frames to. Currently only the bit that
> corresponds to the first found CPU port is set on the bitmap. Introduce the
> MT7531_CPU_PMAP macro to individually set the bits of the CPU port bitmap.
> Set the CPU port bitmap for MT7531 and the switch on the MT7988 SoC on
> mt753x_cpu_port_enable() which runs on a loop for each CPU port. Add
> comments to explain this.
>
> According to the document MT7531 Reference Manual for Development Board
> v1.0, the MT7531_CPU_PMAP bits are unset after reset so no need to clear it
> beforehand. Since there's currently no public document for the switch on
> the MT7988 SoC, I assume this is also the case for this switch.
>
> Tested-by: Arınç ÜNAL <[email protected]>
> Signed-off-by: Arınç ÜNAL <[email protected]>
> ---
Is this supposed to be a bug fix? (incompatibility with past or future
device trees also counts as bugs) If so, it needs a Fixes: tag and to be
targeted towards the "net" tree. Also, the impact of the current behavior
and of the change need to be explained better.
> drivers/net/dsa/mt7530.c | 15 ++++++++-------
> drivers/net/dsa/mt7530.h | 3 ++-
> 2 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 58d8738d94d3..0b513e3628fe 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -963,6 +963,13 @@ mt753x_cpu_port_enable(struct dsa_switch *ds, int port)
> mt7530_rmw(priv, MT753X_MFC, MT7530_CPU_MASK, MT7530_CPU_EN |
> MT7530_CPU_PORT(port));
>
> + /* Add the CPU port to the CPU port bitmap for MT7531 and the switch on
> + * the MT7988 SoC. Any frames set for trapping to CPU port will be
> + * trapped to the CPU port the user port is affine to.
> + */
> + if (priv->id == ID_MT7531 || priv->id == ID_MT7988)
> + mt7530_set(priv, MT7531_CFC, MT7531_CPU_PMAP(BIT(port)));
> +
Stylistically, the existence of an indirect call to priv->info->cpu_port_config()
per switch family is a bit dissonant with an explicit check for device id later
in the same function.
> /* CPU port gets connected to all user ports of
> * the switch.
> */
> @@ -2315,15 +2322,9 @@ static int
> mt7531_setup_common(struct dsa_switch *ds)
> {
> struct mt7530_priv *priv = ds->priv;
> - struct dsa_port *cpu_dp;
> int ret, i;
>
> - /* BPDU to CPU port */
> - dsa_switch_for_each_cpu_port(cpu_dp, ds) {
> - mt7530_rmw(priv, MT7531_CFC, MT7531_CPU_PMAP_MASK,
> - BIT(cpu_dp->index));
> - break;
> - }
> + /* Trap BPDUs to the CPU port(s) */
> mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
> MT753X_BPDU_CPU_ONLY);
>
> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
> index 5ebb942b07ef..fd2a2f726b8a 100644
> --- a/drivers/net/dsa/mt7530.h
> +++ b/drivers/net/dsa/mt7530.h
> @@ -53,7 +53,8 @@ enum mt753x_id {
> #define MT7531_MIRROR_MASK (0x7 << 16)
> #define MT7531_MIRROR_PORT_GET(x) (((x) >> 16) & 0x7)
> #define MT7531_MIRROR_PORT_SET(x) (((x) & 0x7) << 16)
> -#define MT7531_CPU_PMAP_MASK GENMASK(7, 0)
> +#define MT7531_CPU_PMAP(x) ((x) & 0xff)
You can leave this as ((x) & GENMASK(7, 0))
> +#define MT7531_CPU_PMAP_MASK MT7531_CPU_PMAP(~0)
There's no other user of MT7531_CPU_PMAP_MASK, you can remove this.
>
> #define MT753X_MIRROR_REG(id) ((((id) == ID_MT7531) || ((id) == ID_MT7988)) ? \
> MT7531_CFC : MT753X_MFC)
> --
> 2.39.2
>
On Mon, May 22, 2023 at 03:15:28PM +0300, [email protected] wrote:
> From: Arınç ÜNAL <[email protected]>
>
> The MT7530_CPU_PORT bits represent the CPU port to trap frames to for the
> MT7530 switch. There are two issues with the current way of setting these
> bits. ID_MT7530 which is for the standalone MT7530 switch is not included.
It's best to say in the commit title what the change does, rather than
the equivalent of "here, this way is proper!". Commit titles should be
uniquely identifiable, and "properly set MT7530_CPU_PORT" doesn't say a
lot about how proper it is. It's enough to imagine a future person
finding something else that's perfectible and writing another "net: dsa:
mt7530: properly set MT7530_CPU_PORT" commit. Try to be less definitive
and at the same time more specific.
If there are 2 issues, there should be 2 changes with individual titles
which each describes what was wrong and how that was changed.
> When multiple CPU ports are being used, the trapped frames won't be
> received when the DSA conduit interface, which the frames are supposed to
> be trapped to, is down because it's not affine to any user port. This
> requires the DSA conduit interface to be manually set up for the trapped
> frames to be received.
>
> Address these issues by implementing ds->ops->master_state_change() on this
> subdriver and setting the MT7530_CPU_PORT bits there. Introduce the
> active_cpu_ports field to store the information of active CPU ports.
> Correct the macros, MT7530_CPU_PORT is bits 4 through 6 of the register.
>
> Any frames set for trapping to CPU port will be trapped to the numerically
> smallest CPU port which is affine to the DSA conduit interface that is set
> up. To make the understatement obvious, the frames won't necessarily be
> trapped to the CPU port the user port, which these frames are received
> from, is affine to. This operation is only there to make sure the trapped
> frames always reach the CPU.
>
> Tested-by: Arınç ÜNAL <[email protected]>
> Co-developed-by: Vladimir Oltean <[email protected]>
> Signed-off-by: Vladimir Oltean <[email protected]>
A single Suggested-by: is fine. As a rule of thumb, I would use Co-developed-by
when I'm working with a patch formally pre-formatted or committed by somebody else,
that I've changed in a significant manner. Since all I did was to comment with
a suggestion of how to handle this, and with a code snippet written in the email
client to a patch of yours, I don't believe that's necessary here.
> Signed-off-by: Arınç ÜNAL <[email protected]>
> ---
On Mon, May 22, 2023 at 03:15:29PM +0300, [email protected] wrote:
> From: Arınç ÜNAL <[email protected]>
>
> The MT753X switches are capable of trapping certain frames. Introduce
> trapping BPDUs to the CPU port for the MT7530 switch.
>
> BPDUs will be trapped to the numerically smallest CPU port which is affine
> to the DSA conduit interface that is set up. The BPDUs won't necessarily be
> trapped to the CPU port the user port, which these BPDUs are received from,
> is affine to.
>
> Tested-by: Arınç ÜNAL <[email protected]>
> Signed-off-by: Arınç ÜNAL <[email protected]>
> ---
> drivers/net/dsa/mt7530.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index cd16911fcb01..2fb4b0bc6335 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2223,6 +2223,10 @@ mt7530_setup(struct dsa_switch *ds)
> val |= MHWTRAP_MANUAL;
> mt7530_write(priv, MT7530_MHWTRAP, val);
>
> + /* Trap BPDUs to the CPU port */
> + mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
> + MT753X_BPDU_CPU_ONLY);
> +
If the switch doesn't currently trap BPDUs, isn't STP broken?
ip link add br0 type bridge stp_state 1
(with or without a userspace helper installed at /sbin/bridge-stp
for more modern protocols than the original 802.1D STP)
> /* Enable and reset MIB counters */
> mt7530_mib_reset(ds);
>
> --
> 2.39.2
>
On Mon, May 22, 2023 at 03:15:31PM +0300, [email protected] wrote:
> From: Vladimir Oltean <[email protected]>
>
> When multiple CPU ports are being used, the numerically smallest CPU port
> becomes the port all user ports become affine to. This may not be the best
> choice for all switches as there may be a numerically greater CPU port with
> more bandwidth than the numerically smallest one.
>
> Such switches are MT7530 and MT7531BE, which the MT7530 DSA subdriver
> controls. Port 5 of these switches has got RGMII whilst port 6 has got
> either TRGMII or SGMII.
>
> Therefore, introduce the preferred_default_local_cpu_port operation to the
> DSA subsystem and use it on the MT7530 DSA subdriver to prefer port 6 as
> the default CPU port.
>
> To prove the benefit of this operation, I (Arınç) have done a bidirectional
> speed test between two DSA user ports on the MT7531BE switch using iperf3.
> The user ports are 1 Gbps full duplex and on different networks so the SoC
> MAC would have to do 2 Gbps TX and 2 Gbps RX to deliver full speed.
I think the real argument would sound like this:
Since the introduction of the OF bindings, DSA has always had a policy
that in case multiple CPU ports are present in the device tree, the
numerically first one is always chosen.
The MT7530 switch family has 2 CPU ports, 5 and 6, where port 6 is
preferable because it has higher bandwidth.
The MT7530 driver developers had 3 options:
- to modify DSA when the driver was introduced, such as to prefer the
better port
- to declare both CPU ports in device trees as CPU ports, and live with
the sub-optimal performance resulting from not preferring the better
port
- to declare just port 6 in the device tree as a CPU port
Of course they chose the path of least resistance (3rd option), kicking
the can down the road. The hardware description in the device tree is
supposed to be stable - developers are not supposed to adopt the
strategy of piecemeal hardware description, where the device tree is
updated in lockstep with the features that the kernel currently supports.
Now, as a result of the fact that they did that, any attempts to modify
the device tree and describe both CPU ports as CPU ports would make DSA
change its default selection from port 6 to 5, effectively resulting in
a performance degradation visible to users as can be seen below vvvvv
>
> Without preferring port 6:
>
> [ ID][Role] Interval Transfer Bitrate Retr
> [ 5][TX-C] 0.00-20.00 sec 374 MBytes 157 Mbits/sec 734 sender
> [ 5][TX-C] 0.00-20.00 sec 373 MBytes 156 Mbits/sec receiver
> [ 7][RX-C] 0.00-20.00 sec 1.81 GBytes 778 Mbits/sec 0 sender
> [ 7][RX-C] 0.00-20.00 sec 1.81 GBytes 777 Mbits/sec receiver
>
> With preferring port 6:
>
> [ ID][Role] Interval Transfer Bitrate Retr
> [ 5][TX-C] 0.00-20.00 sec 1.99 GBytes 856 Mbits/sec 273 sender
> [ 5][TX-C] 0.00-20.00 sec 1.99 GBytes 855 Mbits/sec receiver
> [ 7][RX-C] 0.00-20.00 sec 1.72 GBytes 737 Mbits/sec 15 sender
> [ 7][RX-C] 0.00-20.00 sec 1.71 GBytes 736 Mbits/sec receiver
>
> Using one port for WAN and the other ports for LAN is a very popular use
> case which is what this test emulates.
As such, this change proposes that we retroactively modify stable
kernels to keep the mt7530 driver preferring port 6 even with device
trees where the hardware is more fully described.
Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
>
> This doesn't affect the remaining switches, MT7531AE and the switch on the
> MT7988 SoC. Both CPU ports of the MT7531AE switch have got SGMII and there
> is only one CPU port on the switch on the MT7988 SoC.
>
> Signed-off-by: Vladimir Oltean <[email protected]>
> Signed-off-by: Arınç ÜNAL <[email protected]>
> ---
See the difference in intent?
On Mon, May 22, 2023 at 03:15:32PM +0300, [email protected] wrote:
> From: Arınç ÜNAL <[email protected]>
>
> Add me as a maintainer of the MediaTek MT7530 DSA subdriver.
>
> List maintainers in alphabetical order by first name.
>
> Signed-off-by: Arınç ÜNAL <[email protected]>
> ---
Well, I suppose we need more active people maintaining this driver, so yeah:
Reviewed-by: Vladimir Oltean <[email protected]>
On Mon, May 22, 2023 at 03:15:02PM +0300, [email protected] wrote:
> Hello!
>
> This patch series simplifies the code, improves the logic of the switch
> hardware support, traps LLDP frames and BPDUs for MT7530, MT7531, and
> MT7988 SoC switches, and introduces the preferring local CPU port
> operation.
>
> There's also a patch for fixing the port capabilities of the switch on the
> MT7988 SoC.
>
> I have done a bidirectional speed test using iperf3 on all ports of the
> MT7530 and MT7531 switches with this patch series applied. I have tested
> every possible configuration on the MCM and standalone MT7530 and MT7531
> switch. I'll let the name of the dtb files speak for themselves.
As general feedback for the series, please sort the fixes to be first,
to have as few dependencies as possible, and submit then to the 'net' tree,
leaving cleanup at the end.
On 26.05.2023 16:01, Vladimir Oltean wrote:
> On Thu, May 25, 2023 at 09:49:58AM +0300, Arınç ÜNAL wrote:
>> On 24.05.2023 20:51, Vladimir Oltean wrote:
>>> On Mon, May 22, 2023 at 03:15:10PM +0300, [email protected] wrote:
>>>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>>>> index 710c6622d648..d837aa20968c 100644
>>>> --- a/drivers/net/dsa/mt7530.c
>>>> +++ b/drivers/net/dsa/mt7530.c
>>>> @@ -2728,25 +2722,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;
>>>
>>> If you don't replace this bit with anything, who will set priv->p5_configured
>>> for mt7530?
>>
>> I intend priv->p5_configured and priv->p6_configured to be only used for
>> MT7531 as I have stated on the mt7530_priv description.
>
> Ok, but given the premise of this patch set, that phylink is always available,
> does it make sense for mt7531_cpu_port_config() and mt7988_cpu_port_config()
> to manually call phylink methods?
All I know is that that's how the implementation of phylink's PCS
support in this driver works. It expects the MAC to be set up before
calling mt753x_phylink_pcs_link_up() and mt753x_phylink_mac_link_up().
Arınç
On Sat, Jun 03, 2023 at 03:15:52PM +0300, Arınç ÜNAL wrote:
> On 26.05.2023 16:01, Vladimir Oltean wrote:
> > Ok, but given the premise of this patch set, that phylink is always available,
> > does it make sense for mt7531_cpu_port_config() and mt7988_cpu_port_config()
> > to manually call phylink methods?
>
> All I know is that that's how the implementation of phylink's PCS support in
> this driver works. It expects the MAC to be set up before calling
> mt753x_phylink_pcs_link_up() and mt753x_phylink_mac_link_up().
First, do you see a message printed for the DSA device indicating that
a link is up, without identifying the interface? For example, with
mv88e6xxx:
mv88e6085 f1072004.mdio-mii:04: Link is Up - 1Gbps/Full - flow control off
as opposed to a user port which will look like this:
mv88e6085 f1072004.mdio-mii:04 lan1: Link is Up - 1Gbps/Full - flow control rx/tx
If you do, that's likely for the CPU port, and indicates that phylink
is being used for the CPU port. If not, then you need to investigate
whether you've provided the full description in DT for the CPU port.
In other words, phy-mode and a fixed-link specification or in-band
mode.
Given that, you should have no need to make explicit calls to your
mac_config, pcs_link_up and mac_link_up functions. If you need to
make these calls, it suggests that phylink is not being used for the
CPU port.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Sat, Jun 03, 2023 at 03:15:52PM +0300, Arınç ÜNAL wrote:
> On 26.05.2023 16:01, Vladimir Oltean wrote:
> > Ok, but given the premise of this patch set, that phylink is always available,
> > does it make sense for mt7531_cpu_port_config() and mt7988_cpu_port_config()
> > to manually call phylink methods?
>
> All I know is that that's how the implementation of phylink's PCS support in
> this driver works. It expects the MAC to be set up before calling
> mt753x_phylink_pcs_link_up() and mt753x_phylink_mac_link_up().
No, but I mean, won't phylink call mt7531_mac_config(), mt753x_phylink_pcs_link_up()
and mt753x_phylink_mac_link_up() automatically and in the expected order already,
and if not, what prevents that from happening? I just don't understand why the
cpu_port_config() methods of MT7531 and MT7988 call phylink methods manually
from the driver.
On Sun, Jun 04, 2023 at 10:05:44AM +0300, Arınç ÜNAL wrote:
> It wouldn't. I'll drop this patch, thanks. For reference, PMCR_TX_EN (bit
> 14) for port 5 is also set in the case of PHY muxing with this code on
> mt7530_setup_port5() which doesn't use phylink.
>
> /* Setup the MAC by default for the cpu port */
> mt7530_write(priv, MT7530_PMCR_P(5), 0x56300);
>
> 0x56300 = 0101 0110 0011 0000 0000
>
> Maybe I should make a patch to use the definitions on mt7530.h for this?
>
> Arınç
Yes, I don't believe magic numbers make things particularly obvious for
other driver writers, they should be avoided if possible.
On 26.05.2023 16:25, Vladimir Oltean wrote:
> On Mon, May 22, 2023 at 03:15:21PM +0300, [email protected] wrote:
>> From: Arınç ÜNAL <[email protected]>
>>
>> Setting this register related to interrupts is only needed for the MT7530
>> switch. Make an exclusive check to ensure this.
>>
>> Signed-off-by: Arınç ÜNAL <[email protected]>
>> Acked-by: Daniel Golle <[email protected]>
>> Tested-by: Daniel Golle <[email protected]>
>> ---
>
> Why does it matter? What prompted you to make this change? I guess it's
> not needed for MT7988? Or the register is not present? Or?...
It's not needed for the switch on the MT7988 SoC. The register is also
likely specific to the MT7530 switch.
Arınç
On 26.05.2023 16:17, Vladimir Oltean wrote:
> On Mon, May 22, 2023 at 03:15:18PM +0300, [email protected] wrote:
>> From: Arınç ÜNAL <[email protected]>
>>
>> Move lowering Tx driving of rgmii on port 5 to right before lowering of Tx
>> driving of trgmii on port 6 on mt7530_setup().
>>
>> This way, the switch should consume less power regardless of port 5 being
>> used.
>>
>> Tested-by: Arınç ÜNAL <[email protected]>
>> Signed-off-by: Arınç ÜNAL <[email protected]>
>> ---
>
> This patch assumes that the MAC has its TXC ticking even when PMCR_TX_EN
> is unset. Why would it do that?
It wouldn't. I'll drop this patch, thanks. For reference, PMCR_TX_EN
(bit 14) for port 5 is also set in the case of PHY muxing with this code
on mt7530_setup_port5() which doesn't use phylink.
/* Setup the MAC by default for the cpu port */
mt7530_write(priv, MT7530_PMCR_P(5), 0x56300);
0x56300 = 0101 0110 0011 0000 0000
Maybe I should make a patch to use the definitions on mt7530.h for this?
Arınç
On 26.05.2023 18:50, Russell King (Oracle) wrote:
> On Fri, May 26, 2023 at 06:42:58PM +0300, Vladimir Oltean wrote:
>> On Mon, May 22, 2023 at 03:15:26PM +0300, [email protected] wrote:
>>> /* Disable flooding on all ports */
>>> - mt7530_clear(priv, MT7530_MFC, BC_FFP_MASK | UNM_FFP_MASK |
>>> - UNU_FFP_MASK);
>>> + mt7530_clear(priv, MT753X_MFC, MT753X_BC_FFP_MASK | MT753X_UNM_FFP_MASK
>>> + | MT753X_UNU_FFP_MASK);
>>
>> The preferred coding style is not to start new lines with operators.
>>
>>> +/* Register for CPU forward control */
>>> #define MT7531_CFC 0x4
>>> #define MT7531_MIRROR_EN BIT(19)
>>> -#define MT7531_MIRROR_MASK (MIRROR_MASK << 16)
>>> -#define MT7531_MIRROR_PORT_GET(x) (((x) >> 16) & MIRROR_MASK)
>>> -#define MT7531_MIRROR_PORT_SET(x) (((x) & MIRROR_MASK) << 16)
>>> +#define MT7531_MIRROR_MASK (0x7 << 16)
>>
>> minor nitpick: if you express this as GENMASK(18, 16), it will be a bit
>> easier to cross-check with the datasheet, since both 18 and 16 are more
>> representative than 0x7.
>>
>>> +#define MT7531_MIRROR_PORT_GET(x) (((x) >> 16) & 0x7)
>>
>> also here: (((x) & GENMASK(18, 16)) >> 16)
>
> Even better are:
> #define MT7531_MIRROR_PORT_GET(x) FIELD_GET(MT7531_MIRROR_MASK, x)
>
>>
>>> +#define MT7531_MIRROR_PORT_SET(x) (((x) & 0x7) << 16)
>>
>> and here: (((x) << 16) & GENMASK(18, 16))
>
> #define MT7531_MIRROR_PORT_SET(x) FIELD_PREP(MT7531_MIRROR_MASK, x)
>
> No need to add parens around "x" in either of these uses as we're not
> doing anything with x other than passing it into another macro.
Thanks. I suppose the GENMASK, FIELD_PREP, and FIELD_GET macros can be
widely used on mt7530.h? Like GENMASK(2, 0) on MT7530_MIRROR_MASK and
FIELD_PREP(MT7530_MIRROR_MASK, x) on MT7530_MIRROR_PORT(x)?
Arınç
On 26.05.2023 18:51, Vladimir Oltean wrote:
> On Mon, May 22, 2023 at 03:15:27PM +0300, [email protected] wrote:
>> From: Arınç ÜNAL <[email protected]>
>>
>> Every bit of the CPU port bitmap for MT7531 and the switch on the MT7988
>> SoC represents a CPU port to trap frames to. Currently only the bit that
>> corresponds to the first found CPU port is set on the bitmap. Introduce the
>> MT7531_CPU_PMAP macro to individually set the bits of the CPU port bitmap.
>> Set the CPU port bitmap for MT7531 and the switch on the MT7988 SoC on
>> mt753x_cpu_port_enable() which runs on a loop for each CPU port. Add
>> comments to explain this.
>>
>> According to the document MT7531 Reference Manual for Development Board
>> v1.0, the MT7531_CPU_PMAP bits are unset after reset so no need to clear it
>> beforehand. Since there's currently no public document for the switch on
>> the MT7988 SoC, I assume this is also the case for this switch.
>>
>> Tested-by: Arınç ÜNAL <[email protected]>
>> Signed-off-by: Arınç ÜNAL <[email protected]>
>> ---
>
> Is this supposed to be a bug fix? (incompatibility with past or future
> device trees also counts as bugs) If so, it needs a Fixes: tag and to be
> targeted towards the "net" tree. Also, the impact of the current behavior
> and of the change need to be explained better.
Yes, this fixes a bug for future devicetrees. I will send this to net
with a more detailed explanation, thanks.
>
>> drivers/net/dsa/mt7530.c | 15 ++++++++-------
>> drivers/net/dsa/mt7530.h | 3 ++-
>> 2 files changed, 10 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index 58d8738d94d3..0b513e3628fe 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -963,6 +963,13 @@ mt753x_cpu_port_enable(struct dsa_switch *ds, int port)
>> mt7530_rmw(priv, MT753X_MFC, MT7530_CPU_MASK, MT7530_CPU_EN |
>> MT7530_CPU_PORT(port));
>>
>> + /* Add the CPU port to the CPU port bitmap for MT7531 and the switch on
>> + * the MT7988 SoC. Any frames set for trapping to CPU port will be
>> + * trapped to the CPU port the user port is affine to.
>> + */
>> + if (priv->id == ID_MT7531 || priv->id == ID_MT7988)
>> + mt7530_set(priv, MT7531_CFC, MT7531_CPU_PMAP(BIT(port)));
>> +
>
> Stylistically, the existence of an indirect call to priv->info->cpu_port_config()
> per switch family is a bit dissonant with an explicit check for device id later
> in the same function.
mt753x_cpu_port_enable() is not being called from
priv->info->cpu_port_config() though. I'm not sure how I would do this
without the device ID check here.
>
>> /* CPU port gets connected to all user ports of
>> * the switch.
>> */
>> @@ -2315,15 +2322,9 @@ static int
>> mt7531_setup_common(struct dsa_switch *ds)
>> {
>> struct mt7530_priv *priv = ds->priv;
>> - struct dsa_port *cpu_dp;
>> int ret, i;
>>
>> - /* BPDU to CPU port */
>> - dsa_switch_for_each_cpu_port(cpu_dp, ds) {
>> - mt7530_rmw(priv, MT7531_CFC, MT7531_CPU_PMAP_MASK,
>> - BIT(cpu_dp->index));
>> - break;
>> - }
>> + /* Trap BPDUs to the CPU port(s) */
>> mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
>> MT753X_BPDU_CPU_ONLY);
>>
>> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
>> index 5ebb942b07ef..fd2a2f726b8a 100644
>> --- a/drivers/net/dsa/mt7530.h
>> +++ b/drivers/net/dsa/mt7530.h
>> @@ -53,7 +53,8 @@ enum mt753x_id {
>> #define MT7531_MIRROR_MASK (0x7 << 16)
>> #define MT7531_MIRROR_PORT_GET(x) (((x) >> 16) & 0x7)
>> #define MT7531_MIRROR_PORT_SET(x) (((x) & 0x7) << 16)
>> -#define MT7531_CPU_PMAP_MASK GENMASK(7, 0)
>> +#define MT7531_CPU_PMAP(x) ((x) & 0xff)
>
> You can leave this as ((x) & GENMASK(7, 0))
Now that I've read Russell's comment on the previous patch, the below
would be even better?
MT7531_CPU_PMAP(x) FIELD_PREP(MT7531_CPU_PMAP_MASK, x)
>
>> +#define MT7531_CPU_PMAP_MASK MT7531_CPU_PMAP(~0)
>
> There's no other user of MT7531_CPU_PMAP_MASK, you can remove this.
Should I do above or remove this?
Arınç
On 26.05.2023 20:02, Vladimir Oltean wrote:
> On Mon, May 22, 2023 at 03:15:29PM +0300, [email protected] wrote:
>> From: Arınç ÜNAL <[email protected]>
>>
>> The MT753X switches are capable of trapping certain frames. Introduce
>> trapping BPDUs to the CPU port for the MT7530 switch.
>>
>> BPDUs will be trapped to the numerically smallest CPU port which is affine
>> to the DSA conduit interface that is set up. The BPDUs won't necessarily be
>> trapped to the CPU port the user port, which these BPDUs are received from,
>> is affine to.
>>
>> Tested-by: Arınç ÜNAL <[email protected]>
>> Signed-off-by: Arınç ÜNAL <[email protected]>
>> ---
>> drivers/net/dsa/mt7530.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index cd16911fcb01..2fb4b0bc6335 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -2223,6 +2223,10 @@ mt7530_setup(struct dsa_switch *ds)
>> val |= MHWTRAP_MANUAL;
>> mt7530_write(priv, MT7530_MHWTRAP, val);
>>
>> + /* Trap BPDUs to the CPU port */
>> + mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
>> + MT753X_BPDU_CPU_ONLY);
>> +
>
> If the switch doesn't currently trap BPDUs, isn't STP broken?
No, the BPDU_PORT_FW bits are 0 after reset. The MT7620 programming
guide states that frames with 01:80:C2:00:00:00 MAC DA (which is how the
BPDU distinction is being made) will follow the system default which
means the BPDUs will be treated as normal multicast frames.
Only if all 3 bits are set will the BPDUs be dropped.
>
> ip link add br0 type bridge stp_state 1
> (with or without a userspace helper installed at /sbin/bridge-stp
> for more modern protocols than the original 802.1D STP)
For reference, the mstpd package on Buildroot includes this.
Arınç
On 26.05.2023 19:55, Vladimir Oltean wrote:
> On Mon, May 22, 2023 at 03:15:28PM +0300, [email protected] wrote:
>> From: Arınç ÜNAL <[email protected]>
>>
>> The MT7530_CPU_PORT bits represent the CPU port to trap frames to for the
>> MT7530 switch. There are two issues with the current way of setting these
>> bits. ID_MT7530 which is for the standalone MT7530 switch is not included.
>
> It's best to say in the commit title what the change does, rather than
> the equivalent of "here, this way is proper!". Commit titles should be
> uniquely identifiable, and "properly set MT7530_CPU_PORT" doesn't say a
> lot about how proper it is. It's enough to imagine a future person
> finding something else that's perfectible and writing another "net: dsa:
> mt7530: properly set MT7530_CPU_PORT" commit. Try to be less definitive
> and at the same time more specific.
>
> If there are 2 issues, there should be 2 changes with individual titles
> which each describes what was wrong and how that was changed.
Got it, this is a bug fix for future devicetrees so I will send a
2-patch patch series to net. First one sets the MT7530_CPU_PORT bit to
the active CPU port, the other adds the ID_MT7530 check.
>
>> When multiple CPU ports are being used, the trapped frames won't be
>> received when the DSA conduit interface, which the frames are supposed to
>> be trapped to, is down because it's not affine to any user port. This
>> requires the DSA conduit interface to be manually set up for the trapped
>> frames to be received.
>>
>> Address these issues by implementing ds->ops->master_state_change() on this
>> subdriver and setting the MT7530_CPU_PORT bits there. Introduce the
>> active_cpu_ports field to store the information of active CPU ports.
>> Correct the macros, MT7530_CPU_PORT is bits 4 through 6 of the register.
>>
>> Any frames set for trapping to CPU port will be trapped to the numerically
>> smallest CPU port which is affine to the DSA conduit interface that is set
>> up. To make the understatement obvious, the frames won't necessarily be
>> trapped to the CPU port the user port, which these frames are received
>> from, is affine to. This operation is only there to make sure the trapped
>> frames always reach the CPU.
>>
>> Tested-by: Arınç ÜNAL <[email protected]>
>> Co-developed-by: Vladimir Oltean <[email protected]>
>> Signed-off-by: Vladimir Oltean <[email protected]>
>
> A single Suggested-by: is fine. As a rule of thumb, I would use Co-developed-by
> when I'm working with a patch formally pre-formatted or committed by somebody else,
> that I've changed in a significant manner. Since all I did was to comment with
> a suggestion of how to handle this, and with a code snippet written in the email
> client to a patch of yours, I don't believe that's necessary here.
Will do, thanks.
Arınç
On Sun, Jun 04, 2023 at 11:51:33AM +0300, Arınç ÜNAL wrote:
> > If the switch doesn't currently trap BPDUs, isn't STP broken?
>
> No, the BPDU_PORT_FW bits are 0 after reset. The MT7620 programming guide
> states that frames with 01:80:C2:00:00:00 MAC DA (which is how the BPDU
> distinction is being made) will follow the system default which means the
> BPDUs will be treated as normal multicast frames.
>
> Only if all 3 bits are set will the BPDUs be dropped.
Right, if you don't trap BPDUs just to the CPU but flood them, I believe
the STP protocol won't behave properly with switching loops. Worth testing.
On 4.06.2023 12:23, Vladimir Oltean wrote:
> On Sun, Jun 04, 2023 at 11:51:33AM +0300, Arınç ÜNAL wrote:
>>> If the switch doesn't currently trap BPDUs, isn't STP broken?
>>
>> No, the BPDU_PORT_FW bits are 0 after reset. The MT7620 programming guide
>> states that frames with 01:80:C2:00:00:00 MAC DA (which is how the BPDU
>> distinction is being made) will follow the system default which means the
>> BPDUs will be treated as normal multicast frames.
>>
>> Only if all 3 bits are set will the BPDUs be dropped.
>
> Right, if you don't trap BPDUs just to the CPU but flood them, I believe
> the STP protocol won't behave properly with switching loops. Worth testing.
I've got no interest spending time playing around with STP at the moment
so I'm going to pass.
Arınç
On 26.05.2023 20:17, Vladimir Oltean wrote:
> On Mon, May 22, 2023 at 03:15:31PM +0300, [email protected] wrote:
>> From: Vladimir Oltean <[email protected]>
>>
>> When multiple CPU ports are being used, the numerically smallest CPU port
>> becomes the port all user ports become affine to. This may not be the best
>> choice for all switches as there may be a numerically greater CPU port with
>> more bandwidth than the numerically smallest one.
>>
>> Such switches are MT7530 and MT7531BE, which the MT7530 DSA subdriver
>> controls. Port 5 of these switches has got RGMII whilst port 6 has got
>> either TRGMII or SGMII.
>>
>> Therefore, introduce the preferred_default_local_cpu_port operation to the
>> DSA subsystem and use it on the MT7530 DSA subdriver to prefer port 6 as
>> the default CPU port.
>>
>> To prove the benefit of this operation, I (Arınç) have done a bidirectional
>> speed test between two DSA user ports on the MT7531BE switch using iperf3.
>> The user ports are 1 Gbps full duplex and on different networks so the SoC
>> MAC would have to do 2 Gbps TX and 2 Gbps RX to deliver full speed.
>
> I think the real argument would sound like this:
>
> Since the introduction of the OF bindings, DSA has always had a policy
> that in case multiple CPU ports are present in the device tree, the
> numerically first one is always chosen.
>
> The MT7530 switch family has 2 CPU ports, 5 and 6, where port 6 is
> preferable because it has higher bandwidth.
>
> The MT7530 driver developers had 3 options:
> - to modify DSA when the driver was introduced, such as to prefer the
> better port
> - to declare both CPU ports in device trees as CPU ports, and live with
> the sub-optimal performance resulting from not preferring the better
> port
> - to declare just port 6 in the device tree as a CPU port
>
> Of course they chose the path of least resistance (3rd option), kicking
> the can down the road. The hardware description in the device tree is
> supposed to be stable - developers are not supposed to adopt the
> strategy of piecemeal hardware description, where the device tree is
> updated in lockstep with the features that the kernel currently supports.
>
> Now, as a result of the fact that they did that, any attempts to modify
> the device tree and describe both CPU ports as CPU ports would make DSA
> change its default selection from port 6 to 5, effectively resulting in
> a performance degradation visible to users as can be seen below vvvvv
>
>>
>> Without preferring port 6:
>>
>> [ ID][Role] Interval Transfer Bitrate Retr
>> [ 5][TX-C] 0.00-20.00 sec 374 MBytes 157 Mbits/sec 734 sender
>> [ 5][TX-C] 0.00-20.00 sec 373 MBytes 156 Mbits/sec receiver
>> [ 7][RX-C] 0.00-20.00 sec 1.81 GBytes 778 Mbits/sec 0 sender
>> [ 7][RX-C] 0.00-20.00 sec 1.81 GBytes 777 Mbits/sec receiver
>>
>> With preferring port 6:
>>
>> [ ID][Role] Interval Transfer Bitrate Retr
>> [ 5][TX-C] 0.00-20.00 sec 1.99 GBytes 856 Mbits/sec 273 sender
>> [ 5][TX-C] 0.00-20.00 sec 1.99 GBytes 855 Mbits/sec receiver
>> [ 7][RX-C] 0.00-20.00 sec 1.72 GBytes 737 Mbits/sec 15 sender
>> [ 7][RX-C] 0.00-20.00 sec 1.71 GBytes 736 Mbits/sec receiver
>>
>> Using one port for WAN and the other ports for LAN is a very popular use
>> case which is what this test emulates.
>
> As such, this change proposes that we retroactively modify stable
> kernels to keep the mt7530 driver preferring port 6 even with device
> trees where the hardware is more fully described.
>
> Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
>
>>
>> This doesn't affect the remaining switches, MT7531AE and the switch on the
>> MT7988 SoC. Both CPU ports of the MT7531AE switch have got SGMII and there
>> is only one CPU port on the switch on the MT7988 SoC.
>>
>> Signed-off-by: Vladimir Oltean <[email protected]>
>> Signed-off-by: Arınç ÜNAL <[email protected]>
>> ---
>
> See the difference in intent?
Yeah, nicely put.
Arınç
On 3.06.2023 15:26, Russell King (Oracle) wrote:
> On Sat, Jun 03, 2023 at 03:15:52PM +0300, Arınç ÜNAL wrote:
>> On 26.05.2023 16:01, Vladimir Oltean wrote:
>>> Ok, but given the premise of this patch set, that phylink is always available,
>>> does it make sense for mt7531_cpu_port_config() and mt7988_cpu_port_config()
>>> to manually call phylink methods?
>>
>> All I know is that that's how the implementation of phylink's PCS support in
>> this driver works. It expects the MAC to be set up before calling
>> mt753x_phylink_pcs_link_up() and mt753x_phylink_mac_link_up().
>
> First, do you see a message printed for the DSA device indicating that
> a link is up, without identifying the interface? For example, with
> mv88e6xxx:
>
> mv88e6085 f1072004.mdio-mii:04: Link is Up - 1Gbps/Full - flow control off
>
> as opposed to a user port which will look like this:
>
> mv88e6085 f1072004.mdio-mii:04 lan1: Link is Up - 1Gbps/Full - flow control rx/tx
>
> If you do, that's likely for the CPU port, and indicates that phylink
> is being used for the CPU port. If not, then you need to investigate
> whether you've provided the full description in DT for the CPU port.
> In other words, phy-mode and a fixed-link specification or in-band
> mode.
Yes I do see this. The DT is properly defined and the port is properly
set up as a CPU port.
>
> Given that, you should have no need to make explicit calls to your
> mac_config, pcs_link_up and mac_link_up functions. If you need to
> make these calls, it suggests that phylink is not being used for the
> CPU port.
Your own commit does this so I don't know what to tell you.
https://github.com/torvalds/linux/commit/cbd1f243bc41056c76fcfc5f3380cfac1f00d37b
Arınç
On Sun, Jun 04, 2023 at 01:46:46PM +0300, Arınç ÜNAL wrote:
> On 3.06.2023 15:26, Russell King (Oracle) wrote:
> > Given that, you should have no need to make explicit calls to your
> > mac_config, pcs_link_up and mac_link_up functions. If you need to
> > make these calls, it suggests that phylink is not being used for the
> > CPU port.
>
> Your own commit does this so I don't know what to tell you.
>
> https://github.com/torvalds/linux/commit/cbd1f243bc41056c76fcfc5f3380cfac1f00d37b
I would like to make a comment here to explain why in that commit I
added a call into mt7531_cpu_port_config() to mt7531_cpu_port_config().
When I'm making changes to drivers, then I follow a golden rule: do
not change the behaviour unless it is an intentional change.
This is exactly what is going on in this commit.
mt7531_cpu_port_config() called mt753x_phylink_mac_link_up(), which
then, as the very first thing, called mt753x_mac_pcs_link_up().
mt753x_mac_pcs_link_up() called priv->info->mac_pcs_link_up() if
it is defined.
Since converting to phylink_pcs involves the removal of the direct
call from mt753x_phylink_mac_link_up() to the
priv->info->mac_pcs_link_up() method, in order to preserve the
behaviour of the driver, it is necessary to ensure that
mt7531_cpu_port_config() still makes that call.
mt753x_phylink_pcs_link_up() is the new function replacing
mt753x_mac_pcs_link_up() which makes that call, so it is entirely
appropriate to add that call into mt7531_cpu_port_config() so that
mt7531_cpu_port_config() behaves _precisely_ the same as it did
before and after this change.
In that sense, as far as mt7531_cpu_port_config() is concerned, this
change is entirely idempotent.
I don't know whether mt7531_cpu_port_config() is necessary to properly
bring up a CPU port, or whether _all_ firmware descriptions for
mt7531 include all the necessary properties so that DSA will always
bring up a phylink instance for the CPU port - that really is not
relevant for the change you point out.
What is relevant is only making the intended change, and the intended
change is to split the PCS-specific code from the MAC-specific code.
This principle of only making one change in a patch, and ensuring that
parts of the code which merely need to be re-organised to achieve that
change are done in an idempotent way is fundamental to good code
maintenance, especially when modifying drivers that one does not have
the hardware to be able to test.
You have provided new information - that basically indicates that
phylink is used for your setup. If we can get to a position where we
can confidently say that phylink will always be used for the CPU port,
the code in mt7531_cpu_port_config() that bypasses phylink, calling
methods in phylink's mac_ops and pcs_ops, should be removed.
I don't remember whether Vladimir's firmware validator will fail for
mt753x if CPU ports are not fully described, but that would be well
worth checking. If it does, then we can be confident that phylink
will always be used, and those bypassing calls should not be necessary.
I hope this explains the situation better.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Sun, Jun 04, 2023 at 12:39:33PM +0300, Arınç ÜNAL wrote:
> On 4.06.2023 12:23, Vladimir Oltean wrote:
> > On Sun, Jun 04, 2023 at 11:51:33AM +0300, Arınç ÜNAL wrote:
> > > > If the switch doesn't currently trap BPDUs, isn't STP broken?
> > >
> > > No, the BPDU_PORT_FW bits are 0 after reset. The MT7620 programming guide
> > > states that frames with 01:80:C2:00:00:00 MAC DA (which is how the BPDU
> > > distinction is being made) will follow the system default which means the
> > > BPDUs will be treated as normal multicast frames.
> > >
> > > Only if all 3 bits are set will the BPDUs be dropped.
> >
> > Right, if you don't trap BPDUs just to the CPU but flood them, I believe
> > the STP protocol won't behave properly with switching loops. Worth testing.
>
> I've got no interest spending time playing around with STP at the moment so
> I'm going to pass.
You can at the very least move it towards the beginning of the net-next patch
set, so that we can be sure it doesn't depend on the other refactoring work,
in case someone in the future makes a request for the patch to be backported
to stable.
On Sun, Jun 04, 2023 at 01:18:04PM +0100, Russell King (Oracle) wrote:
> I don't remember whether Vladimir's firmware validator will fail for
> mt753x if CPU ports are not fully described, but that would be well
> worth checking. If it does, then we can be confident that phylink
> will always be used, and those bypassing calls should not be necessary.
It does, I've just retested this:
[ 8.469152] mscc_felix 0000:00:00.5: OF node /soc/pcie@1f0000000/ethernet-switch@0,5/ports/port@4 of CPU port 4 lacks the required "phy-handle", "fixed-link" or "managed" properties
[ 8.494571] mscc_felix 0000:00:00.5: error -EINVAL: Failed to register DSA switch
[ 8.502151] mscc_felix: probe of 0000:00:00.5 failed with error -22
On Sun, Jun 04, 2023 at 11:21:48AM +0300, Arınç ÜNAL wrote:
> > Stylistically, the existence of an indirect call to priv->info->cpu_port_config()
> > per switch family is a bit dissonant with an explicit check for device id later
> > in the same function.
>
> mt753x_cpu_port_enable() is not being called from priv->info->cpu_port_config()
> though.
Quite the other way around. I'm saying that mt753x_cpu_port_enable(),
the function whose logic you're changing, already has a mechanism to
execute code specific to one switch family.
> I'm not sure how I would do this without the device ID check here.
Hmm, by defining a new mt7530_cpu_port_config() procedure for ID_MT7621
and ID_MT7530?
Although in a different thread we are perhaps challenging the idea that
what is currently in priv->info->cpu_port_config() is useful - at least
half of it are manual invocations of phylink methods which are possibly
not needed. If after the removal of those, it no longer makes sense to
have priv->info->cpu_port_config() at all, then I'm not saying that the
explicit check for device id here doesn't make sense. Just that it's not
in harmony with what currently exists 3 lines above.
> > > -#define MT7531_CPU_PMAP_MASK GENMASK(7, 0)
> > > +#define MT7531_CPU_PMAP(x) ((x) & 0xff)
> >
> > You can leave this as ((x) & GENMASK(7, 0))
>
> Now that I've read Russell's comment on the previous patch, the below would
> be even better?
>
> MT7531_CPU_PMAP(x) FIELD_PREP(MT7531_CPU_PMAP_MASK, x)
>
> >
> > > +#define MT7531_CPU_PMAP_MASK MT7531_CPU_PMAP(~0)
> >
> > There's no other user of MT7531_CPU_PMAP_MASK, you can remove this.
>
> Should I do above or remove this?
No specific preference. If you want to make this driver start using
FIELD_PREP() then go ahead.
On 4.06.2023 16:07, Russell King (Oracle) wrote:
> On Sun, Jun 04, 2023 at 03:55:17PM +0300, Vladimir Oltean wrote:
>> On Sun, Jun 04, 2023 at 01:18:04PM +0100, Russell King (Oracle) wrote:
>>> I don't remember whether Vladimir's firmware validator will fail for
>>> mt753x if CPU ports are not fully described, but that would be well
>>> worth checking. If it does, then we can be confident that phylink
>>> will always be used, and those bypassing calls should not be necessary.
>>
>> It does, I've just retested this:
>>
>> [ 8.469152] mscc_felix 0000:00:00.5: OF node /soc/pcie@1f0000000/ethernet-switch@0,5/ports/port@4 of CPU port 4 lacks the required "phy-handle", "fixed-link" or "managed" properties
>> [ 8.494571] mscc_felix 0000:00:00.5: error -EINVAL: Failed to register DSA switch
>> [ 8.502151] mscc_felix: probe of 0000:00:00.5 failed with error -22
>
> ... which isn't listed in dsa_switches_apply_workarounds[], and
> neither is mt753x. Thanks.
>
> So, that should be sufficient to know that the CPU port will always
> properly described, and thus bypassing phylink in mt753x for the CPU
> port should not be necessary.
Perfect! If I understand correctly, there's this code - specific to
MT7531 and MT7988 ports being used as CPU ports - which runs in addition
to what's in mt753x_phylink_mac_config():
mt7530_write(priv, MT7530_PMCR_P(port),
PMCR_CPU_PORT_SETTING(priv->id));
This should be put on mt753x_phylink_mac_config(), under priv->id ==
ID_MT7531, priv->id == ID_MT7988, and dsa_is_cpu_port(ds, port) checks?
Arınç
On Sun, Jun 04, 2023 at 11:06:32AM +0300, Arınç ÜNAL wrote:
> > Even better are:
> > #define MT7531_MIRROR_PORT_GET(x) FIELD_GET(MT7531_MIRROR_MASK, x)
> >
> > > > +#define MT7531_MIRROR_PORT_SET(x) (((x) & 0x7) << 16)
> > >
> > > and here: (((x) << 16) & GENMASK(18, 16))
> >
> > #define MT7531_MIRROR_PORT_SET(x) FIELD_PREP(MT7531_MIRROR_MASK, x)
> >
> > No need to add parens around "x" in either of these uses as we're not
> > doing anything with x other than passing it into another macro.
>
> Thanks. I suppose the GENMASK, FIELD_PREP, and FIELD_GET macros can be
> widely used on mt7530.h? Like GENMASK(2, 0) on MT7530_MIRROR_MASK and
> FIELD_PREP(MT7530_MIRROR_MASK, x) on MT7530_MIRROR_PORT(x)?
I suppose the answer would be "yes, they can be used", but then, I'm not
really sure what answer you're expecting.
On Sun, Jun 04, 2023 at 03:55:17PM +0300, Vladimir Oltean wrote:
> On Sun, Jun 04, 2023 at 01:18:04PM +0100, Russell King (Oracle) wrote:
> > I don't remember whether Vladimir's firmware validator will fail for
> > mt753x if CPU ports are not fully described, but that would be well
> > worth checking. If it does, then we can be confident that phylink
> > will always be used, and those bypassing calls should not be necessary.
>
> It does, I've just retested this:
>
> [ 8.469152] mscc_felix 0000:00:00.5: OF node /soc/pcie@1f0000000/ethernet-switch@0,5/ports/port@4 of CPU port 4 lacks the required "phy-handle", "fixed-link" or "managed" properties
> [ 8.494571] mscc_felix 0000:00:00.5: error -EINVAL: Failed to register DSA switch
> [ 8.502151] mscc_felix: probe of 0000:00:00.5 failed with error -22
... which isn't listed in dsa_switches_apply_workarounds[], and
neither is mt753x. Thanks.
So, that should be sufficient to know that the CPU port will always
properly described, and thus bypassing phylink in mt753x for the CPU
port should not be necessary.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On 4.06.2023 16:17, Vladimir Oltean wrote:
> On Sun, Jun 04, 2023 at 11:06:32AM +0300, Arınç ÜNAL wrote:
>>> Even better are:
>>> #define MT7531_MIRROR_PORT_GET(x) FIELD_GET(MT7531_MIRROR_MASK, x)
>>>
>>>>> +#define MT7531_MIRROR_PORT_SET(x) (((x) & 0x7) << 16)
>>>>
>>>> and here: (((x) << 16) & GENMASK(18, 16))
>>>
>>> #define MT7531_MIRROR_PORT_SET(x) FIELD_PREP(MT7531_MIRROR_MASK, x)
>>>
>>> No need to add parens around "x" in either of these uses as we're not
>>> doing anything with x other than passing it into another macro.
>>
>> Thanks. I suppose the GENMASK, FIELD_PREP, and FIELD_GET macros can be
>> widely used on mt7530.h? Like GENMASK(2, 0) on MT7530_MIRROR_MASK and
>> FIELD_PREP(MT7530_MIRROR_MASK, x) on MT7530_MIRROR_PORT(x)?
>
> I suppose the answer would be "yes, they can be used", but then, I'm not
> really sure what answer you're expecting.
I was thinking of replacing all manual definitions with these macros on
mt7530.h. For now, I'll just make sure my current changes use them.
Arınç
On 4.06.2023 16:08, Vladimir Oltean wrote:
> On Sun, Jun 04, 2023 at 11:21:48AM +0300, Arınç ÜNAL wrote:
>>> Stylistically, the existence of an indirect call to priv->info->cpu_port_config()
>>> per switch family is a bit dissonant with an explicit check for device id later
>>> in the same function.
>>
>> mt753x_cpu_port_enable() is not being called from priv->info->cpu_port_config()
>> though.
>
> Quite the other way around. I'm saying that mt753x_cpu_port_enable(),
> the function whose logic you're changing, already has a mechanism to
> execute code specific to one switch family.
Ah, makes sense.
>
>> I'm not sure how I would do this without the device ID check here.
>
> Hmm, by defining a new mt7530_cpu_port_config() procedure for ID_MT7621
> and ID_MT7530?
>
> Although in a different thread we are perhaps challenging the idea that
> what is currently in priv->info->cpu_port_config() is useful - at least
> half of it are manual invocations of phylink methods which are possibly
> not needed. If after the removal of those, it no longer makes sense to
> have priv->info->cpu_port_config() at all, then I'm not saying that the
> explicit check for device id here doesn't make sense. Just that it's not
> in harmony with what currently exists 3 lines above.
Regardless of the outcome of that conversation, I would like to avoid
structural changes like this since this patch will go to net.
>
>>>> -#define MT7531_CPU_PMAP_MASK GENMASK(7, 0)
>>>> +#define MT7531_CPU_PMAP(x) ((x) & 0xff)
>>>
>>> You can leave this as ((x) & GENMASK(7, 0))
>>
>> Now that I've read Russell's comment on the previous patch, the below would
>> be even better?
>>
>> MT7531_CPU_PMAP(x) FIELD_PREP(MT7531_CPU_PMAP_MASK, x)
>>
>>>
>>>> +#define MT7531_CPU_PMAP_MASK MT7531_CPU_PMAP(~0)
>>>
>>> There's no other user of MT7531_CPU_PMAP_MASK, you can remove this.
>>
>> Should I do above or remove this?
>
> No specific preference. If you want to make this driver start using
> FIELD_PREP() then go ahead.
Will do.
Arınç
On Sun, Jun 04, 2023 at 04:14:31PM +0300, Arınç ÜNAL wrote:
> On 4.06.2023 16:07, Russell King (Oracle) wrote:
> > On Sun, Jun 04, 2023 at 03:55:17PM +0300, Vladimir Oltean wrote:
> > > On Sun, Jun 04, 2023 at 01:18:04PM +0100, Russell King (Oracle) wrote:
> > > > I don't remember whether Vladimir's firmware validator will fail for
> > > > mt753x if CPU ports are not fully described, but that would be well
> > > > worth checking. If it does, then we can be confident that phylink
> > > > will always be used, and those bypassing calls should not be necessary.
> > >
> > > It does, I've just retested this:
> > >
> > > [ 8.469152] mscc_felix 0000:00:00.5: OF node /soc/pcie@1f0000000/ethernet-switch@0,5/ports/port@4 of CPU port 4 lacks the required "phy-handle", "fixed-link" or "managed" properties
> > > [ 8.494571] mscc_felix 0000:00:00.5: error -EINVAL: Failed to register DSA switch
> > > [ 8.502151] mscc_felix: probe of 0000:00:00.5 failed with error -22
> >
> > ... which isn't listed in dsa_switches_apply_workarounds[], and
> > neither is mt753x. Thanks.
> >
> > So, that should be sufficient to know that the CPU port will always
> > properly described, and thus bypassing phylink in mt753x for the CPU
> > port should not be necessary.
>
> Perfect! If I understand correctly, there's this code - specific to MT7531
> and MT7988 ports being used as CPU ports - which runs in addition to what's
> in mt753x_phylink_mac_config():
>
> mt7530_write(priv, MT7530_PMCR_P(port),
> PMCR_CPU_PORT_SETTING(priv->id));
>
> This should be put on mt753x_phylink_mac_config(), under priv->id ==
> ID_MT7531, priv->id == ID_MT7988, and dsa_is_cpu_port(ds, port) checks?
>
> Arınç
Given that mt753x_phylink_mac_config() and mt753x_phylink_mac_link_up() also
both modifies MT7530_PMCR_P(port), have you studied the code to see what
really is changed compared to what's in the PMCR_CPU_PORT_SETTING() macro,
after both phylink methods have run?
On Sun, Jun 04, 2023 at 04:14:31PM +0300, Arınç ÜNAL wrote:
> On 4.06.2023 16:07, Russell King (Oracle) wrote:
> > On Sun, Jun 04, 2023 at 03:55:17PM +0300, Vladimir Oltean wrote:
> > > On Sun, Jun 04, 2023 at 01:18:04PM +0100, Russell King (Oracle) wrote:
> > > > I don't remember whether Vladimir's firmware validator will fail for
> > > > mt753x if CPU ports are not fully described, but that would be well
> > > > worth checking. If it does, then we can be confident that phylink
> > > > will always be used, and those bypassing calls should not be necessary.
> > >
> > > It does, I've just retested this:
> > >
> > > [ 8.469152] mscc_felix 0000:00:00.5: OF node /soc/pcie@1f0000000/ethernet-switch@0,5/ports/port@4 of CPU port 4 lacks the required "phy-handle", "fixed-link" or "managed" properties
> > > [ 8.494571] mscc_felix 0000:00:00.5: error -EINVAL: Failed to register DSA switch
> > > [ 8.502151] mscc_felix: probe of 0000:00:00.5 failed with error -22
> >
> > ... which isn't listed in dsa_switches_apply_workarounds[], and
> > neither is mt753x. Thanks.
> >
> > So, that should be sufficient to know that the CPU port will always
> > properly described, and thus bypassing phylink in mt753x for the CPU
> > port should not be necessary.
>
> Perfect! If I understand correctly, there's this code - specific to MT7531
> and MT7988 ports being used as CPU ports - which runs in addition to what's
> in mt753x_phylink_mac_config():
>
> mt7530_write(priv, MT7530_PMCR_P(port),
> PMCR_CPU_PORT_SETTING(priv->id));
>
> This should be put on mt753x_phylink_mac_config(), under priv->id ==
> ID_MT7531, priv->id == ID_MT7988, and dsa_is_cpu_port(ds, port) checks?
Please remember that I have very little knowledge of MT753x, so in
order to answer this question, I've read through the mt7530 driver
code.
Looking at mt7530.h:
#define PMCR_CPU_PORT_SETTING(id) (PMCR_FORCE_MODE_ID((id)) | \
PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | \
PMCR_BACKOFF_EN | PMCR_BACKPR_EN | \
PMCR_TX_EN | PMCR_RX_EN | \
PMCR_TX_FC_EN | PMCR_RX_FC_EN | \
PMCR_FORCE_SPEED_1000 | \
PMCR_FORCE_FDX | PMCR_FORCE_LNK)
This seems to be some kind of port control register that sets amongst
other things parameters such as whether flow control is enabled, the
port speed, the duplex setting, whether link is forced up, etc.
Looking at what mt753x_phylink_mac_link_up() does:
1. it sets PMCR_RX_EN | PMCR_TX_EN | PMCR_FORCE_LNK.
2. it sets PMCR_FORCE_SPEED_1000 if speed was 1000Mbps, or if using
an internal, TRGMII, 1000base-X or 2500base-X phy interface mode.
3. it sets PMCR_FORCE_FDX if full duplex was requested.
4. it sets PMCR_TX_FC_EN if full duplex was requested with tx pause.
5. it sets PMCR_RX_FC_EN if full duplex was requested with rx pause.
So, provided this is called with the appropriate parameters, for a
fixed link, that will leave the following:
PMCR_FORCE_MODE_ID(id)
PMCR_IFG_XMIT(1)
PMCR_MAC_MODE
PMCR_BACKOFF_EN
PMCR_BACKPR_EN
If we now look at mt753x_phylink_mac_config(), this sets
PMCR_IFG_XMIT(1), PMCR_MAC_MODE, PMCR_BACKOFF_EN, PMCR_BACKPR_EN,
and PMCR_FORCE_MODE_ID(priv->id), which I believe is everything that
PMCR_CPU_PORT_SETTING(priv->id) is doing.
So, Wouldn't a fixed-link description indicating 1Gbps, full duplex
with pause cause phylink to call both mt753x_phylink_mac_config() and
mt753x_phylink_mac_link_up() with appropriate arguments to set all
of these parameters in PMCR?
Now, I'm going to analyse something else. mt7531_cpu_port_config()
is called from mt753x_cpu_port_enable(), which is itself called from
mt7531_setup_common(). That is ultimately called from the DSA switch
ops .setup() method.
This method is called from dsa_switch_setup() for each switch in the
DSA tree. dsa_tree_setup_switches() calls this, and is called from
dsa_tree_setup(). Once dsa_tree_setup_switches() finishes
successfully, dsa_tree_setup_ports() will be called. This will then
setup DSA and CPU ports, which will then setup a phylink instance
for these ports. phylink will parse the firmware description for
the port. DSA will then call dsa_port_enable().
dsa_port_enable() will then call any port_enable() method in the
mt7530.c driver, which will be mt7530_port_enable(). This then...
mt7530_clear(priv, MT7530_PMCR_P(port), PMCR_LINK_SETTINGS_MASK);
which is:
#define PMCR_LINK_SETTINGS_MASK (PMCR_TX_EN | PMCR_FORCE_SPEED_1000 | \
PMCR_RX_EN | PMCR_FORCE_SPEED_100 | \
PMCR_TX_FC_EN | PMCR_RX_FC_EN | \
PMCR_FORCE_FDX | PMCR_FORCE_LNK | \
PMCR_FORCE_EEE1G | PMCR_FORCE_EEE100)
So it wipes out all the PMCR settings that mt7531_cpu_port_config()
performed - undoing *everything* below that switch() statement in
mt7531_cpu_port_config()!
Once the port_enable() method returns, DSA will then call
phylink_start(), which will trigger phylink to bring up the link
according to the settings it has, which will mean phylink calls
the mac_config(), pcs_config(), pcs_link_up() and mac_link_up()
with the appropriate parameters for the firmware described link.
So I think I have the answer to my initial thought: do the calls in
mt7531_cpu_port_config() to the phylink methods have any use what so
ever? The answer is no, they are entirely useless. The same goes for
the other cpu_port_config() methods that do something similar. The
same goes for the PMCR register write that's changing any bits
included in PMCR_LINK_SETTINGS_MASK.
What that means is that mt7988_cpu_port_config() can be entirely
removed, it serves no useful purpose what so ever. For
mt7531_cpu_port_config(), it only needs to set priv->p[56]_interface
which, as far as I can see, probably only avoids mac_config() doing
any pad setup (that's a guess.)
At least that's what I gather from reading through the driver and
DSA code. It may be I've missed something, but currently, I think
that these cpu_port_config() functions aren't doing too much that
is actually useful work.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Sun, Jun 04, 2023 at 04:13:39PM +0100, Russell King (Oracle) wrote:
> On Sun, Jun 04, 2023 at 04:14:31PM +0300, Arınç ÜNAL wrote:
> > On 4.06.2023 16:07, Russell King (Oracle) wrote:
> > > On Sun, Jun 04, 2023 at 03:55:17PM +0300, Vladimir Oltean wrote:
> > > > On Sun, Jun 04, 2023 at 01:18:04PM +0100, Russell King (Oracle) wrote:
> > > > > I don't remember whether Vladimir's firmware validator will fail for
> > > > > mt753x if CPU ports are not fully described, but that would be well
> > > > > worth checking. If it does, then we can be confident that phylink
> > > > > will always be used, and those bypassing calls should not be necessary.
> > > >
> > > > It does, I've just retested this:
> > > >
> > > > [ 8.469152] mscc_felix 0000:00:00.5: OF node /soc/pcie@1f0000000/ethernet-switch@0,5/ports/port@4 of CPU port 4 lacks the required "phy-handle", "fixed-link" or "managed" properties
> > > > [ 8.494571] mscc_felix 0000:00:00.5: error -EINVAL: Failed to register DSA switch
> > > > [ 8.502151] mscc_felix: probe of 0000:00:00.5 failed with error -22
> > >
> > > ... which isn't listed in dsa_switches_apply_workarounds[], and
> > > neither is mt753x. Thanks.
> > >
> > > So, that should be sufficient to know that the CPU port will always
> > > properly described, and thus bypassing phylink in mt753x for the CPU
> > > port should not be necessary.
> >
> > Perfect! If I understand correctly, there's this code - specific to MT7531
> > and MT7988 ports being used as CPU ports - which runs in addition to what's
> > in mt753x_phylink_mac_config():
> >
> > mt7530_write(priv, MT7530_PMCR_P(port),
> > PMCR_CPU_PORT_SETTING(priv->id));
> >
> > This should be put on mt753x_phylink_mac_config(), under priv->id ==
> > ID_MT7531, priv->id == ID_MT7988, and dsa_is_cpu_port(ds, port) checks?
>
> Please remember that I have very little knowledge of MT753x, so in
> order to answer this question, I've read through the mt7530 driver
> code.
>
> Looking at mt7530.h:
>
> #define PMCR_CPU_PORT_SETTING(id) (PMCR_FORCE_MODE_ID((id)) | \
> PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | \
> PMCR_BACKOFF_EN | PMCR_BACKPR_EN | \
> PMCR_TX_EN | PMCR_RX_EN | \
> PMCR_TX_FC_EN | PMCR_RX_FC_EN | \
> PMCR_FORCE_SPEED_1000 | \
> PMCR_FORCE_FDX | PMCR_FORCE_LNK)
>
> This seems to be some kind of port control register that sets amongst
> other things parameters such as whether flow control is enabled, the
> port speed, the duplex setting, whether link is forced up, etc.
>
> Looking at what mt753x_phylink_mac_link_up() does:
>
> 1. it sets PMCR_RX_EN | PMCR_TX_EN | PMCR_FORCE_LNK.
> 2. it sets PMCR_FORCE_SPEED_1000 if speed was 1000Mbps, or if using
> an internal, TRGMII, 1000base-X or 2500base-X phy interface mode.
> 3. it sets PMCR_FORCE_FDX if full duplex was requested.
> 4. it sets PMCR_TX_FC_EN if full duplex was requested with tx pause.
> 5. it sets PMCR_RX_FC_EN if full duplex was requested with rx pause.
>
> So, provided this is called with the appropriate parameters, for a
> fixed link, that will leave the following:
>
> PMCR_FORCE_MODE_ID(id)
> PMCR_IFG_XMIT(1)
> PMCR_MAC_MODE
> PMCR_BACKOFF_EN
> PMCR_BACKPR_EN
>
> If we now look at mt753x_phylink_mac_config(), this sets
> PMCR_IFG_XMIT(1), PMCR_MAC_MODE, PMCR_BACKOFF_EN, PMCR_BACKPR_EN,
> and PMCR_FORCE_MODE_ID(priv->id), which I believe is everything that
> PMCR_CPU_PORT_SETTING(priv->id) is doing.
>
> So, Wouldn't a fixed-link description indicating 1Gbps, full duplex
> with pause cause phylink to call both mt753x_phylink_mac_config() and
> mt753x_phylink_mac_link_up() with appropriate arguments to set all
> of these parameters in PMCR?
>
> Now, I'm going to analyse something else. mt7531_cpu_port_config()
> is called from mt753x_cpu_port_enable(), which is itself called from
> mt7531_setup_common(). That is ultimately called from the DSA switch
> ops .setup() method.
>
> This method is called from dsa_switch_setup() for each switch in the
> DSA tree. dsa_tree_setup_switches() calls this, and is called from
> dsa_tree_setup(). Once dsa_tree_setup_switches() finishes
> successfully, dsa_tree_setup_ports() will be called. This will then
> setup DSA and CPU ports, which will then setup a phylink instance
> for these ports. phylink will parse the firmware description for
> the port. DSA will then call dsa_port_enable().
>
> dsa_port_enable() will then call any port_enable() method in the
> mt7530.c driver, which will be mt7530_port_enable(). This then...
>
> mt7530_clear(priv, MT7530_PMCR_P(port), PMCR_LINK_SETTINGS_MASK);
>
> which is:
>
> #define PMCR_LINK_SETTINGS_MASK (PMCR_TX_EN | PMCR_FORCE_SPEED_1000 | \
> PMCR_RX_EN | PMCR_FORCE_SPEED_100 | \
> PMCR_TX_FC_EN | PMCR_RX_FC_EN | \
> PMCR_FORCE_FDX | PMCR_FORCE_LNK | \
> PMCR_FORCE_EEE1G | PMCR_FORCE_EEE100)
>
> So it wipes out all the PMCR settings that mt7531_cpu_port_config()
> performed - undoing *everything* below that switch() statement in
> mt7531_cpu_port_config()!
>
> Once the port_enable() method returns, DSA will then call
> phylink_start(), which will trigger phylink to bring up the link
> according to the settings it has, which will mean phylink calls
> the mac_config(), pcs_config(), pcs_link_up() and mac_link_up()
> with the appropriate parameters for the firmware described link.
>
> So I think I have the answer to my initial thought: do the calls in
> mt7531_cpu_port_config() to the phylink methods have any use what so
> ever? The answer is no, they are entirely useless. The same goes for
> the other cpu_port_config() methods that do something similar. The
> same goes for the PMCR register write that's changing any bits
> included in PMCR_LINK_SETTINGS_MASK.
>
> What that means is that mt7988_cpu_port_config() can be entirely
> removed, it serves no useful purpose what so ever. For
> mt7531_cpu_port_config(), it only needs to set priv->p[56]_interface
> which, as far as I can see, probably only avoids mac_config() doing
> any pad setup (that's a guess.)
>
> At least that's what I gather from reading through the driver and
> DSA code. It may be I've missed something, but currently, I think
> that these cpu_port_config() functions aren't doing too much that
> is actually useful work.
Essentially, I think this change will have no effect at all on the
driver, because any effect this code has is totally undone when the
driver's port_enable() method is called:
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 9bc54e1348cb..447e63d74e0c 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2859,8 +2859,6 @@ mt7531_cpu_port_config(struct dsa_switch *ds, int port)
{
struct mt7530_priv *priv = ds->priv;
phy_interface_t interface;
- int speed;
- int ret;
switch (port) {
case 5:
@@ -2880,36 +2878,6 @@ mt7531_cpu_port_config(struct dsa_switch *ds, int port)
return -EINVAL;
}
- if (interface == PHY_INTERFACE_MODE_2500BASEX)
- speed = SPEED_2500;
- else
- speed = SPEED_1000;
-
- ret = mt7531_mac_config(ds, port, MLO_AN_FIXED, interface);
- if (ret)
- return ret;
- mt7530_write(priv, MT7530_PMCR_P(port),
- PMCR_CPU_PORT_SETTING(priv->id));
- mt753x_phylink_pcs_link_up(&priv->pcs[port].pcs, MLO_AN_FIXED,
- interface, speed, DUPLEX_FULL);
- mt753x_phylink_mac_link_up(ds, port, MLO_AN_FIXED, interface, NULL,
- speed, DUPLEX_FULL, true, true);
-
- return 0;
-}
-
-static int
-mt7988_cpu_port_config(struct dsa_switch *ds, int port)
-{
- struct mt7530_priv *priv = ds->priv;
-
- mt7530_write(priv, MT7530_PMCR_P(port),
- PMCR_CPU_PORT_SETTING(priv->id));
-
- mt753x_phylink_mac_link_up(ds, port, MLO_AN_FIXED,
- PHY_INTERFACE_MODE_INTERNAL, NULL,
- SPEED_10000, DUPLEX_FULL, true, true);
-
return 0;
}
@@ -3165,7 +3133,6 @@ const struct mt753x_info mt753x_table[] = {
.phy_read_c45 = mt7531_ind_c45_phy_read,
.phy_write_c45 = mt7531_ind_c45_phy_write,
.pad_setup = mt7988_pad_setup,
- .cpu_port_config = mt7988_cpu_port_config,
.mac_port_get_caps = mt7988_mac_port_get_caps,
.mac_port_config = mt7988_mac_config,
},
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On 4.06.2023 19:06, Russell King (Oracle) wrote:
> On Sun, Jun 04, 2023 at 05:00:11PM +0100, Russell King (Oracle) wrote:
>> On Sun, Jun 04, 2023 at 04:13:39PM +0100, Russell King (Oracle) wrote:
>>> On Sun, Jun 04, 2023 at 04:14:31PM +0300, Arınç ÜNAL wrote:
>>>> On 4.06.2023 16:07, Russell King (Oracle) wrote:
>>>>> On Sun, Jun 04, 2023 at 03:55:17PM +0300, Vladimir Oltean wrote:
>>>>>> On Sun, Jun 04, 2023 at 01:18:04PM +0100, Russell King (Oracle) wrote:
>>>>>>> I don't remember whether Vladimir's firmware validator will fail for
>>>>>>> mt753x if CPU ports are not fully described, but that would be well
>>>>>>> worth checking. If it does, then we can be confident that phylink
>>>>>>> will always be used, and those bypassing calls should not be necessary.
>>>>>>
>>>>>> It does, I've just retested this:
>>>>>>
>>>>>> [ 8.469152] mscc_felix 0000:00:00.5: OF node /soc/pcie@1f0000000/ethernet-switch@0,5/ports/port@4 of CPU port 4 lacks the required "phy-handle", "fixed-link" or "managed" properties
>>>>>> [ 8.494571] mscc_felix 0000:00:00.5: error -EINVAL: Failed to register DSA switch
>>>>>> [ 8.502151] mscc_felix: probe of 0000:00:00.5 failed with error -22
>>>>>
>>>>> ... which isn't listed in dsa_switches_apply_workarounds[], and
>>>>> neither is mt753x. Thanks.
>>>>>
>>>>> So, that should be sufficient to know that the CPU port will always
>>>>> properly described, and thus bypassing phylink in mt753x for the CPU
>>>>> port should not be necessary.
>>>>
>>>> Perfect! If I understand correctly, there's this code - specific to MT7531
>>>> and MT7988 ports being used as CPU ports - which runs in addition to what's
>>>> in mt753x_phylink_mac_config():
>>>>
>>>> mt7530_write(priv, MT7530_PMCR_P(port),
>>>> PMCR_CPU_PORT_SETTING(priv->id));
>>>>
>>>> This should be put on mt753x_phylink_mac_config(), under priv->id ==
>>>> ID_MT7531, priv->id == ID_MT7988, and dsa_is_cpu_port(ds, port) checks?
>>>
>>> Please remember that I have very little knowledge of MT753x, so in
>>> order to answer this question, I've read through the mt7530 driver
>>> code.
>>>
>>> Looking at mt7530.h:
>>>
>>> #define PMCR_CPU_PORT_SETTING(id) (PMCR_FORCE_MODE_ID((id)) | \
>>> PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | \
>>> PMCR_BACKOFF_EN | PMCR_BACKPR_EN | \
>>> PMCR_TX_EN | PMCR_RX_EN | \
>>> PMCR_TX_FC_EN | PMCR_RX_FC_EN | \
>>> PMCR_FORCE_SPEED_1000 | \
>>> PMCR_FORCE_FDX | PMCR_FORCE_LNK)
>>>
>>> This seems to be some kind of port control register that sets amongst
>>> other things parameters such as whether flow control is enabled, the
>>> port speed, the duplex setting, whether link is forced up, etc.
>>>
>>> Looking at what mt753x_phylink_mac_link_up() does:
>>>
>>> 1. it sets PMCR_RX_EN | PMCR_TX_EN | PMCR_FORCE_LNK.
>>> 2. it sets PMCR_FORCE_SPEED_1000 if speed was 1000Mbps, or if using
>>> an internal, TRGMII, 1000base-X or 2500base-X phy interface mode.
>>> 3. it sets PMCR_FORCE_FDX if full duplex was requested.
>>> 4. it sets PMCR_TX_FC_EN if full duplex was requested with tx pause.
>>> 5. it sets PMCR_RX_FC_EN if full duplex was requested with rx pause.
>>>
>>> So, provided this is called with the appropriate parameters, for a
>>> fixed link, that will leave the following:
>>>
>>> PMCR_FORCE_MODE_ID(id)
>>> PMCR_IFG_XMIT(1)
>>> PMCR_MAC_MODE
>>> PMCR_BACKOFF_EN
>>> PMCR_BACKPR_EN
>>>
>>> If we now look at mt753x_phylink_mac_config(), this sets
>>> PMCR_IFG_XMIT(1), PMCR_MAC_MODE, PMCR_BACKOFF_EN, PMCR_BACKPR_EN,
>>> and PMCR_FORCE_MODE_ID(priv->id), which I believe is everything that
>>> PMCR_CPU_PORT_SETTING(priv->id) is doing.
>>>
>>> So, Wouldn't a fixed-link description indicating 1Gbps, full duplex
>>> with pause cause phylink to call both mt753x_phylink_mac_config() and
>>> mt753x_phylink_mac_link_up() with appropriate arguments to set all
>>> of these parameters in PMCR?
>>>
>>> Now, I'm going to analyse something else. mt7531_cpu_port_config()
>>> is called from mt753x_cpu_port_enable(), which is itself called from
>>> mt7531_setup_common(). That is ultimately called from the DSA switch
>>> ops .setup() method.
>>>
>>> This method is called from dsa_switch_setup() for each switch in the
>>> DSA tree. dsa_tree_setup_switches() calls this, and is called from
>>> dsa_tree_setup(). Once dsa_tree_setup_switches() finishes
>>> successfully, dsa_tree_setup_ports() will be called. This will then
>>> setup DSA and CPU ports, which will then setup a phylink instance
>>> for these ports. phylink will parse the firmware description for
>>> the port. DSA will then call dsa_port_enable().
>>>
>>> dsa_port_enable() will then call any port_enable() method in the
>>> mt7530.c driver, which will be mt7530_port_enable(). This then...
>>>
>>> mt7530_clear(priv, MT7530_PMCR_P(port), PMCR_LINK_SETTINGS_MASK);
>>>
>>> which is:
>>>
>>> #define PMCR_LINK_SETTINGS_MASK (PMCR_TX_EN | PMCR_FORCE_SPEED_1000 | \
>>> PMCR_RX_EN | PMCR_FORCE_SPEED_100 | \
>>> PMCR_TX_FC_EN | PMCR_RX_FC_EN | \
>>> PMCR_FORCE_FDX | PMCR_FORCE_LNK | \
>>> PMCR_FORCE_EEE1G | PMCR_FORCE_EEE100)
>>>
>>> So it wipes out all the PMCR settings that mt7531_cpu_port_config()
>>> performed - undoing *everything* below that switch() statement in
>>> mt7531_cpu_port_config()!
>>>
>>> Once the port_enable() method returns, DSA will then call
>>> phylink_start(), which will trigger phylink to bring up the link
>>> according to the settings it has, which will mean phylink calls
>>> the mac_config(), pcs_config(), pcs_link_up() and mac_link_up()
>>> with the appropriate parameters for the firmware described link.
>>>
>>> So I think I have the answer to my initial thought: do the calls in
>>> mt7531_cpu_port_config() to the phylink methods have any use what so
>>> ever? The answer is no, they are entirely useless. The same goes for
>>> the other cpu_port_config() methods that do something similar. The
>>> same goes for the PMCR register write that's changing any bits
>>> included in PMCR_LINK_SETTINGS_MASK.
>>>
>>> What that means is that mt7988_cpu_port_config() can be entirely
>>> removed, it serves no useful purpose what so ever. For
>>> mt7531_cpu_port_config(), it only needs to set priv->p[56]_interface
>>> which, as far as I can see, probably only avoids mac_config() doing
>>> any pad setup (that's a guess.)
>>>
>>> At least that's what I gather from reading through the driver and
>>> DSA code. It may be I've missed something, but currently, I think
>>> that these cpu_port_config() functions aren't doing too much that
>>> is actually useful work.
>>
>> Essentially, I think this change will have no effect at all on the
>> driver, because any effect this code has is totally undone when the
>> driver's port_enable() method is called:
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index 9bc54e1348cb..447e63d74e0c 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -2859,8 +2859,6 @@ mt7531_cpu_port_config(struct dsa_switch *ds, int port)
>> {
>> struct mt7530_priv *priv = ds->priv;
>> phy_interface_t interface;
>> - int speed;
>> - int ret;
>>
>> switch (port) {
>> case 5:
>> @@ -2880,36 +2878,6 @@ mt7531_cpu_port_config(struct dsa_switch *ds, int port)
>> return -EINVAL;
>> }
>>
>> - if (interface == PHY_INTERFACE_MODE_2500BASEX)
>> - speed = SPEED_2500;
>> - else
>> - speed = SPEED_1000;
>> -
>> - ret = mt7531_mac_config(ds, port, MLO_AN_FIXED, interface);
>> - if (ret)
>> - return ret;
>> - mt7530_write(priv, MT7530_PMCR_P(port),
>> - PMCR_CPU_PORT_SETTING(priv->id));
>> - mt753x_phylink_pcs_link_up(&priv->pcs[port].pcs, MLO_AN_FIXED,
>> - interface, speed, DUPLEX_FULL);
>> - mt753x_phylink_mac_link_up(ds, port, MLO_AN_FIXED, interface, NULL,
>> - speed, DUPLEX_FULL, true, true);
>> -
>> - return 0;
>> -}
>> -
>> -static int
>> -mt7988_cpu_port_config(struct dsa_switch *ds, int port)
>> -{
>> - struct mt7530_priv *priv = ds->priv;
>> -
>> - mt7530_write(priv, MT7530_PMCR_P(port),
>> - PMCR_CPU_PORT_SETTING(priv->id));
>> -
>> - mt753x_phylink_mac_link_up(ds, port, MLO_AN_FIXED,
>> - PHY_INTERFACE_MODE_INTERNAL, NULL,
>> - SPEED_10000, DUPLEX_FULL, true, true);
>> -
>> return 0;
>> }
>>
>> @@ -3165,7 +3133,6 @@ const struct mt753x_info mt753x_table[] = {
>> .phy_read_c45 = mt7531_ind_c45_phy_read,
>> .phy_write_c45 = mt7531_ind_c45_phy_write,
>> .pad_setup = mt7988_pad_setup,
>> - .cpu_port_config = mt7988_cpu_port_config,
>> .mac_port_get_caps = mt7988_mac_port_get_caps,
>> .mac_port_config = mt7988_mac_config,
>> },
>
> ... and with that patch we can remove the definition of
> PMCR_CPU_PORT_SETTING() as well!
>
> There is one possibility why we may not be able to remove this code -
> whether there's something in this which requires the CPU port to be
> setup prior to something else. Only someone knowledgeable of the
> hardware, or who has the hardware in front and can test would be able
> to work that out.
I am on the same page with your explanation so far. I will test this out
on MT7531. Thanks a lot for looking at this!
Arınç
On Sun, Jun 04, 2023 at 05:00:11PM +0100, Russell King (Oracle) wrote:
> On Sun, Jun 04, 2023 at 04:13:39PM +0100, Russell King (Oracle) wrote:
> > On Sun, Jun 04, 2023 at 04:14:31PM +0300, Arınç ÜNAL wrote:
> > > On 4.06.2023 16:07, Russell King (Oracle) wrote:
> > > > On Sun, Jun 04, 2023 at 03:55:17PM +0300, Vladimir Oltean wrote:
> > > > > On Sun, Jun 04, 2023 at 01:18:04PM +0100, Russell King (Oracle) wrote:
> > > > > > I don't remember whether Vladimir's firmware validator will fail for
> > > > > > mt753x if CPU ports are not fully described, but that would be well
> > > > > > worth checking. If it does, then we can be confident that phylink
> > > > > > will always be used, and those bypassing calls should not be necessary.
> > > > >
> > > > > It does, I've just retested this:
> > > > >
> > > > > [ 8.469152] mscc_felix 0000:00:00.5: OF node /soc/pcie@1f0000000/ethernet-switch@0,5/ports/port@4 of CPU port 4 lacks the required "phy-handle", "fixed-link" or "managed" properties
> > > > > [ 8.494571] mscc_felix 0000:00:00.5: error -EINVAL: Failed to register DSA switch
> > > > > [ 8.502151] mscc_felix: probe of 0000:00:00.5 failed with error -22
> > > >
> > > > ... which isn't listed in dsa_switches_apply_workarounds[], and
> > > > neither is mt753x. Thanks.
> > > >
> > > > So, that should be sufficient to know that the CPU port will always
> > > > properly described, and thus bypassing phylink in mt753x for the CPU
> > > > port should not be necessary.
> > >
> > > Perfect! If I understand correctly, there's this code - specific to MT7531
> > > and MT7988 ports being used as CPU ports - which runs in addition to what's
> > > in mt753x_phylink_mac_config():
> > >
> > > mt7530_write(priv, MT7530_PMCR_P(port),
> > > PMCR_CPU_PORT_SETTING(priv->id));
> > >
> > > This should be put on mt753x_phylink_mac_config(), under priv->id ==
> > > ID_MT7531, priv->id == ID_MT7988, and dsa_is_cpu_port(ds, port) checks?
> >
> > Please remember that I have very little knowledge of MT753x, so in
> > order to answer this question, I've read through the mt7530 driver
> > code.
> >
> > Looking at mt7530.h:
> >
> > #define PMCR_CPU_PORT_SETTING(id) (PMCR_FORCE_MODE_ID((id)) | \
> > PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | \
> > PMCR_BACKOFF_EN | PMCR_BACKPR_EN | \
> > PMCR_TX_EN | PMCR_RX_EN | \
> > PMCR_TX_FC_EN | PMCR_RX_FC_EN | \
> > PMCR_FORCE_SPEED_1000 | \
> > PMCR_FORCE_FDX | PMCR_FORCE_LNK)
> >
> > This seems to be some kind of port control register that sets amongst
> > other things parameters such as whether flow control is enabled, the
> > port speed, the duplex setting, whether link is forced up, etc.
> >
> > Looking at what mt753x_phylink_mac_link_up() does:
> >
> > 1. it sets PMCR_RX_EN | PMCR_TX_EN | PMCR_FORCE_LNK.
> > 2. it sets PMCR_FORCE_SPEED_1000 if speed was 1000Mbps, or if using
> > an internal, TRGMII, 1000base-X or 2500base-X phy interface mode.
> > 3. it sets PMCR_FORCE_FDX if full duplex was requested.
> > 4. it sets PMCR_TX_FC_EN if full duplex was requested with tx pause.
> > 5. it sets PMCR_RX_FC_EN if full duplex was requested with rx pause.
> >
> > So, provided this is called with the appropriate parameters, for a
> > fixed link, that will leave the following:
> >
> > PMCR_FORCE_MODE_ID(id)
> > PMCR_IFG_XMIT(1)
> > PMCR_MAC_MODE
> > PMCR_BACKOFF_EN
> > PMCR_BACKPR_EN
> >
> > If we now look at mt753x_phylink_mac_config(), this sets
> > PMCR_IFG_XMIT(1), PMCR_MAC_MODE, PMCR_BACKOFF_EN, PMCR_BACKPR_EN,
> > and PMCR_FORCE_MODE_ID(priv->id), which I believe is everything that
> > PMCR_CPU_PORT_SETTING(priv->id) is doing.
> >
> > So, Wouldn't a fixed-link description indicating 1Gbps, full duplex
> > with pause cause phylink to call both mt753x_phylink_mac_config() and
> > mt753x_phylink_mac_link_up() with appropriate arguments to set all
> > of these parameters in PMCR?
> >
> > Now, I'm going to analyse something else. mt7531_cpu_port_config()
> > is called from mt753x_cpu_port_enable(), which is itself called from
> > mt7531_setup_common(). That is ultimately called from the DSA switch
> > ops .setup() method.
> >
> > This method is called from dsa_switch_setup() for each switch in the
> > DSA tree. dsa_tree_setup_switches() calls this, and is called from
> > dsa_tree_setup(). Once dsa_tree_setup_switches() finishes
> > successfully, dsa_tree_setup_ports() will be called. This will then
> > setup DSA and CPU ports, which will then setup a phylink instance
> > for these ports. phylink will parse the firmware description for
> > the port. DSA will then call dsa_port_enable().
> >
> > dsa_port_enable() will then call any port_enable() method in the
> > mt7530.c driver, which will be mt7530_port_enable(). This then...
> >
> > mt7530_clear(priv, MT7530_PMCR_P(port), PMCR_LINK_SETTINGS_MASK);
> >
> > which is:
> >
> > #define PMCR_LINK_SETTINGS_MASK (PMCR_TX_EN | PMCR_FORCE_SPEED_1000 | \
> > PMCR_RX_EN | PMCR_FORCE_SPEED_100 | \
> > PMCR_TX_FC_EN | PMCR_RX_FC_EN | \
> > PMCR_FORCE_FDX | PMCR_FORCE_LNK | \
> > PMCR_FORCE_EEE1G | PMCR_FORCE_EEE100)
> >
> > So it wipes out all the PMCR settings that mt7531_cpu_port_config()
> > performed - undoing *everything* below that switch() statement in
> > mt7531_cpu_port_config()!
> >
> > Once the port_enable() method returns, DSA will then call
> > phylink_start(), which will trigger phylink to bring up the link
> > according to the settings it has, which will mean phylink calls
> > the mac_config(), pcs_config(), pcs_link_up() and mac_link_up()
> > with the appropriate parameters for the firmware described link.
> >
> > So I think I have the answer to my initial thought: do the calls in
> > mt7531_cpu_port_config() to the phylink methods have any use what so
> > ever? The answer is no, they are entirely useless. The same goes for
> > the other cpu_port_config() methods that do something similar. The
> > same goes for the PMCR register write that's changing any bits
> > included in PMCR_LINK_SETTINGS_MASK.
> >
> > What that means is that mt7988_cpu_port_config() can be entirely
> > removed, it serves no useful purpose what so ever. For
> > mt7531_cpu_port_config(), it only needs to set priv->p[56]_interface
> > which, as far as I can see, probably only avoids mac_config() doing
> > any pad setup (that's a guess.)
> >
> > At least that's what I gather from reading through the driver and
> > DSA code. It may be I've missed something, but currently, I think
> > that these cpu_port_config() functions aren't doing too much that
> > is actually useful work.
>
> Essentially, I think this change will have no effect at all on the
> driver, because any effect this code has is totally undone when the
> driver's port_enable() method is called:
>
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 9bc54e1348cb..447e63d74e0c 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2859,8 +2859,6 @@ mt7531_cpu_port_config(struct dsa_switch *ds, int port)
> {
> struct mt7530_priv *priv = ds->priv;
> phy_interface_t interface;
> - int speed;
> - int ret;
>
> switch (port) {
> case 5:
> @@ -2880,36 +2878,6 @@ mt7531_cpu_port_config(struct dsa_switch *ds, int port)
> return -EINVAL;
> }
>
> - if (interface == PHY_INTERFACE_MODE_2500BASEX)
> - speed = SPEED_2500;
> - else
> - speed = SPEED_1000;
> -
> - ret = mt7531_mac_config(ds, port, MLO_AN_FIXED, interface);
> - if (ret)
> - return ret;
> - mt7530_write(priv, MT7530_PMCR_P(port),
> - PMCR_CPU_PORT_SETTING(priv->id));
> - mt753x_phylink_pcs_link_up(&priv->pcs[port].pcs, MLO_AN_FIXED,
> - interface, speed, DUPLEX_FULL);
> - mt753x_phylink_mac_link_up(ds, port, MLO_AN_FIXED, interface, NULL,
> - speed, DUPLEX_FULL, true, true);
> -
> - return 0;
> -}
> -
> -static int
> -mt7988_cpu_port_config(struct dsa_switch *ds, int port)
> -{
> - struct mt7530_priv *priv = ds->priv;
> -
> - mt7530_write(priv, MT7530_PMCR_P(port),
> - PMCR_CPU_PORT_SETTING(priv->id));
> -
> - mt753x_phylink_mac_link_up(ds, port, MLO_AN_FIXED,
> - PHY_INTERFACE_MODE_INTERNAL, NULL,
> - SPEED_10000, DUPLEX_FULL, true, true);
> -
> return 0;
> }
>
> @@ -3165,7 +3133,6 @@ const struct mt753x_info mt753x_table[] = {
> .phy_read_c45 = mt7531_ind_c45_phy_read,
> .phy_write_c45 = mt7531_ind_c45_phy_write,
> .pad_setup = mt7988_pad_setup,
> - .cpu_port_config = mt7988_cpu_port_config,
> .mac_port_get_caps = mt7988_mac_port_get_caps,
> .mac_port_config = mt7988_mac_config,
> },
... and with that patch we can remove the definition of
PMCR_CPU_PORT_SETTING() as well!
There is one possibility why we may not be able to remove this code -
whether there's something in this which requires the CPU port to be
setup prior to something else. Only someone knowledgeable of the
hardware, or who has the hardware in front and can test would be able
to work that out.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On 4.06.2023 18:13, Russell King (Oracle) wrote:
> On Sun, Jun 04, 2023 at 04:14:31PM +0300, Arınç ÜNAL wrote:
>> On 4.06.2023 16:07, Russell King (Oracle) wrote:
>>> On Sun, Jun 04, 2023 at 03:55:17PM +0300, Vladimir Oltean wrote:
>>>> On Sun, Jun 04, 2023 at 01:18:04PM +0100, Russell King (Oracle) wrote:
>>>>> I don't remember whether Vladimir's firmware validator will fail for
>>>>> mt753x if CPU ports are not fully described, but that would be well
>>>>> worth checking. If it does, then we can be confident that phylink
>>>>> will always be used, and those bypassing calls should not be necessary.
>>>>
>>>> It does, I've just retested this:
>>>>
>>>> [ 8.469152] mscc_felix 0000:00:00.5: OF node /soc/pcie@1f0000000/ethernet-switch@0,5/ports/port@4 of CPU port 4 lacks the required "phy-handle", "fixed-link" or "managed" properties
>>>> [ 8.494571] mscc_felix 0000:00:00.5: error -EINVAL: Failed to register DSA switch
>>>> [ 8.502151] mscc_felix: probe of 0000:00:00.5 failed with error -22
>>>
>>> ... which isn't listed in dsa_switches_apply_workarounds[], and
>>> neither is mt753x. Thanks.
>>>
>>> So, that should be sufficient to know that the CPU port will always
>>> properly described, and thus bypassing phylink in mt753x for the CPU
>>> port should not be necessary.
>>
>> Perfect! If I understand correctly, there's this code - specific to MT7531
>> and MT7988 ports being used as CPU ports - which runs in addition to what's
>> in mt753x_phylink_mac_config():
>>
>> mt7530_write(priv, MT7530_PMCR_P(port),
>> PMCR_CPU_PORT_SETTING(priv->id));
>>
>> This should be put on mt753x_phylink_mac_config(), under priv->id ==
>> ID_MT7531, priv->id == ID_MT7988, and dsa_is_cpu_port(ds, port) checks?
>
> Please remember that I have very little knowledge of MT753x, so in
> order to answer this question, I've read through the mt7530 driver
> code.
>
> Looking at mt7530.h:
>
> #define PMCR_CPU_PORT_SETTING(id) (PMCR_FORCE_MODE_ID((id)) | \
> PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | \
> PMCR_BACKOFF_EN | PMCR_BACKPR_EN | \
> PMCR_TX_EN | PMCR_RX_EN | \
> PMCR_TX_FC_EN | PMCR_RX_FC_EN | \
> PMCR_FORCE_SPEED_1000 | \
> PMCR_FORCE_FDX | PMCR_FORCE_LNK)
>
> This seems to be some kind of port control register that sets amongst
> other things parameters such as whether flow control is enabled, the
> port speed, the duplex setting, whether link is forced up, etc.
>
> Looking at what mt753x_phylink_mac_link_up() does:
>
> 1. it sets PMCR_RX_EN | PMCR_TX_EN | PMCR_FORCE_LNK.
> 2. it sets PMCR_FORCE_SPEED_1000 if speed was 1000Mbps, or if using
> an internal, TRGMII, 1000base-X or 2500base-X phy interface mode.
> 3. it sets PMCR_FORCE_FDX if full duplex was requested.
> 4. it sets PMCR_TX_FC_EN if full duplex was requested with tx pause.
> 5. it sets PMCR_RX_FC_EN if full duplex was requested with rx pause.
>
> So, provided this is called with the appropriate parameters, for a
> fixed link, that will leave the following:
>
> PMCR_FORCE_MODE_ID(id)
> PMCR_IFG_XMIT(1)
> PMCR_MAC_MODE
> PMCR_BACKOFF_EN
> PMCR_BACKPR_EN
>
> If we now look at mt753x_phylink_mac_config(), this sets
> PMCR_IFG_XMIT(1), PMCR_MAC_MODE, PMCR_BACKOFF_EN, PMCR_BACKPR_EN,
> and PMCR_FORCE_MODE_ID(priv->id), which I believe is everything that
> PMCR_CPU_PORT_SETTING(priv->id) is doing.
>
> So, Wouldn't a fixed-link description indicating 1Gbps, full duplex
> with pause cause phylink to call both mt753x_phylink_mac_config() and
> mt753x_phylink_mac_link_up() with appropriate arguments to set all
> of these parameters in PMCR?
>
> Now, I'm going to analyse something else. mt7531_cpu_port_config()
> is called from mt753x_cpu_port_enable(), which is itself called from
> mt7531_setup_common(). That is ultimately called from the DSA switch
> ops .setup() method.
>
> This method is called from dsa_switch_setup() for each switch in the
> DSA tree. dsa_tree_setup_switches() calls this, and is called from
> dsa_tree_setup(). Once dsa_tree_setup_switches() finishes
> successfully, dsa_tree_setup_ports() will be called. This will then
> setup DSA and CPU ports, which will then setup a phylink instance
> for these ports. phylink will parse the firmware description for
> the port. DSA will then call dsa_port_enable().
>
> dsa_port_enable() will then call any port_enable() method in the
> mt7530.c driver, which will be mt7530_port_enable(). This then...
>
> mt7530_clear(priv, MT7530_PMCR_P(port), PMCR_LINK_SETTINGS_MASK);
>
> which is:
>
> #define PMCR_LINK_SETTINGS_MASK (PMCR_TX_EN | PMCR_FORCE_SPEED_1000 | \
> PMCR_RX_EN | PMCR_FORCE_SPEED_100 | \
> PMCR_TX_FC_EN | PMCR_RX_FC_EN | \
> PMCR_FORCE_FDX | PMCR_FORCE_LNK | \
> PMCR_FORCE_EEE1G | PMCR_FORCE_EEE100)
>
> So it wipes out all the PMCR settings that mt7531_cpu_port_config()
> performed - undoing *everything* below that switch() statement in
> mt7531_cpu_port_config()!
>
> Once the port_enable() method returns, DSA will then call
> phylink_start(), which will trigger phylink to bring up the link
> according to the settings it has, which will mean phylink calls
> the mac_config(), pcs_config(), pcs_link_up() and mac_link_up()
> with the appropriate parameters for the firmware described link.
I'm slowly learning how DSA and phylink works, this is the full code
path I could make up for the MT7530 DSA subdriver:
mt7530_probe() & mt7988_probe()
-> mt7530_probe_common()
-> dsa_register_switch()
-> dsa_switch_probe()
-> dsa_tree_setup()
-> dsa_tree_setup_switches()
-> dsa_switch_setup()
-> ds->ops->setup(): mt753x_setup()
-> mt7530_setup()
-> mt753x_cpu_port_enable()
-> mt7531_setup()
-> mt7531_setup_common()
-> mt753x_cpu_port_enable()
-> priv->info->cpu_port_config():
mt7531_cpu_port_config()
-> mt7988_setup()
-> mt7531_setup_common()
-> mt753x_cpu_port_enable()
-> priv->info->cpu_port_config():
mt7988_cpu_port_config()
-> dsa_tree_setup_ports()
-> dsa_port_setup()
-> dsa_shared_port_link_register_of()
-> dsa_shared_port_link_register_of()
-> dsa_shared_port_phylink_register()
-> dsa_port_phylink_create()
-> ds->ops->phylink_get_caps():
mt753x_phylink_get_caps()
-> phylink_create()
-> INIT_WORK(&pl->resolve, phylink_resolve)
-> dsa_port_enable()
-> dsa_port_enable_rt()
-> ds->ops->port_enable():
mt7530_port_enable()
-> phylink_start()
-> phylink_mac_initial_config()
-> phylink_major_config()
-> phylink_mac_config()
-> pl->mac_ops->mac_config():
dsa_port_phylink_mac_config()
-> ds->ops->phylink_mac_config():
mt753x_phylink_mac_config()
-> pl->pcs->ops->pcs_config():
mt753x_pcs_config()
-> phylink_enable_and_run_resolve()
-> phylink_run_resolve()
-> queue_work(system_power_efficient_wq, &pl->resolve)
-> phylink_link_up()
-> pl->pcs->ops->pcs_link_up():
mtk_pcs_lynxi_link_up()
-> pl->mac_ops->mac_link_up():
dsa_port_phylink_mac_link_up()
-> ds->ops->phylink_mac_link_up():
mt753x_phylink_mac_link_up()
>
> So I think I have the answer to my initial thought: do the calls in
> mt7531_cpu_port_config() to the phylink methods have any use what so
> ever? The answer is no, they are entirely useless. The same goes for
> the other cpu_port_config() methods that do something similar. The
> same goes for the PMCR register write that's changing any bits
> included in PMCR_LINK_SETTINGS_MASK.
>
> What that means is that mt7988_cpu_port_config() can be entirely
> removed, it serves no useful purpose what so ever. For
> mt7531_cpu_port_config(), it only needs to set priv->p[56]_interface
> which, as far as I can see, probably only avoids mac_config() doing
> any pad setup (that's a guess.)
This is what I also believe and the reason why I made this patch to
simplify it. Looks like I'll just remove priv->info->cpu_port_config()
instead.
Arınç
On 4.06.2023 15:47, Vladimir Oltean wrote:
> On Sun, Jun 04, 2023 at 12:39:33PM +0300, Arınç ÜNAL wrote:
>> On 4.06.2023 12:23, Vladimir Oltean wrote:
>>> On Sun, Jun 04, 2023 at 11:51:33AM +0300, Arınç ÜNAL wrote:
>>>>> If the switch doesn't currently trap BPDUs, isn't STP broken?
>>>>
>>>> No, the BPDU_PORT_FW bits are 0 after reset. The MT7620 programming guide
>>>> states that frames with 01:80:C2:00:00:00 MAC DA (which is how the BPDU
>>>> distinction is being made) will follow the system default which means the
>>>> BPDUs will be treated as normal multicast frames.
>>>>
>>>> Only if all 3 bits are set will the BPDUs be dropped.
>>>
>>> Right, if you don't trap BPDUs just to the CPU but flood them, I believe
>>> the STP protocol won't behave properly with switching loops. Worth testing.
>>
>> I've got no interest spending time playing around with STP at the moment so
>> I'm going to pass.
>
> You can at the very least move it towards the beginning of the net-next patch
> set, so that we can be sure it doesn't depend on the other refactoring work,
> in case someone in the future makes a request for the patch to be backported
> to stable.
Maybe I should submit this and LLDP trapping to net? Currently, the
MT7530 subdriver on the stable kernels treat LLDP frames and BPDUs as
regular multicast frames, therefore flooding them to user ports, which
is wrong. These patches could count as bug fixes.
Arınç
On 4.06.2023 19:14, Arınç ÜNAL wrote:
> On 4.06.2023 19:06, Russell King (Oracle) wrote:
>> On Sun, Jun 04, 2023 at 05:00:11PM +0100, Russell King (Oracle) wrote:
>>> On Sun, Jun 04, 2023 at 04:13:39PM +0100, Russell King (Oracle) wrote:
>>>> On Sun, Jun 04, 2023 at 04:14:31PM +0300, Arınç ÜNAL wrote:
>>>>> On 4.06.2023 16:07, Russell King (Oracle) wrote:
>>>>>> On Sun, Jun 04, 2023 at 03:55:17PM +0300, Vladimir Oltean wrote:
>>>>>>> On Sun, Jun 04, 2023 at 01:18:04PM +0100, Russell King (Oracle)
>>>>>>> wrote:
>>>>>>>> I don't remember whether Vladimir's firmware validator will fail
>>>>>>>> for
>>>>>>>> mt753x if CPU ports are not fully described, but that would be well
>>>>>>>> worth checking. If it does, then we can be confident that phylink
>>>>>>>> will always be used, and those bypassing calls should not be
>>>>>>>> necessary.
>>>>>>>
>>>>>>> It does, I've just retested this:
>>>>>>>
>>>>>>> [ 8.469152] mscc_felix 0000:00:00.5: OF node
>>>>>>> /soc/pcie@1f0000000/ethernet-switch@0,5/ports/port@4 of CPU port
>>>>>>> 4 lacks the required "phy-handle", "fixed-link" or "managed"
>>>>>>> properties
>>>>>>> [ 8.494571] mscc_felix 0000:00:00.5: error -EINVAL: Failed to
>>>>>>> register DSA switch
>>>>>>> [ 8.502151] mscc_felix: probe of 0000:00:00.5 failed with
>>>>>>> error -22
>>>>>>
>>>>>> ... which isn't listed in dsa_switches_apply_workarounds[], and
>>>>>> neither is mt753x. Thanks.
>>>>>>
>>>>>> So, that should be sufficient to know that the CPU port will always
>>>>>> properly described, and thus bypassing phylink in mt753x for the CPU
>>>>>> port should not be necessary.
>>>>>
>>>>> Perfect! If I understand correctly, there's this code - specific to
>>>>> MT7531
>>>>> and MT7988 ports being used as CPU ports - which runs in addition
>>>>> to what's
>>>>> in mt753x_phylink_mac_config():
>>>>>
>>>>> mt7530_write(priv, MT7530_PMCR_P(port),
>>>>> PMCR_CPU_PORT_SETTING(priv->id));
>>>>>
>>>>> This should be put on mt753x_phylink_mac_config(), under priv->id ==
>>>>> ID_MT7531, priv->id == ID_MT7988, and dsa_is_cpu_port(ds, port)
>>>>> checks?
>>>>
>>>> Please remember that I have very little knowledge of MT753x, so in
>>>> order to answer this question, I've read through the mt7530 driver
>>>> code.
>>>>
>>>> Looking at mt7530.h:
>>>>
>>>> #define PMCR_CPU_PORT_SETTING(id) (PMCR_FORCE_MODE_ID((id)) | \
>>>> PMCR_IFG_XMIT(1) |
>>>> PMCR_MAC_MODE | \
>>>> PMCR_BACKOFF_EN |
>>>> PMCR_BACKPR_EN | \
>>>> PMCR_TX_EN | PMCR_RX_EN | \
>>>> PMCR_TX_FC_EN |
>>>> PMCR_RX_FC_EN | \
>>>> PMCR_FORCE_SPEED_1000 | \
>>>> PMCR_FORCE_FDX |
>>>> PMCR_FORCE_LNK)
>>>>
>>>> This seems to be some kind of port control register that sets amongst
>>>> other things parameters such as whether flow control is enabled, the
>>>> port speed, the duplex setting, whether link is forced up, etc.
>>>>
>>>> Looking at what mt753x_phylink_mac_link_up() does:
>>>>
>>>> 1. it sets PMCR_RX_EN | PMCR_TX_EN | PMCR_FORCE_LNK.
>>>> 2. it sets PMCR_FORCE_SPEED_1000 if speed was 1000Mbps, or if using
>>>> an internal, TRGMII, 1000base-X or 2500base-X phy interface mode.
>>>> 3. it sets PMCR_FORCE_FDX if full duplex was requested.
>>>> 4. it sets PMCR_TX_FC_EN if full duplex was requested with tx pause.
>>>> 5. it sets PMCR_RX_FC_EN if full duplex was requested with rx pause.
>>>>
>>>> So, provided this is called with the appropriate parameters, for a
>>>> fixed link, that will leave the following:
>>>>
>>>> PMCR_FORCE_MODE_ID(id)
>>>> PMCR_IFG_XMIT(1)
>>>> PMCR_MAC_MODE
>>>> PMCR_BACKOFF_EN
>>>> PMCR_BACKPR_EN
>>>>
>>>> If we now look at mt753x_phylink_mac_config(), this sets
>>>> PMCR_IFG_XMIT(1), PMCR_MAC_MODE, PMCR_BACKOFF_EN, PMCR_BACKPR_EN,
>>>> and PMCR_FORCE_MODE_ID(priv->id), which I believe is everything that
>>>> PMCR_CPU_PORT_SETTING(priv->id) is doing.
>>>>
>>>> So, Wouldn't a fixed-link description indicating 1Gbps, full duplex
>>>> with pause cause phylink to call both mt753x_phylink_mac_config() and
>>>> mt753x_phylink_mac_link_up() with appropriate arguments to set all
>>>> of these parameters in PMCR?
>>>>
>>>> Now, I'm going to analyse something else. mt7531_cpu_port_config()
>>>> is called from mt753x_cpu_port_enable(), which is itself called from
>>>> mt7531_setup_common(). That is ultimately called from the DSA switch
>>>> ops .setup() method.
>>>>
>>>> This method is called from dsa_switch_setup() for each switch in the
>>>> DSA tree. dsa_tree_setup_switches() calls this, and is called from
>>>> dsa_tree_setup(). Once dsa_tree_setup_switches() finishes
>>>> successfully, dsa_tree_setup_ports() will be called. This will then
>>>> setup DSA and CPU ports, which will then setup a phylink instance
>>>> for these ports. phylink will parse the firmware description for
>>>> the port. DSA will then call dsa_port_enable().
>>>>
>>>> dsa_port_enable() will then call any port_enable() method in the
>>>> mt7530.c driver, which will be mt7530_port_enable(). This then...
>>>>
>>>> mt7530_clear(priv, MT7530_PMCR_P(port),
>>>> PMCR_LINK_SETTINGS_MASK);
>>>>
>>>> which is:
>>>>
>>>> #define PMCR_LINK_SETTINGS_MASK (PMCR_TX_EN |
>>>> PMCR_FORCE_SPEED_1000 | \
>>>> PMCR_RX_EN |
>>>> PMCR_FORCE_SPEED_100 | \
>>>> PMCR_TX_FC_EN |
>>>> PMCR_RX_FC_EN | \
>>>> PMCR_FORCE_FDX |
>>>> PMCR_FORCE_LNK | \
>>>> PMCR_FORCE_EEE1G |
>>>> PMCR_FORCE_EEE100)
>>>>
>>>> So it wipes out all the PMCR settings that mt7531_cpu_port_config()
>>>> performed - undoing *everything* below that switch() statement in
>>>> mt7531_cpu_port_config()!
>>>>
>>>> Once the port_enable() method returns, DSA will then call
>>>> phylink_start(), which will trigger phylink to bring up the link
>>>> according to the settings it has, which will mean phylink calls
>>>> the mac_config(), pcs_config(), pcs_link_up() and mac_link_up()
>>>> with the appropriate parameters for the firmware described link.
>>>>
>>>> So I think I have the answer to my initial thought: do the calls in
>>>> mt7531_cpu_port_config() to the phylink methods have any use what so
>>>> ever? The answer is no, they are entirely useless. The same goes for
>>>> the other cpu_port_config() methods that do something similar. The
>>>> same goes for the PMCR register write that's changing any bits
>>>> included in PMCR_LINK_SETTINGS_MASK.
>>>>
>>>> What that means is that mt7988_cpu_port_config() can be entirely
>>>> removed, it serves no useful purpose what so ever. For
>>>> mt7531_cpu_port_config(), it only needs to set priv->p[56]_interface
>>>> which, as far as I can see, probably only avoids mac_config() doing
>>>> any pad setup (that's a guess.)
>>>>
>>>> At least that's what I gather from reading through the driver and
>>>> DSA code. It may be I've missed something, but currently, I think
>>>> that these cpu_port_config() functions aren't doing too much that
>>>> is actually useful work.
>>>
>>> Essentially, I think this change will have no effect at all on the
>>> driver, because any effect this code has is totally undone when the
>>> driver's port_enable() method is called:
>>>
>>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>>> index 9bc54e1348cb..447e63d74e0c 100644
>>> --- a/drivers/net/dsa/mt7530.c
>>> +++ b/drivers/net/dsa/mt7530.c
>>> @@ -2859,8 +2859,6 @@ mt7531_cpu_port_config(struct dsa_switch *ds,
>>> int port)
>>> {
>>> struct mt7530_priv *priv = ds->priv;
>>> phy_interface_t interface;
>>> - int speed;
>>> - int ret;
>>> switch (port) {
>>> case 5:
>>> @@ -2880,36 +2878,6 @@ mt7531_cpu_port_config(struct dsa_switch *ds,
>>> int port)
>>> return -EINVAL;
>>> }
>>> - if (interface == PHY_INTERFACE_MODE_2500BASEX)
>>> - speed = SPEED_2500;
>>> - else
>>> - speed = SPEED_1000;
>>> -
>>> - ret = mt7531_mac_config(ds, port, MLO_AN_FIXED, interface);
>>> - if (ret)
>>> - return ret;
>>> - mt7530_write(priv, MT7530_PMCR_P(port),
>>> - PMCR_CPU_PORT_SETTING(priv->id));
>>> - mt753x_phylink_pcs_link_up(&priv->pcs[port].pcs, MLO_AN_FIXED,
>>> - interface, speed, DUPLEX_FULL);
>>> - mt753x_phylink_mac_link_up(ds, port, MLO_AN_FIXED, interface, NULL,
>>> - speed, DUPLEX_FULL, true, true);
>>> -
>>> - return 0;
>>> -}
>>> -
>>> -static int
>>> -mt7988_cpu_port_config(struct dsa_switch *ds, int port)
>>> -{
>>> - struct mt7530_priv *priv = ds->priv;
>>> -
>>> - mt7530_write(priv, MT7530_PMCR_P(port),
>>> - PMCR_CPU_PORT_SETTING(priv->id));
>>> -
>>> - mt753x_phylink_mac_link_up(ds, port, MLO_AN_FIXED,
>>> - PHY_INTERFACE_MODE_INTERNAL, NULL,
>>> - SPEED_10000, DUPLEX_FULL, true, true);
>>> -
>>> return 0;
>>> }
>>> @@ -3165,7 +3133,6 @@ const struct mt753x_info mt753x_table[] = {
>>> .phy_read_c45 = mt7531_ind_c45_phy_read,
>>> .phy_write_c45 = mt7531_ind_c45_phy_write,
>>> .pad_setup = mt7988_pad_setup,
>>> - .cpu_port_config = mt7988_cpu_port_config,
>>> .mac_port_get_caps = mt7988_mac_port_get_caps,
>>> .mac_port_config = mt7988_mac_config,
>>> },
>>
>> ... and with that patch we can remove the definition of
>> PMCR_CPU_PORT_SETTING() as well!
>>
>> There is one possibility why we may not be able to remove this code -
>> whether there's something in this which requires the CPU port to be
>> setup prior to something else. Only someone knowledgeable of the
>> hardware, or who has the hardware in front and can test would be able
>> to work that out.
>
> I am on the same page with your explanation so far. I will test this out
> on MT7531. Thanks a lot for looking at this!
I was able to confirm all user ports of the MT7531BE switch
transmit/receive traffic to/from the SGMII CPU port and computer fine
after getting rid of priv->info->cpu_port_config().
Tried all user ports being affine to the RGMII CPU port, that works too.
https://github.com/arinc9/linux/commit/4e79313a95d45950cab526456ef0030286ba4d4e
Arınç
On Sat, Jun 10, 2023 at 01:57:27PM +0300, Arınç ÜNAL wrote:
> I was able to confirm all user ports of the MT7531BE switch transmit/receive
> traffic to/from the SGMII CPU port and computer fine after getting rid of
> priv->info->cpu_port_config().
>
> Tried all user ports being affine to the RGMII CPU port, that works too.
>
> https://github.com/arinc9/linux/commit/4e79313a95d45950cab526456ef0030286ba4d4e
Did you do black-box testing after removing the code, or were you
also able to independently confirm that the configurations done by
cpu_port_config() were later overwritten? I'm trying to disambiguate
between "works by coincidence" and "works because the analysis was
correct".
On Sat, Jun 10, 2023 at 11:32:45AM +0300, Arınç ÜNAL wrote:
> Maybe I should submit this and LLDP trapping to net? Currently, the MT7530
> subdriver on the stable kernels treat LLDP frames and BPDUs as regular
> multicast frames, therefore flooding them to user ports, which is wrong.
> These patches could count as bug fixes.
Yes, I believe that trapping link-local frames only to the host
constitutes "net.git" material.
On 10.06.2023 20:55, Vladimir Oltean wrote:
> On Sat, Jun 10, 2023 at 01:57:27PM +0300, Arınç ÜNAL wrote:
>> I was able to confirm all user ports of the MT7531BE switch transmit/receive
>> traffic to/from the SGMII CPU port and computer fine after getting rid of
>> priv->info->cpu_port_config().
>>
>> Tried all user ports being affine to the RGMII CPU port, that works too.
>>
>> https://github.com/arinc9/linux/commit/4e79313a95d45950cab526456ef0030286ba4d4e
>
> Did you do black-box testing after removing the code, or were you
> also able to independently confirm that the configurations done by
> cpu_port_config() were later overwritten? I'm trying to disambiguate
> between "works by coincidence" and "works because the analysis was
> correct".
I did my testing, merely to make sure we didn't miss anything as Russell
already stated that the configuration from cpu_port_config() is later
overwritten.
I could put some dev_info around to confirm the code path that
overwrites the configuration.
Arınç
On 11.06.2023 10:23, Arınç ÜNAL wrote:
>
> On 10.06.2023 20:55, Vladimir Oltean wrote:
>> On Sat, Jun 10, 2023 at 01:57:27PM +0300, Arınç ÜNAL wrote:
>>> I was able to confirm all user ports of the MT7531BE switch transmit/receive
>>> traffic to/from the SGMII CPU port and computer fine after getting rid of
>>> priv->info->cpu_port_config().
>>>
>>> Tried all user ports being affine to the RGMII CPU port, that works too.
>>>
>>> https://github.com/arinc9/linux/commit/4e79313a95d45950cab526456ef0030286ba4d4e
>>
>> Did you do black-box testing after removing the code, or were you
>> also able to independently confirm that the configurations done by
>> cpu_port_config() were later overwritten? I'm trying to disambiguate
>> between "works by coincidence" and "works because the analysis was
>> correct".
>
> I did my testing, merely to make sure we didn't miss anything as Russell already stated that the configuration from cpu_port_config() is later overwritten.
>
> I could put some dev_info around to confirm the code path that overwrites the configuration.
I have finally tested this.
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index a4468468b53c..7b60a67d016a 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -968,9 +968,11 @@ mt753x_cpu_port_enable(struct dsa_switch *ds, int port)
/* Setup max capability of CPU port at first */
if (priv->info->cpu_port_config) {
+ dev_info(priv->dev, "running cpu_port_config()\n");
ret = priv->info->cpu_port_config(ds, port);
if (ret)
return ret;
+ dev_info(priv->dev, "cpu_port_config() ran\n");
}
/* Enable Mediatek header mode on the cpu port */
@@ -1024,6 +1026,9 @@ mt7530_port_enable(struct dsa_switch *ds, int port,
priv->ports[port].pm);
mt7530_clear(priv, MT7530_PMCR_P(port), PMCR_LINK_SETTINGS_MASK);
+ if ((port == 5 || port == 6) && dsa_port_is_cpu(dp))
+ dev_info(priv->dev, "MT7530_PMCR_P%d PMCR_LINK_SETTINGS_MASK is cleared\n", port);
+
mutex_unlock(&priv->reg_mutex);
return 0;
@@ -2693,6 +2698,9 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
mcr_new |= PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | PMCR_BACKOFF_EN |
PMCR_BACKPR_EN | PMCR_FORCE_MODE_ID(priv->id);
+ if ((port == 5 && dsa_is_cpu_port(ds, 5)) || (port == 6 && dsa_is_cpu_port(ds, 6)))
+ dev_info(priv->dev, "MT7530_PMCR_P%d PMCR_CPU_PORT_SETTING equivalent is set\n", port);
+
/* Are we connected to external phy */
if (port == 5 && dsa_is_user_port(ds, 5))
mcr_new |= PMCR_EXT_PHY;
@@ -2760,6 +2768,9 @@ static void mt753x_phylink_mac_link_up(struct dsa_switch *ds, int port,
}
mt7530_set(priv, MT7530_PMCR_P(port), mcr);
+
+ if ((port == 5 && dsa_is_cpu_port(ds, 5)) || (port == 6 && dsa_is_cpu_port(ds, 6)))
+ dev_info(priv->dev, "MT7530_PMCR_P%d PMCR_LINK_SETTINGS_MASK equivalent is set\n", port);
}
static int
@@ -2796,6 +2807,9 @@ mt7531_cpu_port_config(struct dsa_switch *ds, int port)
mt7530_write(priv, MT7530_PMCR_P(port),
PMCR_CPU_PORT_SETTING(priv->id));
+
+ dev_info(priv->dev, "MT7530_PMCR_P%d PMCR_CPU_PORT_SETTING is set\n", port);
+
mt753x_phylink_mac_link_up(ds, port, MLO_AN_FIXED, interface, NULL,
speed, DUPLEX_FULL, true, true);
[ 1.763066] mt7530-mdio mdio-bus:00: running cpu_port_config()
[ 1.769237] mt7530-mdio mdio-bus:00: MT7530_PMCR_P5 PMCR_CPU_PORT_SETTING is set
[ 1.776724] mt7530-mdio mdio-bus:00: MT7530_PMCR_P5 PMCR_LINK_SETTINGS_MASK equivalent is set
[ 1.785254] mt7530-mdio mdio-bus:00: cpu_port_config() ran
[ 1.792098] mt7530-mdio mdio-bus:00: running cpu_port_config()
[ 1.798019] mt7530-mdio mdio-bus:00: MT7530_PMCR_P6 PMCR_CPU_PORT_SETTING is set
[ 1.805502] mt7530-mdio mdio-bus:00: MT7530_PMCR_P6 PMCR_LINK_SETTINGS_MASK equivalent is set
[ 1.814023] mt7530-mdio mdio-bus:00: cpu_port_config() ran
[ 1.844941] mt7530-mdio mdio-bus:00: MT7530_PMCR_P5 PMCR_LINK_SETTINGS_MASK is cleared
[ 1.852972] mt7530-mdio mdio-bus:00: configuring for fixed/rgmii link mode
[ 1.859944] mt7530-mdio mdio-bus:00: MT7530_PMCR_P5 PMCR_CPU_PORT_SETTING equivalent is set
[ 1.868658] mt7530-mdio mdio-bus:00: MT7530_PMCR_P5 PMCR_LINK_SETTINGS_MASK equivalent is set
[ 1.868913] mt7530-mdio mdio-bus:00: MT7530_PMCR_P6 PMCR_LINK_SETTINGS_MASK is cleared
[ 1.877190] mt7530-mdio mdio-bus:00: Link is Up - 1Gbps/Full - flow control rx/tx
[ 1.885179] mt7530-mdio mdio-bus:00: configuring for fixed/2500base-x link mode
[ 1.899973] mt7530-mdio mdio-bus:00: MT7530_PMCR_P6 PMCR_CPU_PORT_SETTING equivalent is set
[ 1.910147] mt7530-mdio mdio-bus:00: MT7530_PMCR_P6 PMCR_LINK_SETTINGS_MASK equivalent is set
[ 1.918681] mt7530-mdio mdio-bus:00: Link is Up - 2.5Gbps/Full - flow control rx/tx
[ 1.920654] mt7530-mdio mdio-bus:00 wan (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7531 PHY] (irq=137)
[ 1.948453] mt7530-mdio mdio-bus:00 lan0 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7531 PHY] (irq=138)
[ 1.970382] mt7530-mdio mdio-bus:00 lan1 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7531 PHY] (irq=139)
[ 1.992423] mt7530-mdio mdio-bus:00 lan2 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7531 PHY] (irq=140)
[ 2.014310] mt7530-mdio mdio-bus:00 lan3 (uninitialized): PHY [mt7530-0:04] driver [MediaTek MT7531 PHY] (irq=141)
[ 2.025396] mtk_soc_eth 1b100000.ethernet eth1: entered promiscuous mode
[ 2.032160] mtk_soc_eth 1b100000.ethernet eth0: entered promiscuous mode
[ 2.038912] DSA: tree 0 setup
Arınç
On Wed, Jan 10, 2024 at 02:15:57PM +0300, Arınç ÜNAL wrote:
> On 11.06.2023 10:23, Arınç ÜNAL wrote:
> >
> > On 10.06.2023 20:55, Vladimir Oltean wrote:
> > > On Sat, Jun 10, 2023 at 01:57:27PM +0300, Arınç ÜNAL wrote:
> > > > I was able to confirm all user ports of the MT7531BE switch transmit/receive
> > > > traffic to/from the SGMII CPU port and computer fine after getting rid of
> > > > priv->info->cpu_port_config().
> > > >
> > > > Tried all user ports being affine to the RGMII CPU port, that works too.
> > > >
> > > > https://github.com/arinc9/linux/commit/4e79313a95d45950cab526456ef0030286ba4d4e
> > >
> > > Did you do black-box testing after removing the code, or were you
> > > also able to independently confirm that the configurations done by
> > > cpu_port_config() were later overwritten? I'm trying to disambiguate
> > > between "works by coincidence" and "works because the analysis was
> > > correct".
> >
> > I did my testing, merely to make sure we didn't miss anything as Russell already stated that the configuration from cpu_port_config() is later overwritten.
> >
> > I could put some dev_info around to confirm the code path that overwrites the configuration.
>
> I have finally tested this.
Replying to a question from 6 months ago is nice of you, like replying
to any question is. But everybody's short memory is by now hit like a
cold cache, everything has been forgotten. I don't even have this thread
in my inbox anymore, it's in the "seen" folder.
There's something to be said about having to re-read a long thread and
the code for 30 minutes, just to reply "Ok".
I think you need to develop a better feeling for when to let go of past
discussions when they become stale, summarize the essence in the commit
description of a patch, and then just resubmit that new patch. People
will have to open the code and make a fresh analysis anyway, so just
help them skip reading past discussions and just focus on the conclusion.
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index a4468468b53c..7b60a67d016a 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -968,9 +968,11 @@ mt753x_cpu_port_enable(struct dsa_switch *ds, int port)
> /* Setup max capability of CPU port at first */
> if (priv->info->cpu_port_config) {
> + dev_info(priv->dev, "running cpu_port_config()\n");
> ret = priv->info->cpu_port_config(ds, port);
> if (ret)
> return ret;
> + dev_info(priv->dev, "cpu_port_config() ran\n");
> }
> /* Enable Mediatek header mode on the cpu port */
> @@ -1024,6 +1026,9 @@ mt7530_port_enable(struct dsa_switch *ds, int port,
> priv->ports[port].pm);
> mt7530_clear(priv, MT7530_PMCR_P(port), PMCR_LINK_SETTINGS_MASK);
> + if ((port == 5 || port == 6) && dsa_port_is_cpu(dp))
> + dev_info(priv->dev, "MT7530_PMCR_P%d PMCR_LINK_SETTINGS_MASK is cleared\n", port);
> +
FYI, you can prefix your prints with something like this to make the log
easier to follow in terms of code paths taken.
"%s called from %pS <- %pS: ...\n",
__func__, __builtin_return_address(0), __builtin_return_address(1)
> mutex_unlock(&priv->reg_mutex);
> return 0;
> @@ -2693,6 +2698,9 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> mcr_new |= PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | PMCR_BACKOFF_EN |
> PMCR_BACKPR_EN | PMCR_FORCE_MODE_ID(priv->id);
> + if ((port == 5 && dsa_is_cpu_port(ds, 5)) || (port == 6 && dsa_is_cpu_port(ds, 6)))
> + dev_info(priv->dev, "MT7530_PMCR_P%d PMCR_CPU_PORT_SETTING equivalent is set\n", port);
> +
> /* Are we connected to external phy */
> if (port == 5 && dsa_is_user_port(ds, 5))
> mcr_new |= PMCR_EXT_PHY;
> @@ -2760,6 +2768,9 @@ static void mt753x_phylink_mac_link_up(struct dsa_switch *ds, int port,
> }
> mt7530_set(priv, MT7530_PMCR_P(port), mcr);
> +
> + if ((port == 5 && dsa_is_cpu_port(ds, 5)) || (port == 6 && dsa_is_cpu_port(ds, 6)))
> + dev_info(priv->dev, "MT7530_PMCR_P%d PMCR_LINK_SETTINGS_MASK equivalent is set\n", port);
> }
> static int
> @@ -2796,6 +2807,9 @@ mt7531_cpu_port_config(struct dsa_switch *ds, int port)
> mt7530_write(priv, MT7530_PMCR_P(port),
> PMCR_CPU_PORT_SETTING(priv->id));
> +
> + dev_info(priv->dev, "MT7530_PMCR_P%d PMCR_CPU_PORT_SETTING is set\n", port);
> +
> mt753x_phylink_mac_link_up(ds, port, MLO_AN_FIXED, interface, NULL,
> speed, DUPLEX_FULL, true, true);
>
> [ 1.763066] mt7530-mdio mdio-bus:00: running cpu_port_config()
> [ 1.769237] mt7530-mdio mdio-bus:00: MT7530_PMCR_P5 PMCR_CPU_PORT_SETTING is set
> [ 1.776724] mt7530-mdio mdio-bus:00: MT7530_PMCR_P5 PMCR_LINK_SETTINGS_MASK equivalent is set
> [ 1.785254] mt7530-mdio mdio-bus:00: cpu_port_config() ran
This is from mt7531_setup_common(), for port 5.
> [ 1.792098] mt7530-mdio mdio-bus:00: running cpu_port_config()
> [ 1.798019] mt7530-mdio mdio-bus:00: MT7530_PMCR_P6 PMCR_CPU_PORT_SETTING is set
> [ 1.805502] mt7530-mdio mdio-bus:00: MT7530_PMCR_P6 PMCR_LINK_SETTINGS_MASK equivalent is set
> [ 1.814023] mt7530-mdio mdio-bus:00: cpu_port_config() ran
This is from mt7531_setup_common(), for port 6.
> [ 1.844941] mt7530-mdio mdio-bus:00: MT7530_PMCR_P5 PMCR_LINK_SETTINGS_MASK is cleared
This is from mt7530_port_enable() for port 5, undoing what mt7531_setup_common() has done.
It also seems bogus BTW, the enable() function is doing the same "clear"
as mt7530_port_disable() is doing, rather than mt7530_set(). Were it not
for what's to come [1], this would be a bug with an actual user impact.
> [ 1.852972] mt7530-mdio mdio-bus:00: configuring for fixed/rgmii link mode
> [ 1.859944] mt7530-mdio mdio-bus:00: MT7530_PMCR_P5 PMCR_CPU_PORT_SETTING equivalent is set
This is from mt753x_phylink_mac_config(), for port 5, partially
overwriting what mt7531_setup_common() has done.
> [ 1.868658] mt7530-mdio mdio-bus:00: MT7530_PMCR_P5 PMCR_LINK_SETTINGS_MASK equivalent is set
[1] This is from mt753x_phylink_mac_link_up(), for port 5, overwriting what
mt7530_port_enable() has done. I suspect that, in addition to Russell's
analysis, modifying PMCR_LINK_SETTINGS_MASK from the port_enable()/
port_disable() ops is also something that can be removed.
> [ 1.868913] mt7530-mdio mdio-bus:00: MT7530_PMCR_P6 PMCR_LINK_SETTINGS_MASK is cleared
> [ 1.877190] mt7530-mdio mdio-bus:00: Link is Up - 1Gbps/Full - flow control rx/tx
> [ 1.885179] mt7530-mdio mdio-bus:00: configuring for fixed/2500base-x link mode
> [ 1.899973] mt7530-mdio mdio-bus:00: MT7530_PMCR_P6 PMCR_CPU_PORT_SETTING equivalent is set
> [ 1.910147] mt7530-mdio mdio-bus:00: MT7530_PMCR_P6 PMCR_LINK_SETTINGS_MASK equivalent is set
> [ 1.918681] mt7530-mdio mdio-bus:00: Link is Up - 2.5Gbps/Full - flow control rx/tx
> [ 1.920654] mt7530-mdio mdio-bus:00 wan (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7531 PHY] (irq=137)
> [ 1.948453] mt7530-mdio mdio-bus:00 lan0 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7531 PHY] (irq=138)
> [ 1.970382] mt7530-mdio mdio-bus:00 lan1 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7531 PHY] (irq=139)
> [ 1.992423] mt7530-mdio mdio-bus:00 lan2 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7531 PHY] (irq=140)
> [ 2.014310] mt7530-mdio mdio-bus:00 lan3 (uninitialized): PHY [mt7530-0:04] driver [MediaTek MT7531 PHY] (irq=141)
> [ 2.025396] mtk_soc_eth 1b100000.ethernet eth1: entered promiscuous mode
> [ 2.032160] mtk_soc_eth 1b100000.ethernet eth0: entered promiscuous mode
> [ 2.038912] DSA: tree 0 setup
>
> Arınç
And this is all the same for port 6.
So yes, it would be good to consolidate the code to follow a simple principle.
Any register fields should be modified only by the set of methods that
they pertain to. In this case, MT7530_PMCR_P appears to only hold link
control information, so it pertains to phylink's methods. Otherwise,
the natural consequence is that they will get unexpectedly overwritten.
It seems outside of the competence of ds->ops->port_enable() and
ds->ops->port_disable(). Those would be appropriate, for example, to
control the switching matrix settings between a user port and its
corresponding CPU port (but not any more switching matrix settings -
those pertain to port_bridge_join() and port_bridge_leave()).
I hope this helps.
On 10.01.2024 17:27, Vladimir Oltean wrote:
> On Wed, Jan 10, 2024 at 02:15:57PM +0300, Arınç ÜNAL wrote:
>> I have finally tested this.
>
> Replying to a question from 6 months ago is nice of you, like replying
> to any question is. But everybody's short memory is by now hit like a
> cold cache, everything has been forgotten. I don't even have this thread
> in my inbox anymore, it's in the "seen" folder.
>
> There's something to be said about having to re-read a long thread and
> the code for 30 minutes, just to reply "Ok".
>
> I think you need to develop a better feeling for when to let go of past
> discussions when they become stale, summarize the essence in the commit
> description of a patch, and then just resubmit that new patch. People
> will have to open the code and make a fresh analysis anyway, so just
> help them skip reading past discussions and just focus on the conclusion.
I agree, thanks for bearing with me here.
>
> FYI, you can prefix your prints with something like this to make the log
> easier to follow in terms of code paths taken.
>
> "%s called from %pS <- %pS: ...\n",
> __func__, __builtin_return_address(0), __builtin_return_address(1)
__builtin_return_address(1) doesn't seem to work. I'm running this on
arm64. Apart from that, it works well. Thank you.
[ 1.863034] mt7530-mdio mdio-bus:00: mt753x_cpu_port_enable called from mt7531_setup_common+0x32c/0x370 <- 0x0: running cpu_port_config()
[ 1.875736] mt7530-mdio mdio-bus:00: mt7531_cpu_port_config called from mt753x_cpu_port_enable+0x64/0x23c <- 0x0: MT7530_PMCR_P5 PMCR_CPU_PORT_SETTING is set
[ 1.889922] mt7530-mdio mdio-bus:00: mt753x_phylink_mac_link_up called from mt7531_cpu_port_config+0xe8/0x12c <- 0x0: MT7530_PMCR_P5 PMCR_LINK_SETTINGS_MASK equivalent is set
[ 1.905491] mt7530-mdio mdio-bus:00: cpu_port_config() ran
[ 1.912336] mt7530-mdio mdio-bus:00: mt753x_cpu_port_enable called from mt7531_setup_common+0x32c/0x370 <- 0x0: running cpu_port_config()
[ 1.924777] mt7530-mdio mdio-bus:00: mt7531_cpu_port_config called from mt753x_cpu_port_enable+0x64/0x23c <- 0x0: MT7530_PMCR_P6 PMCR_CPU_PORT_SETTING is set
[ 1.938953] mt7530-mdio mdio-bus:00: mt753x_phylink_mac_link_up called from mt7531_cpu_port_config+0xe8/0x12c <- 0x0: MT7530_PMCR_P6 PMCR_LINK_SETTINGS_MASK equivalent is set
[ 1.954525] mt7530-mdio mdio-bus:00: cpu_port_config() ran
[ 1.985409] mt7530-mdio mdio-bus:00: mt7530_port_enable called from dsa_port_enable_rt+0x2c/0x98 <- 0x0: MT7530_PMCR_P5 PMCR_LINK_SETTINGS_MASK is cleared
[ 1.999378] mt7530-mdio mdio-bus:00: configuring for fixed/rgmii link mode
[ 2.006347] mt7530-mdio mdio-bus:00: mt753x_phylink_mac_config called from dsa_port_phylink_mac_config+0x30/0x3c <- 0x0: MT7530_PMCR_P5 PMCR_CPU_PORT_SETTING equivalent is set
[ 2.022197] mt7530-mdio mdio-bus:00: mt753x_phylink_mac_link_up called from dsa_port_phylink_mac_link_up+0x48/0x74 <- 0x0: MT7530_PMCR_P5 PMCR_LINK_SETTINGS_MASK equivalent is set
[ 2.022645] mt7530-mdio mdio-bus:00: mt7530_port_enable called from dsa_port_enable_rt+0x2c/0x98 <- 0x0: MT7530_PMCR_P6 PMCR_LINK_SETTINGS_MASK is cleared
[ 2.038203] mt7530-mdio mdio-bus:00: Link is Up - 1Gbps/Full - flow control rx/tx
[ 2.052090] mt7530-mdio mdio-bus:00: configuring for fixed/2500base-x link mode
[ 2.066894] mt7530-mdio mdio-bus:00: mt753x_phylink_mac_config called from dsa_port_phylink_mac_config+0x30/0x3c <- 0x0: MT7530_PMCR_P6 PMCR_CPU_PORT_SETTING equivalent is set
[ 2.084406] mt7530-mdio mdio-bus:00: mt753x_phylink_mac_link_up called from dsa_port_phylink_mac_link_up+0x48/0x74 <- 0x0: MT7530_PMCR_P6 PMCR_LINK_SETTINGS_MASK equivalent is set
[ 2.095093] mt7530-mdio mdio-bus:00 wan (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7531 PHY] (irq=137)
[ 2.100427] mt7530-mdio mdio-bus:00: Link is Up - 2.5Gbps/Full - flow control rx/tx
>
>> [ 1.763066] mt7530-mdio mdio-bus:00: running cpu_port_config()
>> [ 1.769237] mt7530-mdio mdio-bus:00: MT7530_PMCR_P5 PMCR_CPU_PORT_SETTING is set
>> [ 1.776724] mt7530-mdio mdio-bus:00: MT7530_PMCR_P5 PMCR_LINK_SETTINGS_MASK equivalent is set
>> [ 1.785254] mt7530-mdio mdio-bus:00: cpu_port_config() ran
>
> This is from mt7531_setup_common(), for port 5.
>
>> [ 1.792098] mt7530-mdio mdio-bus:00: running cpu_port_config()
>> [ 1.798019] mt7530-mdio mdio-bus:00: MT7530_PMCR_P6 PMCR_CPU_PORT_SETTING is set
>> [ 1.805502] mt7530-mdio mdio-bus:00: MT7530_PMCR_P6 PMCR_LINK_SETTINGS_MASK equivalent is set
>> [ 1.814023] mt7530-mdio mdio-bus:00: cpu_port_config() ran
>
> This is from mt7531_setup_common(), for port 6.
>
>> [ 1.844941] mt7530-mdio mdio-bus:00: MT7530_PMCR_P5 PMCR_LINK_SETTINGS_MASK is cleared
>
> This is from mt7530_port_enable() for port 5, undoing what mt7531_setup_common() has done.
> It also seems bogus BTW, the enable() function is doing the same "clear"
> as mt7530_port_disable() is doing, rather than mt7530_set(). Were it not
> for what's to come [1], this would be a bug with an actual user impact.
>
>> [ 1.852972] mt7530-mdio mdio-bus:00: configuring for fixed/rgmii link mode
>> [ 1.859944] mt7530-mdio mdio-bus:00: MT7530_PMCR_P5 PMCR_CPU_PORT_SETTING equivalent is set
>
> This is from mt753x_phylink_mac_config(), for port 5, partially
> overwriting what mt7531_setup_common() has done.
>
>> [ 1.868658] mt7530-mdio mdio-bus:00: MT7530_PMCR_P5 PMCR_LINK_SETTINGS_MASK equivalent is set
>
> [1] This is from mt753x_phylink_mac_link_up(), for port 5, overwriting what
> mt7530_port_enable() has done. I suspect that, in addition to Russell's
> analysis, modifying PMCR_LINK_SETTINGS_MASK from the port_enable()/
> port_disable() ops is also something that can be removed.
>
>> [ 1.868913] mt7530-mdio mdio-bus:00: MT7530_PMCR_P6 PMCR_LINK_SETTINGS_MASK is cleared
>> [ 1.877190] mt7530-mdio mdio-bus:00: Link is Up - 1Gbps/Full - flow control rx/tx
>> [ 1.885179] mt7530-mdio mdio-bus:00: configuring for fixed/2500base-x link mode
>> [ 1.899973] mt7530-mdio mdio-bus:00: MT7530_PMCR_P6 PMCR_CPU_PORT_SETTING equivalent is set
>> [ 1.910147] mt7530-mdio mdio-bus:00: MT7530_PMCR_P6 PMCR_LINK_SETTINGS_MASK equivalent is set
>> [ 1.918681] mt7530-mdio mdio-bus:00: Link is Up - 2.5Gbps/Full - flow control rx/tx
>> [ 1.920654] mt7530-mdio mdio-bus:00 wan (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7531 PHY] (irq=137)
>> [ 1.948453] mt7530-mdio mdio-bus:00 lan0 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7531 PHY] (irq=138)
>> [ 1.970382] mt7530-mdio mdio-bus:00 lan1 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7531 PHY] (irq=139)
>> [ 1.992423] mt7530-mdio mdio-bus:00 lan2 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7531 PHY] (irq=140)
>> [ 2.014310] mt7530-mdio mdio-bus:00 lan3 (uninitialized): PHY [mt7530-0:04] driver [MediaTek MT7531 PHY] (irq=141)
>> [ 2.025396] mtk_soc_eth 1b100000.ethernet eth1: entered promiscuous mode
>> [ 2.032160] mtk_soc_eth 1b100000.ethernet eth0: entered promiscuous mode
>> [ 2.038912] DSA: tree 0 setup
>>
>> Arınç
>
> And this is all the same for port 6.
>
> So yes, it would be good to consolidate the code to follow a simple principle.
> Any register fields should be modified only by the set of methods that
> they pertain to. In this case, MT7530_PMCR_P appears to only hold link
> control information, so it pertains to phylink's methods. Otherwise,
> the natural consequence is that they will get unexpectedly overwritten.
>
> It seems outside of the competence of ds->ops->port_enable() and
> ds->ops->port_disable(). Those would be appropriate, for example, to
> control the switching matrix settings between a user port and its
> corresponding CPU port (but not any more switching matrix settings -
> those pertain to port_bridge_join() and port_bridge_leave()).
>
> I hope this helps.
This is very helpful, thank you very much. This is what I deduct I should
do:
First patch: Get rid of cpu_port_config().
Second patch: Collect port link control register operations from
port_enable/port_disable and phylink_mac_config to
phylink_mac_link_up/phylink_mac_link_down.
Arınç
On Wed, Jan 10, 2024 at 08:15:20PM +0300, Arınç ÜNAL wrote:
> __builtin_return_address(1) doesn't seem to work. I'm running this on arm64.
I can't tell you why either, I'm sorry. I can just point to the
documentation, which does specify that "On some machines it may be
impossible to determine the return address of any function other than
the current one". If somebody knows what this depends on, feel free to
interject.
https://gcc.gnu.org/onlinedocs/gcc/Return-Address.html
On my NXP LS1028A (also arm64) plus clang-16 compiler, __builtin_return_address()
does work with multiple nesting levels.
> This is very helpful, thank you very much. This is what I deduct I should
> do:
>
> First patch: Get rid of cpu_port_config().
>
> Second patch: Collect port link control register operations from
> port_enable/port_disable and phylink_mac_config to
> phylink_mac_link_up/phylink_mac_link_down.
I guess. Sounds good.
On Wed, Jan 10, 2024 at 08:05:25PM +0200, Vladimir Oltean wrote:
> On Wed, Jan 10, 2024 at 08:15:20PM +0300, Arınç ÜNAL wrote:
> > __builtin_return_address(1) doesn't seem to work. I'm running this on arm64.
>
> I can't tell you why either, I'm sorry. I can just point to the
> documentation, which does specify that "On some machines it may be
> impossible to determine the return address of any function other than
> the current one". If somebody knows what this depends on, feel free to
> interject.
> https://gcc.gnu.org/onlinedocs/gcc/Return-Address.html
>
> On my NXP LS1028A (also arm64) plus clang-16 compiler, __builtin_return_address()
> does work with multiple nesting levels.
gcc will probably need to be using frame pointers so it can walk the
stack, if gcc even implements non-zero values to
__builtin_return_address(). Without frame pointers, it would need an
unwinder.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Wed, Jan 10, 2024 at 06:31:11PM +0000, Russell King (Oracle) wrote:
> On Wed, Jan 10, 2024 at 08:05:25PM +0200, Vladimir Oltean wrote:
> > On Wed, Jan 10, 2024 at 08:15:20PM +0300, Arınç ÜNAL wrote:
> > > __builtin_return_address(1) doesn't seem to work. I'm running this on arm64.
> >
> > I can't tell you why either, I'm sorry. I can just point to the
> > documentation, which does specify that "On some machines it may be
> > impossible to determine the return address of any function other than
> > the current one". If somebody knows what this depends on, feel free to
> > interject.
> > https://gcc.gnu.org/onlinedocs/gcc/Return-Address.html
> >
> > On my NXP LS1028A (also arm64) plus clang-16 compiler, __builtin_return_address()
> > does work with multiple nesting levels.
>
> gcc will probably need to be using frame pointers so it can walk the
> stack, if gcc even implements non-zero values to
> __builtin_return_address(). Without frame pointers, it would need an
> unwinder.
Yeah, I guess it's a gcc limitation. I recompiled the kernel for the same
platform with gcc 11.2 from Arm, and I get the same result as Arınç now.