Hello!
This patch series simplifies the MT7530 DSA subdriver and improves the
logic of the support for MT7530, MT7531, and the switch on the MT7988 SoC.
I have done a simple ping test to confirm basic communication on all switch
ports on the MCM and standalone MT7530 and MT7531 switch with this patch
series applied.
MT7621 Unielec, MCM MT7530:
rgmii-only-gmac0-mt7621-unielec-u7621-06-16m.dtb
gmac0-and-gmac1-mt7621-unielec-u7621-06-16m.dtb
tftpboot 0x80008000 mips-uzImage.bin; tftpboot 0x83000000 mips-rootfs.cpio.uboot; tftpboot 0x83f00000 $dtb; bootm 0x80008000 0x83000000 0x83f00000
MT7622 Bananapi, MT7531:
gmac0-and-gmac1-mt7622-bananapi-bpi-r64.dtb
tftpboot 0x40000000 arm64-Image; tftpboot 0x45000000 arm64-rootfs.cpio.uboot; tftpboot 0x4a000000 $dtb; booti 0x40000000 0x45000000 0x4a000000
MT7623 Bananapi, standalone MT7530:
rgmii-only-gmac0-mt7623n-bananapi-bpi-r2.dtb
gmac0-and-gmac1-mt7623n-bananapi-bpi-r2.dtb
tftpboot 0x80008000 arm-zImage; tftpboot 0x83000000 arm-rootfs.cpio.uboot; tftpboot 0x83f00000 $dtb; bootz 0x80008000 0x83000000 0x83f00000
This patch series is the continuation of the one linked below.
https://lore.kernel.org/netdev/[email protected]/
Arınç
Arınç ÜNAL (15):
net: dsa: mt7530: always trap frames to active CPU port on MT7530
net: dsa: mt7530: use p5_interface_select as data type for p5_intf_sel
net: dsa: mt7530: store port 5 SGMII capability of MT7531
net: dsa: mt7530: improve comments regarding port 5 and 6
net: dsa: mt7530: improve code path for setting up port 5
net: dsa: mt7530: do not set priv->p5_interface on mt7530_setup_port5()
net: dsa: mt7530: do not run mt7530_setup_port5() if port 5 is disabled
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: simplify mt7530_setup_port6() and change to void
net: dsa: mt7530: correct port capabilities of MT7988
net: dsa: mt7530: do not clear config->supported_interfaces
drivers/net/dsa/mt7530-mdio.c | 7 +-
drivers/net/dsa/mt7530.c | 283 ++++++++++++++++---------------------
drivers/net/dsa/mt7530.h | 19 +--
3 files changed, 137 insertions(+), 172 deletions(-)
There's no logic to numerically order the CPU ports. State the port number
and its capability of being used as a CPU port instead.
Remove the irrelevant PHY muxing information from
mt7530_mac_port_get_caps(). Explain the supported MII modes instead.
Remove the out of place PHY muxing information from
mt753x_phylink_mac_config(). The function is for MT7530, MT7531, and the
switch on the MT7988 SoC but there's no PHY muxing on MT7531 or the switch
on the MT7988 SoC.
These comments were gradually introduced with the commits below.
ca366d6c889b ("net: dsa: mt7530: Convert to PHYLINK API")
38f790a80560 ("net: dsa: mt7530: Add support for port 5")
88bdef8be9f6 ("net: dsa: mt7530: Extend device data ready for adding a new
hardware")
c288575f7810 ("net: dsa: mt7530: Add the support of MT7531 switch")
Signed-off-by: Arınç ÜNAL <[email protected]>
Acked-by: Daniel Golle <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
---
drivers/net/dsa/mt7530.c | 32 ++++++++++++++++++++++----------
1 file changed, 22 insertions(+), 10 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 45c9698ad9dd..8623742b35ee 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2520,12 +2520,16 @@ static void mt7530_mac_port_get_caps(struct dsa_switch *ds, int port,
struct phylink_config *config)
{
switch (port) {
- case 0 ... 4: /* Internal phy */
+ /* Internal PHY */
+ case 0 ... 4:
__set_bit(PHY_INTERFACE_MODE_GMII,
config->supported_interfaces);
break;
- case 5: /* 2nd cpu port with phy of port 0 or 4 / external phy */
+ /* Port 5 which can be used as a CPU port supports rgmii with delays,
+ * mii, and gmii.
+ */
+ case 5:
phy_interface_set_rgmii(config->supported_interfaces);
__set_bit(PHY_INTERFACE_MODE_MII,
config->supported_interfaces);
@@ -2533,7 +2537,8 @@ static void mt7530_mac_port_get_caps(struct dsa_switch *ds, int port,
config->supported_interfaces);
break;
- case 6: /* 1st cpu port */
+ /* Port 6 which can be used as a CPU port supports rgmii and trgmii. */
+ case 6:
__set_bit(PHY_INTERFACE_MODE_RGMII,
config->supported_interfaces);
__set_bit(PHY_INTERFACE_MODE_TRGMII,
@@ -2548,19 +2553,24 @@ static void mt7531_mac_port_get_caps(struct dsa_switch *ds, int port,
struct mt7530_priv *priv = ds->priv;
switch (port) {
- case 0 ... 4: /* Internal phy */
+ /* Internal PHY */
+ case 0 ... 4:
__set_bit(PHY_INTERFACE_MODE_GMII,
config->supported_interfaces);
break;
- case 5: /* 2nd cpu port supports either rgmii or sgmii/8023z */
+ /* Port 5 which can be used as a CPU port supports rgmii with delays on
+ * MT7531BE, sgmii/802.3z on MT7531AE.
+ */
+ case 5:
if (!priv->p5_sgmii) {
phy_interface_set_rgmii(config->supported_interfaces);
break;
}
fallthrough;
- case 6: /* 1st cpu port supports sgmii/8023z only */
+ /* Port 6 which can be used as a CPU port supports sgmii/802.3z. */
+ case 6:
__set_bit(PHY_INTERFACE_MODE_SGMII,
config->supported_interfaces);
__set_bit(PHY_INTERFACE_MODE_1000BASEX,
@@ -2579,11 +2589,13 @@ static void mt7988_mac_port_get_caps(struct dsa_switch *ds, int port,
phy_interface_zero(config->supported_interfaces);
switch (port) {
- case 0 ... 4: /* Internal phy */
+ /* Internal PHY */
+ case 0 ... 4:
__set_bit(PHY_INTERFACE_MODE_INTERNAL,
config->supported_interfaces);
break;
+ /* Port 6 which can be used as a CPU port is an internal 10G port. */
case 6:
__set_bit(PHY_INTERFACE_MODE_INTERNAL,
config->supported_interfaces);
@@ -2747,12 +2759,12 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
u32 mcr_cur, mcr_new;
switch (port) {
- case 0 ... 4: /* Internal phy */
+ case 0 ... 4:
if (state->interface != PHY_INTERFACE_MODE_GMII &&
state->interface != PHY_INTERFACE_MODE_INTERNAL)
goto unsupported;
break;
- case 5: /* 2nd cpu port with phy of port 0 or 4 / external phy */
+ case 5:
if (priv->p5_interface == state->interface)
break;
@@ -2762,7 +2774,7 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
if (priv->p5_intf_sel != P5_DISABLED)
priv->p5_interface = state->interface;
break;
- case 6: /* 1st cpu port */
+ case 6:
if (priv->p6_interface == state->interface)
break;
--
2.40.1
Introduce the p5_sgmii field to store the information for whether port 5
has got SGMII or not. Instead of reading the MT7531_TOP_SIG_SR register
multiple times, the register will be read once and the value will be
stored on the p5_sgmii field. This saves unnecessary reads of the
register.
Move the comment about MT7531AE and MT7531BE to mt7531_setup(), where the
switch is identified.
Get rid of mt7531_dual_sgmii_supported() now that priv->p5_sgmii stores the
information. Address the code where mt7531_dual_sgmii_supported() is used.
Get rid of mt7531_is_rgmii_port() which just prints the opposite of
priv->p5_sgmii.
Instead of calling mt7531_pll_setup() then returning, do not call it if
port 5 is SGMII.
Remove P5_INTF_SEL_GMAC5_SGMII. The p5_interface_select enum is supposed to
represent the mode that port 5 is being used in, not the hardware
information of port 5. Set p5_intf_sel to P5_INTF_SEL_GMAC5 instead, if
port 5 is not dsa_is_unused_port().
Signed-off-by: Arınç ÜNAL <[email protected]>
Acked-by: Daniel Golle <[email protected]>
---
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 442492d62670..45c9698ad9dd 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -487,15 +487,6 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
return 0;
}
-static bool mt7531_dual_sgmii_supported(struct mt7530_priv *priv)
-{
- u32 val;
-
- val = mt7530_read(priv, MT7531_TOP_SIG_SR);
-
- return (val & PAD_DUAL_SGMII_EN) != 0;
-}
-
static int
mt7531_pad_setup(struct dsa_switch *ds, phy_interface_t interface)
{
@@ -510,9 +501,6 @@ mt7531_pll_setup(struct mt7530_priv *priv)
u32 xtal;
u32 val;
- if (mt7531_dual_sgmii_supported(priv))
- return;
-
val = mt7530_read(priv, MT7531_CREV);
top_sig = mt7530_read(priv, MT7531_TOP_SIG_SR);
hwstrap = mt7530_read(priv, MT7531_HWTRAP);
@@ -920,8 +908,6 @@ static const char *p5_intf_modes(unsigned int p5_interface)
return "PHY P4";
case P5_INTF_SEL_GMAC5:
return "GMAC5";
- case P5_INTF_SEL_GMAC5_SGMII:
- return "GMAC5_SGMII";
default:
return "unknown";
}
@@ -2470,6 +2456,12 @@ mt7531_setup(struct dsa_switch *ds)
return -ENODEV;
}
+ /* MT7531AE has got two SGMII units. One for port 5, one for port 6.
+ * MT7531BE has got only one SGMII unit which is for port 6.
+ */
+ val = mt7530_read(priv, MT7531_TOP_SIG_SR);
+ priv->p5_sgmii = !!(val & PAD_DUAL_SGMII_EN);
+
/* all MACs must be forced link-down before sw reset */
for (i = 0; i < MT7530_NUM_PORTS; i++)
mt7530_write(priv, MT7530_PMCR_P(i), MT7531_FORCE_LNK);
@@ -2479,21 +2471,18 @@ mt7531_setup(struct dsa_switch *ds)
SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
SYS_CTRL_REG_RST);
- mt7531_pll_setup(priv);
-
- if (mt7531_dual_sgmii_supported(priv)) {
- priv->p5_intf_sel = P5_INTF_SEL_GMAC5_SGMII;
-
+ if (!priv->p5_sgmii) {
+ mt7531_pll_setup(priv);
+ } else {
/* Let ds->user_mii_bus be able to access external phy. */
mt7530_rmw(priv, MT7531_GPIO_MODE1, MT7531_GPIO11_RG_RXD2_MASK,
MT7531_EXT_P_MDC_11);
mt7530_rmw(priv, MT7531_GPIO_MODE1, MT7531_GPIO12_RG_RXD3_MASK,
MT7531_EXT_P_MDIO_12);
- } else {
- priv->p5_intf_sel = P5_INTF_SEL_GMAC5;
}
- dev_dbg(ds->dev, "P5 support %s interface\n",
- p5_intf_modes(priv->p5_intf_sel));
+
+ if (!dsa_is_unused_port(ds, 5))
+ priv->p5_intf_sel = P5_INTF_SEL_GMAC5;
mt7530_rmw(priv, MT7531_GPIO_MODE0, MT7531_GPIO0_MASK,
MT7531_GPIO0_INTERRUPT);
@@ -2553,11 +2542,6 @@ static void mt7530_mac_port_get_caps(struct dsa_switch *ds, int port,
}
}
-static bool mt7531_is_rgmii_port(struct mt7530_priv *priv, u32 port)
-{
- return (port == 5) && (priv->p5_intf_sel != P5_INTF_SEL_GMAC5_SGMII);
-}
-
static void mt7531_mac_port_get_caps(struct dsa_switch *ds, int port,
struct phylink_config *config)
{
@@ -2570,7 +2554,7 @@ static void mt7531_mac_port_get_caps(struct dsa_switch *ds, int port,
break;
case 5: /* 2nd cpu port supports either rgmii or sgmii/8023z */
- if (mt7531_is_rgmii_port(priv, port)) {
+ if (!priv->p5_sgmii) {
phy_interface_set_rgmii(config->supported_interfaces);
break;
}
@@ -2637,7 +2621,7 @@ static int mt7531_rgmii_setup(struct mt7530_priv *priv, u32 port,
{
u32 val;
- if (!mt7531_is_rgmii_port(priv, port)) {
+ if (priv->p5_sgmii) {
dev_err(priv->dev, "RGMII mode is not available for port %d\n",
port);
return -EINVAL;
@@ -2881,7 +2865,7 @@ mt7531_cpu_port_config(struct dsa_switch *ds, int port)
switch (port) {
case 5:
- if (mt7531_is_rgmii_port(priv, port))
+ if (!priv->p5_sgmii)
interface = PHY_INTERFACE_MODE_RGMII;
else
interface = PHY_INTERFACE_MODE_2500BASEX;
@@ -3033,7 +3017,7 @@ mt753x_setup(struct dsa_switch *ds)
mt7530_free_irq_common(priv);
if (priv->create_sgmii) {
- ret = priv->create_sgmii(priv, mt7531_dual_sgmii_supported(priv));
+ ret = priv->create_sgmii(priv);
if (ret && priv->irq)
mt7530_free_irq(priv);
}
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 1b10b70c1508..12c1731d6201 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -687,7 +687,6 @@ enum p5_interface_select {
P5_INTF_SEL_PHY_P0,
P5_INTF_SEL_PHY_P4,
P5_INTF_SEL_GMAC5,
- P5_INTF_SEL_GMAC5_SGMII,
};
struct mt7530_priv;
@@ -756,6 +755,8 @@ struct mt753x_info {
* registers
* @p6_interface Holding the current port 6 interface
* @p5_intf_sel: Holding the current port 5 interface select
+ * @p5_sgmii: Flag for distinguishing if port 5 of the MT7531 switch
+ * has got SGMII
* @irq: IRQ number of the switch
* @irq_domain: IRQ domain of the switch irq_chip
* @irq_enable: IRQ enable bits, synced to SYS_INT_EN
@@ -777,6 +778,7 @@ struct mt7530_priv {
phy_interface_t p6_interface;
phy_interface_t p5_interface;
enum p5_interface_select p5_intf_sel;
+ bool p5_sgmii;
u8 mirror_rx;
u8 mirror_tx;
struct mt7530_port ports[MT7530_NUM_PORTS];
@@ -786,7 +788,7 @@ struct mt7530_priv {
int irq;
struct irq_domain *irq_domain;
u32 irq_enable;
- int (*create_sgmii)(struct mt7530_priv *priv, bool dual_sgmii);
+ int (*create_sgmii)(struct mt7530_priv *priv);
unsigned long active_cpu_ports;
};
--
2.40.1
There're two code paths for setting up port 5:
mt7530_setup()
-> mt7530_setup_port5()
mt753x_phylink_mac_config()
-> mt753x_mac_config()
-> mt7530_mac_config()
-> mt7530_setup_port5()
Currently mt7530_setup_port5() from mt7530_setup() always runs. If port 5
is used as a CPU, DSA, or user port, mt7530_setup_port5() from
mt753x_phylink_mac_config() won't run. That is because priv->p5_interface
set on mt7530_setup_port5() will match state->interface on
mt753x_phylink_mac_config() which will stop running mt7530_setup_port5()
again.
Therefore, mt7530_setup_port5() will never run from
mt753x_phylink_mac_config().
Address this by not running mt7530_setup_port5() from mt7530_setup() if
port 5 is used as a CPU, DSA, or user port. This driver isn't in the
dsa_switches_apply_workarounds[] array so phylink will always be present.
For the cases of PHY muxing or the port being disabled, call
mt7530_setup_port5() from mt7530_setup(). 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.
Do not set priv->p5_intf_sel to P5_DISABLED. It is already set to that when
"priv" is allocated.
Move setting the interface to a more specific location. It's supposed to be
overwritten if PHY muxing is detected.
Improve the comment which explains the process.
Signed-off-by: Arınç ÜNAL <[email protected]>
---
drivers/net/dsa/mt7530.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 8623742b35ee..069b3dfca6fa 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2308,16 +2308,15 @@ mt7530_setup(struct dsa_switch *ds)
return ret;
/* Setup port 5 */
- priv->p5_intf_sel = P5_DISABLED;
- interface = PHY_INTERFACE_MODE_NA;
-
if (!dsa_is_unused_port(ds, 5)) {
priv->p5_intf_sel = P5_INTF_SEL_GMAC5;
- ret = of_get_phy_mode(dsa_to_port(ds, 5)->dn, &interface);
- if (ret && ret != -ENODEV)
- return ret;
} else {
- /* Scan the ethernet nodes. look for GMAC1, lookup used phy */
+ /* Scan the ethernet nodes. Look for GMAC1, lookup the used PHY.
+ * Set priv->p5_intf_sel to the appropriate value if PHY muxing
+ * is detected.
+ */
+ interface = PHY_INTERFACE_MODE_NA;
+
for_each_child_of_node(dn, mac_np) {
if (!of_device_is_compatible(mac_np,
"mediatek,eth-mac"))
@@ -2348,6 +2347,8 @@ mt7530_setup(struct dsa_switch *ds)
of_node_put(phy_node);
break;
}
+
+ mt7530_setup_port5(ds, interface);
}
#ifdef CONFIG_GPIOLIB
@@ -2358,8 +2359,6 @@ mt7530_setup(struct dsa_switch *ds)
}
#endif /* CONFIG_GPIOLIB */
- mt7530_setup_port5(ds, interface);
-
/* Flush the FDB table */
ret = mt7530_fdb_cmd(priv, MT7530_FDB_FLUSH, NULL);
if (ret < 0)
--
2.40.1
Do not set priv->p5_interface on mt7530_setup_port5(). There isn't a case
where mt753x_phylink_mac_config() runs after mt7530_setup_port5() which
setting priv->p5_interface would prevent mt7530_setup_port5() from running
more than once.
Signed-off-by: Arınç ÜNAL <[email protected]>
---
drivers/net/dsa/mt7530.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 069b3dfca6fa..fc87ec817672 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -978,8 +978,6 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
dev_dbg(ds->dev, "Setup P5, HWTRAP=0x%x, intf_sel=%s, phy-mode=%s\n",
val, p5_intf_modes(priv->p5_intf_sel), phy_modes(interface));
- priv->p5_interface = interface;
-
unlock_exit:
mutex_unlock(&priv->reg_mutex);
}
--
2.40.1
There's no need to run all the code on mt7530_setup_port5() if port 5 is
disabled. The only case for calling mt7530_setup_port5() from
mt7530_setup() is when PHY muxing is enabled. That is because port 5 is not
defined as a port on the devicetree, therefore, it cannot be controlled by
phylink.
Because of this, run mt7530_setup_port5() if priv->p5_intf_sel is
P5_INTF_SEL_PHY_P0 or P5_INTF_SEL_PHY_P4. Remove the P5_DISABLED case from
mt7530_setup_port5().
Stop initialising the interface variable as the remaining cases will always
call mt7530_setup_port5() with it initialised.
Signed-off-by: Arınç ÜNAL <[email protected]>
Reviewed-by: Vladimir Oltean <[email protected]>
---
drivers/net/dsa/mt7530.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index fc87ec817672..1aab4c3f28b0 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -942,9 +942,6 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
/* MT7530_P5_MODE_GMAC: P5 -> External phy or 2nd GMAC */
val &= ~MHWTRAP_P5_DIS;
break;
- case P5_DISABLED:
- interface = PHY_INTERFACE_MODE_NA;
- break;
default:
dev_err(ds->dev, "Unsupported p5_intf_sel %d\n",
priv->p5_intf_sel);
@@ -2313,8 +2310,6 @@ mt7530_setup(struct dsa_switch *ds)
* Set priv->p5_intf_sel to the appropriate value if PHY muxing
* is detected.
*/
- interface = PHY_INTERFACE_MODE_NA;
-
for_each_child_of_node(dn, mac_np) {
if (!of_device_is_compatible(mac_np,
"mediatek,eth-mac"))
@@ -2346,7 +2341,9 @@ mt7530_setup(struct dsa_switch *ds)
break;
}
- mt7530_setup_port5(ds, interface);
+ if (priv->p5_intf_sel == P5_INTF_SEL_PHY_P0 ||
+ priv->p5_intf_sel == P5_INTF_SEL_PHY_P4)
+ mt7530_setup_port5(ds, interface);
}
#ifdef CONFIG_GPIOLIB
--
2.40.1
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()
Signed-off-by: Arınç ÜNAL <[email protected]>
Reviewed-by: Vladimir Oltean <[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 7de55cbf19a4..ae037ff7d4c5 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -415,7 +415,7 @@ mt753x_preferred_default_local_cpu_port(struct dsa_switch *ds)
/* 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;
@@ -487,6 +487,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)
{
@@ -2608,12 +2614,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.40.1
There're two code paths for setting up port 5:
mt7530_setup()
-> mt7530_setup_port5()
mt753x_phylink_mac_config()
-> mt753x_mac_config()
-> mt7530_mac_config()
-> mt7530_setup_port5()
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.
Empty the default case which will never run but is needed nonetheless to
handle all the remaining enumeration values.
Signed-off-by: Arınç ÜNAL <[email protected]>
Reviewed-by: Vladimir Oltean <[email protected]>
---
drivers/net/dsa/mt7530.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 1aab4c3f28b0..7de55cbf19a4 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -943,9 +943,7 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
val &= ~MHWTRAP_P5_DIS;
break;
default:
- dev_err(ds->dev, "Unsupported p5_intf_sel %d\n",
- priv->p5_intf_sel);
- goto unlock_exit;
+ break;
}
/* Setup RGMII settings */
@@ -975,7 +973,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));
-unlock_exit:
mutex_unlock(&priv->reg_mutex);
}
--
2.40.1
Use the p5_interface_select enumeration as the data type for the
p5_intf_sel field. This ensures p5_intf_sel can only take the values
defined in the p5_interface_select enumeration.
Remove the explicit assignment of 0 to P5_DISABLED as the first enum item
is automatically assigned 0.
Signed-off-by: Arınç ÜNAL <[email protected]>
Acked-by: Daniel Golle <[email protected]>
---
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 96d610f5bcf9..1b10b70c1508 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -683,7 +683,7 @@ struct mt7530_port {
/* Port 5 interface select definitions */
enum p5_interface_select {
- P5_DISABLED = 0,
+ P5_DISABLED,
P5_INTF_SEL_PHY_P0,
P5_INTF_SEL_PHY_P4,
P5_INTF_SEL_GMAC5,
@@ -776,7 +776,7 @@ struct mt7530_priv {
bool mcm;
phy_interface_t p6_interface;
phy_interface_t p5_interface;
- unsigned int p5_intf_sel;
+ enum p5_interface_select p5_intf_sel;
u8 mirror_rx;
u8 mirror_tx;
struct mt7530_port ports[MT7530_NUM_PORTS];
--
2.40.1
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.
Signed-off-by: Arınç ÜNAL <[email protected]>
Acked-by: Daniel Golle <[email protected]>
Reviewed-by: Vladimir Oltean <[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 ae037ff7d4c5..efe5ffe3455d 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -487,18 +487,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)
{
@@ -2601,14 +2589,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)
@@ -2778,8 +2758,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;
@@ -3092,11 +3070,6 @@ mt753x_conduit_state_change(struct dsa_switch *ds,
CPU_PORT(__ffs(priv->active_cpu_ports)));
}
-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;
@@ -3160,7 +3133,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,
},
@@ -3172,7 +3144,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,
},
@@ -3184,7 +3155,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,
@@ -3197,7 +3167,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,
@@ -3227,9 +3196,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 12c1731d6201..aabe7f6cffeb 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -704,8 +704,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
@@ -726,7 +724,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.40.1
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(). Drop the unnecessary function printing.
Signed-off-by: Arınç ÜNAL <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
---
drivers/net/dsa/mt7530.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index efe5ffe3455d..167b340350b3 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -422,13 +422,6 @@ mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
xtal = mt7530_read(priv, MT7530_MHWTRAP) & 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;
@@ -2235,6 +2228,12 @@ mt7530_setup(struct dsa_switch *ds)
return -ENODEV;
}
+ if ((val & HWTRAP_XTAL_MASK) == HWTRAP_XTAL_20MHZ) {
+ dev_err(priv->dev,
+ "MT7530 with a 20MHz XTAL is not supported!\n");
+ return -EINVAL;
+ }
+
/* Reset the switch through internal reset */
mt7530_write(priv, MT7530_SYS_CTRL,
SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
--
2.40.1
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'.
Signed-off-by: Arınç ÜNAL <[email protected]>
Acked-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 f36f240231b5..ca42005ff3a9 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2562,7 +2562,7 @@ static void mt7988_mac_port_get_caps(struct dsa_switch *ds, int port,
switch (port) {
/* Internal PHY */
- case 0 ... 4:
+ case 0 ... 3:
__set_bit(PHY_INTERFACE_MODE_INTERNAL,
config->supported_interfaces);
break;
--
2.40.1
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.
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 167b340350b3..2608b09d3295 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -420,6 +420,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_MHWTRAP) & HWTRAP_XTAL_MASK;
switch (interface) {
@@ -910,7 +912,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) {
@@ -2250,9 +2252,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.40.1
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 statement for RGMII and return which simplifies the code and
saves an indent.
Do not set P6_INTF_MODE, which is the the three least significant bits of
the MT7530_P6ECR register, to 0 for RGMII as it will already be 0 after
reset.
Read XTAL after checking for RGMII as it's only needed for the TRGMII
interface mode.
Change mt7530_setup_port6() to void now that there're no error cases left.
Signed-off-by: Arınç ÜNAL <[email protected]>
---
drivers/net/dsa/mt7530.c | 100 ++++++++++++++++-----------------------
1 file changed, 40 insertions(+), 60 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 2608b09d3295..f36f240231b5 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -414,72 +414,56 @@ mt753x_preferred_default_local_cpu_port(struct dsa_switch *ds)
}
/* 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);
+ if (interface == PHY_INTERFACE_MODE_RGMII)
+ return;
+
+ mt7530_rmw(priv, MT7530_P6ECR, P6_INTF_MODE_MASK, P6_INTF_MODE(1));
+
xtal = mt7530_read(priv, MT7530_MHWTRAP) & HWTRAP_XTAL_MASK;
- switch (interface) {
- case PHY_INTERFACE_MODE_RGMII:
- trgint = 0;
- break;
- case PHY_INTERFACE_MODE_TRGMII:
- trgint = 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)
+ ncpo1 = 0x0640;
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)
- ncpo1 = 0x0640;
- if (xtal == HWTRAP_XTAL_25MHZ)
- ncpo1 = 0x0a00;
- } else { /* PLL frequency: 250MHz: 2.0Gbit */
- if (xtal == HWTRAP_XTAL_40MHZ)
- ncpo1 = 0x0c80;
- if (xtal == HWTRAP_XTAL_25MHZ)
- ncpo1 = 0x1400;
- }
- break;
- default:
- dev_err(priv->dev, "xMII interface %d not supported\n",
- interface);
- return -EINVAL;
+ ncpo1 = 0x0a00;
+ } else { /* PLL frequency: 250MHz: 2.0Gbit */
+ if (xtal == HWTRAP_XTAL_40MHZ)
+ ncpo1 = 0x0c80;
+ if (xtal == HWTRAP_XTAL_25MHZ)
+ ncpo1 = 0x1400;
}
- 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);
-
- /* Setup the MT7530 TRGMII Tx Clock */
- core_write(priv, CORE_PLL_GROUP5, RG_LCDDS_PCW_NCPO1(ncpo1));
- core_write(priv, CORE_PLL_GROUP6, RG_LCDDS_PCW_NCPO0(0));
- core_write(priv, CORE_PLL_GROUP10, RG_LCDDS_SSC_DELTA(ssc_delta));
- core_write(priv, CORE_PLL_GROUP11, RG_LCDDS_SSC_DELTA1(ssc_delta));
- core_write(priv, CORE_PLL_GROUP4,
- RG_SYSPLL_DDSFBK_EN | RG_SYSPLL_BIAS_EN |
- RG_SYSPLL_BIAS_LPF_EN);
- core_write(priv, CORE_PLL_GROUP2,
- RG_SYSPLL_EN_NORMAL | RG_SYSPLL_VODEN |
- RG_SYSPLL_POSDIV(1));
- core_write(priv, CORE_PLL_GROUP7,
- RG_LCDDS_PCW_NCPO_CHG | RG_LCCDS_C(3) |
- RG_LCDDS_PWDB | RG_LCDDS_ISO_EN);
+ /* Disable the MT7530 TRGMII clocks */
+ core_clear(priv, CORE_TRGMII_GSW_CLK_CG, REG_TRGMIICK_EN);
- /* Enable the MT7530 TRGMII clocks */
- core_set(priv, CORE_TRGMII_GSW_CLK_CG, REG_TRGMIICK_EN);
- }
+ /* Setup the MT7530 TRGMII Tx Clock */
+ core_write(priv, CORE_PLL_GROUP5, RG_LCDDS_PCW_NCPO1(ncpo1));
+ core_write(priv, CORE_PLL_GROUP6, RG_LCDDS_PCW_NCPO0(0));
+ core_write(priv, CORE_PLL_GROUP10, RG_LCDDS_SSC_DELTA(ssc_delta));
+ core_write(priv, CORE_PLL_GROUP11, RG_LCDDS_SSC_DELTA1(ssc_delta));
+ core_write(priv, CORE_PLL_GROUP4, RG_SYSPLL_DDSFBK_EN |
+ RG_SYSPLL_BIAS_EN | RG_SYSPLL_BIAS_LPF_EN);
+ core_write(priv, CORE_PLL_GROUP2, RG_SYSPLL_EN_NORMAL |
+ RG_SYSPLL_VODEN | RG_SYSPLL_POSDIV(1));
+ core_write(priv, CORE_PLL_GROUP7, RG_LCDDS_PCW_NCPO_CHG |
+ RG_LCCDS_C(3) | RG_LCDDS_PWDB | RG_LCDDS_ISO_EN);
- return 0;
+ /* Enable the MT7530 TRGMII clocks */
+ core_set(priv, CORE_TRGMII_GSW_CLK_CG, REG_TRGMIICK_EN);
}
static void
@@ -2597,15 +2581,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) {
+ if (port == 5)
mt7530_setup_port5(priv->ds, interface);
- } else if (port == 6) {
- ret = mt7530_setup_port6(priv->ds, interface);
- if (ret)
- return ret;
- }
+ else if (port == 6)
+ mt7530_setup_port6(priv->ds, interface);
return 0;
}
--
2.40.1
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 | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index ca42005ff3a9..20ae147b823e 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2558,8 +2558,6 @@ 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) {
/* Internal PHY */
case 0 ... 3:
--
2.40.1
On Sat, Nov 18, 2023 at 03:31:55PM +0300, Arınç ÜNAL wrote:
> There're two code paths for setting up port 5:
>
> mt7530_setup()
> -> mt7530_setup_port5()
>
> mt753x_phylink_mac_config()
> -> mt753x_mac_config()
> -> mt7530_mac_config()
> -> mt7530_setup_port5()
>
> Currently mt7530_setup_port5() from mt7530_setup() always runs. If port 5
> is used as a CPU, DSA, or user port, mt7530_setup_port5() from
> mt753x_phylink_mac_config() won't run. That is because priv->p5_interface
> set on mt7530_setup_port5() will match state->interface on
> mt753x_phylink_mac_config() which will stop running mt7530_setup_port5()
> again.
>
> Therefore, mt7530_setup_port5() will never run from
> mt753x_phylink_mac_config().
>
> Address this by not running mt7530_setup_port5() from mt7530_setup() if
> port 5 is used as a CPU, DSA, or user port. This driver isn't in the
> dsa_switches_apply_workarounds[] array so phylink will always be present.
>
> For the cases of PHY muxing or the port being disabled, call
> mt7530_setup_port5() from mt7530_setup(). 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.
... and this should state why this needs to happen - in other words,
the commit message should state why is it critical that port 5 is
always setup.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Sat, Nov 18, 2023 at 03:31:52PM +0300, Arınç ÜNAL wrote:
> Use the p5_interface_select enumeration as the data type for the
> p5_intf_sel field. This ensures p5_intf_sel can only take the values
> defined in the p5_interface_select enumeration.
>
> Remove the explicit assignment of 0 to P5_DISABLED as the first enum item
> is automatically assigned 0.
>
> Signed-off-by: Arınç ÜNAL <[email protected]>
> Acked-by: Daniel Golle <[email protected]>
> ---
Reviewed-by: Vladimir Oltean <[email protected]>
On Sat, Nov 18, 2023 at 03:31:53PM +0300, Arınç ÜNAL wrote:
> Introduce the p5_sgmii field to store the information for whether port 5
> has got SGMII or not. Instead of reading the MT7531_TOP_SIG_SR register
> multiple times, the register will be read once and the value will be
> stored on the p5_sgmii field. This saves unnecessary reads of the
> register.
>
> Move the comment about MT7531AE and MT7531BE to mt7531_setup(), where the
> switch is identified.
>
> Get rid of mt7531_dual_sgmii_supported() now that priv->p5_sgmii stores the
> information. Address the code where mt7531_dual_sgmii_supported() is used.
>
> Get rid of mt7531_is_rgmii_port() which just prints the opposite of
> priv->p5_sgmii.
>
> Instead of calling mt7531_pll_setup() then returning, do not call it if
> port 5 is SGMII.
>
> Remove P5_INTF_SEL_GMAC5_SGMII. The p5_interface_select enum is supposed to
> represent the mode that port 5 is being used in, not the hardware
> information of port 5. Set p5_intf_sel to P5_INTF_SEL_GMAC5 instead, if
> port 5 is not dsa_is_unused_port().
>
> Signed-off-by: Arınç ÜNAL <[email protected]>
> Acked-by: Daniel Golle <[email protected]>
> ---
Reviewed-by: Vladimir Oltean <[email protected]>
On Sat, Nov 18, 2023 at 03:31:57PM +0300, Arınç ÜNAL wrote:
> There's no need to run all the code on mt7530_setup_port5() if port 5 is
> disabled. The only case for calling mt7530_setup_port5() from
> mt7530_setup() is when PHY muxing is enabled. That is because port 5 is not
> defined as a port on the devicetree, therefore, it cannot be controlled by
> phylink.
>
> Because of this, run mt7530_setup_port5() if priv->p5_intf_sel is
> P5_INTF_SEL_PHY_P0 or P5_INTF_SEL_PHY_P4. Remove the P5_DISABLED case from
> mt7530_setup_port5().
>
> Stop initialising the interface variable as the remaining cases will always
> call mt7530_setup_port5() with it initialised.
>
> Signed-off-by: Arınç ÜNAL <[email protected]>
> Reviewed-by: Vladimir Oltean <[email protected]>
> ---
> drivers/net/dsa/mt7530.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index fc87ec817672..1aab4c3f28b0 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -942,9 +942,6 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
> /* MT7530_P5_MODE_GMAC: P5 -> External phy or 2nd GMAC */
> val &= ~MHWTRAP_P5_DIS;
> break;
> - case P5_DISABLED:
> - interface = PHY_INTERFACE_MODE_NA;
> - break;
> default:
> dev_err(ds->dev, "Unsupported p5_intf_sel %d\n",
> priv->p5_intf_sel);
> @@ -2313,8 +2310,6 @@ mt7530_setup(struct dsa_switch *ds)
> * Set priv->p5_intf_sel to the appropriate value if PHY muxing
> * is detected.
> */
> - interface = PHY_INTERFACE_MODE_NA;
> -
> for_each_child_of_node(dn, mac_np) {
> if (!of_device_is_compatible(mac_np,
> "mediatek,eth-mac"))
> @@ -2346,7 +2341,9 @@ mt7530_setup(struct dsa_switch *ds)
> break;
> }
>
> - mt7530_setup_port5(ds, interface);
> + if (priv->p5_intf_sel == P5_INTF_SEL_PHY_P0 ||
> + priv->p5_intf_sel == P5_INTF_SEL_PHY_P4)
> + mt7530_setup_port5(ds, interface);
Hi Arınç,
It appears that interface is now uninitialised here.
Flagged by Smatch.
> }
>
> #ifdef CONFIG_GPIOLIB
> --
> 2.40.1
>
On 18.11.2023 17:41, Russell King (Oracle) wrote:
> On Sat, Nov 18, 2023 at 03:31:55PM +0300, Arınç ÜNAL wrote:
>> There're two code paths for setting up port 5:
>>
>> mt7530_setup()
>> -> mt7530_setup_port5()
>>
>> mt753x_phylink_mac_config()
>> -> mt753x_mac_config()
>> -> mt7530_mac_config()
>> -> mt7530_setup_port5()
>>
>> Currently mt7530_setup_port5() from mt7530_setup() always runs. If port 5
>> is used as a CPU, DSA, or user port, mt7530_setup_port5() from
>> mt753x_phylink_mac_config() won't run. That is because priv->p5_interface
>> set on mt7530_setup_port5() will match state->interface on
>> mt753x_phylink_mac_config() which will stop running mt7530_setup_port5()
>> again.
>>
>> Therefore, mt7530_setup_port5() will never run from
>> mt753x_phylink_mac_config().
>>
>> Address this by not running mt7530_setup_port5() from mt7530_setup() if
>> port 5 is used as a CPU, DSA, or user port. This driver isn't in the
>> dsa_switches_apply_workarounds[] array so phylink will always be present.
>>
>> For the cases of PHY muxing or the port being disabled, call
>> mt7530_setup_port5() from mt7530_setup(). 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.
>
> ... and this should state why this needs to happen - in other words,
> the commit message should state why is it critical that port 5 is
> always setup.
Actually, port 5 must not always be setup. With patch 7, I explain this
while preventing mt7530_setup_port5() from running if port 5 is disabled.
Arınç
Hi Simon.
On 21.11.2023 21:53, Simon Horman wrote:
> On Sat, Nov 18, 2023 at 03:31:57PM +0300, Arınç ÜNAL wrote:
>> There's no need to run all the code on mt7530_setup_port5() if port 5 is
>> disabled. The only case for calling mt7530_setup_port5() from
>> mt7530_setup() is when PHY muxing is enabled. That is because port 5 is not
>> defined as a port on the devicetree, therefore, it cannot be controlled by
>> phylink.
>>
>> Because of this, run mt7530_setup_port5() if priv->p5_intf_sel is
>> P5_INTF_SEL_PHY_P0 or P5_INTF_SEL_PHY_P4. Remove the P5_DISABLED case from
>> mt7530_setup_port5().
>>
>> Stop initialising the interface variable as the remaining cases will always
>> call mt7530_setup_port5() with it initialised.
>>
>> Signed-off-by: Arınç ÜNAL <[email protected]>
>> Reviewed-by: Vladimir Oltean <[email protected]>
>> ---
>> drivers/net/dsa/mt7530.c | 9 +++------
>> 1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index fc87ec817672..1aab4c3f28b0 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -942,9 +942,6 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
>> /* MT7530_P5_MODE_GMAC: P5 -> External phy or 2nd GMAC */
>> val &= ~MHWTRAP_P5_DIS;
>> break;
>> - case P5_DISABLED:
>> - interface = PHY_INTERFACE_MODE_NA;
>> - break;
>> default:
>> dev_err(ds->dev, "Unsupported p5_intf_sel %d\n",
>> priv->p5_intf_sel);
>> @@ -2313,8 +2310,6 @@ mt7530_setup(struct dsa_switch *ds)
>> * Set priv->p5_intf_sel to the appropriate value if PHY muxing
>> * is detected.
>> */
>> - interface = PHY_INTERFACE_MODE_NA;
>> -
>> for_each_child_of_node(dn, mac_np) {
>> if (!of_device_is_compatible(mac_np,
>> "mediatek,eth-mac"))
>> @@ -2346,7 +2341,9 @@ mt7530_setup(struct dsa_switch *ds)
>> break;
>> }
>>
>> - mt7530_setup_port5(ds, interface);
>> + if (priv->p5_intf_sel == P5_INTF_SEL_PHY_P0 ||
>> + priv->p5_intf_sel == P5_INTF_SEL_PHY_P4)
>> + mt7530_setup_port5(ds, interface);
>
> Hi Arınç,
>
> It appears that interface is now uninitialised here.
>
> Flagged by Smatch.
I'm not sure why it doesn't catch that for mt7530_setup_port5() to run
here, priv->p5_intf_sel must be either P5_INTF_SEL_PHY_P0 or
P5_INTF_SEL_PHY_P4. And for that to happen, the interface variable will be
initialised.
for_each_child_of_node(dn, mac_np) {
if (!of_device_is_compatible(mac_np,
"mediatek,eth-mac"))
continue;
ret = of_property_read_u32(mac_np, "reg", &id);
if (ret < 0 || id != 1)
continue;
phy_node = of_parse_phandle(mac_np, "phy-handle", 0);
if (!phy_node)
continue;
if (phy_node->parent == priv->dev->of_node->parent) {
ret = of_get_phy_mode(mac_np, &interface);
if (ret && ret != -ENODEV) {
of_node_put(mac_np);
of_node_put(phy_node);
return ret;
}
id = of_mdio_parse_addr(ds->dev, phy_node);
if (id == 0)
priv->p5_intf_sel = P5_INTF_SEL_PHY_P0;
if (id == 4)
priv->p5_intf_sel = P5_INTF_SEL_PHY_P4;
}
of_node_put(mac_np);
of_node_put(phy_node);
break;
}
if (priv->p5_intf_sel == P5_INTF_SEL_PHY_P0 ||
priv->p5_intf_sel == P5_INTF_SEL_PHY_P4)
mt7530_setup_port5(ds, interface);
Arınç
Can I receive reviews for patches 6, 12, 13, 14, and 15?
Thanks.
Arınç
On Sat, Dec 02, 2023 at 11:45:42AM +0300, Arınç ÜNAL wrote:
> Hi Simon.
>
> On 21.11.2023 21:53, Simon Horman wrote:
> > On Sat, Nov 18, 2023 at 03:31:57PM +0300, Arınç ÜNAL wrote:
> > > There's no need to run all the code on mt7530_setup_port5() if port 5 is
> > > disabled. The only case for calling mt7530_setup_port5() from
> > > mt7530_setup() is when PHY muxing is enabled. That is because port 5 is not
> > > defined as a port on the devicetree, therefore, it cannot be controlled by
> > > phylink.
> > >
> > > Because of this, run mt7530_setup_port5() if priv->p5_intf_sel is
> > > P5_INTF_SEL_PHY_P0 or P5_INTF_SEL_PHY_P4. Remove the P5_DISABLED case from
> > > mt7530_setup_port5().
> > >
> > > Stop initialising the interface variable as the remaining cases will always
> > > call mt7530_setup_port5() with it initialised.
> > >
> > > Signed-off-by: Arınç ÜNAL <[email protected]>
> > > Reviewed-by: Vladimir Oltean <[email protected]>
> > > ---
> > > drivers/net/dsa/mt7530.c | 9 +++------
> > > 1 file changed, 3 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> > > index fc87ec817672..1aab4c3f28b0 100644
> > > --- a/drivers/net/dsa/mt7530.c
> > > +++ b/drivers/net/dsa/mt7530.c
> > > @@ -942,9 +942,6 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
> > > /* MT7530_P5_MODE_GMAC: P5 -> External phy or 2nd GMAC */
> > > val &= ~MHWTRAP_P5_DIS;
> > > break;
> > > - case P5_DISABLED:
> > > - interface = PHY_INTERFACE_MODE_NA;
> > > - break;
> > > default:
> > > dev_err(ds->dev, "Unsupported p5_intf_sel %d\n",
> > > priv->p5_intf_sel);
> > > @@ -2313,8 +2310,6 @@ mt7530_setup(struct dsa_switch *ds)
> > > * Set priv->p5_intf_sel to the appropriate value if PHY muxing
> > > * is detected.
> > > */
> > > - interface = PHY_INTERFACE_MODE_NA;
> > > -
> > > for_each_child_of_node(dn, mac_np) {
> > > if (!of_device_is_compatible(mac_np,
> > > "mediatek,eth-mac"))
> > > @@ -2346,7 +2341,9 @@ mt7530_setup(struct dsa_switch *ds)
> > > break;
> > > }
> > > - mt7530_setup_port5(ds, interface);
> > > + if (priv->p5_intf_sel == P5_INTF_SEL_PHY_P0 ||
> > > + priv->p5_intf_sel == P5_INTF_SEL_PHY_P4)
> > > + mt7530_setup_port5(ds, interface);
> >
> > Hi Arınç,
> >
> > It appears that interface is now uninitialised here.
> >
> > Flagged by Smatch.
>
> I'm not sure why it doesn't catch that for mt7530_setup_port5() to run
> here, priv->p5_intf_sel must be either P5_INTF_SEL_PHY_P0 or
> P5_INTF_SEL_PHY_P4. And for that to happen, the interface variable will be
> initialised.
It's probably due to the complexities involved in analysing the values
of variables, especially when they're in structures that are passed in.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
+ Dan Carpenter <[email protected]>
On Sat, Dec 02, 2023 at 11:45:42AM +0300, Arınç ÜNAL wrote:
> Hi Simon.
>
> On 21.11.2023 21:53, Simon Horman wrote:
> > On Sat, Nov 18, 2023 at 03:31:57PM +0300, Arınç ÜNAL wrote:
> > > There's no need to run all the code on mt7530_setup_port5() if port 5 is
> > > disabled. The only case for calling mt7530_setup_port5() from
> > > mt7530_setup() is when PHY muxing is enabled. That is because port 5 is not
> > > defined as a port on the devicetree, therefore, it cannot be controlled by
> > > phylink.
> > >
> > > Because of this, run mt7530_setup_port5() if priv->p5_intf_sel is
> > > P5_INTF_SEL_PHY_P0 or P5_INTF_SEL_PHY_P4. Remove the P5_DISABLED case from
> > > mt7530_setup_port5().
> > >
> > > Stop initialising the interface variable as the remaining cases will always
> > > call mt7530_setup_port5() with it initialised.
> > >
> > > Signed-off-by: Arınç ÜNAL <[email protected]>
> > > Reviewed-by: Vladimir Oltean <[email protected]>
> > > ---
> > > drivers/net/dsa/mt7530.c | 9 +++------
> > > 1 file changed, 3 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> > > index fc87ec817672..1aab4c3f28b0 100644
> > > --- a/drivers/net/dsa/mt7530.c
> > > +++ b/drivers/net/dsa/mt7530.c
> > > @@ -942,9 +942,6 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
> > > /* MT7530_P5_MODE_GMAC: P5 -> External phy or 2nd GMAC */
> > > val &= ~MHWTRAP_P5_DIS;
> > > break;
> > > - case P5_DISABLED:
> > > - interface = PHY_INTERFACE_MODE_NA;
> > > - break;
> > > default:
> > > dev_err(ds->dev, "Unsupported p5_intf_sel %d\n",
> > > priv->p5_intf_sel);
> > > @@ -2313,8 +2310,6 @@ mt7530_setup(struct dsa_switch *ds)
> > > * Set priv->p5_intf_sel to the appropriate value if PHY muxing
> > > * is detected.
> > > */
> > > - interface = PHY_INTERFACE_MODE_NA;
> > > -
> > > for_each_child_of_node(dn, mac_np) {
> > > if (!of_device_is_compatible(mac_np,
> > > "mediatek,eth-mac"))
> > > @@ -2346,7 +2341,9 @@ mt7530_setup(struct dsa_switch *ds)
> > > break;
> > > }
> > > - mt7530_setup_port5(ds, interface);
> > > + if (priv->p5_intf_sel == P5_INTF_SEL_PHY_P0 ||
> > > + priv->p5_intf_sel == P5_INTF_SEL_PHY_P4)
> > > + mt7530_setup_port5(ds, interface);
> >
> > Hi Arınç,
> >
> > It appears that interface is now uninitialised here.
> >
> > Flagged by Smatch.
>
> I'm not sure why it doesn't catch that for mt7530_setup_port5() to run
> here, priv->p5_intf_sel must be either P5_INTF_SEL_PHY_P0 or
> P5_INTF_SEL_PHY_P4. And for that to happen, the interface variable will be
> initialised.
Yes, I see your point now. At a guess, perhaps it because:
1. It doesn't know that of_get_phy_mode will set the value of interface
2. It doesn't know if the loop will run (more than zero times)
I CCed Dan Carpenter, who is surely more knowledgeable about this than I,
in case he wants to add anything.
> for_each_child_of_node(dn, mac_np) {
> if (!of_device_is_compatible(mac_np,
> "mediatek,eth-mac"))
> continue;
>
> ret = of_property_read_u32(mac_np, "reg", &id);
> if (ret < 0 || id != 1)
> continue;
>
> phy_node = of_parse_phandle(mac_np, "phy-handle", 0);
> if (!phy_node)
> continue;
>
> if (phy_node->parent == priv->dev->of_node->parent) {
> ret = of_get_phy_mode(mac_np, &interface);
> if (ret && ret != -ENODEV) {
> of_node_put(mac_np);
> of_node_put(phy_node);
> return ret;
> }
> id = of_mdio_parse_addr(ds->dev, phy_node);
> if (id == 0)
> priv->p5_intf_sel = P5_INTF_SEL_PHY_P0;
> if (id == 4)
> priv->p5_intf_sel = P5_INTF_SEL_PHY_P4;
> }
> of_node_put(mac_np);
> of_node_put(phy_node);
> break;
> }
>
> if (priv->p5_intf_sel == P5_INTF_SEL_PHY_P0 ||
> priv->p5_intf_sel == P5_INTF_SEL_PHY_P4)
> mt7530_setup_port5(ds, interface);
>
> Arınç
On Sat, Dec 02, 2023 at 11:45:42AM +0300, Arınç ÜNAL wrote:
>
> I'm not sure why it doesn't catch that for mt7530_setup_port5() to run
> here, priv->p5_intf_sel must be either P5_INTF_SEL_PHY_P0 or
> P5_INTF_SEL_PHY_P4. And for that to happen, the interface variable will be
> initialised.
>
> for_each_child_of_node(dn, mac_np) {
> if (!of_device_is_compatible(mac_np,
> "mediatek,eth-mac"))
> continue;
>
> ret = of_property_read_u32(mac_np, "reg", &id);
> if (ret < 0 || id != 1)
> continue;
>
> phy_node = of_parse_phandle(mac_np, "phy-handle", 0);
> if (!phy_node)
> continue;
>
> if (phy_node->parent == priv->dev->of_node->parent) {
> ret = of_get_phy_mode(mac_np, &interface);
> if (ret && ret != -ENODEV) {
> of_node_put(mac_np);
> of_node_put(phy_node);
> return ret;
> }
> id = of_mdio_parse_addr(ds->dev, phy_node);
> if (id == 0)
> priv->p5_intf_sel = P5_INTF_SEL_PHY_P0;
> if (id == 4)
> priv->p5_intf_sel = P5_INTF_SEL_PHY_P4;
> }
> of_node_put(mac_np);
> of_node_put(phy_node);
> break;
> }
>
> if (priv->p5_intf_sel == P5_INTF_SEL_PHY_P0 ||
> priv->p5_intf_sel == P5_INTF_SEL_PHY_P4)
> mt7530_setup_port5(ds, interface);
Smatch doesn't know:
1) What the value of priv->p5_intf_sel is going into this function
2) We enter the for_each_child_of_node() loop
3) That if (phy_node->parent == priv->dev->of_node->parent) { is
definitely true for one element on the list.
Looking at how Smatch parses this code, I could probably improve problem
#1 a bit. Right now Smatch sees "struct mt7530_priv *priv = ds->priv;"
and "priv->p5_intf_sel" is unknown, but I could probably improve it to
where it says that it's in the 1-3 range. But that doesn't help here
and it doesn't address problems 2 and 3.
It's a hard problem.
regards,
dan carpenter
On Sat, Dec 02, 2023 at 11:36:03AM +0300, Arınç ÜNAL wrote:
> On 18.11.2023 17:41, Russell King (Oracle) wrote:
> > > For the cases of PHY muxing or the port being disabled, call
> > > mt7530_setup_port5() from mt7530_setup(). 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.
> >
> > ... and this should state why this needs to happen - in other words,
> > the commit message should state why is it critical that port 5 is
> > always setup.
>
> Actually, port 5 must not always be setup. With patch 7, I explain this
> while preventing mt7530_setup_port5() from running if port 5 is disabled.
>
> Arınç
Then change that last paragraph. You could say something like this:
To keep the cases where port 5 isn't controlled by phylink working as
before, we need to preserve the mt7530_setup_port5() call from mt7530_setup().
I think it's a case of saying too much, which sparks too many unresolved
questions in the reader's mind, which are irrelevant for the purpose of
this specific change: eliminating the overlap between DSA's setup() time
and phylink.
On Sat, Nov 18, 2023 at 03:31:56PM +0300, Arınç ÜNAL wrote:
> Do not set priv->p5_interface on mt7530_setup_port5(). There isn't a case
> where mt753x_phylink_mac_config() runs after mt7530_setup_port5() which
> setting priv->p5_interface would prevent mt7530_setup_port5() from running
> more than once.
>
> Signed-off-by: Arınç ÜNAL <[email protected]>
> ---
> drivers/net/dsa/mt7530.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 069b3dfca6fa..fc87ec817672 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -978,8 +978,6 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
> dev_dbg(ds->dev, "Setup P5, HWTRAP=0x%x, intf_sel=%s, phy-mode=%s\n",
> val, p5_intf_modes(priv->p5_intf_sel), phy_modes(interface));
>
> - priv->p5_interface = interface;
> -
> unlock_exit:
> mutex_unlock(&priv->reg_mutex);
> }
> --
> 2.40.1
>
Purely as a matter of theory, mt753x_phylink_mac_config() itself could
be called more than once, at any time there is a phylink_major_config() -
first during initial one, then upon any change of the phy_interface_t.
With the port being fixed and internal, maybe that is unlikely.
Destroying and re-creating the phylink instance could also make the
driver see more than 1 mt753x_phylink_mac_config() call. During periods
of -EPROBE_DEFER, maybe this could even happen.
I think what we need to see is a description of what the priv->p5_interface
test is really protecting against, because it certainly is uncommon in other
drivers to have this much logic to avoid mt753x_mac_config() being
called twice.
If there's nothing wrong with calling it twice and it's just an optimization,
I think that's enough of a justification for removing that extra protection.
At the same time, the optimization (and simplification) that we should
pursue is that we should remove the overlap between phylink and the
initial port setup made by the driver.
On Thu, Dec 07, 2023 at 09:51:07AM +0300, Dan Carpenter wrote:
> On Sat, Dec 02, 2023 at 11:45:42AM +0300, Arınç ÜNAL wrote:
> >
> > I'm not sure why it doesn't catch that for mt7530_setup_port5() to run
> > here, priv->p5_intf_sel must be either P5_INTF_SEL_PHY_P0 or
> > P5_INTF_SEL_PHY_P4. And for that to happen, the interface variable will be
> > initialised.
> >
> > for_each_child_of_node(dn, mac_np) {
> > if (!of_device_is_compatible(mac_np,
> > "mediatek,eth-mac"))
> > continue;
> >
> > ret = of_property_read_u32(mac_np, "reg", &id);
> > if (ret < 0 || id != 1)
> > continue;
> >
> > phy_node = of_parse_phandle(mac_np, "phy-handle", 0);
> > if (!phy_node)
> > continue;
> >
> > if (phy_node->parent == priv->dev->of_node->parent) {
> > ret = of_get_phy_mode(mac_np, &interface);
> > if (ret && ret != -ENODEV) {
> > of_node_put(mac_np);
> > of_node_put(phy_node);
> > return ret;
> > }
> > id = of_mdio_parse_addr(ds->dev, phy_node);
> > if (id == 0)
> > priv->p5_intf_sel = P5_INTF_SEL_PHY_P0;
> > if (id == 4)
> > priv->p5_intf_sel = P5_INTF_SEL_PHY_P4;
> > }
> > of_node_put(mac_np);
> > of_node_put(phy_node);
> > break;
> > }
> >
> > if (priv->p5_intf_sel == P5_INTF_SEL_PHY_P0 ||
> > priv->p5_intf_sel == P5_INTF_SEL_PHY_P4)
> > mt7530_setup_port5(ds, interface);
>
> Smatch doesn't know:
> 1) What the value of priv->p5_intf_sel is going into this function
> 2) We enter the for_each_child_of_node() loop
> 3) That if (phy_node->parent == priv->dev->of_node->parent) { is
> definitely true for one element on the list.
>
> Looking at how Smatch parses this code, I could probably improve problem
> #1 a bit. Right now Smatch sees "struct mt7530_priv *priv = ds->priv;"
> and "priv->p5_intf_sel" is unknown, but I could probably improve it to
> where it says that it's in the 1-3 range. But that doesn't help here
> and it doesn't address problems 2 and 3.
>
> It's a hard problem.
>
> regards,
> dan carpenter
>
We could be more pragmatic about this whole sparse false positive warning,
and just move the "if" block which calls mt7530_setup_port5() right
after the priv->p5_intf_sel assignments, instead of waiting to "break;"
from the for_each_child_of_node() loop.
for_each_child_of_node(dn, mac_np) {
if (!of_device_is_compatible(mac_np,
"mediatek,eth-mac"))
continue;
ret = of_property_read_u32(mac_np, "reg", &id);
if (ret < 0 || id != 1)
continue;
phy_node = of_parse_phandle(mac_np, "phy-handle", 0);
if (!phy_node)
continue;
if (phy_node->parent == priv->dev->of_node->parent) {
ret = of_get_phy_mode(mac_np, &interface);
if (ret && ret != -ENODEV) {
of_node_put(mac_np);
of_node_put(phy_node);
return ret;
}
id = of_mdio_parse_addr(ds->dev, phy_node);
if (id == 0)
priv->p5_intf_sel = P5_INTF_SEL_PHY_P0;
if (id == 4)
priv->p5_intf_sel = P5_INTF_SEL_PHY_P4;
if (priv->p5_intf_sel == P5_INTF_SEL_PHY_P0 || <---- here
priv->p5_intf_sel == P5_INTF_SEL_PHY_P4)
mt7530_setup_port5(ds, interface);
}
of_node_put(mac_np);
of_node_put(phy_node);
break;
}
I hope it's now much clearer to sparse that "interface" is used within
the same basic block in which it also got assigned, and that determination
does not depend upon the values taken by a second variable.
Maybe it's also a bit clearer for us humans.
What would also help us humans even more is to extract the entire "dn"
handling from mt7530_setup() into a separate mt7530_setup_phy_muxing()
function, and put a good comment there about what's going on with this
PHY muxing thing.
The advantage of splitting this up is that we don't pollute mt7530_setup()
with finding the "dn" if dsa_is_unused_port(ds, 5) returns false.
Also, reducing the indentation level of for_each_child_of_node() by one
can't be bad. Maybe even by more. There's this pattern:
for_each_child_of_node(dn, mac_np) {
// do stuff with mac_np
break;
}
aka we only care about the first child of dn. We could find the mac_np
as the only operation inside for_each_child_of_node(), break directly,
and "do stuff with mac_np" could be done outside, further reducing the
indentation by 1 level.
On Thu, Dec 07, 2023 at 08:40:15PM +0200, Vladimir Oltean wrote:
> Also, reducing the indentation level of for_each_child_of_node() by one
> can't be bad. Maybe even by more. There's this pattern:
>
> for_each_child_of_node(dn, mac_np) {
> // do stuff with mac_np
> break;
> }
>
> aka we only care about the first child of dn. We could find the mac_np
> as the only operation inside for_each_child_of_node(), break directly,
> and "do stuff with mac_np" could be done outside, further reducing the
> indentation by 1 level.
I noticed just now that there is further filtering on the child OF node
by of_device_is_compatible(). In that case, of_get_compatible_child()
could be used to eliminate for_each_child_of_node() completely.
On Thu, Dec 07, 2023 at 08:40:15PM +0200, Vladimir Oltean wrote:
>
> We could be more pragmatic about this whole sparse false positive warning,
> and just move the "if" block which calls mt7530_setup_port5() right
> after the priv->p5_intf_sel assignments, instead of waiting to "break;"
> from the for_each_child_of_node() loop.
>
> for_each_child_of_node(dn, mac_np) {
> if (!of_device_is_compatible(mac_np,
> "mediatek,eth-mac"))
> continue;
>
> ret = of_property_read_u32(mac_np, "reg", &id);
> if (ret < 0 || id != 1)
> continue;
>
> phy_node = of_parse_phandle(mac_np, "phy-handle", 0);
> if (!phy_node)
> continue;
>
> if (phy_node->parent == priv->dev->of_node->parent) {
> ret = of_get_phy_mode(mac_np, &interface);
> if (ret && ret != -ENODEV) {
> of_node_put(mac_np);
> of_node_put(phy_node);
> return ret;
> }
> id = of_mdio_parse_addr(ds->dev, phy_node);
> if (id == 0)
> priv->p5_intf_sel = P5_INTF_SEL_PHY_P0;
> if (id == 4)
> priv->p5_intf_sel = P5_INTF_SEL_PHY_P4;
>
> if (priv->p5_intf_sel == P5_INTF_SEL_PHY_P0 || <---- here
> priv->p5_intf_sel == P5_INTF_SEL_PHY_P4)
> mt7530_setup_port5(ds, interface);
This doesn't solve the problem that Smatch doesn't know what the
original value of priv->p5_intf_sel. And also I don't like this code
because now we call mt7530_setup_port5() on every iteration after
we find the first P5_INTF_SEL_PHY_P0.
> }
> of_node_put(mac_np);
> of_node_put(phy_node);
> break;
> }
Let's not worry too much about false positives. We can just ignore
them. There is always a trade off between false positives and false
negatives. With GCC we try to get a clean run with no warnings, but
with Smatch I'm targetting the false positive ratio at "this is worth
reviewing one time."
regards,
dan carpenter
On Fri, Dec 08, 2023 at 07:23:38AM +0300, Dan Carpenter wrote:
> On Thu, Dec 07, 2023 at 08:40:15PM +0200, Vladimir Oltean wrote:
> >
> > We could be more pragmatic about this whole sparse false positive warning,
> > and just move the "if" block which calls mt7530_setup_port5() right
> > after the priv->p5_intf_sel assignments, instead of waiting to "break;"
> > from the for_each_child_of_node() loop.
> >
> > for_each_child_of_node(dn, mac_np) {
> > if (!of_device_is_compatible(mac_np,
> > "mediatek,eth-mac"))
> > continue;
> >
> > ret = of_property_read_u32(mac_np, "reg", &id);
> > if (ret < 0 || id != 1)
> > continue;
> >
> > phy_node = of_parse_phandle(mac_np, "phy-handle", 0);
> > if (!phy_node)
> > continue;
> >
> > if (phy_node->parent == priv->dev->of_node->parent) {
> > ret = of_get_phy_mode(mac_np, &interface);
> > if (ret && ret != -ENODEV) {
> > of_node_put(mac_np);
> > of_node_put(phy_node);
> > return ret;
> > }
> > id = of_mdio_parse_addr(ds->dev, phy_node);
> > if (id == 0)
> > priv->p5_intf_sel = P5_INTF_SEL_PHY_P0;
> > if (id == 4)
> > priv->p5_intf_sel = P5_INTF_SEL_PHY_P4;
> >
> > if (priv->p5_intf_sel == P5_INTF_SEL_PHY_P0 || <---- here
> > priv->p5_intf_sel == P5_INTF_SEL_PHY_P4)
> > mt7530_setup_port5(ds, interface);
>
> This doesn't solve the problem that Smatch doesn't know what the
> original value of priv->p5_intf_sel. And also I don't like this code
> because now we call mt7530_setup_port5() on every iteration after
> we find the first P5_INTF_SEL_PHY_P0.
You seem to have not parsed the "break" from 4 lines below. There is at
most one iteration through for_each_child_of_node().
And why would the "original" value of priv->p5_intf_sel matter? Original
or modified by the "if (id == 0)" and "if (id == 4)" blocks, the code
has already executed the of_get_phy_mode(&interface) call, by the time
we reach the "if" that calls mt7530_setup_port5().
Hmm, maybe the problem, all along, was that we let the -ENODEV return
code from of_get_phy_mode() pass through? "interface" will really be
uninitialized in that case. It's not a false positive.
Instead of:
ret = of_get_phy_mode(mac_np, &interface);
if (ret && ret != -ENODEV) {
...
return ret;
}
it should have been like this, to not complain:
ret = of_get_phy_mode(mac_np, &interface);
if (ret) {
...
return ret;
}
> > }
> > of_node_put(mac_np);
> > of_node_put(phy_node);
> > break;
> > }
Hi Arınç,
On Sat, Dec 02, 2023 at 11:52:13AM +0300, Arınç ÜNAL wrote:
> Can I receive reviews for patches 6, 12, 13, 14, and 15?
>
> Thanks.
> Arınç
Sorry, I don't have time to look at this old thread anymore.
Please repost the series with the feedback you got so far addressed.
Also, you can try to split into smaller, more cohesive groups of
patches, that have higher chances of getting merged.
On 13.12.2023 18:19, Vladimir Oltean wrote:
> Hi Arınç,
>
> On Sat, Dec 02, 2023 at 11:52:13AM +0300, Arınç ÜNAL wrote:
>> Can I receive reviews for patches 6, 12, 13, 14, and 15?
>>
>> Thanks.
>> Arınç
>
> Sorry, I don't have time to look at this old thread anymore.
>
> Please repost the series with the feedback you got so far addressed.
> Also, you can try to split into smaller, more cohesive groups of
> patches, that have higher chances of getting merged.
Thanks Vladimir. I've been fighting a bad fever with sore throat. I'll
submit the first 7 patches after working on your reviews.
Thanks a lot for your attention to my patches that's been going on for more
than half a year now.
Arınç
On 7.12.2023 21:03, Vladimir Oltean wrote:
> On Sat, Dec 02, 2023 at 11:36:03AM +0300, Arınç ÜNAL wrote:
>> On 18.11.2023 17:41, Russell King (Oracle) wrote:
>>>> For the cases of PHY muxing or the port being disabled, call
>>>> mt7530_setup_port5() from mt7530_setup(). 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.
>>>
>>> ... and this should state why this needs to happen - in other words,
>>> the commit message should state why is it critical that port 5 is
>>> always setup.
>>
>> Actually, port 5 must not always be setup. With patch 7, I explain this
>> while preventing mt7530_setup_port5() from running if port 5 is disabled.
>>
>> Arınç
>
> Then change that last paragraph. You could say something like this:
>
> To keep the cases where port 5 isn't controlled by phylink working as
> before, we need to preserve the mt7530_setup_port5() call from mt7530_setup().
>
> I think it's a case of saying too much, which sparks too many unresolved
> questions in the reader's mind, which are irrelevant for the purpose of
> this specific change: eliminating the overlap between DSA's setup() time
> and phylink.
Will do, thanks.
On 8.12.2023 21:46, Vladimir Oltean wrote:
> Hmm, maybe the problem, all along, was that we let the -ENODEV return
> code from of_get_phy_mode() pass through? "interface" will really be
> uninitialized in that case. It's not a false positive.
>
> Instead of:
>
> ret = of_get_phy_mode(mac_np, &interface);
> if (ret && ret != -ENODEV) {
> ...
> return ret;
> }
>
> it should have been like this, to not complain:
>
> ret = of_get_phy_mode(mac_np, &interface);
> if (ret) {
> ...
> return ret;
> }
>
I just tried this, smatch still reports "interface" as uninitialised.
$ export ARCH=mips CROSS_COMPILE=mips-linux-gnu-
$ ../smatch/smatch_scripts/kchecker --spammy drivers/net/dsa/mt7530.c
UPD include/config/kernel.release
UPD include/generated/utsrelease.h
CHECK scripts/mod/empty.c
CALL scripts/checksyscalls.sh
CC drivers/net/dsa/mt7530.o
CHECK drivers/net/dsa/mt7530.c
drivers/net/dsa/mt7530.c:217 mt7530_mii_read() warn: call of 'warn_slowpath_fmt' with non-constant format argument
drivers/net/dsa/mt7530.c:454 mt7530_setup_port6() error: uninitialized symbol 'ncpo1'.
drivers/net/dsa/mt7530.c:868 mt7530_set_ageing_time() error: uninitialized symbol 'age_count'.
drivers/net/dsa/mt7530.c:868 mt7530_set_ageing_time() error: uninitialized symbol 'age_unit'.
drivers/net/dsa/mt7530.c:2324 mt7530_setup() error: uninitialized symbol 'interface'.
Arınç
On 7.12.2023 21:18, Vladimir Oltean wrote:
> On Sat, Nov 18, 2023 at 03:31:56PM +0300, Arınç ÜNAL wrote:
>> Do not set priv->p5_interface on mt7530_setup_port5(). There isn't a case
>> where mt753x_phylink_mac_config() runs after mt7530_setup_port5() which
>> setting priv->p5_interface would prevent mt7530_setup_port5() from running
>> more than once.
>>
>> Signed-off-by: Arınç ÜNAL <[email protected]>
>> ---
>> drivers/net/dsa/mt7530.c | 2 --
>> 1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index 069b3dfca6fa..fc87ec817672 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -978,8 +978,6 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
>> dev_dbg(ds->dev, "Setup P5, HWTRAP=0x%x, intf_sel=%s, phy-mode=%s\n",
>> val, p5_intf_modes(priv->p5_intf_sel), phy_modes(interface));
>>
>> - priv->p5_interface = interface;
>> -
>> unlock_exit:
>> mutex_unlock(&priv->reg_mutex);
>> }
>> --
>> 2.40.1
>>
>
> Purely as a matter of theory, mt753x_phylink_mac_config() itself could
> be called more than once, at any time there is a phylink_major_config() -
> first during initial one, then upon any change of the phy_interface_t.
> With the port being fixed and internal, maybe that is unlikely.
>
> Destroying and re-creating the phylink instance could also make the
> driver see more than 1 mt753x_phylink_mac_config() call. During periods
> of -EPROBE_DEFER, maybe this could even happen.
>
> I think what we need to see is a description of what the priv->p5_interface
> test is really protecting against, because it certainly is uncommon in other
> drivers to have this much logic to avoid mt753x_mac_config() being
> called twice.
>
> If there's nothing wrong with calling it twice and it's just an optimization,
> I think that's enough of a justification for removing that extra protection.
> At the same time, the optimization (and simplification) that we should
> pursue is that we should remove the overlap between phylink and the
> initial port setup made by the driver.
priv->p5_interface and priv->p6_interface were actually intended to apply
only for MT7531. It prevents the CPU ports to be configured again. CPU
ports are unnecessarily configured before phylink. I intend to address that
with the patch below. I'll submit it after the current patches here go
through. There's a lot to clean up in this driver.
https://github.com/arinc9/linux/commit/80efcb1870530ef5526d7feda625374b8f308369
If you can recall, this is where me and Russell mostly left off on my 30
patch series. I was supposed to run some debug info prints to make sure
that the MT7531 CPU ports are still configured as they were with
priv->info->cpu_port_config().
https://lore.kernel.org/netdev/[email protected]/
For this patch, I can change the patch log to state that priv->p5_interface
and priv->p6_interface are intended for use on the MT7531 switch.
Arınç
On Sun, Dec 17, 2023 at 03:22:27PM +0300, Arınç ÜNAL wrote:
> On 8.12.2023 21:46, Vladimir Oltean wrote:
> > Hmm, maybe the problem, all along, was that we let the -ENODEV return
> > code from of_get_phy_mode() pass through? "interface" will really be
> > uninitialized in that case. It's not a false positive.
> >
> > Instead of:
> >
> > ret = of_get_phy_mode(mac_np, &interface);
> > if (ret && ret != -ENODEV) {
> > ...
> > return ret;
> > }
> >
> > it should have been like this, to not complain:
> >
> > ret = of_get_phy_mode(mac_np, &interface);
> > if (ret) {
> > ...
> > return ret;
> > }
> >
>
> I just tried this, smatch still reports "interface" as uninitialised.
>
> $ export ARCH=mips CROSS_COMPILE=mips-linux-gnu-
> $ ../smatch/smatch_scripts/kchecker --spammy drivers/net/dsa/mt7530.c
>
> UPD include/config/kernel.release
> UPD include/generated/utsrelease.h
> CHECK scripts/mod/empty.c
> CALL scripts/checksyscalls.sh
> CC drivers/net/dsa/mt7530.o
> CHECK drivers/net/dsa/mt7530.c
> drivers/net/dsa/mt7530.c:217 mt7530_mii_read() warn: call of 'warn_slowpath_fmt' with non-constant format argument
> drivers/net/dsa/mt7530.c:454 mt7530_setup_port6() error: uninitialized symbol 'ncpo1'.
> drivers/net/dsa/mt7530.c:868 mt7530_set_ageing_time() error: uninitialized symbol 'age_count'.
> drivers/net/dsa/mt7530.c:868 mt7530_set_ageing_time() error: uninitialized symbol 'age_unit'.
> drivers/net/dsa/mt7530.c:2324 mt7530_setup() error: uninitialized symbol 'interface'.
That's so strange.
Vladimir was right that I was misreading what he said and also hadn't
noticed the break.
For me, his approach silences the warning with or without the cross
function DB. Also of_get_phy_mode() initializes interface on all paths
so checking for -EINVAL doesn't matter as far as this warning is
concerned.
regards,
dan carpenter
On 1/2/24 13:16, Dan Carpenter wrote:
> On Sun, Dec 17, 2023 at 03:22:27PM +0300, Arınç ÜNAL wrote:
>> On 8.12.2023 21:46, Vladimir Oltean wrote:
>>> Hmm, maybe the problem, all along, was that we let the -ENODEV return
>>> code from of_get_phy_mode() pass through? "interface" will really be
>>> uninitialized in that case. It's not a false positive.
>>>
>>> Instead of:
>>>
>>> ret = of_get_phy_mode(mac_np, &interface);
>>> if (ret && ret != -ENODEV) {
>>> ...
>>> return ret;
>>> }
>>>
>>> it should have been like this, to not complain:
>>>
>>> ret = of_get_phy_mode(mac_np, &interface);
>>> if (ret) {
>>> ...
>>> return ret;
>>> }
>>>
>>
>> I just tried this, smatch still reports "interface" as uninitialised.
>>
>> $ export ARCH=mips CROSS_COMPILE=mips-linux-gnu-
>> $ ../smatch/smatch_scripts/kchecker --spammy drivers/net/dsa/mt7530.c
>>
>> UPD include/config/kernel.release
>> UPD include/generated/utsrelease.h
>> CHECK scripts/mod/empty.c
>> CALL scripts/checksyscalls.sh
>> CC drivers/net/dsa/mt7530.o
>> CHECK drivers/net/dsa/mt7530.c
>> drivers/net/dsa/mt7530.c:217 mt7530_mii_read() warn: call of 'warn_slowpath_fmt' with non-constant format argument
>> drivers/net/dsa/mt7530.c:454 mt7530_setup_port6() error: uninitialized symbol 'ncpo1'.
>> drivers/net/dsa/mt7530.c:868 mt7530_set_ageing_time() error: uninitialized symbol 'age_count'.
>> drivers/net/dsa/mt7530.c:868 mt7530_set_ageing_time() error: uninitialized symbol 'age_unit'.
>> drivers/net/dsa/mt7530.c:2324 mt7530_setup() error: uninitialized symbol 'interface'.
>
> That's so strange.
>
> Vladimir was right that I was misreading what he said and also hadn't
> noticed the break.
>
> For me, his approach silences the warning with or without the cross
> function DB. Also of_get_phy_mode() initializes interface on all paths
> so checking for -EINVAL doesn't matter as far as this warning is
> concerned.
I must be missing something.
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 7f9d63b61168..05ce3face47c 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2325,7 +2325,7 @@ mt7530_setup(struct dsa_switch *ds)
if (phy_node->parent == priv->dev->of_node->parent) {
ret = of_get_phy_mode(mac_np, &interface);
- if (ret && ret != -ENODEV) {
+ if (ret) {
of_node_put(mac_np);
of_node_put(phy_node);
return ret;
$ ../smatch/smatch_scripts/kchecker --spammy drivers/net/dsa/mt7530.c
CHECK scripts/mod/empty.c
CALL scripts/checksyscalls.sh
DESCEND objtool
INSTALL libsubcmd_headers
CC drivers/net/dsa/mt7530.o
CHECK drivers/net/dsa/mt7530.c
drivers/net/dsa/mt7530.c:469 mt7530_pad_clk_setup() error: uninitialized symbol 'ncpo1'.
drivers/net/dsa/mt7530.c:895 mt7530_set_ageing_time() error: uninitialized symbol 'age_count'.
drivers/net/dsa/mt7530.c:895 mt7530_set_ageing_time() error: uninitialized symbol 'age_unit'.
drivers/net/dsa/mt7530.c:2346 mt7530_setup() error: uninitialized symbol 'interface'.
Just so you know, I intend to remove this whole PHY muxing feature once I
bring changing DSA conduit support to this subdriver. I've got two strong
reasons for this.
- Changing the DSA conduit achieves the same result with the only overhead
being the DSA header included on every frame.
- There can't be proper dt-bindings for it as the nature of the feature
shows that it represents an optional way to operate the hardware, it does
not represent a hardware design. Overall, the implementation is a hack to
make it work for specific hardware (switch must be connected to gmac1 of
a MediaTek SoC, no PHY must be present at address 0 or 4 on the MDIO bus
of the SoC. It should rather be configurable on userspace. Which will
never happen as it is specific to this switch and the changing DSA
conduit feature is the perfect substitute for this.
Let me know if you've got any suggestions that can get rid of the warning
without reworking the whole code block. Otherwise, I'm just going to ignore
it until I get rid of the whole code block.
Arınç
On Sat, Jan 06, 2024 at 08:01:25PM +0200, Arınç ÜNAL wrote:
> I must be missing something.
>
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 7f9d63b61168..05ce3face47c 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2325,7 +2325,7 @@ mt7530_setup(struct dsa_switch *ds)
> if (phy_node->parent == priv->dev->of_node->parent) {
> ret = of_get_phy_mode(mac_np, &interface);
> - if (ret && ret != -ENODEV) {
> + if (ret) {
> of_node_put(mac_np);
> of_node_put(phy_node);
> return ret;
>
> $ ../smatch/smatch_scripts/kchecker --spammy drivers/net/dsa/mt7530.c
> CHECK scripts/mod/empty.c
> CALL scripts/checksyscalls.sh
> DESCEND objtool
> INSTALL libsubcmd_headers
> CC drivers/net/dsa/mt7530.o
> CHECK drivers/net/dsa/mt7530.c
> drivers/net/dsa/mt7530.c:469 mt7530_pad_clk_setup() error: uninitialized symbol 'ncpo1'.
> drivers/net/dsa/mt7530.c:895 mt7530_set_ageing_time() error: uninitialized symbol 'age_count'.
> drivers/net/dsa/mt7530.c:895 mt7530_set_ageing_time() error: uninitialized symbol 'age_unit'.
> drivers/net/dsa/mt7530.c:2346 mt7530_setup() error: uninitialized symbol 'interface'.
Yes, well _now_ it is a false positive, probably because smatch cannot
determine that when priv->p5_intf_sel has been set to P5_INTF_SEL_PHY_P0
or P5_INTF_SEL_PHY_P4, "interface" should have been also initialized.
But it doesn't matter, you can ignore a false positive. I'm also seeing it.
Although you should check whether treating -ENODEV as a hard error is fine
and won't cause regressions.
> Just so you know, I intend to remove this whole PHY muxing feature once I
> bring changing DSA conduit support to this subdriver. I've got two strong
> reasons for this.
> - Changing the DSA conduit achieves the same result with the only overhead
> being the DSA header included on every frame.
>
> - There can't be proper dt-bindings for it as the nature of the feature
> shows that it represents an optional way to operate the hardware, it does
> not represent a hardware design. Overall, the implementation is a hack to
> make it work for specific hardware (switch must be connected to gmac1 of
> a MediaTek SoC, no PHY must be present at address 0 or 4 on the MDIO bus
> of the SoC. It should rather be configurable on userspace. Which will
> never happen as it is specific to this switch and the changing DSA
> conduit feature is the perfect substitute for this.
Is PHY muxing a "true" switch bypass, or is it just a route through the
switch for all packets coming from GMAC5 to go to phy0 or phy4? If the
latter, I agree that dynamic conduit changing is a more flexible option,
not to mention the user space tooling is already there.
Are there existing systems that use PHY muxing? The possible problem I
see is breaking those boards which have a phy-handle on gmac5, if the
mt7530 driver is no longer going to modify its HWTRAP register.
>
> Let me know if you've got any suggestions that can get rid of the warning
> without reworking the whole code block. Otherwise, I'm just going to ignore
> it until I get rid of the whole code block.
The obvious way would be to leave the initialization to PHY_INTERFACE_MODE_NA
there. Or to just ignore the warning.
On 9.01.2024 17:57, Vladimir Oltean wrote:
> Yes, well _now_ it is a false positive, probably because smatch cannot
> determine that when priv->p5_intf_sel has been set to P5_INTF_SEL_PHY_P0
> or P5_INTF_SEL_PHY_P4, "interface" should have been also initialized.
> But it doesn't matter, you can ignore a false positive. I'm also seeing it.
> Although you should check whether treating -ENODEV as a hard error is fine
> and won't cause regressions.
>
>> Just so you know, I intend to remove this whole PHY muxing feature once I
>> bring changing DSA conduit support to this subdriver. I've got two strong
>> reasons for this.
>> - Changing the DSA conduit achieves the same result with the only overhead
>> being the DSA header included on every frame.
>>
>> - There can't be proper dt-bindings for it as the nature of the feature
>> shows that it represents an optional way to operate the hardware, it does
>> not represent a hardware design. Overall, the implementation is a hack to
>> make it work for specific hardware (switch must be connected to gmac1 of
>> a MediaTek SoC, no PHY must be present at address 0 or 4 on the MDIO bus
>> of the SoC. It should rather be configurable on userspace. Which will
>> never happen as it is specific to this switch and the changing DSA
>> conduit feature is the perfect substitute for this.
>
> Is PHY muxing a "true" switch bypass, or is it just a route through the
> switch for all packets coming from GMAC5 to go to phy0 or phy4? If the
> latter, I agree that dynamic conduit changing is a more flexible option,
> not to mention the user space tooling is already there.
It's the latter, and that's exactly what I think.
>
> Are there existing systems that use PHY muxing? The possible problem I
> see is breaking those boards which have a phy-handle on gmac5, if the
> mt7530 driver is no longer going to modify its HWTRAP register.
Ah see, for PHY muxing, the driver actually wants the phy-handle to be put
on the SoC MAC, and the PHY to be defined on the SoC ethernet's MDIO bus.
We don't even define gmac5 as a port on the switch dt-bindings.
While none of the DTs on the Linux repository utilise this, some of the
mt7621 DTs on OpenWrt do. The change in behaviour will only be that phy0/4
will be inaccessible from the SoC MAC's network interface. I de-facto
maintain the mt7621 device tree source files there. I intend to revert it
along with adding port 5 as a CPU port so that the conduit changing feature
becomes available.
>
>>
>> Let me know if you've got any suggestions that can get rid of the warning
>> without reworking the whole code block. Otherwise, I'm just going to ignore
>> it until I get rid of the whole code block.
>
> The obvious way would be to leave the initialization to PHY_INTERFACE_MODE_NA
> there. Or to just ignore the warning.
I'll ignore.
Arınç
On Wed, Jan 10, 2024 at 10:26:54AM +0300, Arınç ÜNAL wrote:
> > Are there existing systems that use PHY muxing? The possible problem I
> > see is breaking those boards which have a phy-handle on gmac5, if the
> > mt7530 driver is no longer going to modify its HWTRAP register.
>
> Ah see, for PHY muxing, the driver actually wants the phy-handle to be put
> on the SoC MAC, and the PHY to be defined on the SoC ethernet's MDIO bus.
> We don't even define gmac5 as a port on the switch dt-bindings.
I noticed that from the code already. Maybe I shouldn't have said
"gmac5" when I meant "the GMAC attached to switch port 5, aka GMAC0".
I was under the impression that you were also using this slightly
incorrect terminology, to keep a numerical association between the CPU
port number and its directly attached GMAC.
> While none of the DTs on the Linux repository utilise this, some of the
> mt7621 DTs on OpenWrt do. The change in behaviour will only be that phy0/4
> will be inaccessible from the SoC MAC's network interface. I de-facto
> maintain the mt7621 device tree source files there. I intend to revert it
> along with adding port 5 as a CPU port so that the conduit changing feature
> becomes available.
If OpenWrt kernels are always shipped in tandem with updated device
trees (i.e. no Arm SystemReady IR platforms, where the DT is provided by
U-Boot), I won't oppose to retracting features described via DT if their
platform maintainers agree in a wide enough circle that the breakage is
manageable.
BTW, besides OpenWrt, what other software is deployed on these SoCs
typically?
On 10.01.2024 21:23, Vladimir Oltean wrote:
> On Wed, Jan 10, 2024 at 10:26:54AM +0300, Arınç ÜNAL wrote:
>>> Are there existing systems that use PHY muxing? The possible problem I
>>> see is breaking those boards which have a phy-handle on gmac5, if the
>>> mt7530 driver is no longer going to modify its HWTRAP register.
>>
>> Ah see, for PHY muxing, the driver actually wants the phy-handle to be put
>> on the SoC MAC, and the PHY to be defined on the SoC ethernet's MDIO bus.
>> We don't even define gmac5 as a port on the switch dt-bindings.
>
> I noticed that from the code already. Maybe I shouldn't have said
> "gmac5" when I meant "the GMAC attached to switch port 5, aka GMAC0".
> I was under the impression that you were also using this slightly
> incorrect terminology, to keep a numerical association between the CPU
> port number and its directly attached GMAC.
>
>> While none of the DTs on the Linux repository utilise this, some of the
>> mt7621 DTs on OpenWrt do. The change in behaviour will only be that phy0/4
>> will be inaccessible from the SoC MAC's network interface. I de-facto
>> maintain the mt7621 device tree source files there. I intend to revert it
>> along with adding port 5 as a CPU port so that the conduit changing feature
>> becomes available.
>
> If OpenWrt kernels are always shipped in tandem with updated device
> trees (i.e. no Arm SystemReady IR platforms, where the DT is provided by
> U-Boot), I won't oppose to retracting features described via DT if their
> platform maintainers agree in a wide enough circle that the breakage is
> manageable.
I will see to this when the time comes.
>
> BTW, besides OpenWrt, what other software is deployed on these SoCs
> typically?
Other than OpenWrt which is widely used for these SoCs for its ease of
flashing and upgrading, compatibility with legacy U-boot versions that
usually come with any vendor making a product out of these SoCs, I can only
talk about what I deploy to run Linux. I use mainline U-Boot along with the
device trees from the Linux repository to boot mainline Linux kernels with
Buildroot as the filesystem.
Arınç
On Thu, Jan 11, 2024 at 01:22:12PM +0300, Arınç ÜNAL wrote:
> > BTW, besides OpenWrt, what other software is deployed on these SoCs
> > typically?
>
> Other than OpenWrt which is widely used for these SoCs for its ease of
> flashing and upgrading, compatibility with legacy U-boot versions that
> usually come with any vendor making a product out of these SoCs, I can only
> talk about what I deploy to run Linux. I use mainline U-Boot along with the
> device trees from the Linux repository to boot mainline Linux kernels with
> Buildroot as the filesystem.
>
> Arınç
I meant what other software (operating system) _instead_ of OpenWRT.
I'm trying to figure out what other users of PHY muxing there might be.
Am 11. Januar 2024 11:31:46 MEZ schrieb Vladimir Oltean <[email protected]>:
>On Thu, Jan 11, 2024 at 01:22:12PM +0300, Arınç ÜNAL wrote:
>> > BTW, besides OpenWrt, what other software is deployed on these SoCs
>> > typically?
>>
>> Other than OpenWrt which is widely used for these SoCs for its ease of
>> flashing and upgrading, compatibility with legacy U-boot versions that
>> usually come with any vendor making a product out of these SoCs, I can only
>> talk about what I deploy to run Linux. I use mainline U-Boot along with the
>> device trees from the Linux repository to boot mainline Linux kernels with
>> Buildroot as the filesystem.
>>
>> Arınç
>
>I meant what other software (operating system) _instead_ of OpenWRT.
>I'm trying to figure out what other users of PHY muxing there might be.
Hi
I (and possibly others) use debian/ubuntu and there are also archlinux images for the bpi router boards.
regards Frank
On 11.01.2024 13:31, Vladimir Oltean wrote:
> On Thu, Jan 11, 2024 at 01:22:12PM +0300, Arınç ÜNAL wrote:
>>> BTW, besides OpenWrt, what other software is deployed on these SoCs
>>> typically?
>>
>> Other than OpenWrt which is widely used for these SoCs for its ease of
>> flashing and upgrading, compatibility with legacy U-boot versions that
>> usually come with any vendor making a product out of these SoCs, I can only
>> talk about what I deploy to run Linux. I use mainline U-Boot along with the
>> device trees from the Linux repository to boot mainline Linux kernels with
>> Buildroot as the filesystem.
>>
>> Arınç
>
> I meant what other software (operating system) _instead_ of OpenWRT.
> I'm trying to figure out what other users of PHY muxing there might be.
I'm not aware of any other projects that utilise PHY muxing of MT7530. It
was me spending a few months bringing the feature to OpenWrt in the first
place.
Arınç