Re: [PATCH net-next v4 1/2] net: dsa: realtek: rtl8365mb: add SGMII support for RTL8367S

[Johan Alvarado]

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!

[...]

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

[...]

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