The DP83869 PHY transceiver supports converting from RGMII to 1000base-x.
In this operation mode, autonegotiation can be performed, as described in
IEEE802.3.

The DP83869 has a set of fiber-specific registers located at offset 0xc00.
When the transceiver is configured in RGMII-to-1000base-x mode, these
registers are mapped onto offset 0, which should, in theory, make reading
the autonegotiation status transparent.

However, the fiber registers at offset 0xc04 and 0xc05 do not follow the
bit layout of their standard counterparts. Thus, genphy_read_status()
doesn't properly read the capabilities advertised by the link partner,
resulting in incorrect link parameters.

Similarly, genphy_config_aneg() doesn't properly write advertised
capabilities.

Fix the 1000base-x autonegotiation procedure by replacing
genphy_read_status() and genphy_config_aneg() with driver-specific
functions which take into account the nonstandard bit layout of the DP83869
registers in 1000base-x mode.

Fixes: a29de52ba2a1 ("net: dp83869: Add ability to advertise Fiber connection")
Cc: stable@vger.kernel.org
Signed-off-by: Romain Gantois <romain.gantois@bootlin.com>
---
Changes in v2:
- Fixed an uninitialized use.
- Link to v1: https://lore.kernel.org/r/20241029-dp83869-1000base-x-v1-1-fcafe360bd98@bootlin.com
---
 drivers/net/phy/dp83869.c | 130 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 127 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
index 5f056d7db83eed23f1cab42365fdc566a0d8e47f..ca1c247d478ced890a3c7ae97b855f20056eb916 100644
--- a/drivers/net/phy/dp83869.c
+++ b/drivers/net/phy/dp83869.c
@@ -41,6 +41,8 @@
 #define DP83869_IO_MUX_CFG	0x0170
 #define DP83869_OP_MODE		0x01df
 #define DP83869_FX_CTRL		0x0c00
+#define DP83869_FX_ANADV        0x0c04
+#define DP83869_FX_LPABL        0x0c05
 
 #define DP83869_SW_RESET	BIT(15)
 #define DP83869_SW_RESTART	BIT(14)
@@ -135,6 +137,17 @@
 #define DP83869_DOWNSHIFT_4_COUNT	4
 #define DP83869_DOWNSHIFT_8_COUNT	8
 
+/* FX_ANADV bits */
+#define DP83869_BP_FULL_DUPLEX       BIT(5)
+#define DP83869_BP_PAUSE             BIT(7)
+#define DP83869_BP_ASYMMETRIC_PAUSE  BIT(8)
+
+/* FX_LPABL bits */
+#define DP83869_LPA_1000FULL   BIT(5)
+#define DP83869_LPA_PAUSE_CAP  BIT(7)
+#define DP83869_LPA_PAUSE_ASYM BIT(8)
+#define DP83869_LPA_LPACK      BIT(14)
+
 enum {
 	DP83869_PORT_MIRRORING_KEEP,
 	DP83869_PORT_MIRRORING_EN,
@@ -153,19 +166,129 @@ struct dp83869_private {
 	int mode;
 };
 
+static int dp83869_config_aneg(struct phy_device *phydev)
+{
+	struct dp83869_private *dp83869 = phydev->priv;
+	unsigned long *advertising;
+	int err, changed = false;
+	u32 adv = 0;
+
+	if (dp83869->mode != DP83869_RGMII_1000_BASE)
+		return genphy_config_aneg(phydev);
+
+	/* Forcing speed or duplex isn't supported in 1000base-x mode */
+	if (phydev->autoneg != AUTONEG_ENABLE)
+		return 0;
+
+	/* In fiber modes, register locations 0xc0... get mapped to offset 0.
+	 * Unfortunately, the fiber-specific autonegotiation advertisement
+	 * register at address 0xc04 does not have the same bit layout as the
+	 * corresponding standard MII_ADVERTISE register. Thus, functions such
+	 * as genphy_config_advert() will write the advertisement register
+	 * incorrectly.
+	 */
+	advertising = phydev->advertising;
+
+	/* Only allow advertising what this PHY supports */
+	linkmode_and(advertising, advertising,
+		     phydev->supported);
+
+	if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT, advertising))
+		adv |= DP83869_BP_FULL_DUPLEX;
+	if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT, advertising))
+		adv |= DP83869_BP_PAUSE;
+	if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, advertising))
+		adv |= DP83869_BP_ASYMMETRIC_PAUSE;
+
+	err = phy_modify_changed(phydev, DP83869_FX_ANADV,
+				 DP83869_BP_FULL_DUPLEX | DP83869_BP_PAUSE |
+				 DP83869_BP_ASYMMETRIC_PAUSE,
+				 adv);
+
+	if (err < 0)
+		return err;
+	else if (err)
+		changed = true;
+
+	return genphy_check_and_restart_aneg(phydev, changed);
+}
+
+static int dp83869_read_status_fiber(struct phy_device *phydev)
+{
+	int err, lpa, old_link = phydev->link;
+	unsigned long *lp_advertising;
+
+	err = genphy_update_link(phydev);
+	if (err)
+		return err;
+
+	if (phydev->autoneg == AUTONEG_ENABLE && old_link && phydev->link)
+		return 0;
+
+	phydev->speed = SPEED_UNKNOWN;
+	phydev->duplex = DUPLEX_UNKNOWN;
+	phydev->pause = 0;
+	phydev->asym_pause = 0;
+
+	lp_advertising = phydev->lp_advertising;
+
+	if (phydev->autoneg != AUTONEG_ENABLE) {
+		linkmode_zero(lp_advertising);
+
+		phydev->duplex = DUPLEX_FULL;
+		phydev->speed = SPEED_1000;
+
+		return 0;
+	}
+
+	if (!phydev->autoneg_complete) {
+		linkmode_zero(lp_advertising);
+		return 0;
+	}
+
+	/* In fiber modes, register locations 0xc0... get mapped to offset 0.
+	 * Unfortunately, the fiber-specific link partner capabilities register
+	 * at address 0xc05 does not have the same bit layout as the
+	 * corresponding standard MII_LPA register. Thus, functions such as
+	 * genphy_read_lpa() will read autonegotiation results incorrectly.
+	 */
+
+	lpa = phy_read(phydev, DP83869_FX_LPABL);
+	if (lpa < 0)
+		return lpa;
+
+	linkmode_mod_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
+			 lp_advertising, lpa & DP83869_LPA_1000FULL);
+
+	linkmode_mod_bit(ETHTOOL_LINK_MODE_Pause_BIT, lp_advertising,
+			 lpa & DP83869_LPA_PAUSE_CAP);
+
+	linkmode_mod_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, lp_advertising,
+			 lpa & DP83869_LPA_PAUSE_ASYM);
+
+	linkmode_mod_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
+			 lp_advertising, lpa & DP83869_LPA_LPACK);
+
+	phy_resolve_aneg_linkmode(phydev);
+
+	return 0;
+}
+
 static int dp83869_read_status(struct phy_device *phydev)
 {
 	struct dp83869_private *dp83869 = phydev->priv;
 	int ret;
 
+	if (dp83869->mode == DP83869_RGMII_1000_BASE)
+		return dp83869_read_status_fiber(phydev);
+
 	ret = genphy_read_status(phydev);
 	if (ret)
 		return ret;
 
-	if (linkmode_test_bit(ETHTOOL_LINK_MODE_FIBRE_BIT, phydev->supported)) {
+	if (dp83869->mode == DP83869_RGMII_100_BASE) {
 		if (phydev->link) {
-			if (dp83869->mode == DP83869_RGMII_100_BASE)
-				phydev->speed = SPEED_100;
+			phydev->speed = SPEED_100;
 		} else {
 			phydev->speed = SPEED_UNKNOWN;
 			phydev->duplex = DUPLEX_UNKNOWN;
@@ -898,6 +1021,7 @@ static int dp83869_phy_reset(struct phy_device *phydev)
 	.soft_reset	= dp83869_phy_reset,			\
 	.config_intr	= dp83869_config_intr,			\
 	.handle_interrupt = dp83869_handle_interrupt,		\
+	.config_aneg    = dp83869_config_aneg,                  \
 	.read_status	= dp83869_read_status,			\
 	.get_tunable	= dp83869_get_tunable,			\
 	.set_tunable	= dp83869_set_tunable,			\

---
base-commit: 5ccdcdf186aec6b9111845fd37e1757e9b413e2f
change-id: 20241025-dp83869-1000base-x-0f0a61725784

Best regards,
-- 
Romain Gantois <romain.gantois@bootlin.com>

On Mon, Nov 04, 2024 at 09:52:32AM +0100, Romain Gantois wrote:
> The DP83869 PHY transceiver supports converting from RGMII to 1000base-x.
> In this operation mode, autonegotiation can be performed, as described in
> IEEE802.3.
> 
> The DP83869 has a set of fiber-specific registers located at offset 0xc00.
> When the transceiver is configured in RGMII-to-1000base-x mode, these
> registers are mapped onto offset 0, which should, in theory, make reading
> the autonegotiation status transparent.
> 
> However, the fiber registers at offset 0xc04 and 0xc05 do not follow the
> bit layout of their standard counterparts. Thus, genphy_read_status()
> doesn't properly read the capabilities advertised by the link partner,
> resulting in incorrect link parameters.

This description is wrong. The format of registers 4 and 5 depends on
the media.

In twisted-pair ethernet, then:

ADVERTISE_PAUSE_ASYM / LPA_PAUSE_ASYM
ADVERTISE_PAUSE_CAP / LPA_PAUSE_CAP
ADVERTISE_100FULL / LPA_100FULL
ADVERTISE_100HALF / LPA_100HALF
ADVERTISE_10FULL / LPA_10FULL
ADVERTISE_10HALF / LPA_10HALF
ADVERTISE_CSMA

apply. In 1000base-X:

ADVERTISE_1000XPSE_ASYM / LPA_1000XPAUSE_ASYM
ADVERTISE_1000XPAUSE / LPA_1000XPAUSE
ADVERTISE_1000XHALF / LPA_1000XHALF
ADVERTISE_1000XFULL / LPA_1000XFULL

apply - these being bits 8, 7, 6, 5:

> +#define DP83869_LPA_1000FULL   BIT(5)
> +#define DP83869_LPA_PAUSE_CAP  BIT(7)
> +#define DP83869_LPA_PAUSE_ASYM BIT(8)
> +#define DP83869_LPA_LPACK      BIT(14)

so these are just reimplementing definitions we already have. Please
use the existing definitions. Even better, use mii_lpa_mod_linkmode_x()
and linkmode_adv_to_mii_adv_x() which we already have in your code.

Same likely goes for DP83869_BP_*

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

On Mon, 04 Nov 2024 09:52:32 +0100 Romain Gantois wrote:
> The DP83869 PHY transceiver supports converting from RGMII to 1000base-x.
> In this operation mode, autonegotiation can be performed, as described in
> IEEE802.3.
> 
> The DP83869 has a set of fiber-specific registers located at offset 0xc00.
> When the transceiver is configured in RGMII-to-1000base-x mode, these
> registers are mapped onto offset 0, which should, in theory, make reading
> the autonegotiation status transparent.
> 
> However, the fiber registers at offset 0xc04 and 0xc05 do not follow the
> bit layout of their standard counterparts. Thus, genphy_read_status()
> doesn't properly read the capabilities advertised by the link partner,
> resulting in incorrect link parameters.
> 
> Similarly, genphy_config_aneg() doesn't properly write advertised
> capabilities.
> 
> Fix the 1000base-x autonegotiation procedure by replacing
> genphy_read_status() and genphy_config_aneg() with driver-specific
> functions which take into account the nonstandard bit layout of the DP83869
> registers in 1000base-x mode.

Could we get an ack from PHY maintainers?