Re: [PATCH RFC v3 04/12] net: pcs: add NXP SerDes XPCS shared core

[Jan Petrous]

On Tue, Oct 13, 2026 at 04:47:38PM +0200, Jan Petrous wrote:
> On Tue, Oct 13, 2026 at 03:47:58AM +0800, Coia Prant wrote:
> > Hi Jan,
> > 
> > Thanks for the pointers. A few thoughts.
> > 
> > > Yes. The S32G accessor is the same DW indirect scheme pcs-xpcs-plat
> > > implements (page into the viewport at 0xff, then the low address byte
> > > at a 4-byte stride), so there is no reason for S32G to carry its own
> > > copy of the XPCS code. The next version drops the shared core, exposes
> > > each XPCS instance as a mii_bus from the SerDes PHY driver and creates
> > > the PCS with xpcs_create_pcs_mdiodev(), much like the RK3568 glue.
> > > Details in my reply to Andrew.
> > >
> > > The part that does not fit pcs-xpcs-plat as-is is ownership, not
> > > access: the XPCS windows sit inside the SerDes subsystem register
> > > space, and their bring-up is ordered against the PCIe lane and the
> > > other XPCS instance, so the bus is registered by the SerDes PHY driver
> > > rather than by a standalone snps,dw-xpcs device. The accessor itself
> > > is a few lines; if Alex's pcs-xpcs-regmap is merged first I will use
> > > it rather than open-code another bus.
> > 
> > On the transport split: I think the cleanest path is to split the MMIO
> > accessor out into its own module, say pcs-xpcs-mmio, exporting the
> > read/write helpers. Then pcs-xpcs-plat can call into those exported
> 
> Fine with me. The S32G accessor is a few lines, so I will move it onto
> pcs-xpcs-regmap if that lands first, or onto a shared mmio helper if
> you post one. Until then it stays open-coded in the glue.
> 
> > symbols instead of open-coding the page/stride scheme, and S32G can do
> > the same for its indirect window. That way nobody carries a private
> > copy of the accessor. If that turns out to be too invasive for plat,
> > keeping a separate pcs-xpcs-plat copy is workable too, but I'd rather
> > avoid it.
> > 
> > If Alex's pcs-xpcs-regmap lands first, that's the natural place for
> > the S32G accessor; pcs-xpcs-mmio would only be needed if the regmap
> > approach doesn't fit.
> > 
> > > S32G needs the same kind of thing: skip the generic soft reset, and no
> > > EEE registers. I will start with the existing PMA-ID mechanism, since
> > > that is what TXGBE and FBNIC use today, and switch to a callback if one
> > > is agreed in the Rockchip thread. I'll Cc you on those patches.
> > 
> > On the shared-core duplication: rather than copying the whole XPCS
> > core, I'd look at how the existing users express their quirks.
> > 
> > Wangxun has a genuinely different PMA ID, so it can branch on the
> > ID before the reset path and keep the generic core untouched.
> > 
> > The older NXP devices do the opposite: they fake the PCS ID at the MDIO
> > layer so the core sees a magic value it then interprets. That leaves
> > the magic in the core and a fake ID in the glue, and it doesn't scale
> > to a third user.
> > 
> > For S32G, I'd prefer one of these two approaches:
> > 
> > - A flag in struct dw_xpcs, e.g. DW_XPCS_NO_RESET, that the glue sets
> >   and the core checks before the soft reset and EEE paths. Works when
> >   the quirk applies to a whole instance regardless of the PMA ID.
> > - A platform callback, e.g. plat->pma_config(), where the glue can
> >   return early to skip the parts it doesn't need. More flexible, but
> >   also more surface area; I'd only go there if flags aren't enough.
> > 
> > Either is better than another fake ID, because the quirk stays visible
> > at the point where it matters instead of being hidden behind a made-up
> > value in the transport layer.
> > 
> 
> Agreed on the principle. The problem is that pcs_pre_config and the
> PMA hook are reached only through the ID-keyed compat table, and S32G
> needs a pma_config (link timer, lane on MPLLA). Without a platform
> callback the ID is the only way in.
> 
> What I have for v4 is an ID-keyed entry with the two quirks as
> explicit flags in struct dw_xpcs_desc (no soft reset, no EEE), so at
> least the table says what the entry does.
> 

A follow-up with measurements, since "the glue needs to skip the soft
reset" deserved more than my word. I ran the v4 pcs-xpcs patches on an
S32G399A-RDB3 with both quirks removed, so the S32G entry went through
the generic xpcs_pre_config() soft reset and the generic EEE path.

Soft reset: it is a hardware requirement, not a preference. The SGMII
soft reset writes BMCR_RESET in MMD 31. On the in-band lane the bit
never self-clears, xpcs_poll_reset() times out after 600 ms, and from
then on every register of that XPCS reads 0x8000, BMSR and
VR_MII_DIG_STS included. The block is stuck in reset until the next
SerDes bring-up.
On the other two lanes the reset bit did clear, but the receive path
was dead afterwards: links reported up, TX counted, zero RX frames,
no ARP across either lane. Restoring only the no-soft-reset flag
brought back the reference numbers (2.35/2.32 Gbit/s at 2.5G, 941/942
at 1G, 0 failures in 36 speed changes, 10/10 down/up on each lane).
The reason is the shared combo PHY: the XPCS reset that works is
VR_MII_DIG_CTRL1.VR_RST inside the vendor sequence, with the power-good
wait and the RX reset around it, and that sequence is ordered against
the PCIe lane and the other XPCS instance, so the SerDes driver has to
own it.

EEE: VR_MII_EEE_MCTRL0/1 read as zero and ignore writes, so the generic
xpcs_config_eee() does no damage on S32G. I still want the no-EEE flag:
the registers are not implemented, and a driver should not read and
write registers it knows are not there, nor let phylink believe the PCS
configured EEE when it did not. The flag is a statement of fact about
the integration, not a workaround.

The AN-complete gate in xpcs_get_state_c37_sgmii() also ran ungated in
that test and the stale-speed race stayed fixed, so I can post it as
generic behaviour rather than behind a flag if nobody objects. It has
only been tested on S32G, though.

That leaves S32G needing three things from pcs-xpcs for 1G: skipping
the soft reset, no EEE register access, and a .pma_config for the PMA.
(2.5G will later need the PMA hook on every pcs_config, because the
lane moves between MPLLA and MPLLB with the line rate. That is why a
PMA entry point stays necessary whatever happens to the quirks.)

On the ID: I would rather not add another synthetic PCS ID either. The
Nord RFC picked the same 0x30, which makes your collision point for me.
What the core already has is the dw_xpcs_info platform data that
pcs-xpcs-plat passes for the PMA generations it cannot read from
hardware. S32G could do the same: leave the PCS ID native, it reads the
real 0x7996ced0, and declare an S32G PMA token in dw_xpcs_info, with
xpcs_identify() matching the desc on the PMA token as well. No faked
register read, and the token names what actually differs on S32G, the
PMA integration. The core change is a .pma field in struct dw_xpcs_desc
and about ten lines. If you prefer glue-set flags, the no-soft-reset and
no-EEE parts fit them directly; the .pma_config still needs the entry.

Tell me which of the two you would rather review and I will shape next
series (most likely not v4 as I have it tested and ready for send
already) accordingly.

BR.
/Jan