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

[Vladimir Oltean <olteanv@gmail.com>]

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