| Message ID | 20241017015407.256737-1-paul.davey@alliedtelesis.co.nz (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: phy: aquantia: Add mdix config and reporting | expand |
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 > >
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? > [...] > >
> 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
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
> 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
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); }
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(+)