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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20230604071122.kb3xenjbptm6jebh@skbuf>
Date: Sun, 4 Jun 2023 10:11:22 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: Arınç ÜNAL <arinc.unal@...nc9.com>
Cc: Sean Wang <sean.wang@...iatek.com>,
	Landen Chao <Landen.Chao@...iatek.com>,
	DENG Qingfang <dqfext@...il.com>,
	Daniel Golle <daniel@...rotopia.org>, Andrew Lunn <andrew@...n.ch>,
	Florian Fainelli <f.fainelli@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Matthias Brugger <matthias.bgg@...il.com>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	Russell King <linux@...linux.org.uk>,
	Richard van Schagen <richard@...terhints.com>,
	Richard van Schagen <vschagen@...com>,
	Frank Wunderlich <frank-w@...lic-files.de>,
	Bartel Eerdekens <bartel.eerdekens@...stell8.be>,
	erkin.bozoglu@...ont.com, mithat.guner@...ont.com,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH net-next 05/30] net: dsa: mt7530: read XTAL value from
 correct register

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;