[PATCH net-next v2 0/5] net: mediatek: sgmii stability

[PATCH net-next v2 0/5] net: mediatek: sgmii stability

[Alexander Couzens]

v1 -> v2:
 - add & improve comments on unexpected hw behaviour
 - add patch refactor power cycling into mtk_pcs_config()

Alexander Couzens (5):
  net: mediatek: sgmii: fix powering up the SGMII phy
  net: mediatek: sgmii: ensure the SGMII PHY is powered down on
    configuration
  net: mediatek: sgmii: mtk_pcs_setup_mode_an: don't rely on register
    defaults
  net: mediatek: sgmii: set the speed according to the phy interface in
    AN
  net: mediatek: sgmii: refactor power cycling into mtk_pcs_config()

 drivers/net/ethernet/mediatek/mtk_sgmii.c | 41 ++++++++++++++++-------
 1 file changed, 28 insertions(+), 13 deletions(-)

-- 
2.37.3

[PATCH net-next v2 1/5] net: mediatek: sgmii: fix powering up the SGMII phy

[Alexander Couzens]

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>
---
 drivers/net/ethernet/mediatek/mtk_sgmii.c | 25 ++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c
index 736839c84130..b9b15e1a292c 100644
--- a/drivers/net/ethernet/mediatek/mtk_sgmii.c
+++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c
@@ -36,9 +36,15 @@ static int mtk_pcs_setup_mode_an(struct mtk_pcs *mpcs)
 	val |= SGMII_AN_RESTART;
 	regmap_write(mpcs->regmap, SGMSYS_PCS_CONTROL_1, val);
 
-	regmap_read(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, &val);
-	val &= ~SGMII_PHYA_PWD;
-	regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, val);
+	/* 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.
+	 * Tested on mt7622 & mt7986.
+	 */
+	regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, 0);
 
 	return 0;
 
@@ -69,10 +75,15 @@ static int mtk_pcs_setup_mode_force(struct mtk_pcs *mpcs,
 	val |= SGMII_SPEED_1000;
 	regmap_write(mpcs->regmap, SGMSYS_SGMII_MODE, val);
 
-	/* Release PHYA power down state */
-	regmap_read(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, &val);
-	val &= ~SGMII_PHYA_PWD;
-	regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, val);
+	/* 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.
+	 * Tested on mt7622 & mt7986.
+	 */
+	regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, 0);
 
 	return 0;
 }
-- 
2.37.3

[PATCH net-next v2 2/5] net: mediatek: sgmii: ensure the SGMII PHY is powered down on configuration

[Alexander Couzens]

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.

Signed-off-by: Alexander Couzens <lynxis@fe80.eu>
---
 drivers/net/ethernet/mediatek/mtk_sgmii.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c
index b9b15e1a292c..18de85709e87 100644
--- a/drivers/net/ethernet/mediatek/mtk_sgmii.c
+++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c
@@ -7,6 +7,7 @@
  *
  */
 
+#include <linux/delay.h>
 #include <linux/mfd/syscon.h>
 #include <linux/of.h>
 #include <linux/phylink.h>
@@ -24,6 +25,9 @@ static int mtk_pcs_setup_mode_an(struct mtk_pcs *mpcs)
 {
 	unsigned int val;
 
+	/* PHYA power down */
+	regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, SGMII_PHYA_PWD);
+
 	/* Setup the link timer and QPHY power up inside SGMIISYS */
 	regmap_write(mpcs->regmap, SGMSYS_PCS_LINK_TIMER,
 		     SGMII_LINK_TIMER_DEFAULT);
@@ -42,8 +46,10 @@ static int mtk_pcs_setup_mode_an(struct mtk_pcs *mpcs)
 	 * 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 0;
@@ -58,6 +64,9 @@ static int mtk_pcs_setup_mode_force(struct mtk_pcs *mpcs,
 {
 	unsigned int val;
 
+	/* PHYA power down */
+	regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, SGMII_PHYA_PWD);
+
 	regmap_read(mpcs->regmap, mpcs->ana_rgc3, &val);
 	val &= ~RG_PHY_SPEED_MASK;
 	if (interface == PHY_INTERFACE_MODE_2500BASEX)
@@ -81,8 +90,10 @@ static int mtk_pcs_setup_mode_force(struct mtk_pcs *mpcs,
 	 * 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 0;
-- 
2.37.3

[PATCH net-next v2 3/5] net: mediatek: sgmii: mtk_pcs_setup_mode_an: don't rely on register defaults

[Alexander Couzens]

Ensure autonegotiation is enabled.

Signed-off-by: Alexander Couzens <lynxis@fe80.eu>
---
 drivers/net/ethernet/mediatek/mtk_sgmii.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c
index 18de85709e87..6f4c1ca5a36f 100644
--- a/drivers/net/ethernet/mediatek/mtk_sgmii.c
+++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c
@@ -32,12 +32,13 @@ static int mtk_pcs_setup_mode_an(struct mtk_pcs *mpcs)
 	regmap_write(mpcs->regmap, SGMSYS_PCS_LINK_TIMER,
 		     SGMII_LINK_TIMER_DEFAULT);
 
+	/* disable remote fault & enable auto neg */
 	regmap_read(mpcs->regmap, SGMSYS_SGMII_MODE, &val);
-	val |= SGMII_REMOTE_FAULT_DIS;
+	val |= SGMII_REMOTE_FAULT_DIS | SGMII_SPEED_DUPLEX_AN;
 	regmap_write(mpcs->regmap, SGMSYS_SGMII_MODE, val);
 
 	regmap_read(mpcs->regmap, SGMSYS_PCS_CONTROL_1, &val);
-	val |= SGMII_AN_RESTART;
+	val |= SGMII_AN_RESTART | SGMII_AN_ENABLE;
 	regmap_write(mpcs->regmap, SGMSYS_PCS_CONTROL_1, val);
 
 	/* Release PHYA power down state
-- 
2.37.3

Re: [PATCH net-next v2 3/5] net: mediatek: sgmii: mtk_pcs_setup_mode_an: don't rely on register defaults

[Russell King (Oracle)]

On Mon, Sep 19, 2022 at 10:37:10AM +0200, Alexander Couzens wrote:
> Ensure autonegotiation is enabled.
> 
> Signed-off-by: Alexander Couzens <lynxis@fe80.eu>
> ---
>  drivers/net/ethernet/mediatek/mtk_sgmii.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c
> index 18de85709e87..6f4c1ca5a36f 100644
> --- a/drivers/net/ethernet/mediatek/mtk_sgmii.c
> +++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c
> @@ -32,12 +32,13 @@ static int mtk_pcs_setup_mode_an(struct mtk_pcs *mpcs)
>  	regmap_write(mpcs->regmap, SGMSYS_PCS_LINK_TIMER,
>  		     SGMII_LINK_TIMER_DEFAULT);
>  
> +	/* disable remote fault & enable auto neg */
>  	regmap_read(mpcs->regmap, SGMSYS_SGMII_MODE, &val);
> -	val |= SGMII_REMOTE_FAULT_DIS;
> +	val |= SGMII_REMOTE_FAULT_DIS | SGMII_SPEED_DUPLEX_AN;
>  	regmap_write(mpcs->regmap, SGMSYS_SGMII_MODE, val);
>  
>  	regmap_read(mpcs->regmap, SGMSYS_PCS_CONTROL_1, &val);
> -	val |= SGMII_AN_RESTART;
> +	val |= SGMII_AN_RESTART | SGMII_AN_ENABLE;

I'm not sure if I've asked this before, but why does SGMII_AN_RESTART
need to be set here? It could do with a comment in the code.

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 net-next v2 3/5] net: mediatek: sgmii: mtk_pcs_setup_mode_an: don't rely on register defaults

[Alexander 'lynxis' Couzens]

On Mon, 19 Sep 2022 12:29:34 +0100
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> I'm not sure if I've asked this before, but why does SGMII_AN_RESTART
> need to be set here? It could do with a comment in the code.

It's not my bit :). I've not added it. But why not (re)start autoneg
when powering up the phy?

Should it done elsewhere?

[PATCH net-next v2 4/5] net: mediatek: sgmii: set the speed according to the phy interface in AN

[Alexander Couzens]

The non auto-negotioting code path is setting the correct speed for the
interface. Ensure auto-negotiation code path is doing it as well.

Signed-off-by: Alexander Couzens <lynxis@fe80.eu>
---
 drivers/net/ethernet/mediatek/mtk_sgmii.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c
index 6f4c1ca5a36f..4c8e8c7b1d32 100644
--- a/drivers/net/ethernet/mediatek/mtk_sgmii.c
+++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c
@@ -21,13 +21,20 @@ static struct mtk_pcs *pcs_to_mtk_pcs(struct phylink_pcs *pcs)
 }
 
 /* For SGMII interface mode */
-static int mtk_pcs_setup_mode_an(struct mtk_pcs *mpcs)
+static int mtk_pcs_setup_mode_an(struct mtk_pcs *mpcs, phy_interface_t interface)
 {
 	unsigned int val;
 
 	/* PHYA power down */
 	regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, SGMII_PHYA_PWD);
 
+	/* Set SGMII phy speed */
+	regmap_read(mpcs->regmap, mpcs->ana_rgc3, &val);
+	val &= ~RG_PHY_SPEED_MASK;
+	if (interface == PHY_INTERFACE_MODE_2500BASEX)
+		val |= RG_PHY_SPEED_3_125G;
+	regmap_write(mpcs->regmap, mpcs->ana_rgc3, val);
+
 	/* Setup the link timer and QPHY power up inside SGMIISYS */
 	regmap_write(mpcs->regmap, SGMSYS_PCS_LINK_TIMER,
 		     SGMII_LINK_TIMER_DEFAULT);
@@ -112,7 +119,7 @@ static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
 	if (interface != PHY_INTERFACE_MODE_SGMII)
 		err = mtk_pcs_setup_mode_force(mpcs, interface);
 	else if (phylink_autoneg_inband(mode))
-		err = mtk_pcs_setup_mode_an(mpcs);
+		err = mtk_pcs_setup_mode_an(mpcs, interface);
 
 	return err;
 }
-- 
2.37.3

Re: [PATCH net-next v2 4/5] net: mediatek: sgmii: set the speed according to the phy interface in AN

[Russell King (Oracle)]

On Mon, Sep 19, 2022 at 10:37:11AM +0200, Alexander Couzens wrote:
> The non auto-negotioting code path is setting the correct speed for the
> interface. Ensure auto-negotiation code path is doing it as well.

While I see the logic in doing this in the autoneg path, if you look
at mtk_pcs_config(), you'll notice that this code you're adding is
unreachable.

If interface is PHY_INTERFACE_MODE_2500BASEX, then we will call
mtk_pcs_setup_mode_force(). We only call mtk_pcs_setup_mode_an() for
the PHY_INTERFACE_MODE_SGMII case when in-band mode is selected, so
this can become:

	regmap_read(mpcs->regmap, mpcs->ana_rgc3, &val);
	val &= ~RG_PHY_SPEED_MASK;
	regmap_write(mpcs->regmap, mpcs->ana_rgc3, val);

Thanks.

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

[PATCH net-next v2 5/5] net: mediatek: sgmii: refactor power cycling into mtk_pcs_config()

[Alexander Couzens]

Both code paths (autonegotiated and force mode) are power cycling
the phy. Move power cycling code to the caller to remove code
duplicity.

Signed-off-by: Alexander Couzens <lynxis@fe80.eu>
---
 drivers/net/ethernet/mediatek/mtk_sgmii.c | 45 ++++++++---------------
 1 file changed, 15 insertions(+), 30 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c
index 4c8e8c7b1d32..50f605208295 100644
--- a/drivers/net/ethernet/mediatek/mtk_sgmii.c
+++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c
@@ -25,9 +25,6 @@ static int mtk_pcs_setup_mode_an(struct mtk_pcs *mpcs, phy_interface_t interface
 {
 	unsigned int val;
 
-	/* PHYA power down */
-	regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, SGMII_PHYA_PWD);
-
 	/* Set SGMII phy speed */
 	regmap_read(mpcs->regmap, mpcs->ana_rgc3, &val);
 	val &= ~RG_PHY_SPEED_MASK;
@@ -48,18 +45,6 @@ static int mtk_pcs_setup_mode_an(struct mtk_pcs *mpcs, phy_interface_t interface
 	val |= SGMII_AN_RESTART | SGMII_AN_ENABLE;
 	regmap_write(mpcs->regmap, SGMSYS_PCS_CONTROL_1, val);
 
-	/* 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 0;
 
 }
@@ -72,9 +57,6 @@ static int mtk_pcs_setup_mode_force(struct mtk_pcs *mpcs,
 {
 	unsigned int val;
 
-	/* PHYA power down */
-	regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, SGMII_PHYA_PWD);
-
 	regmap_read(mpcs->regmap, mpcs->ana_rgc3, &val);
 	val &= ~RG_PHY_SPEED_MASK;
 	if (interface == PHY_INTERFACE_MODE_2500BASEX)
@@ -92,18 +74,6 @@ static int mtk_pcs_setup_mode_force(struct mtk_pcs *mpcs,
 	val |= SGMII_SPEED_1000;
 	regmap_write(mpcs->regmap, SGMSYS_SGMII_MODE, val);
 
-	/* 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 0;
 }
 
@@ -115,12 +85,27 @@ static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
 	struct mtk_pcs *mpcs = pcs_to_mtk_pcs(pcs);
 	int err = 0;
 
+	/* PHYA power down */
+	regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, SGMII_PHYA_PWD);
+
 	/* Setup SGMIISYS with the determined property */
 	if (interface != PHY_INTERFACE_MODE_SGMII)
 		err = mtk_pcs_setup_mode_force(mpcs, interface);
 	else if (phylink_autoneg_inband(mode))
 		err = mtk_pcs_setup_mode_an(mpcs, interface);
 
+	/* 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 err;
 }
 
-- 
2.37.3

Re: [PATCH net-next v2 5/5] net: mediatek: sgmii: refactor power cycling into mtk_pcs_config()

[Russell King (Oracle)]

On Mon, Sep 19, 2022 at 10:37:12AM +0200, Alexander Couzens wrote:
> Both code paths (autonegotiated and force mode) are power cycling
> the phy. Move power cycling code to the caller to remove code
> duplicity.

I think we can do more consolidation here - and it probably makes sense
to do in another patch.

> diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c
> index 4c8e8c7b1d32..50f605208295 100644
> --- a/drivers/net/ethernet/mediatek/mtk_sgmii.c
> +++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c
> @@ -25,9 +25,6 @@ static int mtk_pcs_setup_mode_an(struct mtk_pcs *mpcs, phy_interface_t interface
>  {
>  	unsigned int val;
>  
> -	/* PHYA power down */
> -	regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, SGMII_PHYA_PWD);
> -
>  	/* Set SGMII phy speed */
>  	regmap_read(mpcs->regmap, mpcs->ana_rgc3, &val);
>  	val &= ~RG_PHY_SPEED_MASK;
> @@ -72,9 +57,6 @@ static int mtk_pcs_setup_mode_force(struct mtk_pcs *mpcs,
>  {
>  	unsigned int val;
>  
> -	/* PHYA power down */
> -	regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, SGMII_PHYA_PWD);
> -
>  	regmap_read(mpcs->regmap, mpcs->ana_rgc3, &val);
>  	val &= ~RG_PHY_SPEED_MASK;
>  	if (interface == PHY_INTERFACE_MODE_2500BASEX)

After powering the PHY down, the next thing that is done is to configure
the speed. Even with my comments on patch 4, this can still be
consolidated.

> @@ -115,12 +85,27 @@ static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
>  	struct mtk_pcs *mpcs = pcs_to_mtk_pcs(pcs);

	unsigned int val;

>  	int err = 0;
>  
> +	/* PHYA power down */
> +	regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, SGMII_PHYA_PWD);
> +

	regmap_read(mpcs->regmap, mpcs->ana_rgc3, &val);
	val &= ~RG_PHY_SPEED_MASK;
	if (interface == PHY_INTERFACE_MODE_2500BASEX)
		val |= RG_PHY_SPEED_3_125G;
	regmap_write(mpcs->regmap, mpcs->ana_rgc3, val);

which would make logical sense to do here, so we always configure the
speed for the PCS correctly.

That then leaves the configuration of SGMSYS_PCS_CONTROL_1 and
SGMSYS_SGMII_MODE, which I think could also be consolidated, but I'll
leave that to those with the hardware to make that decision.

Reading between the lines of the code in this driver, it looks to me
like this hardware supports only SGMII, but doesn't actually support
1000base-X and 2500base-X with negotiation. Is that correct? If so,
it would be good to add a mtk_pcs_validate() function that clears
ETHTOOL_LINK_MODE_Autoneg_BIT for these modes.

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 net-next v2 5/5] net: mediatek: sgmii: refactor power cycling into mtk_pcs_config()

[Alexander 'lynxis' Couzens]

> > -	/* PHYA power down */
> > -	regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL,
> > SGMII_PHYA_PWD); -
> >  	/* Set SGMII phy speed */
> >  	regmap_read(mpcs->regmap, mpcs->ana_rgc3, &val);
> >  	val &= ~RG_PHY_SPEED_MASK;
> > @@ -72,9 +57,6 @@ static int mtk_pcs_setup_mode_force(struct
> > mtk_pcs *mpcs, {
> >  	unsigned int val;
> >  
> > -	/* PHYA power down */
> > -	regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL,
> > SGMII_PHYA_PWD); -
> >  	regmap_read(mpcs->regmap, mpcs->ana_rgc3, &val);
> >  	val &= ~RG_PHY_SPEED_MASK;
> >  	if (interface == PHY_INTERFACE_MODE_2500BASEX)  
> 
> After powering the PHY down, the next thing that is done is to
> configure the speed. Even with my comments on patch 4, this can still
> be consolidated.

I'll move more code out of the functions.

> 
> > @@ -115,12 +85,27 @@ static int mtk_pcs_config(struct phylink_pcs
> > *pcs, unsigned int mode, struct mtk_pcs *mpcs =
> > pcs_to_mtk_pcs(pcs);  
> 
> 	unsigned int val;
> 
> >  	int err = 0;
> >  
> > +	/* PHYA power down */
> > +	regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL,
> > SGMII_PHYA_PWD);
> > +  
> 
> 	regmap_read(mpcs->regmap, mpcs->ana_rgc3, &val);
> 	val &= ~RG_PHY_SPEED_MASK;
> 	if (interface == PHY_INTERFACE_MODE_2500BASEX)
> 		val |= RG_PHY_SPEED_3_125G;
> 	regmap_write(mpcs->regmap, mpcs->ana_rgc3, val);
> 
> which would make logical sense to do here, so we always configure the
> speed for the PCS correctly.
> 
> That then leaves the configuration of SGMSYS_PCS_CONTROL_1 and
> SGMSYS_SGMII_MODE, which I think could also be consolidated, but I'll
> leave that to those with the hardware to make that decision.
> 
> Reading between the lines of the code in this driver, it looks to me
> like this hardware supports only SGMII, but doesn't actually support
> 1000base-X and 2500base-X with negotiation. Is that correct? If so,
> it would be good to add a mtk_pcs_validate() function that clears
> ETHTOOL_LINK_MODE_Autoneg_BIT for these modes.

I don't know. I don't have hardware to debug
the serdes interface further. I only have a test board with mt7622 soc
connect via SGMII/2500basex to a realtek phy (rtl8221).

Maybe the maintainers from mediatek could share some knowledge if the
SGMII block supports 1000/2500basex autoneg?

At least with the public available datasheets (mt7531, mt7622) doesn't
explain it further.
I could also imagine we need to modify the page register
(PCS_SPEED_ABILITY) and link timer to get autoneg for
1000basex/2500basex working.

Best,
lynxis