diff mbox series

[net-next,v1,1/1] net: phy: dp83td510: add cable testing support

Message ID 20240708140542.2424824-1-o.rempel@pengutronix.de (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net-next,v1,1/1] net: phy: dp83td510: add cable testing support | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 816 this patch: 816
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 1 maintainers not CCed: linux@armlinux.org.uk
netdev/build_clang success Errors and warnings before: 821 this patch: 821
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 821 this patch: 821
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 188 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc fail Errors and warnings before: 0 this patch: 1
netdev/source_inline success Was 0 now: 0

Commit Message

Oleksij Rempel July 8, 2024, 2:05 p.m. UTC 
Implement the TDR test procedure as described in "Application Note
DP83TD510E Cable Diagnostics Toolkit revC", section 3.2.

The procedure was tested with "draka 08 signalkabel 2x0.8mm". The reported
cable length was 5 meters more for each 20 meters of actual cable length.
For instance, a 20-meter cable showed as 25 meters, and a 40-meter cable
showed as 50 meters. Since other parts of the diagnostics provided by this
PHY (e.g., Active Link Cable Diagnostics) require accurate cable
characterization to provide proper results, this tuning can be implemented
in a separate patch/interface.

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

Comments

Andrew Lunn July 8, 2024, 4:44 p.m. UTC | #1 
On Mon, Jul 08, 2024 at 04:05:42PM +0200, Oleksij Rempel wrote:
> Implement the TDR test procedure as described in "Application Note
> DP83TD510E Cable Diagnostics Toolkit revC", section 3.2.
> 
> The procedure was tested with "draka 08 signalkabel 2x0.8mm". The reported
> cable length was 5 meters more for each 20 meters of actual cable length.
> For instance, a 20-meter cable showed as 25 meters, and a 40-meter cable
> showed as 50 meters. Since other parts of the diagnostics provided by this
> PHY (e.g., Active Link Cable Diagnostics) require accurate cable
> characterization to provide proper results, this tuning can be implemented
> in a separate patch/interface.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/phy/dp83td510.c | 158 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 158 insertions(+)
> 
> diff --git a/drivers/net/phy/dp83td510.c b/drivers/net/phy/dp83td510.c
> index d7616b13c5946..3375fa82927d0 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>
> @@ -29,6 +30,42 @@
>  #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_CTRL				0x1f
> +#define DP83TD510E_CTRL_HW_RESET		BIT(15)
> +#define DP83TD510E_CTRL_SW_RESET		BIT(14)
> +
> +#define DP83TD510E_TDR_CFG1			0x300

> +/* TX_TYPE: Transmit voltage level for TDR. 0 = 1V, 1 = 2.4V */
> +#define DP83TD510E_TDR_TX_TYPE			BIT(12)

This does not appear to be used, so it is not too important. But i
generally encode this as

#define DP83TD510E_TDR_TX_TYPE_1V		(0 << 12)
#define DP83TD510E_TDR_TX_TYPE_2V4		(1 << 12)

You can then OR in DP83TD510E_TDR_TX_TYPE_1V which does nothing, but
does document you are using 1V for the test.

> +#define DP83TD510E_TDR_CFG2			0x301
> +#define DP83TD510E_TDR_END_TAP_INDEX_1		GENMASK(14, 8)
> +#define DP83TD510E_TDR_END_TAP_INDEX_1_DEF	36
> +#define DP83TD510E_TDR_START_TAP_INDEX_1	GENMASK(6, 0)
> +#define DP83TD510E_TDR_START_TAP_INDEX_1_DEF	3

Does this correspond the minimum and maximum distance it will test?
Is this 3m to 36m?

   Andrew
Simon Horman July 9, 2024, 12:19 p.m. UTC | #2 
On Mon, Jul 08, 2024 at 04:05:42PM +0200, Oleksij Rempel wrote:
> Implement the TDR test procedure as described in "Application Note
> DP83TD510E Cable Diagnostics Toolkit revC", section 3.2.
> 
> The procedure was tested with "draka 08 signalkabel 2x0.8mm". The reported
> cable length was 5 meters more for each 20 meters of actual cable length.
> For instance, a 20-meter cable showed as 25 meters, and a 40-meter cable
> showed as 50 meters. Since other parts of the diagnostics provided by this
> PHY (e.g., Active Link Cable Diagnostics) require accurate cable
> characterization to provide proper results, this tuning can be implemented
> in a separate patch/interface.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/phy/dp83td510.c | 158 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 158 insertions(+)
> 
> diff --git a/drivers/net/phy/dp83td510.c b/drivers/net/phy/dp83td510.c

...

> +/**
> + * dp83td510_cable_test_get_status - Get the status of the cable test for the
> + *                                   DP83TD510 PHY.
> + * @phydev: Pointer to the phy_device structure.
> + * @finished: Pointer to a boolean that indicates whether the test is finished.
> + *
> + * The function sets the @finished flag to true if the test is complete, and
> + * returns 0 on success or a negative error code on failure.

Hi Oleksij,

Please consider documenting the return value using a "Return:" or
"Returns:" section in this Kernel doc.

Flagged by: kernel-doc -none -Wall

> + */
> +static int dp83td510_cable_test_get_status(struct phy_device *phydev,
> +					   bool *finished)

...
Oleksij Rempel July 12, 2024, 2:36 p.m. UTC | #3 
Hi Andrew,

On Mon, Jul 08, 2024 at 06:44:22PM +0200, Andrew Lunn wrote:
> On Mon, Jul 08, 2024 at 04:05:42PM +0200, Oleksij Rempel wrote:
> > Implement the TDR test procedure as described in "Application Note
> > DP83TD510E Cable Diagnostics Toolkit revC", section 3.2.
> > 
> > The procedure was tested with "draka 08 signalkabel 2x0.8mm". The reported
> > cable length was 5 meters more for each 20 meters of actual cable length.
> > For instance, a 20-meter cable showed as 25 meters, and a 40-meter cable
> > showed as 50 meters. Since other parts of the diagnostics provided by this
> > PHY (e.g., Active Link Cable Diagnostics) require accurate cable
> > characterization to provide proper results, this tuning can be implemented
> > in a separate patch/interface.
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> >  drivers/net/phy/dp83td510.c | 158 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 158 insertions(+)
> > 
> > diff --git a/drivers/net/phy/dp83td510.c b/drivers/net/phy/dp83td510.c
> > index d7616b13c5946..3375fa82927d0 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>
> > @@ -29,6 +30,42 @@
> >  #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_CTRL				0x1f
> > +#define DP83TD510E_CTRL_HW_RESET		BIT(15)
> > +#define DP83TD510E_CTRL_SW_RESET		BIT(14)
> > +
> > +#define DP83TD510E_TDR_CFG1			0x300
> 
> > +/* TX_TYPE: Transmit voltage level for TDR. 0 = 1V, 1 = 2.4V */
> > +#define DP83TD510E_TDR_TX_TYPE			BIT(12)
> 
> This does not appear to be used, so it is not too important. But i
> generally encode this as
> 
> #define DP83TD510E_TDR_TX_TYPE_1V		(0 << 12)
> #define DP83TD510E_TDR_TX_TYPE_2V4		(1 << 12)
> 
> You can then OR in DP83TD510E_TDR_TX_TYPE_1V which does nothing, but
> does document you are using 1V for the test.

Ack.

> > +#define DP83TD510E_TDR_CFG2			0x301
> > +#define DP83TD510E_TDR_END_TAP_INDEX_1		GENMASK(14, 8)
> > +#define DP83TD510E_TDR_END_TAP_INDEX_1_DEF	36
> > +#define DP83TD510E_TDR_START_TAP_INDEX_1	GENMASK(6, 0)
> > +#define DP83TD510E_TDR_START_TAP_INDEX_1_DEF	3
> 
> Does this correspond the minimum and maximum distance it will test?
> Is this 3m to 36m?

No. At least, i can't confirm it with tests.

If I see it correctly, this PHY is using SSTDR instead of usual TDR.
Instead of pulses it will send modulated transmission with default
length of 16ms

I tried my best google foo, but was not able to find anything
understandable about "Start/End tap index for echo coeff sweep for segment 1"
im context of SSTDR. If anyone know more about this, please tell me :)

Regards,
Oleksij
Andrew Lunn July 12, 2024, 3:20 p.m. UTC | #4 
> > > +#define DP83TD510E_TDR_CFG2			0x301
> > > +#define DP83TD510E_TDR_END_TAP_INDEX_1		GENMASK(14, 8)
> > > +#define DP83TD510E_TDR_END_TAP_INDEX_1_DEF	36
> > > +#define DP83TD510E_TDR_START_TAP_INDEX_1	GENMASK(6, 0)
> > > +#define DP83TD510E_TDR_START_TAP_INDEX_1_DEF	3
> > 
> > Does this correspond the minimum and maximum distance it will test?
> > Is this 3m to 36m?
> 
> No. At least, i can't confirm it with tests.
> 
> If I see it correctly, this PHY is using SSTDR instead of usual TDR.
> Instead of pulses it will send modulated transmission with default
> length of 16ms
> 
> I tried my best google foo, but was not able to find anything
> understandable about "Start/End tap index for echo coeff sweep for segment 1"
> im context of SSTDR. If anyone know more about this, please tell me :)

I was just curious. Does not really matter with respect to getting the
patch applied.

      Andrew
Oleksij Rempel July 12, 2024, 3:25 p.m. UTC | #5 
On Fri, Jul 12, 2024 at 05:20:06PM +0200, Andrew Lunn wrote:
> > > > +#define DP83TD510E_TDR_CFG2			0x301
> > > > +#define DP83TD510E_TDR_END_TAP_INDEX_1		GENMASK(14, 8)
> > > > +#define DP83TD510E_TDR_END_TAP_INDEX_1_DEF	36
> > > > +#define DP83TD510E_TDR_START_TAP_INDEX_1	GENMASK(6, 0)
> > > > +#define DP83TD510E_TDR_START_TAP_INDEX_1_DEF	3
> > > 
> > > Does this correspond the minimum and maximum distance it will test?
> > > Is this 3m to 36m?
> > 
> > No. At least, i can't confirm it with tests.
> > 
> > If I see it correctly, this PHY is using SSTDR instead of usual TDR.
> > Instead of pulses it will send modulated transmission with default
> > length of 16ms
> > 
> > I tried my best google foo, but was not able to find anything
> > understandable about "Start/End tap index for echo coeff sweep for segment 1"
> > im context of SSTDR. If anyone know more about this, please tell me :)
> 
> I was just curious. Does not really matter with respect to getting the
> patch applied.
 
No problem, I was so nerd sniped that I had to dive deeper into the
matter :)
diff mbox series

Patch

diff --git a/drivers/net/phy/dp83td510.c b/drivers/net/phy/dp83td510.c
index d7616b13c5946..3375fa82927d0 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>
@@ -29,6 +30,42 @@ 
 #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_CTRL				0x1f
+#define DP83TD510E_CTRL_HW_RESET		BIT(15)
+#define DP83TD510E_CTRL_SW_RESET		BIT(14)
+
+#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)
+#define DP83TD510E_TDR_END_TAP_INDEX_1_DEF	36
+#define DP83TD510E_TDR_START_TAP_INDEX_1	GENMASK(6, 0)
+#define DP83TD510E_TDR_START_TAP_INDEX_1_DEF	3
+
+#define DP83TD510E_TDR_FAULT_CFG1		0x303
+#define DP83TD510E_TDR_FLT_LOC_OFFSET_1		GENMASK(14, 8)
+#define DP83TD510E_TDR_FLT_LOC_OFFSET_1_DEF	4
+#define DP83TD510E_TDR_FLT_INIT_1		GENMASK(7, 0)
+#define DP83TD510E_TDR_FLT_INIT_1_DEF		62
+
+#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 revC"
+ */
+#define DP83TD510E_UNKN_030E			0x30e
+#define DP83TD510E_030E_VAL			0x2520
+
 #define DP83TD510E_AN_STAT_1			0x60c
 #define DP83TD510E_MASTER_SLAVE_RESOL_FAIL	BIT(15)
 
@@ -198,6 +235,124 @@  static int dp83td510_get_sqi_max(struct phy_device *phydev)
 	return DP83TD510_SQI_MAX;
 }
 
+/**
+ * dp83td510_cable_test_start - Start the cable test for the DP83TD510 PHY.
+ * @phydev: Pointer to the phy_device structure.
+ *
+ * This sequence is implemented according to the "Application Note DP83TD510E
+ * Cable Diagnostics Toolkit revC".
+ *
+ * Return: 0 on success, a negative error code on failure.
+ */
+static int dp83td510_cable_test_start(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_CTRL,
+			       DP83TD510E_CTRL_HW_RESET);
+	if (ret)
+		return ret;
+
+	ret = genphy_c45_an_disable_aneg(phydev);
+	if (ret)
+		return ret;
+
+	/* Force master mode */
+	ret = phy_set_bits_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_PMD_BT1_CTRL,
+			       MDIO_PMA_PMD_BT1_CTRL_CFG_MST);
+	if (ret)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_TDR_CFG2,
+			    FIELD_PREP(DP83TD510E_TDR_END_TAP_INDEX_1,
+				       DP83TD510E_TDR_END_TAP_INDEX_1_DEF) |
+			    FIELD_PREP(DP83TD510E_TDR_START_TAP_INDEX_1,
+				       DP83TD510E_TDR_START_TAP_INDEX_1_DEF));
+	if (ret)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_TDR_FAULT_CFG1,
+			    FIELD_PREP(DP83TD510E_TDR_FLT_LOC_OFFSET_1,
+				       DP83TD510E_TDR_FLT_LOC_OFFSET_1_DEF) |
+			    FIELD_PREP(DP83TD510E_TDR_FLT_INIT_1,
+				       DP83TD510E_TDR_FLT_INIT_1_DEF));
+	if (ret)
+		return ret;
+
+	/* Undocumented register, from the "Application Note DP83TD510E Cable
+	 * Diagnostics Toolkit revC".
+	 */
+	ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_UNKN_030E,
+			    DP83TD510E_030E_VAL);
+	if (ret)
+		return ret;
+
+	ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_CTRL,
+			       DP83TD510E_CTRL_SW_RESET);
+	if (ret)
+		return ret;
+
+	return phy_set_bits_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_TDR_CFG,
+				DP83TD510E_TDR_START);
+}
+
+/**
+ * dp83td510_cable_test_get_status - Get the status of the cable test for the
+ *                                   DP83TD510 PHY.
+ * @phydev: Pointer to the phy_device structure.
+ * @finished: Pointer to a boolean that indicates whether the test is finished.
+ *
+ * The function sets the @finished flag to true if the test is complete, and
+ * returns 0 on success or a negative error code on failure.
+ */
+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);
+
+	return phy_init_hw(phydev);
+}
+
 static int dp83td510_get_features(struct phy_device *phydev)
 {
 	/* This PHY can't respond on MDIO bus if no RMII clock is enabled.
@@ -221,6 +376,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,
@@ -228,6 +384,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,