| Message ID | 20240113102529.80371-7-arinc.unal@arinc9.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | MT7530 DSA Subdriver Improvements Act II | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next |
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ç
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; }
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(-)