Re: [RFC PATCH net] net: dsa: mt7530: move PLL setup out of port 6 pad configuration
From: Arınç ÜNAL
Date: Tue Mar 07 2023 - 06:27:14 EST
On 6.03.2023 23:19, Vladimir Oltean wrote:
On Mon, Mar 06, 2023 at 08:03:54PM +0300, Arınç ÜNAL wrote:
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
[0] https://en.wikipedia.org/wiki/Media-independent_interface#GMII
Wikipedia will only tell you what the frequency of the interface signals
needs to be. That is useful to keep in mind, but without information from
the datasheet regarding what the SoC's clock distribution tree looks like,
it's hard to know how that interface clock is derived from internal PLLs
and ultimately from the oscillators. I was hoping that was the kind of
information you could provide. The manuals I have access to, through charity,
don't say anything on that front.
I wish I could. There are three people associated with MediaTek CC'd
here. Maybe one will care to inform us.
Since I don't know what I'm commenting on, I'll stop commenting any further.
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.
(...)
Sorry for the vague reply, my assumption was that interfaces with slower
speeds, 100M, 1000M, etc. works fine at higher PLL frequency whilst they
won't work the other way around.
This looks much better, thanks a lot! The only missing part is setting the
PLL frequency when only port 5 is enabled.
True. Although with the limited information I have, I'm not sure that
the ncpo1 value written into CORE_PLL_GROUP5 is needed by port5 either
way. The fact that you claim port5 works when ncpo1 ranges from 125 to
250 MHz tells me that it's either very tolerant of the ncpo1 value
(through mechanisms unknown to me), or simply unaffected by it (more
likely ATM). Since I don't have any details regarding the value, I'd
just like to treat the configuration procedure as plain code, and not
make any changes until there's a proof that they're needed.
I'll test it regardless.
Thanks.
Port 5 as CPU port works fine with this patch. I completely removed from
port 6 phy modes.
With your patch on MT7621 (remember port 5 always worked on MT7623):
- Port 5 at rgmii as the only CPU port works, even though the PLL
frequency won't be set. The download/upload speed is not affected.
- port 6 at trgmii mode won't work if the PLL frequency is not set. The
SoC's MAC (gmac0) won't receive anything. It checks out since setting
the PLL frequency is put under the "Setup the MT7530 TRGMII Tx Clock"
comment. So port 6 cannot properly transmit frames to the SoC's MAC.
- Port 6 at rgmii mode works without setting the PLL frequency. Speed is
not affected.
I commented out core_write(priv, CORE_PLL_GROUP5,
RG_LCDDS_PCW_NCPO1(ncpo1)); to stop setting the PLL frequency.
In conclusion, setting the PLL frequency is only needed for the trgmii
mode, so I believe we can get rid of it on other cases.
One more thing, on MT7621, xtal matches to both HWTRAP_XTAL_40MHZ and
HWTRAP_XTAL_25MHZ so the final value of ncpo1 is 0x0a00. I'm not sure if
xtal matching both of them is the expected behaviour.
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index fbf27d4ab5d9..12cea89ae0ac 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -439,8 +439,12 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
/* PLL frequency: 150MHz: 1.2GBit */
if (xtal == HWTRAP_XTAL_40MHZ)
ncpo1 = 0x0780;
+ dev_info(priv->dev, "XTAL is 40MHz, ncpo1 is 0x0780\n");
if (xtal == HWTRAP_XTAL_25MHZ)
ncpo1 = 0x0a00;
+ dev_info(priv->dev, "XTAL is 25MHz, ncpo1 is 0x0a00\n");
+ if (xtal == HWTRAP_XTAL_20MHZ)
+ dev_info(priv->dev, "XTAL is 20MHz too\n");
} else { /* PLL frequency: 250MHz: 2.0Gbit */
if (xtal == HWTRAP_XTAL_40MHZ)
ncpo1 = 0x0c80;
[ 0.710455] mt7530 mdio-bus:1f: MT7530 adapts as multi-chip module
[ 0.734419] mt7530 mdio-bus:1f: configuring for fixed/rgmii link mode
[ 0.741766] mt7530 mdio-bus:1f: Link is Up - 1Gbps/Full - flow control rx/tx
[ 0.743647] mt7530 mdio-bus:1f: configuring for fixed/trgmii link mode
[ 0.755422] mt7530 mdio-bus:1f: XTAL is 40MHz, ncpo1 is 0x0780
[ 0.761250] mt7530 mdio-bus:1f: XTAL is 25MHz, ncpo1 is 0x0a00
[ 0.769414] mt7530 mdio-bus:1f: Link is Up - 1Gbps/Full - flow control rx/tx
[ 0.772067] mt7530 mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7530 PHY] (irq=17)
[ 0.788647] mt7530 mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7530 PHY] (irq=18)
[ 0.800354] mt7530 mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7530 PHY] (irq=19)
[ 0.812031] mt7530 mdio-bus:1f lan4 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7530 PHY] (irq=20)
[ 0.823418] mtk_soc_eth 1e100000.ethernet eth1: entered promiscuous mode
[ 0.830250] mtk_soc_eth 1e100000.ethernet eth0: entered promiscuous mode
[ 0.837007] DSA: tree 0 setup
Thunderbird limits lines to about 72 columns, so I'm pasting as
quotation which seems to bypass that.
Arınç