Re: [PATCH net-next v6 2/2] net: dsa: realtek: rtl8365mb: add HSGMII support for RTL8367S

[Johan Alvarado <contact@c127.dev>]

Hi Mieczyslaw,

On 7/12/2026 8:05 PM, Mieczyslaw Nalewaj wrote:

[...]

> As discussed earlier in the thread: the rate limiter helper, as well
> as pcs_config()/RTL8365MB_SDS_BYPASS_LINE_RATE_MASK, currently assume
> the SerDes is always muxed to external interface 1 / port 6
> (RTL8365MB_SDS_EXT_INTERFACE_ID / _PORT). That's true for every chip
> currently in rtl8365mb_chip_infos[] (RTL8367S and RTL8367SB both have
> their SGMII/HSGMII-capable extint at { 6, 1, ... }), so nothing is
> broken today.
>
> I still think it's worth guarding against this ahead of time though.
> It isn't an architectural constant of the family, just something that
> happens to hold for the two chips currently in the table, and there's
> no guarantee a future chip won't mux its SerDes to a different port
> or extint id.

I see it the other way around: for the family this driver covers,
the port 6 muxing is architectural. Every entry in
rtl8365mb_chip_infos[] is chip id 0x6367, and on this silicon the
SerDes mux is a pair of bits in SDS_MISC that select SGMII or HSGMII
specifically for MAC8 - that is what the
RTL8365MB_SDS_MISC_MAC8_SEL_* names in the driver reflect. Neither
the register set nor the vendor driver offers a way to route the
SerDes anywhere else on this chip id. A chip that muxes its SerDes
to a different interface would not be a new table entry; it would be
different silicon with a different SerDes datapath.

Your draft illustrates this, I think: it makes the
DIGITAL_INTERFACE_SELECT write follow mb->sds_id, but the SerDes mux
write in pcs_config() still sets the MAC8_SEL bits unconditionally,
because there is nothing else it could set. On the hypothetical
future chip, the generalized path would program the interface mode
for the cached id while still muxing the SerDes to MAC8 - as broken
as before, only harder to see, because the code now looks as if it
handled the case.

The bypass mask has the same problem on a different axis. As came up
in the v4 review, the bit layout of the line rate bypass register is
not a function of the port number across the wider RTL8367 family;
the BIT(port - 5) relationship in RTL8365MB_BYPASS_LINE_RATE_MASK()
is the layout of this chip id specifically. The same goes for the
SerDes tuning tables, the chip option probe and the rate limiter
register addresses: all of them are tied to this silicon, not
derivable from the extint table. Caching the extint port and id
would generalize the two easiest parameters and leave every hard one
behind.

> Since rtl8365mb_sds_probe_option() already walks
> chip_info->extints[] looking for the SGMII/HSGMII-capable one, I'd
> like to have it cache the discovered port and id (mb->sds_port /
> mb->sds_id) instead of relying on the fixed
> RTL8365MB_SDS_EXT_INTERFACE_ID/_PORT. pcs_config() and the bypass
> line-rate mask would then use the cached values, and
> rtl8365mb_sds_raise_rate_limits() would check mb->sds_port == 6 and
> warn+skip instead of assuming, since only the port 6 register
> addresses are known/verified so far.

The warn+skip in particular I think would be actively harmful:
skipping the write means that hypothetical chip ships with the
reset-default ~1.048 Gbps cap - the exact problem this patch exists
to fix, reduced to a one-line boot warning. The MR85X report showed
how easy that cap is to miss; it took multi-client throughput
testing to even notice it. I would rather keep the assumption a
hard, greppable constant next to a comment stating it, so that
whoever adds such a chip trips over it while writing the chip_info
entry and brings the SerDes path up against real hardware - which
they will have to do anyway, for the mux, the tuning tables, the
option probe and the limiter addresses alike.

So I would prefer to keep the series as is. If a chip with the
SerDes on another interface ever appears, its submitter will have a
datasheet or vendor code in hand and can parametrize exactly what
that silicon needs, with values that can be verified. Doing it now
would generalize the two parameters we can test and guess at the
rest.

Best regards,
Johan