[PATCH v1 0/2] Add a driver for the Marvell 88Q2110 PHY

[PATCH v1 0/2] Add a driver for the Marvell 88Q2110 PHY

[Stefan Eichenberger]

Add support for 1000BASE-T1 to the phy_device driver and add a first
1000BASE-T1 driver for the Marvell 88Q2110 PHY.

Stefan Eichenberger (2):
  net: phy: add the link modes for 1000BASE-T1 Ethernet PHY
  net: phy: marvell-88q2xxx: add driver for the Marvell 88Q2110 PHY

 drivers/net/phy/Kconfig           |   6 +
 drivers/net/phy/Makefile          |   1 +
 drivers/net/phy/marvell-88q2xxx.c | 217 ++++++++++++++++++++++++++++++
 drivers/net/phy/phy_device.c      |  14 ++
 include/linux/phy.h               |   2 +
 5 files changed, 240 insertions(+)
 create mode 100644 drivers/net/phy/marvell-88q2xxx.c

-- 
2.39.2

[PATCH v1 1/2] net: phy: add the link modes for 1000BASE-T1 Ethernet PHY

[Stefan Eichenberger]

This patch adds the link modes for the 1000BASE-T1 Ethernet PHYs. It
supports 100BASE-T1/1000BASE-T1 in full duplex mode. So far I could not
find a 1000BASE-T1 PHY that also supports 10BASE-T1, so this mode is not
added.

Signed-off-by: Stefan Eichenberger <eichest@gmail.com>
---
 drivers/net/phy/phy_device.c | 14 ++++++++++++++
 include/linux/phy.h          |  2 ++
 2 files changed, 16 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 17d0d0555a79f..4492dbd525f4f 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -50,6 +50,9 @@ EXPORT_SYMBOL_GPL(phy_basic_t1_features);
 __ETHTOOL_DECLARE_LINK_MODE_MASK(phy_basic_t1s_p2mp_features) __ro_after_init;
 EXPORT_SYMBOL_GPL(phy_basic_t1s_p2mp_features);
 
+__ETHTOOL_DECLARE_LINK_MODE_MASK(phy_gbit_t1_features) __ro_after_init;
+EXPORT_SYMBOL_GPL(phy_gbit_t1_features);
+
 __ETHTOOL_DECLARE_LINK_MODE_MASK(phy_gbit_features) __ro_after_init;
 EXPORT_SYMBOL_GPL(phy_gbit_features);
 
@@ -109,6 +112,13 @@ const int phy_basic_t1s_p2mp_features_array[2] = {
 };
 EXPORT_SYMBOL_GPL(phy_basic_t1s_p2mp_features_array);
 
+const int phy_gbit_t1_features_array[3] = {
+	ETHTOOL_LINK_MODE_TP_BIT,
+	ETHTOOL_LINK_MODE_100baseT1_Full_BIT,
+	ETHTOOL_LINK_MODE_1000baseT1_Full_BIT,
+};
+EXPORT_SYMBOL_GPL(phy_gbit_t1_features_array);
+
 const int phy_gbit_features_array[2] = {
 	ETHTOOL_LINK_MODE_1000baseT_Half_BIT,
 	ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
@@ -165,6 +175,10 @@ static void features_init(void)
 	linkmode_set_bit_array(phy_basic_t1s_p2mp_features_array,
 			       ARRAY_SIZE(phy_basic_t1s_p2mp_features_array),
 			       phy_basic_t1s_p2mp_features);
+	/* 1000 full, TP */
+	linkmode_set_bit_array(phy_gbit_t1_features_array,
+			       ARRAY_SIZE(phy_gbit_t1_features_array),
+			       phy_gbit_t1_features);
 
 	/* 10/100 half/full + 1000 half/full */
 	linkmode_set_bit_array(phy_basic_ports_array,
diff --git a/include/linux/phy.h b/include/linux/phy.h
index c5a0dc829714f..81b6731a85a86 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -47,6 +47,7 @@
 extern __ETHTOOL_DECLARE_LINK_MODE_MASK(phy_basic_features) __ro_after_init;
 extern __ETHTOOL_DECLARE_LINK_MODE_MASK(phy_basic_t1_features) __ro_after_init;
 extern __ETHTOOL_DECLARE_LINK_MODE_MASK(phy_basic_t1s_p2mp_features) __ro_after_init;
+extern __ETHTOOL_DECLARE_LINK_MODE_MASK(phy_gbit_t1_features) __ro_after_init;
 extern __ETHTOOL_DECLARE_LINK_MODE_MASK(phy_gbit_features) __ro_after_init;
 extern __ETHTOOL_DECLARE_LINK_MODE_MASK(phy_gbit_fibre_features) __ro_after_init;
 extern __ETHTOOL_DECLARE_LINK_MODE_MASK(phy_gbit_all_ports_features) __ro_after_init;
@@ -58,6 +59,7 @@ extern __ETHTOOL_DECLARE_LINK_MODE_MASK(phy_eee_cap1_features) __ro_after_init;
 #define PHY_BASIC_FEATURES ((unsigned long *)&phy_basic_features)
 #define PHY_BASIC_T1_FEATURES ((unsigned long *)&phy_basic_t1_features)
 #define PHY_BASIC_T1S_P2MP_FEATURES ((unsigned long *)&phy_basic_t1s_p2mp_features)
+#define PHY_GBIT_T1_FEATURES ((unsigned long *)&phy_gbit_t1_features)
 #define PHY_GBIT_FEATURES ((unsigned long *)&phy_gbit_features)
 #define PHY_GBIT_FIBRE_FEATURES ((unsigned long *)&phy_gbit_fibre_features)
 #define PHY_GBIT_ALL_PORTS_FEATURES ((unsigned long *)&phy_gbit_all_ports_features)
-- 
2.39.2

[PATCH v1 2/2] net: phy: marvell-88q2xxx: add driver for the Marvell 88Q2110 PHY

[Stefan Eichenberger]

Add a driver for the Marvell 88Q2110. This driver is minimalistic, but
already allows to detect the link, switch between 100BASE-T1 and
1000BASE-T1 and switch between master and slave mode. Autonegotiation
supported by the PHY is not yet implemented.

Signed-off-by: Stefan Eichenberger <eichest@gmail.com>
---
 drivers/net/phy/Kconfig           |   6 +
 drivers/net/phy/Makefile          |   1 +
 drivers/net/phy/marvell-88q2xxx.c | 217 ++++++++++++++++++++++++++++++
 3 files changed, 224 insertions(+)
 create mode 100644 drivers/net/phy/marvell-88q2xxx.c

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 93b8efc792273..2913b145d5406 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -223,6 +223,12 @@ config MARVELL_88X2222_PHY
 	  Support for the Marvell 88X2222 Dual-port Multi-speed Ethernet
 	  Transceiver.
 
+config MARVELL_88Q2XXX_PHY
+	tristate "Marvell 88Q2XXX PHY"
+	help
+	  Support for the Marvell Automotive 88Q2XXX 100/1000BASE-T1 Ethernet
+	  PHYs.
+
 config MAXLINEAR_GPHY
 	tristate "Maxlinear Ethernet PHYs"
 	select POLYNOMIAL if HWMON
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index f289ab16a1dab..15d1908fd5cb7 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_LXT_PHY)		+= lxt.o
 obj-$(CONFIG_MARVELL_10G_PHY)	+= marvell10g.o
 obj-$(CONFIG_MARVELL_PHY)	+= marvell.o
 obj-$(CONFIG_MARVELL_88X2222_PHY)	+= marvell-88x2222.o
+obj-$(CONFIG_MARVELL_88Q2XXX_PHY)	+= marvell-88q2xxx.o
 obj-$(CONFIG_MAXLINEAR_GPHY)	+= mxl-gpy.o
 obj-$(CONFIG_MEDIATEK_GE_PHY)	+= mediatek-ge.o
 obj-$(CONFIG_MESON_GXL_PHY)	+= meson-gxl.o
diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
new file mode 100644
index 0000000000000..637697e9f19fa
--- /dev/null
+++ b/drivers/net/phy/marvell-88q2xxx.c
@@ -0,0 +1,217 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Marvell 88Q2XXX automotive 100BASE-T1/1000BASE-T1 PHY driver
+ */
+
+#include <linux/marvell_phy.h>
+#include <linux/phy.h>
+#include <linux/ethtool_netlink.h>
+
+#define MARVELL_PHY_ID_88Q2110		0x002b0981
+
+static int mv88q2xxx_soft_reset(struct phy_device *phydev)
+{
+	phy_write_mmd(phydev, 3, 0x0900, 0x8000);
+
+	return 0;
+}
+
+static void init_fast_ethernet(struct phy_device *phydev)
+{
+	u16 value = phy_read_mmd(phydev, 1, 0x0834);
+
+	value = value & 0xFFF0;
+
+	phy_write_mmd(phydev, 1, 0x0834, value);
+}
+
+static void init_gbit_ethernet(struct phy_device *phydev)
+{
+	u16 value = phy_read_mmd(phydev, 1, 0x0834);
+
+	value = (value & 0xFFF0) | 0x0001;
+
+	phy_write_mmd(phydev, 1, 0x0834, value);
+}
+
+static int setup_master_slave(struct phy_device *phydev)
+{
+	u16 reg_data = phy_read_mmd(phydev, 1, 0x0834);
+
+	switch (phydev->master_slave_set) {
+	case MASTER_SLAVE_CFG_MASTER_FORCE:
+	case MASTER_SLAVE_CFG_MASTER_PREFERRED:
+		reg_data |= 0x4000;
+		break;
+	case MASTER_SLAVE_CFG_SLAVE_PREFERRED:
+	case MASTER_SLAVE_CFG_SLAVE_FORCE:
+		reg_data &= ~0x4000;
+		break;
+	case MASTER_SLAVE_CFG_UNKNOWN:
+	case MASTER_SLAVE_CFG_UNSUPPORTED:
+		return 0;
+	default:
+		phydev_warn(phydev, "Unsupported Master/Slave mode\n");
+		return -EOPNOTSUPP;
+	}
+
+	phy_write_mmd(phydev, 1, 0x0834, reg_data);
+
+	return 0;
+}
+
+static int mv88q2xxx_config_aneg(struct phy_device *phydev)
+{
+	int ret;
+
+	if (phydev->speed == SPEED_100)
+		init_fast_ethernet(phydev);
+	else if (phydev->speed == SPEED_1000)
+		init_gbit_ethernet(phydev);
+
+	ret = setup_master_slave(phydev);
+	if (ret)
+		return ret;
+
+	mv88q2xxx_soft_reset(phydev);
+
+	return 0;
+}
+
+static int mv88q2xxx_config_init(struct phy_device *phydev)
+{
+	return mv88q2xxx_config_aneg(phydev);
+}
+
+static int get_speed(struct phy_device *phydev)
+{
+	u16 value = 0;
+
+	if (phydev->autoneg)
+		value = (phy_read_mmd(phydev, 7, 0x801a) & 0x4000) >> 14;
+	else
+		value = (phy_read_mmd(phydev, 1, 0x0834) & 0x1);
+
+	return value ? SPEED_1000 : SPEED_100;
+}
+
+static int check_link(struct phy_device *phydev)
+{
+	u16 ret1, ret2;
+
+	if (phydev->speed == SPEED_1000) {
+		ret1 = phy_read_mmd(phydev, 3, 0x0901);
+		ret1 = phy_read_mmd(phydev, 3, 0x0901);
+		ret2 = phy_read_mmd(phydev, 7, 0x8001);
+	} else {
+		ret1 = phy_read_mmd(phydev, 3, 0x8109);
+		ret2 = phy_read_mmd(phydev, 3, 0x8108);
+	}
+
+	return (0x0 != (ret1 & 0x0004)) && (0x0 != (ret2 & 0x3000)) ? 1 : 0;
+}
+
+static int read_master_slave(struct phy_device *phydev)
+{
+	int reg;
+
+	phydev->master_slave_get = MASTER_SLAVE_CFG_UNKNOWN;
+	phydev->master_slave_state = MASTER_SLAVE_STATE_UNKNOWN;
+
+	reg = phy_read_mmd(phydev, 7, 0x8001);
+	if (reg & (1 << 14)) {
+		phydev->master_slave_get = MASTER_SLAVE_CFG_MASTER_FORCE;
+		phydev->master_slave_state = MASTER_SLAVE_STATE_MASTER;
+	} else {
+		phydev->master_slave_get = MASTER_SLAVE_CFG_SLAVE_FORCE;
+		phydev->master_slave_state = MASTER_SLAVE_STATE_SLAVE;
+	}
+
+	return 0;
+}
+
+static int mv88q2xxx_read_status(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = genphy_update_link(phydev);
+	if (ret)
+		return ret;
+
+	phydev->link = check_link(phydev);
+	phydev->speed = get_speed(phydev);
+
+	ret = read_master_slave(phydev);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int mv88q2xxx_probe(struct phy_device *phydev)
+{
+	if (!phydev->is_c45)
+		return -ENODEV;
+
+	return 0;
+}
+
+static int mv88q2xxxx_cable_test_start(struct phy_device *phydev)
+{
+	return 0;
+}
+
+static int mv88q2xxxx_cable_test_get_status(struct phy_device *phydev,
+					    bool *finished)
+{
+	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_A,
+				ETHTOOL_A_CABLE_RESULT_CODE_OK);
+	return 0;
+}
+
+static int mv88q2xxxx_get_sqi(struct phy_device *phydev)
+{
+	u16 value;
+
+	if (phydev->speed == SPEED_100)
+		value = (phy_read_mmd(phydev, 3, 0x8230) >> 12) & 0x0F;
+	else
+		value = phy_read_mmd(phydev, 3, 0xfc88) & 0x0F;
+
+	return value;
+}
+
+static int mv88q2xxxx_get_sqi_max(struct phy_device *phydev)
+{
+	return 15;
+}
+
+static struct phy_driver mv88q2xxx_driver[] = {
+	{
+		.phy_id			= MARVELL_PHY_ID_88Q2110,
+		.phy_id_mask		= MARVELL_PHY_ID_MASK,
+		.features		= PHY_GBIT_T1_FEATURES,
+		.name			= "mv88q2110",
+		.probe			= mv88q2xxx_probe,
+		.soft_reset		= mv88q2xxx_soft_reset,
+		.config_init		= mv88q2xxx_config_init,
+		.read_status		= mv88q2xxx_read_status,
+		.config_aneg		= mv88q2xxx_config_aneg,
+		.cable_test_start	= mv88q2xxxx_cable_test_start,
+		.cable_test_get_status	= mv88q2xxxx_cable_test_get_status,
+		.set_loopback		= genphy_c45_loopback,
+		.get_sqi		= mv88q2xxxx_get_sqi,
+		.get_sqi_max		= mv88q2xxxx_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 },
+	{ /*sentinel*/ },
+};
+MODULE_DEVICE_TABLE(mdio, mv88q2xxx_tbl);
+
+MODULE_DESCRIPTION("Marvell Automotive 100BASE-T1/1000BASE-T1 Ethernet PHY driver (MV88Q2xxx)");
+MODULE_LICENSE("GPL");
-- 
2.39.2

Re: [PATCH v1 2/2] net: phy: marvell-88q2xxx: add driver for the Marvell 88Q2110 PHY

[Andrew Lunn]

> index 93b8efc792273..2913b145d5406 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -223,6 +223,12 @@ config MARVELL_88X2222_PHY
>  	  Support for the Marvell 88X2222 Dual-port Multi-speed Ethernet
>  	  Transceiver.
>  
> +config MARVELL_88Q2XXX_PHY
> +	tristate "Marvell 88Q2XXX PHY"
> +	help
> +	  Support for the Marvell Automotive 88Q2XXX 100/1000BASE-T1 Ethernet
> +	  PHYs.

I suggest you follow the pattern of the 88X2222.

	  Support for the Marvell 88Q2XXX 100/1000BASE-T1 Automotive Ethernet
	  PHYs.

These entries are sorted by these strings, so should be before the
88Q2XXX.


> +
>  config MAXLINEAR_GPHY
>  	tristate "Maxlinear Ethernet PHYs"
>  	select POLYNOMIAL if HWMON
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index f289ab16a1dab..15d1908fd5cb7 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -67,6 +67,7 @@ obj-$(CONFIG_LXT_PHY)		+= lxt.o
>  obj-$(CONFIG_MARVELL_10G_PHY)	+= marvell10g.o
>  obj-$(CONFIG_MARVELL_PHY)	+= marvell.o
>  obj-$(CONFIG_MARVELL_88X2222_PHY)	+= marvell-88x2222.o
> +obj-$(CONFIG_MARVELL_88Q2XXX_PHY)	+= marvell-88q2xxx.o

These are sorted by the CONFIG_*, so it should also be before the
88X2XXX

> +static int mv88q2xxx_soft_reset(struct phy_device *phydev)
> +{
> +	phy_write_mmd(phydev, 3, 0x0900, 0x8000);

Please use the #defines, e.g. MDIO_MMD_PCS.

0x900 is listed in IEEE 802.3 2022. It is a standard register. Please
add a #define for it next to all the others. You can also add BIT()
macros for PMA/PMD reset, and maybe Tx Disable, low power.

> +
> +	return 0;
> +}
> +
> +static void init_fast_ethernet(struct phy_device *phydev)
> +{
> +	u16 value = phy_read_mmd(phydev, 1, 0x0834);

2100 is also a standard register.

> +
> +	value = value & 0xFFF0;
> +
> +	phy_write_mmd(phydev, 1, 0x0834, value);
> +}
> +
> +static void init_gbit_ethernet(struct phy_device *phydev)
> +{
> +	u16 value = phy_read_mmd(phydev, 1, 0x0834);
> +
> +	value = (value & 0xFFF0) | 0x0001;
> +
> +	phy_write_mmd(phydev, 1, 0x0834, value);
> +}
> +
> +static int setup_master_slave(struct phy_device *phydev)
> +{
> +	u16 reg_data = phy_read_mmd(phydev, 1, 0x0834);
> +
> +	switch (phydev->master_slave_set) {
> +	case MASTER_SLAVE_CFG_MASTER_FORCE:
> +	case MASTER_SLAVE_CFG_MASTER_PREFERRED:
> +		reg_data |= 0x4000;
> +		break;
> +	case MASTER_SLAVE_CFG_SLAVE_PREFERRED:
> +	case MASTER_SLAVE_CFG_SLAVE_FORCE:
> +		reg_data &= ~0x4000;
> +		break;
> +	case MASTER_SLAVE_CFG_UNKNOWN:
> +	case MASTER_SLAVE_CFG_UNSUPPORTED:
> +		return 0;
> +	default:
> +		phydev_warn(phydev, "Unsupported Master/Slave mode\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	phy_write_mmd(phydev, 1, 0x0834, reg_data);
> +
> +	return 0;
> +}
> +
> +static int mv88q2xxx_config_aneg(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	if (phydev->speed == SPEED_100)
> +		init_fast_ethernet(phydev);
> +	else if (phydev->speed == SPEED_1000)
> +		init_gbit_ethernet(phydev);
> +
> +	ret = setup_master_slave(phydev);
> +	if (ret)
> +		return ret;
> +
> +	mv88q2xxx_soft_reset(phydev);
> +
> +	return 0;
> +}
> +
> +static int mv88q2xxx_config_init(struct phy_device *phydev)
> +{
> +	return mv88q2xxx_config_aneg(phydev);
> +}
> +
> +static int get_speed(struct phy_device *phydev)
> +{
> +	u16 value = 0;
> +
> +	if (phydev->autoneg)
> +		value = (phy_read_mmd(phydev, 7, 0x801a) & 0x4000) >> 14;
> +	else
> +		value = (phy_read_mmd(phydev, 1, 0x0834) & 0x1);
> +
> +	return value ? SPEED_1000 : SPEED_100;
> +}
> +
> +static int check_link(struct phy_device *phydev)
> +{
> +	u16 ret1, ret2;
> +
> +	if (phydev->speed == SPEED_1000) {
> +		ret1 = phy_read_mmd(phydev, 3, 0x0901);
> +		ret1 = phy_read_mmd(phydev, 3, 0x0901);
> +		ret2 = phy_read_mmd(phydev, 7, 0x8001);
> +	} else {
> +		ret1 = phy_read_mmd(phydev, 3, 0x8109);
> +		ret2 = phy_read_mmd(phydev, 3, 0x8108);
> +	}
> +
> +	return (0x0 != (ret1 & 0x0004)) && (0x0 != (ret2 & 0x3000)) ? 1 : 0;
> +}
> +
> +static int read_master_slave(struct phy_device *phydev)
> +{
> +	int reg;
> +
> +	phydev->master_slave_get = MASTER_SLAVE_CFG_UNKNOWN;
> +	phydev->master_slave_state = MASTER_SLAVE_STATE_UNKNOWN;
> +
> +	reg = phy_read_mmd(phydev, 7, 0x8001);
> +	if (reg & (1 << 14)) {
> +		phydev->master_slave_get = MASTER_SLAVE_CFG_MASTER_FORCE;
> +		phydev->master_slave_state = MASTER_SLAVE_STATE_MASTER;
> +	} else {
> +		phydev->master_slave_get = MASTER_SLAVE_CFG_SLAVE_FORCE;
> +		phydev->master_slave_state = MASTER_SLAVE_STATE_SLAVE;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mv88q2xxx_read_status(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = genphy_update_link(phydev);
> +	if (ret)
> +		return ret;
> +
> +	phydev->link = check_link(phydev);
> +	phydev->speed = get_speed(phydev);
> +
> +	ret = read_master_slave(phydev);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}


So all these functions can be placed in phy-c45.c, since they are
generic to any 1000BaseT1 PHY. Please look at the other functions in
there, and try the fit the patterns.

Please also look at genphy_c45_pma_read_abilities() and extend it to
detect the features of this PHY.

> +static int mv88q2xxx_probe(struct phy_device *phydev)
> +{
> +	if (!phydev->is_c45)
> +		return -ENODEV;

Why?

> +
> +	return 0;
> +}
> +
> +static int mv88q2xxxx_cable_test_start(struct phy_device *phydev)
> +{
> +	return 0;
> +}
> +
> +static int mv88q2xxxx_cable_test_get_status(struct phy_device *phydev,
> +					    bool *finished)
> +{
> +	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_A,
> +				ETHTOOL_A_CABLE_RESULT_CODE_OK);
> +	return 0;
> +}

There is no need to implement them if you don't have anything useful
to return.

> +static int mv88q2xxxx_get_sqi(struct phy_device *phydev)
> +{
> +	u16 value;
> +
> +	if (phydev->speed == SPEED_100)
> +		value = (phy_read_mmd(phydev, 3, 0x8230) >> 12) & 0x0F;
> +	else
> +		value = phy_read_mmd(phydev, 3, 0xfc88) & 0x0F;

This looks proprietary to this PHY. However the open alliance does
have some standards in this area. Please check to see if they define
registers.

	Andrew

Re: [PATCH v1 2/2] net: phy: marvell-88q2xxx: add driver for the Marvell 88Q2110 PHY

[Francesco Dolcini]

On Mon, Jul 03, 2023 at 02:44:40PM +0200, Stefan Eichenberger wrote:
> Add a driver for the Marvell 88Q2110. This driver is minimalistic, but
> already allows to detect the link, switch between 100BASE-T1 and
> 1000BASE-T1 and switch between master and slave mode. Autonegotiation
> supported by the PHY is not yet implemented.
> 
> Signed-off-by: Stefan Eichenberger <eichest@gmail.com>

...

> +static struct mdio_device_id __maybe_unused mv88q2xxx_tbl[] = {
> +	{ MARVELL_PHY_ID_88Q2110, MARVELL_PHY_ID_MASK },
> +	{ /*sentinel*/ },
No comma for a terminator entry.

Francesco

Re: [PATCH v1 2/2] net: phy: marvell-88q2xxx: add driver for the Marvell 88Q2110 PHY

[Francesco Dolcini]

On Mon, Jul 03, 2023 at 02:44:40PM +0200, Stefan Eichenberger wrote:
> Add a driver for the Marvell 88Q2110. This driver is minimalistic, but
> already allows to detect the link, switch between 100BASE-T1 and
> 1000BASE-T1 and switch between master and slave mode. Autonegotiation
> supported by the PHY is not yet implemented.
> 
> Signed-off-by: Stefan Eichenberger <eichest@gmail.com>

...

> --- /dev/null
> +++ b/drivers/net/phy/marvell-88q2xxx.c
> @@ -0,0 +1,217 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Marvell 88Q2XXX automotive 100BASE-T1/1000BASE-T1 PHY driver
> + */
> +
> +#include <linux/marvell_phy.h>
> +#include <linux/phy.h>
> +#include <linux/ethtool_netlink.h>
sort?

> +#define MARVELL_PHY_ID_88Q2110		0x002b0981
> +
> +static int mv88q2xxx_soft_reset(struct phy_device *phydev)
> +{
> +	phy_write_mmd(phydev, 3, 0x0900, 0x8000);
> +
> +	return 0;
Should we `return phy_write_mmd()` to allow error propagation? This
eventually applies to other functions.

Francesco

Re: [PATCH v1 2/2] net: phy: marvell-88q2xxx: add driver for the Marvell 88Q2110 PHY

[Russell King (Oracle)]

On Mon, Jul 03, 2023 at 02:44:40PM +0200, Stefan Eichenberger wrote:
> +static int check_link(struct phy_device *phydev)
> +{
> +	u16 ret1, ret2;
> +
> +	if (phydev->speed == SPEED_1000) {
> +		ret1 = phy_read_mmd(phydev, 3, 0x0901);
> +		ret1 = phy_read_mmd(phydev, 3, 0x0901);

This looks like some kind of BMSR register, where the link status
latches on failure until the following read. By reading the register
twice, you are discarding the information that the link _had_ failed
since we last read it. You don't want to do that - it could come up with
different parameters and because the link to phylib appears to remain
up, it won't respond to the new link parameters.

Always report the latched link status, not the current status.

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