All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] net: phy: micrel: ksz9031/ksz9131: add cabletest support
@ 2022-04-07  2:08 Marek Vasut
  2022-04-07  4:57 ` Jakub Kicinski
  2022-04-07  5:06 ` Oleksij Rempel
  0 siblings, 2 replies; 5+ messages in thread
From: Marek Vasut @ 2022-04-07  2:08 UTC (permalink / raw)
  To: netdev
  Cc: Marek Vasut, Andrew Lunn, David S . Miller, Heiner Kallweit,
	Jakub Kicinski, Oleksij Rempel, Paolo Abeni

Add cable test support for Micrel KSZ9x31 PHYs.

Tested on i.MX8M Mini with KSZ9131RNX in 100/Full mode with pairs shuffled
before magnetics:
(note: Cable test started/completed messages are omitted)

  mx8mm-ksz9131-a-d-connected$ ethtool --cable-test eth0
  Pair A code OK
  Pair B code Short within Pair
  Pair B, fault length: 0.80m
  Pair C code Short within Pair
  Pair C, fault length: 0.80m
  Pair D code OK

  mx8mm-ksz9131-a-b-connected$ ethtool --cable-test eth0
  Pair A code OK
  Pair B code OK
  Pair C code Short within Pair
  Pair C, fault length: 0.00m
  Pair D code Short within Pair
  Pair D, fault length: 0.00m

Tested on R8A77951 Salvator-XS with KSZ9031RNX and all four pairs connected:
(note: Cable test started/completed messages are omitted)

  r8a7795-ksz9031-all-connected$ ethtool --cable-test eth0
  Pair A code OK
  Pair B code OK
  Pair C code OK
  Pair D code OK

The CTRL1000 CTL1000_ENABLE_MASTER and CTL1000_AS_MASTER bits are not
restored by calling phy_init_hw(), they must be manually cached in
ksz9x31_cable_test_start() and restored at the end of
ksz9x31_cable_test_get_status().

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: David S. Miller <davem@davemloft.net>
Cc: Heiner Kallweit <hkallweit1@gmail.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Oleksij Rempel <linux@rempel-privat.de>
Cc: Paolo Abeni <pabeni@redhat.com>
---
V2: - Cache CTRL1000 CTL1000_ENABLE_MASTER | CTL1000_AS_MASTER in
      ksz9x31_cable_test_start() and restore them in
      ksz9x31_cable_test_get_status()
---
 drivers/net/phy/micrel.c | 221 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 221 insertions(+)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index fc53b71dc872b..26731fc3ca39c 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -70,6 +70,27 @@
 #define KSZ8081_LMD_SHORT_INDICATOR		BIT(12)
 #define KSZ8081_LMD_DELTA_TIME_MASK		GENMASK(8, 0)
 
+#define KSZ9x31_LMD				0x12
+#define KSZ9x31_LMD_VCT_EN			BIT(15)
+#define KSZ9x31_LMD_VCT_DIS_TX			BIT(14)
+#define KSZ9x31_LMD_VCT_PAIR(n)			(((n) & 0x3) << 12)
+#define KSZ9x31_LMD_VCT_SEL_RESULT		0
+#define KSZ9x31_LMD_VCT_SEL_THRES_HI		BIT(10)
+#define KSZ9x31_LMD_VCT_SEL_THRES_LO		BIT(11)
+#define KSZ9x31_LMD_VCT_SEL_MASK		GENMASK(11, 10)
+#define KSZ9x31_LMD_VCT_ST_NORMAL		0
+#define KSZ9x31_LMD_VCT_ST_OPEN			1
+#define KSZ9x31_LMD_VCT_ST_SHORT		2
+#define KSZ9x31_LMD_VCT_ST_FAIL			3
+#define KSZ9x31_LMD_VCT_ST_MASK			GENMASK(9, 8)
+#define KSZ9x31_LMD_VCT_DATA_REFLECTED_INVALID	BIT(7)
+#define KSZ9x31_LMD_VCT_DATA_SIG_WAIT_TOO_LONG	BIT(6)
+#define KSZ9x31_LMD_VCT_DATA_MASK100		BIT(5)
+#define KSZ9x31_LMD_VCT_DATA_NLP_FLP		BIT(4)
+#define KSZ9x31_LMD_VCT_DATA_LO_PULSE_MASK	GENMASK(3, 2)
+#define KSZ9x31_LMD_VCT_DATA_HI_PULSE_MASK	GENMASK(1, 0)
+#define KSZ9x31_LMD_VCT_DATA_MASK		GENMASK(7, 0)
+
 /* Lan8814 general Interrupt control/status reg in GPHY specific block. */
 #define LAN8814_INTC				0x18
 #define LAN8814_INTS				0x1B
@@ -280,6 +301,7 @@ struct kszphy_priv {
 	struct kszphy_ptp_priv ptp_priv;
 	const struct kszphy_type *type;
 	int led_mode;
+	u16 vct_ctrl1000;
 	bool rmii_ref_clk_sel;
 	bool rmii_ref_clk_sel_val;
 	u64 stats[ARRAY_SIZE(kszphy_hw_stats)];
@@ -1326,6 +1348,199 @@ static int ksz9031_read_status(struct phy_device *phydev)
 	return 0;
 }
 
+static int ksz9x31_cable_test_start(struct phy_device *phydev)
+{
+	struct kszphy_priv *priv = phydev->priv;
+	int ret;
+
+	/* KSZ9131RNX, DS00002841B-page 38, 4.14 LinkMD (R) Cable Diagnostic
+	 * Prior to running the cable diagnostics, Auto-negotiation should
+	 * be disabled, full duplex set and the link speed set to 1000Mbps
+	 * via the Basic Control Register.
+	 */
+	ret = phy_modify(phydev, MII_BMCR,
+			 BMCR_SPEED1000 | BMCR_FULLDPLX |
+			 BMCR_ANENABLE | BMCR_SPEED100,
+			 BMCR_SPEED1000 | BMCR_FULLDPLX);
+	if (ret)
+		return ret;
+
+	/* KSZ9131RNX, DS00002841B-page 38, 4.14 LinkMD (R) Cable Diagnostic
+	 * The Master-Slave configuration should be set to Slave by writing
+	 * a value of 0x1000 to the Auto-Negotiation Master Slave Control
+	 * Register.
+	 */
+	ret = phy_read(phydev, MII_CTRL1000);
+	if (ret < 0)
+		return ret;
+
+	/* Cache these bits, they need to be restored once LinkMD finishes. */
+	priv->vct_ctrl1000 = ret & (CTL1000_ENABLE_MASTER | CTL1000_AS_MASTER);
+	ret &= ~(CTL1000_ENABLE_MASTER | CTL1000_AS_MASTER);
+	ret |= CTL1000_ENABLE_MASTER;
+
+	return phy_write(phydev, MII_CTRL1000, ret);
+}
+
+static int ksz9x31_cable_test_result_trans(u16 status)
+{
+	switch (FIELD_GET(KSZ9x31_LMD_VCT_ST_MASK, status)) {
+	case KSZ9x31_LMD_VCT_ST_NORMAL:
+		return ETHTOOL_A_CABLE_RESULT_CODE_OK;
+	case KSZ9x31_LMD_VCT_ST_OPEN:
+		return ETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT;
+	case KSZ9x31_LMD_VCT_ST_SHORT:
+		return ETHTOOL_A_CABLE_RESULT_CODE_OPEN;
+	case KSZ9x31_LMD_VCT_ST_FAIL:
+		fallthrough;
+	default:
+		return ETHTOOL_A_CABLE_RESULT_CODE_UNSPEC;
+	}
+}
+
+static bool ksz9x31_cable_test_failed(u16 status)
+{
+	int stat = FIELD_GET(KSZ9x31_LMD_VCT_ST_MASK, status);
+
+	return stat == KSZ9x31_LMD_VCT_ST_FAIL;
+}
+
+static bool ksz9x31_cable_test_fault_length_valid(u16 status)
+{
+	switch (FIELD_GET(KSZ9x31_LMD_VCT_ST_MASK, status)) {
+	case KSZ9x31_LMD_VCT_ST_OPEN:
+		fallthrough;
+	case KSZ9x31_LMD_VCT_ST_SHORT:
+		return true;
+	}
+	return false;
+}
+
+static int ksz9x31_cable_test_fault_length(struct phy_device *phydev, u16 stat)
+{
+	int dt = FIELD_GET(KSZ9x31_LMD_VCT_DATA_MASK, stat);
+
+	/* KSZ9131RNX, DS00002841B-page 38, 4.14 LinkMD (R) Cable Diagnostic
+	 *
+	 * distance to fault = (VCT_DATA - 22) * 4 / cable propagation velocity
+	 */
+	if ((phydev->phy_id & MICREL_PHY_ID_MASK) == PHY_ID_KSZ9131)
+		dt = clamp(dt - 22, 0, 255);
+
+	return (dt * 400) / 10;
+}
+
+static int ksz9x31_cable_test_wait_for_completion(struct phy_device *phydev)
+{
+	int val, ret;
+
+	ret = phy_read_poll_timeout(phydev, KSZ9x31_LMD, val,
+				    !(val & KSZ9x31_LMD_VCT_EN),
+				    30000, 100000, true);
+
+	return ret < 0 ? ret : 0;
+}
+
+static int ksz9x31_cable_test_get_pair(int pair)
+{
+	static const int ethtool_pair[] = {
+		ETHTOOL_A_CABLE_PAIR_A,
+		ETHTOOL_A_CABLE_PAIR_B,
+		ETHTOOL_A_CABLE_PAIR_C,
+		ETHTOOL_A_CABLE_PAIR_D,
+	};
+
+	return ethtool_pair[pair];
+}
+
+static int ksz9x31_cable_test_one_pair(struct phy_device *phydev, int pair)
+{
+	int ret, val;
+
+	/* KSZ9131RNX, DS00002841B-page 38, 4.14 LinkMD (R) Cable Diagnostic
+	 * To test each individual cable pair, set the cable pair in the Cable
+	 * Diagnostics Test Pair (VCT_PAIR[1:0]) field of the LinkMD Cable
+	 * Diagnostic Register, along with setting the Cable Diagnostics Test
+	 * Enable (VCT_EN) bit. The Cable Diagnostics Test Enable (VCT_EN) bit
+	 * will self clear when the test is concluded.
+	 */
+	ret = phy_write(phydev, KSZ9x31_LMD,
+			KSZ9x31_LMD_VCT_EN | KSZ9x31_LMD_VCT_PAIR(pair));
+	if (ret)
+		return ret;
+
+	ret = ksz9x31_cable_test_wait_for_completion(phydev);
+	if (ret)
+		return ret;
+
+	val = phy_read(phydev, KSZ9x31_LMD);
+	if (val < 0)
+		return val;
+
+	if (ksz9x31_cable_test_failed(val))
+		return -EAGAIN;
+
+	ret = ethnl_cable_test_result(phydev,
+				      ksz9x31_cable_test_get_pair(pair),
+				      ksz9x31_cable_test_result_trans(val));
+	if (ret)
+		return ret;
+
+	if (!ksz9x31_cable_test_fault_length_valid(val))
+		return 0;
+
+	return ethnl_cable_test_fault_length(phydev,
+					     ksz9x31_cable_test_get_pair(pair),
+					     ksz9x31_cable_test_fault_length(phydev, val));
+}
+
+static int ksz9x31_cable_test_get_status(struct phy_device *phydev,
+					 bool *finished)
+{
+	struct kszphy_priv *priv = phydev->priv;
+	unsigned long pair_mask = 0xf;
+	int retries = 20;
+	int pair, ret, rv;
+
+	*finished = false;
+
+	/* Try harder if link partner is active */
+	while (pair_mask && retries--) {
+		for_each_set_bit(pair, &pair_mask, 4) {
+			ret = ksz9x31_cable_test_one_pair(phydev, pair);
+			if (ret == -EAGAIN)
+				continue;
+			if (ret < 0)
+				return ret;
+			clear_bit(pair, &pair_mask);
+		}
+		/* If link partner is in autonegotiation mode it will send 2ms
+		 * of FLPs with at least 6ms of silence.
+		 * Add 2ms sleep to have better chances to hit this silence.
+		 */
+		if (pair_mask)
+			usleep_range(2000, 3000);
+	}
+
+	/* Report remaining unfinished pair result as unknown. */
+	for_each_set_bit(pair, &pair_mask, 4) {
+		ret = ethnl_cable_test_result(phydev,
+					      ksz9x31_cable_test_get_pair(pair),
+					      ETHTOOL_A_CABLE_RESULT_CODE_UNSPEC);
+	}
+
+	*finished = true;
+
+	/* Restore cached bits from before LinkMD got started. */
+	rv = phy_modify(phydev, MII_CTRL1000,
+			CTL1000_ENABLE_MASTER | CTL1000_AS_MASTER,
+			priv->vct_ctrl1000);
+	if (rv)
+		return rv;
+
+	return ret;
+}
+
 static int ksz8873mll_config_aneg(struct phy_device *phydev)
 {
 	return 0;
@@ -2806,6 +3021,7 @@ static struct phy_driver ksphy_driver[] = {
 	.phy_id		= PHY_ID_KSZ9031,
 	.phy_id_mask	= MICREL_PHY_ID_MASK,
 	.name		= "Micrel KSZ9031 Gigabit PHY",
+	.flags		= PHY_POLL_CABLE_TEST,
 	.driver_data	= &ksz9021_type,
 	.probe		= kszphy_probe,
 	.get_features	= ksz9031_get_features,
@@ -2819,6 +3035,8 @@ static struct phy_driver ksphy_driver[] = {
 	.get_stats	= kszphy_get_stats,
 	.suspend	= kszphy_suspend,
 	.resume		= kszphy_resume,
+	.cable_test_start	= ksz9x31_cable_test_start,
+	.cable_test_get_status	= ksz9x31_cable_test_get_status,
 }, {
 	.phy_id		= PHY_ID_LAN8814,
 	.phy_id_mask	= MICREL_PHY_ID_MASK,
@@ -2853,6 +3071,7 @@ static struct phy_driver ksphy_driver[] = {
 	.phy_id_mask	= MICREL_PHY_ID_MASK,
 	.name		= "Microchip KSZ9131 Gigabit PHY",
 	/* PHY_GBIT_FEATURES */
+	.flags		= PHY_POLL_CABLE_TEST,
 	.driver_data	= &ksz9021_type,
 	.probe		= kszphy_probe,
 	.config_init	= ksz9131_config_init,
@@ -2863,6 +3082,8 @@ static struct phy_driver ksphy_driver[] = {
 	.get_stats	= kszphy_get_stats,
 	.suspend	= kszphy_suspend,
 	.resume		= kszphy_resume,
+	.cable_test_start	= ksz9x31_cable_test_start,
+	.cable_test_get_status	= ksz9x31_cable_test_get_status,
 }, {
 	.phy_id		= PHY_ID_KSZ8873MLL,
 	.phy_id_mask	= MICREL_PHY_ID_MASK,
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] net: phy: micrel: ksz9031/ksz9131: add cabletest support
  2022-04-07  2:08 [PATCH v2] net: phy: micrel: ksz9031/ksz9131: add cabletest support Marek Vasut
@ 2022-04-07  4:57 ` Jakub Kicinski
  2022-04-07 10:56   ` Marek Vasut
  2022-04-07  5:06 ` Oleksij Rempel
  1 sibling, 1 reply; 5+ messages in thread
From: Jakub Kicinski @ 2022-04-07  4:57 UTC (permalink / raw)
  To: Marek Vasut
  Cc: netdev, Andrew Lunn, David S . Miller, Heiner Kallweit,
	Oleksij Rempel, Paolo Abeni

On Thu,  7 Apr 2022 04:08:12 +0200 Marek Vasut wrote:
> Add cable test support for Micrel KSZ9x31 PHYs.
> 
> Tested on i.MX8M Mini with KSZ9131RNX in 100/Full mode with pairs shuffled
> before magnetics:
> (note: Cable test started/completed messages are omitted)
> 
>   mx8mm-ksz9131-a-d-connected$ ethtool --cable-test eth0
>   Pair A code OK
>   Pair B code Short within Pair
>   Pair B, fault length: 0.80m
>   Pair C code Short within Pair
>   Pair C, fault length: 0.80m
>   Pair D code OK
> 
>   mx8mm-ksz9131-a-b-connected$ ethtool --cable-test eth0
>   Pair A code OK
>   Pair B code OK
>   Pair C code Short within Pair
>   Pair C, fault length: 0.00m
>   Pair D code Short within Pair
>   Pair D, fault length: 0.00m
> 
> Tested on R8A77951 Salvator-XS with KSZ9031RNX and all four pairs connected:
> (note: Cable test started/completed messages are omitted)
> 
>   r8a7795-ksz9031-all-connected$ ethtool --cable-test eth0
>   Pair A code OK
>   Pair B code OK
>   Pair C code OK
>   Pair D code OK
> 
> The CTRL1000 CTL1000_ENABLE_MASTER and CTL1000_AS_MASTER bits are not
> restored by calling phy_init_hw(), they must be manually cached in
> ksz9x31_cable_test_start() and restored at the end of
> ksz9x31_cable_test_get_status().
> 
> Signed-off-by: Marek Vasut <marex@denx.de>

It does not apply completely cleanly to net-next, could you rebase?

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] net: phy: micrel: ksz9031/ksz9131: add cabletest support
  2022-04-07  2:08 [PATCH v2] net: phy: micrel: ksz9031/ksz9131: add cabletest support Marek Vasut
  2022-04-07  4:57 ` Jakub Kicinski
@ 2022-04-07  5:06 ` Oleksij Rempel
  2022-04-07 10:47   ` Marek Vasut
  1 sibling, 1 reply; 5+ messages in thread
From: Oleksij Rempel @ 2022-04-07  5:06 UTC (permalink / raw)
  To: Marek Vasut
  Cc: netdev, Andrew Lunn, David S . Miller, Heiner Kallweit,
	Jakub Kicinski, Oleksij Rempel, Paolo Abeni, kernel

On Thu, Apr 07, 2022 at 04:08:12AM +0200, Marek Vasut wrote:
...

> +static int ksz9x31_cable_test_result_trans(u16 status)
> +{
> +	switch (FIELD_GET(KSZ9x31_LMD_VCT_ST_MASK, status)) {
> +	case KSZ9x31_LMD_VCT_ST_NORMAL:
> +		return ETHTOOL_A_CABLE_RESULT_CODE_OK;
> +	case KSZ9x31_LMD_VCT_ST_OPEN:
> +		return ETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT;
> +	case KSZ9x31_LMD_VCT_ST_SHORT:
> +		return ETHTOOL_A_CABLE_RESULT_CODE_OPEN;

This conversation looks twisted:
VCT_ST_OPEN -> CODE_SAME_SHORT
VCT_ST_SHORT -> CODE_OPEN

> +	case KSZ9x31_LMD_VCT_ST_FAIL:
> +		fallthrough;
> +	default:
> +		return ETHTOOL_A_CABLE_RESULT_CODE_UNSPEC;
> +	}
> +}
> +
...
> +static int ksz9x31_cable_test_get_status(struct phy_device *phydev,
> +					 bool *finished)
> +{
> +	struct kszphy_priv *priv = phydev->priv;
> +	unsigned long pair_mask = 0xf;
> +	int retries = 20;
> +	int pair, ret, rv;
> +
> +	*finished = false;
> +
> +	/* Try harder if link partner is active */
> +	while (pair_mask && retries--) {
> +		for_each_set_bit(pair, &pair_mask, 4) {
> +			ret = ksz9x31_cable_test_one_pair(phydev, pair);
> +			if (ret == -EAGAIN)
> +				continue;
> +			if (ret < 0)
> +				return ret;
> +			clear_bit(pair, &pair_mask);
> +		}
> +		/* If link partner is in autonegotiation mode it will send 2ms
> +		 * of FLPs with at least 6ms of silence.
> +		 * Add 2ms sleep to have better chances to hit this silence.
> +		 */
> +		if (pair_mask)
> +			usleep_range(2000, 3000);

Not a blocker, just some ideas:
In my experience, active link partner may affect test result in way,
that the PHY wont report it as failed test. Especially, if the cable is
on the edge of specification (for example too long cable).
At same time 6ms of silence is PHY specific implementation. For example
KSZ PHYs tend to send burst of FLPs and the switch to other MDI-X pair
(if auto MDI-X is enabled). Some RTL PHYs tend to send random firework of pulses
on different pairs.

May be we should not fight against autoneg and misuse it a bit? For
example:
0. force MDI configuration
1. limit autoneg to 10mbit
2. allow the link
3. switch to test and run on unused pair
4. force MDI-X configuration and goto 1.

Other options can be:
- implement autoneg next page tx/rx and pass linux-vendor specific
  information over it (some thing like, please stop autoneg for X amount
  of sec)
- advertise remote fault

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] net: phy: micrel: ksz9031/ksz9131: add cabletest support
  2022-04-07  5:06 ` Oleksij Rempel
@ 2022-04-07 10:47   ` Marek Vasut
  0 siblings, 0 replies; 5+ messages in thread
From: Marek Vasut @ 2022-04-07 10:47 UTC (permalink / raw)
  To: Oleksij Rempel
  Cc: netdev, Andrew Lunn, David S . Miller, Heiner Kallweit,
	Jakub Kicinski, Oleksij Rempel, Paolo Abeni, kernel

On 4/7/22 07:06, Oleksij Rempel wrote:
> On Thu, Apr 07, 2022 at 04:08:12AM +0200, Marek Vasut wrote:
> ...
> 
>> +static int ksz9x31_cable_test_result_trans(u16 status)
>> +{
>> +	switch (FIELD_GET(KSZ9x31_LMD_VCT_ST_MASK, status)) {
>> +	case KSZ9x31_LMD_VCT_ST_NORMAL:
>> +		return ETHTOOL_A_CABLE_RESULT_CODE_OK;
>> +	case KSZ9x31_LMD_VCT_ST_OPEN:
>> +		return ETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT;
>> +	case KSZ9x31_LMD_VCT_ST_SHORT:
>> +		return ETHTOOL_A_CABLE_RESULT_CODE_OPEN;
> 
> This conversation looks twisted:
> VCT_ST_OPEN -> CODE_SAME_SHORT
> VCT_ST_SHORT -> CODE_OPEN

Doh ... fixed

>> +	case KSZ9x31_LMD_VCT_ST_FAIL:
>> +		fallthrough;
>> +	default:
>> +		return ETHTOOL_A_CABLE_RESULT_CODE_UNSPEC;
>> +	}
>> +}
>> +
> ...
>> +static int ksz9x31_cable_test_get_status(struct phy_device *phydev,
>> +					 bool *finished)
>> +{
>> +	struct kszphy_priv *priv = phydev->priv;
>> +	unsigned long pair_mask = 0xf;
>> +	int retries = 20;
>> +	int pair, ret, rv;
>> +
>> +	*finished = false;
>> +
>> +	/* Try harder if link partner is active */
>> +	while (pair_mask && retries--) {
>> +		for_each_set_bit(pair, &pair_mask, 4) {
>> +			ret = ksz9x31_cable_test_one_pair(phydev, pair);
>> +			if (ret == -EAGAIN)
>> +				continue;
>> +			if (ret < 0)
>> +				return ret;
>> +			clear_bit(pair, &pair_mask);
>> +		}
>> +		/* If link partner is in autonegotiation mode it will send 2ms
>> +		 * of FLPs with at least 6ms of silence.
>> +		 * Add 2ms sleep to have better chances to hit this silence.
>> +		 */
>> +		if (pair_mask)
>> +			usleep_range(2000, 3000);
> 
> Not a blocker, just some ideas:
> In my experience, active link partner may affect test result in way,
> that the PHY wont report it as failed test. Especially, if the cable is
> on the edge of specification (for example too long cable).
> At same time 6ms of silence is PHY specific implementation. For example
> KSZ PHYs tend to send burst of FLPs and the switch to other MDI-X pair
> (if auto MDI-X is enabled). Some RTL PHYs tend to send random firework of pulses
> on different pairs.
> 
> May be we should not fight against autoneg and misuse it a bit? For
> example:
> 0. force MDI configuration
> 1. limit autoneg to 10mbit

We can't do that in this case, can we ?

KSZ9131RNX, DS00002841B-page 38, 4.14 LinkMD (R) Cable Diagnostic
"
Prior to running the cable diagnostics, Auto-negotiation should
be disabled, full duplex set and the link speed set to 1000Mbps
via the Basic Control Register.
"

> 2. allow the link
> 3. switch to test and run on unused pair
> 4. force MDI-X configuration and goto 1.
> 
> Other options can be:
> - implement autoneg next page tx/rx and pass linux-vendor specific
>    information over it (some thing like, please stop autoneg for X amount
>    of sec)
> - advertise remote fault

Wouldn't that only work if the link partner is also up to date linux 
machine ?

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] net: phy: micrel: ksz9031/ksz9131: add cabletest support
  2022-04-07  4:57 ` Jakub Kicinski
@ 2022-04-07 10:56   ` Marek Vasut
  0 siblings, 0 replies; 5+ messages in thread
From: Marek Vasut @ 2022-04-07 10:56 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: netdev, Andrew Lunn, David S . Miller, Heiner Kallweit,
	Oleksij Rempel, Paolo Abeni

On 4/7/22 06:57, Jakub Kicinski wrote:
> On Thu,  7 Apr 2022 04:08:12 +0200 Marek Vasut wrote:
>> Add cable test support for Micrel KSZ9x31 PHYs.
>>
>> Tested on i.MX8M Mini with KSZ9131RNX in 100/Full mode with pairs shuffled
>> before magnetics:
>> (note: Cable test started/completed messages are omitted)
>>
>>    mx8mm-ksz9131-a-d-connected$ ethtool --cable-test eth0
>>    Pair A code OK
>>    Pair B code Short within Pair
>>    Pair B, fault length: 0.80m
>>    Pair C code Short within Pair
>>    Pair C, fault length: 0.80m
>>    Pair D code OK
>>
>>    mx8mm-ksz9131-a-b-connected$ ethtool --cable-test eth0
>>    Pair A code OK
>>    Pair B code OK
>>    Pair C code Short within Pair
>>    Pair C, fault length: 0.00m
>>    Pair D code Short within Pair
>>    Pair D, fault length: 0.00m
>>
>> Tested on R8A77951 Salvator-XS with KSZ9031RNX and all four pairs connected:
>> (note: Cable test started/completed messages are omitted)
>>
>>    r8a7795-ksz9031-all-connected$ ethtool --cable-test eth0
>>    Pair A code OK
>>    Pair B code OK
>>    Pair C code OK
>>    Pair D code OK
>>
>> The CTRL1000 CTL1000_ENABLE_MASTER and CTL1000_AS_MASTER bits are not
>> restored by calling phy_init_hw(), they must be manually cached in
>> ksz9x31_cable_test_start() and restored at the end of
>> ksz9x31_cable_test_get_status().
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
> 
> It does not apply completely cleanly to net-next, could you rebase?

Done, v3 is out.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2022-04-07 10:56 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-07  2:08 [PATCH v2] net: phy: micrel: ksz9031/ksz9131: add cabletest support Marek Vasut
2022-04-07  4:57 ` Jakub Kicinski
2022-04-07 10:56   ` Marek Vasut
2022-04-07  5:06 ` Oleksij Rempel
2022-04-07 10:47   ` Marek Vasut

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.