Date:   Mon, 6 Mar 2023 17:45:52 +0200
From:   Vladimir Oltean <olteanv@gmail.com>
To:     arinc9.unal@gmail.com
Cc:     Sean Wang <sean.wang@mediatek.com>,
        Landen Chao <Landen.Chao@mediatek.com>,
        DENG Qingfang <dqfext@gmail.com>, Andrew Lunn <andrew@lunn.ch>,
        Florian Fainelli <f.fainelli@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>,
        Russell King <linux@armlinux.org.uk>,
        =?utf-8?B?QXLEsW7DpyDDnE5BTA==?= <arinc.unal@arinc9.com>,
        Russell King <rmk+kernel@armlinux.org.uk>,
        =?utf-8?B?UmVuw6k=?= van Dorst <opensource@vdorst.com>,
        Alexander Couzens <lynxis@fe80.eu>,
        Ilya Lipnitskiy <ilya.lipnitskiy@gmail.com>,
        Richard van Schagen <richard@routerhints.com>,
        Frank Wunderlich <frank-w@public-files.de>,
        erkin.bozoglu@xeront.com, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org
Subject: Re: [RFC PATCH net] net: dsa: mt7530: move PLL setup out of port 6
 pad configuration
Message-ID: <20230306154552.26o6sbwf3rfekcyz@skbuf>
References: <20230304125453.53476-1-arinc.unal@arinc9.com>
 <20230304125453.53476-1-arinc.unal@arinc9.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20230304125453.53476-1-arinc.unal@arinc9.com>
 <20230304125453.53476-1-arinc.unal@arinc9.com>
Precedence: bulk

Hi Arınç,

On Sat, Mar 04, 2023 at 03:54:54PM +0300, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> 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@arinc9.com>
> ---
> 
> 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