net: phy: marvell-88q2xxx: add driver for the Marvell 88Q2220 PHY

  Add a driver for the Marvell 88Q2220. This driver allows to detect the
link, switch between 100BASE-T1 and 1000BASE-T1 and switch between
master and slave mode and autonegotiation.

Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
---
 drivers/net/phy/marvell-88q2xxx.c | 258 +++++++++++++++++++++++++++++-
 include/linux/marvell_phy.h       |   1 +
 2 files changed, 258 insertions(+), 1 deletion(-)
  

> +static int mv88q222x_config_aneg_gbit(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	/* send_s detection threshold, slave and master */
> +	ret = phy_write_mmd(phydev, MDIO_MMD_AN, 0x8032, 0x2020);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = phy_write_mmd(phydev, MDIO_MMD_AN, 0x8031, 0xa28);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = phy_write_mmd(phydev, MDIO_MMD_AN, 0x8031, 0xc28);
> +	if (ret < 0)
> +		return ret;

Same register with two different values?

There are a lot of magic values here. Does the datasheet names these
registers? Does it define the bits? Adding #defines would be good.

> +static int mv88q222x_config_aneg_preinit(struct phy_device *phydev)
> +{
> +	int ret, val, i;
> +
> +	/* Enable txdac */
> +	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0x8033, 0x6801);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Disable ANEG */
> +	ret = phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_T1_CTRL, 0x0);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Set IEEE power down */
> +	ret = phy_write_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1, 0x840);

0x800 is MDIO_CTRL1_LPOWER. What is the other? It seems like a speed
selection bit?

	  Andrew
  

Am Sat, Dec 16, 2023 at 05:46:32PM +0100 schrieb Andrew Lunn:
> > +static int mv88q222x_config_aneg_gbit(struct phy_device *phydev)
> > +{
> > +	int ret;
> > +
> > +	/* send_s detection threshold, slave and master */
> > +	ret = phy_write_mmd(phydev, MDIO_MMD_AN, 0x8032, 0x2020);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = phy_write_mmd(phydev, MDIO_MMD_AN, 0x8031, 0xa28);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = phy_write_mmd(phydev, MDIO_MMD_AN, 0x8031, 0xc28);
> > +	if (ret < 0)
> > +		return ret;
> 
> Same register with two different values?
>
Just copied the init sequence from sample code provided by Marvell.
I don't know if its a mistake. There is no documentation on this.

> There are a lot of magic values here. Does the datasheet names these
> registers? Does it define the bits? Adding #defines would be good.
> 
Datasheet is not naming them. I once asked Marvell Support for
documentation on the init sequence and what purpose each register has.
Just got the answer to use the sample code as it is.

> > +static int mv88q222x_config_aneg_preinit(struct phy_device *phydev)
> > +{
> > +	int ret, val, i;
> > +
> > +	/* Enable txdac */
> > +	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0x8033, 0x6801);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/* Disable ANEG */
> > +	ret = phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_T1_CTRL, 0x0);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/* Set IEEE power down */
> > +	ret = phy_write_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1, 0x840);
> 
> 0x800 is MDIO_CTRL1_LPOWER. What is the other? It seems like a speed
> selection bit?
>
The other is MDIO_PMA_CTRL1_SPEED1000. Will fix this in V2.

> 	  Andrew
  

> > > +	/* Set IEEE power down */
> > > +	ret = phy_write_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1, 0x840);
> > 
> > 0x800 is MDIO_CTRL1_LPOWER. What is the other? It seems like a speed
> > selection bit?
> >
> The other is MDIO_PMA_CTRL1_SPEED1000. Will fix this in V2.

It seems odd to set a speed, and power it down. But i guess you have
blindly copied the reference code, so have no idea why?

	Andrew
  

Am Sun, Dec 17, 2023 at 10:22:54AM +0100 schrieb Andrew Lunn:
> > > > +	/* Set IEEE power down */
> > > > +	ret = phy_write_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1, 0x840);
> > > 
> > > 0x800 is MDIO_CTRL1_LPOWER. What is the other? It seems like a speed
> > > selection bit?
> > >
> > The other is MDIO_PMA_CTRL1_SPEED1000. Will fix this in V2.
> 
> It seems odd to set a speed, and power it down. But i guess you have
> blindly copied the reference code, so have no idea why?
>
I agree, absolutely no idea. I already asked the Marvell support for
any document describing the init sequence, but they couldn't help me.
So I have to stick to the reference code. At least I copied the comments
that were part of the init sequence, trying to give some meaning to it.

> 	Andrew

	Dimitri
  

Hi Dimitri,

On Sun, Dec 17, 2023 at 12:15:38PM +0100, Dimitri Fedrau wrote:
> Am Sun, Dec 17, 2023 at 10:22:54AM +0100 schrieb Andrew Lunn:
> > > > > +	/* Set IEEE power down */
> > > > > +	ret = phy_write_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1, 0x840);
> > > > 
> > > > 0x800 is MDIO_CTRL1_LPOWER. What is the other? It seems like a speed
> > > > selection bit?
> > > >
> > > The other is MDIO_PMA_CTRL1_SPEED1000. Will fix this in V2.
> > 
> > It seems odd to set a speed, and power it down. But i guess you have
> > blindly copied the reference code, so have no idea why?
> >
> I agree, absolutely no idea. I already asked the Marvell support for
> any document describing the init sequence, but they couldn't help me.
> So I have to stick to the reference code. At least I copied the comments
> that were part of the init sequence, trying to give some meaning to it.

I also tried to make the 88Q2221 work but didn't find the time yet to
write a clean version yet. My last minimal patch looks as attached
bellow.

I think the main thing to make the PHY work is to call this
sequence to set the master/slave detection threshold:

/* Set detection threshold slave master */
phy_write_mmd(phydev, MDIO_MMD_AN, 0x8032, 0x2020);
phy_write_mmd(phydev, MDIO_MMD_AN, 0x8031, 0x0a28);
phy_write_mmd(phydev, MDIO_MMD_AN, 0x8031, 0x0c28);

Without this sequence the PHY does not work. I was also wondering as
Andrew wrote why we write twice to the same register. My assumption is
that 0x8032 is some kind of selector for a subregister while 0x8031 will
set a 32 bit value. Unforunately, I also didn't get that information
from Marvell and it is just a wild guess. Please also note that calling
the sequence in the probe function (as I do it in the example below) is
definitely wrong, it was just a quick and dirty test I did because I
wanted to know if it is enough to call it only once.

Are you able to test everyting with the upstream kernel? I'm asking
because I have to backport a lot of stuff to a downstream kernel 5.15
from NXP to test the 88Q2221. 

Further, are you able to verify that autonegotion works? Somehow for me
this never really worked even when using the example sequence from
Marvell.

Best regards,
Stefan

diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
index 94a8c99b58da..15e82e8ff8f4 100644
--- a/drivers/net/phy/marvell-88q2xxx.c
+++ b/drivers/net/phy/marvell-88q2xxx.c
@@ -208,17 +214,26 @@ static int mv88q2xxxx_get_sqi(struct phy_device *phydev)
 
 		ret = ret >> 12;
 	} else {
-		/* Read from vendor specific registers, they are not documented
-		 * but can be found in the Software Initialization Guide. Only
-		 * revisions >= A0 are supported.
-		 */
-		ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, 0xFC5D, 0x00FF, 0x00AC);
-		if (ret < 0)
-			return ret;
+		if (phydev->drv->phy_id == MARVELL_PHY_ID_88Q2221) {
+			/* Read from vendor specific register, they can be
+			 * found in the sample source code of the Q222X API
+			 */
+			ret = phy_read_mmd(phydev, MDIO_MMD_PCS, 0xfcd9);
+			if (ret < 0)
+				return ret;
+		} else {
+			/* Read from vendor specific registers, they are not documented
+			 * but can be found in the Software Initialization Guide. Only
+			 * revisions >= A0 are supported.
+			 */
+			ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, 0xFC5D, 0x00FF, 0x00AC);
+			if (ret < 0)
+				return ret;
 
-		ret = phy_read_mmd(phydev, MDIO_MMD_PCS, 0xfc88);
-		if (ret < 0)
-			return ret;
+			ret = phy_read_mmd(phydev, MDIO_MMD_PCS, 0xfc88);
+			if (ret < 0)
+				return ret;
+		}
 	}
 
 	return ret & 0x0F;
@@ -229,6 +244,16 @@ static int mv88q2xxxx_get_sqi_max(struct phy_device *phydev)
 	return 15;
 }
 
+static int mv88q2221_probe(struct phy_device *phydev)
+{
+	/* Set detection threshold slave master */
+	phy_write_mmd(phydev, MDIO_MMD_AN, 0x8032, 0x2020);
+	phy_write_mmd(phydev, MDIO_MMD_AN, 0x8031, 0x0a28);
+	phy_write_mmd(phydev, MDIO_MMD_AN, 0x8031, 0x0c28);
+
+	return 0;
+}
+
 static struct phy_driver mv88q2xxx_driver[] = {
 	{
 		.phy_id			= MARVELL_PHY_ID_88Q2110,
@@ -243,12 +268,27 @@ static struct phy_driver mv88q2xxx_driver[] = {
 		.get_sqi		= mv88q2xxxx_get_sqi,
 		.get_sqi_max		= mv88q2xxxx_get_sqi_max,
 	},
+	{
+		.phy_id			= MARVELL_PHY_ID_88Q2221,
+		.phy_id_mask		= MARVELL_PHY_ID_MASK,
+		.name			= "mv88q2221",
+		.get_features		= mv88q2xxx_get_features,
+		.config_aneg		= mv88q2xxx_config_aneg,
+		.config_init		= mv88q2xxx_config_init,
+		.read_status		= mv88q2xxx_read_status,
+		.soft_reset		= mv88q2xxx_soft_reset,
+		.set_loopback		= genphy_c45_loopback,
+		.get_sqi		= mv88q2xxxx_get_sqi,
+		.get_sqi_max		= mv88q2xxxx_get_sqi_max,
+		.probe			= mv88q2221_probe,
+	},
 };
 
 module_phy_driver(mv88q2xxx_driver);
 
 static struct mdio_device_id __maybe_unused mv88q2xxx_tbl[] = {
 	{ MARVELL_PHY_ID_88Q2110, MARVELL_PHY_ID_MASK },
+	{ MARVELL_PHY_ID_88Q2221, MARVELL_PHY_ID_MASK },
 	{ /*sentinel*/ }
 };
 MODULE_DEVICE_TABLE(mdio, mv88q2xxx_tbl);
  

Am Sun, Dec 17, 2023 at 02:50:49PM +0100 schrieb Stefan Eichenberger:
> Hi Dimitri,
>
Hi Stefan,

> On Sun, Dec 17, 2023 at 12:15:38PM +0100, Dimitri Fedrau wrote:
> > Am Sun, Dec 17, 2023 at 10:22:54AM +0100 schrieb Andrew Lunn:
> > > > > > +	/* Set IEEE power down */
> > > > > > +	ret = phy_write_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1, 0x840);
> > > > > 
> > > > > 0x800 is MDIO_CTRL1_LPOWER. What is the other? It seems like a speed
> > > > > selection bit?
> > > > >
> > > > The other is MDIO_PMA_CTRL1_SPEED1000. Will fix this in V2.
> > > 
> > > It seems odd to set a speed, and power it down. But i guess you have
> > > blindly copied the reference code, so have no idea why?
> > >
> > I agree, absolutely no idea. I already asked the Marvell support for
> > any document describing the init sequence, but they couldn't help me.
> > So I have to stick to the reference code. At least I copied the comments
> > that were part of the init sequence, trying to give some meaning to it.
> 
> I also tried to make the 88Q2221 work but didn't find the time yet to
> write a clean version yet. My last minimal patch looks as attached
> bellow.
> 
I probably will also get a 88Q2221 PHY, but it could take some time.
When looking at the reference code the only difference for the 88Q2220
and 88Q2221 seems to be an additional init sequence with 28 register writes.
Remaining code seems to be identical. Am I right ? If yes we can use the
same code base here. Besides that it seems that both PHYs share the same
PHY id and are only distinguished by the "Secondary ID Register".

> I think the main thing to make the PHY work is to call this
> sequence to set the master/slave detection threshold:
> 
> /* Set detection threshold slave master */
> phy_write_mmd(phydev, MDIO_MMD_AN, 0x8032, 0x2020);
> phy_write_mmd(phydev, MDIO_MMD_AN, 0x8031, 0x0a28);
> phy_write_mmd(phydev, MDIO_MMD_AN, 0x8031, 0x0c28);
> 
> Without this sequence the PHY does not work. I was also wondering as
> Andrew wrote why we write twice to the same register. My assumption is
> that 0x8032 is some kind of selector for a subregister while 0x8031 will
> set a 32 bit value. Unforunately, I also didn't get that information
> from Marvell and it is just a wild guess. Please also note that calling
> the sequence in the probe function (as I do it in the example below) is
> definitely wrong, it was just a quick and dirty test I did because I
> wanted to know if it is enough to call it only once.
> 
You are maybe right about your guess. Without the init sequence at all I
was able to get the PHY work with a fixed setting (100Mbit/Master).
Maybe it was due to bootstrapping the pins of the PHY. But still I'm not
getting the point. What are we going to do ? Do we want to strip down or
generalize the init sequence ? There is probably a reason for such an
annoying large undocumented series of register writes.

> Are you able to test everyting with the upstream kernel? I'm asking
> because I have to backport a lot of stuff to a downstream kernel 5.15
> from NXP to test the 88Q2221. 
> 
I'm testing with the upstream kernel, no problems so far. Didn't have to
backport anything or test on another kernel. First version of my driver
was also on NXPs 5.15 kernel. Gladly you already upstreamed some T1
specific code which helped me a lot to reduce code size.

> Further, are you able to verify that autonegotion works? Somehow for me
> this never really worked even when using the example sequence from
> Marvell.
> 

Autonegotiation works fine, didn't have any problems. I'm using the
88Q2220M rev B0. I test it with a Media Converter, the NETLion1000 C2T and
with another 88Q2220M PHY. What do you use for testing ?

> Best regards,
> Stefan
> 
Best regards,
Dimitri

> diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
> index 94a8c99b58da..15e82e8ff8f4 100644
> --- a/drivers/net/phy/marvell-88q2xxx.c
> +++ b/drivers/net/phy/marvell-88q2xxx.c
> @@ -208,17 +214,26 @@ static int mv88q2xxxx_get_sqi(struct phy_device *phydev)
>  
>  		ret = ret >> 12;
>  	} else {
> -		/* Read from vendor specific registers, they are not documented
> -		 * but can be found in the Software Initialization Guide. Only
> -		 * revisions >= A0 are supported.
> -		 */
> -		ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, 0xFC5D, 0x00FF, 0x00AC);
> -		if (ret < 0)
> -			return ret;
> +		if (phydev->drv->phy_id == MARVELL_PHY_ID_88Q2221) {
> +			/* Read from vendor specific register, they can be
> +			 * found in the sample source code of the Q222X API
> +			 */
> +			ret = phy_read_mmd(phydev, MDIO_MMD_PCS, 0xfcd9);
> +			if (ret < 0)
> +				return ret;
> +		} else {
> +			/* Read from vendor specific registers, they are not documented
> +			 * but can be found in the Software Initialization Guide. Only
> +			 * revisions >= A0 are supported.
> +			 */
> +			ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, 0xFC5D, 0x00FF, 0x00AC);
> +			if (ret < 0)
> +				return ret;
>  
> -		ret = phy_read_mmd(phydev, MDIO_MMD_PCS, 0xfc88);
> -		if (ret < 0)
> -			return ret;
> +			ret = phy_read_mmd(phydev, MDIO_MMD_PCS, 0xfc88);
> +			if (ret < 0)
> +				return ret;
> +		}
>  	}
>  
>  	return ret & 0x0F;
> @@ -229,6 +244,16 @@ static int mv88q2xxxx_get_sqi_max(struct phy_device *phydev)
>  	return 15;
>  }
>  
> +static int mv88q2221_probe(struct phy_device *phydev)
> +{
> +	/* Set detection threshold slave master */
> +	phy_write_mmd(phydev, MDIO_MMD_AN, 0x8032, 0x2020);
> +	phy_write_mmd(phydev, MDIO_MMD_AN, 0x8031, 0x0a28);
> +	phy_write_mmd(phydev, MDIO_MMD_AN, 0x8031, 0x0c28);
> +
> +	return 0;
> +}
> +
>  static struct phy_driver mv88q2xxx_driver[] = {
>  	{
>  		.phy_id			= MARVELL_PHY_ID_88Q2110,
> @@ -243,12 +268,27 @@ static struct phy_driver mv88q2xxx_driver[] = {
>  		.get_sqi		= mv88q2xxxx_get_sqi,
>  		.get_sqi_max		= mv88q2xxxx_get_sqi_max,
>  	},
> +	{
> +		.phy_id			= MARVELL_PHY_ID_88Q2221,
> +		.phy_id_mask		= MARVELL_PHY_ID_MASK,
> +		.name			= "mv88q2221",
> +		.get_features		= mv88q2xxx_get_features,
> +		.config_aneg		= mv88q2xxx_config_aneg,
> +		.config_init		= mv88q2xxx_config_init,
> +		.read_status		= mv88q2xxx_read_status,
> +		.soft_reset		= mv88q2xxx_soft_reset,
> +		.set_loopback		= genphy_c45_loopback,
> +		.get_sqi		= mv88q2xxxx_get_sqi,
> +		.get_sqi_max		= mv88q2xxxx_get_sqi_max,
> +		.probe			= mv88q2221_probe,
> +	},
>  };
>  
>  module_phy_driver(mv88q2xxx_driver);
>  
>  static struct mdio_device_id __maybe_unused mv88q2xxx_tbl[] = {
>  	{ MARVELL_PHY_ID_88Q2110, MARVELL_PHY_ID_MASK },
> +	{ MARVELL_PHY_ID_88Q2221, MARVELL_PHY_ID_MASK },
>  	{ /*sentinel*/ }
>  };
>  MODULE_DEVICE_TABLE(mdio, mv88q2xxx_tbl);
  

Hi Dimitri,

On Mon, Dec 18, 2023 at 10:09:32AM +0100, Dimitri Fedrau wrote:
> Am Sun, Dec 17, 2023 at 02:50:49PM +0100 schrieb Stefan Eichenberger:
> > I also tried to make the 88Q2221 work but didn't find the time yet to
> > write a clean version yet. My last minimal patch looks as attached
> > bellow.
> > 
> I probably will also get a 88Q2221 PHY, but it could take some time.
> When looking at the reference code the only difference for the 88Q2220
> and 88Q2221 seems to be an additional init sequence with 28 register writes.
> Remaining code seems to be identical. Am I right ? If yes we can use the
> same code base here. Besides that it seems that both PHYs share the same
> PHY id and are only distinguished by the "Secondary ID Register".

I think the init sequence is the same for both PHYs. At least they share
the same reference manual and the API User Guide.

> > I think the main thing to make the PHY work is to call this
> > sequence to set the master/slave detection threshold:
> > 
> > /* Set detection threshold slave master */
> > phy_write_mmd(phydev, MDIO_MMD_AN, 0x8032, 0x2020);
> > phy_write_mmd(phydev, MDIO_MMD_AN, 0x8031, 0x0a28);
> > phy_write_mmd(phydev, MDIO_MMD_AN, 0x8031, 0x0c28);
> > 
> > Without this sequence the PHY does not work. I was also wondering as
> > Andrew wrote why we write twice to the same register. My assumption is
> > that 0x8032 is some kind of selector for a subregister while 0x8031 will
> > set a 32 bit value. Unforunately, I also didn't get that information
> > from Marvell and it is just a wild guess. Please also note that calling
> > the sequence in the probe function (as I do it in the example below) is
> > definitely wrong, it was just a quick and dirty test I did because I
> > wanted to know if it is enough to call it only once.
> > 
> You are maybe right about your guess. Without the init sequence at all I
> was able to get the PHY work with a fixed setting (100Mbit/Master).
> Maybe it was due to bootstrapping the pins of the PHY. But still I'm not
> getting the point. What are we going to do ? Do we want to strip down or
> generalize the init sequence ? There is probably a reason for such an
> annoying large undocumented series of register writes.

The documentation is really annoying, I agree. I would propose to try to
keep the driver as minimal as possible. If we see that something is not
working, we can still add it later on. Maybe this helps to get a better
understanding of what the registers do. Further, they always do a full
initialization when they switch e.g. from 100MBit/s to 1GBit/s. This
definitely seems to be unnecessary.

> > Are you able to test everyting with the upstream kernel? I'm asking
> > because I have to backport a lot of stuff to a downstream kernel 5.15
> > from NXP to test the 88Q2221. 
> > 
> I'm testing with the upstream kernel, no problems so far. Didn't have to
> backport anything or test on another kernel. First version of my driver
> was also on NXPs 5.15 kernel. Gladly you already upstreamed some T1
> specific code which helped me a lot to reduce code size.

That's good to hear. The 88Q2221 in my setup is connected to an S32G274A
from NXP and unfortunately that one doesn't have proper upstream support
yet.

> > Further, are you able to verify that autonegotion works? Somehow for me
> > this never really worked even when using the example sequence from
> > Marvell.
> > 
> 
> Autonegotiation works fine, didn't have any problems. I'm using the
> 88Q2220M rev B0. I test it with a Media Converter, the NETLion1000 C2T and
> with another 88Q2220M PHY. What do you use for testing ?

I have to try it again. I'm using the Goepel Media Converter (EasyCON)
and I'm pretty sure autoneg works on the Media Converter but somehow not
on the PHY side. It could be that this is because of one of this
undocumented registers.

Regards,
Stefan
  

Am Mon, Dec 18, 2023 at 12:19:32PM +0100 schrieb Stefan Eichenberger:
> Hi Dimitri,
> 
> On Mon, Dec 18, 2023 at 10:09:32AM +0100, Dimitri Fedrau wrote:
> > Am Sun, Dec 17, 2023 at 02:50:49PM +0100 schrieb Stefan Eichenberger:
> > > I also tried to make the 88Q2221 work but didn't find the time yet to
> > > write a clean version yet. My last minimal patch looks as attached
> > > bellow.
> > > 
> > I probably will also get a 88Q2221 PHY, but it could take some time.
> > When looking at the reference code the only difference for the 88Q2220
> > and 88Q2221 seems to be an additional init sequence with 28 register writes.
> > Remaining code seems to be identical. Am I right ? If yes we can use the
> > same code base here. Besides that it seems that both PHYs share the same
> > PHY id and are only distinguished by the "Secondary ID Register".
> 
> I think the init sequence is the same for both PHYs. At least they share
> the same reference manual and the API User Guide.
>
I could add the init sequence for the 88Q2221 PHY. Then you could test
it on your side. Would this be helpful to you ? Did you already have the
chance to test the patch ?

> > > I think the main thing to make the PHY work is to call this
> > > sequence to set the master/slave detection threshold:
> > > 
> > > /* Set detection threshold slave master */
> > > phy_write_mmd(phydev, MDIO_MMD_AN, 0x8032, 0x2020);
> > > phy_write_mmd(phydev, MDIO_MMD_AN, 0x8031, 0x0a28);
> > > phy_write_mmd(phydev, MDIO_MMD_AN, 0x8031, 0x0c28);
> > > 
> > > Without this sequence the PHY does not work. I was also wondering as
> > > Andrew wrote why we write twice to the same register. My assumption is
> > > that 0x8032 is some kind of selector for a subregister while 0x8031 will
> > > set a 32 bit value. Unforunately, I also didn't get that information
> > > from Marvell and it is just a wild guess. Please also note that calling
> > > the sequence in the probe function (as I do it in the example below) is
> > > definitely wrong, it was just a quick and dirty test I did because I
> > > wanted to know if it is enough to call it only once.
> > > 
> > You are maybe right about your guess. Without the init sequence at all I
> > was able to get the PHY work with a fixed setting (100Mbit/Master).
> > Maybe it was due to bootstrapping the pins of the PHY. But still I'm not
> > getting the point. What are we going to do ? Do we want to strip down or
> > generalize the init sequence ? There is probably a reason for such an
> > annoying large undocumented series of register writes.
> 
> The documentation is really annoying, I agree. I would propose to try to
> keep the driver as minimal as possible. If we see that something is not
> working, we can still add it later on. Maybe this helps to get a better
> understanding of what the registers do. Further, they always do a full
> initialization when they switch e.g. from 100MBit/s to 1GBit/s. This
> definitely seems to be unnecessary.
> 
You are right, but I would propose to stick to the reference init
sequence and make sure the PHYs works with our code and then work on
optimizing the code. We still can remove and/or document parts of it.

> > > Further, are you able to verify that autonegotion works? Somehow for me
> > > this never really worked even when using the example sequence from
> > > Marvell.
> > > 
> > 
> > Autonegotiation works fine, didn't have any problems. I'm using the
> > 88Q2220M rev B0. I test it with a Media Converter, the NETLion1000 C2T and
> > with another 88Q2220M PHY. What do you use for testing ?
> 
> I have to try it again. I'm using the Goepel Media Converter (EasyCON)
> and I'm pretty sure autoneg works on the Media Converter but somehow not
> on the PHY side. It could be that this is because of one of this
> undocumented registers.
> 
Are you trying with the patch I provided or your own code ? If you use
my patch you should wait until V3, because I found some problems with
it. Switching from 1000Mbit/s to 100Mbit/s in autonegotiation mode doesn't
work. I could fix it but the fix touches some code already upstreamed. So
I tried to push parts of it yesterday. I forgot to cc you, just used the
get_maintainer script. I will add you to the cc list. Until then you can
look it up here: 20231218221814.69304-2-dima.fedrau@gmail.com

> Regards,
> Stefan

Regards,
Dimitri
  

On Tue, Dec 19, 2023 at 09:11:17AM +0100, Dimitri Fedrau wrote:

> I could add the init sequence for the 88Q2221 PHY. Then you could test
> it on your side. Would this be helpful to you ? Did you already have the
> chance to test the patch ?

Unfortunately I haven't had time to test it yet. I will try to do it on
Thursday, otherwise sadly it will be next year.

> You are right, but I would propose to stick to the reference init
> sequence and make sure the PHYs works with our code and then work on
> optimizing the code. We still can remove and/or document parts of it.

I am not sure that it will be accepted by the maintainers if you use a
lot of registers that are not documented. For this reason, keeping it to
a minimum might increase the chances of it being accepted.

> Are you trying with the patch I provided or your own code ? If you use
> my patch you should wait until V3, because I found some problems with
> it. Switching from 1000Mbit/s to 100Mbit/s in autonegotiation mode doesn't
> work. I could fix it but the fix touches some code already upstreamed. So
> I tried to push parts of it yesterday. I forgot to cc you, just used the
> get_maintainer script. I will add you to the cc list. Until then you can
> look it up here: 20231218221814.69304-2-dima.fedrau@gmail.com

I used my own code so far but I will try again with your patches. Maybe
send this and the other patches as a whole series so that it gets clear
why you need the changes as Andrew wrote.

Regards,
Stefan
  

Am Tue, Dec 19, 2023 at 10:19:41AM +0100 schrieb Stefan Eichenberger:
> On Tue, Dec 19, 2023 at 09:11:17AM +0100, Dimitri Fedrau wrote:
> 
> > I could add the init sequence for the 88Q2221 PHY. Then you could test
> > it on your side. Would this be helpful to you ? Did you already have the
> > chance to test the patch ?
> 
> Unfortunately I haven't had time to test it yet. I will try to do it on
> Thursday, otherwise sadly it will be next year.
>
Ok.

> > You are right, but I would propose to stick to the reference init
> > sequence and make sure the PHYs works with our code and then work on
> > optimizing the code. We still can remove and/or document parts of it.
> 
> I am not sure that it will be accepted by the maintainers if you use a
> lot of registers that are not documented. For this reason, keeping it to
> a minimum might increase the chances of it being accepted.
>
Ok. Will try to reduce them.

> > Are you trying with the patch I provided or your own code ? If you use
> > my patch you should wait until V3, because I found some problems with
> > it. Switching from 1000Mbit/s to 100Mbit/s in autonegotiation mode doesn't
> > work. I could fix it but the fix touches some code already upstreamed. So
> > I tried to push parts of it yesterday. I forgot to cc you, just used the
> > get_maintainer script. I will add you to the cc list. Until then you can
> > look it up here: 20231218221814.69304-2-dima.fedrau@gmail.com
> 
> I used my own code so far but I will try again with your patches. Maybe
> send this and the other patches as a whole series so that it gets clear
> why you need the changes as Andrew wrote.
> 
Ok. Will send an V3 including all patches.

> Regards,
> Stefan

Regards,
Dimitri
  

> I am not sure that it will be accepted by the maintainers if you use a
> lot of registers that are not documented.

Sometimes there is no choice, there is no documentation except the
vendor crap driver which we try to clean up as much as possible, but
we still end up with lots of magic numbers.

	Andrew
  

On Thu, Dec 21, 2023 at 08:28:47AM +0100, Dimitri Fedrau wrote:
> Changes in v2:
> 	- used defines MDIO_CTRL1_LPOWER and MDIO_PMA_CTRL1_SPEED1000
> 	  in mv88q222x_config_aneg_preinit
> 	- use genphy_c45_loopback
> 	- mv88q2xxx_read_status reads speed, master or slave state when
> 	  autonegotiation is enabled
> 	- added defines for magic values in mv88q222x_get_sqi

Please create a new thread for each version of the patch. The
automation does not like new versions appended onto old versions, so
this might not of been build tested.

     Andrew
  

Am Thu, Dec 21, 2023 at 10:44:39AM +0100 schrieb Andrew Lunn:
> On Thu, Dec 21, 2023 at 08:28:47AM +0100, Dimitri Fedrau wrote:
> > Changes in v2:
> > 	- used defines MDIO_CTRL1_LPOWER and MDIO_PMA_CTRL1_SPEED1000
> > 	  in mv88q222x_config_aneg_preinit
> > 	- use genphy_c45_loopback
> > 	- mv88q2xxx_read_status reads speed, master or slave state when
> > 	  autonegotiation is enabled
> > 	- added defines for magic values in mv88q222x_get_sqi
> 
> Please create a new thread for each version of the patch. The
> automation does not like new versions appended onto old versions, so
> this might not of been build tested.
>
Ok. Thanks for reviewing my code and your patience.
>      Andrew

Best regards,
Dimitri
  

Am Tue, Dec 19, 2023 at 04:57:50PM +0100 schrieb Andrew Lunn:
> > I am not sure that it will be accepted by the maintainers if you use a
> > lot of registers that are not documented.
> 
> Sometimes there is no choice, there is no documentation except the
> vendor crap driver which we try to clean up as much as possible, but
> we still end up with lots of magic numbers.
>

Hi Andrew, hi Stefan,

tried to reduce the init sequence. This worked for me:

static int mv88q222x_config_init(struct phy_device *phydev)
{
	int ret;

	/* send_s detection threshold, slave and master */
	ret = phy_write_mmd(phydev, MDIO_MMD_AN, 0x8032, 0x2020);
	if (ret < 0)
		return ret;

	ret = phy_write_mmd(phydev, MDIO_MMD_AN, 0x8031, 0xa28);
	if (ret < 0)
		return ret;

	ret = phy_write_mmd(phydev, MDIO_MMD_AN, 0x8031, 0xc28);
	if (ret < 0)
		return ret;

	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xffe4, 0xc);
	if (ret < 0)
		return ret;

	return mv88q2xxx_config_init(phydev);
}

The four register writes were required to make the PHY work in 1000Mbit forced
mode. When using autonegotiation or 100Mbit forced mode they weren't needed.
It was enough to write them once in mv88q222x_config_init as you can
see. Thanks Stefan for the hint with the first three register writes, it
helped a lot.

> 	Andrew

Best regards,
Dimitri
  

On Fri, Jan 05, 2024 at 01:42:21PM +0100, Dimitri Fedrau wrote:
> Am Tue, Dec 19, 2023 at 04:57:50PM +0100 schrieb Andrew Lunn:
> > > I am not sure that it will be accepted by the maintainers if you use a
> > > lot of registers that are not documented.
> > 
> > Sometimes there is no choice, there is no documentation except the
> > vendor crap driver which we try to clean up as much as possible, but
> > we still end up with lots of magic numbers.
> >
> 
> Hi Andrew, hi Stefan,
> 
> tried to reduce the init sequence. This worked for me:
> 
> static int mv88q222x_config_init(struct phy_device *phydev)
> {
> 	int ret;
> 
> 	/* send_s detection threshold, slave and master */
> 	ret = phy_write_mmd(phydev, MDIO_MMD_AN, 0x8032, 0x2020);
> 	if (ret < 0)
> 		return ret;
> 
> 	ret = phy_write_mmd(phydev, MDIO_MMD_AN, 0x8031, 0xa28);
> 	if (ret < 0)
> 		return ret;
> 
> 	ret = phy_write_mmd(phydev, MDIO_MMD_AN, 0x8031, 0xc28);
> 	if (ret < 0)
> 		return ret;
> 
> 	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xffe4, 0xc);
> 	if (ret < 0)
> 		return ret;
> 
> 	return mv88q2xxx_config_init(phydev);
> }
> 
> The four register writes were required to make the PHY work in 1000Mbit forced
> mode. When using autonegotiation or 100Mbit forced mode they weren't needed.
> It was enough to write them once in mv88q222x_config_init as you can
> see. Thanks Stefan for the hint with the first three register writes, it
> helped a lot.

Hi Dimitri

Do we need to reduce the init sequence? Since this is all undocumented
magic which nobody understands, it would be safer to just keep with
the Marvell vendor crap code dump. Unless we really do need to change
it.

	Andrew
  

Am Fri, Jan 05, 2024 at 03:00:58PM +0100 schrieb Andrew Lunn:
> On Fri, Jan 05, 2024 at 01:42:21PM +0100, Dimitri Fedrau wrote:
> > Am Tue, Dec 19, 2023 at 04:57:50PM +0100 schrieb Andrew Lunn:
> > > > I am not sure that it will be accepted by the maintainers if you use a
> > > > lot of registers that are not documented.
> > > 
> > > Sometimes there is no choice, there is no documentation except the
> > > vendor crap driver which we try to clean up as much as possible, but
> > > we still end up with lots of magic numbers.
> > >
> > 
> > Hi Andrew, hi Stefan,
> > 
> > tried to reduce the init sequence. This worked for me:
> > 
> > static int mv88q222x_config_init(struct phy_device *phydev)
> > {
> > 	int ret;
> > 
> > 	/* send_s detection threshold, slave and master */
> > 	ret = phy_write_mmd(phydev, MDIO_MMD_AN, 0x8032, 0x2020);
> > 	if (ret < 0)
> > 		return ret;
> > 
> > 	ret = phy_write_mmd(phydev, MDIO_MMD_AN, 0x8031, 0xa28);
> > 	if (ret < 0)
> > 		return ret;
> > 
> > 	ret = phy_write_mmd(phydev, MDIO_MMD_AN, 0x8031, 0xc28);
> > 	if (ret < 0)
> > 		return ret;
> > 
> > 	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xffe4, 0xc);
> > 	if (ret < 0)
> > 		return ret;
> > 
> > 	return mv88q2xxx_config_init(phydev);
> > }
> > 
> > The four register writes were required to make the PHY work in 1000Mbit forced
> > mode. When using autonegotiation or 100Mbit forced mode they weren't needed.
> > It was enough to write them once in mv88q222x_config_init as you can
> > see. Thanks Stefan for the hint with the first three register writes, it
> > helped a lot.
> 
> Hi Dimitri
> 
Hi Andrew,

> Do we need to reduce the init sequence? Since this is all undocumented
> magic which nobody understands, it would be safer to just keep with
> the Marvell vendor crap code dump. Unless we really do need to change
> it.
>
You are right, it would be safer to use the vendor code. But when
looking at the vendor code, the init sequence changed a lot from rev. B0
to rev. B1 of the PHY. There are some additional register writes, but
mostly the order of the register writes changed. I don't know if this is
going to be worse in the future. Maintaining different revisions will
probably take some effort or at least result in bloated code. We probably
don't need all of the init sequence. I'm not sure how to deal with it,
keeping the init sequence at a minimum is probably a good idea.

> 	Andrew

Best regards,
Dimitri
  

> Hi Andrew,
> 
> > Do we need to reduce the init sequence? Since this is all undocumented
> > magic which nobody understands, it would be safer to just keep with
> > the Marvell vendor crap code dump. Unless we really do need to change
> > it.
> >
> You are right, it would be safer to use the vendor code. But when
> looking at the vendor code, the init sequence changed a lot from rev. B0
> to rev. B1 of the PHY. There are some additional register writes, but
> mostly the order of the register writes changed. I don't know if this is
> going to be worse in the future. Maintaining different revisions will
> probably take some effort or at least result in bloated code. We probably
> don't need all of the init sequence. I'm not sure how to deal with it,
> keeping the init sequence at a minimum is probably a good idea.

Is the revision in the lower nibble of the ID register? We can handle
them as different PHYs, each gets its own init code, and share what
can be shared in helper functions.

	Andrew
  

Am Fri, Jan 05, 2024 at 05:06:53PM +0100 schrieb Andrew Lunn:
> > Hi Andrew,
> > 
> > > Do we need to reduce the init sequence? Since this is all undocumented
> > > magic which nobody understands, it would be safer to just keep with
> > > the Marvell vendor crap code dump. Unless we really do need to change
> > > it.
> > >
> > You are right, it would be safer to use the vendor code. But when
> > looking at the vendor code, the init sequence changed a lot from rev. B0
> > to rev. B1 of the PHY. There are some additional register writes, but
> > mostly the order of the register writes changed. I don't know if this is
> > going to be worse in the future. Maintaining different revisions will
> > probably take some effort or at least result in bloated code. We probably
> > don't need all of the init sequence. I'm not sure how to deal with it,
> > keeping the init sequence at a minimum is probably a good idea.
> 
> Is the revision in the lower nibble of the ID register? We can handle
> them as different PHYs, each gets its own init code, and share what
> can be shared in helper functions.
>
Yes, lowest four bits. Handling them as different PHYs would definitely
help maintaining PHY revisions. Still there is the problem with this
huge undocumented init sequence. Is this going to be accepted ? Didn't
see such a long undocumented init sequence in any other phy driver.

> 	Andrew

Best regards,
Dimitri