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

Choong Yong Liang
2025-02-07T09:20:41Z

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@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.

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