Mailing List Archive

[PATCH net-next 05/30] net: dsa: mt7530: read XTAL value from correct register
From: Ar?nç ÜNAL <arinc.unal@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@arinc9.com>
Signed-off-by: Ar?nç ÜNAL <arinc.unal@arinc9.com>
---
drivers/net/dsa/mt7530.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index b28d66a7c0b2..1a842d6fbc27 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -406,7 +406,7 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
struct mt7530_priv *priv = ds->priv;
u32 ncpo1, ssc_delta, trgint, xtal;

- xtal = mt7530_read(priv, MT7530_MHWTRAP) & HWTRAP_XTAL_MASK;
+ xtal = mt7530_read(priv, MT7530_HWTRAP) & HWTRAP_XTAL_MASK;

if (xtal == HWTRAP_XTAL_20MHZ) {
dev_err(priv->dev,
--
2.39.2
Re: [PATCH net-next 05/30] net: dsa: mt7530: read XTAL value from correct register [ In reply to ]
On Mon, May 22, 2023 at 03:15:07PM +0300, arinc9.unal@gmail.com wrote:
> From: Ar?nç ÜNAL <arinc.unal@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@arinc9.com>
> Signed-off-by: Ar?nç ÜNAL <arinc.unal@arinc9.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

Andrew
Re: [PATCH net-next 05/30] net: dsa: mt7530: read XTAL value from correct register [ In reply to ]
On Mon, May 22, 2023 at 03:15:07PM +0300, arinc9.unal@gmail.com wrote:
> From: Ar?nç ÜNAL <arinc.unal@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@arinc9.com>
> Signed-off-by: Ar?nç ÜNAL <arinc.unal@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.

While in this very particular case there should be no negative effect
caused by the change (*because* XTAL_FSEL is read-only), your approach
absolutely does not scale to the other situations that you will be faced
with.

If it was any other r/w field from HWTRAP vs MHWTRAP, you would have
needed to get used to that coding pattern (or do something about the
coding pattern itself), and not just decide to change the register to
what you think is correct - which is a *behavior* change and not just
a *coding style* change. You don't change the *behavior* when you don't
like the *coding style* !!! because that's not a punctual fix to your
problem.

I'm sorry that it is like this, but you can't expect the Linux kernel to
be written for the level of understanding of a beginner C programmer.
It's simply too hard for everyone to change, and much easier, and more
beneficial overall, for you to change.

I understand that you're poking sticks at everything in order to become
more familiar with the driver. That's perfectly normal and fine. But not
everything that comes as a result of that is of value for sharing back
to the mainline kernel's mailing lists.

Seriously, please first share these small rewrites with someone more
senior than you, and ask for a preliminary second opinion.

As they say, "on the Internet, nobody knows you're a dog". If reviewers
don't take into account that you're a newbie with C (which is a badge
that you don't carry everywhere - because you don't have to), it's easy
for patches like this (and most of this series) to come across as:
"I have no consideration for the fact that the existing code works, and
the way in which it works, I'm just gonna rewrite all of it according to
my own sensibilities and subjective justifications, and throw baseless
words at it such as: fix this, correct that, when in fact all I'm doing
is make silly changes with no effect to the observable behavior".

Because I know that the above isn't the case, I try to be as polite as
possible expressing my frustration that I am looking at a large volume
of worthless and misguided refactoring.
Re: [PATCH net-next 05/30] net: dsa: mt7530: read XTAL value from correct register [ In reply to ]
On 24.05.2023 19:57, Vladimir Oltean wrote:
> On Mon, May 22, 2023 at 03:15:07PM +0300, arinc9.unal@gmail.com wrote:
>> From: Ar?nç ÜNAL <arinc.unal@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@arinc9.com>
>> Signed-off-by: Ar?nç ÜNAL <arinc.unal@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. 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.

>
> While in this very particular case there should be no negative effect
> caused by the change (*because* XTAL_FSEL is read-only), your approach
> absolutely does not scale to the other situations that you will be faced
> with.
>
> If it was any other r/w field from HWTRAP vs MHWTRAP, you would have
> needed to get used to that coding pattern (or do something about the
> coding pattern itself), and not just decide to change the register to
> what you think is correct - which is a *behavior* change and not just
> a *coding style* change. You don't change the *behavior* when you don't
> like the *coding style* !!! because that's not a punctual fix to your
> problem.
>
> I'm sorry that it is like this, but you can't expect the Linux kernel to
> be written for the level of understanding of a beginner C programmer.
> It's simply too hard for everyone to change, and much easier, and more
> beneficial overall, for you to change.
>
> I understand that you're poking sticks at everything in order to become
> more familiar with the driver. That's perfectly normal and fine. But not
> everything that comes as a result of that is of value for sharing back
> to the mainline kernel's mailing lists.

Makes sense.

>
> Seriously, please first share these small rewrites with someone more
> senior than you, and ask for a preliminary second opinion.

Would submitting this as an RFC had been a similar action to your
describing here? Because I already did that:

https://lore.kernel.org/netdev/20230421143648.87889-6-arinc.unal@arinc9.com/

>
> As they say, "on the Internet, nobody knows you're a dog". If reviewers
> don't take into account that you're a newbie with C (which is a badge
> that you don't carry everywhere - because you don't have to), it's easy
> for patches like this (and most of this series) to come across as:
> "I have no consideration for the fact that the existing code works, and
> the way in which it works, I'm just gonna rewrite all of it according to
> my own sensibilities and subjective justifications, and throw baseless
> words at it such as: fix this, correct that, when in fact all I'm doing
> is make silly changes with no effect to the observable behavior".
>
> Because I know that the above isn't the case, I try to be as polite as
> possible expressing my frustration that I am looking at a large volume
> of worthless and misguided refactoring.

I should've given more effort to explain my reasons for this patch. I
disagree that the series is a large volume of worthless and misguided
refactoring and am happy to discuss it patch by patch.

Ar?nç
Re: [PATCH net-next 05/30] net: dsa: mt7530: read XTAL value from correct register [ In reply to ]
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@gmail.com wrote:
> > > From: Ar?nç ÜNAL <arinc.unal@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@arinc9.com>
> > > Signed-off-by: Ar?nç ÜNAL <arinc.unal@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 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.

> > Seriously, please first share these small rewrites with someone more
> > senior than you, and ask for a preliminary second opinion.
>
> Would submitting this as an RFC had been a similar action to your describing
> here? Because I already did that:
>
> https://lore.kernel.org/netdev/20230421143648.87889-6-arinc.unal@arinc9.com/

In practice, volume is also an issue. The higher the volume, the lower
the chances that people will be able to crop a chunk of time large enough
to review.

> I should've given more effort to explain my reasons for this patch. I
> disagree that the series is a large volume of worthless and misguided
> refactoring and am happy to discuss it patch by patch.

I agree that the follow-up patches, as far as I could reach into this
series, are not as gratuitous as this one.
Re: [PATCH net-next 05/30] net: dsa: mt7530: read XTAL value from correct register [ In reply to ]
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@gmail.com wrote:
>>>> From: Ar?nç ÜNAL <arinc.unal@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@arinc9.com>
>>>> Signed-off-by: Ar?nç ÜNAL <arinc.unal@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ç
Re: [PATCH net-next 05/30] net: dsa: mt7530: read XTAL value from correct register [ In reply to ]
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;
Re: [PATCH net-next 05/30] net: dsa: mt7530: read XTAL value from correct register [ In reply to ]
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ç