Messages in this thread Patch in this message | | | Date | Mon, 6 Mar 2023 17:45:52 +0200 | From | Vladimir Oltean <> | Subject | Re: [RFC PATCH net] net: dsa: mt7530: move PLL setup out of port 6 pad configuration |
| |
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
| |