[PATCH net-next 05/30] net: dsa: mt7530: read XTAL value from correct register

[Vladimir Oltean]

On Sun, Jun 04, 2023 at 09:34:38AM +0300, Arınç ÜNAL wrote:
> > I disagree as a matter of principle with the reasoning. The fact that
> > XTAL constants are defined under HWTRAP is not a reason to change the
> > code to read the XTAL values from the HWTRAP register. The fact that
> > XTAL_FSEL is read-only in MHWTRAP is indeed a reason why you *could*
> > read it from HWTRAP, but also not one why you *should* make a change.
> 
> Makes sense. I have refactored the hardware trap constants definitions
> by looking at the documents for MT7530 and MT7531. The registers are the
> same on both switches so I combine the bits under MT753X_(M)HWTRAP.
> 
> I put the r/w bits on MHWTRAP to MWHTRAP, the read-only bits on HWTRAP
> and MHWTRAP to HWTRAP. Mind that the MT7531_CHG_STRAP bit exists only on
> the MHWTRAP register.
> 
> To follow this, I read XTAL for MT7530 from HWTRAP instead of MHWTRAP
> since the XTAL bits are read-only. Would this change make sense as a
> matter of refactoring?

Possibly. The maintainers of mt7530 have the definitive word on that.
Behavior changes (reading XTAL from HWTRAP instead of MHWTRAP) should
still have their separate change which isn't noisy, separate from the
renaming of constants.

> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 5b77799f41cc..444fa97db7c0 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -420,9 +420,9 @@ mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
>  	struct mt7530_priv *priv = ds->priv;
>  	u32 ncpo1, ssc_delta, i, xtal;
> -	mt7530_clear(priv, MT7530_MHWTRAP, MHWTRAP_P6_DIS);
> +	mt7530_clear(priv, MT753X_MHWTRAP, MT7530_P6_DIS);
> -	xtal = mt7530_read(priv, MT7530_MHWTRAP) & HWTRAP_XTAL_MASK;
> +	xtal = mt7530_read(priv, MT753X_HWTRAP) & MT7530_XTAL_MASK;
>  	if (interface == PHY_INTERFACE_MODE_RGMII) {
>  		mt7530_rmw(priv, MT7530_P6ECR, P6_INTF_MODE_MASK,
> @@ -431,21 +431,21 @@ mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
>  		mt7530_rmw(priv, MT7530_P6ECR, P6_INTF_MODE_MASK,
>  			   P6_INTF_MODE(1));
> -		if (xtal == HWTRAP_XTAL_25MHZ)
> +		if (xtal == MT7530_XTAL_25MHZ)
>  			ssc_delta = 0x57;
>  		else
>  			ssc_delta = 0x87;
>  		if (priv->id == ID_MT7621) {
>  			/* PLL frequency: 125MHz: 1.0GBit */
> -			if (xtal == HWTRAP_XTAL_40MHZ)
> +			if (xtal == MT7530_XTAL_40MHZ)
>  				ncpo1 = 0x0640;
> -			if (xtal == HWTRAP_XTAL_25MHZ)
> +			if (xtal == MT7530_XTAL_25MHZ)
>  				ncpo1 = 0x0a00;
>  		} else { /* PLL frequency: 250MHz: 2.0Gbit */
> -			if (xtal == HWTRAP_XTAL_40MHZ)
> +			if (xtal == MT7530_XTAL_40MHZ)
>  				ncpo1 = 0x0c80;
> -			if (xtal == HWTRAP_XTAL_25MHZ)
> +			if (xtal == MT7530_XTAL_25MHZ)
>  				ncpo1 = 0x1400;
>  		}
> @@ -487,12 +487,12 @@ mt7531_pll_setup(struct mt7530_priv *priv)
>  	val = mt7530_read(priv, MT7531_CREV);
>  	top_sig = mt7530_read(priv, MT7531_TOP_SIG_SR);
> -	hwstrap = mt7530_read(priv, MT7531_HWTRAP);
> +	hwstrap = mt7530_read(priv, MT753X_HWTRAP);
>  	if ((val & CHIP_REV_M) > 0)
> -		xtal = (top_sig & PAD_MCM_SMI_EN) ? HWTRAP_XTAL_FSEL_40MHZ :
> -						    HWTRAP_XTAL_FSEL_25MHZ;
> +		xtal = (top_sig & PAD_MCM_SMI_EN) ? MT7531_XTAL_FSEL_40MHZ :
> +						    MT7531_XTAL_FSEL_25MHZ;
>  	else
> -		xtal = hwstrap & HWTRAP_XTAL_FSEL_MASK;
> +		xtal = hwstrap & MT7531_XTAL25;

xtal = hwstrap & BIT(7). The "xtal" variable will either hold the value
of 0 or BIT(7), do you agree?

>  	/* Step 1 : Disable MT7531 COREPLL */
>  	val = mt7530_read(priv, MT7531_PLLGP_EN);
> @@ -521,13 +521,13 @@ mt7531_pll_setup(struct mt7530_priv *priv)
>  	usleep_range(25, 35);
>  	switch (xtal) {
> -	case HWTRAP_XTAL_FSEL_25MHZ:
> +	case MT7531_XTAL_FSEL_25MHZ:

reworded:
	case 1:

when will "xtal" be equal to 1?

>  		val = mt7530_read(priv, MT7531_PLLGP_CR0);
>  		val &= ~RG_COREPLL_SDM_PCW_M;
>  		val |= 0x140000 << RG_COREPLL_SDM_PCW_S;
>  		mt7530_write(priv, MT7531_PLLGP_CR0, val);
>  		break;
> -	case HWTRAP_XTAL_FSEL_40MHZ:
> +	case MT7531_XTAL_FSEL_40MHZ:
>  		val = mt7530_read(priv, MT7531_PLLGP_CR0);
>  		val &= ~RG_COREPLL_SDM_PCW_M;
>  		val |= 0x190000 << RG_COREPLL_SDM_PCW_S;