From: Arınç ÜNAL <[email protected]>
Move the PLL setup of the MT7530 switch out of the pad configuration of
port 6 to mt7530_setup, after reset.
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 <[email protected]>
---
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ç
---
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) {
+ case PHY_INTERFACE_MODE_GMII:
+ /* PLL frequency: 125MHz */
+ ncpo1 = 0x0c80;
+ break;
+ case PHY_INTERFACE_MODE_MII:
+ break;
+ 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;
+ }
+
+ /* FIXME: run this switch if p6 is defined on the devicetree */
+ /* and change interface to the phy-mode of port 6 */
switch (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);
+
+ /* 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,
.mac_port_get_caps = mt7530_mac_port_get_caps,
.mac_port_config = mt7530_mac_config,
},
--
2.37.2
On Sat, Mar 04, 2023 at 03:54:54PM +0300, [email protected] wrote:
> From: Arınç ÜNAL <[email protected]>
>
> Move the PLL setup of the MT7530 switch out of the pad configuration of
> port 6 to mt7530_setup, after reset.
>
> 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.
If this is what is necessary, you're taking some of the configuration
out of phylink's control, effectively making port 6 a fixed-interface
mode port. In that case, there should only ever be one interface
mode set in port 6's supported_interface mask, so that phylink knows
that no other interface modes can be selected for that port.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On 4.03.2023 16:04, Russell King (Oracle) wrote:
> On Sat, Mar 04, 2023 at 03:54:54PM +0300, [email protected] wrote:
>> From: Arınç ÜNAL <[email protected]>
>>
>> Move the PLL setup of the MT7530 switch out of the pad configuration of
>> port 6 to mt7530_setup, after reset.
>>
>> 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.
>
> If this is what is necessary, you're taking some of the configuration
> out of phylink's control, effectively making port 6 a fixed-interface
> mode port. In that case, there should only ever be one interface
> mode set in port 6's supported_interface mask, so that phylink knows
> that no other interface modes can be selected for that port.
Thanks for the heads up Russell.
Arınç
On 4.03.2023 15:54, [email protected] wrote:
> From: Arınç ÜNAL <[email protected]>
>
> Move the PLL setup of the MT7530 switch out of the pad configuration of
> port 6 to mt7530_setup, after reset.
>
> 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 <[email protected]>
> ---
>
> 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ç
>
> ---
> 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) {
> + case PHY_INTERFACE_MODE_GMII:
> + /* PLL frequency: 125MHz */
> + ncpo1 = 0x0c80;
> + break;
> + case PHY_INTERFACE_MODE_MII:
> + break;
> + 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;
> + }
> +
> + /* FIXME: run this switch if p6 is defined on the devicetree */
> + /* and change interface to the phy-mode of port 6 */
> switch (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);
Unlike mt7531_pll_setup, there are return codes on mt7530_pad_clk_setup
so we may need to check for the return code here.
Arınç
Hi Arınç,
On Sat, Mar 04, 2023 at 03:54:54PM +0300, [email protected] wrote:
> From: Arınç ÜNAL <[email protected]>
>
> 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 <[email protected]>
> ---
>
> 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
On 6.03.2023 18:45, Vladimir Oltean wrote:
> Hi Arınç,
>
> On Sat, Mar 04, 2023 at 03:54:54PM +0300, [email protected] wrote:
>> From: Arınç ÜNAL <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>>
>> 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ç
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.
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.
(...)
> 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.
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ç
On Tue, Mar 07, 2023 at 02:26:08PM +0300, Arınç ÜNAL wrote:
> 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.
That's good. Empirically it seems to prove that ncpo1 only affects p6,
which was my initial assumption.
> - 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.
Got it, sounds expected, then? My patch can be submitted as-is, correct?
> 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");
In the C language, you need to put brackets { } around multi-statement
"if" blocks. Otherwise, despite the indentation, "dev_info" will be
executed unconditionally (unlike in Python for example). There should
also be a warning with newer gcc compilers about the misleading
indentation not leading to the expected code.
Like this:
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.
That seems to have worked, but shouldn't have been needed. I've uninstalled
Thunderbird in favor of mutt + vim for email editing.. although, isn't
there a Word Wrap option which you can just turn off?
On 7.03.2023 14:37, Vladimir Oltean wrote:
> On Tue, Mar 07, 2023 at 02:26:08PM +0300, Arınç ÜNAL wrote:
>> 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.
>
> That's good. Empirically it seems to prove that ncpo1 only affects p6,
> which was my initial assumption.
>
>> - 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.
>
> Got it, sounds expected, then? My patch can be submitted as-is, correct?
Yup, your patch is a go! I can submit other patches to address the
incorrect comment regarding port 5, and remove ncpo1 from rgmii mode for
port 6.
>
>> 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");
>
> In the C language, you need to put brackets { } around multi-statement
> "if" blocks. Otherwise, despite the indentation, "dev_info" will be
> executed unconditionally (unlike in Python for example). There should
> also be a warning with newer gcc compilers about the misleading
> indentation not leading to the expected code.
>
> Like this:
>
> if (xtal == HWTRAP_XTAL_40MHZ) {
> ncpo1 = 0x0780;
> dev_info(priv->dev, "XTAL is 40MHz, ncpo1 is 0x0780\n");
> }
>
Oh that figures, thanks a bunch. Clearly I've got a lot to learn. Now I
see only 40MHz.
[ 0.763772] mt7530 mdio-bus:1f: XTAL is 40MHz, ncpo1 is 0x0780
>>> 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.
>
> That seems to have worked, but shouldn't have been needed. I've uninstalled
> Thunderbird in favor of mutt + vim for email editing.. although, isn't
> there a Word Wrap option which you can just turn off?
It turns out there is indeed:
https://support.mozilla.org/en-US/questions/1307935
Mutt seems to be too techy for my taste but one can't know until they
try it, thanks for the info.
Arınç