[PATCH v5 net-next 00/13] net: phy: marvell-88q2xxx: add driver for the Marvell 88Q2220 PHY

[PATCH v5 net-next 00/13] 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

Changes in V4:
	- clean up init sequence
	- separate patch for fixing typos in upstreamed code

Changes in V5:
	- add missing statics for mv88q222x_revb0_init_seq0 and
	  mv88q222x_revb0_init_seq1
	- fix typo in commit message: autonegotiation
	- fix ordering of Signed-off-by and Reviewed-by in commit messages
	- add interrupt support for link detection
	- add suspend / resume ops
	- add support for internal temperature sensor
	- add cable test support
	- call .soft_reset in mv88q2xxx_config_aneg, this makes
	  mv88q2xxx_config_aneg compatible for Marvell88Q222x devices and
	  remove mv88q222x_config_aneg which is then just duplicated code
	- cleanup mv88q2xxx_config_init and make it compatible with
	  Marvell88Q222x devices
	- move parts from mv88q222x_config_init to mv88q2xxx_config_init
	  that are applicable for all Marvell88Q2xxx devices.

Dimitri Fedrau (13):
  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: fix typos
  net: phy: marvell-88q2xxx: add driver for the Marvell 88Q2220 PHY
  net: phy: marvell-88q2xxx: add interrupt support for link detection
  net: phy: marvell-88q2xxx: add suspend / resume ops
  net: phy: marvell-88q2xxx: add support for temperature sensor
  net: phy: marvell-88q2xxx: add cable test support
  net: phy: marvell-88q2xxx: make mv88q2xxx_config_aneg generic
  net: phy: marvell-88q2xxx: switch to mv88q2xxx_config_aneg
  net: phy: marvell-88q2xxx: cleanup mv88q2xxx_config_init
  net: phy: marvell-88q2xxx: remove redundant code

 drivers/net/phy/marvell-88q2xxx.c | 634 ++++++++++++++++++++++++++++--
 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, 618 insertions(+), 30 deletions(-)

-- 
2.39.2

[PATCH v5 net-next 01/13] 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)

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
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 v5 net-next 02/13] 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.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
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 79ceee3c8673..ecd21acc7eed 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -373,6 +373,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);
 }
 
 /**
@@ -409,6 +413,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 v5 net-next 03/13] 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.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
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 747d14bf152c..de8f5dc8be12 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 v5 net-next 04/13] net: phy: marvell-88q2xxx: fix typos

[Dimitri Fedrau]

Rename mv88q2xxxx_get_sqi to mv88q2xxx_get_sqi and
mv88q2xxxx_get_sqi_max to mv88q2xxx_get_sqi_max.
Fix linebreaks and use everywhere hexadecimal numbers written with
lowercase letters instead of mixing it up.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
---
 drivers/net/phy/marvell-88q2xxx.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
index 1c3ff77de56b..dcebb4643aff 100644
--- a/drivers/net/phy/marvell-88q2xxx.c
+++ b/drivers/net/phy/marvell-88q2xxx.c
@@ -14,7 +14,7 @@
 #define MDIO_MMD_AN_MV_STAT_MS_CONF_FAULT	0x8000
 
 #define MDIO_MMD_PCS_MV_100BT1_STAT1			33032
-#define MDIO_MMD_PCS_MV_100BT1_STAT1_IDLE_ERROR	0x00FF
+#define MDIO_MMD_PCS_MV_100BT1_STAT1_IDLE_ERROR		0x00ff
 #define MDIO_MMD_PCS_MV_100BT1_STAT1_JABBER		0x0100
 #define MDIO_MMD_PCS_MV_100BT1_STAT1_LINK		0x0200
 #define MDIO_MMD_PCS_MV_100BT1_STAT1_LOCAL_RX		0x1000
@@ -27,6 +27,8 @@
 #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
+
 static int mv88q2xxx_soft_reset(struct phy_device *phydev)
 {
 	int ret;
@@ -63,7 +65,8 @@ static int mv88q2xxx_read_link_gbit(struct phy_device *phydev)
 		 * the link was already down.
 		 */
 		if (!phy_polling_mode(phydev) || !phydev->link) {
-			ret = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT);
+			ret = phy_read_mmd(phydev, MDIO_MMD_PCS,
+					   MDIO_PCS_1000BT1_STAT);
 			if (ret < 0)
 				return ret;
 			else if (ret & MDIO_PCS_1000BT1_STAT_LINK)
@@ -71,7 +74,8 @@ static int mv88q2xxx_read_link_gbit(struct phy_device *phydev)
 		}
 
 		if (!link) {
-			ret = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT);
+			ret = phy_read_mmd(phydev, MDIO_MMD_PCS,
+					   MDIO_PCS_1000BT1_STAT);
 			if (ret < 0)
 				return ret;
 			else if (ret & MDIO_PCS_1000BT1_STAT_LINK)
@@ -95,7 +99,8 @@ static int mv88q2xxx_read_link_100m(struct phy_device *phydev)
 	 * we always read the realtime status.
 	 */
 	if (!phy_polling_mode(phydev) || !phydev->link) {
-		ret = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_MMD_PCS_MV_100BT1_STAT1);
+		ret = phy_read_mmd(phydev, MDIO_MMD_PCS,
+				   MDIO_MMD_PCS_MV_100BT1_STAT1);
 		if (ret < 0)
 			return ret;
 		else if (ret & MDIO_MMD_PCS_MV_100BT1_STAT1_LINK)
@@ -200,7 +205,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 +213,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;
 
@@ -218,7 +224,7 @@ static int mv88q2xxxx_get_sqi(struct phy_device *phydev)
 		 * 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);
+		ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, 0xfc5d, 0xff, 0xac);
 		if (ret < 0)
 			return ret;
 
@@ -227,10 +233,10 @@ static int mv88q2xxxx_get_sqi(struct phy_device *phydev)
 			return ret;
 	}
 
-	return ret & 0x0F;
+	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;
 }
@@ -246,8 +252,8 @@ 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,
 	},
 };
 
-- 
2.39.2

[PATCH v5 net-next 05/13] 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. Autonegotiation is supported.

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

diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
index dcebb4643aff..637ef4821e35 100644
--- a/drivers/net/phy/marvell-88q2xxx.c
+++ b/drivers/net/phy/marvell-88q2xxx.c
@@ -6,6 +6,8 @@
 #include <linux/marvell_phy.h>
 #include <linux/phy.h>
 
+#define PHY_ID_88Q2220_REVB0	(MARVELL_PHY_ID_88Q2220 | 0x1)
+
 #define MDIO_MMD_AN_MV_STAT			32769
 #define MDIO_MMD_AN_MV_STAT_ANEG		0x0100
 #define MDIO_MMD_AN_MV_STAT_LOCAL_RX		0x1000
@@ -13,6 +15,11 @@
 #define MDIO_MMD_AN_MV_STAT_LOCAL_MASTER	0x4000
 #define MDIO_MMD_AN_MV_STAT_MS_CONF_FAULT	0x8000
 
+#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
+
 #define MDIO_MMD_PCS_MV_100BT1_STAT1			33032
 #define MDIO_MMD_PCS_MV_100BT1_STAT1_IDLE_ERROR		0x00ff
 #define MDIO_MMD_PCS_MV_100BT1_STAT1_JABBER		0x0100
@@ -29,6 +36,42 @@
 
 #define MDIO_MMD_PCS_MV_RX_STAT			33328
 
+struct mmd_val {
+	int devad;
+	u32 regnum;
+	u16 val;
+};
+
+static const struct mmd_val mv88q222x_revb0_init_seq0[] = {
+	{ MDIO_MMD_PCS, 0x8033, 0x6801 },
+	{ MDIO_MMD_AN, MDIO_AN_T1_CTRL, 0x0 },
+	{ MDIO_MMD_PMAPMD, MDIO_CTRL1,
+	  MDIO_CTRL1_LPOWER | MDIO_PMA_CTRL1_SPEED1000 },
+	{ MDIO_MMD_PCS, 0xfe1b, 0x48 },
+	{ MDIO_MMD_PCS, 0xffe4, 0x6b6 },
+	{ MDIO_MMD_PMAPMD, MDIO_CTRL1, 0x0 },
+	{ MDIO_MMD_PCS, MDIO_CTRL1, 0x0 },
+};
+
+static const struct mmd_val mv88q222x_revb0_init_seq1[] = {
+	{ MDIO_MMD_PCS, 0xfe79, 0x0 },
+	{ MDIO_MMD_PCS, 0xfe07, 0x125a },
+	{ MDIO_MMD_PCS, 0xfe09, 0x1288 },
+	{ MDIO_MMD_PCS, 0xfe08, 0x2588 },
+	{ MDIO_MMD_PCS, 0xfe11, 0x1105 },
+	{ MDIO_MMD_PCS, 0xfe72, 0x042c },
+	{ MDIO_MMD_PCS, 0xfbba, 0xcb2 },
+	{ MDIO_MMD_PCS, 0xfbbb, 0xc4a },
+	{ MDIO_MMD_AN, 0x8032, 0x2020 },
+	{ MDIO_MMD_AN, 0x8031, 0xa28 },
+	{ MDIO_MMD_AN, 0x8031, 0xc28 },
+	{ MDIO_MMD_PCS, 0xffdb, 0xfc10 },
+	{ MDIO_MMD_PCS, 0xfe1b, 0x58 },
+	{ MDIO_MMD_PCS, 0xfe79, 0x4 },
+	{ MDIO_MMD_PCS, 0xfe5f, 0xe8 },
+	{ MDIO_MMD_PCS, 0xfe05, 0x755c },
+};
+
 static int mv88q2xxx_soft_reset(struct phy_device *phydev)
 {
 	int ret;
@@ -125,24 +168,90 @@ 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 ret;
+	return 0;
+}
+
+static int mv88q2xxx_read_aneg_speed(struct phy_device *phydev)
+{
+	int 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) {
+		/* 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;
+
+		ret = mv88q2xxx_read_master_slave_state(phydev);
+		if (ret < 0)
+			return ret;
+
+		phy_resolve_aneg_linkmode(phydev);
+
+		return 0;
+	}
+
 	ret = mv88q2xxx_read_link(phydev);
 	if (ret < 0)
 		return ret;
@@ -171,7 +280,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;
 }
@@ -241,6 +352,75 @@ 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;
+	}
+
+	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);
+
+	return 0;
+}
+
+static int mv88q222x_config_aneg(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = genphy_c45_config_aneg(phydev);
+	if (ret)
+		return ret;
+
+	return mv88q222x_soft_reset(phydev);
+}
+
+static int mv88q222x_revb0_config_init(struct phy_device *phydev)
+{
+	int ret, i;
+
+	for (i = 0; i < ARRAY_SIZE(mv88q222x_revb0_init_seq0); i++) {
+		ret = phy_write_mmd(phydev, mv88q222x_revb0_init_seq0[i].devad,
+				    mv88q222x_revb0_init_seq0[i].regnum,
+				    mv88q222x_revb0_init_seq0[i].val);
+		if (ret < 0)
+			return ret;
+	}
+
+	usleep_range(5000, 10000);
+
+	for (i = 0; i < ARRAY_SIZE(mv88q222x_revb0_init_seq1); i++) {
+		ret = phy_write_mmd(phydev, mv88q222x_revb0_init_seq1[i].devad,
+				    mv88q222x_revb0_init_seq1[i].regnum,
+				    mv88q222x_revb0_init_seq1[i].val);
+		if (ret < 0)
+			return 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;
+
+	return 0;
+}
+
 static struct phy_driver mv88q2xxx_driver[] = {
 	{
 		.phy_id			= MARVELL_PHY_ID_88Q2110,
@@ -255,12 +435,26 @@ static struct phy_driver mv88q2xxx_driver[] = {
 		.get_sqi		= mv88q2xxx_get_sqi,
 		.get_sqi_max		= mv88q2xxx_get_sqi_max,
 	},
+	{
+		PHY_ID_MATCH_EXACT(PHY_ID_88Q2220_REVB0),
+		.name			= "mv88q2220",
+		.get_features		= mv88q2xxx_get_features,
+		.config_aneg		= mv88q222x_config_aneg,
+		.aneg_done		= genphy_c45_aneg_done,
+		.config_init		= mv88q222x_revb0_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,
+	},
 };
 
 module_phy_driver(mv88q2xxx_driver);
 
 static struct mdio_device_id __maybe_unused mv88q2xxx_tbl[] = {
 	{ MARVELL_PHY_ID_88Q2110, MARVELL_PHY_ID_MASK },
+	{ PHY_ID_MATCH_EXACT(PHY_ID_88Q2220_REVB0), },
 	{ /*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

[PATCH v5 net-next 06/13] net: phy: marvell-88q2xxx: add interrupt support for link detection

[Dimitri Fedrau]

Added .config_intr and .handle_interrupt callbacks. Whenever the link
goes up or down an interrupt will be triggered. Interrupts are configured
separately for 100/1000BASET1.

Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
---
 drivers/net/phy/marvell-88q2xxx.c | 123 +++++++++++++++++++++++++++++-
 1 file changed, 119 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
index 637ef4821e35..635827b4a692 100644
--- a/drivers/net/phy/marvell-88q2xxx.c
+++ b/drivers/net/phy/marvell-88q2xxx.c
@@ -20,6 +20,19 @@
 #define MDIO_MMD_AN_MV_STAT2_100BT1		0x2000
 #define MDIO_MMD_AN_MV_STAT2_1000BT1		0x4000
 
+#define MDIO_MMD_PCS_MV_INT_EN			32784
+#define MDIO_MMD_PCS_MV_INT_EN_LINK_UP		0x0040
+#define MDIO_MMD_PCS_MV_INT_EN_LINK_DOWN	0x0080
+#define MDIO_MMD_PCS_MV_INT_EN_100BT1		0x1000
+
+#define MDIO_MMD_PCS_MV_GPIO_INT_STAT			32785
+#define MDIO_MMD_PCS_MV_GPIO_INT_STAT_LINK_UP		0x0040
+#define MDIO_MMD_PCS_MV_GPIO_INT_STAT_LINK_DOWN		0x0080
+#define MDIO_MMD_PCS_MV_GPIO_INT_STAT_100BT1_GEN	0x1000
+
+#define MDIO_MMD_PCS_MV_GPIO_INT_CTRL			32787
+#define MDIO_MMD_PCS_MV_GPIO_INT_CTRL_TRI_DIS		0x0800
+
 #define MDIO_MMD_PCS_MV_100BT1_STAT1			33032
 #define MDIO_MMD_PCS_MV_100BT1_STAT1_IDLE_ERROR		0x00ff
 #define MDIO_MMD_PCS_MV_100BT1_STAT1_JABBER		0x0100
@@ -34,6 +47,12 @@
 #define MDIO_MMD_PCS_MV_100BT1_STAT2_LINK	0x0004
 #define MDIO_MMD_PCS_MV_100BT1_STAT2_ANGE	0x0008
 
+#define MDIO_MMD_PCS_MV_100BT1_INT_EN			33042
+#define MDIO_MMD_PCS_MV_100BT1_INT_EN_LINKEVENT		0x0400
+
+#define MDIO_MMD_PCS_MV_COPPER_INT_STAT			33043
+#define MDIO_MMD_PCS_MV_COPPER_INT_STAT_LINKEVENT	0x0400
+
 #define MDIO_MMD_PCS_MV_RX_STAT			33328
 
 struct mmd_val {
@@ -95,13 +114,15 @@ static int mv88q2xxx_read_link_gbit(struct phy_device *phydev)
 
 	/* Read vendor specific Auto-Negotiation status register to get local
 	 * and remote receiver status according to software initialization
-	 * guide.
+	 * guide. However, when not in polling mode the local and remote
+	 * receiver status are not evaluated due to the Marvell 88Q2xxx APIs.
 	 */
 	ret = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_MMD_AN_MV_STAT);
 	if (ret < 0) {
 		return ret;
-	} else if ((ret & MDIO_MMD_AN_MV_STAT_LOCAL_RX) &&
-		   (ret & MDIO_MMD_AN_MV_STAT_REMOTE_RX)) {
+	} else if (((ret & MDIO_MMD_AN_MV_STAT_LOCAL_RX) &&
+		   (ret & MDIO_MMD_AN_MV_STAT_REMOTE_RX)) ||
+		   !phy_polling_mode(phydev)) {
 		/* The link state is latched low so that momentary link
 		 * drops can be detected. Do not double-read the status
 		 * in polling mode to detect such short link drops except
@@ -141,7 +162,18 @@ static int mv88q2xxx_read_link_100m(struct phy_device *phydev)
 	 * the link was already down. In case we are not polling,
 	 * we always read the realtime status.
 	 */
-	if (!phy_polling_mode(phydev) || !phydev->link) {
+	if (!phy_polling_mode(phydev)) {
+		phydev->link = false;
+		ret = phy_read_mmd(phydev, MDIO_MMD_PCS,
+				   MDIO_MMD_PCS_MV_100BT1_STAT2);
+		if (ret < 0)
+			return ret;
+
+		if (ret & MDIO_MMD_PCS_MV_100BT1_STAT2_LINK)
+			phydev->link = true;
+
+		return 0;
+	} else if (!phydev->link) {
 		ret = phy_read_mmd(phydev, MDIO_MMD_PCS,
 				   MDIO_MMD_PCS_MV_100BT1_STAT1);
 		if (ret < 0)
@@ -352,6 +384,79 @@ static int mv88q2xxx_get_sqi_max(struct phy_device *phydev)
 	return 15;
 }
 
+static int mv88q2xxx_config_intr(struct phy_device *phydev)
+{
+	int ret;
+
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+		/* Enable interrupts for 1000BASE-T1 link up and down events
+		 * and enable general interrupts for 100BASE-T1.
+		 */
+		ret = phy_write_mmd(phydev, MDIO_MMD_PCS,
+				    MDIO_MMD_PCS_MV_INT_EN,
+				    MDIO_MMD_PCS_MV_INT_EN_LINK_UP |
+				    MDIO_MMD_PCS_MV_INT_EN_LINK_DOWN |
+				    MDIO_MMD_PCS_MV_INT_EN_100BT1);
+		if (ret < 0)
+			return ret;
+
+		/* Enable interrupts for 100BASE-T1 link events */
+		return phy_write_mmd(phydev, MDIO_MMD_PCS,
+				     MDIO_MMD_PCS_MV_100BT1_INT_EN,
+				     MDIO_MMD_PCS_MV_100BT1_INT_EN_LINKEVENT);
+	} else {
+		ret = phy_write_mmd(phydev, MDIO_MMD_PCS,
+				    MDIO_MMD_PCS_MV_INT_EN, 0);
+		if (ret < 0)
+			return ret;
+
+		return phy_write_mmd(phydev, MDIO_MMD_PCS,
+				     MDIO_MMD_PCS_MV_100BT1_INT_EN, 0);
+	}
+}
+
+static irqreturn_t mv88q2xxx_handle_interrupt(struct phy_device *phydev)
+{
+	bool trigger_machine = false;
+	int irq;
+
+	/* Before we can acknowledge the 100BT1 general interrupt, that is in
+	 * the 1000BT1 interrupt status register, we have to acknowledge any
+	 * interrupts that are related to it. Therefore we read first the 100BT1
+	 * interrupt status register, followed by reading the 1000BT1 interrupt
+	 * status register.
+	 */
+
+	irq = phy_read_mmd(phydev, MDIO_MMD_PCS,
+			   MDIO_MMD_PCS_MV_COPPER_INT_STAT);
+	if (irq < 0) {
+		phy_error(phydev);
+		return IRQ_NONE;
+	}
+
+	/* Check link status for 100BT1 */
+	if (irq & MDIO_MMD_PCS_MV_COPPER_INT_STAT_LINKEVENT)
+		trigger_machine = true;
+
+	irq = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_MMD_PCS_MV_GPIO_INT_STAT);
+	if (irq < 0) {
+		phy_error(phydev);
+		return IRQ_NONE;
+	}
+
+	/* Check link status for 1000BT1 */
+	if ((irq & MDIO_MMD_PCS_MV_GPIO_INT_STAT_LINK_UP) ||
+	    (irq & MDIO_MMD_PCS_MV_GPIO_INT_STAT_LINK_DOWN))
+		trigger_machine = true;
+
+	if (!trigger_machine)
+		return IRQ_NONE;
+
+	phy_trigger_machine(phydev);
+
+	return IRQ_HANDLED;
+}
+
 static int mv88q222x_soft_reset(struct phy_device *phydev)
 {
 	int ret;
@@ -418,6 +523,14 @@ static int mv88q222x_revb0_config_init(struct phy_device *phydev)
 	 */
 	phydev->pma_extable = MDIO_PMA_EXTABLE_BT1;
 
+	/* Configure interrupt with default settings, output is driven low for
+	 * active interrupt and high for inactive.
+	 */
+	if (phy_interrupt_is_valid(phydev))
+		return phy_set_bits_mmd(phydev, MDIO_MMD_PCS,
+					MDIO_MMD_PCS_MV_GPIO_INT_CTRL,
+					MDIO_MMD_PCS_MV_GPIO_INT_CTRL_TRI_DIS);
+
 	return 0;
 }
 
@@ -444,6 +557,8 @@ static struct phy_driver mv88q2xxx_driver[] = {
 		.config_init		= mv88q222x_revb0_config_init,
 		.read_status		= mv88q2xxx_read_status,
 		.soft_reset		= mv88q222x_soft_reset,
+		.config_intr		= mv88q2xxx_config_intr,
+		.handle_interrupt	= mv88q2xxx_handle_interrupt,
 		.set_loopback		= genphy_c45_loopback,
 		.get_sqi		= mv88q2xxx_get_sqi,
 		.get_sqi_max		= mv88q2xxx_get_sqi_max,
-- 
2.39.2

[PATCH v5 net-next 07/13] net: phy: marvell-88q2xxx: add suspend / resume ops

[Dimitri Fedrau]

Add suspend/resume ops for Marvell 88Q2xxx devices.

Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
---
 drivers/net/phy/marvell-88q2xxx.c | 34 +++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
index 635827b4a692..4cb8fe524795 100644
--- a/drivers/net/phy/marvell-88q2xxx.c
+++ b/drivers/net/phy/marvell-88q2xxx.c
@@ -457,6 +457,38 @@ static irqreturn_t mv88q2xxx_handle_interrupt(struct phy_device *phydev)
 	return IRQ_HANDLED;
 }
 
+static int mv88q2xxx_suspend(struct phy_device *phydev)
+{
+	int ret;
+
+	/* Disable PHY interrupts */
+	if (phy_interrupt_is_valid(phydev)) {
+		phydev->interrupts = PHY_INTERRUPT_DISABLED;
+		ret = mv88q2xxx_config_intr(phydev);
+		if (ret)
+			return ret;
+	}
+
+	return phy_set_bits_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1,
+				MDIO_CTRL1_LPOWER);
+}
+
+static int mv88q2xxx_resume(struct phy_device *phydev)
+{
+	int ret;
+
+	/* Enable PHY interrupts */
+	if (phy_interrupt_is_valid(phydev)) {
+		phydev->interrupts = PHY_INTERRUPT_ENABLED;
+		ret = mv88q2xxx_config_intr(phydev);
+		if (ret)
+			return ret;
+	}
+
+	return phy_clear_bits_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1,
+				  MDIO_CTRL1_LPOWER);
+}
+
 static int mv88q222x_soft_reset(struct phy_device *phydev)
 {
 	int ret;
@@ -562,6 +594,8 @@ static struct phy_driver mv88q2xxx_driver[] = {
 		.set_loopback		= genphy_c45_loopback,
 		.get_sqi		= mv88q2xxx_get_sqi,
 		.get_sqi_max		= mv88q2xxx_get_sqi_max,
+		.suspend		= mv88q2xxx_suspend,
+		.resume			= mv88q2xxx_resume,
 	},
 };
 
-- 
2.39.2

[PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

[Dimitri Fedrau]

Marvell 88q2xxx devices have an inbuilt temperature sensor. Add hwmon
support for this sensor.

Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
---
 drivers/net/phy/marvell-88q2xxx.c | 152 ++++++++++++++++++++++++++++++
 1 file changed, 152 insertions(+)

diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
index 4cb8fe524795..6900bad275d0 100644
--- a/drivers/net/phy/marvell-88q2xxx.c
+++ b/drivers/net/phy/marvell-88q2xxx.c
@@ -5,6 +5,7 @@
 #include <linux/ethtool_netlink.h>
 #include <linux/marvell_phy.h>
 #include <linux/phy.h>
+#include <linux/hwmon.h>
 
 #define PHY_ID_88Q2220_REVB0	(MARVELL_PHY_ID_88Q2220 | 0x1)
 
@@ -33,6 +34,19 @@
 #define MDIO_MMD_PCS_MV_GPIO_INT_CTRL			32787
 #define MDIO_MMD_PCS_MV_GPIO_INT_CTRL_TRI_DIS		0x0800
 
+#define MDIO_MMD_PCS_MV_TEMP_SENSOR1			32833
+#define MDIO_MMD_PCS_MV_TEMP_SENSOR1_RAW_INT		0x0001
+#define MDIO_MMD_PCS_MV_TEMP_SENSOR1_INT		0x0040
+#define MDIO_MMD_PCS_MV_TEMP_SENSOR1_INT_EN		0x0080
+
+#define MDIO_MMD_PCS_MV_TEMP_SENSOR2			32834
+#define MDIO_MMD_PCS_MV_TEMP_SENSOR2_DIS_MASK		0xc000
+
+#define MDIO_MMD_PCS_MV_TEMP_SENSOR3			32835
+#define MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_MASK	0xff00
+#define MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_SHIFT	8
+#define MDIO_MMD_PCS_MV_TEMP_SENSOR3_MASK		0x00ff
+
 #define MDIO_MMD_PCS_MV_100BT1_STAT1			33032
 #define MDIO_MMD_PCS_MV_100BT1_STAT1_IDLE_ERROR		0x00ff
 #define MDIO_MMD_PCS_MV_100BT1_STAT1_JABBER		0x0100
@@ -488,6 +502,143 @@ static int mv88q2xxx_resume(struct phy_device *phydev)
 	return phy_clear_bits_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1,
 				  MDIO_CTRL1_LPOWER);
 }
+#ifdef CONFIG_HWMON
+static const struct hwmon_channel_info * const mv88q2xxx_hwmon_info[] = {
+	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_ALARM),
+	NULL
+};
+
+static umode_t mv88q2xxx_hwmon_is_visible(const void *data,
+					  enum hwmon_sensor_types type,
+					  u32 attr, int channel)
+{
+	switch (attr) {
+	case hwmon_temp_input:
+		return 0444;
+	case hwmon_temp_max:
+		return 0644;
+	case hwmon_temp_alarm:
+		return 0444;
+	default:
+		return 0;
+	}
+}
+
+static int mv88q2xxx_hwmon_read(struct device *dev,
+				enum hwmon_sensor_types type,
+				u32 attr, int channel, long *val)
+{
+	struct phy_device *phydev = dev_get_drvdata(dev);
+	int ret;
+
+	switch (attr) {
+	case hwmon_temp_input:
+		ret = phy_read_mmd(phydev, MDIO_MMD_PCS,
+				   MDIO_MMD_PCS_MV_TEMP_SENSOR3);
+		if (ret < 0)
+			return ret;
+
+		*val = ((ret & MDIO_MMD_PCS_MV_TEMP_SENSOR3_MASK) - 75) * 1000;
+		return 0;
+	case hwmon_temp_max:
+		ret = phy_read_mmd(phydev, MDIO_MMD_PCS,
+				   MDIO_MMD_PCS_MV_TEMP_SENSOR3);
+		if (ret < 0)
+			return ret;
+
+		*val = (((ret & MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_MASK) >>
+			MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_SHIFT) - 75) *
+			1000;
+		return 0;
+	case hwmon_temp_alarm:
+		ret = phy_read_mmd(phydev, MDIO_MMD_PCS,
+				   MDIO_MMD_PCS_MV_TEMP_SENSOR1);
+		if (ret < 0)
+			return ret;
+
+		*val = !!(ret & MDIO_MMD_PCS_MV_TEMP_SENSOR1_RAW_INT);
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int mv88q2xxx_hwmon_write(struct device *dev,
+				 enum hwmon_sensor_types type, u32 attr,
+				 int channel, long val)
+{
+	struct phy_device *phydev = dev_get_drvdata(dev);
+
+	switch (attr) {
+	case hwmon_temp_max:
+		if (val < -75000 || val > 180000)
+			return -EINVAL;
+
+		val = ((val / 1000) + 75) <<
+		       MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_SHIFT;
+		return phy_modify_mmd(phydev, MDIO_MMD_PCS,
+				      MDIO_MMD_PCS_MV_TEMP_SENSOR3,
+				      MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_MASK,
+				      val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static const struct hwmon_ops mv88q2xxx_hwmon_hwmon_ops = {
+	.is_visible = mv88q2xxx_hwmon_is_visible,
+	.read = mv88q2xxx_hwmon_read,
+	.write = mv88q2xxx_hwmon_write,
+};
+
+static const struct hwmon_chip_info mv88q2xxx_hwmon_chip_info = {
+	.ops = &mv88q2xxx_hwmon_hwmon_ops,
+	.info = mv88q2xxx_hwmon_info,
+};
+
+static int mv88q2xxx_hwmon_probe(struct phy_device *phydev)
+{
+	struct device *dev = &phydev->mdio.dev;
+	struct device *hwmon;
+	char *hwmon_name;
+	int ret;
+
+	/* Enable temperature sensor interrupt */
+	ret = phy_set_bits_mmd(phydev, MDIO_MMD_PCS,
+			       MDIO_MMD_PCS_MV_TEMP_SENSOR1,
+			       MDIO_MMD_PCS_MV_TEMP_SENSOR1_INT_EN);
+	if (ret < 0)
+		return ret;
+
+	/* Enable temperature sense */
+	ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, MDIO_MMD_PCS_MV_TEMP_SENSOR2,
+			     MDIO_MMD_PCS_MV_TEMP_SENSOR2_DIS_MASK, 0);
+	if (ret < 0)
+		return ret;
+
+	hwmon_name = devm_hwmon_sanitize_name(dev, dev_name(dev));
+	if (IS_ERR(hwmon_name))
+		return PTR_ERR(hwmon_name);
+
+	hwmon = devm_hwmon_device_register_with_info(dev,
+						     hwmon_name,
+						     phydev,
+						     &mv88q2xxx_hwmon_chip_info,
+						     NULL);
+
+	return PTR_ERR_OR_ZERO(hwmon);
+}
+
+#else
+static int mv88q2xxx_hwmon_probe(struct phy_device *phydev)
+{
+	return 0;
+}
+#endif
+static int mv88q2xxx_probe(struct phy_device *phydev)
+{
+	return mv88q2xxx_hwmon_probe(phydev);
+}
 
 static int mv88q222x_soft_reset(struct phy_device *phydev)
 {
@@ -583,6 +734,7 @@ static struct phy_driver mv88q2xxx_driver[] = {
 	{
 		PHY_ID_MATCH_EXACT(PHY_ID_88Q2220_REVB0),
 		.name			= "mv88q2220",
+		.probe			= mv88q2xxx_probe,
 		.get_features		= mv88q2xxx_get_features,
 		.config_aneg		= mv88q222x_config_aneg,
 		.aneg_done		= genphy_c45_aneg_done,
-- 
2.39.2

[PATCH v5 net-next 09/13] net: phy: marvell-88q2xxx: add cable test support

[Dimitri Fedrau]

Add cable test support for Marvell 88Q222x devices. Reported distance
granularity is 1m.

1m cable, open:
  Cable test started for device eth0.
  Cable test completed for device eth0.
  Pair A code Open Circuit
  Pair A, fault length: 1.00m

1m cable, shorted:
  Cable test started for device eth0.
  Cable test completed for device eth0.
  Pair A code Short within Pair
  Pair A, fault length: 1.00m

6m cable, open:
  Cable test started for device eth0.
  Cable test completed for device eth0.
  Pair A code Open Circuit
  Pair A, fault length: 6.00m

6m cable, shorted:
  Cable test started for device eth0.
  Cable test completed for device eth0.
  Pair A code Short within Pair
  Pair A, fault length: 6.00m

Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
---
 drivers/net/phy/marvell-88q2xxx.c | 99 +++++++++++++++++++++++++++++++
 1 file changed, 99 insertions(+)

diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
index 6900bad275d0..f59822cf9526 100644
--- a/drivers/net/phy/marvell-88q2xxx.c
+++ b/drivers/net/phy/marvell-88q2xxx.c
@@ -69,6 +69,27 @@
 
 #define MDIO_MMD_PCS_MV_RX_STAT			33328
 
+#define MDIO_MMD_PCS_MV_TDR_RESET			65226
+#define MDIO_MMD_PCS_MV_TDR_RESET_TDR_RST		0x1000
+
+#define MDIO_MMD_PCS_MV_TDR_OFF_SHORT_CABLE		65241
+
+#define MDIO_MMD_PCS_MV_TDR_OFF_LONG_CABLE		65242
+
+#define MDIO_MMD_PCS_MV_TDR_STATUS			65245
+#define MDIO_MMD_PCS_MV_TDR_STATUS_OFF			0x0001
+#define MDIO_MMD_PCS_MV_TDR_STATUS_ON			0x0002
+#define MDIO_MMD_PCS_MV_TDR_STATUS_DIST_MASK		0xff00
+#define MDIO_MMD_PCS_MV_TDR_STATUS_DIST_SHIFT		8
+#define MDIO_MMD_PCS_MV_TDR_STATUS_VCT_STAT_MASK	0x00f0
+#define MDIO_MMD_PCS_MV_TDR_STATUS_VCT_STAT_SHORT	0x0030
+#define MDIO_MMD_PCS_MV_TDR_STATUS_VCT_STAT_OPEN	0x00e0
+#define MDIO_MMD_PCS_MV_TDR_STATUS_VCT_STAT_OK		0x0070
+#define MDIO_MMD_PCS_MV_TDR_STATUS_VCT_STAT_IN_PROGR	0x0080
+#define MDIO_MMD_PCS_MV_TDR_STATUS_VCT_STAT_NOISE	0x0050
+
+#define MDIO_MMD_PCS_MV_TDR_OFF_CUTOFF			65246
+
 struct mmd_val {
 	int devad;
 	u32 regnum;
@@ -717,6 +738,81 @@ static int mv88q222x_revb0_config_init(struct phy_device *phydev)
 	return 0;
 }
 
+static int mv88q222x_cable_test_start(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS,
+			    MDIO_MMD_PCS_MV_TDR_OFF_CUTOFF, 0x0058);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS,
+			    MDIO_MMD_PCS_MV_TDR_OFF_LONG_CABLE, 0x00eb);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS,
+			    MDIO_MMD_PCS_MV_TDR_OFF_SHORT_CABLE, 0x010e);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, MDIO_MMD_PCS_MV_TDR_RESET,
+			    0x0d90);
+	if (ret < 0)
+		return ret;
+
+	return phy_write_mmd(phydev, MDIO_MMD_PCS, MDIO_MMD_PCS_MV_TDR_STATUS,
+			     MDIO_MMD_PCS_MV_TDR_STATUS_ON);
+}
+
+static int mv88q222x_cable_test_get_status(struct phy_device *phydev,
+					   bool *finished)
+{
+	int ret;
+	u32 dist;
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_MMD_PCS_MV_TDR_STATUS);
+	if (ret < 0)
+		return ret;
+
+	*finished = true;
+	/* fault length in meters */
+	dist = ((ret & MDIO_MMD_PCS_MV_TDR_STATUS_DIST_MASK) >>
+		MDIO_MMD_PCS_MV_TDR_STATUS_DIST_SHIFT) * 100;
+	switch (ret & MDIO_MMD_PCS_MV_TDR_STATUS_VCT_STAT_MASK) {
+	case MDIO_MMD_PCS_MV_TDR_STATUS_VCT_STAT_OPEN:
+		ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_A,
+					ETHTOOL_A_CABLE_RESULT_CODE_OPEN);
+		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_A,
+					      dist);
+		break;
+	case MDIO_MMD_PCS_MV_TDR_STATUS_VCT_STAT_SHORT:
+		ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_A,
+					ETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT);
+		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_A,
+					      dist);
+		break;
+	case MDIO_MMD_PCS_MV_TDR_STATUS_VCT_STAT_OK:
+		ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_A,
+					ETHTOOL_A_CABLE_RESULT_CODE_OK);
+		break;
+	case MDIO_MMD_PCS_MV_TDR_STATUS_VCT_STAT_IN_PROGR:
+		*finished = false;
+		break;
+	default:
+		ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_A,
+					ETHTOOL_A_CABLE_RESULT_CODE_UNSPEC);
+	}
+
+	if (*finished)
+		return phy_write_mmd(phydev, MDIO_MMD_PCS,
+				     MDIO_MMD_PCS_MV_TDR_RESET,
+				     MDIO_MMD_PCS_MV_TDR_RESET_TDR_RST | 0xd90);
+
+	return 0;
+}
+
 static struct phy_driver mv88q2xxx_driver[] = {
 	{
 		.phy_id			= MARVELL_PHY_ID_88Q2110,
@@ -734,6 +830,7 @@ static struct phy_driver mv88q2xxx_driver[] = {
 	{
 		PHY_ID_MATCH_EXACT(PHY_ID_88Q2220_REVB0),
 		.name			= "mv88q2220",
+		.flags			= PHY_POLL_CABLE_TEST,
 		.probe			= mv88q2xxx_probe,
 		.get_features		= mv88q2xxx_get_features,
 		.config_aneg		= mv88q222x_config_aneg,
@@ -744,6 +841,8 @@ static struct phy_driver mv88q2xxx_driver[] = {
 		.config_intr		= mv88q2xxx_config_intr,
 		.handle_interrupt	= mv88q2xxx_handle_interrupt,
 		.set_loopback		= genphy_c45_loopback,
+		.cable_test_start	= mv88q222x_cable_test_start,
+		.cable_test_get_status	= mv88q222x_cable_test_get_status,
 		.get_sqi		= mv88q2xxx_get_sqi,
 		.get_sqi_max		= mv88q2xxx_get_sqi_max,
 		.suspend		= mv88q2xxx_suspend,
-- 
2.39.2

[PATCH v5 net-next 10/13] net: phy: marvell-88q2xxx: make mv88q2xxx_config_aneg generic

[Dimitri Fedrau]

Marvell 88Q2xxx devices follow the same scheme, after configuration they
need a soft reset. Soft resets differ between devices, so we use the
.soft_reset callback instead of creating .config_aneg callbacks for each
device.

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

diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
index f59822cf9526..8bd68b3c3ea1 100644
--- a/drivers/net/phy/marvell-88q2xxx.c
+++ b/drivers/net/phy/marvell-88q2xxx.c
@@ -362,7 +362,7 @@ static int mv88q2xxx_config_aneg(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
-	return mv88q2xxx_soft_reset(phydev);
+	return phydev->drv->soft_reset(phydev);
 }
 
 static int mv88q2xxx_config_init(struct phy_device *phydev)
-- 
2.39.2

[PATCH v5 net-next 11/13] net: phy: marvell-88q2xxx: switch to mv88q2xxx_config_aneg

[Dimitri Fedrau]

Switch to mv88q2xxx_config_aneg for Marvell 88Q2220 devices and remove
the mv88q222x_config_aneg function which is basically a copy of the
mv88q2xxx_config_aneg function.

Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
---
 drivers/net/phy/marvell-88q2xxx.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
index 8bd68b3c3ea1..e40ea351fcde 100644
--- a/drivers/net/phy/marvell-88q2xxx.c
+++ b/drivers/net/phy/marvell-88q2xxx.c
@@ -688,17 +688,6 @@ static int mv88q222x_soft_reset(struct phy_device *phydev)
 	return 0;
 }
 
-static int mv88q222x_config_aneg(struct phy_device *phydev)
-{
-	int ret;
-
-	ret = genphy_c45_config_aneg(phydev);
-	if (ret)
-		return ret;
-
-	return mv88q222x_soft_reset(phydev);
-}
-
 static int mv88q222x_revb0_config_init(struct phy_device *phydev)
 {
 	int ret, i;
@@ -833,7 +822,7 @@ static struct phy_driver mv88q2xxx_driver[] = {
 		.flags			= PHY_POLL_CABLE_TEST,
 		.probe			= mv88q2xxx_probe,
 		.get_features		= mv88q2xxx_get_features,
-		.config_aneg		= mv88q222x_config_aneg,
+		.config_aneg		= mv88q2xxx_config_aneg,
 		.aneg_done		= genphy_c45_aneg_done,
 		.config_init		= mv88q222x_revb0_config_init,
 		.read_status		= mv88q2xxx_read_status,
-- 
2.39.2

[PATCH v5 net-next 12/13] net: phy: marvell-88q2xxx: cleanup mv88q2xxx_config_init

[Dimitri Fedrau]

mv88q2xxx_config_init calls genphy_c45_read_pma which is done by
mv88q2xxx_read_status, it calls also mv88q2xxx_config_aneg which is
also called by the PHY state machine. Let the PHY state machine handle
the phydriver ops in their intendend way.

Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
---
 drivers/net/phy/marvell-88q2xxx.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
index e40ea351fcde..081ed110c87e 100644
--- a/drivers/net/phy/marvell-88q2xxx.c
+++ b/drivers/net/phy/marvell-88q2xxx.c
@@ -367,20 +367,13 @@ static int mv88q2xxx_config_aneg(struct phy_device *phydev)
 
 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;
 
-	/* Read the current PHY configuration */
-	ret = genphy_c45_read_pma(phydev);
-	if (ret)
-		return ret;
-
-	return mv88q2xxx_config_aneg(phydev);
+	return 0;
 }
 
 static int mv88q2xxx_get_sqi(struct phy_device *phydev)
-- 
2.39.2

[PATCH v5 net-next 13/13] net: phy: marvell-88q2xxx: remove redundant code

[Dimitri Fedrau]

Remove redundant code in mv88q222x_revb0_config_init,
phydev->pma_extable is already set in mv88q2xxx_config_init. Just call
mv88q2xxx_config_init in mv88q222x_revb0_config_init.

Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
---
 drivers/net/phy/marvell-88q2xxx.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
index 081ed110c87e..6df580442b6b 100644
--- a/drivers/net/phy/marvell-88q2xxx.c
+++ b/drivers/net/phy/marvell-88q2xxx.c
@@ -373,6 +373,14 @@ static int mv88q2xxx_config_init(struct phy_device *phydev)
 	 */
 	phydev->pma_extable = MDIO_PMA_EXTABLE_BT1;
 
+	/* Configure interrupt with default settings, output is driven low for
+	 * active interrupt and high for inactive.
+	 */
+	if (phy_interrupt_is_valid(phydev))
+		return phy_set_bits_mmd(phydev, MDIO_MMD_PCS,
+					MDIO_MMD_PCS_MV_GPIO_INT_CTRL,
+					MDIO_MMD_PCS_MV_GPIO_INT_CTRL_TRI_DIS);
+
 	return 0;
 }
 
@@ -703,21 +711,7 @@ static int mv88q222x_revb0_config_init(struct phy_device *phydev)
 			return 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;
-
-	/* Configure interrupt with default settings, output is driven low for
-	 * active interrupt and high for inactive.
-	 */
-	if (phy_interrupt_is_valid(phydev))
-		return phy_set_bits_mmd(phydev, MDIO_MMD_PCS,
-					MDIO_MMD_PCS_MV_GPIO_INT_CTRL,
-					MDIO_MMD_PCS_MV_GPIO_INT_CTRL_TRI_DIS);
-
-	return 0;
+	return mv88q2xxx_config_init(phydev);
 }
 
 static int mv88q222x_cable_test_start(struct phy_device *phydev)
-- 
2.39.2

Re: [PATCH v5 net-next 00/13] net: phy: marvell-88q2xxx: add driver for the Marvell 88Q2220 PHY

[Jakub Kicinski]

On Mon, 22 Jan 2024 22:28:33 +0100 Dimitri Fedrau wrote:
> Changes in V5:
> 	- add missing statics for mv88q222x_revb0_init_seq0 and
> 	  mv88q222x_revb0_init_seq1
> 	- fix typo in commit message: autonegotiation
> 	- fix ordering of Signed-off-by and Reviewed-by in commit messages
> 	- add interrupt support for link detection
> 	- add suspend / resume ops
> 	- add support for internal temperature sensor
> 	- add cable test support
> 	- call .soft_reset in mv88q2xxx_config_aneg, this makes
> 	  mv88q2xxx_config_aneg compatible for Marvell88Q222x devices and
> 	  remove mv88q222x_config_aneg which is then just duplicated code
> 	- cleanup mv88q2xxx_config_init and make it compatible with
> 	  Marvell88Q222x devices
> 	- move parts from mv88q222x_config_init to mv88q2xxx_config_init
> 	  that are applicable for all Marvell88Q2xxx devices.

PHY maintainers - could anyone take a look at these patches?

Re: [PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

[Russell King (Oracle)]

On Mon, Jan 22, 2024 at 10:28:41PM +0100, Dimitri Fedrau wrote:

	int tmp;

> +	switch (attr) {
> +	case hwmon_temp_input:
> +		ret = phy_read_mmd(phydev, MDIO_MMD_PCS,
> +				   MDIO_MMD_PCS_MV_TEMP_SENSOR3);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = ((ret & MDIO_MMD_PCS_MV_TEMP_SENSOR3_MASK) - 75) * 1000;

		tmp = FIELD_GET(MDIO_MMD_PCS_MV_TEMP_SENSOR3_MASK, ret);
		*val = (tmp - 75) * 1000;

> +		return 0;
> +	case hwmon_temp_max:
> +		ret = phy_read_mmd(phydev, MDIO_MMD_PCS,
> +				   MDIO_MMD_PCS_MV_TEMP_SENSOR3);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = (((ret & MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_MASK) >>
> +			MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_SHIFT) - 75) *
> +			1000;

		tmp = FIELD_GET(MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_MASK,
				ret);
		*val = (tmp - 75) * 1000;

> +		return 0;
> +	case hwmon_temp_alarm:
> +		ret = phy_read_mmd(phydev, MDIO_MMD_PCS,
> +				   MDIO_MMD_PCS_MV_TEMP_SENSOR1);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = !!(ret & MDIO_MMD_PCS_MV_TEMP_SENSOR1_RAW_INT);
> +		return 0;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int mv88q2xxx_hwmon_write(struct device *dev,
> +				 enum hwmon_sensor_types type, u32 attr,
> +				 int channel, long val)
> +{
> +	struct phy_device *phydev = dev_get_drvdata(dev);
> +
> +	switch (attr) {
> +	case hwmon_temp_max:
> +		if (val < -75000 || val > 180000)
> +			return -EINVAL;
> +
> +		val = ((val / 1000) + 75) <<
> +		       MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_SHIFT;

		val = (val / 1000) + 75;
		val = FIELD_PREP(MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_MASK,
				 val);

... and therefore no need for the _SHIFT constants.

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

Re: [PATCH v5 net-next 09/13] net: phy: marvell-88q2xxx: add cable test support

[Russell King (Oracle)]

On Mon, Jan 22, 2024 at 10:28:42PM +0100, Dimitri Fedrau wrote:
> +static int mv88q222x_cable_test_get_status(struct phy_device *phydev,
> +					   bool *finished)
> +{
> +	int ret;
> +	u32 dist;
> +
> +	ret = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_MMD_PCS_MV_TDR_STATUS);
> +	if (ret < 0)
> +		return ret;
> +
> +	*finished = true;
> +	/* fault length in meters */
> +	dist = ((ret & MDIO_MMD_PCS_MV_TDR_STATUS_DIST_MASK) >>
> +		MDIO_MMD_PCS_MV_TDR_STATUS_DIST_SHIFT) * 100;

	dist = FIELD_GET(MDIO_MMD_PCS_MV_TDR_STATUS_DIST_MASK, ret) * 100;


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

Re: [PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

[Dimitri Fedrau]

Am Tue, Jan 30, 2024 at 09:18:31AM +0000 schrieb Russell King (Oracle):
> On Mon, Jan 22, 2024 at 10:28:41PM +0100, Dimitri Fedrau wrote:
> 
> 	int tmp;
> 
> > +	switch (attr) {
> > +	case hwmon_temp_input:
> > +		ret = phy_read_mmd(phydev, MDIO_MMD_PCS,
> > +				   MDIO_MMD_PCS_MV_TEMP_SENSOR3);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		*val = ((ret & MDIO_MMD_PCS_MV_TEMP_SENSOR3_MASK) - 75) * 1000;
> 
> 		tmp = FIELD_GET(MDIO_MMD_PCS_MV_TEMP_SENSOR3_MASK, ret);
> 		*val = (tmp - 75) * 1000;
> 
> > +		return 0;
> > +	case hwmon_temp_max:
> > +		ret = phy_read_mmd(phydev, MDIO_MMD_PCS,
> > +				   MDIO_MMD_PCS_MV_TEMP_SENSOR3);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		*val = (((ret & MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_MASK) >>
> > +			MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_SHIFT) - 75) *
> > +			1000;
> 
> 		tmp = FIELD_GET(MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_MASK,
> 				ret);
> 		*val = (tmp - 75) * 1000;
> 
> > +		return 0;
> > +	case hwmon_temp_alarm:
> > +		ret = phy_read_mmd(phydev, MDIO_MMD_PCS,
> > +				   MDIO_MMD_PCS_MV_TEMP_SENSOR1);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		*val = !!(ret & MDIO_MMD_PCS_MV_TEMP_SENSOR1_RAW_INT);
> > +		return 0;
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +}
> > +
> > +static int mv88q2xxx_hwmon_write(struct device *dev,
> > +				 enum hwmon_sensor_types type, u32 attr,
> > +				 int channel, long val)
> > +{
> > +	struct phy_device *phydev = dev_get_drvdata(dev);
> > +
> > +	switch (attr) {
> > +	case hwmon_temp_max:
> > +		if (val < -75000 || val > 180000)
> > +			return -EINVAL;
> > +
> > +		val = ((val / 1000) + 75) <<
> > +		       MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_SHIFT;
> 
> 		val = (val / 1000) + 75;
> 		val = FIELD_PREP(MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_MASK,
> 				 val);
> 
> ... and therefore no need for the _SHIFT constants.
>
Will fix it.
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v5 net-next 09/13] net: phy: marvell-88q2xxx: add cable test support

[Dimitri Fedrau]

Am Tue, Jan 30, 2024 at 09:19:30AM +0000 schrieb Russell King (Oracle):
> On Mon, Jan 22, 2024 at 10:28:42PM +0100, Dimitri Fedrau wrote:
> > +static int mv88q222x_cable_test_get_status(struct phy_device *phydev,
> > +					   bool *finished)
> > +{
> > +	int ret;
> > +	u32 dist;
> > +
> > +	ret = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_MMD_PCS_MV_TDR_STATUS);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	*finished = true;
> > +	/* fault length in meters */
> > +	dist = ((ret & MDIO_MMD_PCS_MV_TDR_STATUS_DIST_MASK) >>
> > +		MDIO_MMD_PCS_MV_TDR_STATUS_DIST_SHIFT) * 100;
> 
> 	dist = FIELD_GET(MDIO_MMD_PCS_MV_TDR_STATUS_DIST_MASK, ret) * 100;
>
Hi Russell,

sure can fix it. Did you have a chance to review the other patches ? Is
there anything I could do to ease the review process ? I'm new to
submitting patches, maybe I crossed some line without knowing ? Would be
glad to get some feedback. I'm just asking because there was not much
feedback besides your comments.

Best regards,
Dimitri

Re: [PATCH v5 net-next 05/13] net: phy: marvell-88q2xxx: add driver for the Marvell 88Q2220 PHY

[Andrew Lunn]

On Mon, Jan 22, 2024 at 10:28:38PM +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. Autonegotiation is supported.
> 
> Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>

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

    Andrew

Re: [PATCH v5 net-next 06/13] net: phy: marvell-88q2xxx: add interrupt support for link detection

[Andrew Lunn]

On Mon, Jan 22, 2024 at 10:28:39PM +0100, Dimitri Fedrau wrote:
> Added .config_intr and .handle_interrupt callbacks. Whenever the link
> goes up or down an interrupt will be triggered. Interrupts are configured
> separately for 100/1000BASET1.
> 
> Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>

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

    Andrew

Re: [PATCH v5 net-next 07/13] net: phy: marvell-88q2xxx: add suspend / resume ops

[Andrew Lunn]

On Mon, Jan 22, 2024 at 10:28:40PM +0100, Dimitri Fedrau wrote:
> Add suspend/resume ops for Marvell 88Q2xxx devices.
> 
> Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>

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

    Andrew

Re: [PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

[Andrew Lunn]

> +static int mv88q2xxx_hwmon_probe(struct phy_device *phydev)
> +{
> +	struct device *dev = &phydev->mdio.dev;
> +	struct device *hwmon;
> +	char *hwmon_name;
> +	int ret;
> +
> +	/* Enable temperature sensor interrupt */
> +	ret = phy_set_bits_mmd(phydev, MDIO_MMD_PCS,
> +			       MDIO_MMD_PCS_MV_TEMP_SENSOR1,
> +			       MDIO_MMD_PCS_MV_TEMP_SENSOR1_INT_EN);

You enable an interrupt, but i don't see any changes to the interrupt
handler to handle any interrupts which are generated?

	Andrew

Re: [PATCH v5 net-next 10/13] net: phy: marvell-88q2xxx: make mv88q2xxx_config_aneg generic

[Andrew Lunn]

On Mon, Jan 22, 2024 at 10:28:43PM +0100, Dimitri Fedrau wrote:
> Marvell 88Q2xxx devices follow the same scheme, after configuration they
> need a soft reset. Soft resets differ between devices, so we use the
> .soft_reset callback instead of creating .config_aneg callbacks for each
> device.
> 
> Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>

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

    Andrew

Re: [PATCH v5 net-next 11/13] net: phy: marvell-88q2xxx: switch to mv88q2xxx_config_aneg

[Andrew Lunn]

On Mon, Jan 22, 2024 at 10:28:44PM +0100, Dimitri Fedrau wrote:
> Switch to mv88q2xxx_config_aneg for Marvell 88Q2220 devices and remove
> the mv88q222x_config_aneg function which is basically a copy of the
> mv88q2xxx_config_aneg function.
> 
> Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>

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

    Andrew

Re: [PATCH v5 net-next 12/13] net: phy: marvell-88q2xxx: cleanup mv88q2xxx_config_init

[Andrew Lunn]

On Mon, Jan 22, 2024 at 10:28:45PM +0100, Dimitri Fedrau wrote:
> mv88q2xxx_config_init calls genphy_c45_read_pma which is done by
> mv88q2xxx_read_status, it calls also mv88q2xxx_config_aneg which is
> also called by the PHY state machine. Let the PHY state machine handle
> the phydriver ops in their intendend way.
> 
> Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>

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

    Andrew

Re: [PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

[Dimitri Fedrau]

Am Wed, Jan 31, 2024 at 04:17:06PM +0100 schrieb Andrew Lunn:
> > +static int mv88q2xxx_hwmon_probe(struct phy_device *phydev)
> > +{
> > +	struct device *dev = &phydev->mdio.dev;
> > +	struct device *hwmon;
> > +	char *hwmon_name;
> > +	int ret;
> > +
> > +	/* Enable temperature sensor interrupt */
> > +	ret = phy_set_bits_mmd(phydev, MDIO_MMD_PCS,
> > +			       MDIO_MMD_PCS_MV_TEMP_SENSOR1,
> > +			       MDIO_MMD_PCS_MV_TEMP_SENSOR1_INT_EN);
> 
> You enable an interrupt, but i don't see any changes to the interrupt
> handler to handle any interrupts which are generated?
>
Hi Andrew,

you are right. Have to remove these lines. Besides enabling the interrupt
in MDIO_MMD_PCS_MV_TEMP_SENSOR1, there are two further register writes
necessary to make the interrupt propagate. I didn't want it to propagate.
Anyway it's wrong. I couldn't find a good solution to use the temperature
interrupt. Will have a look into this, and probably figuring out how to
do so. But it won't be part of this patch series.

Dimitri

Re: [PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

[Guenter Roeck]

On 1/22/24 13:28, Dimitri Fedrau wrote:
> Marvell 88q2xxx devices have an inbuilt temperature sensor. Add hwmon
> support for this sensor.
> 
> Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
> ---
>   drivers/net/phy/marvell-88q2xxx.c | 152 ++++++++++++++++++++++++++++++
>   1 file changed, 152 insertions(+)
> 
> diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
> index 4cb8fe524795..6900bad275d0 100644
> --- a/drivers/net/phy/marvell-88q2xxx.c
> +++ b/drivers/net/phy/marvell-88q2xxx.c
> @@ -5,6 +5,7 @@
>   #include <linux/ethtool_netlink.h>
>   #include <linux/marvell_phy.h>
>   #include <linux/phy.h>
> +#include <linux/hwmon.h>
>   
>   #define PHY_ID_88Q2220_REVB0	(MARVELL_PHY_ID_88Q2220 | 0x1)
>   
> @@ -33,6 +34,19 @@
>   #define MDIO_MMD_PCS_MV_GPIO_INT_CTRL			32787
>   #define MDIO_MMD_PCS_MV_GPIO_INT_CTRL_TRI_DIS		0x0800
>   
> +#define MDIO_MMD_PCS_MV_TEMP_SENSOR1			32833
> +#define MDIO_MMD_PCS_MV_TEMP_SENSOR1_RAW_INT		0x0001
> +#define MDIO_MMD_PCS_MV_TEMP_SENSOR1_INT		0x0040
> +#define MDIO_MMD_PCS_MV_TEMP_SENSOR1_INT_EN		0x0080
> +
> +#define MDIO_MMD_PCS_MV_TEMP_SENSOR2			32834
> +#define MDIO_MMD_PCS_MV_TEMP_SENSOR2_DIS_MASK		0xc000
> +
> +#define MDIO_MMD_PCS_MV_TEMP_SENSOR3			32835
> +#define MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_MASK	0xff00
> +#define MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_SHIFT	8
> +#define MDIO_MMD_PCS_MV_TEMP_SENSOR3_MASK		0x00ff
> +
>   #define MDIO_MMD_PCS_MV_100BT1_STAT1			33032
>   #define MDIO_MMD_PCS_MV_100BT1_STAT1_IDLE_ERROR		0x00ff
>   #define MDIO_MMD_PCS_MV_100BT1_STAT1_JABBER		0x0100
> @@ -488,6 +502,143 @@ static int mv88q2xxx_resume(struct phy_device *phydev)
>   	return phy_clear_bits_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1,
>   				  MDIO_CTRL1_LPOWER);
>   }
> +#ifdef CONFIG_HWMON

HWMON is tristate, so this may be problematic if the driver is built
into the kernel and hwmon is built as module.

[ ... ]
> +
> +static int mv88q2xxx_hwmon_write(struct device *dev,
> +				 enum hwmon_sensor_types type, u32 attr,
> +				 int channel, long val)
> +{
> +	struct phy_device *phydev = dev_get_drvdata(dev);
> +
> +	switch (attr) {
> +	case hwmon_temp_max:
> +		if (val < -75000 || val > 180000)
> +			return -EINVAL;
> +

Not that it matters much, but we typically use clamp_val() to limit
the range of temperature limits because the valid range differs for
each chip and is otherwise difficult to determine for the user.

Guenter

Re: [PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

[Andrew Lunn]

> Anyway it's wrong. I couldn't find a good solution to use the temperature
> interrupt. Will have a look into this, and probably figuring out how to
> do so. But it won't be part of this patch series.

I don't know of any PHY driver you can follow, those that do have a
temperature sensor just report the temperature and don't do anything
in addition.

You might need to look at thermal zones, and indicate there has been a
thermal trip point. That could then be used by the thermal subsystem
to increase cooling via a fan, etc. In theory, you could also make the
PHY active to thermal pressure, by forcing the link to renegotiate to
a lower link speed. If you decide to go this route, please try to make
is generic to any PHY. But its going to be quite a disruptive thing,
the link will be lost of a little over a second...

   Andrew

Re: [PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

[Andrew Lunn]

> > +#ifdef CONFIG_HWMON
> 
> HWMON is tristate, so this may be problematic if the driver is built
> into the kernel and hwmon is built as module.

There should be Kconfig in addition to this, e.g.

config MAXLINEAR_GPHY
        tristate "Maxlinear Ethernet PHYs"
        select POLYNOMIAL if HWMON
        depends on HWMON || HWMON=n
        help
          Support for the Maxlinear GPY115, GPY211, GPY212, GPY215,
          GPY241, GPY245 PHYs.

So its forced to being built in, or not built at all.

	Andrew

Re: [PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

[Guenter Roeck]

On 1/31/24 23:11, Dimitri Fedrau wrote:
> Am Wed, Jan 31, 2024 at 04:17:06PM +0100 schrieb Andrew Lunn:
>>> +static int mv88q2xxx_hwmon_probe(struct phy_device *phydev)
>>> +{
>>> +	struct device *dev = &phydev->mdio.dev;
>>> +	struct device *hwmon;
>>> +	char *hwmon_name;
>>> +	int ret;
>>> +
>>> +	/* Enable temperature sensor interrupt */
>>> +	ret = phy_set_bits_mmd(phydev, MDIO_MMD_PCS,
>>> +			       MDIO_MMD_PCS_MV_TEMP_SENSOR1,
>>> +			       MDIO_MMD_PCS_MV_TEMP_SENSOR1_INT_EN);
>>
>> You enable an interrupt, but i don't see any changes to the interrupt
>> handler to handle any interrupts which are generated?
>>
> Hi Andrew,
> 
> you are right. Have to remove these lines. Besides enabling the interrupt
> in MDIO_MMD_PCS_MV_TEMP_SENSOR1, there are two further register writes
> necessary to make the interrupt propagate. I didn't want it to propagate.
> Anyway it's wrong. I couldn't find a good solution to use the temperature
> interrupt. Will have a look into this, and probably figuring out how to
> do so. But it won't be part of this patch series.
> 

 From hwmon perspective, the expected use of such an interrupt would be
to call hwmon_notify_event() with the affected limit attribute as argument.
This would notify the thermal subsystem if the sensor is registered with it
(your patch doesn't set the necessary flag when registering the driver,
so this would not happen), it will send a notification to the sysfs
attribute, and generate a udev event.

Guenter

Re: [PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

[Guenter Roeck]

On 2/1/24 05:27, Andrew Lunn wrote:
>>> +#ifdef CONFIG_HWMON
>>
>> HWMON is tristate, so this may be problematic if the driver is built
>> into the kernel and hwmon is built as module.
> 
> There should be Kconfig in addition to this, e.g.
> 
> config MAXLINEAR_GPHY
>          tristate "Maxlinear Ethernet PHYs"
>          select POLYNOMIAL if HWMON
>          depends on HWMON || HWMON=n
>          help
>            Support for the Maxlinear GPY115, GPY211, GPY212, GPY215,
>            GPY241, GPY245 PHYs.
> 
> So its forced to being built in, or not built at all.
> 

Even then it should be "#if IS_ENABLED(HWMON)" in the code.

Thanks,
Guenter

Re: [PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

[Dimitri Fedrau]

Am Thu, Feb 01, 2024 at 05:34:05AM -0800 schrieb Guenter Roeck:
> On 1/31/24 23:11, Dimitri Fedrau wrote:
> > Am Wed, Jan 31, 2024 at 04:17:06PM +0100 schrieb Andrew Lunn:
> > > > +static int mv88q2xxx_hwmon_probe(struct phy_device *phydev)
> > > > +{
> > > > +	struct device *dev = &phydev->mdio.dev;
> > > > +	struct device *hwmon;
> > > > +	char *hwmon_name;
> > > > +	int ret;
> > > > +
> > > > +	/* Enable temperature sensor interrupt */
> > > > +	ret = phy_set_bits_mmd(phydev, MDIO_MMD_PCS,
> > > > +			       MDIO_MMD_PCS_MV_TEMP_SENSOR1,
> > > > +			       MDIO_MMD_PCS_MV_TEMP_SENSOR1_INT_EN);
> > > 
> > > You enable an interrupt, but i don't see any changes to the interrupt
> > > handler to handle any interrupts which are generated?
> > > 
> > Hi Andrew,
> > 
> > you are right. Have to remove these lines. Besides enabling the interrupt
> > in MDIO_MMD_PCS_MV_TEMP_SENSOR1, there are two further register writes
> > necessary to make the interrupt propagate. I didn't want it to propagate.
> > Anyway it's wrong. I couldn't find a good solution to use the temperature
> > interrupt. Will have a look into this, and probably figuring out how to
> > do so. But it won't be part of this patch series.
> > 
> 
> From hwmon perspective, the expected use of such an interrupt would be
> to call hwmon_notify_event() with the affected limit attribute as argument.
> This would notify the thermal subsystem if the sensor is registered with it
> (your patch doesn't set the necessary flag when registering the driver,
> so this would not happen), it will send a notification to the sysfs
> attribute, and generate a udev event.
>
Thanks, noted it down. Didn't know about the notification to the thermal
subsystem and the generated udev event. :)

Dimitri

Re: [PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

[Dimitri Fedrau]

Am Thu, Feb 01, 2024 at 05:18:23AM -0800 schrieb Guenter Roeck:
[...]
> > +
> > +static int mv88q2xxx_hwmon_write(struct device *dev,
> > +				 enum hwmon_sensor_types type, u32 attr,
> > +				 int channel, long val)
> > +{
> > +	struct phy_device *phydev = dev_get_drvdata(dev);
> > +
> > +	switch (attr) {
> > +	case hwmon_temp_max:
> > +		if (val < -75000 || val > 180000)
> > +			return -EINVAL;
> > +
> 
> Not that it matters much, but we typically use clamp_val() to limit
> the range of temperature limits because the valid range differs for
> each chip and is otherwise difficult to determine for the user.
>

Will fix it.

Dimitri

Re: [PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

[Dimitri Fedrau]

Am Thu, Feb 01, 2024 at 05:39:25AM -0800 schrieb Guenter Roeck:
> On 2/1/24 05:27, Andrew Lunn wrote:
> > > > +#ifdef CONFIG_HWMON
> > > 
> > > HWMON is tristate, so this may be problematic if the driver is built
> > > into the kernel and hwmon is built as module.
> > 
> > There should be Kconfig in addition to this, e.g.
> > 
> > config MAXLINEAR_GPHY
> >          tristate "Maxlinear Ethernet PHYs"
> >          select POLYNOMIAL if HWMON
> >          depends on HWMON || HWMON=n
> >          help
> >            Support for the Maxlinear GPY115, GPY211, GPY212, GPY215,
> >            GPY241, GPY245 PHYs.
> > 
> > So its forced to being built in, or not built at all.
> > 
> 
> Even then it should be "#if IS_ENABLED(HWMON)" in the code.
> 
>
If using "#if IS_ENABLED(HWMON)" do I have to add the dependency in
the KConfig file ? When looking at other PHY drivers, they do.

Dimitri

Re: [PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

[Andrew Lunn]

On Thu, Feb 01, 2024 at 05:23:49PM +0100, Dimitri Fedrau wrote:
> Am Thu, Feb 01, 2024 at 05:39:25AM -0800 schrieb Guenter Roeck:
> > On 2/1/24 05:27, Andrew Lunn wrote:
> > > > > +#ifdef CONFIG_HWMON
> > > > 
> > > > HWMON is tristate, so this may be problematic if the driver is built
> > > > into the kernel and hwmon is built as module.
> > > 
> > > There should be Kconfig in addition to this, e.g.
> > > 
> > > config MAXLINEAR_GPHY
> > >          tristate "Maxlinear Ethernet PHYs"
> > >          select POLYNOMIAL if HWMON
> > >          depends on HWMON || HWMON=n
> > >          help
> > >            Support for the Maxlinear GPY115, GPY211, GPY212, GPY215,
> > >            GPY241, GPY245 PHYs.
> > > 
> > > So its forced to being built in, or not built at all.
> > > 
> > 
> > Even then it should be "#if IS_ENABLED(HWMON)" in the code.
> > 
> >
> If using "#if IS_ENABLED(HWMON)" do I have to add the dependency in
> the KConfig file ? When looking at other PHY drivers, they do.

Please follow what other drivers do. Its easy to break the build,
resulting is undefined symbols. What we have now works.

	Andrew	  

Re: [PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

[Dimitri Fedrau]

Am Thu, Feb 01, 2024 at 02:23:15PM +0100 schrieb Andrew Lunn:
> > Anyway it's wrong. I couldn't find a good solution to use the temperature
> > interrupt. Will have a look into this, and probably figuring out how to
> > do so. But it won't be part of this patch series.
> 
> I don't know of any PHY driver you can follow, those that do have a
> temperature sensor just report the temperature and don't do anything
> in addition.
> 
> You might need to look at thermal zones, and indicate there has been a
> thermal trip point. That could then be used by the thermal subsystem
> to increase cooling via a fan, etc. In theory, you could also make the
> PHY active to thermal pressure, by forcing the link to renegotiate to
> a lower link speed. If you decide to go this route, please try to make
> is generic to any PHY. But its going to be quite a disruptive thing,
> the link will be lost of a little over a second...
> 
Making the PHY active to thermal pressure sounds interesting. Will look
into this.

Re: [PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

[Dimitri Fedrau]

Am Thu, Feb 01, 2024 at 05:47:21PM +0100 schrieb Andrew Lunn:
> On Thu, Feb 01, 2024 at 05:23:49PM +0100, Dimitri Fedrau wrote:
> > Am Thu, Feb 01, 2024 at 05:39:25AM -0800 schrieb Guenter Roeck:
> > > On 2/1/24 05:27, Andrew Lunn wrote:
> > > > > > +#ifdef CONFIG_HWMON
> > > > > 
> > > > > HWMON is tristate, so this may be problematic if the driver is built
> > > > > into the kernel and hwmon is built as module.
> > > > 
> > > > There should be Kconfig in addition to this, e.g.
> > > > 
> > > > config MAXLINEAR_GPHY
> > > >          tristate "Maxlinear Ethernet PHYs"
> > > >          select POLYNOMIAL if HWMON
> > > >          depends on HWMON || HWMON=n
> > > >          help
> > > >            Support for the Maxlinear GPY115, GPY211, GPY212, GPY215,
> > > >            GPY241, GPY245 PHYs.
> > > > 
> > > > So its forced to being built in, or not built at all.
> > > > 
> > > 
> > > Even then it should be "#if IS_ENABLED(HWMON)" in the code.
> > > 
> > >
> > If using "#if IS_ENABLED(HWMON)" do I have to add the dependency in
> > the KConfig file ? When looking at other PHY drivers, they do.
> 
> Please follow what other drivers do. Its easy to break the build,
> resulting is undefined symbols. What we have now works.

Sure.

Re: [PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

[Guenter Roeck]

On Thu, Feb 01, 2024 at 05:23:49PM +0100, Dimitri Fedrau wrote:
> Am Thu, Feb 01, 2024 at 05:39:25AM -0800 schrieb Guenter Roeck:
> > On 2/1/24 05:27, Andrew Lunn wrote:
> > > > > +#ifdef CONFIG_HWMON
> > > > 
> > > > HWMON is tristate, so this may be problematic if the driver is built
> > > > into the kernel and hwmon is built as module.
> > > 
> > > There should be Kconfig in addition to this, e.g.
> > > 
> > > config MAXLINEAR_GPHY
> > >          tristate "Maxlinear Ethernet PHYs"
> > >          select POLYNOMIAL if HWMON
> > >          depends on HWMON || HWMON=n
> > >          help
> > >            Support for the Maxlinear GPY115, GPY211, GPY212, GPY215,
> > >            GPY241, GPY245 PHYs.
> > > 
> > > So its forced to being built in, or not built at all.
> > > 
> > 
> > Even then it should be "#if IS_ENABLED(HWMON)" in the code.
> > 
> >
> If using "#if IS_ENABLED(HWMON)" do I have to add the dependency in
> the KConfig file ? When looking at other PHY drivers, they do.

Yes, to handle CONFIG_HWMON=m. Note that it is "IS_ENABLED(CONFIG_HWMON)"
      				                           ^^^^^^^

Guenter

Re: [PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

[Guenter Roeck]

On Thu, Feb 01, 2024 at 05:14:35PM +0100, Dimitri Fedrau wrote:
> Am Thu, Feb 01, 2024 at 05:34:05AM -0800 schrieb Guenter Roeck:
> > On 1/31/24 23:11, Dimitri Fedrau wrote:
> > > Am Wed, Jan 31, 2024 at 04:17:06PM +0100 schrieb Andrew Lunn:
> > > > > +static int mv88q2xxx_hwmon_probe(struct phy_device *phydev)
> > > > > +{
> > > > > +	struct device *dev = &phydev->mdio.dev;
> > > > > +	struct device *hwmon;
> > > > > +	char *hwmon_name;
> > > > > +	int ret;
> > > > > +
> > > > > +	/* Enable temperature sensor interrupt */
> > > > > +	ret = phy_set_bits_mmd(phydev, MDIO_MMD_PCS,
> > > > > +			       MDIO_MMD_PCS_MV_TEMP_SENSOR1,
> > > > > +			       MDIO_MMD_PCS_MV_TEMP_SENSOR1_INT_EN);
> > > > 
> > > > You enable an interrupt, but i don't see any changes to the interrupt
> > > > handler to handle any interrupts which are generated?
> > > > 
> > > Hi Andrew,
> > > 
> > > you are right. Have to remove these lines. Besides enabling the interrupt
> > > in MDIO_MMD_PCS_MV_TEMP_SENSOR1, there are two further register writes
> > > necessary to make the interrupt propagate. I didn't want it to propagate.
> > > Anyway it's wrong. I couldn't find a good solution to use the temperature
> > > interrupt. Will have a look into this, and probably figuring out how to
> > > do so. But it won't be part of this patch series.
> > > 
> > 
> > From hwmon perspective, the expected use of such an interrupt would be
> > to call hwmon_notify_event() with the affected limit attribute as argument.
> > This would notify the thermal subsystem if the sensor is registered with it
> > (your patch doesn't set the necessary flag when registering the driver,
> > so this would not happen), it will send a notification to the sysfs
> > attribute, and generate a udev event.
> >
> Thanks, noted it down. Didn't know about the notification to the thermal
> subsystem and the generated udev event. :)
> 

Note that you'd have to add something like

	HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),

to the code to register the sensor as thermal zone.

Guenter

Re: [PATCH v5 net-next 12/13] net: phy: marvell-88q2xxx: cleanup mv88q2xxx_config_init

[Dimitri Fedrau]

Am Wed, Jan 31, 2024 at 04:21:44PM +0100 schrieb Andrew Lunn:
> On Mon, Jan 22, 2024 at 10:28:45PM +0100, Dimitri Fedrau wrote:
> > mv88q2xxx_config_init calls genphy_c45_read_pma which is done by
> > mv88q2xxx_read_status, it calls also mv88q2xxx_config_aneg which is
> > also called by the PHY state machine. Let the PHY state machine handle
> > the phydriver ops in their intendend way.
> > 
> > Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
>
Thanks, for reviewing. Is there a chance that you also have a look on
patch 13 of the series ? It relies on this patch.

Re: [PATCH v5 net-next 00/13] net: phy: marvell-88q2xxx: add driver for the Marvell 88Q2220 PHY

[Dimitri Fedrau]

[...]

Probably late, but there are parts of the code which are based on the
sample code provided by Marvell. The sample code is licensed under BSD
2 Clause. Should I change the license in the driver to dual license ?

Best regards,
Dimitri

Re: [PATCH v5 net-next 00/13] net: phy: marvell-88q2xxx: add driver for the Marvell 88Q2220 PHY

[Andrew Lunn]

On Fri, Feb 02, 2024 at 07:30:29PM +0100, Dimitri Fedrau wrote:
> [...]
> 
> Probably late, but there are parts of the code which are based on the
> sample code provided by Marvell. The sample code is licensed under BSD
> 2 Clause. Should I change the license in the driver to dual license ?

IANAL

You can take BSD code and release it with a GPL license. But it would
be better to indicate it is derived from BSD code. Either make it dual
license, or add a comment about the origin of the code.

	 Andrew

Re: [PATCH v5 net-next 00/13] net: phy: marvell-88q2xxx: add driver for the Marvell 88Q2220 PHY

[Dimitri Fedrau]

Am Sat, Feb 03, 2024 at 05:07:13PM +0100 schrieb Andrew Lunn:
> On Fri, Feb 02, 2024 at 07:30:29PM +0100, Dimitri Fedrau wrote:
> > [...]
> > 
> > Probably late, but there are parts of the code which are based on the
> > sample code provided by Marvell. The sample code is licensed under BSD
> > 2 Clause. Should I change the license in the driver to dual license ?
> 
> IANAL
> 
> You can take BSD code and release it with a GPL license. But it would
> be better to indicate it is derived from BSD code. Either make it dual
> license, or add a comment about the origin of the code.
>
Is there a reason why the "MODULE_AUTHOR" is not in the driver and it's
also missing in marvell-88x2222.c. I also don't see any copyright
information. I'm just curious, not seeking for any legal advice. :) I
just have to get this figured out to stay out of trouble.

Dimitri