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]

Date: Sun, 4 Jun 2023 19:22:53 +0300
From: Arınç ÜNAL <arinc.unal@...nc9.com>
To: Vladimir Oltean <olteanv@...il.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 4.06.2023 10:11, Vladimir Oltean wrote:
> 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?

Yes I suppose the correct code should be:

xtal = (hwstrap & MT7531_XTAL25) ? MT7531_XTAL_FSEL_25MHZ :
				   MT7531_XTAL_FSEL_40MHZ;

> 
>>   	/* 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?

This should work correctly with my comment above.

Arınç