[PATCH net-next 05/30] net: dsa: mt7530: read XTAL value from correct register
Arınç ÜNAL
arinc.unal at arinc9.com
Sat Jun 3 23:34:38 PDT 2023
On 25.05.2023 16:31, Vladimir Oltean wrote:
> On Thu, May 25, 2023 at 09:20:08AM +0300, Arınç ÜNAL wrote:
>> On 24.05.2023 19:57, Vladimir Oltean wrote:
>>> On Mon, May 22, 2023 at 03:15:07PM +0300, arinc9.unal at gmail.com wrote:
>>>> From: Arınç ÜNAL <arinc.unal at arinc9.com>
>>>>
>>>> On commit 7ef6f6f8d237 ("net: dsa: mt7530: Add MT7621 TRGMII mode support")
>>>> macros for reading the crystal frequency were added under the MT7530_HWTRAP
>>>> register. However, the value given to the xtal variable on
>>>> mt7530_pad_clk_setup() is read from the MT7530_MHWTRAP register instead.
>>>>
>>>> Although the document MT7621 Giga Switch Programming Guide v0.3 states that
>>>> the value can be read from both registers, use the register where the
>>>> macros were defined under.
>>>>
>>>> Tested-by: Arınç ÜNAL <arinc.unal at arinc9.com>
>>>> Signed-off-by: Arınç ÜNAL <arinc.unal at arinc9.com>
>>>> ---
>>>
>>> I'm sorry, but I refuse this patch, mainly as a matter of principle -
>>> because that's just not how we do things, and you need to understand why.
>>>
>>> The commit title ("read XTAL value from correct register") claims that
>>> the process of reading a field which cannot be changed by software is
>>> any more correct when it is read from HWTRAP rather than MHWTRAP
>>> (modified HWTRAP).
>>>
>>> Your justification is that it's confusing to you if two registers have
>>> the same layout, and the driver has a single set of macros to decode the
>>> fields from both. You seem to think it's somehow not correct to decode
>>> fields from the MHWTRAP register using macros which have just HWTRAP in
>>> the name.
>>
>> No, it doesn't confuse me that two registers share the same layout. My
>> understanding was that the MHWTRAP register should be used for modifying the
>> hardware trap, and the HWTRAP register should be used for reading from the
>> hardware trap.
>
> My understanding is that reading from the read-only HWTRAP always gives
> you the power-on settings, while reading from the r/w MHWTRAP always
> gives you the current settings. If those settings coincide, as happens
> here, there's no practical difference.
I can confirm the bits on the HWTRAP register won't change after
changing the r/w bits on the MHWTRAP register.
val = mt7530_read(priv, MT753X_HWTRAP);
val &= ~MT7530_PHY_ACCESS;
val |= MT7530_MANUAL;
mt7530_write(priv, MT753X_MHWTRAP, val);
val = mt7530_read(priv, MT753X_MHWTRAP);
if (val & MT7530_PHY_ACCESS)
dev_info(dev, "mhwtrap: MT7530_PHY_ACCESS is set\n");
else
dev_info(dev, "mhwtrap: MT7530_PHY_ACCESS is unset\n");
val = mt7530_read(priv, MT753X_HWTRAP);
if (val & MT7530_PHY_ACCESS)
dev_info(dev, "hwtrap: MT7530_PHY_ACCESS is set\n");
else
dev_info(dev, "hwtrap: MT7530_PHY_ACCESS is unset\n");
[ 4.194897] mt7530-mdio mdio-bus:00: mhwtrap: MT7530_PHY_ACCESS is unset
[ 4.201770] mt7530-mdio mdio-bus:00: hwtrap: MT7530_PHY_ACCESS is set
>
>> I see that the XTAL constants were defined under the HWTRAP
>> register so I thought it would make sense to change the code to read the
>> XTAL values from the HWTRAP register instead. Let me know if you disagree
>> with this.
>
> 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?
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;
/* 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:
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;
@@ -902,32 +902,32 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
mutex_lock(&priv->reg_mutex);
- val = mt7530_read(priv, MT7530_MHWTRAP);
+ val = mt7530_read(priv, MT753X_MHWTRAP);
- val |= MHWTRAP_P5_MAC_SEL | MHWTRAP_P5_DIS;
- val &= ~MHWTRAP_P5_RGMII_MODE & ~MHWTRAP_PHY0_SEL;
+ val |= MT7530_P5_MAC_SEL | MT7530_P5_DIS;
+ val &= ~MT7530_P5_RGMII_MODE & ~MT7530_P5_PHY0_SEL;
switch (priv->p5_mode) {
case MUX_PHY_P0:
/* MUX_PHY_P0: P0 -> P5 -> SoC MAC */
- val |= MHWTRAP_PHY0_SEL;
+ val |= MT7530_P5_PHY0_SEL;
fallthrough;
case MUX_PHY_P4:
/* MUX_PHY_P4: P4 -> P5 -> SoC MAC */
- val &= ~MHWTRAP_P5_MAC_SEL & ~MHWTRAP_P5_DIS;
+ val &= ~MT7530_P5_MAC_SEL & ~MT7530_P5_DIS;
/* Setup the MAC by default for the cpu port */
mt7530_write(priv, MT7530_PMCR_P(5), 0x56300);
break;
default:
/* GMAC5: P5 -> SoC MAC or external PHY */
- val &= ~MHWTRAP_P5_DIS;
+ val &= ~MT7530_P5_DIS;
break;
}
/* Setup RGMII settings */
if (phy_interface_mode_is_rgmii(interface)) {
- val |= MHWTRAP_P5_RGMII_MODE;
+ val |= MT7530_P5_RGMII_MODE;
/* P5 RGMII RX Clock Control: delay setting for 1000M */
mt7530_write(priv, MT7530_P5RGMIIRXCR, CSR_RGMII_EDGE_ALIGN);
@@ -943,7 +943,7 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
CSR_RGMII_TXC_CFG(0x10 + tx_delay));
}
- mt7530_write(priv, MT7530_MHWTRAP, val);
+ mt7530_write(priv, MT753X_MHWTRAP, val);
dev_dbg(ds->dev, "Setup P5, HWTRAP=0x%x, mode=%s, phy-mode=%s\n", val,
mt7530_p5_mode_str(priv->p5_mode), phy_modes(interface));
@@ -2188,7 +2188,7 @@ mt7530_setup(struct dsa_switch *ds)
}
/* Waiting for MT7530 got to stable */
- INIT_MT7530_DUMMY_POLL(&p, priv, MT7530_HWTRAP);
+ INIT_MT7530_DUMMY_POLL(&p, priv, MT753X_HWTRAP);
ret = readx_poll_timeout(_mt7530_read, &p, val, val != 0,
20, 1000000);
if (ret < 0) {
@@ -2203,7 +2203,7 @@ mt7530_setup(struct dsa_switch *ds)
return -ENODEV;
}
- if (val == HWTRAP_XTAL_20MHZ) {
+ if (val == MT7530_XTAL_20MHZ) {
dev_err(priv->dev,
"%s: MT7530 with a 20MHz XTAL is not supported!\n",
__func__);
@@ -2215,7 +2215,7 @@ mt7530_setup(struct dsa_switch *ds)
SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
SYS_CTRL_REG_RST);
- if (val == HWTRAP_XTAL_40MHZ)
+ if (val == MT7530_XTAL_40MHZ)
mt7530_pll_setup(priv);
/* Lower P5 RGMII Tx driving, 8mA */
@@ -2230,10 +2230,10 @@ mt7530_setup(struct dsa_switch *ds)
/* Directly access the PHY registers via C_MDC/C_MDIO. The bit that
* enables modifying the hardware trap must be set for this.
*/
- val = mt7530_read(priv, MT7530_MHWTRAP);
- val &= ~MHWTRAP_PHY_ACCESS;
- val |= MHWTRAP_MANUAL;
- mt7530_write(priv, MT7530_MHWTRAP, val);
+ val = mt7530_read(priv, MT753X_MHWTRAP);
+ val &= ~MT7530_PHY_ACCESS;
+ val |= MT7530_CHG_TRAP;
+ mt7530_write(priv, MT753X_MHWTRAP, val);
/* Trap BPDUs to the CPU port */
mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
@@ -2411,7 +2411,7 @@ mt7531_setup(struct dsa_switch *ds)
}
/* Waiting for MT7530 got to stable */
- INIT_MT7530_DUMMY_POLL(&p, priv, MT7530_HWTRAP);
+ INIT_MT7530_DUMMY_POLL(&p, priv, MT753X_HWTRAP);
ret = readx_poll_timeout(_mt7530_read, &p, val, val != 0,
20, 1000000);
if (ret < 0) {
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 2664057b3cd2..d7451943f5d6 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -445,31 +445,29 @@ enum mt7531_clk_skew {
};
/* Register for hw trap status */
-#define MT7530_HWTRAP 0x7800
-#define HWTRAP_XTAL_MASK (BIT(10) | BIT(9))
-#define HWTRAP_XTAL_25MHZ (BIT(10) | BIT(9))
-#define HWTRAP_XTAL_40MHZ (BIT(10))
-#define HWTRAP_XTAL_20MHZ (BIT(9))
-
-#define MT7531_HWTRAP 0x7800
-#define HWTRAP_XTAL_FSEL_MASK BIT(7)
-#define HWTRAP_XTAL_FSEL_25MHZ BIT(7)
-#define HWTRAP_XTAL_FSEL_40MHZ 0
-/* Unique fields of (M)HWSTRAP for MT7531 */
-#define XTAL_FSEL_S 7
-#define XTAL_FSEL_M BIT(7)
-#define PHY_EN BIT(6)
-#define CHG_STRAP BIT(8)
+#define MT753X_HWTRAP 0x7800
+#define MT7530_XTAL_MASK (BIT(10) | BIT(9))
+#define MT7530_XTAL_25MHZ (BIT(10) | BIT(9))
+#define MT7530_XTAL_40MHZ (BIT(10))
+#define MT7530_XTAL_20MHZ (BIT(9))
/* Register for hw trap modification */
-#define MT7530_MHWTRAP 0x7804
-#define MHWTRAP_PHY0_SEL BIT(20)
-#define MHWTRAP_MANUAL BIT(16)
-#define MHWTRAP_P5_MAC_SEL BIT(13)
-#define MHWTRAP_P6_DIS BIT(8)
-#define MHWTRAP_P5_RGMII_MODE BIT(7)
-#define MHWTRAP_P5_DIS BIT(6)
-#define MHWTRAP_PHY_ACCESS BIT(5)
+#define MT753X_MHWTRAP 0x7804
+#define MT7530_P5_PHY0_SEL BIT(20)
+#define MT7530_CHG_TRAP BIT(16)
+#define MT7530_P5_MAC_SEL BIT(13)
+#define MT7530_P6_DIS BIT(8)
+#define MT7530_P5_RGMII_MODE BIT(7)
+#define MT7530_P5_DIS BIT(6)
+#define MT7530_PHY_ACCESS BIT(5)
+#define MT7531_CHG_STRAP BIT(8)
+#define MT7531_XTAL25 BIT(7)
+#define MT7531_PHY_EN BIT(6)
+
+enum mt7531_xtal_fsel {
+ MT7531_XTAL_FSEL_40MHZ,
+ MT7531_XTAL_FSEL_25MHZ,
+};
/* Register for TOP signal control */
#define MT7530_TOP_SIG_CTRL 0x7808
Arınç
More information about the linux-arm-kernel
mailing list