Re: [PATCH] net: phy: marvell-88q2xxx: add driver for the Marvell 88Q2220 PHY

[Stefan Eichenberger <eichest@gmail.com>]

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);