Messages in this thread | | | Date | Mon, 6 Mar 2023 20:03:54 +0300 | Subject | Re: [RFC PATCH net] net: dsa: mt7530: move PLL setup out of port 6 pad configuration | From | Arınç ÜNAL <> |
| |
On 6.03.2023 18:45, Vladimir Oltean wrote: > 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?
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 <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 ???
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ç
| |