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.
