Re: [RFC PATCH net] net: dsa: mt7530: move PLL setup out of port 6 pad configuration
From: Arınç ÜNAL
Date: Mon Mar 06 2023 - 12:05:32 EST
On 6.03.2023 18:45, Vladimir Oltean wrote:
Hi Arınç,
On Sat, Mar 04, 2023 at 03:54:54PM +0300, arinc9.unal@xxxxxxxxx wrote:
From: Arınç ÜNAL <arinc.unal@xxxxxxxxxx>
Move the PLL setup of the MT7530 switch out of the pad configuration of
port 6 to mt7530_setup, after reset.
it would have been good if this patch had done that and only that, no?
Agreed.
This fixes the improper initialisation of the switch when only port 5 is
used as a CPU port.
Add supported phy modes of port 5 on the PLL setup.
Remove now incorrect comment regarding P5 as GMAC5.
Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
Fixes: 38f790a80560 ("net: dsa: mt7530: Add support for port 5")
Signed-off-by: Arınç ÜNAL <arinc.unal@xxxxxxxxxx>
---
I'm trying to mimic this change by Alexander [0] for the MT7530 switch.
This is already the case for MT7530 and MT7531 on the MediaTek ethernet
driver on U-Boot [1] [2].
[0] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=42bc4fafe359ed6b73602b7a2dba0dd99588f8ce
[1] https://github.com/u-boot/u-boot/blob/a94ab561e2f49a80d8579930e840b810ab1a1330/drivers/net/mtk_eth.c#L729
[2] https://github.com/u-boot/u-boot/blob/a94ab561e2f49a80d8579930e840b810ab1a1330/drivers/net/mtk_eth.c#L903
There are some parts I couldn't figure out myself with my limited C
knowledge. I've pointed them out on the code. Vladimir, could you help?
There is a lot of code which is only needed for port 6 or trgmii on port 6,
but runs whether port 6 or trgmii is used or not. For now, the best I can
do is to fix port 5 so it works without port 6 being used.
The U-Boot driver seems to be much more organised so it could be taken as
a reference to sort out this DSA driver further.
Also, now that the pad setup for mt7530 is also moved somewhere else, can
we completely get rid of @pad_setup?
Arınç
I don't like the strategy you used in this patch here, and I don't want
to answer these questions just yet, because I'm not certain that my
answers will be useful in the end.
---
drivers/net/dsa/mt7530.c | 46 +++++++++++++++++++++++++++++-----------
1 file changed, 34 insertions(+), 12 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 0e99de26d159..fb20ce4f443e 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -395,9 +395,8 @@ mt7530_fdb_write(struct mt7530_priv *priv, u16 vid,
/* Setup TX circuit including relevant PAD and driving */
static int
-mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
+mt7530_pad_clk_setup(struct mt7530_priv *priv, phy_interface_t interface)
{
- struct mt7530_priv *priv = ds->priv;
u32 ncpo1, ssc_delta, trgint, i, xtal;
xtal = mt7530_read(priv, MT7530_MHWTRAP) & HWTRAP_XTAL_MASK;
@@ -409,9 +408,32 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
return -EINVAL;
}
+ /* Is setting trgint to 0 really needed for the !trgint check? */
+ trgint = 0;
+
+ /* FIXME: run this switch if p5 is defined on the devicetree */
+ /* and change interface to the phy-mode of port 5 */
+ switch (interface) {
so this should be something like priv->p5_interface (assuming this is
initialized by the time mt7530_pad_clk_setup() is called, which it isn't).
+ case PHY_INTERFACE_MODE_GMII:
+ /* PLL frequency: 125MHz */
+ ncpo1 = 0x0c80;
+ break;
+ case PHY_INTERFACE_MODE_MII:
+ break;
so with priv->p5_interface == "mii", ncpo1 is uninitialized (will be
potentially still initialized by the port 6 "switch" statement below).
+ case PHY_INTERFACE_MODE_RGMII:
+ /* PLL frequency: 125MHz */
+ ncpo1 = 0x0c80;
+ break;
+ default:
+ dev_err(priv->dev, "xMII interface %d not supported\n",
+ interface);
+ return -EINVAL;
+ }
What method did you use to determine the ncpo1 values here? Are they
coordinated with what p6 wants? I would expect to see a table with
priv->p5_interface priv->p6_interface ncpo1 value
gmii rgmii ???
mii rgmii ???
rgmii rgmii ???
gmii trgmii ???
mii trgmii ???
rgmii trgmii ???
Looking at the Wikipedia page for Media-independent interface [0], the
data interface must be clocked at 125 MHz for gigabit MIIs, which I
believe what the "PLL" here refers to. trgmii needs higher frequency in
some cases so if both CPU ports are enabled, the table would be:
priv->p5_interface priv->p6_interface ncpo1 value
gmii rgmii 125MHz
mii rgmii 125MHz
rgmii rgmii 125MHz
gmii trgmii 125-250MHz
mii trgmii 125-250MHz
rgmii trgmii 125-250MHz
right now, you let the p6_interface logic overwrite the ncpo1 selected
by the p5_interface logic like crazy, and it's not clear to me that this
is what you want.
This seems to be fine as p6 sets the frequency either the same or higher.
This problem is way deeper than knowledge of the C language, your pseudo
code simply does not describe what should happen in all combinations.
From our private conversation dated Feb 08, I guess that what you're
doing is also a useless complication and not what you truly want. What
you truly want is for the CORE_GSWPLL_GRP2 register to be programmed
regardless of whether port 6 is used or not.
+
+ /* FIXME: run this switch if p6 is defined on the devicetree */
+ /* and change interface to the phy-mode of port 6 */
switch (interface) {
same comment as above, this should approximate priv->p6_interface.
case PHY_INTERFACE_MODE_RGMII:
- trgint = 0;
/* PLL frequency: 125MHz */
ncpo1 = 0x0c80;
break;
@@ -2172,7 +2194,11 @@ mt7530_setup(struct dsa_switch *ds)
SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
SYS_CTRL_REG_RST);
- /* Enable Port 6 only; P5 as GMAC5 which currently is not supported */
+ /* Setup switch core pll */
+ /* FIXME: feed the phy-mode of port 5 and 6, if the ports are defined on the devicetree */
+ mt7530_pad_clk_setup(priv, interface);
"interface" is completely uninitialized here.
Sorry, I'm not reviewing this any further, because it obviously doesn't work.
+
+ /* Enable Port 6 */
val = mt7530_read(priv, MT7530_MHWTRAP);
val &= ~MHWTRAP_P6_DIS & ~MHWTRAP_PHY_ACCESS;
val |= MHWTRAP_MANUAL;
@@ -2491,11 +2517,9 @@ static void mt7531_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)
+mt7530_pad_setup(struct dsa_switch *ds, phy_interface_t interface)
{
- struct mt7530_priv *priv = ds->priv;
-
- return priv->info->pad_setup(ds, state->interface);
+ return 0;
}
static int
@@ -2769,8 +2793,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;
@@ -3187,7 +3209,7 @@ static 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,
+ .pad_setup = mt7530_pad_setup,
.mac_port_get_caps = mt7530_mac_port_get_caps,
.mac_port_config = mt7530_mac_config,
},
@@ -3199,7 +3221,7 @@ static 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,
+ .pad_setup = mt7530_pad_setup,
as a general rule, for bug fixes don't touch stuff that you don't need to touch
.mac_port_get_caps = mt7530_mac_port_get_caps,
.mac_port_config = mt7530_mac_config,
},
--
2.37.2
Could you please let me know if this patch works, instead of what you've
posted above? It does what you *said* you tried to do, aka it performs
the initialization of CORE_GSWPLL_GRP2 independently of port 6 setup.
There is a slight reordering of register writes with MT7530_P6ECR, but
hopefully that doesn't bother anyone.
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 3a15015bc409..a508402c4ecb 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -393,6 +393,24 @@ mt7530_fdb_write(struct mt7530_priv *priv, u16 vid,
mt7530_write(priv, MT7530_ATA1 + (i * 4), reg[i]);
}
+/* Set up switch core clock for MT7530 */
+static void mt7530_pll_setup(struct mt7530_priv *priv)
+{
+ /* Disable PLL */
+ core_write(priv, CORE_GSWPLL_GRP1, 0);
+
+ /* Set core clock into 500Mhz */
+ core_write(priv, CORE_GSWPLL_GRP2,
+ RG_GSWPLL_POSDIV_500M(1) |
+ RG_GSWPLL_FBKDIV_500M(25));
+
+ /* Enable PLL */
+ core_write(priv, CORE_GSWPLL_GRP1,
+ RG_GSWPLL_EN_PRE |
+ RG_GSWPLL_POSDIV_200M(2) |
+ RG_GSWPLL_FBKDIV_200M(32));
+}
+
/* Setup TX circuit including relevant PAD and driving */
static int
mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
@@ -453,21 +471,6 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
core_clear(priv, CORE_TRGMII_GSW_CLK_CG,
REG_GSWCK_EN | REG_TRGMIICK_EN);
- /* Setup core clock for MT7530 */
- /* Disable PLL */
- core_write(priv, CORE_GSWPLL_GRP1, 0);
-
- /* Set core clock into 500Mhz */
- core_write(priv, CORE_GSWPLL_GRP2,
- RG_GSWPLL_POSDIV_500M(1) |
- RG_GSWPLL_FBKDIV_500M(25));
-
- /* Enable PLL */
- core_write(priv, CORE_GSWPLL_GRP1,
- RG_GSWPLL_EN_PRE |
- RG_GSWPLL_POSDIV_200M(2) |
- RG_GSWPLL_FBKDIV_200M(32));
-
/* 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));
@@ -2196,6 +2199,8 @@ mt7530_setup(struct dsa_switch *ds)
SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
SYS_CTRL_REG_RST);
+ mt7530_pll_setup(priv);
+
/* Enable Port 6 only; P5 as GMAC5 which currently is not supported */
val = mt7530_read(priv, MT7530_MHWTRAP);
val &= ~MHWTRAP_P6_DIS & ~MHWTRAP_PHY_ACCESS;
This looks much better, thanks a lot! The only missing part is setting
the PLL frequency when only port 5 is enabled.
I'll test it regardless.
[0] https://en.wikipedia.org/wiki/Media-independent_interface#GMII
Arınç