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

[Choong Yong Liang]

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:
> > 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@xxxxxxxxxxxxxxxxxxxxxx/

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.

> > +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.
>
> > +			//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.