[PATCH net-next v3 0/3] net: phy: dp83td510: add cable diag support

[PATCH net-next v3 0/3] net: phy: dp83td510: add cable diag support

[Oleksij Rempel]

changes v3:
- spell fixes

changes v2:
- optimize SQI array

Oleksij Rempel (3):
  net: phy: dp83td510: add SQI support
  net: phy: dp83td510: add cable testing support
  net: phy: dp83td510: disable cable test support for 1Vpp PHYs

 drivers/net/phy/dp83td510.c | 260 ++++++++++++++++++++++++++++++++++++
 1 file changed, 260 insertions(+)

-- 
2.30.2

[PATCH net-next v3 1/3] net: phy: dp83td510: add SQI support

[Oleksij Rempel]

Convert MSE (mean-square error) values to SNR and split it SQI (Signal Quality
Indicator) ranges. The used ranges are taken from "OPEN ALLIANCE - Advanced
diagnostic features for 100BASE-T1 automotive Ethernet PHYs"
specification.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/phy/dp83td510.c | 49 +++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/drivers/net/phy/dp83td510.c b/drivers/net/phy/dp83td510.c
index 1ae792b0daaa..3cd9a77f9532 100644
--- a/drivers/net/phy/dp83td510.c
+++ b/drivers/net/phy/dp83td510.c
@@ -27,6 +27,27 @@
 #define DP83TD510E_AN_STAT_1			0x60c
 #define DP83TD510E_MASTER_SLAVE_RESOL_FAIL	BIT(15)
 
+#define DP83TD510E_MSE_DETECT			0xa85
+
+#define DP83TD510_SQI_MAX	7
+
+/* Register values are converted to SNR(dB) as suggested by
+ * "Application Report - DP83TD510E Cable Diagnostics Toolkit":
+ * SNR(dB) = -10 * log10 (VAL/2^17) - 1.76 dB.
+ * SQI ranges are implemented according to "OPEN ALLIANCE - Advanced diagnostic
+ * features for 100BASE-T1 automotive Ethernet PHYs"
+ */
+static const u16 dp83td510_mse_sqi_map[] = {
+	0x0569, /* < 18dB */
+	0x044c, /* 18dB =< SNR < 19dB */
+	0x0369, /* 19dB =< SNR < 20dB */
+	0x02b6, /* 20dB =< SNR < 21dB */
+	0x0227, /* 21dB =< SNR < 22dB */
+	0x01b6, /* 22dB =< SNR < 23dB */
+	0x015b, /* 23dB =< SNR < 24dB */
+	0x0000  /* 24dB =< SNR */
+};
+
 static int dp83td510_config_intr(struct phy_device *phydev)
 {
 	int ret;
@@ -164,6 +185,32 @@ static int dp83td510_config_aneg(struct phy_device *phydev)
 	return genphy_c45_check_and_restart_aneg(phydev, changed);
 }
 
+static int dp83td510_get_sqi(struct phy_device *phydev)
+{
+	int sqi, ret;
+	u16 mse_val;
+
+	if (!phydev->link)
+		return 0;
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_MSE_DETECT);
+	if (ret < 0)
+		return ret;
+
+	mse_val = 0xFFFF & ret;
+	for (sqi = 0; sqi < ARRAY_SIZE(dp83td510_mse_sqi_map); sqi++) {
+		if (mse_val >= dp83td510_mse_sqi_map[sqi])
+			return sqi;
+	}
+
+	return -EINVAL;
+}
+
+static int dp83td510_get_sqi_max(struct phy_device *phydev)
+{
+	return DP83TD510_SQI_MAX;
+}
+
 static int dp83td510_get_features(struct phy_device *phydev)
 {
 	/* This PHY can't respond on MDIO bus if no RMII clock is enabled.
@@ -192,6 +239,8 @@ static struct phy_driver dp83td510_driver[] = {
 	.get_features	= dp83td510_get_features,
 	.config_intr	= dp83td510_config_intr,
 	.handle_interrupt = dp83td510_handle_interrupt,
+	.get_sqi	= dp83td510_get_sqi,
+	.get_sqi_max	= dp83td510_get_sqi_max,
 
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
-- 
2.30.2

Re: [PATCH net-next v3 1/3] net: phy: dp83td510: add SQI support

[Andrew Lunn]

On Wed, Jun 08, 2022 at 02:32:34PM +0200, Oleksij Rempel wrote:
> Convert MSE (mean-square error) values to SNR and split it SQI (Signal Quality
> Indicator) ranges. The used ranges are taken from "OPEN ALLIANCE - Advanced
> diagnostic features for 100BASE-T1 automotive Ethernet PHYs"
> specification.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

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

    Andrew

[PATCH net-next v3 2/3] net: phy: dp83td510: add cable testing support

[Oleksij Rempel]

Cable testing was tested in different HW configurations and cables:
- SJA1105 + DP83TD510
- ASIX + DP83TD510
- STM32MP1 + DP83TD510

Results provided by this PHY should be interpreted with grain of salt.
For example testing unshielded and shielded twisted pair may give
different results. Nevertheless, it still can be usable.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/phy/dp83td510.c | 161 ++++++++++++++++++++++++++++++++++++
 1 file changed, 161 insertions(+)

diff --git a/drivers/net/phy/dp83td510.c b/drivers/net/phy/dp83td510.c
index 3cd9a77f9532..de32ab1a262d 100644
--- a/drivers/net/phy/dp83td510.c
+++ b/drivers/net/phy/dp83td510.c
@@ -4,6 +4,7 @@
  */
 
 #include <linux/bitfield.h>
+#include <linux/ethtool_netlink.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/phy.h>
@@ -24,6 +25,52 @@
 #define DP83TD510E_INT1_LINK			BIT(13)
 #define DP83TD510E_INT1_LINK_EN			BIT(5)
 
+#define DP83TD510E_TDR_CFG			0x1e
+#define DP83TD510E_TDR_START			BIT(15)
+#define DP83TD510E_TDR_DONE			BIT(1)
+#define DP83TD510E_TDR_FAIL			BIT(0)
+
+#define DP83TD510E_TDR_CFG1			0x300
+
+#define DP83TD510E_TDR_CFG2			0x301
+#define DP83TD510E_TDR_END_TAP_INDEX_1		GENMASK(14, 8)
+#define DP83TD510E_TDR_START_TAP_INDEX_1	GENMASK(6, 0)
+
+#define DP83TD510E_TDR_CFG3			0x302
+#define DP83TD510E_TDR_TX_DURATION_US		GENMASK(15, 0)
+
+#define DP83TD510E_TDR_FAULT_CFG1		0x303
+#define DP83TD510E_TDR_FLT_LOC_OFFSET_1		GENMASK(14, 8)
+#define DP83TD510E_TDR_FLT_INIT_1		GENMASK(7, 0)
+
+#define DP83TD510E_TDR_FAULT_CFG2		0x304
+#define DP83TD510E_TDR_FLT_SLOPE_1		GENMASK(7, 0)
+
+#define DP83TD510E_TDR_FAULT_STAT1		0x305
+#define DP83TD510E_TDR_FAULT_STAT2		0x306
+#define DP83TD510E_TDR_FAULT_STAT3		0x307
+#define DP83TD510E_TDR_FAULT_STAT4		0x308
+#define DP83TD510E_TDR_FAULT_STAT5		0x309
+#define DP83TD510E_TDR_FAULT_STAT6		0x30a
+
+#define DP83TD510E_TDR_FAULT_STAT		0x30c
+#define DP83TD510E_TDR_PEAK_DETECT		BIT(11)
+#define DP83TD510E_TDR_PEAK_SIGN		BIT(10)
+#define DP83TD510E_TDR_PEAK_LOCATION		GENMASK(9, 0)
+
+
+/* Not documented registers and values but recommended according to
+ * "DP83TD510E Cable Diagnostics Toolkit"
+ */
+#define DP83TD510E_UNKN_030D			0x30d
+#define DP83TD510E_030D_VAL			0x5f25
+#define DP83TD510E_UNKN_030E			0x30e
+#define DP83TD510E_030E_VAL			0x0536
+#define DP83TD510E_UNKN_030F			0x30f
+#define DP83TD510E_030F_VAL			0x0008
+#define DP83TD510E_UNKN_0310			0x310
+#define DP83TD510E_0310_VAL			0x0036
+
 #define DP83TD510E_AN_STAT_1			0x60c
 #define DP83TD510E_MASTER_SLAVE_RESOL_FAIL	BIT(15)
 
@@ -211,6 +258,117 @@ static int dp83td510_get_sqi_max(struct phy_device *phydev)
 	return DP83TD510_SQI_MAX;
 }
 
+/* Configure the TDR circuitry within the PHY as described in
+ * "Application Report - DP83TD510E Cable Diagnostics Toolkit"
+ */
+static int dp83td510_tdr_init(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_TDR_CFG2,
+			    FIELD_PREP(DP83TD510E_TDR_END_TAP_INDEX_1, 36) |
+			    FIELD_PREP(DP83TD510E_TDR_START_TAP_INDEX_1, 4));
+	if (ret)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_UNKN_030D,
+			    DP83TD510E_030D_VAL);
+	if (ret)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_TDR_FAULT_CFG1,
+			    FIELD_PREP(DP83TD510E_TDR_FLT_LOC_OFFSET_1, 0x5) |
+			    FIELD_PREP(DP83TD510E_TDR_FLT_INIT_1, 0x3e));
+	if (ret)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_UNKN_030E,
+			    DP83TD510E_030E_VAL);
+	if (ret)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_TDR_FAULT_CFG2,
+			    FIELD_PREP(DP83TD510E_TDR_FLT_SLOPE_1, 0xa));
+	if (ret)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_UNKN_030F,
+			    DP83TD510E_030F_VAL);
+	if (ret)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_TDR_CFG3,
+			    FIELD_PREP(DP83TD510E_TDR_TX_DURATION_US, 16000));
+	if (ret)
+		return ret;
+
+	return phy_write_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_UNKN_0310,
+			     DP83TD510E_0310_VAL);
+}
+
+static int dp83td510_cable_test_start(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = dp83td510_tdr_init(phydev);
+	if (ret)
+		return ret;
+
+	return phy_set_bits_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_TDR_CFG,
+				DP83TD510E_TDR_START);
+}
+
+static int dp83td510_cable_test_get_status(struct phy_device *phydev,
+					   bool *finished)
+{
+	int ret, stat;
+
+	*finished = false;
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_TDR_CFG);
+	if (ret < 0)
+		return ret;
+
+	if (!(ret & DP83TD510E_TDR_DONE))
+		return 0;
+
+	if (!(ret & DP83TD510E_TDR_FAIL)) {
+		int location;
+
+		ret = phy_read_mmd(phydev, MDIO_MMD_VEND2,
+				   DP83TD510E_TDR_FAULT_STAT);
+		if (ret < 0)
+			return ret;
+
+		if (ret & DP83TD510E_TDR_PEAK_DETECT) {
+			if (ret & DP83TD510E_TDR_PEAK_SIGN)
+				stat = ETHTOOL_A_CABLE_RESULT_CODE_OPEN;
+			else
+				stat = ETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT;
+
+			location = FIELD_GET(DP83TD510E_TDR_PEAK_LOCATION,
+					     ret) * 100;
+			ethnl_cable_test_fault_length(phydev,
+						      ETHTOOL_A_CABLE_PAIR_A,
+						      location);
+		} else {
+			stat = ETHTOOL_A_CABLE_RESULT_CODE_OK;
+		}
+	} else {
+		/* Most probably we have active link partner */
+		stat = ETHTOOL_A_CABLE_RESULT_CODE_UNSPEC;
+	}
+
+	*finished = true;
+
+	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_A, stat);
+
+	/* Reset state machine, otherwise at least other TDR attempts may
+	 * provide not reliable results.
+	 */
+	return phy_set_bits(phydev, MII_BMCR, BMCR_RESET);
+}
+
 static int dp83td510_get_features(struct phy_device *phydev)
 {
 	/* This PHY can't respond on MDIO bus if no RMII clock is enabled.
@@ -234,6 +392,7 @@ static struct phy_driver dp83td510_driver[] = {
 	PHY_ID_MATCH_MODEL(DP83TD510E_PHY_ID),
 	.name		= "TI DP83TD510E",
 
+	.flags          = PHY_POLL_CABLE_TEST,
 	.config_aneg	= dp83td510_config_aneg,
 	.read_status	= dp83td510_read_status,
 	.get_features	= dp83td510_get_features,
@@ -241,6 +400,8 @@ static struct phy_driver dp83td510_driver[] = {
 	.handle_interrupt = dp83td510_handle_interrupt,
 	.get_sqi	= dp83td510_get_sqi,
 	.get_sqi_max	= dp83td510_get_sqi_max,
+	.cable_test_start = dp83td510_cable_test_start,
+	.cable_test_get_status = dp83td510_cable_test_get_status,
 
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
-- 
2.30.2

Re: [PATCH net-next v3 2/3] net: phy: dp83td510: add cable testing support

[Andrew Lunn]

> +	} else {
> +		/* Most probably we have active link partner */
> +		stat = ETHTOOL_A_CABLE_RESULT_CODE_UNSPEC;
> +	}

So drivers will make a few attempts to get a valid result. Have you
tried that?

> +
> +	*finished = true;
> +
> +	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_A, stat);
> +
> +	/* Reset state machine, otherwise at least other TDR attempts may
> +	 * provide not reliable results.
> +	 */
> +	return phy_set_bits(phydev, MII_BMCR, BMCR_RESET);

I thought i made the comment that you should use the helper? The
helper will wait around for the bit to clear indicating the PHY is
ready for further configuration.

      Andrew

[PATCH net-next v3 3/3] net: phy: dp83td510: disable cable test support for 1Vpp PHYs

[Oleksij Rempel]

Using 1Vpp pulse provides most unreliable results. So, disable cable
testing if PHY is bootstrapped to use 1Vpp-only mode.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/phy/dp83td510.c | 52 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 51 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/dp83td510.c b/drivers/net/phy/dp83td510.c
index de32ab1a262d..1eaf2ceaca8c 100644
--- a/drivers/net/phy/dp83td510.c
+++ b/drivers/net/phy/dp83td510.c
@@ -31,6 +31,8 @@
 #define DP83TD510E_TDR_FAIL			BIT(0)
 
 #define DP83TD510E_TDR_CFG1			0x300
+/* TX_TYPE: Transmit voltage level for TDR. 0 = 1V, 1 = 2.4V */
+#define DP83TD510E_TDR_TX_TYPE			BIT(12)
 
 #define DP83TD510E_TDR_CFG2			0x301
 #define DP83TD510E_TDR_END_TAP_INDEX_1		GENMASK(14, 8)
@@ -71,6 +73,10 @@
 #define DP83TD510E_UNKN_0310			0x310
 #define DP83TD510E_0310_VAL			0x0036
 
+#define DP83TD510E_CHIP_SOR_1			0x467
+/* If LED_2 is set, blacklist 2.4V mode */
+#define DP83TD510E_SOR_LED_2			BIT(7)
+
 #define DP83TD510E_AN_STAT_1			0x60c
 #define DP83TD510E_MASTER_SLAVE_RESOL_FAIL	BIT(15)
 
@@ -78,6 +84,10 @@
 
 #define DP83TD510_SQI_MAX	7
 
+struct dp83td510_priv {
+	bool allow_v2_4_mode;
+};
+
 /* Register values are converted to SNR(dB) as suggested by
  * "Application Report - DP83TD510E Cable Diagnostics Toolkit":
  * SNR(dB) = -10 * log10 (VAL/2^17) - 1.76 dB.
@@ -308,12 +318,29 @@ static int dp83td510_tdr_init(struct phy_device *phydev)
 
 static int dp83td510_cable_test_start(struct phy_device *phydev)
 {
-	int ret;
+	struct dp83td510_priv *priv = phydev->priv;
+	int ret, cfg = 0;
+
+	/* Generate 2.4Vpp pulse if HW is allowed to do so */
+	if (priv->allow_v2_4_mode) {
+		cfg |= DP83TD510E_TDR_TX_TYPE;
+	} else {
+		/* This PHY do not provide usable results with 1Vpp pulse.
+		 * Potentially different dp83td510_tdr_init() values are
+		 * needed.
+		 */
+		return -EOPNOTSUPP;
+	}
 
 	ret = dp83td510_tdr_init(phydev);
 	if (ret)
 		return ret;
 
+	ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_TDR_CFG1,
+			     DP83TD510E_TDR_TX_TYPE, cfg);
+	if (ret)
+		return ret;
+
 	return phy_set_bits_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_TDR_CFG,
 				DP83TD510E_TDR_START);
 }
@@ -369,6 +396,28 @@ static int dp83td510_cable_test_get_status(struct phy_device *phydev,
 	return phy_set_bits(phydev, MII_BMCR, BMCR_RESET);
 }
 
+static int dp83td510_probe(struct phy_device *phydev)
+{
+	struct device *dev = &phydev->mdio.dev;
+	struct dp83td510_priv *priv;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	phydev->priv = priv;
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_CHIP_SOR_1);
+	if (ret < 0)
+		return ret;
+
+	if (!(ret & DP83TD510E_SOR_LED_2))
+		priv->allow_v2_4_mode = true;
+
+	return 0;
+}
+
 static int dp83td510_get_features(struct phy_device *phydev)
 {
 	/* This PHY can't respond on MDIO bus if no RMII clock is enabled.
@@ -393,6 +442,7 @@ static struct phy_driver dp83td510_driver[] = {
 	.name		= "TI DP83TD510E",
 
 	.flags          = PHY_POLL_CABLE_TEST,
+	.probe		= dp83td510_probe,
 	.config_aneg	= dp83td510_config_aneg,
 	.read_status	= dp83td510_read_status,
 	.get_features	= dp83td510_get_features,
-- 
2.30.2

Re: [PATCH net-next v3 3/3] net: phy: dp83td510: disable cable test support for 1Vpp PHYs

[Andrew Lunn]

>  static int dp83td510_cable_test_start(struct phy_device *phydev)
>  {
> -	int ret;
> +	struct dp83td510_priv *priv = phydev->priv;
> +	int ret, cfg = 0;
> +
> +	/* Generate 2.4Vpp pulse if HW is allowed to do so */
> +	if (priv->allow_v2_4_mode) {
> +		cfg |= DP83TD510E_TDR_TX_TYPE;
> +	} else {
> +		/* This PHY do not provide usable results with 1Vpp pulse.

s/do/does


> +		 * Potentially different dp83td510_tdr_init() values are
> +		 * needed.
> +		 */
> +		return -EOPNOTSUPP;
> +	}

I don't remember the details for v2.4. Is it possible to change up
from 1v to 2.4v for the duration of the cable test? Is there a danger
to damage the link peer? I guess not, since you need to pass EMC
testing which zaps it with 100Kv or something. So is this more a local
supply issue?

       Andrew

Re: [PATCH net-next v3 3/3] net: phy: dp83td510: disable cable test support for 1Vpp PHYs

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1405 bytes --]

On 11.06.2022 17:16:06, Andrew Lunn wrote:
> >  static int dp83td510_cable_test_start(struct phy_device *phydev)
> >  {
> > -	int ret;
> > +	struct dp83td510_priv *priv = phydev->priv;
> > +	int ret, cfg = 0;
> > +
> > +	/* Generate 2.4Vpp pulse if HW is allowed to do so */
> > +	if (priv->allow_v2_4_mode) {
> > +		cfg |= DP83TD510E_TDR_TX_TYPE;
> > +	} else {
> > +		/* This PHY do not provide usable results with 1Vpp pulse.
> 
> s/do/does
> 
> 
> > +		 * Potentially different dp83td510_tdr_init() values are
> > +		 * needed.
> > +		 */
> > +		return -EOPNOTSUPP;
> > +	}
> 
> I don't remember the details for v2.4. Is it possible to change up
> from 1v to 2.4v for the duration of the cable test? Is there a danger
> to damage the link peer? I guess not, since you need to pass EMC
> testing which zaps it with 100Kv or something. So is this more a local
> supply issue?

In some industrial areas (e.g. ATEX (explosion protection)) 2.4V is not
allowed due to regulations. If the PHY is strapped to 1.0V we assume for
now this has a good reason and don't crank up the voltage :)

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH net-next v3 0/3] net: phy: dp83td510: add cable diag support

[Jakub Kicinski]

On Wed,  8 Jun 2022 14:32:33 +0200 Oleksij Rempel wrote:
> changes v3:
> - spell fixes

FWIW relatively recently we started asking folks to wait 24h before
reposting:

https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html#i-have-received-review-feedback-when-should-i-post-a-revised-version-of-the-patches

Would be doubly useful here since it sounds like Andrew is out for 
the week.