[RFC PATCH net] net: dsa: mt7530: move PLL setup out of port 6 pad configuration

[Arınç ÜNAL]

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ç