Messages in this thread Patch in this message | ![/](/images/icornerl.gif) | | Date | Wed, 8 Mar 2023 11:50:09 +0300 | From | Arınç ÜNAL <> | Subject | Re: [PATCH net 2/2] net: dsa: mt7530: set PLL frequency only when trgmii is used |
| |
On 8.03.2023 02:33, Vladimir Oltean wrote: > On Wed, Mar 08, 2023 at 01:03:28AM +0300, arinc9.unal@gmail.com wrote: >> From: Arınç ÜNAL <arinc.unal@arinc9.com> >> >> As my testing on the MCM MT7530 switch on MT7621 SoC shows, setting the PLL >> frequency does not affect MII modes other than trgmii on port 5 and port 6. >> So the assumption is that the operation here called "setting the PLL >> frequency" actually sets the frequency of the TRGMII TX clock. >> >> Make it so that it is set only when the trgmii mode is used. >> >> Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch") >> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com> >> --- >> drivers/net/dsa/mt7530.c | 2 -- >> 1 file changed, 2 deletions(-) >> >> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c >> index b1a79460df0e..961306c1ac14 100644 >> --- a/drivers/net/dsa/mt7530.c >> +++ b/drivers/net/dsa/mt7530.c >> @@ -430,8 +430,6 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface) >> switch (interface) { >> case PHY_INTERFACE_MODE_RGMII: >> trgint = 0; >> - /* PLL frequency: 125MHz */ >> - ncpo1 = 0x0c80; >> break; >> case PHY_INTERFACE_MODE_TRGMII: >> trgint = 1; >> -- >> 2.37.2 >> > > NACK. > > By deleting the assignment to the ncpo1 variable, it becomes > uninitialized when port 6's interface mode is PHY_INTERFACE_MODE_RGMII. > In the C language, uninitialized variables take the value of whatever > memory happens to be on the stack at the address they are placed, > interpreted as an appropriate data type for that variable - here u32. > > Writing the value to CORE_PLL_GROUP5 happens when the function below is > called, not when the "ncpo1" variable is assigned. > > core_write(priv, CORE_PLL_GROUP5, RG_LCDDS_PCW_NCPO1(ncpo1)); > > It is not a good idea to write uninitialized kernel stack memory to > hardware registers, unless perhaps you want to use it as some sort of > poor quality entropy source for a random number generator...
Thanks a lot for this. Now that you moved setting the core clock to somewhere else, I think we can run the TRGMII setup only when trgmii mode is used, exactly what I already explained on the patch log, with the diff below. This should make it so that writing the value to CORE_PLL_GROUP5 happens in the case where ncpo1 is always set.
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c index b1a79460df0e..c2d81b7a429d 100644 --- a/drivers/net/dsa/mt7530.c +++ b/drivers/net/dsa/mt7530.c @@ -430,8 +430,6 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface) switch (interface) { case PHY_INTERFACE_MODE_RGMII: trgint = 0; - /* PLL frequency: 125MHz */ - ncpo1 = 0x0c80; break; case PHY_INTERFACE_MODE_TRGMII: trgint = 1; @@ -462,38 +460,40 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface) mt7530_rmw(priv, MT7530_P6ECR, P6_INTF_MODE_MASK, P6_INTF_MODE(trgint)); - /* Lower Tx Driving for TRGMII path */ - for (i = 0 ; i < NUM_TRGMII_CTRL ; i++) - mt7530_write(priv, MT7530_TRGMII_TD_ODT(i), - TD_DM_DRVP(8) | TD_DM_DRVN(8)); - - /* Disable MT7530 core and TRGMII Tx clocks */ - core_clear(priv, CORE_TRGMII_GSW_CLK_CG, - REG_GSWCK_EN | REG_TRGMIICK_EN); - - /* 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)); - core_write(priv, CORE_PLL_GROUP10, RG_LCDDS_SSC_DELTA(ssc_delta)); - core_write(priv, CORE_PLL_GROUP11, RG_LCDDS_SSC_DELTA1(ssc_delta)); - core_write(priv, CORE_PLL_GROUP4, - RG_SYSPLL_DDSFBK_EN | RG_SYSPLL_BIAS_EN | - RG_SYSPLL_BIAS_LPF_EN); - core_write(priv, CORE_PLL_GROUP2, - RG_SYSPLL_EN_NORMAL | RG_SYSPLL_VODEN | - RG_SYSPLL_POSDIV(1)); - core_write(priv, CORE_PLL_GROUP7, - RG_LCDDS_PCW_NCPO_CHG | RG_LCCDS_C(3) | - RG_LCDDS_PWDB | RG_LCDDS_ISO_EN); - - /* Enable MT7530 core and TRGMII Tx clocks */ - core_set(priv, CORE_TRGMII_GSW_CLK_CG, - REG_GSWCK_EN | REG_TRGMIICK_EN); - - if (!trgint) + if (trgint) { + /* Lower Tx Driving for TRGMII path */ + for (i = 0 ; i < NUM_TRGMII_CTRL ; i++) + mt7530_write(priv, MT7530_TRGMII_TD_ODT(i), + TD_DM_DRVP(8) | TD_DM_DRVN(8)); + + /* Disable MT7530 core and TRGMII Tx clocks */ + core_clear(priv, CORE_TRGMII_GSW_CLK_CG, + REG_GSWCK_EN | REG_TRGMIICK_EN); + + /* 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)); + core_write(priv, CORE_PLL_GROUP10, RG_LCDDS_SSC_DELTA(ssc_delta)); + core_write(priv, CORE_PLL_GROUP11, RG_LCDDS_SSC_DELTA1(ssc_delta)); + core_write(priv, CORE_PLL_GROUP4, + RG_SYSPLL_DDSFBK_EN | RG_SYSPLL_BIAS_EN | + RG_SYSPLL_BIAS_LPF_EN); + core_write(priv, CORE_PLL_GROUP2, + RG_SYSPLL_EN_NORMAL | RG_SYSPLL_VODEN | + RG_SYSPLL_POSDIV(1)); + core_write(priv, CORE_PLL_GROUP7, + RG_LCDDS_PCW_NCPO_CHG | RG_LCCDS_C(3) | + RG_LCDDS_PWDB | RG_LCDDS_ISO_EN); + + /* Enable MT7530 core and TRGMII Tx clocks */ + core_set(priv, CORE_TRGMII_GSW_CLK_CG, + REG_GSWCK_EN | REG_TRGMIICK_EN); + } else { for (i = 0 ; i < NUM_TRGMII_CTRL; i++) mt7530_rmw(priv, MT7530_TRGMII_RD(i), RD_TAP_MASK, RD_TAP(16)); + } + return 0; } I'll do some tests to make sure everything works fine.
Arınç
| ![\](/images/icornerr.gif) |