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

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

[João Rodrigues]

This series adds more diagnostics of the physical medium to the DP83867.
The TDR reporting is similar to DP83869, so there might be some interest
in porting this changes to that driver.
The TDR reporting in PD83867 is divided into segments (from which a
cable length can be extracted). Because the reported lengths do not come
in regular intervals, when doing cable-test-tdr from ethtool, the value
of the reflection is reported, but not the length at which occurred
(even though the PHY reports it).
Likewise, the configuration of measurement lengths is also not
supported in this series, for the same reason (it is theoretically
possible to do it, with more complex configuration)

João Rodrigues (3):
  net: phy: dp83867: Add SQI support
  net: phy: dp83867: add cable test support
  net: phy: dp83867: Add support for amplitude graph

 drivers/net/phy/dp83867.c | 353 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 353 insertions(+)

-- 
2.25.1

[PATCH net-next 1/3] net: phy: dp83867: Add SQI support

[João Rodrigues]

Don't report SQI values for 10 ethernet, since the datasheet
says MSE values are only valid for 100/1000 ethernet

Signed-off-by: João Rodrigues <jrodrigues@ubimet.com>
---
 drivers/net/phy/dp83867.c | 51 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index 4120385c5a79..5741f09e29cb 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -53,6 +53,7 @@
 #define DP83867_RXFSOP2	0x013A
 #define DP83867_RXFSOP3	0x013B
 #define DP83867_IO_MUX_CFG	0x0170
+#define DP83867_MSE_REG_1	0x0225
 #define DP83867_SGMIICTL	0x00D3
 #define DP83867_10M_SGMII_CFG   0x016F
 #define DP83867_10M_SGMII_RATE_ADAPT_MASK BIT(7)
@@ -153,6 +154,9 @@
 /* FLD_THR_CFG */
 #define DP83867_FLD_THR_CFG_ENERGY_LOST_THR_MASK	0x7
 
+/* SQI */
+#define DP83867_MAX_SQI	0x07
+
 #define DP83867_LED_COUNT	4
 
 /* LED_DRV bits */
@@ -196,6 +200,24 @@ struct dp83867_private {
 	bool sgmii_ref_clk_en;
 };
 
+/* Register values are converted to SNR(dB) as suggested by
+ * "How to utilize diagnostic test suite of Ethernet PHY":
+ * SNR(dB) = -10 * log10 (VAL/2^17) - 3 dB.
+ * SQI ranges are implemented according to "OPEN ALLIANCE - Advanced diagnostic
+ * features for 100BASE-T1 automotive Ethernet PHYs"
+ */
+
+static const u16 dp83867_mse_sqi_map[] = {
+	0x0411, /* < 18dB */
+	0x033b, /* 18dB =< SNR < 19dB */
+	0x0290, /* 19dB =< SNR < 20dB */
+	0x0209, /* 20dB =< SNR < 21dB */
+	0x019e, /* 21dB =< SNR < 22dB */
+	0x0149, /* 22dB =< SNR < 23dB */
+	0x0105, /* 23dB =< SNR < 24dB */
+	0x0000  /* 24dB =< SNR */
+};
+
 static int dp83867_ack_interrupt(struct phy_device *phydev)
 {
 	int err = phy_read(phydev, MII_DP83867_ISR);
@@ -1015,6 +1037,32 @@ static int dp83867_loopback(struct phy_device *phydev, bool enable)
 			  enable ? BMCR_LOOPBACK : 0);
 }
 
+static int dp83867_get_sqi(struct phy_device *phydev)
+{
+	u16 mse_val;
+	int sqi;
+	int ret;
+
+	if (phydev->speed == SPEED_10)
+		return -EOPNOTSUPP;
+
+	ret = phy_read_mmd(phydev, DP83867_DEVADDR, DP83867_MSE_REG_1);
+	if (ret < 0)
+		return ret;
+
+	mse_val = 0xFFFF & ret;
+	for (sqi = 0; sqi < ARRAY_SIZE(dp83867_mse_sqi_map); sqi++) {
+		if (mse_val >= dp83867_mse_sqi_map[sqi])
+			return sqi;
+	}
+	return -EINVAL;
+}
+
+static int dp83867_get_sqi_max(struct phy_device *phydev)
+{
+	return DP83867_MAX_SQI;
+}
+
 static int
 dp83867_led_brightness_set(struct phy_device *phydev,
 			   u8 index, enum led_brightness brightness)
@@ -1195,6 +1243,9 @@ static struct phy_driver dp83867_driver[] = {
 		.config_intr	= dp83867_config_intr,
 		.handle_interrupt = dp83867_handle_interrupt,
 
+		.get_sqi	= dp83867_get_sqi,
+		.get_sqi_max	= dp83867_get_sqi_max,
+
 		.suspend	= dp83867_suspend,
 		.resume		= dp83867_resume,
 
-- 
2.25.1

[PATCH net-next 2/3] net: phy: dp83867: add cable test support

[João Rodrigues]

TI DP83867 returns TDR information into 5 segments
for each of the cable.
Implement the testing based on "Time Domain Reflectometry with DP83867
and DP83869"

Signed-off-by: João Rodrigues <jrodrigues@ubimet.com>
---
 drivers/net/phy/dp83867.c | 228 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 228 insertions(+)

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index 5741f09e29cb..ff8c97a29195 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -5,6 +5,7 @@
  */
 
 #include <linux/ethtool.h>
+#include <linux/ethtool_netlink.h>
 #include <linux/kernel.h>
 #include <linux/mii.h>
 #include <linux/module.h>
@@ -53,6 +54,14 @@
 #define DP83867_RXFSOP2	0x013A
 #define DP83867_RXFSOP3	0x013B
 #define DP83867_IO_MUX_CFG	0x0170
+#define DP83867_TDR_GEN_CFG5	0x0186
+#define DP83867_TDR_GEN_CFG6	0x0187
+#define DP83867_TDR_GEN_CFG7	0x0189
+#define DP83867_TDR_PEAKS_LOC_1	0x0190
+#define DP83867_TDR_PEAKS_AMP_1	0x019A
+#define DP83867_TDR_GEN_STATUS	0x01A4
+#define DP83867_TDR_PEAKS_SIGN_1	0x01A5
+#define DP83867_TDR_PEAKS_SIGN_2	0x01A6
 #define DP83867_MSE_REG_1	0x0225
 #define DP83867_SGMIICTL	0x00D3
 #define DP83867_10M_SGMII_CFG   0x016F
@@ -157,6 +166,22 @@
 /* SQI */
 #define DP83867_MAX_SQI	0x07
 
+/* TDR bits */
+#define DP83867_TDR_GEN_CFG5_FLAGS	0x294A
+#define DP83867_TDR_GEN_CFG6_FLAGS	0x0A9B
+#define DP83867_TDR_GEN_CFG7_FLAGS	0x0000
+#define DP83867_TDR_START	BIT(0)
+#define DP83867_TDR_DONE	BIT(1)
+#define DP83867_TDR_FAIL	BIT(2)
+#define DP83867_TDR_PEAKS_LOC_LOW	GENMASK(7, 0)
+#define DP83867_TDR_PEAKS_LOC_HIGH	GENMASK(15, 8)
+#define DP83867_TDR_PEAKS_AMP_LOW	GENMASK(6, 0)
+#define DP83867_TDR_PEAKS_AMP_HIGH	GENMASK(14, 8)
+#define DP83867_TDR_INITIAL_THRESHOLD 10
+#define DP83867_TDR_FINAL_THRESHOLD 24
+#define DP83867_TDR_OFFSET	16
+#define DP83867_TDR_P_LOC_CROSS_MODE_SHIFT	8
+
 #define DP83867_LED_COUNT	4
 
 /* LED_DRV bits */
@@ -1037,6 +1062,205 @@ static int dp83867_loopback(struct phy_device *phydev, bool enable)
 			  enable ? BMCR_LOOPBACK : 0);
 }
 
+static u32 dp83867_cycles2cm(u32 cycles)
+{
+	/* for cat. 6 cable, signals travel at 5 ns / m */
+	return cycles * 8 * 20;
+}
+
+static int dp83867_cable_test_start(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_TDR_GEN_CFG7,
+			    DP83867_TDR_GEN_CFG7_FLAGS);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_TDR_GEN_CFG6,
+			    DP83867_TDR_GEN_CFG6_FLAGS);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_TDR_GEN_CFG5,
+			    DP83867_TDR_GEN_CFG5_FLAGS);
+	if (ret < 0)
+		return ret;
+
+	return phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_CFG3,
+			     DP83867_TDR_START);
+}
+
+static int dp83867_cable_test_report_trans(struct phy_device *phydev, s8 pair,
+					   int *peak_location, int *peak_value,
+					   int *peak_sign,
+					   unsigned int number_peaks)
+{
+	int fault_rslt = ETHTOOL_A_CABLE_RESULT_CODE_OK;
+	int threshold = DP83867_TDR_INITIAL_THRESHOLD;
+	unsigned long cross_result;
+	u32 fault_location = 0;
+	int i;
+	int ret;
+
+	for (i = number_peaks; i >= 0; --i) {
+		if (peak_value[i] > threshold) {
+			fault_location =
+				dp83867_cycles2cm(peak_location[i] -
+					DP83867_TDR_OFFSET) / 2;
+			if (peak_sign[i] == 1) {
+				fault_rslt =
+					ETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT;
+			} else {
+				fault_rslt = ETHTOOL_A_CABLE_RESULT_CODE_OPEN;
+			}
+			break;
+		}
+		if (i == 1)
+			threshold = DP83867_TDR_FINAL_THRESHOLD;
+	}
+
+	ret = phy_read_mmd(phydev, DP83867_DEVADDR, DP83867_TDR_GEN_STATUS);
+	if (ret < 0)
+		return ret;
+
+	cross_result = ret;
+
+	if (test_bit(DP83867_TDR_P_LOC_CROSS_MODE_SHIFT + pair -
+		     ETHTOOL_A_CABLE_PAIR_A, &cross_result))
+		fault_rslt = ETHTOOL_A_CABLE_RESULT_CODE_CROSS_SHORT;
+
+	ret = ethnl_cable_test_result(phydev, pair, fault_rslt);
+	if (ret < 0)
+		return ret;
+
+	if (fault_rslt != ETHTOOL_A_CABLE_RESULT_CODE_OK) {
+		return ethnl_cable_test_fault_length(phydev, pair,
+						     fault_location);
+	}
+	return 0;
+}
+
+static int dp83867_read_tdr_registers(struct phy_device *phydev, s8 pair,
+				      int *peak_loc, int *peak_val,
+				      int *peak_sign,
+				      unsigned int number_peaks)
+{
+	u32 segment_dist, register_dist;
+	unsigned long peak_sign_result;
+	int ret;
+	int i;
+
+	segment_dist = (pair - ETHTOOL_A_CABLE_PAIR_A) * 5;
+	for (i = 0; i < number_peaks; ++i) {
+		register_dist = (segment_dist + i) / 2;
+		ret = phy_read_mmd(phydev, DP83867_DEVADDR,
+				   DP83867_TDR_PEAKS_LOC_1 + register_dist);
+		if (ret < 0)
+			return ret;
+		if (((register_dist + i) % 2) == 0)
+			peak_loc[i] = FIELD_GET(DP83867_TDR_PEAKS_LOC_LOW, ret);
+		else
+			peak_loc[i] = FIELD_GET(DP83867_TDR_PEAKS_LOC_HIGH, ret);
+
+		ret = phy_read_mmd(phydev, DP83867_DEVADDR,
+				   DP83867_TDR_PEAKS_AMP_1 + register_dist);
+		if (ret < 0)
+			return ret;
+		if (((register_dist + i) % 2) == 0)
+			peak_val[i] = FIELD_GET(DP83867_TDR_PEAKS_AMP_LOW, ret);
+		else
+			peak_val[i] = FIELD_GET(DP83867_TDR_PEAKS_AMP_HIGH, ret);
+	}
+
+	switch (pair) {
+	case ETHTOOL_A_CABLE_PAIR_A:
+	case ETHTOOL_A_CABLE_PAIR_B:
+		ret = phy_read_mmd(phydev, DP83867_DEVADDR,
+				   DP83867_TDR_PEAKS_SIGN_1);
+		break;
+	case ETHTOOL_A_CABLE_PAIR_C:
+	case ETHTOOL_A_CABLE_PAIR_D:
+		ret = phy_read_mmd(phydev, DP83867_DEVADDR,
+				   DP83867_TDR_PEAKS_SIGN_2);
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+	if (ret < 0)
+		return ret;
+
+	peak_sign_result = ret;
+
+	switch (pair) {
+	case ETHTOOL_A_CABLE_PAIR_A:
+	case ETHTOOL_A_CABLE_PAIR_C:
+		for (i = 0; i < number_peaks; i++)
+			peak_sign[i] = test_bit(i, &peak_sign_result);
+		break;
+	case ETHTOOL_A_CABLE_PAIR_B:
+	case ETHTOOL_A_CABLE_PAIR_D:
+		for (i = 0; i < number_peaks; i++)
+			peak_sign[i] = test_bit(i + 5, &peak_sign_result);
+		break;
+	}
+
+	return 0;
+}
+
+static int dp83867_cable_test_report_pair(struct phy_device *phydev, s8 pair)
+{
+	int peak_sign[5];
+	int peak_loc[ARRAY_SIZE(peak_sign)];
+	int peak_val[ARRAY_SIZE(peak_sign)];
+	int ret;
+
+	ret = dp83867_read_tdr_registers(phydev, pair, peak_loc, peak_val,
+					 peak_sign, ARRAY_SIZE(peak_sign));
+	if (ret < 0)
+		return ret;
+	return dp83867_cable_test_report_trans(phydev, pair, peak_loc,
+					       peak_val, peak_sign,
+					       ARRAY_SIZE(peak_sign));
+}
+
+static int dp83867_cable_test_report(struct phy_device *phydev)
+{
+	s8 pair;
+	int ret;
+
+	for (pair = ETHTOOL_A_CABLE_PAIR_A; pair <= ETHTOOL_A_CABLE_PAIR_D;
+	     ++pair) {
+		ret = dp83867_cable_test_report_pair(phydev, pair);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int dp83867_cable_test_get_status(struct phy_device *phydev,
+					 bool *finished)
+{
+	int ret;
+
+	*finished = false;
+
+	ret = phy_read_mmd(phydev, DP83867_DEVADDR, DP83867_CFG3);
+	if (ret < 0)
+		return ret;
+
+	if (!(ret & DP83867_TDR_DONE))
+		return 0;
+
+	*finished = true;
+
+	if (ret & DP83867_TDR_FAIL)
+		return -EBUSY;
+
+	return dp83867_cable_test_report(phydev);
+}
+
 static int dp83867_get_sqi(struct phy_device *phydev)
 {
 	u16 mse_val;
@@ -1226,6 +1450,7 @@ static struct phy_driver dp83867_driver[] = {
 		.phy_id		= DP83867_PHY_ID,
 		.phy_id_mask	= 0xfffffff0,
 		.name		= "TI DP83867",
+		.flags		= PHY_POLL_CABLE_TEST,
 		/* PHY_GBIT_FEATURES */
 
 		.probe          = dp83867_probe,
@@ -1252,6 +1477,9 @@ static struct phy_driver dp83867_driver[] = {
 		.link_change_notify = dp83867_link_change_notify,
 		.set_loopback	= dp83867_loopback,
 
+		.cable_test_start	= dp83867_cable_test_start,
+		.cable_test_get_status	= dp83867_cable_test_get_status,
+
 		.led_brightness_set = dp83867_led_brightness_set,
 		.led_hw_is_supported = dp83867_led_hw_is_supported,
 		.led_hw_control_set = dp83867_led_hw_control_set,
-- 
2.25.1

[PATCH net-next 3/3] net: phy: dp83867: Add support for amplitude graph

[João Rodrigues]

Output the raw information of each segment.
Each segment also comes with the distance information,
but this does not map to the current output.

Signed-off-by: João Rodrigues <jrodrigues@ubimet.com>
---
 drivers/net/phy/dp83867.c | 86 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 80 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index ff8c97a29195..e56a892d3da7 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -54,6 +54,8 @@
 #define DP83867_RXFSOP2	0x013A
 #define DP83867_RXFSOP3	0x013B
 #define DP83867_IO_MUX_CFG	0x0170
+#define DP83867_TDR_GEN_CFG2	0x0181
+#define DP83867_TDR_GEN_CFG4	0x0185
 #define DP83867_TDR_GEN_CFG5	0x0186
 #define DP83867_TDR_GEN_CFG6	0x0187
 #define DP83867_TDR_GEN_CFG7	0x0189
@@ -223,6 +225,8 @@ struct dp83867_private {
 	bool set_clk_output;
 	u32 clk_output_sel;
 	bool sgmii_ref_clk_en;
+	bool cable_test_tdr;
+	s8 pair;
 };
 
 /* Register values are converted to SNR(dB) as suggested by
@@ -1068,7 +1072,7 @@ static u32 dp83867_cycles2cm(u32 cycles)
 	return cycles * 8 * 20;
 }
 
-static int dp83867_cable_test_start(struct phy_device *phydev)
+static int dp83867_cable_test_common(struct phy_device *phydev)
 {
 	int ret;
 
@@ -1082,11 +1086,38 @@ static int dp83867_cable_test_start(struct phy_device *phydev)
 	if (ret < 0)
 		return ret;
 
-	ret = phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_TDR_GEN_CFG5,
-			    DP83867_TDR_GEN_CFG5_FLAGS);
+	return phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_TDR_GEN_CFG5,
+			     DP83867_TDR_GEN_CFG5_FLAGS);
+}
+
+static int dp83867_cable_test_start(struct phy_device *phydev)
+{
+	struct dp83867_private *priv = phydev->priv;
+	int ret;
+
+	ret = dp83867_cable_test_common(phydev);
+	if (ret < 0)
+		return ret;
+
+	priv->cable_test_tdr = false;
+
+	return phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_CFG3,
+			     DP83867_TDR_START);
+}
+
+static int dp83867_cable_test_tdr_start(struct phy_device *phydev,
+					const struct phy_tdr_config *cfg)
+{
+	struct dp83867_private *priv = phydev->priv;
+	int ret;
+
+	ret = dp83867_cable_test_common(phydev);
 	if (ret < 0)
 		return ret;
 
+	priv->cable_test_tdr = true;
+	priv->pair = cfg->pair;
+
 	return phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_CFG3,
 			     DP83867_TDR_START);
 }
@@ -1105,9 +1136,14 @@ static int dp83867_cable_test_report_trans(struct phy_device *phydev, s8 pair,
 
 	for (i = number_peaks; i >= 0; --i) {
 		if (peak_value[i] > threshold) {
-			fault_location =
-				dp83867_cycles2cm(peak_location[i] -
+			if (peak_location[i] >= DP83867_TDR_OFFSET) {
+				fault_location = dp83867_cycles2cm(peak_location[i] -
 					DP83867_TDR_OFFSET) / 2;
+			} else {
+				phydev_dbg(phydev, "Returned TDR fault location is too low");
+				fault_location = dp83867_cycles2cm(peak_location[i]) / 2;
+			}
+
 			if (peak_sign[i] == 1) {
 				fault_rslt =
 					ETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT;
@@ -1208,6 +1244,39 @@ static int dp83867_read_tdr_registers(struct phy_device *phydev, s8 pair,
 	return 0;
 }
 
+static int dp83867_simplified_amplitude_graph(struct phy_device *phydev)
+{
+	struct dp83867_private *priv = phydev->priv;
+	int peak_sign[5];
+	int peak_loc[ARRAY_SIZE(peak_sign)];
+	int peak_val[ARRAY_SIZE(peak_sign)];
+	int i;
+	int mV;
+	int ret;
+	s8 pair;
+
+	for (pair = ETHTOOL_A_CABLE_PAIR_A; pair <= ETHTOOL_A_CABLE_PAIR_D;
+			pair++) {
+		if (priv->pair != PHY_PAIR_ALL && pair != priv->pair)
+			continue;
+
+		ret = dp83867_read_tdr_registers(phydev, pair, peak_loc,
+						 peak_val, peak_sign,
+						 ARRAY_SIZE(peak_sign));
+		if (ret < 0)
+			return ret;
+
+		for (i = 0; i < ARRAY_SIZE(peak_loc); i++) {
+			mV = peak_val[i];
+			if (peak_sign[i])
+				mV *= -1;
+
+			ethnl_cable_test_amplitude(phydev, pair, mV);
+		}
+	}
+	return 0;
+}
+
 static int dp83867_cable_test_report_pair(struct phy_device *phydev, s8 pair)
 {
 	int peak_sign[5];
@@ -1242,6 +1311,7 @@ static int dp83867_cable_test_report(struct phy_device *phydev)
 static int dp83867_cable_test_get_status(struct phy_device *phydev,
 					 bool *finished)
 {
+	struct dp83867_private *priv = phydev->priv;
 	int ret;
 
 	*finished = false;
@@ -1258,7 +1328,10 @@ static int dp83867_cable_test_get_status(struct phy_device *phydev,
 	if (ret & DP83867_TDR_FAIL)
 		return -EBUSY;
 
-	return dp83867_cable_test_report(phydev);
+	if (priv->cable_test_tdr)
+		return dp83867_simplified_amplitude_graph(phydev);
+	else
+		return dp83867_cable_test_report(phydev);
 }
 
 static int dp83867_get_sqi(struct phy_device *phydev)
@@ -1478,6 +1551,7 @@ static struct phy_driver dp83867_driver[] = {
 		.set_loopback	= dp83867_loopback,
 
 		.cable_test_start	= dp83867_cable_test_start,
+		.cable_test_tdr_start	= dp83867_cable_test_tdr_start,
 		.cable_test_get_status	= dp83867_cable_test_get_status,
 
 		.led_brightness_set = dp83867_led_brightness_set,
-- 
2.25.1

Re: [PATCH net-next 1/3] net: phy: dp83867: Add SQI support

[Andrew Lunn]

On Thu, Jun 13, 2024 at 04:51:51PM +0200, João Rodrigues wrote:
> Don't report SQI values for 10 ethernet, since the datasheet
> says MSE values are only valid for 100/1000 ethernet

The commit message could be better. Something like:

Don't report the SQI value when the link speed is 10Mbps, since the
datasheet says MSE values are only valid for 100/1000 links.

> +static int dp83867_get_sqi(struct phy_device *phydev)
> +{
> +	u16 mse_val;
> +	int sqi;
> +	int ret;
> +
> +	if (phydev->speed == SPEED_10)
> +		return -EOPNOTSUPP;

What does the datasheet say about MSE where there is no link at all?
Maybe you need to expand this test to include SPEED_UNKNOWN?

    Andrew

---
pw-bot: cr

Re: [PATCH net-next 2/3] net: phy: dp83867: add cable test support

[Andrew Lunn]

> +/* TDR bits */
> +#define DP83867_TDR_GEN_CFG5_FLAGS	0x294A
> +#define DP83867_TDR_GEN_CFG6_FLAGS	0x0A9B

Is it documented what these bits actually mean?

   Andrew

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

[Andrew Lunn]

> The TDR reporting in PD83867 is divided into segments (from which a
> cable length can be extracted). Because the reported lengths do not come
> in regular intervals, when doing cable-test-tdr from ethtool, the value
> of the reflection is reported, but not the length at which occurred
> (even though the PHY reports it).

So what does the output look like?

   Andrew

Re: [PATCH net-next 1/3] net: phy: dp83867: Add SQI support

[João Rodrigues]

On Thu, 13 Jun 2024 19:13:27 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> On Thu, Jun 13, 2024 at 04:51:51PM +0200, João Rodrigues wrote:
> > Don't report SQI values for 10 ethernet, since the datasheet
> > says MSE values are only valid for 100/1000 ethernet  
> 
> The commit message could be better. Something like:
> 
> Don't report the SQI value when the link speed is 10Mbps, since the
> datasheet says MSE values are only valid for 100/1000 links.
> 

Thank you, I will use your wording on the next version.

> > +static int dp83867_get_sqi(struct phy_device *phydev)
> > +{
> > +	u16 mse_val;
> > +	int sqi;
> > +	int ret;
> > +
> > +	if (phydev->speed == SPEED_10)
> > +		return -EOPNOTSUPP;  
> 
> What does the datasheet say about MSE where there is no link at all?
> Maybe you need to expand this test to include SPEED_UNKNOWN?
> 
The datasheet does not have any information regarding this register
(or the related 0x265, 0x2A5 and 0x2E5, for the other pairs).
The information from these registers come from the "DP83867
Troubleshooting Guide (Rev. B)".

I will add the additional check for SPEED_UNKNOWN in the next
version.

João

Re: [PATCH net-next 2/3] net: phy: dp83867: add cable test support

[João Rodrigues]

On Thu, 13 Jun 2024 19:19:45 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> > +/* TDR bits */
> > +#define DP83867_TDR_GEN_CFG5_FLAGS	0x294A
> > +#define DP83867_TDR_GEN_CFG6_FLAGS	0x0A9B  
> 
> Is it documented what these bits actually mean?
> 
>    Andrew

No, all three (CFG5, CFG6 and CFG7) are undocumented.

João

Re: [PATCH net-next 2/3] net: phy: dp83867: add cable test support

[Andrew Lunn]

On Fri, Jun 14, 2024 at 06:52:10PM +0200, João Rodrigues wrote:
> On Thu, 13 Jun 2024 19:19:45 +0200
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
> > > +/* TDR bits */
> > > +#define DP83867_TDR_GEN_CFG5_FLAGS	0x294A
> > > +#define DP83867_TDR_GEN_CFG6_FLAGS	0x0A9B  
> > 
> > Is it documented what these bits actually mean?
> > 
> >    Andrew
> 
> No, all three (CFG5, CFG6 and CFG7) are undocumented.

Then i would suggest you use the magic values directly.  The only
advantage i see with DP83867_TDR_GEN_CFG5_FLAGS is that is stops you
accidentally using it with CFG4, etc. But magic is magic, and you
should not really hide it.

       Andrew