Re: [PATCH net 2/2] net: dsa: mt7530: set PLL frequency only when trgmii is used

["Arınç ÜNAL" <arinc.unal@arinc9.com>]

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ç