[PATCH net 1/2] net: dsa: mt7530: remove now incorrect comment regarding port 5

[PATCH net 1/2] net: dsa: mt7530: remove now incorrect comment regarding port 5

[arinc9.unal]

From: Arınç ÜNAL <arinc.unal@arinc9.com>

Remove now incorrect comment regarding port 5 as GMAC5. This is supposed to
be supported since commit 38f790a80560 ("net: dsa: mt7530: Add support for
port 5") under mt7530_setup_port5().

Fixes: 38f790a80560 ("net: dsa: mt7530: Add support for port 5")
Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
 drivers/net/dsa/mt7530.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index a508402c4ecb..b1a79460df0e 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2201,7 +2201,7 @@ mt7530_setup(struct dsa_switch *ds)
 
 	mt7530_pll_setup(priv);
 
-	/* Enable Port 6 only; P5 as GMAC5 which currently is not supported */
+	/* Enable port 6 */
 	val = mt7530_read(priv, MT7530_MHWTRAP);
 	val &= ~MHWTRAP_P6_DIS & ~MHWTRAP_PHY_ACCESS;
 	val |= MHWTRAP_MANUAL;
-- 
2.37.2

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

[arinc9.unal]

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

Re: [PATCH net 1/2] net: dsa: mt7530: remove now incorrect comment regarding port 5

[Vladimir Oltean]

On Wed, Mar 08, 2023 at 01:03:27AM +0300, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> Remove now incorrect comment regarding port 5 as GMAC5. This is supposed to
> be supported since commit 38f790a80560 ("net: dsa: mt7530: Add support for
> port 5") under mt7530_setup_port5().
> 
> Fixes: 38f790a80560 ("net: dsa: mt7530: Add support for port 5")
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---
>  drivers/net/dsa/mt7530.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index a508402c4ecb..b1a79460df0e 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2201,7 +2201,7 @@ mt7530_setup(struct dsa_switch *ds)
>  
>  	mt7530_pll_setup(priv);
>  
> -	/* Enable Port 6 only; P5 as GMAC5 which currently is not supported */
> +	/* Enable port 6 */
>  	val = mt7530_read(priv, MT7530_MHWTRAP);
>  	val &= ~MHWTRAP_P6_DIS & ~MHWTRAP_PHY_ACCESS;
>  	val |= MHWTRAP_MANUAL;
> -- 
> 2.37.2
> 

It's best not to abuse the net.git tree with non-bugfix patches.

Re: [PATCH net 1/2] net: dsa: mt7530: remove now incorrect comment regarding port 5

[Arınç ÜNAL]

On 8.03.2023 01:06, Vladimir Oltean wrote:
> On Wed, Mar 08, 2023 at 01:03:27AM +0300, arinc9.unal@gmail.com wrote:
>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>
>> Remove now incorrect comment regarding port 5 as GMAC5. This is supposed to
>> be supported since commit 38f790a80560 ("net: dsa: mt7530: Add support for
>> port 5") under mt7530_setup_port5().
>>
>> Fixes: 38f790a80560 ("net: dsa: mt7530: Add support for port 5")
>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>> ---
>>   drivers/net/dsa/mt7530.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index a508402c4ecb..b1a79460df0e 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -2201,7 +2201,7 @@ mt7530_setup(struct dsa_switch *ds)
>>   
>>   	mt7530_pll_setup(priv);
>>   
>> -	/* Enable Port 6 only; P5 as GMAC5 which currently is not supported */
>> +	/* Enable port 6 */
>>   	val = mt7530_read(priv, MT7530_MHWTRAP);
>>   	val &= ~MHWTRAP_P6_DIS & ~MHWTRAP_PHY_ACCESS;
>>   	val |= MHWTRAP_MANUAL;
>> -- 
>> 2.37.2
>>
> 
> It's best not to abuse the net.git tree with non-bugfix patches.

Fixing incorrect comments sounds bug fixing to me. Let's hear Jakub and 
David's thoughts.

Arınç

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

[Vladimir Oltean]

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...

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

[Arınç ÜNAL]

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ç