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

From: Vladimir Oltean
Date: Mon Mar 06 2023 - 10:46:27 EST

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?

> 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 ???

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 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;
--
2.34.1