Hi Maxime,

On 7/3/26 09:29, Maxime Chevallier wrote:

Ah nice conversion to phylink PCS :) I have a few comments below

Thanks for the review!

[...]

quoted

+	/* This driver does not implement SGMII in-band autonegotiation yet, so
+	 * the link parameters are forced from rtl8365mb_pcs_link_up() instead.
+	 * rtl8365mb_pcs_inband_caps() reports this to phylink, which should
+	 * therefore never select an in-band-enabled negotiation mode.
+	 */
+	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
+		return -EOPNOTSUPP;

As you implement the .pcs_inband_caps() method, phylink will pass you a valid
mode, no need for that check :)

Indeed. I went through phylink_pcs_neg_mode() again and with
pcs_inband_caps() returning LINK_INBAND_DISABLE every path resolves to
an outband (or inband-disabled, for the 802.3z no-PHY case) negotiation
mode, so this is dead code. I will drop it in v5.

[...]

quoted

+		/* The SerDes has its own pause controls; program them from
+		 * the resolved pause modes, as the vendor driver does when
+		 * forcing the link on a SerDes external interface. This is
+		 * done here rather than in rtl8365mb_pcs_link_up() because
+		 * pcs_link_up() carries no pause information.
+		 */
+		if (rtl8365mb_interface_is_serdes(interface)) {
+			u32 val = 0;
+
+			if (tx_pause)
+				val |= RTL8365MB_SDS_MISC_SGMII_TXFC_MASK;
+			if (rx_pause)
+				val |= RTL8365MB_SDS_MISC_SGMII_RXFC_MASK;

Do you know what this does in HW ? Is this so that the PCS lets the Pause frames through
in either directions ?

I suspect this is something that would be only used for inband advertising
of pause settings (in such case, you don't even need that), but ofc I'm not sure :)

You already configure the MAC pause settings, can you test that these bits actually do
anything by exercising a bit flow control and checking if these registers are used ?

I tested it on the hardware, and it turns out these bits are the
operative pause controls for the SerDes port: pause frames are only
ever emitted when SDS_MISC_SGMII_TXFC is set, and the MAC force-mode
pause bits alone have no observable effect on this port. They are not
tied to in-band advertisement either - they act with the link fully
forced.

For context, there is no public documentation for these bits, but in
the vendor GPL code every write to them lives in
rtl8367c_setAsicPortForceLinkExt(), the forced-link path, where they
are programmed from the resolved tx/rx pause settings right alongside
the MAC force-mode pause bits - never from the nway/in-band paths.

Test setup: RTL8367S (Mercusys MR80X), CPU port 6 on HSGMII,
2500base-x fixed link to an IPQ5018. I enabled regmap debugfs writes
so I could flip the bits live, and forced congestion by L2-forwarding
~350 Mbit/s of SoC-generated UDP in through the CPU port and out of a
100M user port (~3.5x oversubscription, ~135 MB per run), watching the
CPU port's dot3OutPauseFrames MIB counter:

  everything cleared (baseline):                 0 pause frames
  MAC force pause only (DIGITAL_INTERFACE_FORCE
      TXPAUSE|RXPAUSE, reg 0x1311):              0 pause frames
  MAC force pause + SDS TXFC|RXFC:            2396 pause frames
  SDS TXFC|RXFC only (MAC pause cleared):     2362 pause frames
  SDS TXFC only:                              2934 pause frames
  SDS RXFC only:                                 0 pause frames

RXFC did not influence pause emission, so by symmetry it presumably
gates honouring received pause frames; I could not exercise that
direction because the SoC MAC on this board never transmits pause.

So the write has to stay - without it flow control simply does not
work on SGMII/HSGMII. I will expand the comment in v5 to say that
these are the functional pause enables for the SerDes, rather than
describing it as mirroring the vendor forced-link sequence.

Best regards,
Johan