[PATCH net-next] net: phy: aquantia: Add mdix config and reporting

[PATCH net-next] net: phy: aquantia: Add mdix config and reporting

[Paul Davey]

Add support for configuring MDI-X state of PHY.
Add reporting of resolved MDI-X state in status information.

Tested on AQR113C.

Signed-off-by: Paul Davey <paul.davey@alliedtelesis.co.nz>
---
 drivers/net/phy/aquantia/aquantia_main.c | 45 ++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
index 38d0dd5c80a4..8fe63a13b9f0 100644
--- a/drivers/net/phy/aquantia/aquantia_main.c
+++ b/drivers/net/phy/aquantia/aquantia_main.c
@@ -54,6 +54,12 @@
 #define MDIO_AN_VEND_PROV_DOWNSHIFT_MASK	GENMASK(3, 0)
 #define MDIO_AN_VEND_PROV_DOWNSHIFT_DFLT	4
 
+#define MDIO_AN_RESVD_VEND_PROV			0xc410
+#define MDIO_AN_RESVD_VEND_PROV_MDIX_AUTO	0
+#define MDIO_AN_RESVD_VEND_PROV_MDIX_MDI	1
+#define MDIO_AN_RESVD_VEND_PROV_MDIX_MDIX	2
+#define MDIO_AN_RESVD_VEND_PROV_MDIX_MASK	GENMASK(1, 0)
+
 #define MDIO_AN_TX_VEND_STATUS1			0xc800
 #define MDIO_AN_TX_VEND_STATUS1_RATE_MASK	GENMASK(3, 1)
 #define MDIO_AN_TX_VEND_STATUS1_10BASET		0
@@ -64,6 +70,9 @@
 #define MDIO_AN_TX_VEND_STATUS1_5000BASET	5
 #define MDIO_AN_TX_VEND_STATUS1_FULL_DUPLEX	BIT(0)
 
+#define MDIO_AN_RESVD_VEND_STATUS1		0xc810
+#define MDIO_AN_RESVD_VEND_STATUS1_MDIX		BIT(8)
+
 #define MDIO_AN_TX_VEND_INT_STATUS1		0xcc00
 #define MDIO_AN_TX_VEND_INT_STATUS1_DOWNSHIFT	BIT(1)
 
@@ -155,12 +164,40 @@ static void aqr107_get_stats(struct phy_device *phydev,
 	}
 }
 
+static int aqr_set_polarity(struct phy_device *phydev, int polarity)
+{
+	u16 val = 0;
+
+	switch (polarity) {
+	case ETH_TP_MDI:
+		val = MDIO_AN_RESVD_VEND_PROV_MDIX_MDI;
+		break;
+	case ETH_TP_MDI_X:
+		val = MDIO_AN_RESVD_VEND_PROV_MDIX_MDIX;
+		break;
+	case ETH_TP_MDI_AUTO:
+	case ETH_TP_MDI_INVALID:
+	default:
+		val = MDIO_AN_RESVD_VEND_PROV_MDIX_AUTO;
+		break;
+	}
+
+	return phy_modify_mmd_changed(phydev, MDIO_MMD_AN, MDIO_AN_RESVD_VEND_PROV,
+				      MDIO_AN_RESVD_VEND_PROV_MDIX_MASK, val);
+}
+
 static int aqr_config_aneg(struct phy_device *phydev)
 {
 	bool changed = false;
 	u16 reg;
 	int ret;
 
+	ret = aqr_set_polarity(phydev, phydev->mdix_ctrl);
+	if (ret < 0)
+		return ret;
+	if (ret > 0)
+		changed = true;
+
 	if (phydev->autoneg == AUTONEG_DISABLE)
 		return genphy_c45_pma_setup_forced(phydev);
 
@@ -278,6 +315,14 @@ static int aqr_read_status(struct phy_device *phydev)
 				 val & MDIO_AN_RX_LP_STAT1_1000BASET_HALF);
 	}
 
+	val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_RESVD_VEND_STATUS1);
+	if (val < 0)
+		return val;
+	if (val & MDIO_AN_RESVD_VEND_STATUS1_MDIX)
+		phydev->mdix = ETH_TP_MDI_X;
+	else
+		phydev->mdix = ETH_TP_MDI;
+
 	return genphy_c45_read_status(phydev);
 }
 
-- 
2.47.0

Re: [PATCH net-next] net: phy: aquantia: Add mdix config and reporting

[Daniel Golle]

On Thu, Oct 17, 2024 at 02:54:07PM +1300, Paul Davey wrote:
> Add support for configuring MDI-X state of PHY.
> Add reporting of resolved MDI-X state in status information.

> [...]
> +static int aqr_set_polarity(struct phy_device *phydev, int polarity)

"polarity" is not the right term here. This is not about the polarity
of copper pairs, but rather about pairs being swapped.
Please name the function accordingly, eg. aqr_set_mdix().

> +{
> +	u16 val = 0;
> +
> +	switch (polarity) {
> +	case ETH_TP_MDI:
> +		val = MDIO_AN_RESVD_VEND_PROV_MDIX_MDI;
> +		break;
> +	case ETH_TP_MDI_X:
> +		val = MDIO_AN_RESVD_VEND_PROV_MDIX_MDIX;
> +		break;
> +	case ETH_TP_MDI_AUTO:
> +	case ETH_TP_MDI_INVALID:
> +	default:
> +		val = MDIO_AN_RESVD_VEND_PROV_MDIX_AUTO;
> +		break;
> +	}
> +
> +	return phy_modify_mmd_changed(phydev, MDIO_MMD_AN, MDIO_AN_RESVD_VEND_PROV,
> +				      MDIO_AN_RESVD_VEND_PROV_MDIX_MASK, val);
> +}
> +
>  static int aqr_config_aneg(struct phy_device *phydev)
>  {
>  	bool changed = false;
>  	u16 reg;
>  	int ret;
>  
> +	ret = aqr_set_polarity(phydev, phydev->mdix_ctrl);
> +	if (ret < 0)
> +		return ret;
> +	if (ret > 0)
> +		changed = true;
> +
>  	if (phydev->autoneg == AUTONEG_DISABLE)
>  		return genphy_c45_pma_setup_forced(phydev);
>  
> @@ -278,6 +315,14 @@ static int aqr_read_status(struct phy_device *phydev)
>  				 val & MDIO_AN_RX_LP_STAT1_1000BASET_HALF);
>  	}
>  
> +	val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_RESVD_VEND_STATUS1);

According to the datasheet the MDI/MDI-X indication should only be
interpreted when autonegotiation has completed.
Hence this call should be protected by genphy_c45_aneg_done(phydev) and
phydev->mdix set to ETH_TP_MDI_INVALID in case auto-negotiation hasn't
completed.

> +	if (val < 0)
> +		return val;
> +	if (val & MDIO_AN_RESVD_VEND_STATUS1_MDIX)
> +		phydev->mdix = ETH_TP_MDI_X;
> +	else
> +		phydev->mdix = ETH_TP_MDI;
> +
>  	return genphy_c45_read_status(phydev);
>  }
>  
> -- 
> 2.47.0
> 
> 

Re: [PATCH net-next] net: phy: aquantia: Add mdix config and reporting

[Paul Davey]

On Thu, 2024-10-17 at 12:54 +0100, Daniel Golle wrote:
> On Thu, Oct 17, 2024 at 02:54:07PM +1300, Paul Davey wrote:
> > Add support for configuring MDI-X state of PHY.
> > Add reporting of resolved MDI-X state in status information.
> 
> > [...]
> > +static int aqr_set_polarity(struct phy_device *phydev, int
> > polarity)
> 
> "polarity" is not the right term here. This is not about the polarity
> of copper pairs, but rather about pairs being swapped.
> Please name the function accordingly, eg. aqr_set_mdix().

I will fix the name in the next version.

> [...]
> According to the datasheet the MDI/MDI-X indication should only be
> interpreted when autonegotiation has completed.
> Hence this call should be protected by genphy_c45_aneg_done(phydev)
> and
> phydev->mdix set to ETH_TP_MDI_INVALID in case auto-negotiation
> hasn't
> completed.

I will add a guard by this.  I did some testing because I was concerned
with what the behaviour is when disabling auto-negotiation, and have
concluded that auto MDI/MDI-X detection does not occur if auto-negotion
is disabled.

Due to this I wonder whether the mdix configuration should reject
ETH_TP_MDI_AUTO if auto-negotiation is disabled?

> [...]
> > 

Re: [PATCH net-next] net: phy: aquantia: Add mdix config and reporting

[Andrew Lunn]

> Due to this I wonder whether the mdix configuration should reject
> ETH_TP_MDI_AUTO if auto-negotiation is disabled?

How does MDIX actually work? Is there anything in 802.3?

For 10BaseT, one pair Rx, one pair Tx, i guess you can find out by
just looking at the signal. But for 4 pairs?

    Andrew

---
pw-bot: cr

Re: [PATCH net-next] net: phy: aquantia: Add mdix config and reporting

[Paul Davey]

On Fri, 2024-10-18 at 02:19 +0200, Andrew Lunn wrote:
> > Due to this I wonder whether the mdix configuration should reject
> > ETH_TP_MDI_AUTO if auto-negotiation is disabled?
> 
> How does MDIX actually work? Is there anything in 802.3?
> 
> For 10BaseT, one pair Rx, one pair Tx, i guess you can find out by
> just looking at the signal. But for 4 pairs?
> 
802.3 Clause 40.4.4 Automatic MDI/MDI-X Configuration specifies some
aspects of how Auto MDI/MDI-X crossover detection works for 1000BASE-T

For 1000BASE-T (and above) with 4 pairs the MDI-X state crosses over
both pairs A/B and pairs C/D.  Though some PHYs have support for
detecting partial crossover.

The crossover is required for auto-negotiation to complete so the
Automatic MDI/MDI-X resolution has to occur prior to auto-negotiation.

I believe the detection works by selecting a proposed crossover config,
then waiting a period to detect either auto-negotiation fast link
pulses or an RX link detection. If it doesn't find one it uses a
pseudorandom process (an LFSR I believe) to decide whether it should
swap to the other crossover state or remain in the current one.  This
is to ensure two link partners performing this process do not get stuck
both trying the wrong crossover and then swapping at the same time to
the other crossover.  From the state machine diagrams it appears the
initial crossover config is MDI.

As auto-negotiation is required for 1000BASE-T (and higher speed
twisted pair modes) the question of whether Auto MDI/MDI-X detection
occurs when auto-negotiation is turned off is only really relevant for
10BASE-T and 100BASE-T being forced.

When I was wondering if mdix_ctrl being set to ETH_TP_MDI_AUTO should
be rejected if auto-negotiation is disabled I meant for this specific
PHY driver as it definitely does not appear to perform the Auto
MDI/MDI-X resolution so if the wiring/cabling between and/or config on
the link partner does not match the default (MDI I think for the AQR)
then the link will not establish.

Thanks,
Paul

Re: [PATCH net-next] net: phy: aquantia: Add mdix config and reporting

[Andrew Lunn]

> As auto-negotiation is required for 1000BASE-T (and higher speed
> twisted pair modes) the question of whether Auto MDI/MDI-X detection
> occurs when auto-negotiation is turned off is only really relevant for
> 10BASE-T and 100BASE-T being forced.

Yes.

> When I was wondering if mdix_ctrl being set to ETH_TP_MDI_AUTO should
> be rejected if auto-negotiation is disabled I meant for this specific
> PHY driver as it definitely does not appear to perform the Auto
> MDI/MDI-X resolution so if the wiring/cabling between and/or config on
> the link partner does not match the default (MDI I think for the AQR)
> then the link will not establish.

Well, as you say, 1000Base-T needs autoneg, so there is no need to
reject ETH_TP_MDI_AUTO for that link mode and above.

It seems like for lower speeds, ETH_TP_MDI_AUTO could work without
autoneg. So to me, this validation is not a core feature, but per PHY.
Please feel free to implement it for this PHY.

    Andrew

---
pw-bot: cr

Re: [PATCH net-next] net: phy: aquantia: Add mdix config and reporting

[Paul Davey]

On Fri, 2024-10-18 at 19:20 +0200, Andrew Lunn wrote:
> 
> > When I was wondering if mdix_ctrl being set to ETH_TP_MDI_AUTO
> > should
> > be rejected if auto-negotiation is disabled I meant for this
> > specific
> > PHY driver as it definitely does not appear to perform the Auto
> > MDI/MDI-X resolution so if the wiring/cabling between and/or config
> > on
> > the link partner does not match the default (MDI I think for the
> > AQR)
> > then the link will not establish.
> 
> Well, as you say, 1000Base-T needs autoneg, so there is no need to
> reject ETH_TP_MDI_AUTO for that link mode and above.
> 
> It seems like for lower speeds, ETH_TP_MDI_AUTO could work without
> autoneg. So to me, this validation is not a core feature, but per
> PHY.
> Please feel free to implement it for this PHY.
> 
Having briefly tried it the _phy_state_machine code does not like
phy_config_aneg returning errors, so I will leave the code as is.

I will submit a v2 patch soon with the other requested changes.

Thanks,
Paul