LinuxLists.cc - [PATCH net-next v3 0/7] MT7530 DSA Subdriver Improvements Act II

[permalink] [raw]

Subject: [PATCH net-next v3 2/7] net: dsa: mt7530: call port 6 setup from mt7530_mac_config()

From: Arınç ÜNAL <[email protected]>

mt7530_pad_clk_setup() is called if port 6 is enabled. It used to do more
things than setting up port 6. That part was moved to more appropriate
locations, mt7530_setup() and mt7530_pll_setup().

Now that all it does is set up port 6, rename it to mt7530_setup_port6(),
and move it to a more appropriate location, under mt7530_mac_config().

Leave an empty mt7530_pad_clk_setup() to satisfy the pad_setup function
pointer.

This is the code path for setting up the ports before:

mt753x_phylink_mac_config()
-> mt753x_mac_config()
-> mt7530_mac_config()
-> mt7530_setup_port5()
-> mt753x_pad_setup()
-> mt7530_pad_clk_setup()

This is after:

mt753x_phylink_mac_config()
-> mt753x_mac_config()
-> mt7530_mac_config()
-> mt7530_setup_port5()
-> mt7530_setup_port6()

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 330e22abc076..2d468a5f2e70 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)
{
@@ -2624,12 +2630,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

2024-02-02 09:21:03

[permalink] [raw]

Subject: [PATCH net-next v3 4/7] net: dsa: mt7530: move XTAL check to mt7530_setup()

From: Arınç ÜNAL <[email protected]>

The crystal frequency concerns the switch core. The frequency should be
checked when the switch is being set up so the driver can reject the
unsupported hardware earlier and without requiring port 6 to be used.

Move it to mt7530_setup(). Drop the unnecessary function printing.

Signed-off-by: Arınç ÜNAL <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
Reviewed-by: Vladimir Oltean <[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 fdaf65b58b72..c4d492e29fdf 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;
@@ -2253,6 +2246,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

2024-02-02 09:21:44

[permalink] [raw]

Subject: [PATCH net-next v3 5/7] net: dsa: mt7530: simplify mt7530_setup_port6() and change to void

From: Arınç ÜNAL <[email protected]>

This code is from before this driver was converted to phylink API. Phylink
deals with the unsupported interface cases before mt7530_setup_port6() is
run. Therefore, the default case would never run. However, it must be
defined nonetheless to handle all the remaining enumeration values, the
phy-modes.

Switch to if statement for RGMII and return which simplifies the code and
saves an indent.

Set P6_INTF_MODE, which is the the three least significant bits of the
MT7530_P6ECR register, to 0 for RGMII even though it will already be 0
after reset. This is to keep supporting dynamic reconfiguration of the port
in the case the interface changes from TRGMII to RGMII.

Disable the TRGMII clocks for all cases. They will be enabled if TRGMII is
being used.

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.

Reviewed-by: Daniel Golle <[email protected]>
Signed-off-by: Arınç ÜNAL <[email protected]>
---
drivers/net/dsa/mt7530.c | 101 ++++++++++++++++++++---------------------------
1 file changed, 42 insertions(+), 59 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index c4d492e29fdf..e1fdef5766fb 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -414,70 +414,57 @@ 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;

- xtal = mt7530_read(priv, MT7530_MHWTRAP) & HWTRAP_XTAL_MASK;
+ /* Disable the MT7530 TRGMII clocks */
+ core_clear(priv, CORE_TRGMII_GSW_CLK_CG, REG_TRGMIICK_EN);

- 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)
- 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;
+ if (interface == PHY_INTERFACE_MODE_RGMII) {
+ mt7530_rmw(priv, MT7530_P6ECR, P6_INTF_MODE_MASK,
+ P6_INTF_MODE(0));
+ return;
}

- mt7530_rmw(priv, MT7530_P6ECR, P6_INTF_MODE_MASK,
- P6_INTF_MODE(trgint));
+ mt7530_rmw(priv, MT7530_P6ECR, P6_INTF_MODE_MASK, P6_INTF_MODE(1));

- if (trgint) {
- /* Disable the MT7530 TRGMII clocks */
- core_clear(priv, CORE_TRGMII_GSW_CLK_CG, REG_TRGMIICK_EN);
+ xtal = mt7530_read(priv, MT7530_MHWTRAP) & HWTRAP_XTAL_MASK;

- /* 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);
+ if (xtal == HWTRAP_XTAL_25MHZ)
+ ssc_delta = 0x57;
+ else
+ ssc_delta = 0x87;

- /* Enable the MT7530 TRGMII clocks */
- core_set(priv, CORE_TRGMII_GSW_CLK_CG, REG_TRGMIICK_EN);
+ 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;
}

- return 0;
+ /* 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);
+
+ /* Enable the MT7530 TRGMII clocks */
+ core_set(priv, CORE_TRGMII_GSW_CLK_CG, REG_TRGMIICK_EN);
}

static void
@@ -2609,15 +2596,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

2024-02-02 09:21:55

[permalink] [raw]

Subject: [PATCH net-next v3 7/7] net: dsa: mt7530: do not clear config->supported_interfaces

From: Arınç ÜNAL <[email protected]>

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. The
"config" pointer points to &dp->phylink_config, and "dp" is allocated by
dsa_port_touch() with kzalloc(), so all its fields are filled with zeroes.

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]>
Reviewed-by: Vladimir Oltean <[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 4ca7ba94788e..aed1ea6815ee 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2573,8 +2573,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) {
/* Ports which are connected to switch PHYs. There is no MII pinout. */
case 0 ... 3:

--
2.40.1

2024-02-02 09:21:56

[permalink] [raw]

Subject: [PATCH net-next v3 6/7] net: dsa: mt7530: correct port capabilities of MT7988

From: Arınç ÜNAL <[email protected]>

On the switch on the MT7988 SoC, as shown in Block Diagram 8.1.1.3 on page
125 of "MT7988A Wi-Fi 7 Generation Router Platform: Datasheet (Open
Version) v0.1", there are only 4 PHYs. That's port 0 to 3. Set the case for
ports which connect to switch PHYs to '0 ... 3'.

Port 4 and 5 are not used at all in this design.

Link: https://wiki.banana-pi.org/Banana_Pi_BPI-R4#Documents [1]
Acked-by: Daniel Golle <[email protected]>
Reviewed-by: Vladimir Oltean <[email protected]>
Signed-off-by: Arınç ÜNAL <[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 e1fdef5766fb..4ca7ba94788e 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2577,7 +2577,7 @@ static void mt7988_mac_port_get_caps(struct dsa_switch *ds, int port,

switch (port) {
/* Ports which are connected to switch PHYs. There is no MII pinout. */
- case 0 ... 4:
+ case 0 ... 3:
__set_bit(PHY_INTERFACE_MODE_INTERNAL,
config->supported_interfaces);
break;

--
2.40.1

2024-02-02 09:24:31

[permalink] [raw]

Subject: [PATCH net-next v3 1/7] net: dsa: mt7530: empty default case on mt7530_setup_port5()

From: Arınç ÜNAL <[email protected]>

There're two code paths for setting up port 5:

mt7530_setup()
-> mt7530_setup_port5()

mt753x_phylink_mac_config()
-> mt753x_mac_config()
-> mt7530_mac_config()
-> mt7530_setup_port5()

On the first code path, priv->p5_intf_sel is either set to
P5_INTF_SEL_PHY_P0 or P5_INTF_SEL_PHY_P4 when mt7530_setup_port5() is run.

On the second code path, priv->p5_intf_sel is set to P5_INTF_SEL_GMAC5 when
mt7530_setup_port5() is run.

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 68be38ae66e0..330e22abc076 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

2024-02-02 09:55:58

[permalink] [raw]

Subject: [PATCH net-next v3 3/7] net: dsa: mt7530: remove pad_setup function pointer

From: Arınç ÜNAL <[email protected]>

The pad_setup function pointer was introduced with 88bdef8be9f6 ("net: dsa:
mt7530: Extend device data ready for adding a new hardware"). It was being
used to set up the core clock and port 6 of the MT7530 switch, and pll of
the MT7531 switch.

All of these were moved to more appropriate locations, and it was never
used for the switch on the MT7988 SoC. Therefore, this function pointer
hasn't got a use anymore. Remove it.

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 2d468a5f2e70..fdaf65b58b72 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)
{
@@ -2617,14 +2605,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)
@@ -2794,8 +2774,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;

@@ -3113,11 +3091,6 @@ mt753x_conduit_state_change(struct dsa_switch *ds,
mt7530_rmw(priv, MT7530_MFC, CPU_EN | CPU_PORT_MASK, val);
}

-static int mt7988_pad_setup(struct dsa_switch *ds, phy_interface_t interface)
-{
- return 0;
-}
-
static int mt7988_setup(struct dsa_switch *ds)
{
struct mt7530_priv *priv = ds->priv;
@@ -3181,7 +3154,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,
},
@@ -3193,7 +3165,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,
},
@@ -3205,7 +3176,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,
@@ -3218,7 +3188,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,
@@ -3248,9 +3217,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 80060cc740d2..26a6d2160c08 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

2024-02-02 11:47:02

[permalink] [raw]

Subject: Re: [PATCH net-next v3 3/7] net: dsa: mt7530: remove pad_setup function pointer

On Fri, Feb 02, 2024 at 12:19:09PM +0300, Arınç ÜNAL via B4 Relay wrote:
> From: Arınç ÜNAL <[email protected]>
>
> The pad_setup function pointer was introduced with 88bdef8be9f6 ("net: dsa:
> mt7530: Extend device data ready for adding a new hardware"). It was being
> used to set up the core clock and port 6 of the MT7530 switch, and pll of
> the MT7531 switch.
>
> All of these were moved to more appropriate locations, and it was never
> used for the switch on the MT7988 SoC. Therefore, this function pointer
> hasn't got a use anymore. Remove it.
>
> Signed-off-by: Arınç ÜNAL <[email protected]>
> Acked-by: Daniel Golle <[email protected]>
> Reviewed-by: Vladimir Oltean <[email protected]>

Reviewed-by: Russell King (Oracle) <[email protected]>

Thanks!

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

2024-02-02 11:52:14

[permalink] [raw]

Subject: Re: [PATCH net-next v3 5/7] net: dsa: mt7530: simplify mt7530_setup_port6() and change to void

On Fri, Feb 02, 2024 at 12:19:11PM +0300, Arınç ÜNAL via B4 Relay wrote:
> From: Arınç ÜNAL <[email protected]>
>
> This code is from before this driver was converted to phylink API. Phylink
> deals with the unsupported interface cases before mt7530_setup_port6() is
> run. Therefore, the default case would never run. However, it must be
> defined nonetheless to handle all the remaining enumeration values, the
> phy-modes.
>
> Switch to if statement for RGMII and return which simplifies the code and
> saves an indent.
>
> Set P6_INTF_MODE, which is the the three least significant bits of the
> MT7530_P6ECR register, to 0 for RGMII even though it will already be 0
> after reset. This is to keep supporting dynamic reconfiguration of the port
> in the case the interface changes from TRGMII to RGMII.
>
> Disable the TRGMII clocks for all cases. They will be enabled if TRGMII is
> being used.
>
> 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.
>
> Reviewed-by: Daniel Golle <[email protected]>
> Signed-off-by: Arınç ÜNAL <[email protected]>

Yay. Less possible errors in mac_config()!

Reviewed-by: Russell King (Oracle) <[email protected]>

Thanks!

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

2024-02-02 11:52:43

[permalink] [raw]

Subject: Re: [PATCH net-next v3 7/7] net: dsa: mt7530: do not clear config->supported_interfaces

On Fri, Feb 02, 2024 at 12:19:13PM +0300, Arınç ÜNAL via B4 Relay wrote:
> From: Arınç ÜNAL <[email protected]>
>
> 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. The
> "config" pointer points to &dp->phylink_config, and "dp" is allocated by
> dsa_port_touch() with kzalloc(), so all its fields are filled with zeroes.
>
> 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]>
> Reviewed-by: Vladimir Oltean <[email protected]>

Reviewed-by: Russell King (Oracle) <[email protected]>

Thanks!

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

2024-02-02 12:08:55

[permalink] [raw]

Subject: Re: [PATCH net-next v3 1/7] net: dsa: mt7530: empty default case on mt7530_setup_port5()

On Fri, Feb 02, 2024 at 12:19:07PM +0300, Arınç ÜNAL via B4 Relay wrote:
> From: Arınç ÜNAL <[email protected]>
>
> There're two code paths for setting up port 5:
>
> mt7530_setup()
> -> mt7530_setup_port5()
>
> mt753x_phylink_mac_config()
> -> mt753x_mac_config()
> -> mt7530_mac_config()
> -> mt7530_setup_port5()
>
> 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]>

Reviewed-by: Russell King (Oracle) <[email protected]>

Thanks!

While reviewing this change, but not related to it, I notice that this
function sets the TX delay based on the RGMII interface mode. This isn't
correct. I've explained why this is this many times in the past, but
essentially it comes down to the model:

phy-mode in NIC node Network driver PCB PHY
rgmii no delays delays no delays
rgmii-id no delays no delays tx/rx delays
rgmii-txid no delays no delays tx delays
rgmii-rxid no delays no delays rx delays

Then we have rx-internal-delay-ps and tx-internal-delay-ps in the NIC
node which define the RGMII delays at the local end and similar
properties for the PHY node.

So, if we take the view that, when a switch is connected to a NIC in
RGMII mode, then the phy-mode specified delays still should not impact
the local NIC.

Now, for the switch, we specify the phy-mode in the port node as well.
Consider the case of a switch port connected to a RGMII PHY. This has
to operate in exactly the same way as a normal NIC - that is, the
RGMII delays at the port should be ignored as it's the responsibility
of a PHY.

The final scenario to examine is the case of a RGMII switch port
connected to a NIC. The NIC's phy-mode has no way to be communicated
to DSA or vice versa, so neither phy-mode can impact the other side
of the RGMII link, but should only place the link into RGMII mode.
Given everything I've said above, the only way to configure RGMII
delays is via the rx-internal-delay-ps and tx-internal-delay-ps
properties. So, DSA drivers should _not_ be configuring their ports
with RGMII delays based on the RGMII phy interface mode.

The above is my purely logically reasoned point of view on this
subject. Others may have other (to me completely illogical)
interpretations that can only lead to interoperability issues.

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

2024-02-02 13:14:57

[permalink] [raw]

Subject: Re: [PATCH net-next v3 2/7] net: dsa: mt7530: call port 6 setup from mt7530_mac_config()

On Fri, Feb 02, 2024 at 12:19:08PM +0300, Arınç ÜNAL via B4 Relay wrote:
> From: Arınç ÜNAL <[email protected]>
>
> mt7530_pad_clk_setup() is called if port 6 is enabled. It used to do more
> things than setting up port 6. That part was moved to more appropriate
> locations, mt7530_setup() and mt7530_pll_setup().
>
> Now that all it does is set up port 6, rename it to mt7530_setup_port6(),
> and move it to a more appropriate location, under mt7530_mac_config().
>
> Leave an empty mt7530_pad_clk_setup() to satisfy the pad_setup function
> pointer.
>
> This is the code path for setting up the ports before:
>
> mt753x_phylink_mac_config()
> -> mt753x_mac_config()
> -> mt7530_mac_config()
> -> mt7530_setup_port5()
> -> mt753x_pad_setup()
> -> mt7530_pad_clk_setup()
>
> This is after:
>
> mt753x_phylink_mac_config()
> -> mt753x_mac_config()
> -> mt7530_mac_config()
> -> mt7530_setup_port5()
> -> mt7530_setup_port6()
>
> Signed-off-by: Arınç ÜNAL <[email protected]>
> Reviewed-by: Vladimir Oltean <[email protected]>

I hope that the "default" case in mt7530_setup_port6() is effectively
unreachable - as long as mt7530_mac_port_get_caps() is paired with this
and only sets the interface modes in supported_interfaces that
mt7530_setup_port6() handles, then that will be fine.

Reviewed-by: Russell King (Oracle) <[email protected]>

Thanks!

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

2024-02-02 17:47:22

[permalink] [raw]

Subject: Re: [PATCH net-next v3 1/7] net: dsa: mt7530: empty default case on mt7530_setup_port5()

On 2.02.2024 14:40, Russell King (Oracle) wrote:
> While reviewing this change, but not related to it, I notice that this
> function sets the TX delay based on the RGMII interface mode. This isn't
> correct. I've explained why this is this many times in the past, but
> essentially it comes down to the model:
>
>
> phy-mode in NIC node Network driver PCB PHY
> rgmii no delays delays no delays
> rgmii-id no delays no delays tx/rx delays
> rgmii-txid no delays no delays tx delays
> rgmii-rxid no delays no delays rx delays
>
> Then we have rx-internal-delay-ps and tx-internal-delay-ps in the NIC
> node which define the RGMII delays at the local end and similar
> properties for the PHY node.
>
>
> So, if we take the view that, when a switch is connected to a NIC in
> RGMII mode, then the phy-mode specified delays still should not impact
> the local NIC.
>
> Now, for the switch, we specify the phy-mode in the port node as well.
> Consider the case of a switch port connected to a RGMII PHY. This has
> to operate in exactly the same way as a normal NIC - that is, the
> RGMII delays at the port should be ignored as it's the responsibility
> of a PHY.
>
> The final scenario to examine is the case of a RGMII switch port
> connected to a NIC. The NIC's phy-mode has no way to be communicated
> to DSA or vice versa, so neither phy-mode can impact the other side
> of the RGMII link, but should only place the link into RGMII mode.
> Given everything I've said above, the only way to configure RGMII
> delays is via the rx-internal-delay-ps and tx-internal-delay-ps
> properties. So, DSA drivers should _not_ be configuring their ports
> with RGMII delays based on the RGMII phy interface mode.
>
> The above is my purely logically reasoned point of view on this
> subject. Others may have other (to me completely illogical)
> interpretations that can only lead to interoperability issues.

I will address this with the next patch series. Thank you for explaining it
in detail.

Arınç

2024-02-02 18:06:14

[permalink] [raw]

Subject: Re: [PATCH net-next v3 1/7] net: dsa: mt7530: empty default case on mt7530_setup_port5()

On Fri, Feb 02, 2024 at 08:44:39PM +0300, Arınç ÜNAL wrote:
> On 2.02.2024 14:40, Russell King (Oracle) wrote:
> > While reviewing this change, but not related to it, I notice that this
> > function sets the TX delay based on the RGMII interface mode. This isn't
> > correct. I've explained why this is this many times in the past, but
> > essentially it comes down to the model:
> >
> >
> > phy-mode in NIC node Network driver PCB PHY
> > rgmii no delays delays no delays
> > rgmii-id no delays no delays tx/rx delays
> > rgmii-txid no delays no delays tx delays
> > rgmii-rxid no delays no delays rx delays
> >
> > Then we have rx-internal-delay-ps and tx-internal-delay-ps in the NIC
> > node which define the RGMII delays at the local end and similar
> > properties for the PHY node.
> >
> >
> > So, if we take the view that, when a switch is connected to a NIC in
> > RGMII mode, then the phy-mode specified delays still should not impact
> > the local NIC.
> >
> > Now, for the switch, we specify the phy-mode in the port node as well.
> > Consider the case of a switch port connected to a RGMII PHY. This has
> > to operate in exactly the same way as a normal NIC - that is, the
> > RGMII delays at the port should be ignored as it's the responsibility
> > of a PHY.
> >
> > The final scenario to examine is the case of a RGMII switch port
> > connected to a NIC. The NIC's phy-mode has no way to be communicated
> > to DSA or vice versa, so neither phy-mode can impact the other side
> > of the RGMII link, but should only place the link into RGMII mode.
> > Given everything I've said above, the only way to configure RGMII
> > delays is via the rx-internal-delay-ps and tx-internal-delay-ps
> > properties. So, DSA drivers should _not_ be configuring their ports
> > with RGMII delays based on the RGMII phy interface mode.
> >
> > The above is my purely logically reasoned point of view on this
> > subject. Others may have other (to me completely illogical)
> > interpretations that can only lead to interoperability issues.
>
> I will address this with the next patch series. Thank you for explaining it
> in detail.

This is a good time to point out not to rush with the next patch
series, as my email will _likely_ provoke some additional discussion
from Andrew and/or Vladimir. So please give it a few days (maybe
around the middle of next week) to give them time to consider my
email and respond.

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

2024-02-02 18:16:24

[permalink] [raw]

Subject: Re: [PATCH net-next v3 4/7] net: dsa: mt7530: move XTAL check to mt7530_setup()

On 2.02.2024 14:48, Russell King (Oracle) wrote:
> On Fri, Feb 02, 2024 at 12:19:10PM +0300, Arınç ÜNAL via B4 Relay wrote:
>> From: Arınç ÜNAL <[email protected]>
>>
>> The crystal frequency concerns the switch core. The frequency should be
>> checked when the switch is being set up so the driver can reject the
>> unsupported hardware earlier and without requiring port 6 to be used.
>>
>> Move it to mt7530_setup(). Drop the unnecessary function printing.
>>
>> Signed-off-by: Arınç ÜNAL <[email protected]>
>> Reviewed-by: Andrew Lunn <[email protected]>
>> Reviewed-by: Vladimir Oltean <[email protected]>
>
> I would prefer this to be earlier in the series, before patch 2 which
> moves mt7530_setup_port6() to be called from mac_config(). mac_config()
> is supposed to be configuration error-free - in other words, all state
> should have been checked before hand.

I agree but mt7530_mac_config() is not a void function yet. The
mac_port_config member of the mt753x_info structure points to this
function. My next patch series gets rid of all useless error returns on the
phylink path and change mac_port_config to void. So I don't think working
on this patch series further will worth the effort. I'd rather have this
version applied as is.

Arınç

2024-02-02 19:17:05

by Daniel Golle

[permalink] [raw]

Subject: Re: [PATCH net-next v3 4/7] net: dsa: mt7530: move XTAL check to mt7530_setup()

On Fri, Feb 02, 2024 at 09:16:02PM +0300, Arınç ÜNAL wrote:
> On 2.02.2024 14:48, Russell King (Oracle) wrote:
> > On Fri, Feb 02, 2024 at 12:19:10PM +0300, Arınç ÜNAL via B4 Relay wrote:
> > > From: Arınç ÜNAL <[email protected]>
> > >
> > > The crystal frequency concerns the switch core. The frequency should be
> > > checked when the switch is being set up so the driver can reject the
> > > unsupported hardware earlier and without requiring port 6 to be used.
> > >
> > > Move it to mt7530_setup(). Drop the unnecessary function printing.
> > >
> > > Signed-off-by: Arınç ÜNAL <[email protected]>
> > > Reviewed-by: Andrew Lunn <[email protected]>
> > > Reviewed-by: Vladimir Oltean <[email protected]>
> >
> > I would prefer this to be earlier in the series, before patch 2 which
> > moves mt7530_setup_port6() to be called from mac_config(). mac_config()
> > is supposed to be configuration error-free - in other words, all state
> > should have been checked before hand.
>
> I agree but mt7530_mac_config() is not a void function yet. The
> mac_port_config member of the mt753x_info structure points to this
> function. My next patch series gets rid of all useless error returns on the
> phylink path and change mac_port_config to void. So I don't think working
> on this patch series further will worth the effort. I'd rather have this
> version applied as is.

I agree regarding not changing the patch itself, but I also agree
with Russell regarding the patch ordering. I know it's a 10-minute
git headache to rebase the patches on top of each other in a different
order, but you can easily compare the end result being identical to
what you had before and hence don't need to retest.

2024-02-02 20:26:32

by Andrew Lunn

[permalink] [raw]

Subject: Re: [PATCH net-next v3 1/7] net: dsa: mt7530: empty default case on mt7530_setup_port5()

On Fri, Feb 02, 2024 at 06:05:36PM +0000, Russell King (Oracle) wrote:
> On Fri, Feb 02, 2024 at 08:44:39PM +0300, Arınç ÜNAL wrote:
> > On 2.02.2024 14:40, Russell King (Oracle) wrote:
> > > While reviewing this change, but not related to it, I notice that this
> > > function sets the TX delay based on the RGMII interface mode. This isn't
> > > correct. I've explained why this is this many times in the past, but
> > > essentially it comes down to the model:
> > >
> > >
> > > phy-mode in NIC node Network driver PCB PHY
> > > rgmii no delays delays no delays
> > > rgmii-id no delays no delays tx/rx delays
> > > rgmii-txid no delays no delays tx delays
> > > rgmii-rxid no delays no delays rx delays
> > >
> > > Then we have rx-internal-delay-ps and tx-internal-delay-ps in the NIC
> > > node which define the RGMII delays at the local end and similar
> > > properties for the PHY node.
> > >
> > >
> > > So, if we take the view that, when a switch is connected to a NIC in
> > > RGMII mode, then the phy-mode specified delays still should not impact
> > > the local NIC.
> > >
> > > Now, for the switch, we specify the phy-mode in the port node as well.
> > > Consider the case of a switch port connected to a RGMII PHY. This has
> > > to operate in exactly the same way as a normal NIC - that is, the
> > > RGMII delays at the port should be ignored as it's the responsibility
> > > of a PHY.

Not quite. It is unusual, but there are a few MAC drivers which do act
on phy-mode, to configure MAC delays. However, if they do this, they
then mask the value of phy-mode passed to the PHY in order to avoid
double delays. Its not something i recommend, we prefer the PHYs do
the delays. And it something i strongly suggest we avoid in DSA.

> > > The final scenario to examine is the case of a RGMII switch port
> > > connected to a NIC.

This should also extend to a port connected to a port of another
switch. For Marvell switches, these are so called DSA ports.

> > > The NIC's phy-mode has no way to be communicated
> > > to DSA or vice versa, so neither phy-mode can impact the other side
> > > of the RGMII link, but should only place the link into RGMII mode.
> > > Given everything I've said above, the only way to configure RGMII
> > > delays is via the rx-internal-delay-ps and tx-internal-delay-ps
> > > properties. So, DSA drivers should _not_ be configuring their ports
> > > with RGMII delays based on the RGMII phy interface mode.

Marvell goes against this. rx-internal-delay-ps and
tx-internal-delay-ps are pretty new, when compared to the age of
mv88e6xxx. I had the problem of a FEC connected to an mv88e6xxx using
RGMII and i needed to somehow configure RGMII delays, or packets did
not get through. So mv88e6xxx will apply phy-mode to ports being used
in CPU or DSA mode. So in the case of the FEC->switch, rgmii-id is
used by the switch. For DSA->DSA, there are DT blobs with rgmii-txid
on both ends of the link, which results in the needed delays.

> > > The above is my purely logically reasoned point of view on this
> > > subject. Others may have other (to me completely illogical)
> > > interpretations that can only lead to interoperability issues.

Now that rx-internal-delay-ps and tx-internal-delay-ps actually exist,
these are the best ways to handle such delays, for new drivers. But we
cannot change old drivers without causing regressions.

Andrew

2024-02-04 14:00:56

[permalink] [raw]

Subject: Re: [PATCH net-next v3 4/7] net: dsa: mt7530: move XTAL check to mt7530_setup()

On 2.02.2024 21:39, Daniel Golle wrote:
> On Fri, Feb 02, 2024 at 09:16:02PM +0300, Arınç ÜNAL wrote:
>> On 2.02.2024 14:48, Russell King (Oracle) wrote:
>>> On Fri, Feb 02, 2024 at 12:19:10PM +0300, Arınç ÜNAL via B4 Relay wrote:
>>>> From: Arınç ÜNAL <[email protected]>
>>>>
>>>> The crystal frequency concerns the switch core. The frequency should be
>>>> checked when the switch is being set up so the driver can reject the
>>>> unsupported hardware earlier and without requiring port 6 to be used.
>>>>
>>>> Move it to mt7530_setup(). Drop the unnecessary function printing.
>>>>
>>>> Signed-off-by: Arınç ÜNAL <[email protected]>
>>>> Reviewed-by: Andrew Lunn <[email protected]>
>>>> Reviewed-by: Vladimir Oltean <[email protected]>
>>>
>>> I would prefer this to be earlier in the series, before patch 2 which
>>> moves mt7530_setup_port6() to be called from mac_config(). mac_config()
>>> is supposed to be configuration error-free - in other words, all state
>>> should have been checked before hand.
>>
>> I agree but mt7530_mac_config() is not a void function yet. The
>> mac_port_config member of the mt753x_info structure points to this
>> function. My next patch series gets rid of all useless error returns on the
>> phylink path and change mac_port_config to void. So I don't think working
>> on this patch series further will worth the effort. I'd rather have this
>> version applied as is.
>
> I agree regarding not changing the patch itself, but I also agree
> with Russell regarding the patch ordering. I know it's a 10-minute
> git headache to rebase the patches on top of each other in a different
> order, but you can easily compare the end result being identical to
> what you had before and hence don't need to retest.

This is not about laziness. This is before patch 2:

phylink_mac_ops :: mac_config() -> dsa_port_phylink_mac_config()
-> dsa_switch_ops :: phylink_mac_config() -> mt753x_phylink_mac_config()
-> mt753x_mac_config()
-> mt753x_info :: mac_port_config() -> mt7530_mac_config()
-> mt7530_setup_port5()
-> mt753x_pad_setup()
-> mt753x_info :: pad_setup() -> mt7530_pad_clk_setup()

This is after:

phylink_mac_ops :: mac_config() -> dsa_port_phylink_mac_config()
-> dsa_switch_ops :: phylink_mac_config() -> mt753x_phylink_mac_config()
-> mt753x_mac_config()
-> mt753x_info :: mac_port_config() -> mt7530_mac_config()
-> mt7530_setup_port5()
-> mt7530_setup_port6()

Patch 2 does not move mt7530_setup_port6() to be called from
phylink_mac_ops :: mac_config(), it already is. There is no valid reason to
reorder the patches.

My response to Russell should've stated this instead of focusing on his
second sentence.

pw-bot: under-review

Arınç

2024-02-04 14:19:16

[permalink] [raw]

Subject: Re: [PATCH net-next v3 4/7] net: dsa: mt7530: move XTAL check to mt7530_setup()

On Sun, Feb 04, 2024 at 04:55:40PM +0300, Arınç ÜNAL wrote:
> On 2.02.2024 21:39, Daniel Golle wrote:
> > On Fri, Feb 02, 2024 at 09:16:02PM +0300, Arınç ÜNAL wrote:
> > > On 2.02.2024 14:48, Russell King (Oracle) wrote:
> > > > On Fri, Feb 02, 2024 at 12:19:10PM +0300, Arınç ÜNAL via B4 Relay wrote:
> > > > > From: Arınç ÜNAL <[email protected]>
> > > > >
> > > > > The crystal frequency concerns the switch core. The frequency should be
> > > > > checked when the switch is being set up so the driver can reject the
> > > > > unsupported hardware earlier and without requiring port 6 to be used.
> > > > >
> > > > > Move it to mt7530_setup(). Drop the unnecessary function printing.
> > > > >
> > > > > Signed-off-by: Arınç ÜNAL <[email protected]>
> > > > > Reviewed-by: Andrew Lunn <[email protected]>
> > > > > Reviewed-by: Vladimir Oltean <[email protected]>
> > > >
> > > > I would prefer this to be earlier in the series, before patch 2 which
> > > > moves mt7530_setup_port6() to be called from mac_config(). mac_config()
> > > > is supposed to be configuration error-free - in other words, all state
> > > > should have been checked before hand.
> > >
> > > I agree but mt7530_mac_config() is not a void function yet. The
> > > mac_port_config member of the mt753x_info structure points to this
> > > function. My next patch series gets rid of all useless error returns on the
> > > phylink path and change mac_port_config to void. So I don't think working
> > > on this patch series further will worth the effort. I'd rather have this
> > > version applied as is.
> >
> > I agree regarding not changing the patch itself, but I also agree
> > with Russell regarding the patch ordering. I know it's a 10-minute
> > git headache to rebase the patches on top of each other in a different
> > order, but you can easily compare the end result being identical to
> > what you had before and hence don't need to retest.
>
> This is not about laziness. This is before patch 2:
>
> phylink_mac_ops :: mac_config() -> dsa_port_phylink_mac_config()
> -> dsa_switch_ops :: phylink_mac_config() -> mt753x_phylink_mac_config()
> -> mt753x_mac_config()
> -> mt753x_info :: mac_port_config() -> mt7530_mac_config()
> -> mt7530_setup_port5()
> -> mt753x_pad_setup()
> -> mt753x_info :: pad_setup() -> mt7530_pad_clk_setup()
>
> This is after:
>
> phylink_mac_ops :: mac_config() -> dsa_port_phylink_mac_config()
> -> dsa_switch_ops :: phylink_mac_config() -> mt753x_phylink_mac_config()
> -> mt753x_mac_config()
> -> mt753x_info :: mac_port_config() -> mt7530_mac_config()
> -> mt7530_setup_port5()
> -> mt7530_setup_port6()
>
> Patch 2 does not move mt7530_setup_port6() to be called from
> phylink_mac_ops :: mac_config(), it already is. There is no valid reason to
> reorder the patches.
>
> My response to Russell should've stated this instead of focusing on his
> second sentence.

This patch moves the test for a 20MHz crystal to mt7530_setup(),
which is something that is entirely orthogonal to patch 2, which
can be done cleanly (I've just applied the patches in the original
order and then reordered them:

98c481f5d706 net: dsa: mt7530: do not clear config->supported_interfaces
93c6b53b17f4 net: dsa: mt7530: correct port capabilities of MT7988
c9c6d4c51a1d net: dsa: mt7530: simplify mt7530_setup_port6() and change to void
adfa948253e0 net: dsa: mt7530: remove pad_setup function pointer
57e21e6c2fc0 net: dsa: mt7530: call port 6 setup from mt7530_mac_config()
959a0f9323c8 net: dsa: mt7530: move XTAL check to mt7530_setup()
856ab64a22ef net: dsa: mt7530: empty default case on mt7530_setup_port5()

No problems. The end result is identical comparing the git tree at the
original "move XTAL" patch with adfa948253e0.

Now, if we look at "net: dsa: mt7530: remove pad_setup function pointer"
we can see that yes, the pad_setup() method was called from mac_confing,
but this is the exact contents of that patch removing the callsite:

- mt753x_pad_setup(ds, state);

This returns an integer, which may be an error code, which is ignored.
Therefore, if the XTAL frequency check fires, and mt753x_pad_setup()
returns an error, it is ignored today.

After "net: dsa: mt7530: call port 6 setup from mt7530_mac_config()"
the renamed pad_setup() method is now called from mac_config() thusly:

+ ret = mt7530_setup_port6(priv->ds, interface);
+ if (ret)
+ return ret;

So now the error checks cause mt7530_mac_config() to return an error
which in turn causes mt753x_mac_config() to fail, and therefore
mt753x_phylink_mac_config() has different behaviour.

So, patch 2 changes the driver behaviour in the case of a 20MHz XTAL,
which is then changed again by patch 4.

It would be better to have only one change of behaviour by moving
patch 4 before patch 2.

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

2024-02-04 15:55:48

[permalink] [raw]

Subject: Re: [PATCH net-next v3 4/7] net: dsa: mt7530: move XTAL check to mt7530_setup()

On 4.02.2024 17:18, Russell King (Oracle) wrote:
> On Sun, Feb 04, 2024 at 04:55:40PM +0300, Arınç ÜNAL wrote:
>> This is not about laziness. This is before patch 2:
>>
>> phylink_mac_ops :: mac_config() -> dsa_port_phylink_mac_config()
>> -> dsa_switch_ops :: phylink_mac_config() -> mt753x_phylink_mac_config()
>> -> mt753x_mac_config()
>> -> mt753x_info :: mac_port_config() -> mt7530_mac_config()
>> -> mt7530_setup_port5()
>> -> mt753x_pad_setup()
>> -> mt753x_info :: pad_setup() -> mt7530_pad_clk_setup()
>>
>> This is after:
>>
>> phylink_mac_ops :: mac_config() -> dsa_port_phylink_mac_config()
>> -> dsa_switch_ops :: phylink_mac_config() -> mt753x_phylink_mac_config()
>> -> mt753x_mac_config()
>> -> mt753x_info :: mac_port_config() -> mt7530_mac_config()
>> -> mt7530_setup_port5()
>> -> mt7530_setup_port6()
>>
>> Patch 2 does not move mt7530_setup_port6() to be called from
>> phylink_mac_ops :: mac_config(), it already is. There is no valid reason to
>> reorder the patches.
>>
>> My response to Russell should've stated this instead of focusing on his
>> second sentence.
>
> This patch moves the test for a 20MHz crystal to mt7530_setup(),
> which is something that is entirely orthogonal to patch 2, which
> can be done cleanly (I've just applied the patches in the original
> order and then reordered them:
>
> 98c481f5d706 net: dsa: mt7530: do not clear config->supported_interfaces
> 93c6b53b17f4 net: dsa: mt7530: correct port capabilities of MT7988
> c9c6d4c51a1d net: dsa: mt7530: simplify mt7530_setup_port6() and change to void
> adfa948253e0 net: dsa: mt7530: remove pad_setup function pointer
> 57e21e6c2fc0 net: dsa: mt7530: call port 6 setup from mt7530_mac_config()
> 959a0f9323c8 net: dsa: mt7530: move XTAL check to mt7530_setup()
> 856ab64a22ef net: dsa: mt7530: empty default case on mt7530_setup_port5()
>
> No problems. The end result is identical comparing the git tree at the
> original "move XTAL" patch with adfa948253e0.
>
> Now, if we look at "net: dsa: mt7530: remove pad_setup function pointer"
> we can see that yes, the pad_setup() method was called from mac_confing,
> but this is the exact contents of that patch removing the callsite:
>
> - mt753x_pad_setup(ds, state);
>
> This returns an integer, which may be an error code, which is ignored.
> Therefore, if the XTAL frequency check fires, and mt753x_pad_setup()
> returns an error, it is ignored today.
>
> After "net: dsa: mt7530: call port 6 setup from mt7530_mac_config()"
> the renamed pad_setup() method is now called from mac_config() thusly:
>
> + ret = mt7530_setup_port6(priv->ds, interface);
> + if (ret)
> + return ret;
>
> So now the error checks cause mt7530_mac_config() to return an error
> which in turn causes mt753x_mac_config() to fail, and therefore
> mt753x_phylink_mac_config() has different behaviour.
>
> So, patch 2 changes the driver behaviour in the case of a 20MHz XTAL,
> which is then changed again by patch 4.
>
> It would be better to have only one change of behaviour by moving
> patch 4 before patch 2.

If the idea is to not bring any more error returns to mt753x_mac_config()
because the return code is actually checked for that, I should do a bit
more effort and put patch 5 before patch 2 as well, to live up to what you
originally requested. Because, to get rid of all error returns on
mt7530_setup_port6(), both "net: dsa: mt7530: move XTAL check to
mt7530_setup()" and "net: dsa: mt7530: simplify mt7530_setup_port6() and
change to void" patches are needed. After these patches, I can move
mt7530_setup_port6() to mt7530_mac_config() and there won't be any error
returns being brought under mt753x_mac_config().

pw-bot: cr

Arınç

2024-02-04 16:52:37

[permalink] [raw]

Subject: Re: [PATCH net-next v3 4/7] net: dsa: mt7530: move XTAL check to mt7530_setup()

On 4.02.2024 19:38, Russell King (Oracle) wrote:
> On Sun, Feb 04, 2024 at 06:55:14PM +0300, Arınç ÜNAL wrote:
>> On 4.02.2024 17:18, Russell King (Oracle) wrote:
>>> On Sun, Feb 04, 2024 at 04:55:40PM +0300, Arınç ÜNAL wrote:
>>>> This is not about laziness. This is before patch 2:
>>>>
>>>> phylink_mac_ops :: mac_config() -> dsa_port_phylink_mac_config()
>>>> -> dsa_switch_ops :: phylink_mac_config() -> mt753x_phylink_mac_config()
>>>> -> mt753x_mac_config()
>>>> -> mt753x_info :: mac_port_config() -> mt7530_mac_config()
>>>> -> mt7530_setup_port5()
>>>> -> mt753x_pad_setup()
>>>> -> mt753x_info :: pad_setup() -> mt7530_pad_clk_setup()
>>>>
>>>> This is after:
>>>>
>>>> phylink_mac_ops :: mac_config() -> dsa_port_phylink_mac_config()
>>>> -> dsa_switch_ops :: phylink_mac_config() -> mt753x_phylink_mac_config()
>>>> -> mt753x_mac_config()
>>>> -> mt753x_info :: mac_port_config() -> mt7530_mac_config()
>>>> -> mt7530_setup_port5()
>>>> -> mt7530_setup_port6()
>>>>
>>>> Patch 2 does not move mt7530_setup_port6() to be called from
>>>> phylink_mac_ops :: mac_config(), it already is. There is no valid reason to
>>>> reorder the patches.
>>>>
>>>> My response to Russell should've stated this instead of focusing on his
>>>> second sentence.
>>>
>>> This patch moves the test for a 20MHz crystal to mt7530_setup(),
>>> which is something that is entirely orthogonal to patch 2, which
>>> can be done cleanly (I've just applied the patches in the original
>>> order and then reordered them:
>>>
>>> 98c481f5d706 net: dsa: mt7530: do not clear config->supported_interfaces
>>> 93c6b53b17f4 net: dsa: mt7530: correct port capabilities of MT7988
>>> c9c6d4c51a1d net: dsa: mt7530: simplify mt7530_setup_port6() and change to void
>>> adfa948253e0 net: dsa: mt7530: remove pad_setup function pointer
>>> 57e21e6c2fc0 net: dsa: mt7530: call port 6 setup from mt7530_mac_config()
>>> 959a0f9323c8 net: dsa: mt7530: move XTAL check to mt7530_setup()
>>> 856ab64a22ef net: dsa: mt7530: empty default case on mt7530_setup_port5()
>>>
>>> No problems. The end result is identical comparing the git tree at the
>>> original "move XTAL" patch with adfa948253e0.
>>>
>>> Now, if we look at "net: dsa: mt7530: remove pad_setup function pointer"
>>> we can see that yes, the pad_setup() method was called from mac_confing,
>>> but this is the exact contents of that patch removing the callsite:
>>>
>>> - mt753x_pad_setup(ds, state);
>>>
>>> This returns an integer, which may be an error code, which is ignored.
>>> Therefore, if the XTAL frequency check fires, and mt753x_pad_setup()
>>> returns an error, it is ignored today.
>>>
>>> After "net: dsa: mt7530: call port 6 setup from mt7530_mac_config()"
>>> the renamed pad_setup() method is now called from mac_config() thusly:
>>>
>>> + ret = mt7530_setup_port6(priv->ds, interface);
>>> + if (ret)
>>> + return ret;
>>>
>>> So now the error checks cause mt7530_mac_config() to return an error
>>> which in turn causes mt753x_mac_config() to fail, and therefore
>>> mt753x_phylink_mac_config() has different behaviour.
>>>
>>> So, patch 2 changes the driver behaviour in the case of a 20MHz XTAL,
>>> which is then changed again by patch 4.
>>>
>>> It would be better to have only one change of behaviour by moving
>>> patch 4 before patch 2.
>>
>> If the idea is to not bring any more error returns to mt753x_mac_config()
>> because the return code is actually checked for that, I should do a bit
>> more effort and put patch 5 before patch 2 as well, to live up to what you
>> originally requested.
>
> I assume you are referring to getting rid of the default case in
> mt7530_pad_clk_setup().
>
> In patch "net: dsa: mt7530: call port 6 setup from mt7530_mac_config()"
> where you move this to be called from mt7530_mac_config(), you add it
> as:
>
> + } else if (port == 6) {
> + ret = mt7530_setup_port6(priv->ds, interface);
> + if (ret)
> + return ret;
> + }
>
> So it is only called for port 6. The switch within the called function
> deals with PHY_INTERFACE_MODE_RGMII and PHY_INTERFACE_MODE_TRGMII.
> Anything else results in the use of the default case, and thus
> returning an error.
>
> Since mt7530_mac_port_get_caps() does this for port 6:
>
> __set_bit(PHY_INTERFACE_MODE_RGMII,
> config->supported_interfaces);
> __set_bit(PHY_INTERFACE_MODE_TRGMII,
> config->supported_interfaces);
>
> mt7530_setup_port6() will only ever be called for these two modes,
> which means that the default case is unreachable, thus we will never
> execute that path, thus whether that path returns an error or not is
> completely irrelevant.
>
> The only case in mt7530_setup_port6() / mt7530_pad_clk_setup() which
> can today return an error is the XTAL check.
>
> Therefore, my suggestion makes complete sense, and there is no need
> to also move patch 5.

Understood. I've already submitted v4 which moves patch 5. The remaining
benefit is that there're fewer code changes as I don't need to add an error
return for mt7530_setup_port6() and then remove it.

Arınç

2024-02-04 17:08:24

[permalink] [raw]

Subject: Re: [PATCH net-next v3 4/7] net: dsa: mt7530: move XTAL check to mt7530_setup()

On Sun, Feb 04, 2024 at 07:51:49PM +0300, Arınç ÜNAL wrote:
> On 4.02.2024 19:38, Russell King (Oracle) wrote:
> > On Sun, Feb 04, 2024 at 06:55:14PM +0300, Arınç ÜNAL wrote:
> > > On 4.02.2024 17:18, Russell King (Oracle) wrote:
> > > > On Sun, Feb 04, 2024 at 04:55:40PM +0300, Arınç ÜNAL wrote:
> > > > > This is not about laziness. This is before patch 2:
> > > > >
> > > > > phylink_mac_ops :: mac_config() -> dsa_port_phylink_mac_config()
> > > > > -> dsa_switch_ops :: phylink_mac_config() -> mt753x_phylink_mac_config()
> > > > > -> mt753x_mac_config()
> > > > > -> mt753x_info :: mac_port_config() -> mt7530_mac_config()
> > > > > -> mt7530_setup_port5()
> > > > > -> mt753x_pad_setup()
> > > > > -> mt753x_info :: pad_setup() -> mt7530_pad_clk_setup()
> > > > >
> > > > > This is after:
> > > > >
> > > > > phylink_mac_ops :: mac_config() -> dsa_port_phylink_mac_config()
> > > > > -> dsa_switch_ops :: phylink_mac_config() -> mt753x_phylink_mac_config()
> > > > > -> mt753x_mac_config()
> > > > > -> mt753x_info :: mac_port_config() -> mt7530_mac_config()
> > > > > -> mt7530_setup_port5()
> > > > > -> mt7530_setup_port6()
> > > > >
> > > > > Patch 2 does not move mt7530_setup_port6() to be called from
> > > > > phylink_mac_ops :: mac_config(), it already is. There is no valid reason to
> > > > > reorder the patches.
> > > > >
> > > > > My response to Russell should've stated this instead of focusing on his
> > > > > second sentence.
> > > >
> > > > This patch moves the test for a 20MHz crystal to mt7530_setup(),
> > > > which is something that is entirely orthogonal to patch 2, which
> > > > can be done cleanly (I've just applied the patches in the original
> > > > order and then reordered them:
> > > >
> > > > 98c481f5d706 net: dsa: mt7530: do not clear config->supported_interfaces
> > > > 93c6b53b17f4 net: dsa: mt7530: correct port capabilities of MT7988
> > > > c9c6d4c51a1d net: dsa: mt7530: simplify mt7530_setup_port6() and change to void
> > > > adfa948253e0 net: dsa: mt7530: remove pad_setup function pointer
> > > > 57e21e6c2fc0 net: dsa: mt7530: call port 6 setup from mt7530_mac_config()
> > > > 959a0f9323c8 net: dsa: mt7530: move XTAL check to mt7530_setup()
> > > > 856ab64a22ef net: dsa: mt7530: empty default case on mt7530_setup_port5()
> > > >
> > > > No problems. The end result is identical comparing the git tree at the
> > > > original "move XTAL" patch with adfa948253e0.
> > > >
> > > > Now, if we look at "net: dsa: mt7530: remove pad_setup function pointer"
> > > > we can see that yes, the pad_setup() method was called from mac_confing,
> > > > but this is the exact contents of that patch removing the callsite:
> > > >
> > > > - mt753x_pad_setup(ds, state);
> > > >
> > > > This returns an integer, which may be an error code, which is ignored.
> > > > Therefore, if the XTAL frequency check fires, and mt753x_pad_setup()
> > > > returns an error, it is ignored today.
> > > >
> > > > After "net: dsa: mt7530: call port 6 setup from mt7530_mac_config()"
> > > > the renamed pad_setup() method is now called from mac_config() thusly:
> > > >
> > > > + ret = mt7530_setup_port6(priv->ds, interface);
> > > > + if (ret)
> > > > + return ret;
> > > >
> > > > So now the error checks cause mt7530_mac_config() to return an error
> > > > which in turn causes mt753x_mac_config() to fail, and therefore
> > > > mt753x_phylink_mac_config() has different behaviour.
> > > >
> > > > So, patch 2 changes the driver behaviour in the case of a 20MHz XTAL,
> > > > which is then changed again by patch 4.
> > > >
> > > > It would be better to have only one change of behaviour by moving
> > > > patch 4 before patch 2.
> > >
> > > If the idea is to not bring any more error returns to mt753x_mac_config()
> > > because the return code is actually checked for that, I should do a bit
> > > more effort and put patch 5 before patch 2 as well, to live up to what you
> > > originally requested.
> >
> > I assume you are referring to getting rid of the default case in
> > mt7530_pad_clk_setup().
> >
> > In patch "net: dsa: mt7530: call port 6 setup from mt7530_mac_config()"
> > where you move this to be called from mt7530_mac_config(), you add it
> > as:
> >
> > + } else if (port == 6) {
> > + ret = mt7530_setup_port6(priv->ds, interface);
> > + if (ret)
> > + return ret;
> > + }
> >
> > So it is only called for port 6. The switch within the called function
> > deals with PHY_INTERFACE_MODE_RGMII and PHY_INTERFACE_MODE_TRGMII.
> > Anything else results in the use of the default case, and thus
> > returning an error.
> >
> > Since mt7530_mac_port_get_caps() does this for port 6:
> >
> > __set_bit(PHY_INTERFACE_MODE_RGMII,
> > config->supported_interfaces);
> > __set_bit(PHY_INTERFACE_MODE_TRGMII,
> > config->supported_interfaces);
> >
> > mt7530_setup_port6() will only ever be called for these two modes,
> > which means that the default case is unreachable, thus we will never
> > execute that path, thus whether that path returns an error or not is
> > completely irrelevant.
> >
> > The only case in mt7530_setup_port6() / mt7530_pad_clk_setup() which
> > can today return an error is the XTAL check.
> >
> > Therefore, my suggestion makes complete sense, and there is no need
> > to also move patch 5.
>
> Understood. I've already submitted v4 which moves patch 5. The remaining
> benefit is that there're fewer code changes as I don't need to add an error
> return for mt7530_setup_port6() and then remove it.

I'm pretty sure netdev has a rule _not_ to submit the next version of
a patch series if discussion about the previous is still ongoing...
but I can't find any of the netdev rules in Documentation/networking
anymore, and nothing stands out in Documentation/process... ah,
Documentation/process/maintainer-netdev.rst, and here we are:

"Make sure you address all the feedback in your new posting. Do not post
a new
version of the code if the discussion about the previous version is
still
ongoing, unless directly instructed by a reviewer."

Discussion was still ongoing over this point, so ideally you should
have waited instead of presenting a fait-accompli _during_ the
ongoing discussion.

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

2024-02-04 17:15:03