diff mbox series

[net] net: phy: dp83869: fix status reporting for 1000base-x autonegotiation

Message ID 20241029-dp83869-1000base-x-v1-1-fcafe360bd98@bootlin.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net] net: phy: dp83869: fix status reporting for 1000base-x autonegotiation | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 5 this patch: 5
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 9 of 9 maintainers
netdev/build_clang fail Errors and warnings before: 3 this patch: 6
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 Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 4 this patch: 4
netdev/checkpatch warning WARNING: line length of 81 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Romain Gantois Oct. 29, 2024, 9:30 a.m. UTC
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>
---
 drivers/net/phy/dp83869.c | 130 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 127 insertions(+), 3 deletions(-)


---
base-commit: 94c11e852955b2eef5c4f0b36cfeae7dcf11a759
change-id: 20241025-dp83869-1000base-x-0f0a61725784

Best regards,

Comments

kernel test robot Oct. 29, 2024, 5:57 p.m. UTC | #1
Hi Romain,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 94c11e852955b2eef5c4f0b36cfeae7dcf11a759]

url:    https://github.com/intel-lab-lkp/linux/commits/Romain-Gantois/net-phy-dp83869-fix-status-reporting-for-1000base-x-autonegotiation/20241029-173146
base:   94c11e852955b2eef5c4f0b36cfeae7dcf11a759
patch link:    https://lore.kernel.org/r/20241029-dp83869-1000base-x-v1-1-fcafe360bd98%40bootlin.com
patch subject: [PATCH net] net: phy: dp83869: fix status reporting for 1000base-x autonegotiation
config: arm-randconfig-004-20241029 (https://download.01.org/0day-ci/archive/20241030/202410300125.K125vk3f-lkp@intel.com/config)
compiler: clang version 16.0.6 (https://github.com/llvm/llvm-project 7cbf1a2591520c2491aa35339f227775f4d3adf6)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241030/202410300125.K125vk3f-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410300125.K125vk3f-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/net/phy/dp83869.c:197:3: warning: variable 'adv' is uninitialized when used here [-Wuninitialized]
                   adv |= DP83869_BP_FULL_DUPLEX;
                   ^~~
   drivers/net/phy/dp83869.c:174:9: note: initialize the variable 'adv' to silence this warning
           u32 adv;
                  ^
                   = 0
   1 warning generated.


vim +/adv +197 drivers/net/phy/dp83869.c

   168	
   169	static int dp83869_config_aneg(struct phy_device *phydev)
   170	{
   171		struct dp83869_private *dp83869 = phydev->priv;
   172		unsigned long *advertising;
   173		int err, changed = false;
   174		u32 adv;
   175	
   176		if (dp83869->mode != DP83869_RGMII_1000_BASE)
   177			return genphy_config_aneg(phydev);
   178	
   179		/* Forcing speed or duplex isn't supported in 1000base-x mode */
   180		if (phydev->autoneg != AUTONEG_ENABLE)
   181			return 0;
   182	
   183		/* In fiber modes, register locations 0xc0... get mapped to offset 0.
   184		 * Unfortunately, the fiber-specific autonegotiation advertisement
   185		 * register at address 0xc04 does not have the same bit layout as the
   186		 * corresponding standard MII_ADVERTISE register. Thus, functions such
   187		 * as genphy_config_advert() will write the advertisement register
   188		 * incorrectly.
   189		 */
   190		advertising = phydev->advertising;
   191	
   192		/* Only allow advertising what this PHY supports */
   193		linkmode_and(advertising, advertising,
   194			     phydev->supported);
   195	
   196		if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT, advertising))
 > 197			adv |= DP83869_BP_FULL_DUPLEX;
   198		if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT, advertising))
   199			adv |= DP83869_BP_PAUSE;
   200		if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, advertising))
   201			adv |= DP83869_BP_ASYMMETRIC_PAUSE;
   202	
   203		err = phy_modify_changed(phydev, DP83869_FX_ANADV,
   204					 DP83869_BP_FULL_DUPLEX | DP83869_BP_PAUSE |
   205					 DP83869_BP_ASYMMETRIC_PAUSE,
   206					 adv);
   207	
   208		if (err < 0)
   209			return err;
   210		else if (err)
   211			changed = true;
   212	
   213		return genphy_check_and_restart_aneg(phydev, changed);
   214	}
   215
diff mbox series

Patch

diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
index 5f056d7db83eed23f1cab42365fdc566a0d8e47f..7f89a4f963cab50d6954e8b8996d7bbe2c72a9ca 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;
+
+	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,			\