[PATCH net 0/3] fixes for mtk_eth_soc

[PATCH net 0/3] fixes for mtk_eth_soc

[Bjørn Mork]

Fix mtk_eth_soc sgmii configuration.

This has been tested on a MT7986 with a Maxlinear GPY211C phy
permanently attached to the second SoC mac.

Alexander Couzens (1):
  net: mediatek: sgmii: ensure the SGMII PHY is powered down on
    configuration

Bjørn Mork (2):
  net: mediatek: sgmii: autonegotiation is required
  net: mediatek: sgmii: fix duplex configuration

 drivers/net/ethernet/mediatek/mtk_eth_soc.h |  2 +-
 drivers/net/ethernet/mediatek/mtk_sgmii.c   | 35 ++++++++++++---------
 2 files changed, 22 insertions(+), 15 deletions(-)

-- 
2.30.2

[PATCH net 1/3] net: mediatek: sgmii: ensure the SGMII PHY is powered down on configuration

[Bjørn Mork]

From: Alexander Couzens <lynxis@fe80.eu>

The code expect the PHY to be in power down which is only true after reset.
Allow changes of the SGMII parameters more than once.

There are cases when the SGMII_PHYA_PWD register contains 0x9 which
prevents SGMII from working. The SGMII still shows link but no traffic
can flow. Writing 0x0 to the PHYA_PWD register fix the issue. 0x0 was
taken from a good working state of the SGMII interface.

Signed-off-by: Alexander Couzens <lynxis@fe80.eu>
[ bmork: rebased and squashed into one patch ]
Signed-off-by: Bjørn Mork <bjorn@mork.no>
---
 drivers/net/ethernet/mediatek/mtk_sgmii.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c
index 5c286f2c9418..481f2f1e39f5 100644
--- a/drivers/net/ethernet/mediatek/mtk_sgmii.c
+++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c
@@ -88,6 +88,10 @@ static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
 		bmcr = 0;
 	}
 
+	/* PHYA power down */
+	regmap_update_bits(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL,
+			   SGMII_PHYA_PWD, SGMII_PHYA_PWD);
+
 	/* Configure the underlying interface speed */
 	regmap_update_bits(mpcs->regmap, mpcs->ana_rgc3,
 			   RG_PHY_SPEED_3_125G, rgc3);
@@ -108,9 +112,17 @@ static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
 	regmap_update_bits(mpcs->regmap, SGMSYS_PCS_CONTROL_1,
 			   SGMII_AN_RESTART | SGMII_AN_ENABLE, bmcr);
 
-	/* Release PHYA power down state */
-	regmap_update_bits(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL,
-			   SGMII_PHYA_PWD, 0);
+	/* Release PHYA power down state
+	 * Only removing bit SGMII_PHYA_PWD isn't enough.
+	 * There are cases when the SGMII_PHYA_PWD register contains 0x9 which
+	 * prevents SGMII from working. The SGMII still shows link but no traffic
+	 * can flow. Writing 0x0 to the PHYA_PWD register fix the issue. 0x0 was
+	 * taken from a good working state of the SGMII interface.
+	 * Unknown how much the QPHY needs but it is racy without a sleep.
+	 * Tested on mt7622 & mt7986.
+	 */
+	usleep_range(50, 100);
+	regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, 0);
 
 	return changed;
 }
-- 
2.30.2

Re: [PATCH net 1/3] net: mediatek: sgmii: ensure the SGMII PHY is powered down on configuration

[Russell King (Oracle)]

On Thu, Jan 19, 2023 at 06:12:46PM +0100, Bjørn Mork wrote:
> From: Alexander Couzens <lynxis@fe80.eu>
> 
> The code expect the PHY to be in power down which is only true after reset.
> Allow changes of the SGMII parameters more than once.
> 
> There are cases when the SGMII_PHYA_PWD register contains 0x9 which
> prevents SGMII from working. The SGMII still shows link but no traffic
> can flow. Writing 0x0 to the PHYA_PWD register fix the issue. 0x0 was
> taken from a good working state of the SGMII interface.
> 
> Signed-off-by: Alexander Couzens <lynxis@fe80.eu>
> [ bmork: rebased and squashed into one patch ]
> Signed-off-by: Bjørn Mork <bjorn@mork.no>
> ---
>  drivers/net/ethernet/mediatek/mtk_sgmii.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c
> index 5c286f2c9418..481f2f1e39f5 100644
> --- a/drivers/net/ethernet/mediatek/mtk_sgmii.c
> +++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c
> @@ -88,6 +88,10 @@ static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
>  		bmcr = 0;
>  	}
>  
> +	/* PHYA power down */
> +	regmap_update_bits(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL,
> +			   SGMII_PHYA_PWD, SGMII_PHYA_PWD);
> +

Doing this unconditionally means that the link will drop - even when
we aren't doing any reconfiguration (except changing the advertisement).
That's why I made it conditional in the version of the patch I sent
(which failed due to the unknown bits 3 and 0.)

We should always avoid bouncing the link when there's no reason to.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net 1/3] net: mediatek: sgmii: ensure the SGMII PHY is powered down on configuration

[Bjørn Mork]

"Russell King (Oracle)" <linux@armlinux.org.uk> writes:

> Doing this unconditionally means that the link will drop - even when
> we aren't doing any reconfiguration (except changing the advertisement).
> That's why I made it conditional in the version of the patch I sent
> (which failed due to the unknown bits 3 and 0.)

Right.  Sorry for missing that crucial point.  Will fix.


Bjørn

[PATCH net 2/3] net: mediatek: sgmii: autonegotiation is required

[Bjørn Mork]

sgmii mode fails if autonegotiation is disabled.

Signed-off-by: Bjørn Mork <bjorn@mork.no>
---
 drivers/net/ethernet/mediatek/mtk_sgmii.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c
index 481f2f1e39f5..d1f2bcb21242 100644
--- a/drivers/net/ethernet/mediatek/mtk_sgmii.c
+++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c
@@ -62,14 +62,9 @@ static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
 	 * other words, 1000Mbps or 2500Mbps).
 	 */
 	if (interface == PHY_INTERFACE_MODE_SGMII) {
-		sgm_mode = SGMII_IF_MODE_SGMII;
-		if (phylink_autoneg_inband(mode)) {
-			sgm_mode |= SGMII_REMOTE_FAULT_DIS |
-				    SGMII_SPEED_DUPLEX_AN;
-			use_an = true;
-		} else {
-			use_an = false;
-		}
+		sgm_mode = SGMII_IF_MODE_SGMII | SGMII_REMOTE_FAULT_DIS |
+			   SGMII_SPEED_DUPLEX_AN;
+		use_an = true;
 	} else if (phylink_autoneg_inband(mode)) {
 		/* 1000base-X or 2500base-X autoneg */
 		sgm_mode = SGMII_REMOTE_FAULT_DIS;
-- 
2.30.2

Re: [PATCH net 2/3] net: mediatek: sgmii: autonegotiation is required

[Russell King (Oracle)]

On Thu, Jan 19, 2023 at 06:12:47PM +0100, Bjørn Mork wrote:
> sgmii mode fails if autonegotiation is disabled.
> 
> Signed-off-by: Bjørn Mork <bjorn@mork.no>
> ---
>  drivers/net/ethernet/mediatek/mtk_sgmii.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c
> index 481f2f1e39f5..d1f2bcb21242 100644
> --- a/drivers/net/ethernet/mediatek/mtk_sgmii.c
> +++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c
> @@ -62,14 +62,9 @@ static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
>  	 * other words, 1000Mbps or 2500Mbps).
>  	 */
>  	if (interface == PHY_INTERFACE_MODE_SGMII) {
> -		sgm_mode = SGMII_IF_MODE_SGMII;
> -		if (phylink_autoneg_inband(mode)) {
> -			sgm_mode |= SGMII_REMOTE_FAULT_DIS |
> -				    SGMII_SPEED_DUPLEX_AN;
> -			use_an = true;
> -		} else {
> -			use_an = false;
> -		}
> +		sgm_mode = SGMII_IF_MODE_SGMII | SGMII_REMOTE_FAULT_DIS |
> +			   SGMII_SPEED_DUPLEX_AN;
> +		use_an = true;

I wasn't actually suggesting in our discussion that this is something
which should be changed.

The reference implementation for the expected behaviour is
phylink_mii_c22_pcs_config(), and it only enables in-band if "mode"
says so. If we have a PHY which has in-band disabled (yes, they do
exist) then having SGMII in-band unconditionally enabled breaks them,
and yes, those PHYs appear on SFP modules.

The proper answer is to use 'managed = "in-band-status";' in your DT
to have in-band used with SGMII.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net 2/3] net: mediatek: sgmii: autonegotiation is required

[Bjørn Mork]

"Russell King (Oracle)" <linux@armlinux.org.uk> writes:
> On Thu, Jan 19, 2023 at 06:12:47PM +0100, Bjørn Mork wrote:
>> sgmii mode fails if autonegotiation is disabled.
>> 
>> Signed-off-by: Bjørn Mork <bjorn@mork.no>
>> ---
>>  drivers/net/ethernet/mediatek/mtk_sgmii.c | 11 +++--------
>>  1 file changed, 3 insertions(+), 8 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c
>> index 481f2f1e39f5..d1f2bcb21242 100644
>> --- a/drivers/net/ethernet/mediatek/mtk_sgmii.c
>> +++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c
>> @@ -62,14 +62,9 @@ static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
>>  	 * other words, 1000Mbps or 2500Mbps).
>>  	 */
>>  	if (interface == PHY_INTERFACE_MODE_SGMII) {
>> -		sgm_mode = SGMII_IF_MODE_SGMII;
>> -		if (phylink_autoneg_inband(mode)) {
>> -			sgm_mode |= SGMII_REMOTE_FAULT_DIS |
>> -				    SGMII_SPEED_DUPLEX_AN;
>> -			use_an = true;
>> -		} else {
>> -			use_an = false;
>> -		}
>> +		sgm_mode = SGMII_IF_MODE_SGMII | SGMII_REMOTE_FAULT_DIS |
>> +			   SGMII_SPEED_DUPLEX_AN;
>> +		use_an = true;
>
> I wasn't actually suggesting in our discussion that this is something
> which should be changed.
>
> The reference implementation for the expected behaviour is
> phylink_mii_c22_pcs_config(), and it only enables in-band if "mode"
> says so. If we have a PHY which has in-band disabled (yes, they do
> exist) then having SGMII in-band unconditionally enabled breaks them,
> and yes, those PHYs appear on SFP modules.
>
> The proper answer is to use 'managed = "in-band-status";' in your DT
> to have in-band used with SGMII.

Well, yeah, I'd love to.  But then I'm back to the drawing board without
a link.  That just doesn't work for me.

What I did here also reflects what I saw in the mt7530.c debug dumps,
and how I read that code:

static int mt7531_sgmii_setup_mode_an(struct mt7530_priv *priv, int port,
				      phy_interface_t interface)
{
	if (!mt753x_is_mac_port(port))
		return -EINVAL;

	mt7530_set(priv, MT7531_QPHY_PWR_STATE_CTRL(port),
		   MT7531_SGMII_PHYA_PWD);

	mt7530_rmw(priv, MT7531_PHYA_CTRL_SIGNAL3(port),
		   MT7531_RG_TPHY_SPEED_MASK, MT7531_RG_TPHY_SPEED_1_25G);

	mt7530_set(priv, MT7531_SGMII_MODE(port),
		   MT7531_SGMII_REMOTE_FAULT_DIS |
		   MT7531_SGMII_SPEED_DUPLEX_AN);

	mt7530_rmw(priv, MT7531_PCS_SPEED_ABILITY(port),
		   MT7531_SGMII_TX_CONFIG_MASK, 1);

	mt7530_set(priv, MT7531_PCS_CONTROL_1(port), MT7531_SGMII_AN_ENABLE);

	mt7530_set(priv, MT7531_PCS_CONTROL_1(port), MT7531_SGMII_AN_RESTART);

	mt7530_write(priv, MT7531_QPHY_PWR_STATE_CTRL(port), 0);

	return 0;
}

static int
mt7531_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
		  phy_interface_t interface)
{
	struct mt7530_priv *priv = ds->priv;
	struct phy_device *phydev;
	struct dsa_port *dp;

	if (!mt753x_is_mac_port(port)) {
		dev_err(priv->dev, "port %d is not a MAC port\n", port);
		return -EINVAL;
	}

	switch (interface) {
	case PHY_INTERFACE_MODE_RGMII:
	case PHY_INTERFACE_MODE_RGMII_ID:
	case PHY_INTERFACE_MODE_RGMII_RXID:
	case PHY_INTERFACE_MODE_RGMII_TXID:
		dp = dsa_to_port(ds, port);
		phydev = dp->slave->phydev;
		return mt7531_rgmii_setup(priv, port, interface, phydev);
	case PHY_INTERFACE_MODE_SGMII:
		return mt7531_sgmii_setup_mode_an(priv, port, interface);
	case PHY_INTERFACE_MODE_NA:
	case PHY_INTERFACE_MODE_1000BASEX:
	case PHY_INTERFACE_MODE_2500BASEX:
		return mt7531_sgmii_setup_mode_force(priv, port, interface);
	default:
		return -EINVAL;
	}

	return -EINVAL;
}


AFAICS, this calls mt7531_sgmii_setup_mode_an() unconditionally for
PHY_INTERFACE_MODE_SGMII. Resulting in AN_ENABLE|AN_RESTART being set in
PCS_CONTROL_1 and SGMII_REMOTE_FAULT_DIS|SGMII_SPEED_DUPLEX_AN being set
in SGMII_MODE.  Regardless of inband or not.



Bjørn

Re: [PATCH net 2/3] net: mediatek: sgmii: autonegotiation is required

[Russell King (Oracle)]

On Thu, Jan 19, 2023 at 08:33:17PM +0100, Bjørn Mork wrote:
> "Russell King (Oracle)" <linux@armlinux.org.uk> writes:
> > On Thu, Jan 19, 2023 at 06:12:47PM +0100, Bjørn Mork wrote:
> >> sgmii mode fails if autonegotiation is disabled.
> >> 
> >> Signed-off-by: Bjørn Mork <bjorn@mork.no>
> >> ---
> >>  drivers/net/ethernet/mediatek/mtk_sgmii.c | 11 +++--------
> >>  1 file changed, 3 insertions(+), 8 deletions(-)
> >> 
> >> diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c
> >> index 481f2f1e39f5..d1f2bcb21242 100644
> >> --- a/drivers/net/ethernet/mediatek/mtk_sgmii.c
> >> +++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c
> >> @@ -62,14 +62,9 @@ static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
> >>  	 * other words, 1000Mbps or 2500Mbps).
> >>  	 */
> >>  	if (interface == PHY_INTERFACE_MODE_SGMII) {
> >> -		sgm_mode = SGMII_IF_MODE_SGMII;
> >> -		if (phylink_autoneg_inband(mode)) {
> >> -			sgm_mode |= SGMII_REMOTE_FAULT_DIS |
> >> -				    SGMII_SPEED_DUPLEX_AN;
> >> -			use_an = true;
> >> -		} else {
> >> -			use_an = false;
> >> -		}
> >> +		sgm_mode = SGMII_IF_MODE_SGMII | SGMII_REMOTE_FAULT_DIS |
> >> +			   SGMII_SPEED_DUPLEX_AN;
> >> +		use_an = true;
> >
> > I wasn't actually suggesting in our discussion that this is something
> > which should be changed.
> >
> > The reference implementation for the expected behaviour is
> > phylink_mii_c22_pcs_config(), and it only enables in-band if "mode"
> > says so. If we have a PHY which has in-band disabled (yes, they do
> > exist) then having SGMII in-band unconditionally enabled breaks them,
> > and yes, those PHYs appear on SFP modules.
> >
> > The proper answer is to use 'managed = "in-band-status";' in your DT
> > to have in-band used with SGMII.
> 
> Well, yeah, I'd love to.  But then I'm back to the drawing board without
> a link.  That just doesn't work for me.

If you have 'managed = "in-band-status";' in your DT, that will set
"mode" to be MLO_AN_INBAND, and phylink_autoneg_inband(mode) will be
true - which should result in the link being programmed for in-band
mode. You should also find that mtk_pcs_get_state() gets called.

Hmm, it looks like setting ss->pcs[i].pcs.poll to true was missed
when support for inband was properly added, so that might be the
issue there - as the mtk ethernet driver doesn't make use of
phylink_mac_change().

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net 2/3] net: mediatek: sgmii: autonegotiation is required

[Bjørn Mork]

"Russell King (Oracle)" <linux@armlinux.org.uk> writes:

> If you have 'managed = "in-band-status";' in your DT, that will set
> "mode" to be MLO_AN_INBAND, and phylink_autoneg_inband(mode) will be
> true - which should result in the link being programmed for in-band
> mode. You should also find that mtk_pcs_get_state() gets called.
>
> Hmm, it looks like setting ss->pcs[i].pcs.poll to true was missed
> when support for inband was properly added, so that might be the
> issue there - as the mtk ethernet driver doesn't make use of
> phylink_mac_change().

OK, this doesn't just work as-is either. But I guess that's something
else.  Will try to debug some more.

But I wonder:  Why would I want to use "in-band-status" here? Is that
the preferred mode?  Probably stupid question.  But shouldn't we also
make this link work without it, whatever that takes?

Note that I don't have to care about unknown phys.  No SFP cage on this
board.


Bjørn

[PATCH net 3/3] net: mediatek: sgmii: fix duplex configuration

[Bjørn Mork]

This bit is cleared for full duplex and set for half duplex.
Rename the macro to avoid confusion.

Signed-off-by: Bjørn Mork <bjorn@mork.no>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.h | 2 +-
 drivers/net/ethernet/mediatek/mtk_sgmii.c   | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
index dff0e3ad2de6..f602a0b5238d 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
@@ -533,7 +533,7 @@
 #define SGMII_SPEED_10			FIELD_PREP(SGMII_SPEED_MASK, 0)
 #define SGMII_SPEED_100			FIELD_PREP(SGMII_SPEED_MASK, 1)
 #define SGMII_SPEED_1000		FIELD_PREP(SGMII_SPEED_MASK, 2)
-#define SGMII_DUPLEX_FULL		BIT(4)
+#define SGMII_DUPLEX_HALF		BIT(4)
 #define SGMII_IF_MODE_BIT5		BIT(5)
 #define SGMII_REMOTE_FAULT_DIS		BIT(8)
 #define SGMII_CODE_SYNC_SET_VAL		BIT(9)
diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c
index d1f2bcb21242..c976e99e37dc 100644
--- a/drivers/net/ethernet/mediatek/mtk_sgmii.c
+++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c
@@ -145,11 +145,11 @@ static void mtk_pcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
 		else
 			sgm_mode = SGMII_SPEED_1000;
 
-		if (duplex == DUPLEX_FULL)
-			sgm_mode |= SGMII_DUPLEX_FULL;
+		if (duplex != DUPLEX_FULL)
+			sgm_mode |= SGMII_DUPLEX_HALF;
 
 		regmap_update_bits(mpcs->regmap, SGMSYS_SGMII_MODE,
-				   SGMII_DUPLEX_FULL | SGMII_SPEED_MASK,
+				   SGMII_DUPLEX_HALF | SGMII_SPEED_MASK,
 				   sgm_mode);
 	}
 }
-- 
2.30.2

Re: [PATCH net 3/3] net: mediatek: sgmii: fix duplex configuration

[Russell King (Oracle)]

On Thu, Jan 19, 2023 at 06:12:48PM +0100, Bjørn Mork wrote:
> This bit is cleared for full duplex and set for half duplex.
> Rename the macro to avoid confusion.
> 
> Signed-off-by: Bjørn Mork <bjorn@mork.no>

Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Thanks!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!