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

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

[Dimitri Fedrau]

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

diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
index 1c3ff77de56b..1b79f2ea5ed7 100644
--- a/drivers/net/phy/marvell-88q2xxx.c
+++ b/drivers/net/phy/marvell-88q2xxx.c
@@ -166,7 +166,9 @@ static int mv88q2xxx_get_features(struct phy_device *phydev)
 	 * sequence provided by Marvell. Disable it for now until a proper
 	 * workaround is found or a new PHY revision is released.
 	 */
-	linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, phydev->supported);
+	if (phydev->drv->phy_id == MARVELL_PHY_ID_88Q2110)
+		linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
+				   phydev->supported);
 
 	return 0;
 }
@@ -192,6 +194,9 @@ static int mv88q2xxx_config_init(struct phy_device *phydev)
 	 */
 	phydev->pma_extable = MDIO_PMA_EXTABLE_BT1;
 
+	if (phydev->drv->phy_id == MARVELL_PHY_ID_88Q2220)
+		return 0;
+
 	/* Read the current PHY configuration */
 	ret = genphy_c45_read_pma(phydev);
 	if (ret)
@@ -235,6 +240,242 @@ static int mv88q2xxxx_get_sqi_max(struct phy_device *phydev)
 	return 15;
 }
 
+static int mv88q222x_soft_reset(struct phy_device *phydev)
+{
+	int ret;
+
+	/* Enable RESET of DCL */
+	if (phydev->autoneg == AUTONEG_ENABLE || phydev->speed == SPEED_1000) {
+		ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe1b, 0x48);
+		if (ret < 0)
+			return ret;
+	}
+
+	/* Soft reset */
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_CTRL,
+			    MDIO_PCS_1000BT1_CTRL_RESET);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xffe4, 0xc);
+	if (ret < 0)
+		return ret;
+
+	/* Disable RESET of DCL */
+	if (phydev->autoneg == AUTONEG_ENABLE || phydev->speed == SPEED_1000)
+		return phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe1b, 0x58);
+	else
+		return ret;
+}
+
+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;
+
+	/* Disable DCL calibratin during tear down */
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xffdb, 0xfc10);
+	if (ret < 0)
+		return ret;
+
+	/* Disable RESET of DCL*/
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe1b, 0x58);
+	if (ret < 0)
+		return ret;
+
+	/* Turn CM Clamp ON */
+	return phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe79, 0x4);
+}
+
+static int mv88q222x_config_aneg_100m(struct phy_device *phydev)
+{
+	int ret;
+
+	/* Update Initial FFE Coefficients */
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfbba, 0xcb2);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfbbb, 0xc4a);
+	if (ret < 0)
+		return ret;
+
+	/* Turn CM Clamp ON */
+	return phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe79, 0x4);
+}
+
+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);
+	if (ret < 0)
+		return ret;
+
+	/* Exit standby state(internal state) */
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe1b, 0x48);
+	if (ret < 0)
+		return ret;
+
+	/* Set power management state breakpoint (internal state) */
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xffe4, 0x6b6);
+	if (ret < 0)
+		return ret;
+
+	/* Exit IEEE power down */
+	ret = phy_write_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1, 0x0);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1, 0x0);
+	if (ret < 0)
+		return ret;
+
+	/* Wait up to 5ms to enter to power management state, if we do not meet
+	 * the target value, it is still ok to proceed
+	 */
+	for (i = 0; i < 5; i++) {
+		val = phy_read_mmd(phydev, MDIO_MMD_PCS, 0xffe4);
+		if (val == 0x6ba)
+			break;
+
+		usleep_range(1000, 2000);
+	}
+
+	/* Turn CM Clamp OFF */
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe79, 0x0);
+	if (ret < 0)
+		return ret;
+
+	/* mdi vcm */
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe07, 0x125a);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe09, 0x1288);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe08, 0x2588);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe11, 0x1105);
+	if (ret < 0)
+		return ret;
+
+	/* aux_boost */
+	return phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe72, 0x042c);
+}
+
+static int mv88q222x_config_aneg_init_b0(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = mv88q222x_config_aneg_preinit(phydev);
+	if (ret < 0)
+		return ret;
+
+	if (phydev->autoneg == AUTONEG_DISABLE) {
+		if (phydev->speed == SPEED_100)
+			return mv88q222x_config_aneg_100m(phydev);
+		else
+			return mv88q222x_config_aneg_gbit(phydev);
+	}
+
+	ret = mv88q222x_config_aneg_100m(phydev);
+	if (ret)
+		return ret;
+
+	ret = mv88q222x_config_aneg_gbit(phydev);
+	if (ret)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe5f, 0xe8);
+	if (ret)
+		return ret;
+
+	return phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe05, 0x755c);
+}
+
+static int mv88q222x_config_aneg(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = mv88q222x_config_aneg_init_b0(phydev);
+	if (ret < 0)
+		return ret;
+
+	ret = genphy_c45_config_aneg(phydev);
+	if (ret)
+		return ret;
+
+	return mv88q222x_soft_reset(phydev);
+}
+
+static int mv88q222x_set_loopback(struct phy_device *phydev, bool enable)
+{
+	return phy_modify_mmd(phydev,
+			      MDIO_MMD_PCS,
+			      MDIO_PCS_1000BT1_CTRL,
+			      MDIO_PCS_CTRL1_LOOPBACK,
+			      enable ? MDIO_PCS_CTRL1_LOOPBACK : 0);
+}
+
+static int mv88q222x_get_sqi(struct phy_device *phydev)
+{
+	int ret;
+
+	if (phydev->speed == SPEED_100) {
+		/* Read the SQI from the vendor specific receiver status
+		 * register
+		 */
+		ret = phy_read_mmd(phydev, MDIO_MMD_PCS, 0x8230);
+		if (ret < 0)
+			return ret;
+
+		ret = (ret & 0xe0000) >> 13;
+	} else {
+		/* Read the SQI from the vendor specific signal quality index
+		 * register
+		 */
+
+		ret = phy_read_mmd(phydev, MDIO_MMD_PCS, 0xfcd8);
+		if (ret < 0)
+			return ret;
+	}
+
+	return ret & 0x0F;
+}
+
+static int mv88q222x_get_sqi_max(struct phy_device *phydev)
+{
+	return 7;
+}
+
 static struct phy_driver mv88q2xxx_driver[] = {
 	{
 		.phy_id			= MARVELL_PHY_ID_88Q2110,
@@ -249,12 +490,27 @@ static struct phy_driver mv88q2xxx_driver[] = {
 		.get_sqi		= mv88q2xxxx_get_sqi,
 		.get_sqi_max		= mv88q2xxxx_get_sqi_max,
 	},
+	{
+		.phy_id			= MARVELL_PHY_ID_88Q2220,
+		.phy_id_mask		= MARVELL_PHY_ID_MASK,
+		.name			= "mv88q2220",
+		.get_features		= mv88q2xxx_get_features,
+		.config_aneg		= mv88q222x_config_aneg,
+		.aneg_done		= genphy_c45_aneg_done,
+		.config_init		= mv88q2xxx_config_init,
+		.read_status		= mv88q2xxx_read_status,
+		.soft_reset		= mv88q222x_soft_reset,
+		.set_loopback		= mv88q222x_set_loopback,
+		.get_sqi		= mv88q222x_get_sqi,
+		.get_sqi_max		= mv88q222x_get_sqi_max,
+	},
 };
 
 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_88Q2220, MARVELL_PHY_ID_MASK },
 	{ /*sentinel*/ }
 };
 MODULE_DEVICE_TABLE(mdio, mv88q2xxx_tbl);
diff --git a/include/linux/marvell_phy.h b/include/linux/marvell_phy.h
index 9b54c4f0677f..693eba9869e4 100644
--- a/include/linux/marvell_phy.h
+++ b/include/linux/marvell_phy.h
@@ -26,6 +26,7 @@
 #define MARVELL_PHY_ID_88E2110		0x002b09b0
 #define MARVELL_PHY_ID_88X2222		0x01410f10
 #define MARVELL_PHY_ID_88Q2110		0x002b0980
+#define MARVELL_PHY_ID_88Q2220		0x002b0b20
 
 /* Marvel 88E1111 in Finisar SFP module with modified PHY ID */
 #define MARVELL_PHY_ID_88E1111_FINISAR	0x01ff0cc0
-- 
2.39.2

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

[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?

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

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

[Dimitri Fedrau]

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

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

[Dimitri Fedrau]

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

 drivers/net/phy/marvell-88q2xxx.c | 290 +++++++++++++++++++++++++++++-
 include/linux/marvell_phy.h       |   1 +
 2 files changed, 289 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
index 1c3ff77de56b..722a62509247 100644
--- a/drivers/net/phy/marvell-88q2xxx.c
+++ b/drivers/net/phy/marvell-88q2xxx.c
@@ -27,6 +27,10 @@
 #define MDIO_MMD_PCS_MV_100BT1_STAT2_LINK	0x0004
 #define MDIO_MMD_PCS_MV_100BT1_STAT2_ANGE	0x0008
 
+#define MDIO_MMD_PCS_MV_RX_STAT			33328
+
+#define MDIO_MMD_PCS_MV_SQI			64728
+
 static int mv88q2xxx_soft_reset(struct phy_device *phydev)
 {
 	int ret;
@@ -134,6 +138,22 @@ static int mv88q2xxx_read_link(struct phy_device *phydev)
 	return ret;
 }
 
+static int mv88q2xxx_read_master_slave_state(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_MMD_AN_MV_STAT);
+	if (ret < 0)
+		return ret;
+
+	if (ret & MDIO_MMD_AN_MV_STAT_LOCAL_MASTER)
+		phydev->master_slave_state = MASTER_SLAVE_STATE_MASTER;
+	else
+		phydev->master_slave_state = MASTER_SLAVE_STATE_SLAVE;
+
+	return 0;
+}
+
 static int mv88q2xxx_read_status(struct phy_device *phydev)
 {
 	int ret;
@@ -142,7 +162,25 @@ static int mv88q2xxx_read_status(struct phy_device *phydev)
 	if (ret < 0)
 		return ret;
 
-	return genphy_c45_read_pma(phydev);
+	if (phydev->autoneg == AUTONEG_ENABLE) {
+		ret = genphy_c45_read_lpa(phydev);
+		if (ret < 0)
+			return ret;
+
+		ret = genphy_c45_baset1_read_status(phydev);
+		if (ret < 0)
+			return ret;
+
+		ret = mv88q2xxx_read_master_slave_state(phydev);
+		if (ret < 0)
+			return ret;
+
+		phy_resolve_aneg_linkmode(phydev);
+
+		return 0;
+	} else {
+		return genphy_c45_read_pma(phydev);
+	}
 }
 
 static int mv88q2xxx_get_features(struct phy_device *phydev)
@@ -166,7 +204,9 @@ static int mv88q2xxx_get_features(struct phy_device *phydev)
 	 * sequence provided by Marvell. Disable it for now until a proper
 	 * workaround is found or a new PHY revision is released.
 	 */
-	linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, phydev->supported);
+	if (phydev->drv->phy_id == MARVELL_PHY_ID_88Q2110)
+		linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
+				   phydev->supported);
 
 	return 0;
 }
@@ -192,6 +232,9 @@ static int mv88q2xxx_config_init(struct phy_device *phydev)
 	 */
 	phydev->pma_extable = MDIO_PMA_EXTABLE_BT1;
 
+	if (phydev->drv->phy_id == MARVELL_PHY_ID_88Q2220)
+		return 0;
+
 	/* Read the current PHY configuration */
 	ret = genphy_c45_read_pma(phydev);
 	if (ret)
@@ -235,6 +278,234 @@ static int mv88q2xxxx_get_sqi_max(struct phy_device *phydev)
 	return 15;
 }
 
+static int mv88q222x_soft_reset(struct phy_device *phydev)
+{
+	int ret;
+
+	/* Enable RESET of DCL */
+	if (phydev->autoneg == AUTONEG_ENABLE || phydev->speed == SPEED_1000) {
+		ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe1b, 0x48);
+		if (ret < 0)
+			return ret;
+	}
+
+	/* Soft reset */
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_CTRL,
+			    MDIO_PCS_1000BT1_CTRL_RESET);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xffe4, 0xc);
+	if (ret < 0)
+		return ret;
+
+	/* Disable RESET of DCL */
+	if (phydev->autoneg == AUTONEG_ENABLE || phydev->speed == SPEED_1000)
+		return phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe1b, 0x58);
+	else
+		return ret;
+}
+
+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;
+
+	/* Disable DCL calibratin during tear down */
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xffdb, 0xfc10);
+	if (ret < 0)
+		return ret;
+
+	/* Disable RESET of DCL*/
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe1b, 0x58);
+	if (ret < 0)
+		return ret;
+
+	/* Turn CM Clamp ON */
+	return phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe79, 0x4);
+}
+
+static int mv88q222x_config_aneg_100m(struct phy_device *phydev)
+{
+	int ret;
+
+	/* Update Initial FFE Coefficients */
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfbba, 0xcb2);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfbbb, 0xc4a);
+	if (ret < 0)
+		return ret;
+
+	/* Turn CM Clamp ON */
+	return phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe79, 0x4);
+}
+
+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,
+			    MDIO_CTRL1_LPOWER | MDIO_PMA_CTRL1_SPEED1000);
+	if (ret < 0)
+		return ret;
+
+	/* Exit standby state(internal state) */
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe1b, 0x48);
+	if (ret < 0)
+		return ret;
+
+	/* Set power management state breakpoint (internal state) */
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xffe4, 0x6b6);
+	if (ret < 0)
+		return ret;
+
+	/* Exit IEEE power down */
+	ret = phy_write_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1, 0x0);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1, 0x0);
+	if (ret < 0)
+		return ret;
+
+	/* Wait up to 5ms to enter to power management state, if we do not meet
+	 * the target value, it is still ok to proceed
+	 */
+	for (i = 0; i < 5; i++) {
+		val = phy_read_mmd(phydev, MDIO_MMD_PCS, 0xffe4);
+		if (val == 0x6ba)
+			break;
+
+		usleep_range(1000, 2000);
+	}
+
+	/* Turn CM Clamp OFF */
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe79, 0x0);
+	if (ret < 0)
+		return ret;
+
+	/* mdi vcm */
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe07, 0x125a);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe09, 0x1288);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe08, 0x2588);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe11, 0x1105);
+	if (ret < 0)
+		return ret;
+
+	/* aux_boost */
+	return phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe72, 0x042c);
+}
+
+static int mv88q222x_config_aneg_init_b0(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = mv88q222x_config_aneg_preinit(phydev);
+	if (ret < 0)
+		return ret;
+
+	if (phydev->autoneg == AUTONEG_DISABLE) {
+		if (phydev->speed == SPEED_100)
+			return mv88q222x_config_aneg_100m(phydev);
+		else
+			return mv88q222x_config_aneg_gbit(phydev);
+	}
+
+	ret = mv88q222x_config_aneg_100m(phydev);
+	if (ret)
+		return ret;
+
+	ret = mv88q222x_config_aneg_gbit(phydev);
+	if (ret)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe5f, 0xe8);
+	if (ret)
+		return ret;
+
+	return phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe05, 0x755c);
+}
+
+static int mv88q222x_config_aneg(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = mv88q222x_config_aneg_init_b0(phydev);
+	if (ret < 0)
+		return ret;
+
+	ret = genphy_c45_config_aneg(phydev);
+	if (ret)
+		return ret;
+
+	return mv88q222x_soft_reset(phydev);
+}
+
+static int mv88q222x_get_sqi(struct phy_device *phydev)
+{
+	int ret;
+
+	if (phydev->speed == SPEED_100) {
+		/* Read the SQI from the vendor specific receiver status
+		 * register
+		 */
+		ret = phy_read_mmd(phydev, MDIO_MMD_PCS,
+				   MDIO_MMD_PCS_MV_RX_STAT);
+		if (ret < 0)
+			return ret;
+
+		ret = (ret & 0xe0000) >> 13;
+	} else {
+		/* Read the SQI from the vendor specific signal quality index
+		 * register
+		 */
+		ret = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_MMD_PCS_MV_SQI);
+		if (ret < 0)
+			return ret;
+	}
+
+	return ret & 0x0f;
+}
+
+static int mv88q222x_get_sqi_max(struct phy_device *phydev)
+{
+	return 7;
+}
+
 static struct phy_driver mv88q2xxx_driver[] = {
 	{
 		.phy_id			= MARVELL_PHY_ID_88Q2110,
@@ -249,12 +520,27 @@ static struct phy_driver mv88q2xxx_driver[] = {
 		.get_sqi		= mv88q2xxxx_get_sqi,
 		.get_sqi_max		= mv88q2xxxx_get_sqi_max,
 	},
+	{
+		.phy_id			= MARVELL_PHY_ID_88Q2220,
+		.phy_id_mask		= MARVELL_PHY_ID_MASK,
+		.name			= "mv88q2220",
+		.get_features		= mv88q2xxx_get_features,
+		.config_aneg		= mv88q222x_config_aneg,
+		.aneg_done		= genphy_c45_aneg_done,
+		.config_init		= mv88q2xxx_config_init,
+		.read_status		= mv88q2xxx_read_status,
+		.soft_reset		= mv88q222x_soft_reset,
+		.set_loopback		= genphy_c45_loopback,
+		.get_sqi		= mv88q222x_get_sqi,
+		.get_sqi_max		= mv88q222x_get_sqi_max,
+	},
 };
 
 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_88Q2220, MARVELL_PHY_ID_MASK },
 	{ /*sentinel*/ }
 };
 MODULE_DEVICE_TABLE(mdio, mv88q2xxx_tbl);
diff --git a/include/linux/marvell_phy.h b/include/linux/marvell_phy.h
index 9b54c4f0677f..693eba9869e4 100644
--- a/include/linux/marvell_phy.h
+++ b/include/linux/marvell_phy.h
@@ -26,6 +26,7 @@
 #define MARVELL_PHY_ID_88E2110		0x002b09b0
 #define MARVELL_PHY_ID_88X2222		0x01410f10
 #define MARVELL_PHY_ID_88Q2110		0x002b0980
+#define MARVELL_PHY_ID_88Q2220		0x002b0b20
 
 /* Marvel 88E1111 in Finisar SFP module with modified PHY ID */
 #define MARVELL_PHY_ID_88E1111_FINISAR	0x01ff0cc0
-- 
2.39.2

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

[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?

	Andrew

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

[Dimitri Fedrau]

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

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

[Stefan Eichenberger]

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

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

[Dimitri Fedrau]

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

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

[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 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

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

[Dimitri Fedrau]

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

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

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

> 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

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

[Dimitri Fedrau]

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

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

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

	Andrew

[PATCH v3 0/4] net: phy: marvell-88q2xxx: add driver for the Marvell 88Q2220 PHY

[Dimitri Fedrau]

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

Changes in v3:
	- mv88q2xxx_read_status includes autonegotiation case
	- add support for 100BT1 and 1000BT1 linkmode advertisement
	- use mv88q2xxx_get_sqi and mv88q2xxx_get_sqi_max, remove
	  mv88q222x_get_sqi and mv88q222x_get_sqi_max
	- fix typo: rename mv88q2xxxx_get_sqi and mv88q2xxxx_get_sqi_max to
	  mv88q2xxx_get_sqi and mv88q2xxx_get_sqi
	- add define MDIO_MMD_PCS_MV_RX_STAT for magic value 0x8230, documented
	  in latest datasheets for both PHYs

Dimitri Fedrau (4):
  net: phy: Add BaseT1 auto-negotiation constants
  net: phy: Support 100/1000BT1 linkmode advertisements
  net: phy: c45: detect 100/1000BASE-T1 linkmode advertisements
  net: phy: marvell-88q2xxx: add driver for the Marvell 88Q2220 PHY

 drivers/net/phy/marvell-88q2xxx.c | 310 ++++++++++++++++++++++++++++--
 drivers/net/phy/phy-c45.c         |   3 +-
 include/linux/marvell_phy.h       |   1 +
 include/linux/mdio.h              |   8 +
 include/uapi/linux/mdio.h         |   2 +
 5 files changed, 312 insertions(+), 12 deletions(-)

-- 
2.39.2

[PATCH v3 1/4] net: phy: Add BaseT1 auto-negotiation constants

[Dimitri Fedrau]

Added constants for advertising 100BT1 and 1000BT1 in register BASE-T1
auto-negotiation advertisement register [31:16] (Register 7.515)

Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
---
 include/uapi/linux/mdio.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/uapi/linux/mdio.h b/include/uapi/linux/mdio.h
index d03863da180e..020ccc810d23 100644
--- a/include/uapi/linux/mdio.h
+++ b/include/uapi/linux/mdio.h
@@ -348,6 +348,8 @@
 
 /* BASE-T1 auto-negotiation advertisement register [31:16] */
 #define MDIO_AN_T1_ADV_M_B10L		0x4000	/* device is compatible with 10BASE-T1L */
+#define MDIO_AN_T1_ADV_M_1000BT1	0x0080	/* advertise 1000BASE-T1 */
+#define MDIO_AN_T1_ADV_M_100BT1		0x0020	/* advertise 100BASE-T1 */
 #define MDIO_AN_T1_ADV_M_MST		0x0010	/* advertise master preference */
 
 /* BASE-T1 auto-negotiation advertisement register [47:32] */
-- 
2.39.2

[PATCH v3 2/4] net: phy: Support 100/1000BT1 linkmode advertisements

[Dimitri Fedrau]

Extend helper functions mii_t1_adv_m_mod_linkmode_t and
linkmode_adv_to_mii_t1_adv_m_t to support 100BT1 and 1000BT1 linkmode
advertisements.

Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
---
 include/linux/mdio.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index 007fd9c3e4b6..3d177f265cd3 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -372,6 +372,10 @@ static inline void mii_t1_adv_m_mod_linkmode_t(unsigned long *advertising, u32 l
 {
 	linkmode_mod_bit(ETHTOOL_LINK_MODE_10baseT1L_Full_BIT,
 			 advertising, lpa & MDIO_AN_T1_ADV_M_B10L);
+	linkmode_mod_bit(ETHTOOL_LINK_MODE_100baseT1_Full_BIT,
+			 advertising, lpa & MDIO_AN_T1_ADV_M_100BT1);
+	linkmode_mod_bit(ETHTOOL_LINK_MODE_1000baseT1_Full_BIT,
+			 advertising, lpa & MDIO_AN_T1_ADV_M_1000BT1);
 }
 
 /**
@@ -408,6 +412,10 @@ static inline u32 linkmode_adv_to_mii_t1_adv_m_t(unsigned long *advertising)
 
 	if (linkmode_test_bit(ETHTOOL_LINK_MODE_10baseT1L_Full_BIT, advertising))
 		result |= MDIO_AN_T1_ADV_M_B10L;
+	if (linkmode_test_bit(ETHTOOL_LINK_MODE_100baseT1_Full_BIT, advertising))
+		result |= MDIO_AN_T1_ADV_M_100BT1;
+	if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseT1_Full_BIT, advertising))
+		result |= MDIO_AN_T1_ADV_M_1000BT1;
 
 	return result;
 }
-- 
2.39.2

[PATCH v3 3/4] net: phy: c45: detect 100/1000BASE-T1 linkmode advertisements

[Dimitri Fedrau]

Set 100BT1 and 1000BT1 linkmode advertisement bits to adv_l_mask to
enable detection.

Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
---
 drivers/net/phy/phy-c45.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index 8e6fd4962c48..81ee2915bfc7 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -208,7 +208,8 @@ static int genphy_c45_baset1_an_config_aneg(struct phy_device *phydev)
 
 	adv_l_mask = MDIO_AN_T1_ADV_L_FORCE_MS | MDIO_AN_T1_ADV_L_PAUSE_CAP |
 		MDIO_AN_T1_ADV_L_PAUSE_ASYM;
-	adv_m_mask = MDIO_AN_T1_ADV_M_MST | MDIO_AN_T1_ADV_M_B10L;
+	adv_m_mask = MDIO_AN_T1_ADV_M_1000BT1 | MDIO_AN_T1_ADV_M_100BT1 |
+		MDIO_AN_T1_ADV_M_MST | MDIO_AN_T1_ADV_M_B10L;
 
 	switch (phydev->master_slave_set) {
 	case MASTER_SLAVE_CFG_MASTER_FORCE:
-- 
2.39.2

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

[Dimitri Fedrau]

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 | 310 ++++++++++++++++++++++++++++--
 include/linux/marvell_phy.h       |   1 +
 2 files changed, 300 insertions(+), 11 deletions(-)

diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
index 1c3ff77de56b..4e930b5ffaef 100644
--- a/drivers/net/phy/marvell-88q2xxx.c
+++ b/drivers/net/phy/marvell-88q2xxx.c
@@ -27,6 +27,13 @@
 #define MDIO_MMD_PCS_MV_100BT1_STAT2_LINK	0x0004
 #define MDIO_MMD_PCS_MV_100BT1_STAT2_ANGE	0x0008
 
+#define MDIO_MMD_PCS_MV_RX_STAT			33328
+
+#define MDIO_MMD_AN_MV_STAT2			32794
+#define MDIO_MMD_AN_MV_STAT2_AN_RESOLVED	0x0800
+#define MDIO_MMD_AN_MV_STAT2_100BT1		0x2000
+#define MDIO_MMD_AN_MV_STAT2_1000BT1		0x4000
+
 static int mv88q2xxx_soft_reset(struct phy_device *phydev)
 {
 	int ret;
@@ -120,24 +127,87 @@ static int mv88q2xxx_read_link_100m(struct phy_device *phydev)
 
 static int mv88q2xxx_read_link(struct phy_device *phydev)
 {
-	int ret;
-
 	/* The 88Q2XXX PHYs do not have the PMA/PMD status register available,
 	 * therefore we need to read the link status from the vendor specific
 	 * registers depending on the speed.
 	 */
+
 	if (phydev->speed == SPEED_1000)
-		ret = mv88q2xxx_read_link_gbit(phydev);
+		return mv88q2xxx_read_link_gbit(phydev);
+	else if (phydev->speed == SPEED_100)
+		return mv88q2xxx_read_link_100m(phydev);
+
+	phydev->link = false;
+	return 0;
+}
+
+static int mv88q2xxx_read_master_slave_state(struct phy_device *phydev)
+{
+	int ret;
+
+	phydev->master_slave_state = MASTER_SLAVE_STATE_UNKNOWN;
+	ret = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_MMD_AN_MV_STAT);
+	if (ret < 0)
+		return ret;
+
+	if (ret & MDIO_MMD_AN_MV_STAT_LOCAL_MASTER)
+		phydev->master_slave_state = MASTER_SLAVE_STATE_MASTER;
 	else
-		ret = mv88q2xxx_read_link_100m(phydev);
+		phydev->master_slave_state = MASTER_SLAVE_STATE_SLAVE;
+
+	return 0;
+}
+
+static int mv88q2xxx_read_aneg_speed(struct phy_device *phydev)
+{
+	int ret;
 
-	return ret;
+	phydev->speed = SPEED_UNKNOWN;
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_MMD_AN_MV_STAT2);
+	if (ret < 0)
+		return ret;
+
+	if (!(ret & MDIO_MMD_AN_MV_STAT2_AN_RESOLVED))
+		return 0;
+
+	if (ret & MDIO_MMD_AN_MV_STAT2_100BT1)
+		phydev->speed = SPEED_100;
+	else if (ret & MDIO_MMD_AN_MV_STAT2_1000BT1)
+		phydev->speed = SPEED_1000;
+
+	return 0;
 }
 
 static int mv88q2xxx_read_status(struct phy_device *phydev)
 {
 	int ret;
 
+	if (phydev->autoneg == AUTONEG_ENABLE) {
+		phydev->duplex = DUPLEX_FULL;
+
+		/* We have to get the negotiated speed first, otherwise we are
+		 * not able to read the link.
+		 */
+		ret = mv88q2xxx_read_aneg_speed(phydev);
+		if (ret < 0)
+			return ret;
+
+		ret = mv88q2xxx_read_link(phydev);
+		if (ret < 0)
+			return ret;
+
+		ret = genphy_c45_read_lpa(phydev);
+		if (ret < 0)
+			return ret;
+
+		ret = genphy_c45_baset1_read_status(phydev);
+		if (ret < 0)
+			return ret;
+
+		return mv88q2xxx_read_master_slave_state(phydev);
+	}
+
 	ret = mv88q2xxx_read_link(phydev);
 	if (ret < 0)
 		return ret;
@@ -166,7 +236,9 @@ static int mv88q2xxx_get_features(struct phy_device *phydev)
 	 * sequence provided by Marvell. Disable it for now until a proper
 	 * workaround is found or a new PHY revision is released.
 	 */
-	linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, phydev->supported);
+	if (phydev->drv->phy_id == MARVELL_PHY_ID_88Q2110)
+		linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
+				   phydev->supported);
 
 	return 0;
 }
@@ -192,6 +264,9 @@ static int mv88q2xxx_config_init(struct phy_device *phydev)
 	 */
 	phydev->pma_extable = MDIO_PMA_EXTABLE_BT1;
 
+	if (phydev->drv->phy_id == MARVELL_PHY_ID_88Q2220)
+		return 0;
+
 	/* Read the current PHY configuration */
 	ret = genphy_c45_read_pma(phydev);
 	if (ret)
@@ -200,7 +275,7 @@ static int mv88q2xxx_config_init(struct phy_device *phydev)
 	return mv88q2xxx_config_aneg(phydev);
 }
 
-static int mv88q2xxxx_get_sqi(struct phy_device *phydev)
+static int mv88q2xxx_get_sqi(struct phy_device *phydev)
 {
 	int ret;
 
@@ -208,7 +283,8 @@ static int mv88q2xxxx_get_sqi(struct phy_device *phydev)
 		/* Read the SQI from the vendor specific receiver status
 		 * register
 		 */
-		ret = phy_read_mmd(phydev, MDIO_MMD_PCS, 0x8230);
+		ret = phy_read_mmd(phydev, MDIO_MMD_PCS,
+				   MDIO_MMD_PCS_MV_RX_STAT);
 		if (ret < 0)
 			return ret;
 
@@ -230,11 +306,208 @@ static int mv88q2xxxx_get_sqi(struct phy_device *phydev)
 	return ret & 0x0F;
 }
 
-static int mv88q2xxxx_get_sqi_max(struct phy_device *phydev)
+static int mv88q2xxx_get_sqi_max(struct phy_device *phydev)
 {
 	return 15;
 }
 
+static int mv88q222x_soft_reset(struct phy_device *phydev)
+{
+	int ret;
+
+	/* Enable RESET of DCL */
+	if (phydev->autoneg == AUTONEG_ENABLE || phydev->speed == SPEED_1000) {
+		ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe1b, 0x48);
+		if (ret < 0)
+			return ret;
+	}
+
+	/* Soft reset */
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_CTRL,
+			    MDIO_PCS_1000BT1_CTRL_RESET);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xffe4, 0xc);
+	if (ret < 0)
+		return ret;
+
+	/* Disable RESET of DCL */
+	if (phydev->autoneg == AUTONEG_ENABLE || phydev->speed == SPEED_1000)
+		return phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe1b, 0x58);
+	else
+		return ret;
+}
+
+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;
+
+	/* Disable DCL calibratin during tear down */
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xffdb, 0xfc10);
+	if (ret < 0)
+		return ret;
+
+	/* Disable RESET of DCL*/
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe1b, 0x58);
+	if (ret < 0)
+		return ret;
+
+	/* Turn CM Clamp ON */
+	return phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe79, 0x4);
+}
+
+static int mv88q222x_config_aneg_100m(struct phy_device *phydev)
+{
+	int ret;
+
+	/* Update Initial FFE Coefficients */
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfbba, 0xcb2);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfbbb, 0xc4a);
+	if (ret < 0)
+		return ret;
+
+	/* Turn CM Clamp ON */
+	return phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe79, 0x4);
+}
+
+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,
+			    MDIO_CTRL1_LPOWER | MDIO_PMA_CTRL1_SPEED1000);
+	if (ret < 0)
+		return ret;
+
+	/* Exit standby state(internal state) */
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe1b, 0x48);
+	if (ret < 0)
+		return ret;
+
+	/* Set power management state breakpoint (internal state) */
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xffe4, 0x6b6);
+	if (ret < 0)
+		return ret;
+
+	/* Exit IEEE power down */
+	ret = phy_write_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1, 0x0);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1, 0x0);
+	if (ret < 0)
+		return ret;
+
+	/* Wait up to 5ms to enter to power management state, if we do not meet
+	 * the target value, it is still ok to proceed
+	 */
+	for (i = 0; i < 5; i++) {
+		val = phy_read_mmd(phydev, MDIO_MMD_PCS, 0xffe4);
+		if (val == 0x6ba)
+			break;
+
+		usleep_range(1000, 2000);
+	}
+
+	/* Turn CM Clamp OFF */
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe79, 0x0);
+	if (ret < 0)
+		return ret;
+
+	/* mdi vcm */
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe07, 0x125a);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe09, 0x1288);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe08, 0x2588);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe11, 0x1105);
+	if (ret < 0)
+		return ret;
+
+	/* aux_boost */
+	return phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe72, 0x042c);
+}
+
+static int mv88q222x_config_aneg_init_b0(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = mv88q222x_config_aneg_preinit(phydev);
+	if (ret < 0)
+		return ret;
+
+	if (phydev->autoneg == AUTONEG_DISABLE) {
+		if (phydev->speed == SPEED_100)
+			return mv88q222x_config_aneg_100m(phydev);
+		else
+			return mv88q222x_config_aneg_gbit(phydev);
+	}
+
+	ret = mv88q222x_config_aneg_100m(phydev);
+	if (ret)
+		return ret;
+
+	ret = mv88q222x_config_aneg_gbit(phydev);
+	if (ret)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe5f, 0xe8);
+	if (ret)
+		return ret;
+
+	return phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe05, 0x755c);
+}
+
+static int mv88q222x_config_aneg(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = mv88q222x_config_aneg_init_b0(phydev);
+	if (ret < 0)
+		return ret;
+
+	ret = genphy_c45_config_aneg(phydev);
+	if (ret)
+		return ret;
+
+	return mv88q222x_soft_reset(phydev);
+}
+
 static struct phy_driver mv88q2xxx_driver[] = {
 	{
 		.phy_id			= MARVELL_PHY_ID_88Q2110,
@@ -246,8 +519,22 @@ static struct phy_driver mv88q2xxx_driver[] = {
 		.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,
+		.get_sqi		= mv88q2xxx_get_sqi,
+		.get_sqi_max		= mv88q2xxx_get_sqi_max,
+	},
+	{
+		.phy_id			= MARVELL_PHY_ID_88Q2220,
+		.phy_id_mask		= MARVELL_PHY_ID_MASK,
+		.name			= "mv88q2220",
+		.get_features		= mv88q2xxx_get_features,
+		.config_aneg		= mv88q222x_config_aneg,
+		.aneg_done		= genphy_c45_aneg_done,
+		.config_init		= mv88q2xxx_config_init,
+		.read_status		= mv88q2xxx_read_status,
+		.soft_reset		= mv88q222x_soft_reset,
+		.set_loopback		= genphy_c45_loopback,
+		.get_sqi		= mv88q2xxx_get_sqi,
+		.get_sqi_max		= mv88q2xxx_get_sqi_max,
 	},
 };
 
@@ -255,6 +542,7 @@ 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_88Q2220, MARVELL_PHY_ID_MASK },
 	{ /*sentinel*/ }
 };
 MODULE_DEVICE_TABLE(mdio, mv88q2xxx_tbl);
diff --git a/include/linux/marvell_phy.h b/include/linux/marvell_phy.h
index 9b54c4f0677f..693eba9869e4 100644
--- a/include/linux/marvell_phy.h
+++ b/include/linux/marvell_phy.h
@@ -26,6 +26,7 @@
 #define MARVELL_PHY_ID_88E2110		0x002b09b0
 #define MARVELL_PHY_ID_88X2222		0x01410f10
 #define MARVELL_PHY_ID_88Q2110		0x002b0980
+#define MARVELL_PHY_ID_88Q2220		0x002b0b20
 
 /* Marvel 88E1111 in Finisar SFP module with modified PHY ID */
 #define MARVELL_PHY_ID_88E1111_FINISAR	0x01ff0cc0
-- 
2.39.2

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

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

     Andrew

Re: [PATCH v3 1/4] net: phy: Add BaseT1 auto-negotiation constants

[Andrew Lunn]

On Thu, Dec 21, 2023 at 08:28:48AM +0100, Dimitri Fedrau wrote:
> Added constants for advertising 100BT1 and 1000BT1 in register BASE-T1
> auto-negotiation advertisement register [31:16] (Register 7.515)
> 
> Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v3 2/4] net: phy: Support 100/1000BT1 linkmode advertisements

[Andrew Lunn]

On Thu, Dec 21, 2023 at 08:28:49AM +0100, Dimitri Fedrau wrote:
> Extend helper functions mii_t1_adv_m_mod_linkmode_t and
> linkmode_adv_to_mii_t1_adv_m_t to support 100BT1 and 1000BT1 linkmode
> advertisements.
> 
> Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v3 3/4] net: phy: c45: detect 100/1000BASE-T1 linkmode advertisements

[Andrew Lunn]

On Thu, Dec 21, 2023 at 08:28:50AM +0100, Dimitri Fedrau wrote:
> Set 100BT1 and 1000BT1 linkmode advertisement bits to adv_l_mask to
> enable detection.
> 
> Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

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

[Andrew Lunn]

> -static int mv88q2xxxx_get_sqi(struct phy_device *phydev)
> +static int mv88q2xxx_get_sqi(struct phy_device *phydev)
>  {
>  	int ret;
>  
> @@ -208,7 +283,8 @@ static int mv88q2xxxx_get_sqi(struct phy_device *phydev)
>  		/* Read the SQI from the vendor specific receiver status
>  		 * register
>  		 */
> -		ret = phy_read_mmd(phydev, MDIO_MMD_PCS, 0x8230);
> +		ret = phy_read_mmd(phydev, MDIO_MMD_PCS,
> +				   MDIO_MMD_PCS_MV_RX_STAT);
>  		if (ret < 0)
>  			return ret;
>  
> @@ -230,11 +306,208 @@ static int mv88q2xxxx_get_sqi(struct phy_device *phydev)
>  	return ret & 0x0F;
>  }
>  
> -static int mv88q2xxxx_get_sqi_max(struct phy_device *phydev)
> +static int mv88q2xxx_get_sqi_max(struct phy_device *phydev)
>  {
>  	return 15;
>  }

This could be a patch of its own.

     Andrew

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

[Dimitri Fedrau]

Am Thu, Dec 21, 2023 at 10:53:27AM +0100 schrieb Andrew Lunn:
> > -static int mv88q2xxxx_get_sqi(struct phy_device *phydev)
> > +static int mv88q2xxx_get_sqi(struct phy_device *phydev)
> >  {
> >  	int ret;
> >  
> > @@ -208,7 +283,8 @@ static int mv88q2xxxx_get_sqi(struct phy_device *phydev)
> >  		/* Read the SQI from the vendor specific receiver status
> >  		 * register
> >  		 */
> > -		ret = phy_read_mmd(phydev, MDIO_MMD_PCS, 0x8230);
> > +		ret = phy_read_mmd(phydev, MDIO_MMD_PCS,
> > +				   MDIO_MMD_PCS_MV_RX_STAT);
> >  		if (ret < 0)
> >  			return ret;
> >  
> > @@ -230,11 +306,208 @@ static int mv88q2xxxx_get_sqi(struct phy_device *phydev)
> >  	return ret & 0x0F;
> >  }
> >  
> > -static int mv88q2xxxx_get_sqi_max(struct phy_device *phydev)
> > +static int mv88q2xxx_get_sqi_max(struct phy_device *phydev)
> >  {
> >  	return 15;
> >  }
> 
> This could be a patch of its own.
>
Will fix this in V4.

>      Andrew

Best regards,
Dimitri

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

[Dimitri Fedrau]

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

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

[Stefan Eichenberger]

Hi Dimitri,

On Thu, Dec 21, 2023 at 08:28:51AM +0100, Dimitri Fedrau wrote:
> 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>

I tried to make your patch work in my setup but I'm unable to force a
link speed. Were you able to force a different link speed with the
following command?
ethtool -s eth0 speed 100 autoneg off

Regards,
Stefan

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

[Dimitri Fedrau]

Am Thu, Dec 21, 2023 at 02:47:57PM +0100 schrieb Stefan Eichenberger:
> Hi Dimitri,
>
Hi Stefan,

> On Thu, Dec 21, 2023 at 08:28:51AM +0100, Dimitri Fedrau wrote:
> > 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>
> 
> I tried to make your patch work in my setup but I'm unable to force a
> link speed. Were you able to force a different link speed with the
> following command?
> ethtool -s eth0 speed 100 autoneg off
>

I tested following modes, which all worked:

ethtool -s eth0 autoneg on master-slave forced-master
ethtool -s eth0 autoneg on master-slave preferred-master
ethtool -s eth0 autoneg on master-slave preferred-slave
ethtool -s eth0 autoneg on master-slave forced-slave

ethtool -s eth0 autoneg on master-slave forced-master speed 100
ethtool -s eth0 autoneg on master-slave preferred-master speed 100
ethtool -s eth0 autoneg on master-slave preferred-slave	speed 100
ethtool -s eth0 autoneg on master-slave forced-slave speed 100

ethtool -s eth0 autoneg off master-slave forced-master speed 1000
ethtool -s eth0 autoneg off master-slave preferred-master speed 1000
ethtool -s eth0 autoneg off master-slave preferred-slave speed 1000
ethtool -s eth0 autoneg off master-slave forced-slave speed 1000

ethtool -s eth0 autoneg off master-slave forced-master speed 100
ethtool -s eth0 autoneg off master-slave preferred-master speed 100
ethtool -s eth0 autoneg off master-slave preferred-slave speed 100
ethtool -s eth0 autoneg off master-slave forced-slave speed 100

Without setting the master-slave option it didn't work. I think its
mandatory.

> Regards,
> Stefan

Best regards,
Stefan

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

[Andrew Lunn]

> Without setting the master-slave option it didn't work. I think its
> mandatory.

I don't think it is. The PHY should have a default setting for
master-slave. Often its based on the typical use case. If its
typically inside a switch, then it should default to prefer-master. If
its typically in an end system, then it should be prefer-slave.

    Andrew

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

[Dimitri Fedrau]

Am Thu, Dec 21, 2023 at 03:25:56PM +0100 schrieb Andrew Lunn:
> > Without setting the master-slave option it didn't work. I think its
> > mandatory.
> 
> I don't think it is. The PHY should have a default setting for
> master-slave. Often its based on the typical use case. If its
> typically inside a switch, then it should default to prefer-master. If
> its typically in an end system, then it should be prefer-slave.
> 
That would be the case if you use a forced configuration. I think this
is already implemented by reading out the MDIO_PMA_PMD_BT1_CTRL in        
genphy_c45_pma_baset1_read_master_slave. Probably the problem arises    
with following lines which prevents an inital read of the configuration:

static int mv88q2xxx_config_init(struct phy_device *phydev)
{
        int ret;
                                                                        
        /* The 88Q2XXX PHYs do have the extended ability register available, but
         * register MDIO_PMA_EXTABLE where they should signalize it does not
         * work according to specification. Therefore, we force it here.
         */
        phydev->pma_extable = MDIO_PMA_EXTABLE_BT1;

        if (phydev->drv->phy_id == MARVELL_PHY_ID_88Q2220)
                return 0;

        /* Read the current PHY configuration */
        ret = genphy_c45_read_pma(phydev);
        if (ret)
                return ret;

        return mv88q2xxx_config_aneg(phydev);
}

If I type in "ethtool -s eth0 autoneg on" I don't have to set the 
master-slave option. Is the assumption here that we always start with a 
forced configuration ? If yes, then I have to fix it.

>     Andrew

	Dimitri

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

[Russell King (Oracle)]

On Thu, Dec 21, 2023 at 03:25:56PM +0100, Andrew Lunn wrote:
> > Without setting the master-slave option it didn't work. I think its
> > mandatory.
> 
> I don't think it is. The PHY should have a default setting for
> master-slave. Often its based on the typical use case. If its
> typically inside a switch, then it should default to prefer-master. If
> its typically in an end system, then it should be prefer-slave.

However, master/slave needs autoneg, and if autoneg is disabled, then
that may not be resolvable, and it may need manual configuration.

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

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

[Dimitri Fedrau]

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

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

[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

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

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

[Dimitri Fedrau]

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

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

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

	Andrew

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

[Dimitri Fedrau]

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