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 <arinc.unal@arinc9.com>
---
 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 3ce4e0bb04dd..3a02308763ca 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

On Sat, Jan 13, 2024 at 01:25:27PM +0300, Arınç ÜNAL wrote:
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 3ce4e0bb04dd..3a02308763ca 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;

It would be good to add a comment here which states that the port comes
out of reset with values good for RGMII.

Also, there's a built-in assumption in this patch, that dynamically
switching between RGMII and TRGMII is not possible. This is because
phylink mac_config() is not necesarily called only once immediately
after reset, but after each major_config().

The fact that the driver sets both PHY_INTERFACE_MODE_RGMII and
PHY_INTERFACE_MODE_TRGMII at once in config->supported_interfaces does
not disprove that dynamic reconfiguration is possible. Normally,
interfaces for which it doesn't make sense to dynamically reconfigure
(they are wired to fixed pinout) have a single bit set in
supported_interfaces. Is this switching something that makes any sense
at all, given that port 6 is internal? It's not something that phylink
knows to do today, but if this is theoretically possible and remotely
useful, someone might end up wanting, in the future, to revert this patch.

On 16.01.2024 00:37, Vladimir Oltean wrote:
> On Sat, Jan 13, 2024 at 01:25:27PM +0300, Arınç ÜNAL wrote:
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index 3ce4e0bb04dd..3a02308763ca 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;
> 
> It would be good to add a comment here which states that the port comes
> out of reset with values good for RGMII.
> 
> Also, there's a built-in assumption in this patch, that dynamically
> switching between RGMII and TRGMII is not possible. This is because
> phylink mac_config() is not necesarily called only once immediately
> after reset, but after each major_config().
> 
> The fact that the driver sets both PHY_INTERFACE_MODE_RGMII and
> PHY_INTERFACE_MODE_TRGMII at once in config->supported_interfaces does
> not disprove that dynamic reconfiguration is possible. Normally,
> interfaces for which it doesn't make sense to dynamically reconfigure
> (they are wired to fixed pinout) have a single bit set in
> supported_interfaces. Is this switching something that makes any sense
> at all, given that port 6 is internal? It's not something that phylink
> knows to do today, but if this is theoretically possible and remotely
> useful, someone might end up wanting, in the future, to revert this patch.

Do you mean by internal port that the port does not have MII pinout? Port 6
of the MT7530 switch do. It is possible to have an external PHY wired to
it. So it would make sense to design mt7530_setup_port6() in the sense that
dynamic reconfiguration is possible.

I've tested to see that the core operations for TRGMII does not interfere
so no need to undo them when the interface changes from TRGMII to RGMII.

I'll do below on this patch:

	if (interface == PHY_INTERFACE_MODE_RGMII) {
		mt7530_rmw(priv, MT7530_P6ECR, P6_INTF_MODE_MASK,
			   P6_INTF_MODE(0));
		return;
	}

Arınç

On Tue, Jan 16, 2024 at 04:09:18PM +0300, Arınç ÜNAL wrote:
> Do you mean by internal port that the port does not have MII pinout? Port 6
> of the MT7530 switch do. It is possible to have an external PHY wired to it.

Yes, this is what I meant by internal port. It seems I was wrong to
assume it is always connected to GMAC0.

How is the selection done between internal and external wiring?

If external wiring to a PHY is possible, shouldn't the driver accept all
4 RGMII variants with phy_interface_mode_is_rgmii(), because the delays
specified in "rgmii-txid", "rgmii-rxid", "rgmii-id" always pertain to
the PHY, and thus it doesn't make sense for the MAC to not allow the use
of the full spectrum?

> So it would make sense to design mt7530_setup_port6() in the sense that
> dynamic reconfiguration is possible.

Ok, you mean to keep the dynamic reconfiguration possible rather than
redesign to disallow it.

> I've tested to see that the core operations for TRGMII does not interfere
> so no need to undo them when the interface changes from TRGMII to RGMII.
> 
> I'll do below on this patch:
> 
> 	if (interface == PHY_INTERFACE_MODE_RGMII) {
> 		mt7530_rmw(priv, MT7530_P6ECR, P6_INTF_MODE_MASK,
> 			   P6_INTF_MODE(0));
> 		return;
> 	}

Ok.

On 16.01.2024 16:47, Vladimir Oltean wrote:
> On Tue, Jan 16, 2024 at 04:09:18PM +0300, Arınç ÜNAL wrote:
>> Do you mean by internal port that the port does not have MII pinout? Port 6
>> of the MT7530 switch do. It is possible to have an external PHY wired to it.
> 
> Yes, this is what I meant by internal port. It seems I was wrong to
> assume it is always connected to GMAC0.
> 
> How is the selection done between internal and external wiring?

There are two variants of MT7530. One is standalone, the other comes with
the SoC as a part of the multi-chip module. More information at
mediatek,mt7530.yaml.

The standalone one is straightforward. A MAC or a PHY can be wired to the
MII pinouts of port 5 and 6 on the PCB, just as the relevant MII standard
describes.

On the MT7621 SoCs which include the switch IC, port 6 is wired to GMAC0,
port 5 is wired to GMAC1. I assume you mean internal and external wiring in
reference to this case. This is the internal wiring.

The external wiring works by wiring the PHY or MAC to the MII pinouts of
the SoC's two MACs. Since RX of the switch MAC is wired to TX of the SoC
MAC and vice versa, the external PHY or MAC must be wired TX to TX and RX
to RX. Ubiquiti EdgeRouter X SFP is wired this way. The wiring for clock
pins may need to be mirrored too, I haven't studied the RGMII specification
that much in detail. This works by not enabling the SoC MAC.

> 
> If external wiring to a PHY is possible, shouldn't the driver accept all
> 4 RGMII variants with phy_interface_mode_is_rgmii(), because the delays
> specified in "rgmii-txid", "rgmii-rxid", "rgmii-id" always pertain to
> the PHY, and thus it doesn't make sense for the MAC to not allow the use
> of the full spectrum?

Great point. I think delays are not supported on port 6. There's only the
"MT7530 Giga Switch programming guide v0.1" document mentioning setting
delays on page 8, and it's only for port 5. It is also implemented on
mt7530_setup_port5():

/* P5 RGMII RX Clock Control: delay setting for 1000M */
mt7530_write(priv, MT7530_P5RGMIIRXCR, CSR_RGMII_EDGE_ALIGN);

/* Don't set delay in DSA mode */
if (!dsa_is_dsa_port(priv->ds, 5) &&
     (interface == PHY_INTERFACE_MODE_RGMII_TXID ||
      interface == PHY_INTERFACE_MODE_RGMII_ID))
	tx_delay = 4; /* n * 0.5 ns */

/* P5 RGMII TX Clock Control: delay x */
mt7530_write(priv, MT7530_P5RGMIITXCR,
	     CSR_RGMII_TXC_CFG(0x10 + tx_delay));

There's only the TX driving mentioned for port 6 on the document. I'm
guessing port 6 was intended to connect to another TRGMII capable MAC so
delays were out of the question. TRGMII is just overclocked RGMII to
provide up to 2Gbps TX/RX, at least in theory. The whole existence of the
TRGMII interface on Linux is only being used by the MT7621 and MT7623 SoC
MACs, and port 6 MAC of the MT7530 switch.

> 
>> So it would make sense to design mt7530_setup_port6() in the sense that
>> dynamic reconfiguration is possible.
> 
> Ok, you mean to keep the dynamic reconfiguration possible rather than
> redesign to disallow it.

Very much so.

Arınç