Re: [PATCH net-next v7 2/7] net: pcs: xpcs: re-initiate clause 37 Auto-negotiation

On 6/2/2025 11:30 pm, Russell King (Oracle) wrote:

On Thu, Feb 06, 2025 at 09:18:54PM +0800, Choong Yong Liang wrote:

quoted

The xpcs_switch_interface_mode function was introduced to handle
interface switching.

According to the XPCS datasheet, a soft reset is required to initiate
Clause 37 auto-negotiation when the XPCS switches interface modes.

Hmm. Given that description, taking it literally, claus 37
auto-negotiation is 1000BASE-X, not Cisco SGMII (which isn't an IEEE
802.3 standard.) Are you absolutely sure that this applies to Cisco
SGMII?

Hi Russell,

Yes, you are correct that Clause 37 auto-negotiation is for 1000BASE-X. 
However, I do not believe it applies to Cisco SGMII. The XPCS implements 
Clause 37 auto-negotiation for both 1000BASE-X and SGMII.

If the reset is required when switching to SGMII, should it be done
before or after configuring the XPCS for SGMII?

A soft reset is required before configuring the XPCS for SGMII. Based on 
the existing XPCS handling in the initial state, the xpcs_create() function 
will be called, and then xpcs->need_reset will be set to true. Later on, 
phylink_major_config() will call xpcs_pre_config() to perform the 
xpcs_soft_reset(), and then it will continue with xpcs_config().

I apologize for missing this patch: 
https://patchwork.kernel.org/project/netdevbpf/patch/E1svfMA-005ZI3-Va@rmk-PC.armlinux.org.uk/

I think I should move xpcs_switch_interface_mode() to xpcs_pre_config() and 
just update xpcs->need_reset instead of implementing my own method for 
calling xpcs_soft_reset().

Also, if the reset is required, what happens if we're already using
SGMII, but AN has been disabled temporarily and is then re-enabled?
Is a reset required?

Good point. I cannot find this scenario in the datasheet. Please allow me 
some time to test this scenario. I will update you with the results.

What about 1000BASE-X when AN is enabled or disabled and then switching
to SGMII?

According to the datasheet, a soft reset is required.

quoted

+static int xpcs_switch_to_aneg_c37_sgmii(const struct dw_xpcs_compat *compat,
+					 struct dw_xpcs *xpcs,
+					 unsigned int neg_mode)
+{
+	bool an_c37_enabled;
+	int ret, mdio_ctrl;
+
+	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) {
+		mdio_ctrl = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_BMCR);
+		if (mdio_ctrl < 0)
+			return mdio_ctrl;
+
+		an_c37_enabled = mdio_ctrl & BMCR_ANENABLE;
+		if (!an_c37_enabled) {

I don't think that we need "an_c37_enabled" here, I think simply:

		if (!(mdio_ctrl & BMCR_ANENABLE)) {

would be sufficient.

quoted

+			//Perform soft reset to initiate C37 auto-negotiation
+			ret = xpcs_soft_reset(xpcs, compat);
+			if (ret)
+				return ret;
+		}
+	}
+	return 0;

I'm also wondering (as above) whether this soft reset needs to happen
_after_ xpcs_config_aneg_c37_sgmii() has done its work - this function
temporarily disables AN while it's doing its work.

Based on the programming sequence in the datasheet, it is not necessary to 
perform a soft reset after xpcs_config_aneg_c37_sgmii() has completed its work.

I'm also wondering whether AN being disabled is really a deciding
factor (e.g. when switching from 1000BASE-X AN-enabled to SGMII, is a
reset required?)

Thank you for pointing this out. The datasheet only mentions performing a 
soft reset when switching to the 1000BASE-X and SGMII interfaces, and it 
does not specify whether AN needs to be enabled or disabled. I thought 
adding a check would reduce the calls to the soft reset. However, I did not 
consider the scenario of switching from 1000BASE-X with AN enabled to SGMII 
with AN enabled. This scenario might cause regression. I will remove all 
the checks and just perform a soft reset when switching to the SGMII interface.