[PATCH 0/4] net: mediatek: sgmii: add support to change interface parameter while running

[PATCH 0/4] net: mediatek: sgmii: add support to change interface parameter while running

[Alexander Couzens]

When connecting the SGMII interface to a 2.5gbit ethernet phy (e.g. rtl8221
or a SFP slot) the SGMII interface need to change the SGMII connection
parameters (speed, autoneg, duplex, ..) while running to match the phy speed.

Alexander Couzens (4):
  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

 drivers/net/ethernet/mediatek/mtk_sgmii.c | 40 ++++++++++++++++-------
 1 file changed, 29 insertions(+), 11 deletions(-)

-- 
2.35.1

[PATCH 1/4] 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 | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c
index 736839c84130..a01bb20ea957 100644
--- a/drivers/net/ethernet/mediatek/mtk_sgmii.c
+++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c
@@ -36,9 +36,7 @@ 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);
+	regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, 0);
 
 	return 0;
 
@@ -70,9 +68,7 @@ static int mtk_pcs_setup_mode_force(struct mtk_pcs *mpcs,
 	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);
+	regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, 0);
 
 	return 0;
 }
-- 
2.35.1

[PATCH 2/4] 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 | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c
index a01bb20ea957..782812434367 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);
@@ -36,6 +40,10 @@ static int mtk_pcs_setup_mode_an(struct mtk_pcs *mpcs)
 	val |= SGMII_AN_RESTART;
 	regmap_write(mpcs->regmap, SGMSYS_PCS_CONTROL_1, val);
 
+	/* Release PHYA power down state
+	 * unknown how much the QPHY needs but it is racy without a sleep
+	 */
+	usleep_range(50, 100);
 	regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, 0);
 
 	return 0;
@@ -50,6 +58,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)
@@ -67,7 +78,10 @@ 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 */
+	/* Release PHYA power down state
+	 * unknown how much the QPHY needs but it is racy without a sleep
+	 */
+	usleep_range(50, 100);
 	regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, 0);
 
 	return 0;
-- 
2.35.1

[PATCH 3/4] 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 782812434367..aa69baf1a42f 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.35.1

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

[Alexander Couzens]

The non auto-negotiating 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 aa69baf1a42f..75de2c73a048 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);
@@ -100,7 +107,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.35.1

Re: [PATCH 1/4] net: mediatek: sgmii: fix powering up the SGMII phy

[Paolo Abeni]

Hello,

On Sun, 2022-08-21 at 00:45 +0200, Alexander Couzens wrote:
> 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.

do you have access to register documentation? what does 0x9 actually
mean? is the '0' value based on just empirical evaluation?

Thanks!

Paolo

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

[Paolo Abeni]

On Sun, 2022-08-21 at 00:45 +0200, Alexander Couzens wrote:
> 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 | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c
> index a01bb20ea957..782812434367 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);

in mtk_pcs_setup_mode_an() and in mtk_pcs_setup_mode_force() the code
carefully flips only the SGMII_PHYA_PWD bit. Is it safe to overwrite
the full register contents?

> +
>  	/* Setup the link timer and QPHY power up inside SGMIISYS */
>  	regmap_write(mpcs->regmap, SGMSYS_PCS_LINK_TIMER,
>  		     SGMII_LINK_TIMER_DEFAULT);
> @@ -36,6 +40,10 @@ static int mtk_pcs_setup_mode_an(struct mtk_pcs *mpcs)
>  	val |= SGMII_AN_RESTART;
>  	regmap_write(mpcs->regmap, SGMSYS_PCS_CONTROL_1, val);
>  
> +	/* Release PHYA power down state
> +	 * unknown how much the QPHY needs but it is racy without a sleep
> +	 */
> +	usleep_range(50, 100);

Ouch, this looks fragile, without any related H/W specification. 

>  	regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, 0);
>  
>  	return 0;
> @@ -50,6 +58,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)
> @@ -67,7 +78,10 @@ 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 */
> +	/* Release PHYA power down state
> +	 * unknown how much the QPHY needs but it is racy without a sleep
> +	 */
> +	usleep_range(50, 100);

Same here.

Thanks!

Paolo

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

[Alexander 'lynxis' Couzens]

On Tue, 23 Aug 2022 15:18:31 +0200
Paolo Abeni <pabeni@redhat.com> wrote:

> On Sun, 2022-08-21 at 00:45 +0200, Alexander Couzens wrote:
> > 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 | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c
> > b/drivers/net/ethernet/mediatek/mtk_sgmii.c index
> > a01bb20ea957..782812434367 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);  
> 
> in mtk_pcs_setup_mode_an() and in mtk_pcs_setup_mode_force() the code
> carefully flips only the SGMII_PHYA_PWD bit. Is it safe to overwrite
> the full register contents?

I've read out the register without my patch and it's 0x0. The old driver
worked as long the engine came out of reset.
When writing the single bit SGMII_PHYA_PWD (0x10), the register might
end up containing 0x19 and as long 0x9 is in the register the link
doesn't work.

I've tested the driver with a mt7622 and Daniel Golle tested it with a
mt7986.


> 
> > +
> >  	/* Setup the link timer and QPHY power up inside SGMIISYS
> > */ regmap_write(mpcs->regmap, SGMSYS_PCS_LINK_TIMER,
> >  		     SGMII_LINK_TIMER_DEFAULT);
> > @@ -36,6 +40,10 @@ static int mtk_pcs_setup_mode_an(struct mtk_pcs
> > *mpcs) val |= SGMII_AN_RESTART;
> >  	regmap_write(mpcs->regmap, SGMSYS_PCS_CONTROL_1, val);
> >  
> > +	/* Release PHYA power down state
> > +	 * unknown how much the QPHY needs but it is racy without
> > a sleep
> > +	 */
> > +	usleep_range(50, 100);  
> 
> Ouch, this looks fragile, without any related H/W specification. 

The datasheet [1] doesn't say anything about it. I'ven't found a
mediatek SDK which adds a usleep(). It seems they always expect the
SGMII came out of reset and don't change after initial configured.
But without it, it's racy.

[1] MT7622 Reference Manual, v1.0, 2018-12-19, 1972 pages

> 
> >  	regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, 0);
> >  
> >  	return 0;
> > @@ -50,6 +58,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)
> > @@ -67,7 +78,10 @@ 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 */
> > +	/* Release PHYA power down state
> > +	 * unknown how much the QPHY needs but it is racy without
> > a sleep
> > +	 */
> > +	usleep_range(50, 100);  
> 
> Same here.

Best,
lynxis

Re: [PATCH 1/4] net: mediatek: sgmii: fix powering up the SGMII phy

[Alexander 'lynxis' Couzens]

Hi Paolo,

> On Sun, 2022-08-21 at 00:45 +0200, Alexander Couzens wrote:
> > 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.  
> 
> do you have access to register documentation? what does 0x9 actually
> mean? is the '0' value based on just empirical evaluation?

I don't have any documentation which describes 0x9.
The datasheet [1] only contains the PHYA_PWD (0x10) bit and the initial
value is 0x10. 0x0 value is based on a register readout without the
patch from a working state.

I've tested it on mt7622 and Daniel Golle on mt7986.

[1] MT7622 Reference Manual, v1.0, 2018-12-19, 1972 pages

Best,
lynxis

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

[Russell King (Oracle)]

On Sun, Aug 21, 2022 at 12:45:38AM +0200, Alexander Couzens wrote:
> The non auto-negotiating 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 aa69baf1a42f..75de2c73a048 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);
> +

It looks to me like, after this commit, the initial part of
mtk_pcs_setup_mode_an() and mtk_pcs_setup_mode_force() are identical.
Also, I think that the tail of each of these functions is also
identical.

So, would it make sense for a final patch to tidy this code up?

>  	/* Setup the link timer and QPHY power up inside SGMIISYS */
>  	regmap_write(mpcs->regmap, SGMSYS_PCS_LINK_TIMER,
>  		     SGMII_LINK_TIMER_DEFAULT);
> @@ -100,7 +107,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);

What is the situation when PHY_INTERFACE_MODE_SGMII is being used, but
we're not using inband mode? Right now, we don't do any configuration
of this block, and that seems rather wrong.

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

Re: [PATCH 3/4] net: mediatek: sgmii: mtk_pcs_setup_mode_an: don't rely on register defaults

[Russell King (Oracle)]

On Sun, Aug 21, 2022 at 12:45:37AM +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 782812434367..aa69baf1a42f 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;

Does SGMII_SPEED_DUPLEX_AN need to be cleared in
mtk_pcs_setup_mode_force(), so mtk_pcs_link_up() can force the
duplex setting for base-X protocols?

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

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

[Paolo Abeni]

On Tue, 2022-08-23 at 16:17 +0200, Alexander 'lynxis' Couzens wrote:
> On Tue, 23 Aug 2022 15:18:31 +0200
> Paolo Abeni <pabeni@redhat.com> wrote:
> 
> > On Sun, 2022-08-21 at 00:45 +0200, Alexander Couzens wrote:
> > > 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 | 16 +++++++++++++++-
> > >  1 file changed, 15 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c
> > > b/drivers/net/ethernet/mediatek/mtk_sgmii.c index
> > > a01bb20ea957..782812434367 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);  
> > 
> > in mtk_pcs_setup_mode_an() and in mtk_pcs_setup_mode_force() the code
> > carefully flips only the SGMII_PHYA_PWD bit. Is it safe to overwrite
> > the full register contents?
> 
> I've read out the register without my patch and it's 0x0. The old driver
> worked as long the engine came out of reset.
> When writing the single bit SGMII_PHYA_PWD (0x10), the register might
> end up containing 0x19 and as long 0x9 is in the register the link
> doesn't work.
> 
> I've tested the driver with a mt7622 and Daniel Golle tested it with a
> mt7986.
> 
> 
> > 
> > > +
> > >  	/* Setup the link timer and QPHY power up inside SGMIISYS
> > > */ regmap_write(mpcs->regmap, SGMSYS_PCS_LINK_TIMER,
> > >  		     SGMII_LINK_TIMER_DEFAULT);
> > > @@ -36,6 +40,10 @@ static int mtk_pcs_setup_mode_an(struct mtk_pcs
> > > *mpcs) val |= SGMII_AN_RESTART;
> > >  	regmap_write(mpcs->regmap, SGMSYS_PCS_CONTROL_1, val);
> > >  
> > > +	/* Release PHYA power down state
> > > +	 * unknown how much the QPHY needs but it is racy without
> > > a sleep
> > > +	 */
> > > +	usleep_range(50, 100);  
> > 
> > Ouch, this looks fragile, without any related H/W specification. 
> 
> The datasheet [1] doesn't say anything about it. I'ven't found a
> mediatek SDK which adds a usleep(). It seems they always expect the
> SGMII came out of reset and don't change after initial configured.
> But without it, it's racy.
> 
> [1] MT7622 Reference Manual, v1.0, 2018-12-19, 1972 pages

I see. 

Since it looks like a new revision of this patchset will be needed - as
per Russell comments - I suggest to extend/replace the comments with
something more alike this longer description, it will make the future
mainteinance simpler.

Thanks,

Paolo

Re: [PATCH 3/4] net: mediatek: sgmii: mtk_pcs_setup_mode_an: don't rely on register defaults

[Alexander 'lynxis' Couzens]

On Tue, 23 Aug 2022 16:28:47 +0100
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> On Sun, Aug 21, 2022 at 12:45:37AM +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
> > 782812434367..aa69baf1a42f 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;  
> 
> Does SGMII_SPEED_DUPLEX_AN need to be cleared in
> mtk_pcs_setup_mode_force(), so mtk_pcs_link_up() can force the
> duplex setting for base-X protocols?
> 

Yes SGMII_SPEED_DUPLEX_AN needs to be cleared to have FORCE_DUPLEX
working. But mtk_pcs_setup_mode_force() is clearing it implicit by

val &= SGMII_DUPLEX_FULL & ~SGMII_IF_MODE_MASK

because it's included in the SGMII_IF_MODE_MASK.
I also don't understand why it's forcing it in the
mtk_pcs_link_up().

Re: [PATCH 3/4] net: mediatek: sgmii: mtk_pcs_setup_mode_an: don't rely on register defaults

[Russell King (Oracle)]

On Fri, Sep 02, 2022 at 05:47:10PM +0200, Alexander 'lynxis' Couzens wrote:
> On Tue, 23 Aug 2022 16:28:47 +0100
> "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> 
> > On Sun, Aug 21, 2022 at 12:45:37AM +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
> > > 782812434367..aa69baf1a42f 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;  
> > 
> > Does SGMII_SPEED_DUPLEX_AN need to be cleared in
> > mtk_pcs_setup_mode_force(), so mtk_pcs_link_up() can force the
> > duplex setting for base-X protocols?
> > 
> 
> Yes SGMII_SPEED_DUPLEX_AN needs to be cleared to have FORCE_DUPLEX
> working. But mtk_pcs_setup_mode_force() is clearing it implicit by
> 
> val &= SGMII_DUPLEX_FULL & ~SGMII_IF_MODE_MASK

It would make the code more reasonable to spell out what is included
in SGMII_IF_MODE_MASK. It's:

#define SGMII_SPEED_DUPLEX_AN           BIT(1)
#define SGMII_SPEED_MASK                GENMASK(3, 2)
#define SGMII_DUPLEX_FULL               BIT(4)
#define SGMII_IF_MODE_BIT5              BIT(5)

I'm guessing no one knows what SGMII_IF_MODE_BIT0 and
SGMII_IF_MODE_BIT5 actually do - and as neither seem to be used in
the code, the definitions are redundant.

> because it's included in the SGMII_IF_MODE_MASK.
> I also don't understand why it's forcing it in the
> mtk_pcs_link_up().

Please note that I've forgotten the contents of these patches, so
these comments may not be entirely accurate...

A lot of the code in the driver is quite weird, so when I converted it
to phylink_pcs, I tried to keep the decision making the same as in the
original code. It would help readability if the decision making was
cleaned up - so similar tests in mtk_pcs_link_up() and
mtk_pcs_config().

By that, I mean - if the test in mtk_pcs_link_up() is for 802.3z modes,
then shouldn't the test in mtk_pcs_config() also be using the same?
From what I understand from mtk_eth_soc.c as it originally stood, the
PCS driver is only used for SGMII, 1000base-X and 2500base-X.

The SGMII duplex setting is changed in mtk_pcs_link_up() only for 802.3z
modes - in other words, 1000base-X and 2500base-X.

When mtk_pcs_config() is called for 1000base-X and 2500base-X, it calls
mtk_pcs_setup_mode_force(). This clears SGMII_AN_ENABLE, disabling
autonegotiation, and forcing 1000Mbps. Presumably, this PCS as the code
was originally written is only capable of SGMII negotiation.

It looks to me like the original code does not support autonegotiation
on 802.3z interface modes.

Thanks.

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