Hello!
This patch series is mainly focused on improving the support for port 5,
setting up port 6, and refactoring the MT7530 DSA subdriver. There're also
fixes for the switch on the MT7988 SoC.
I'm asking for your comments on patch 4 and 9.
For patch 4:
If you think priv->p5_interface should not be set when port 5 is used for
PHY muxing, let me know.
For patch 9:
Do I need to protect the register from being accessed by processes while
this operation is being done? I don't see this on mt7530_setup() but it's
being done on mt7530_setup_port5().
There's an oddity here. The XTAL mask is defined on the MT7530_HWTRAP
register, but it's being read from MT7530_MHWTRAP instead which is at a
different address.
HWTRAP_XTAL_MASK and reading HWTRAP_XTAL_MASK from MT7530_MHWTRAP was both
added with:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit?id=7ef6f6f8d237fa6724108b57d9706cb5069688e4
I did this to test it:
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 46749aee3c49..7aa3b5828ac3 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -404,7 +404,7 @@ static int
mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
{
struct mt7530_priv *priv = ds->priv;
- u32 ncpo1, ssc_delta, trgint, xtal, val;
+ u32 ncpo1, ssc_delta, trgint, xtal, xtal2, val;
/* Enable port 6 */
val = mt7530_read(priv, MT7530_MHWTRAP);
@@ -413,6 +413,21 @@ mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
mt7530_write(priv, MT7530_MHWTRAP, val);
xtal = val & HWTRAP_XTAL_MASK;
+ xtal2 = mt7530_read(priv, MT7530_HWTRAP) & HWTRAP_XTAL_MASK;
+
+ if (xtal == HWTRAP_XTAL_20MHZ)
+ dev_info(priv->dev, "xtal 20 Mhz\n");
+ if (xtal == HWTRAP_XTAL_25MHZ)
+ dev_info(priv->dev, "xtal 25 Mhz\n");
+ if (xtal == HWTRAP_XTAL_40MHZ)
+ dev_info(priv->dev, "xtal 40 Mhz\n");
+
+ if (xtal2 == HWTRAP_XTAL_20MHZ)
+ dev_info(priv->dev, "actual xtal 20 Mhz\n");
+ if (xtal2 == HWTRAP_XTAL_25MHZ)
+ dev_info(priv->dev, "actual xtal 25 Mhz\n");
+ if (xtal2 == HWTRAP_XTAL_40MHZ)
+ dev_info(priv->dev, "actual xtal 40 Mhz\n");
if (xtal == HWTRAP_XTAL_20MHZ) {
dev_err(priv->dev,
Both ended up reporting 40 Mhz so I'm not sure if this is a bug or intended
to be done this way.
Please advise.
The only missing piece to properly support port 5 as a CPU port is the
fixes [0] [1] [2] from Richard.
I have very thoroughly tested the patch series with every possible mode to
use. 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.bin; tftpboot 0x83000000 mips-rootfs.cpio.uboot; tftpboot 0x83f00000 $dtb; bootm 0x80008000 0x83000000 0x83f00000
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-uImage; tftpboot 0x83000000 arm-rootfs.cpio.uboot; tftpboot 0x83f00000 $dtb; bootm 0x80008000 0x83000000 0x83f00000
Current CPU ports setup of MT7530:
mt7530_setup()
-> mt7530_setup_port5()
mt753x_phylink_mac_config()
-> mt753x_mac_config()
-> mt7530_mac_config()
-> mt7530_setup_port5()
-> mt753x_pad_setup()
-> mt7530_pad_clk_setup() sets up port 6, rename to mt7530_setup_port6()
How it will be with the patch series:
mt7530_setup()
-> mt7530_setup_port5() runs if the port is not used as a CPU, DSA, or user port
mt753x_phylink_mac_config()
-> mt753x_mac_config()
-> mt7530_mac_config()
-> mt7530_setup_port5()
-> mt7530_setup_port6()
CPU ports setup of MT7531 for reference:
mt7531_setup()
-> mt753x_cpu_port_enable()
-> mt7531_cpu_port_config()
-> mt7531_mac_config()
-> mt7531_rgmii_setup()
-> mt7531_sgmii_setup_mode_an()
-> etc.
mt753x_phylink_mac_config()
-> mt753x_mac_config()
-> mt7531_mac_config()
-> mt7531_rgmii_setup()
-> mt7531_sgmii_setup_mode_an()
-> etc.
[0] https://lore.kernel.org/netdev/[email protected]/
[1] https://lore.kernel.org/netdev/[email protected]/
[2] https://lore.kernel.org/netdev/[email protected]/
Arınç
RFC v2: Add two patches related to MT7988 to the end.
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 | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index a00aabe4987e..9ab2e128b564 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -943,9 +943,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);
@@ -2299,7 +2296,6 @@ mt7530_setup(struct dsa_switch *ds)
* if PHY muxing is detected.
*/
priv->p5_intf_sel = P5_DISABLED;
- interface = PHY_INTERFACE_MODE_NA;
for_each_child_of_node(dn, mac_np) {
if (!of_device_is_compatible(mac_np,
@@ -2332,7 +2328,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.37.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()
The first code path is supposed to run when PHY muxing is being used. In
this case, port 5 is somewhat of a hidden port. It won't be defined on the
devicetree so phylink can't be used to manage the port.
The second code path used to call mt7530_setup_port5() directly under case
5 on mt7530_phylink_mac_config() before it was moved to mt7530_mac_config()
with 88bdef8be9f6 ("net: dsa: mt7530: Extend device data ready for adding a
new hardware"). mt7530_setup_port5() will never run through this code path
because the current code on mt7530_setup() bypasses phylink for all cases
of port 5.
Fix this by leaving it to phylink 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() directly from mt7530_setup() without involving
phylink.
Move setting the interface and P5_DISABLED mode to a more specific
location. They're supposed to be overwritten if PHY muxing is detected.
Add comments which explain the process.
Tested-by: Arınç ÜNAL <[email protected]>
Signed-off-by: Arınç ÜNAL <[email protected]>
---
drivers/net/dsa/mt7530.c | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 31ef70f0cd12..a00aabe4987e 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2288,16 +2288,19 @@ 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)) {
+ /* Set the interface selection of port 5 to GMAC5 when it's used
+ * as a CPU, DSA, or user port. Let phylink handle the rest.
+ */
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 P5_DISABLED first, then overwrite it
+ * if PHY muxing is detected.
+ */
+ priv->p5_intf_sel = P5_DISABLED;
+ interface = PHY_INTERFACE_MODE_NA;
+
for_each_child_of_node(dn, mac_np) {
if (!of_device_is_compatible(mac_np,
"mediatek,eth-mac"))
@@ -2328,6 +2331,8 @@ mt7530_setup(struct dsa_switch *ds)
of_node_put(phy_node);
break;
}
+
+ mt7530_setup_port5(ds, interface);
}
#ifdef CONFIG_GPIOLIB
@@ -2338,8 +2343,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.37.2
From: Arınç ÜNAL <[email protected]>
There's no logic to numerically order the CPU ports. State the port number
and its being 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]>
---
drivers/net/dsa/mt7530.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index e4bb5037d352..31ef70f0cd12 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2506,7 +2506,7 @@ 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, a CPU port, supports rgmii, mii, and gmii. */
phy_interface_set_rgmii(config->supported_interfaces);
__set_bit(PHY_INTERFACE_MODE_MII,
config->supported_interfaces);
@@ -2514,7 +2514,7 @@ 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, a CPU port, supports rgmii and trgmii. */
__set_bit(PHY_INTERFACE_MODE_RGMII,
config->supported_interfaces);
__set_bit(PHY_INTERFACE_MODE_TRGMII,
@@ -2539,14 +2539,14 @@ 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, a CPU port, supports rgmii and sgmii/802.3z. */
if (mt7531_is_rgmii_port(priv, port)) {
phy_interface_set_rgmii(config->supported_interfaces);
break;
}
fallthrough;
- case 6: /* 1st cpu port supports sgmii/8023z only */
+ case 6: /* Port 6, a CPU port, supports sgmii/802.3z only. */
__set_bit(PHY_INTERFACE_MODE_SGMII,
config->supported_interfaces);
__set_bit(PHY_INTERFACE_MODE_1000BASEX,
@@ -2738,7 +2738,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, a CPU port. */
if (priv->p5_interface == state->interface)
break;
@@ -2748,7 +2748,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, a CPU port. */
if (priv->p6_interface == state->interface)
break;
--
2.37.2
From: Arınç ÜNAL <[email protected]>
Currently, priv->p5_interface is set on mt7530_setup_port5() even though
it's being set on mt753x_phylink_mac_config() after mt7530_setup_port5() is
run.
The only case for setting priv->p5_interface on mt7530_setup_port5() 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.
To address this, set priv->p5_interface only if PHY muxing is enabled.
On mt753x_phylink_mac_config, != P5_DISABLED is enough but to be explicit,
look for p5_intf_sel being P5_INTF_SEL_GMAC5 or P5_INTF_SEL_GMAC5_SGMII.
Tested-by: Arınç ÜNAL <[email protected]>
Signed-off-by: Arınç ÜNAL <[email protected]>
---
drivers/net/dsa/mt7530.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 9ab2e128b564..fccd59564532 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -976,7 +976,9 @@ 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;
+ if (priv->p5_intf_sel == P5_INTF_SEL_PHY_P0 ||
+ priv->p5_intf_sel == P5_INTF_SEL_PHY_P4)
+ priv->p5_interface = interface;
unlock_exit:
mutex_unlock(&priv->reg_mutex);
@@ -2746,7 +2748,8 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
if (mt753x_mac_config(ds, port, mode, state) < 0)
goto unsupported;
- if (priv->p5_intf_sel != P5_DISABLED)
+ if (priv->p5_intf_sel == P5_INTF_SEL_GMAC5 ||
+ priv->p5_intf_sel == P5_INTF_SEL_GMAC5_SGMII)
priv->p5_interface = state->interface;
break;
case 6: /* Port 6, a CPU port. */
--
2.37.2
From: Arınç ÜNAL <[email protected]>
There is no need to set priv->p5_interface and priv->p6_interface to
PHY_INTERFACE_MODE_NA on mt7530_setup() and mt7531_setup().
As Vladimir explained, in include/linux/phy.h we have:
typedef enum {
PHY_INTERFACE_MODE_NA,
In lack of other initialiser, the first element of an enum gets the value 0
in C.
Then, "priv" is allocated by this driver with devm_kzalloc(), which means
that its entire memory is zero-filled. So priv->p5_interface and
priv->p6_interface are already set to 0, PHY_INTERFACE_MODE_NA.
There is no code path between the devm_kzalloc(), and the position in
mt7530_setup() and mt7531_setup() that would change the value of
priv->p5_interface or priv->p6_interface from 0.
The only place they are modified is mt753x_phylink_mac_config() but
mt753x_phylink_mac_config() runs after mt753x_setup(), as can be seen on
the code path below.
mt7530_probe()
-> dsa_register_switch()
-> dsa_switch_probe()
-> dsa_tree_setup()
-> dsa_tree_setup_switches()
-> dsa_switch_setup()
-> ds->ops->setup(): mt753x_setup()
-> dsa_tree_setup_ports()
-> dsa_port_setup()
[...]
-> dsa_port_phylink_create()
[...]
-> phylink_mac_config()
-> pl->mac_ops->mac_config():
dsa_port_phylink_mac_config()
-> ds->ops->phylink_mac_config():
mt753x_phylink_mac_config()
Therefore, do not put 0 into a variable containing 0.
Tested-by: Arınç ÜNAL <[email protected]>
Signed-off-by: Arınç ÜNAL <[email protected]>
---
drivers/net/dsa/mt7530.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 8a47dcb96cdf..fc5428baa905 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2247,8 +2247,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);
@@ -2466,10 +2464,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
--
2.37.2
From: Arınç ÜNAL <[email protected]>
On the switch on the MT7988 SoC, there are only 4 PHYs. That's port 0 to 3.
Set the internal phy cases to '0 ... 3'.
There's no need to clear the config->supported_interfaces bitmap before
reporting the supported interfaces as all bits in the bitmap will already
be initialized to zero when the phylink_config structure is allocated.
There's no code that would change the bitmap beforehand. Remove it.
Signed-off-by: Arınç ÜNAL <[email protected]>
---
drivers/net/dsa/mt7530.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 6fbbdcb5987f..903e39b7b772 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2545,10 +2545,8 @@ static void mt7531_mac_port_get_caps(struct dsa_switch *ds, int port,
static void mt7988_mac_port_get_caps(struct dsa_switch *ds, int port,
struct phylink_config *config)
{
- phy_interface_zero(config->supported_interfaces);
-
switch (port) {
- case 0 ... 4: /* Internal phy */
+ case 0 ... 3: /* Internal phy */
__set_bit(PHY_INTERFACE_MODE_INTERNAL,
config->supported_interfaces);
break;
--
2.37.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()
On the first code path, priv->p5_intf_sel is either set to
P5_INTF_SEL_PHY_P0 or P5_INTF_SEL_PHY_P4 when mt7530_setup_port5() is run.
On the second code path, priv->p5_intf_sel is set to P5_INTF_SEL_GMAC5 when
mt7530_setup_port5() is run.
Remove this default case which will never run.
Tested-by: Arınç ÜNAL <[email protected]>
Signed-off-by: Arınç ÜNAL <[email protected]>
---
drivers/net/dsa/mt7530.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index fccd59564532..8a47dcb96cdf 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -943,10 +943,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;
- default:
- dev_err(ds->dev, "Unsupported p5_intf_sel %d\n",
- priv->p5_intf_sel);
- goto unlock_exit;
}
/* Setup RGMII settings */
@@ -980,7 +976,6 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
priv->p5_intf_sel == P5_INTF_SEL_PHY_P4)
priv->p5_interface = interface;
-unlock_exit:
mutex_unlock(&priv->reg_mutex);
}
--
2.37.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 call 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 fc5428baa905..c636a888d194 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 bool mt7531_dual_sgmii_supported(struct mt7530_priv *priv)
{
u32 val;
@@ -2583,12 +2589,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.37.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]>
---
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 c636a888d194..0a6d1c0872be 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -473,12 +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 bool mt7531_dual_sgmii_supported(struct mt7530_priv *priv)
{
u32 val;
@@ -488,12 +482,6 @@ static bool mt7531_dual_sgmii_supported(struct mt7530_priv *priv)
return (val & PAD_DUAL_SGMII_EN) != 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)
{
@@ -2576,14 +2564,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)
@@ -2754,8 +2734,6 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
if (priv->p6_interface == state->interface)
break;
- mt753x_pad_setup(ds, state);
-
if (mt753x_mac_config(ds, port, mode, state) < 0)
goto unsupported;
@@ -3053,11 +3031,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;
@@ -3119,7 +3092,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 +3103,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,
},
@@ -3143,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 = 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,
@@ -3156,7 +3126,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,
@@ -3186,9 +3155,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 01db5c9724fa..9e5b99b853ba 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -697,8 +697,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
@@ -719,7 +717,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.37.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 384e601b2ecd..6fbbdcb5987f 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -956,10 +956,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);
@@ -2227,6 +2223,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.37.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 fe496d865478..384e601b2ecd 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -404,7 +404,7 @@ static int
mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
{
struct mt7530_priv *priv = ds->priv;
- u32 ncpo1, ssc_delta, xtal, val;
+ u32 ncpo1, ssc_delta, i, xtal, val;
val = mt7530_read(priv, MT7530_MHWTRAP);
val &= ~MHWTRAP_P6_DIS;
@@ -464,6 +464,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));
}
return 0;
@@ -2227,10 +2232,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));
-
/* Enable PHY access and operate in manual mode */
val = mt7530_read(priv, MT7530_MHWTRAP);
val &= ~MHWTRAP_PHY_ACCESS;
--
2.37.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.
Signed-off-by: Arınç ÜNAL <[email protected]>
Co-authored-by: Daniel Golle <[email protected]>
Signed-off-by: Daniel Golle <[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 903e39b7b772..dd2221e839d9 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2629,17 +2629,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)
@@ -2680,6 +2669,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);
}
@@ -3123,7 +3115,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);
@@ -3151,8 +3142,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.37.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.
Tested-by: Arınç ÜNAL <[email protected]>
Signed-off-by: Arınç ÜNAL <[email protected]>
---
drivers/net/dsa/mt7530.c | 26 +++++++++-----------------
1 file changed, 9 insertions(+), 17 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 70a673347cf9..fe496d865478 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -404,7 +404,7 @@ static int
mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
{
struct mt7530_priv *priv = ds->priv;
- u32 ncpo1, ssc_delta, trgint, xtal, val;
+ u32 ncpo1, ssc_delta, xtal, val;
val = mt7530_read(priv, MT7530_MHWTRAP);
val &= ~MHWTRAP_P6_DIS;
@@ -419,16 +419,18 @@ mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
return -EINVAL;
}
- 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: 150MHz: 1.2GBit */
if (xtal == HWTRAP_XTAL_40MHZ)
@@ -441,17 +443,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);
--
2.37.2
From: Arınç ÜNAL <[email protected]>
Enable port 6 only when port 6 is being used. Read the HWTRAP_XTAL_MASK
value from val now that val is equal to the value of MT7530_MHWTRAP.
Update the comment on mt7530_setup() with a better explanation.
Tested-by: Arınç ÜNAL <[email protected]>
Signed-off-by: Arınç ÜNAL <[email protected]>
---
drivers/net/dsa/mt7530.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 0a6d1c0872be..70a673347cf9 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -404,9 +404,13 @@ static int
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, trgint, xtal, val;
- xtal = mt7530_read(priv, MT7530_MHWTRAP) & HWTRAP_XTAL_MASK;
+ val = mt7530_read(priv, MT7530_MHWTRAP);
+ val &= ~MHWTRAP_P6_DIS;
+ mt7530_write(priv, MT7530_MHWTRAP, val);
+
+ xtal = val & HWTRAP_XTAL_MASK;
if (xtal == HWTRAP_XTAL_20MHZ) {
dev_err(priv->dev,
@@ -2235,9 +2239,9 @@ mt7530_setup(struct dsa_switch *ds)
mt7530_rmw(priv, MT7530_TRGMII_RD(i),
RD_TAP_MASK, RD_TAP(16));
- /* Enable port 6 */
+ /* Enable PHY access and operate in manual mode */
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.37.2
On Fri, Apr 07, 2023 at 04:46:13PM +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 being a CPU port instead.
Port 5 is often used as a user port as well, eg. on the BPi-R3 where
it serves to provide SerDes for the 2nd SFP cage.
On other boards (e.g. Netgear WAX-206) it is used to connect a 2.5G
PHY used as WAN port.
Hence just stating that port 5 "a CPU port" could be a bit misleading
as it is not always used as a CPU port.
>
> 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]>
> ---
> drivers/net/dsa/mt7530.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index e4bb5037d352..31ef70f0cd12 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2506,7 +2506,7 @@ 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, a CPU port, supports rgmii, mii, and gmii. */
> phy_interface_set_rgmii(config->supported_interfaces);
> __set_bit(PHY_INTERFACE_MODE_MII,
> config->supported_interfaces);
> @@ -2514,7 +2514,7 @@ 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, a CPU port, supports rgmii and trgmii. */
> __set_bit(PHY_INTERFACE_MODE_RGMII,
> config->supported_interfaces);
> __set_bit(PHY_INTERFACE_MODE_TRGMII,
> @@ -2539,14 +2539,14 @@ 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, a CPU port, supports rgmii and sgmii/802.3z. */
> if (mt7531_is_rgmii_port(priv, port)) {
> phy_interface_set_rgmii(config->supported_interfaces);
> break;
> }
> fallthrough;
>
> - case 6: /* 1st cpu port supports sgmii/8023z only */
> + case 6: /* Port 6, a CPU port, supports sgmii/802.3z only. */
> __set_bit(PHY_INTERFACE_MODE_SGMII,
> config->supported_interfaces);
> __set_bit(PHY_INTERFACE_MODE_1000BASEX,
> @@ -2738,7 +2738,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, a CPU port. */
> if (priv->p5_interface == state->interface)
> break;
>
> @@ -2748,7 +2748,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, a CPU port. */
> if (priv->p6_interface == state->interface)
> break;
>
> --
> 2.37.2
>
On Fri, Apr 07, 2023 at 04:46:20PM +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]>
> ---
> 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 c636a888d194..0a6d1c0872be 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -473,12 +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 bool mt7531_dual_sgmii_supported(struct mt7530_priv *priv)
> {
> u32 val;
> @@ -488,12 +482,6 @@ static bool mt7531_dual_sgmii_supported(struct mt7530_priv *priv)
> return (val & PAD_DUAL_SGMII_EN) != 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)
> {
> @@ -2576,14 +2564,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)
> @@ -2754,8 +2734,6 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> if (priv->p6_interface == state->interface)
> break;
>
> - mt753x_pad_setup(ds, state);
> -
> if (mt753x_mac_config(ds, port, mode, state) < 0)
> goto unsupported;
>
> @@ -3053,11 +3031,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;
> @@ -3119,7 +3092,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 +3103,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,
> },
> @@ -3143,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 = 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,
> @@ -3156,7 +3126,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,
> @@ -3186,9 +3155,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 01db5c9724fa..9e5b99b853ba 100644
> --- a/drivers/net/dsa/mt7530.h
> +++ b/drivers/net/dsa/mt7530.h
> @@ -697,8 +697,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
> @@ -719,7 +717,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.37.2
>
On Fri, Apr 07, 2023 at 04:46:25PM +0300, [email protected] wrote:
> From: Arınç ÜNAL <[email protected]>
>
> On the switch on the MT7988 SoC, there are only 4 PHYs. That's port 0 to 3.
> Set the internal phy cases to '0 ... 3'.
>
> There's no need to clear the config->supported_interfaces bitmap before
> reporting the supported interfaces as all bits in the bitmap will already
> be initialized to zero when the phylink_config structure is allocated.
> There's no code that would change the bitmap beforehand. Remove it.
>
> Signed-off-by: Arınç ÜNAL <[email protected]>
Acked-by: Daniel Golle <[email protected]>
> ---
> drivers/net/dsa/mt7530.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 6fbbdcb5987f..903e39b7b772 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2545,10 +2545,8 @@ static void mt7531_mac_port_get_caps(struct dsa_switch *ds, int port,
> static void mt7988_mac_port_get_caps(struct dsa_switch *ds, int port,
> struct phylink_config *config)
> {
> - phy_interface_zero(config->supported_interfaces);
> -
> switch (port) {
> - case 0 ... 4: /* Internal phy */
> + case 0 ... 3: /* Internal phy */
> __set_bit(PHY_INTERFACE_MODE_INTERNAL,
> config->supported_interfaces);
> break;
> --
> 2.37.2
>
On 10.04.2023 17:30, Daniel Golle wrote:
> On Fri, Apr 07, 2023 at 04:46:13PM +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 being a CPU port instead.
>
> Port 5 is often used as a user port as well, eg. on the BPi-R3 where
> it serves to provide SerDes for the 2nd SFP cage.
> On other boards (e.g. Netgear WAX-206) it is used to connect a 2.5G
> PHY used as WAN port.
>
> Hence just stating that port 5 "a CPU port" could be a bit misleading
> as it is not always used as a CPU port.
Makes sense. I was not using the DSA term, so the "a CPU port" here
rather meant that the port connects to the CPU. I'll change it to "which
can be used as a CPU port" on both ports, that should explain that it
can be used as other than a CPU port, and it does not necessarily
connect to a CPU MAC.
Thanks.
Arınç
On Fri, Apr 07, 2023 at 04:46:18PM +0300, [email protected] wrote:
> From: Arınç ÜNAL <[email protected]>
>
> There is no need to set priv->p5_interface and priv->p6_interface to
> PHY_INTERFACE_MODE_NA on mt7530_setup() and mt7531_setup().
>
> As Vladimir explained, in include/linux/phy.h we have:
>
> Therefore, do not put 0 into a variable containing 0.
The explanation is unnecessarily long. I only provided it to make sure
you understand.
On Fri, Apr 07, 2023 at 04:46:14PM +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()
>
> The first code path is supposed to run when PHY muxing is being used. In
> this case, port 5 is somewhat of a hidden port. It won't be defined on the
> devicetree so phylink can't be used to manage the port.
>
> The second code path used to call mt7530_setup_port5() directly under case
> 5 on mt7530_phylink_mac_config() before it was moved to mt7530_mac_config()
> with 88bdef8be9f6 ("net: dsa: mt7530: Extend device data ready for adding a
> new hardware"). mt7530_setup_port5() will never run through this code path
> because the current code on mt7530_setup() bypasses phylink for all cases
> of port 5.
>
> Fix this by leaving it to phylink 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() directly from mt7530_setup() without involving
> phylink.
>
> Move setting the interface and P5_DISABLED mode to a more specific
> location. They're supposed to be overwritten if PHY muxing is detected.
>
> Add comments which explain the process.
>
> Tested-by: Arınç ÜNAL <[email protected]>
> Signed-off-by: Arınç ÜNAL <[email protected]>
> ---
We have a natural language processing engine (AUTOSEL) which
automatically picks up as candidates for "stable" those patches which
weren't explicitly submitted through the proper process for that (and
they contain words like "fix", "bug", "crash", "leak" etc).
Your chosen wording, both in the commit title and message, would most
likely trigger that bot, and then you'd have to explain why you chose
this language and not something else more descriptive of your change.
It would be nice if you could rewrite the commit messages for your
entire series to be a bit more succint as to what is the purpose of the
change you are making, and don't use the word "fix" when there is no
problem to be observed.
On 11.04.2023 17:54, Vladimir Oltean wrote:
> On Fri, Apr 07, 2023 at 04:46:18PM +0300, [email protected] wrote:
>> From: Arınç ÜNAL <[email protected]>
>>
>> There is no need to set priv->p5_interface and priv->p6_interface to
>> PHY_INTERFACE_MODE_NA on mt7530_setup() and mt7531_setup().
>>
>> As Vladimir explained, in include/linux/phy.h we have:
>>
>> Therefore, do not put 0 into a variable containing 0.
>
> The explanation is unnecessarily long. I only provided it to make sure
> you understand.
I don't see a problem with a bit of verbosity on the patch log. It
should make the reader understand the code path better.
Arınç
On 11.04.2023 18:00, Vladimir Oltean wrote:
> On Fri, Apr 07, 2023 at 04:46:14PM +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()
>>
>> The first code path is supposed to run when PHY muxing is being used. In
>> this case, port 5 is somewhat of a hidden port. It won't be defined on the
>> devicetree so phylink can't be used to manage the port.
>>
>> The second code path used to call mt7530_setup_port5() directly under case
>> 5 on mt7530_phylink_mac_config() before it was moved to mt7530_mac_config()
>> with 88bdef8be9f6 ("net: dsa: mt7530: Extend device data ready for adding a
>> new hardware"). mt7530_setup_port5() will never run through this code path
>> because the current code on mt7530_setup() bypasses phylink for all cases
>> of port 5.
>>
>> Fix this by leaving it to phylink 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() directly from mt7530_setup() without involving
>> phylink.
>>
>> Move setting the interface and P5_DISABLED mode to a more specific
>> location. They're supposed to be overwritten if PHY muxing is detected.
>>
>> Add comments which explain the process.
>>
>> Tested-by: Arınç ÜNAL <[email protected]>
>> Signed-off-by: Arınç ÜNAL <[email protected]>
>> ---
>
> We have a natural language processing engine (AUTOSEL) which
> automatically picks up as candidates for "stable" those patches which
> weren't explicitly submitted through the proper process for that (and
> they contain words like "fix", "bug", "crash", "leak" etc).
>
> Your chosen wording, both in the commit title and message, would most
> likely trigger that bot, and then you'd have to explain why you chose
> this language and not something else more descriptive of your change.
> It would be nice if you could rewrite the commit messages for your
> entire series to be a bit more succint as to what is the purpose of the
> change you are making, and don't use the word "fix" when there is no
> problem to be observed.
Will do, thanks.
Arınç