[PATCH v3 net 0/3] fixes for mtk_eth_soc

[PATCH v3 net 0/3] fixes for mtk_eth_soc

[Bjørn Mork]

Changes since v2:
 - use "true" for boolean
 - fix SoB typo
 - updated tags

Changes since v1:
- only power down on changes, fix from Russel
- dropped bogus uncondional in-band patch
- added pcs poll patch from Alexander
- updated tags


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 (2):
  net: mediatek: sgmii: ensure the SGMII PHY is powered down on
    configuration
  mtk_sgmii: enable PCS polling to allow SFP work

Bjørn Mork (1):
  net: mediatek: sgmii: fix duplex configuration

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

-- 
2.30.2

[PATCH v3 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.

Only power down when reconfiguring to avoid bouncing the link when there's
no reason to - based on code from Russell King.

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.

Suggested-by: Russell King (Oracle) <linux@armlinux.org.uk>
Signed-off-by: Alexander Couzens <lynxis@fe80.eu>
[ bmork: rebased and squashed into one patch ]
Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
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   | 39 +++++++++++++++------
 2 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
index dff0e3ad2de6..70e729468a95 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
@@ -1067,11 +1067,13 @@ struct mtk_soc_data {
  *                     SGMII modes
  * @ana_rgc3:          The offset refers to register ANA_RGC3 related to regmap
  * @pcs:               Phylink PCS structure
+ * @interface:         Currently configured interface mode
  */
 struct mtk_pcs {
 	struct regmap	*regmap;
 	u32             ana_rgc3;
 	struct phylink_pcs pcs;
+	phy_interface_t	interface;
 };
 
 /* struct mtk_sgmii -  This is the structure holding sgmii regmap and its
diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c
index 5c286f2c9418..0a06995099cf 100644
--- a/drivers/net/ethernet/mediatek/mtk_sgmii.c
+++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c
@@ -43,11 +43,6 @@ static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
 	int advertise, link_timer;
 	bool changed, use_an;
 
-	if (interface == PHY_INTERFACE_MODE_2500BASEX)
-		rgc3 = RG_PHY_SPEED_3_125G;
-	else
-		rgc3 = 0;
-
 	advertise = phylink_mii_c22_pcs_encode_advertisement(interface,
 							     advertising);
 	if (advertise < 0)
@@ -88,9 +83,22 @@ static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
 		bmcr = 0;
 	}
 
-	/* Configure the underlying interface speed */
-	regmap_update_bits(mpcs->regmap, mpcs->ana_rgc3,
-			   RG_PHY_SPEED_3_125G, rgc3);
+	if (mpcs->interface != interface) {
+		/* PHYA power down */
+		regmap_update_bits(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL,
+				   SGMII_PHYA_PWD, SGMII_PHYA_PWD);
+
+		if (interface == PHY_INTERFACE_MODE_2500BASEX)
+			rgc3 = RG_PHY_SPEED_3_125G;
+		else
+			rgc3 = 0;
+
+		/* Configure the underlying interface speed */
+		regmap_update_bits(mpcs->regmap, mpcs->ana_rgc3,
+				   RG_PHY_SPEED_3_125G, rgc3);
+
+		mpcs->interface = interface;
+	}
 
 	/* Update the advertisement, noting whether it has changed */
 	regmap_update_bits_check(mpcs->regmap, SGMSYS_PCS_ADVERTISE,
@@ -108,9 +116,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;
 }
@@ -171,6 +187,7 @@ int mtk_sgmii_init(struct mtk_sgmii *ss, struct device_node *r, u32 ana_rgc3)
 			return PTR_ERR(ss->pcs[i].regmap);
 
 		ss->pcs[i].pcs.ops = &mtk_pcs_ops;
+		ss->pcs[i].interface = PHY_INTERFACE_MODE_NA;
 	}
 
 	return 0;
-- 
2.30.2

[PATCH v3 net 2/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.

Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
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 70e729468a95..bab3699dcdca 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 0a06995099cf..c4261069b521 100644
--- a/drivers/net/ethernet/mediatek/mtk_sgmii.c
+++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c
@@ -154,11 +154,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

[PATCH v3 net 3/3] mtk_sgmii: enable PCS polling to allow SFP work

[Bjørn Mork]

From: Alexander Couzens <lynxis@fe80.eu>

Currently there is no IRQ handling (even the SGMII supports it).
Enable polling to support SFP ports.

Signed-off-by: Alexander Couzens <lynxis@fe80.eu>
[ bmork: changed "1" => "true" ]
Signed-off-by: Bjørn Mork <bjorn@mork.no>
---
 drivers/net/ethernet/mediatek/mtk_sgmii.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c
index c4261069b521..bb00de1003ac 100644
--- a/drivers/net/ethernet/mediatek/mtk_sgmii.c
+++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c
@@ -187,6 +187,7 @@ int mtk_sgmii_init(struct mtk_sgmii *ss, struct device_node *r, u32 ana_rgc3)
 			return PTR_ERR(ss->pcs[i].regmap);
 
 		ss->pcs[i].pcs.ops = &mtk_pcs_ops;
+		ss->pcs[i].pcs.poll = true;
 		ss->pcs[i].interface = PHY_INTERFACE_MODE_NA;
 	}
 
-- 
2.30.2

Re: [PATCH v3 net 3/3] mtk_sgmii: enable PCS polling to allow SFP work

[Russell King (Oracle)]

On Sun, Jan 22, 2023 at 10:21:53PM +0100, Bjørn Mork wrote:
> From: Alexander Couzens <lynxis@fe80.eu>
> 
> Currently there is no IRQ handling (even the SGMII supports it).
> Enable polling to support SFP ports.
> 
> Signed-off-by: Alexander Couzens <lynxis@fe80.eu>
> [ bmork: changed "1" => "true" ]
> 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!

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

[Simon Horman]

On Sun, Jan 22, 2023 at 10:21:51PM +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.
> 
> Only power down when reconfiguring to avoid bouncing the link when there's
> no reason to - based on code from Russell King.
> 
> 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.
> 
> Suggested-by: Russell King (Oracle) <linux@armlinux.org.uk>
> Signed-off-by: Alexander Couzens <lynxis@fe80.eu>
> [ bmork: rebased and squashed into one patch ]
> Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> 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   | 39 +++++++++++++++------
>  2 files changed, 30 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> index dff0e3ad2de6..70e729468a95 100644
> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> @@ -1067,11 +1067,13 @@ struct mtk_soc_data {
>   *                     SGMII modes
>   * @ana_rgc3:          The offset refers to register ANA_RGC3 related to regmap
>   * @pcs:               Phylink PCS structure
> + * @interface:         Currently configured interface mode
>   */
>  struct mtk_pcs {
>  	struct regmap	*regmap;
>  	u32             ana_rgc3;

nit: on x86_64 (at least) there is a 4 byte hole here.
It could be filled by the interface field.

>  	struct phylink_pcs pcs;
> +	phy_interface_t	interface;
>  };

...

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

[Paolo Abeni]

On Sun, 2023-01-22 at 22:21 +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.
> 
> Only power down when reconfiguring to avoid bouncing the link when there's
> no reason to - based on code from Russell King.
> 
> 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.

This looks like a legitimate fix for -net, but we need a suitable Fixes
tag pointing to the culprit commit.

Please repost including such tag. While at that you could also consider
including Simon's suggestion.

The following 2 patches looks like new features/refactor that would be
more suitable for net-next, and included here due to the code
dependency.

If so, please repost the later 2 separately for net-next after the fix
will land there.

Thanks,

Paolo

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

[Bjørn Mork]

Paolo Abeni <pabeni@redhat.com> writes:
> On Sun, 2023-01-22 at 22:21 +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.
>> 
>> Only power down when reconfiguring to avoid bouncing the link when there's
>> no reason to - based on code from Russell King.
>> 
>> 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.
>
> This looks like a legitimate fix for -net, but we need a suitable Fixes
> tag pointing to the culprit commit.
>
> Please repost including such tag. While at that you could also consider
> including Simon's suggestion.

Thanks.  Will do.  Taking advantage of the hole is a good idea.

> The following 2 patches looks like new features/refactor that would be
> more suitable for net-next, and included here due to the code
> dependency.
>
> If so, please repost the later 2 separately for net-next after the fix
> will land there.

I believe all these 3 patches are fixes and therefore appropriate for
net.  I will verify and add Fixes tags for all of them.


Bjørn

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

[Russell King (Oracle)]

On Tue, Jan 24, 2023 at 01:19:15PM +0100, Paolo Abeni wrote:
> On Sun, 2023-01-22 at 22:21 +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.
> > 
> > Only power down when reconfiguring to avoid bouncing the link when there's
> > no reason to - based on code from Russell King.
> > 
> > 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.
> 
> This looks like a legitimate fix for -net, but we need a suitable Fixes
> tag pointing to the culprit commit.
> 
> Please repost including such tag. While at that you could also consider
> including Simon's suggestion.
> 
> The following 2 patches looks like new features/refactor that would be
> more suitable for net-next, and included here due to the code
> dependency.

I'm not sure why you think that, especially for patch 2.

Patch 2 corrects the sense of the duplex bit - the code originally
set this for full duplex, but in actual fact, the bit needs to be set
for half duplex. I can't see how one could regard that as a feature
or a refactor.

I'm also not sure how you could regard patch 3 as a refactor. It
could be argued that it is a new feature, but it is actually a bug
fix for the patch converting the driver to phylink_pcs which
omitted setting this, making the pcs_get_state() method rather
useless.

So I regard all three patches as fixes, not features or refactoring.

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

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

[Paolo Abeni]

On Tue, 2023-01-24 at 12:45 +0000, Russell King (Oracle) wrote:
> On Tue, Jan 24, 2023 at 01:19:15PM +0100, Paolo Abeni wrote:
> > On Sun, 2023-01-22 at 22:21 +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.
> > > 
> > > Only power down when reconfiguring to avoid bouncing the link when there's
> > > no reason to - based on code from Russell King.
> > > 
> > > 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.
> > 
> > This looks like a legitimate fix for -net, but we need a suitable Fixes
> > tag pointing to the culprit commit.
> > 
> > Please repost including such tag. While at that you could also consider
> > including Simon's suggestion.
> > 
> > The following 2 patches looks like new features/refactor that would be
> > more suitable for net-next, and included here due to the code
> > dependency.
> 
> I'm not sure why you think that, especially for patch 2.

Because I misread the patch contents as a simple macro rename (I missed
the s/==/!=/ part).
> 
> Patch 2 corrects the sense of the duplex bit - the code originally
> set this for full duplex, but in actual fact, the bit needs to be set
> for half duplex. I can't see how one could regard that as a feature
> or a refactor.
> 
> I'm also not sure how you could regard patch 3 as a refactor. It
> could be argued that it is a new feature, but it is actually a bug
> fix for the patch converting the driver to phylink_pcs which
> omitted setting this, making the pcs_get_state() method rather
> useless.

Well, it looked more a new feature than a fix to me. The above
explanation cleared the matter. A more descriptive commit could have
avoid confusion on my side :)

> So I regard all three patches as fixes, not features or refactoring.

No objections on my side, but please add suitable fixes tag on every
patch then.

Thanks!

Paolo