[PATCH net-next v2 0/4] Add a driver for the Marvell 88Q2110 PHY

[PATCH net-next v2 0/4] Add a driver for the Marvell 88Q2110 PHY

[Stefan Eichenberger]

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

v2:
 - Use the same pattern in Kconfig as for 88X2222 (Andrew)
 - Sort Kconfig and Makefile entries (Andrew)
 - Add generic registers to mdio.h (Andrew)
 - Move generic functionality to phy-c45.c (Andrew)
 - Document where proprietary registers are used (Andrew)
 - Remove unnecessary c45 check (Andrew)
 - Remove cable tests which were not implemented (Andrew)
 - Remove comma for terminator entry (Francesco)
 - Sort include files (Francesco)
 - Return phy_write_mmd value in soft_reset (Francesco)

Stefan Eichenberger (4):
  net: phy: add the link modes for 1000BASE-T1 Ethernet PHY
  net: phy: add registers to support 1000BASE-T1
  net: phy: c45: add support for 1000BASE-T1
  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 | 149 ++++++++++++++++++++++++++++++
 drivers/net/phy/phy-c45.c         |  12 ++-
 drivers/net/phy/phy_device.c      |  14 +++
 include/linux/phy.h               |   2 +
 include/uapi/linux/mdio.h         |  12 ++-
 7 files changed, 194 insertions(+), 2 deletions(-)
 create mode 100644 drivers/net/phy/marvell-88q2xxx.c

-- 
2.39.2

[PATCH net-next v2 1/4] 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 0c2014accba7d..acc8950f08cfa 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 11c1e91563d47..6c71f10e0f7f0 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

Re: [PATCH net-next v2 1/4] net: phy: add the link modes for 1000BASE-T1 Ethernet PHY

[Andrew Lunn]

On Mon, Jul 10, 2023 at 10:58:57PM +0200, Stefan Eichenberger wrote:
> 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.

Is this actually needed? Ideally you want to extend
genphy_c45_pma_read_abilities() to look in the PHY registers and
determine what the PHY can do. You should only use .features if it is
impossible to determine the PHY abilities by reading registers.

	   Andrew

Re: [PATCH net-next v2 1/4] net: phy: add the link modes for 1000BASE-T1 Ethernet PHY

[Stefan Eichenberger]

Hi Andrew,

On Mon, Jul 10, 2023 at 11:10:17PM +0200, Andrew Lunn wrote:
> On Mon, Jul 10, 2023 at 10:58:57PM +0200, Stefan Eichenberger wrote:
> > 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.
> 
> Is this actually needed? Ideally you want to extend
> genphy_c45_pma_read_abilities() to look in the PHY registers and
> determine what the PHY can do. You should only use .features if it is
> impossible to determine the PHY abilities by reading registers.

Unfortunately the MDIO_PMA_EXTABLE register does not work on this PHY.
It will not signalize that it is BT1 capable (MDIO_PMA_EXTABLE_BT1). I
tested it again (should be 0x0800):
root@verdin-imx8mp-14777637:~# ./phytool read eth0/7:1/11
0x0020
The PHY documentation does not list this register at all.

Even though the MDIO_PMA_PMD_BT1 exists and works correctly:
root@verdin-imx8mp-14777637:~# ./phytool read eth0/7:1/18
0x0003
This register is documented in the datasheet again.

So unfortunately I think I have to force the features, or do you see
another possibility?

Regards,
Stefan

Re: [PATCH net-next v2 1/4] net: phy: add the link modes for 1000BASE-T1 Ethernet PHY

[Andrew Lunn]

On Thu, Jul 13, 2023 at 04:10:21PM +0200, Stefan Eichenberger wrote:
> Hi Andrew,
> 
> On Mon, Jul 10, 2023 at 11:10:17PM +0200, Andrew Lunn wrote:
> > On Mon, Jul 10, 2023 at 10:58:57PM +0200, Stefan Eichenberger wrote:
> > > 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.
> > 
> > Is this actually needed? Ideally you want to extend
> > genphy_c45_pma_read_abilities() to look in the PHY registers and
> > determine what the PHY can do. You should only use .features if it is
> > impossible to determine the PHY abilities by reading registers.
> 
> Unfortunately the MDIO_PMA_EXTABLE register does not work on this PHY.
> It will not signalize that it is BT1 capable (MDIO_PMA_EXTABLE_BT1). I
> tested it again (should be 0x0800):

**
 * genphy_c45_baset1_able - checks if the PMA has BASE-T1 extended abilities
 * @phydev: target phy_device struct
 */
static bool genphy_c45_baset1_able(struct phy_device *phydev)
{
        int val;

        if (phydev->pma_extable == -ENODATA) {
                val = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_EXTABLE);
                if (val < 0)
                        return false;

                phydev->pma_extable = val;
        }

        return !!(phydev->pma_extable & MDIO_PMA_EXTABLE_BT1);
}

This is rather odd, but might help you. You already have a workaround
in mv88q2xxx_config_init(). Have you tried adding a get_features()
callback with sets phydev->pma_extable to the correct value, and then
calls genphy_c45_pma_read_abilities()?

Please also report the bug in the PHY to Marvell. Maybe a later
revision might have it fixed.

      Andrew

Re: [PATCH net-next v2 1/4] net: phy: add the link modes for 1000BASE-T1 Ethernet PHY

[Stefan Eichenberger]

On Thu, Jul 13, 2023 at 05:18:02PM +0200, Andrew Lunn wrote:
> 
> **
>  * genphy_c45_baset1_able - checks if the PMA has BASE-T1 extended abilities
>  * @phydev: target phy_device struct
>  */
> static bool genphy_c45_baset1_able(struct phy_device *phydev)
> {
>         int val;
> 
>         if (phydev->pma_extable == -ENODATA) {
>                 val = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_EXTABLE);
>                 if (val < 0)
>                         return false;
> 
>                 phydev->pma_extable = val;
>         }
> 
>         return !!(phydev->pma_extable & MDIO_PMA_EXTABLE_BT1);
> }
> 
> This is rather odd, but might help you. You already have a workaround
> in mv88q2xxx_config_init(). Have you tried adding a get_features()
> callback with sets phydev->pma_extable to the correct value, and then
> calls genphy_c45_pma_read_abilities()?
> 
> Please also report the bug in the PHY to Marvell. Maybe a later
> revision might have it fixed.

Thanks for the suggestion. Unfortunately,
genphy_c45_pma_read_abilities() directly reads from the PHY registers.
Therefore, I can't use the pma_extable. But probably I can just set the
missing features in get_features for this PHY. I will see what I can do
here.

Thanks,
Stefan

Re: [PATCH net-next v2 1/4] net: phy: add the link modes for 1000BASE-T1 Ethernet PHY

[Andrew Lunn]

> Thanks for the suggestion. Unfortunately,
> genphy_c45_pma_read_abilities() directly reads from the PHY registers.

Please consider refactoring the bits of
genphy_c45_pma_read_abilities() you need into a helper. You can then
call the helper directly from your driver.

     Andrew

[PATCH net-next v2 2/4] net: phy: add registers to support 1000BASE-T1

[Stefan Eichenberger]

Add registers and definitions to support 1000BASE-T1. This includes the
PCS Control and Status registers (3.2304 and 3.2305) as well as some
missing bits on the PMA/PMD extended ability register (1.18) and PMA/PMD
CTRL (1.2100) register.

Signed-off-by: Stefan Eichenberger <eichest@gmail.com>
---
 include/uapi/linux/mdio.h | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/mdio.h b/include/uapi/linux/mdio.h
index b826598d1e94c..ffa821cb291a5 100644
--- a/include/uapi/linux/mdio.h
+++ b/include/uapi/linux/mdio.h
@@ -82,6 +82,8 @@
 #define MDIO_AN_10BT1_AN_CTRL	526	/* 10BASE-T1 AN control register */
 #define MDIO_AN_10BT1_AN_STAT	527	/* 10BASE-T1 AN status register */
 #define MDIO_PMA_PMD_BT1_CTRL	2100	/* BASE-T1 PMA/PMD control register */
+#define MDIO_PCS_1000BT1_CTRL	2304	/* 1000BASE-T1 PCS control register */
+#define MDIO_PCS_1000BT1_STAT	2305	/* 1000BASE-T1 PCS status register */
 
 /* LASI (Link Alarm Status Interrupt) registers, defined by XENPAK MSA. */
 #define MDIO_PMA_LASI_RXCTRL	0x9000	/* RX_ALARM control */
@@ -332,6 +334,7 @@
 #define MDIO_PCS_10T1L_CTRL_RESET	0x8000	/* PCS reset */
 
 /* BASE-T1 PMA/PMD extended ability register. */
+#define MDIO_PMA_PMD_BT1_B1000_ABLE	0x0002	/* 1000BASE-T1 Ability */
 #define MDIO_PMA_PMD_BT1_B10L_ABLE	0x0004	/* 10BASE-T1L Ability */
 
 /* BASE-T1 auto-negotiation advertisement register [15:0] */
@@ -373,7 +376,14 @@
 #define MDIO_AN_10BT1_AN_STAT_LPA_EEE_T1L	0x4000 /* 10BASE-T1L LP EEE ability advertisement */
 
 /* BASE-T1 PMA/PMD control register */
-#define MDIO_PMA_PMD_BT1_CTRL_CFG_MST	0x4000 /* MASTER-SLAVE config value */
+#define MDIO_PMA_PMD_BT1_CTRL_STRAP		0x000F /* Type selection (Strap) */
+#define MDIO_PMA_PMD_BT1_CTRL_STRAP_B1000	0x0001 /* Select 1000BASE-T1 */
+#define MDIO_PMA_PMD_BT1_CTRL_CFG_MST		0x4000 /* MASTER-SLAVE config value */
+
+/* 1000BASE-T1 PCS control register */
+#define MDIO_PCS_1000BT1_LOW_POWER	0x0800 /* Low power mode */
+#define MDIO_PCS_1000BT1_DISABLE_TX	0x4000 /* Global PMA transmit disable */
+#define MDIO_PCS_1000BT1_CTRL_RESET	0x8000 /* Software reset value */
 
 /* EEE Supported/Advertisement/LP Advertisement registers.
  *
-- 
2.39.2

[PATCH net-next v2 3/4] net: phy: c45: add support for 1000BASE-T1

[Stefan Eichenberger]

Add support for 1000BASE-T1 to the forced link setup.

Signed-off-by: Stefan Eichenberger <eichest@gmail.com>
---
 drivers/net/phy/phy-c45.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index 93ed072233779..ec1232066b914 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -108,7 +108,7 @@ EXPORT_SYMBOL_GPL(genphy_c45_pma_baset1_setup_master_slave);
  */
 int genphy_c45_pma_setup_forced(struct phy_device *phydev)
 {
-	int ctrl1, ctrl2, ret;
+	int bt1_ctrl, ctrl1, ctrl2, ret;
 
 	/* Half duplex is not supported */
 	if (phydev->duplex != DUPLEX_FULL)
@@ -176,6 +176,12 @@ int genphy_c45_pma_setup_forced(struct phy_device *phydev)
 		ret = genphy_c45_pma_baset1_setup_master_slave(phydev);
 		if (ret < 0)
 			return ret;
+
+		bt1_ctrl = 0;
+		if (phydev->speed == SPEED_1000)
+			bt1_ctrl = MDIO_PMA_PMD_BT1_CTRL_STRAP_B1000;
+		phy_modify_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_PMD_BT1_CTRL,
+			       MDIO_PMA_PMD_BT1_CTRL_STRAP, bt1_ctrl);
 	}
 
 	return genphy_c45_an_disable_aneg(phydev);
@@ -976,6 +982,10 @@ int genphy_c45_pma_read_abilities(struct phy_device *phydev)
 					 phydev->supported,
 					 val & MDIO_PMA_PMD_BT1_B10L_ABLE);
 
+			linkmode_mod_bit(ETHTOOL_LINK_MODE_1000baseT1_Full_BIT,
+					 phydev->supported,
+					 val & MDIO_PMA_PMD_BT1_B1000_ABLE);
+
 			val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_T1_STAT);
 			if (val < 0)
 				return val;
-- 
2.39.2

Re: [PATCH net-next v2 3/4] net: phy: c45: add support for 1000BASE-T1

[Francesco Dolcini]

On Mon, Jul 10, 2023 at 10:58:59PM +0200, Stefan Eichenberger wrote:
> Add support for 1000BASE-T1 to the forced link setup.
> 
> Signed-off-by: Stefan Eichenberger <eichest@gmail.com>
> ---
>  drivers/net/phy/phy-c45.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
> index 93ed072233779..ec1232066b914 100644
> --- a/drivers/net/phy/phy-c45.c
> +++ b/drivers/net/phy/phy-c45.c
...

> @@ -176,6 +176,12 @@ int genphy_c45_pma_setup_forced(struct phy_device *phydev)
>  		ret = genphy_c45_pma_baset1_setup_master_slave(phydev);
>  		if (ret < 0)
>  			return ret;
> +
> +		bt1_ctrl = 0;
> +		if (phydev->speed == SPEED_1000)
> +			bt1_ctrl = MDIO_PMA_PMD_BT1_CTRL_STRAP_B1000;
> +		phy_modify_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_PMD_BT1_CTRL,
> +			       MDIO_PMA_PMD_BT1_CTRL_STRAP, bt1_ctrl);

check the return value here?

[PATCH net-next v2 4/4] 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 | 149 ++++++++++++++++++++++++++++++
 3 files changed, 156 insertions(+)
 create mode 100644 drivers/net/phy/marvell-88q2xxx.c

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 78e6981650d94..87b8238587173 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -217,6 +217,12 @@ config MARVELL_10G_PHY
 	help
 	  Support for the Marvell Alaska MV88X3310 and compatible PHYs.
 
+config MARVELL_88Q2XXX_PHY
+	tristate "Marvell 88Q2XXX PHY"
+	help
+	  Support for the Marvell 88Q2XXX 100/1000BASE-T1 Automotive Ethernet
+	  PHYs.
+
 config MARVELL_88X2222_PHY
 	tristate "Marvell 88X2222 PHY"
 	help
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 2fe51ea83babe..35142780fc9da 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -66,6 +66,7 @@ obj-$(CONFIG_LSI_ET1011C_PHY)	+= et1011c.o
 obj-$(CONFIG_LXT_PHY)		+= lxt.o
 obj-$(CONFIG_MARVELL_10G_PHY)	+= marvell10g.o
 obj-$(CONFIG_MARVELL_PHY)	+= marvell.o
+obj-$(CONFIG_MARVELL_88Q2XXX_PHY)	+= marvell-88q2xxx.o
 obj-$(CONFIG_MARVELL_88X2222_PHY)	+= marvell-88x2222.o
 obj-$(CONFIG_MAXLINEAR_GPHY)	+= mxl-gpy.o
 obj-$(CONFIG_MEDIATEK_GE_PHY)	+= mediatek-ge.o
diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
new file mode 100644
index 0000000000000..a073124c33f5e
--- /dev/null
+++ b/drivers/net/phy/marvell-88q2xxx.c
@@ -0,0 +1,149 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Marvell 88Q2XXX automotive 100BASE-T1/1000BASE-T1 PHY driver
+ */
+#include <linux/ethtool_netlink.h>
+#include <linux/marvell_phy.h>
+#include <linux/phy.h>
+
+#define MARVELL_PHY_ID_88Q2110		0x002b0981
+
+static int mv88q2xxx_soft_reset(struct phy_device *phydev)
+{
+	return phy_write_mmd(phydev, MDIO_MMD_PCS,
+			     MDIO_PCS_1000BT1_CTRL, MDIO_PCS_1000BT1_CTRL_RESET);
+}
+
+static int mv88q2xxx_read_link(struct phy_device *phydev)
+{
+	u16 ret1, ret2;
+
+	/* 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.
+	 */
+	if (phydev->speed == SPEED_1000) {
+		/* Read twice to clear the latched status */
+		ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT);
+		ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT);
+		/* Read vendor specific Auto-Negotiation status register to get
+		 * local and remote receiver status
+		 */
+		ret2 = phy_read_mmd(phydev, MDIO_MMD_AN, 0x8001);
+	} else {
+		/* Read vendor specific status registers, the registers are not
+		 * documented but they can be found in the Software
+		 * Initialization Guide
+		 */
+		ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, 0x8109);
+		ret2 = phy_read_mmd(phydev, MDIO_MMD_PCS, 0x8108);
+	}
+
+	/* Check the link status according to Software Initialization Guide */
+	return (0x0 != (ret1 & 0x0004)) && (0x0 != (ret2 & 0x3000)) ? 1 : 0;
+}
+
+static int mv88q2xxx_read_status(struct phy_device *phydev)
+{
+	int ret;
+
+	phydev->link = mv88q2xxx_read_link(phydev);
+
+	ret = genphy_c45_read_pma(phydev);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int mv88q2xxx_config_aneg(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = genphy_c45_config_aneg(phydev);
+	if (ret)
+		return ret;
+
+	ret = mv88q2xxx_soft_reset(phydev);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+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);
+}
+
+static int mv88q2xxx_probe(struct phy_device *phydev)
+{
+	return 0;
+}
+
+static int mv88q2xxxx_get_sqi(struct phy_device *phydev)
+{
+	u16 value;
+
+	if (phydev->speed == SPEED_100) {
+		/* Read the SQI from the vendor specific receiver status
+		 * register
+		 */
+		value = (phy_read_mmd(phydev, MDIO_MMD_PCS, 0x8230) >> 12) & 0x0F;
+	} else {
+		/* Read from vendor specific registers, they are not documented
+		 * but can be found in the Software Initialization Guide. Only
+		 * revisions >= A0 are supported.
+		 */
+		phy_modify_mmd(phydev, MDIO_MMD_PCS, 0xFC5D, 0x00FF, 0x00AC);
+		value = phy_read_mmd(phydev, MDIO_MMD_PCS, 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,
+		.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 88Q2XXX 100/1000BASE-T1 Automotive Ethernet PHY driver");
+MODULE_LICENSE("GPL");
-- 
2.39.2

Re: [PATCH net-next v2 4/4] net: phy: marvell-88q2xxx: add driver for the Marvell 88Q2110 PHY

[Andrew Lunn]

> +static int mv88q2xxx_soft_reset(struct phy_device *phydev)
> +{
> +	return phy_write_mmd(phydev, MDIO_MMD_PCS,
> +			     MDIO_PCS_1000BT1_CTRL, MDIO_PCS_1000BT1_CTRL_RESET);
> +}

Does this bit clear on its own when the reset has completed? When
performing a C22 soft reset, the code polls waiting for the bit to
clear. Otherwise there is a danger you start writing other registers
while it is still resetting.

> +static int mv88q2xxx_read_link(struct phy_device *phydev)
> +{
> +	u16 ret1, ret2;
> +
> +	/* 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.
> +	 */
> +	if (phydev->speed == SPEED_1000) {
> +		/* Read twice to clear the latched status */
> +		ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT);
> +		ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT);

This is generally wrong. See for example genphy_update_link() and
genphy_c45_read_link().

> +static int mv88q2xxx_probe(struct phy_device *phydev)
> +{
> +	return 0;
> +}

If it does nothing, it should not be needed.

    Andrew

---
pw-bot: cr

Re: [PATCH net-next v2 4/4] net: phy: marvell-88q2xxx: add driver for the Marvell 88Q2110 PHY

[Stefan Eichenberger]

Hi Andrew,

Thanks a lot for the review and all the hints, I have one short question
below.

> > +static int mv88q2xxx_read_link(struct phy_device *phydev)
> > +{
> > +	u16 ret1, ret2;
> > +
> > +	/* 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.
> > +	 */
> > +	if (phydev->speed == SPEED_1000) {
> > +		/* Read twice to clear the latched status */
> > +		ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT);
> > +		ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT);
> 
> This is generally wrong. See for example genphy_update_link() and
> genphy_c45_read_link().
> 

Would something like this look fine to you? The issue is that I mix
realtime data with latched data because the local and remote rx status
is only available in realtime from what I can find in the datasheet.
This would be for gbit, I split that up compared to the last version:

static int mv88q2xxx_read_link_gbit(struct phy_device *phydev)
{
	int ret1, ret2;

	/* 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 the link was already down. In case we
	 * are not polling, we always read the realtime status.
	 */
	if (!phy_polling_mode(phydev) || !phydev->link) {
		ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT);
		if (ret1 < 0)
			return ret1;
	}

	ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT);
	if (ret1 < 0)
		return ret1;

	/* Read vendor specific Auto-Negotiation status register to get local
	 * and remote receiver status according to software initialization
	 * guide.
	 */
	ret2 = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_MMD_AN_MV_STATUS);
	if (ret2 < 0)
		return ret2;

	/* Check if we have link and if the remote and local receiver are ok */
	return (ret1 & MDIO_PCS_1000BT1_STAT_LINK) &&
	       (ret2 & MDIO_MMD_AN_MV_STATUS_LOCAL_RX) &&
	       (ret2 & MDIO_MMD_AN_MV_STATUS_REMOTE_RX);
}

With this we will detect link loss in polling mode and read the realtime
status in non-polling mode. Compared to genphy_c45_read_link we will not
immediately return "link up" in non polling mode but always do the
second read to get the realtime link status.

If we are only interested in the link status we could also skip the
remote and local receiver check. However, as I understand the software
initialization guide it could be that the receivers are not ready in
that moment.

Regards,
Stefan

Re: [PATCH net-next v2 4/4] net: phy: marvell-88q2xxx: add driver for the Marvell 88Q2110 PHY

[Russell King (Oracle)]

On Thu, Jul 13, 2023 at 11:42:12AM +0200, Stefan Eichenberger wrote:
> Hi Andrew,
> 
> Thanks a lot for the review and all the hints, I have one short question
> below.
> 
> > > +static int mv88q2xxx_read_link(struct phy_device *phydev)
> > > +{
> > > +	u16 ret1, ret2;
> > > +
> > > +	/* 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.
> > > +	 */
> > > +	if (phydev->speed == SPEED_1000) {
> > > +		/* Read twice to clear the latched status */
> > > +		ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT);
> > > +		ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT);
> > 
> > This is generally wrong. See for example genphy_update_link() and
> > genphy_c45_read_link().
> 
> Would something like this look fine to you? The issue is that I mix
> realtime data with latched data because the local and remote rx status
> is only available in realtime from what I can find in the datasheet.
> This would be for gbit, I split that up compared to the last version:

I've never really understood this kind of reaction from people. A bit
of example code gets suggested, and then the code gets sort of used
as a half-hearted template...

> static int mv88q2xxx_read_link_gbit(struct phy_device *phydev)
> {
> 	int ret1, ret2;
> 
> 	/* 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 the link was already down. In case we
> 	 * are not polling, we always read the realtime status.
> 	 */
> 	if (!phy_polling_mode(phydev) || !phydev->link) {
> 		ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT);
> 		if (ret1 < 0)
> 			return ret1;

Both genphy_update_link() and genphy_c45_read_link() check here whether
the link is up, and if it is, it bypasses the second read (because
there's no point re-reading it.) I've no idea why you dropped that
optimisation.

MDIO accesses aren't the cheapest thing...

> With this we will detect link loss in polling mode and read the realtime
> status in non-polling mode. Compared to genphy_c45_read_link we will not
> immediately return "link up" in non polling mode but always do the
> second read to get the realtime link status.

Why do you think that's better? "Link" only latches low, and the
entire point of that behaviour is so that management software can
detect when the link has failed at some point since it last read
the link status.

There is only any point in re-reading the status register if on the
first read it reports that the link has failed, and only then if we
already knew that the link has failed.

If we're using interrupt mode, then we need the current link status
but that's only because of the way phylib has been written.

> If we are only interested in the link status we could also skip the
> remote and local receiver check. However, as I understand the software
> initialization guide it could be that the receivers are not ready in
> that moment.

With copper PHYs, link up status means that the link is ready to pass
data. Is this not the case with T1 PHYs?

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

Re: [PATCH net-next v2 4/4] net: phy: marvell-88q2xxx: add driver for the Marvell 88Q2110 PHY

[Stefan Eichenberger]

Hi Russell,

Thanks for your reply.

On Thu, Jul 13, 2023 at 11:14:23AM +0100, Russell King (Oracle) wrote:
> On Thu, Jul 13, 2023 at 11:42:12AM +0200, Stefan Eichenberger wrote:
> > With this we will detect link loss in polling mode and read the realtime
> > status in non-polling mode. Compared to genphy_c45_read_link we will not
> > immediately return "link up" in non polling mode but always do the
> > second read to get the realtime link status.
> 
> Why do you think that's better? "Link" only latches low, and the
> entire point of that behaviour is so that management software can
> detect when the link has failed at some point since it last read
> the link status.
> 
> There is only any point in re-reading the status register if on the
> first read it reports that the link has failed, and only then if we
> already knew that the link has failed.
> 
> If we're using interrupt mode, then we need the current link status
> but that's only because of the way phylib has been written.
> 

I agree with you. I missunderstood how the link status worked. I thought
it will always show the status since the last read, independent of
whether it went up or down. But it only latches link down.

> > If we are only interested in the link status we could also skip the
> > remote and local receiver check. However, as I understand the software
> > initialization guide it could be that the receivers are not ready in
> > that moment.
> 
> With copper PHYs, link up status means that the link is ready to pass
> data. Is this not the case with T1 PHYs?

If I interpret their documentation correctly, they differentiate between
PMA and PCS. The PMA link status could be up while the PCS link status
is not. Maybe when the cable test is running this happens. Therefore, I
keep the read for MDIO_MMD_AN_MV_STAT for now.

Regards,
Stefan

Re: [PATCH net-next v2 4/4] net: phy: marvell-88q2xxx: add driver for the Marvell 88Q2110 PHY

[Francesco Dolcini]

On Mon, Jul 10, 2023 at 10:59:00PM +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 int mv88q2xxx_read_link(struct phy_device *phydev)
> +{
> +	u16 ret1, ret2;
> +
> +	/* 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.
> +	 */
> +	if (phydev->speed == SPEED_1000) {
> +		/* Read twice to clear the latched status */
> +		ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT);
> +		ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT);
> +		/* Read vendor specific Auto-Negotiation status register to get
> +		 * local and remote receiver status
> +		 */
> +		ret2 = phy_read_mmd(phydev, MDIO_MMD_AN, 0x8001);
> +	} else {
> +		/* Read vendor specific status registers, the registers are not
> +		 * documented but they can be found in the Software
> +		 * Initialization Guide
> +		 */
> +		ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, 0x8109);
> +		ret2 = phy_read_mmd(phydev, MDIO_MMD_PCS, 0x8108);
> +	}
you are ignoring errors from phy_read_mmd() here

> +
> +	/* Check the link status according to Software Initialization Guide */
> +	return (0x0 != (ret1 & 0x0004)) && (0x0 != (ret2 & 0x3000)) ? 1 : 0;
> +}
> +
> +static int mv88q2xxx_read_status(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	phydev->link = mv88q2xxx_read_link(phydev);
> +
> +	ret = genphy_c45_read_pma(phydev);
> +	if (ret)
> +		return ret;
return genphy_c45_read_pma(phydev);

> +static int mv88q2xxx_config_aneg(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = genphy_c45_config_aneg(phydev);
> +	if (ret)
> +		return ret;
> +
> +	ret = mv88q2xxx_soft_reset(phydev);
> +	if (ret)
> +		return ret;
return mv88q2xxx_soft_reset(phydev);

> +static int mv88q2xxx_probe(struct phy_device *phydev)
> +{
> +	return 0;
> +}
not needed? just remove it, if nothing to be done

> +static int mv88q2xxxx_get_sqi(struct phy_device *phydev)
> +{
> +	u16 value;
> +
> +	if (phydev->speed == SPEED_100) {
> +		/* Read the SQI from the vendor specific receiver status
> +		 * register
> +		 */
> +		value = (phy_read_mmd(phydev, MDIO_MMD_PCS, 0x8230) >> 12) & 0x0F;
> +	} else {
> +		/* Read from vendor specific registers, they are not documented
> +		 * but can be found in the Software Initialization Guide. Only
> +		 * revisions >= A0 are supported.
> +		 */
> +		phy_modify_mmd(phydev, MDIO_MMD_PCS, 0xFC5D, 0x00FF, 0x00AC);
> +		value = phy_read_mmd(phydev, MDIO_MMD_PCS, 0xfc88) & 0x0F;
> +	}

errors from phy_modify_mmd/phy_read_mmd ignored?

Re: [PATCH net-next v2 4/4] net: phy: marvell-88q2xxx: add driver for the Marvell 88Q2110 PHY

[Andrew Lunn]

> +	if (phydev->speed == SPEED_1000) {
> +		/* Read twice to clear the latched status */
> +		ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT);
> +		ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT);
> +		/* Read vendor specific Auto-Negotiation status register to get
> +		 * local and remote receiver status
> +		 */
> +		ret2 = phy_read_mmd(phydev, MDIO_MMD_AN, 0x8001);
> +	} else {
> +		/* Read vendor specific status registers, the registers are not
> +		 * documented but they can be found in the Software
> +		 * Initialization Guide
> +		 */
> +		ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, 0x8109);
> +		ret2 = phy_read_mmd(phydev, MDIO_MMD_PCS, 0x8108);
> +	}
> +
> +	/* Check the link status according to Software Initialization Guide */
> +	return (0x0 != (ret1 & 0x0004)) && (0x0 != (ret2 & 0x3000)) ? 1 : 0;

MDIO_PCS_1000BT1_STAT bit 2 is "Receive polarity" as defined in 802.3?
Please add a #define for it.

Please also add a #define for 0x3000 so we have some idea what is
means.

	Andrew

Re: [PATCH net-next v2 4/4] net: phy: marvell-88q2xxx: add driver for the Marvell 88Q2110 PHY

[Russell King (Oracle)]

On Mon, Jul 10, 2023 at 10:59:00PM +0200, Stefan Eichenberger wrote:
> +	/* 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.
> +	 */
> +	if (phydev->speed == SPEED_1000) {
> +		/* Read twice to clear the latched status */
> +		ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT);
> +		ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT);

Please address my comment on this from v1, sent today.

(Sorry it's late, but I've been on vacation since the start of July...
which has now been disrupted. Presently "stuck" out on the English
canals in beautiful countryside due to a lock failure, waiting for it
to be repaired! :D Then I'll be taking the remainder of the vacation
because I'll again be moving the boat!)

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