On Wed, Oct 28, 2026 at 10:02:56AM +0000, Akash Nair wrote:
> This RFC adds an interface for Ethernet PCS (Physical Coding
> Sublayer) devices and a driver for the Synopsys DesignWare Ethernet
> PCS (DW XPCS) that implements it. I would like feedback on the
> interface and on how the driver is structured before I post the MAC
> driver that uses it.
>
> Background
> ----------
> On SoCs with a DW XPCS between the MAC and the SerDes, the MAC driver
> has to configure the PCS and read back its link and auto-negotiation
> state to bring up 1000BASE-KX, 10GBASE-R and 10GBASE-KR links in
> U-Boot, for example for network boot.
>
> U-Boot has no reusable driver for this IP.
>
> What is in the series
> ---------------------
> 1/2 net: pcs: Add generic PCS interface
> 2/2 net: pcs: Add Synopsys DesignWare XPCS driver
>
> Patch 1 adds include/pcs.h. Patch 2 adds the driver in
> drivers/net/pcs/ with its Kconfig and Makefile, and hooks them into
> drivers/net.
>
> Architecture
> ------------
> +--------------------+
> | MAC driver | (follow-up patch, not in this series)
> +---------+----------+
> | pcs_set_config() pcs_get_state() pcs_disable()
> v
> +--------------------+
> | include/pcs.h | patch 1: struct pcs_ops, pcs_config,
> | | pcs_state
> +---------+----------+
> | ops of the PCS device
> v
> +--------------------+
> | pcs-xpcs.c | patch 2: UCLASS_MISC, "snps,dw-xpcs"
> | indirect window: | page register + register offset
> +---------+----------+
> | MMIO reg-names = "indirect", reg-io-width = <4>
> v
> +--------------------+ +--------------+
> | DW XPCS |<----->| PMA / SerDes |<--> link partner
> +--------------------+ +--------------+
> 1000BASE-KX: C37 AN 10GBASE-R: no AN 10GBASE-KR: C73 AN
>
> Interface (patch 1)
> -------------------
> include/pcs.h defines struct pcs_ops with three operations, and an
> inline wrapper for each one. A wrapper returns -ENOSYS if the driver
> does not provide the operation.
>
> pcs_set_config() select the interface mode, the auto-negotiation
> type (none, Clause 37 or Clause 73) and the
> speed, using struct pcs_config
> pcs_get_state() report link, auto-negotiation complete and speed,
> using struct pcs_state
> pcs_disable() stop auto-negotiation and power the PCS down
>
> Semantics:
> - set_config() can be called again later, including after
> pcs_disable(). If it fails, the PCS is left unconfigured.
> - get_state() reports the speed as soon as the PCS is configured,
> not when the link comes up. A powered down or unconfigured PCS
> reports link, an_complete and speed as 0.
> - A negative return from get_state() is an error, for example a
> failed Clause 73 negotiation.
> - Calling pcs_disable() again has no effect.
>
> Driver (patch 2)
> ----------------
> drivers/net/pcs/pcs-xpcs.c is a driver model driver for the
> "snps,dw-xpcs" compatible. It is a UCLASS_MISC device and is enabled
> with PCS_XPCS, which depends on MISC.
>
> The XPCS registers are Clause 45 registers in the PMA/PMD, PCS, AN and
> vendor MMDs. They are reached through the memory mapped indirect
> window: the driver writes the page number of the register to the page
> register of the window, then accesses the register at its offset in
> the page. The selected page is cached to avoid redundant writes.
>
> At probe the driver waits for the PCS to come out of reset and checks
> the Clause 45 device ID. It does not configure a mode. The MAC driver
> does that with pcs_set_config(). The driver checks the whole request
> before it touches the hardware, so a bad request does not leave the
> PCS half configured. It then stops auto-negotiation, programs the
> mode, power cycles the PCS to latch it, and starts auto-negotiation if
> the mode uses it.
>
> Supported modes:
> 1000BASE-KX Clause 37 auto-negotiation
> 10GBASE-R fixed speed, no auto-negotiation
> 10GBASE-KR Clause 73 auto-negotiation
>
> pcs_set_config() accepts only these combinations. Anything else is
> rejected before the hardware is touched, with -EOPNOTSUPP for a wrong
> auto-negotiation type and -EINVAL for a wrong speed or interface.
>
> iface an_mode speed_mbps
> 1000BASE-KX PCS_AN_C37 0 or 1000
> 10GBASE-R PCS_AN_FORCED 10000 (required)
> 10GBASE-KR PCS_AN_C73 0 or 10000
>
> Auto-negotiation completion is polled from pcs_get_state(). The link
> status bit is latched low, so it is read twice and the second value is
> reported. For Clause 73, pcs_get_state() returns an error if the link
> partner cannot do 10GBASE-KR or uses Next Pages.
>
> Devicetree
> ----------
> The driver uses the existing "snps,dw-xpcs" binding from dts/upstream
> (Bindings/net/pcs/snps,dw-xpcs.yaml), so no binding change is needed.
> Only part of the binding is implemented. reg-names must be "indirect",
> the window must be at least 1 KiB, and reg-io-width must be <4>. The
> binding default of 2 is rejected, so the property cannot be left out.
> The clocks and the interrupt are not used. The driver does not enable
> clocks, so they must already be running when it probes. The gen*-PMA
> compatibles are not matched.
>
> Example node:
>
> pcs@1f05d000 {
> compatible = "snps,dw-xpcs";
> reg = <0x1f05d000 0x1000>;
> reg-names = "indirect";
> reg-io-width = <4>;
> };
>
> Origin of the code
> ------------------
> Some parts of the driver are influenced by the Linux XPCS driver
> (drivers/net/pcs/pcs-xpcs*.c).
>
> Design choices
> --------------
> UCLASS_MISC: a PCS is a UCLASS_MISC device and the MAC driver is
> expected to get it by phandle. This is what the Airoha PCS driver and
> airoha_eth do today. A dedicated UCLASS_PCS would be cleaner, but I
> left it out of this series on purpose to keep the scope small.
>
> No in-tree user yet: the MAC driver that calls this interface will
> follow as a separate patch in about a month. Until then PCS_XPCS is
> not selected by anything and not enabled in any defconfig. This is
> why this is an RFC.
>
> Questions for reviewers
> -----------------------
> 1. Is UCLASS_MISC with the ops in include/pcs.h acceptable as a
> starting point?
> 2. Are struct pcs_config and struct pcs_state enough for other PCS
> drivers, or is something missing?
> 3. Auto-negotiation completion is polled from get_state() and the
> interrupt is not used. Is that fine for U-Boot?
>
> Testing
> -------
> Not tested on hardware yet.
> Build tested with CONFIG_PCS_XPCS=y on qemu_arm64.
> checkpatch.pl --strict reports no errors.
>
> Not done
> --------
> - Interface modes other than the three above. More modes are planned
> once this is approved.
> - Clause 73 Next Pages: a link partner that uses them is rejected
> - Register access over MDIO and the "direct" mapping
> - Clocks and the interrupt from the binding
> - PMA/SerDes initialisation: not done by this driver, it must be set
> up before pcs_set_config() is called
> - SPDX tags and a MAINTAINERS entry (checkpatch warnings)
> - Sandbox emulation and a dm test
> - A dedicated UCLASS_PCS
>
> Akash Nair (2):
> net: pcs: Add generic PCS interface
> net: pcs: Add Synopsys DesignWare XPCS driver
>
> drivers/net/Kconfig | 1 +
> drivers/net/Makefile | 1 +
> drivers/net/pcs/Kconfig | 12 +
> drivers/net/pcs/Makefile | 1 +
> drivers/net/pcs/pcs-xpcs.c | 881 +++++++++++++++++++++++++++++++++++++
> include/pcs.h | 140 ++++++
> 6 files changed, 1036 insertions(+)
> create mode 100644 drivers/net/pcs/Kconfig
> create mode 100644 drivers/net/pcs/Makefile
> create mode 100644 drivers/net/pcs/pcs-xpcs.c
> create mode 100644 include/pcs.hFirst thing is that none of the new files have SPDX headers. The second thing would be how is this tested then sine you don't mention running it in an emulator of sorts. The third thing would be you should include some documentation, especially since you plan to move this to its own UCLASS, which you should probably just do to start with. You should also make sure that this compiles on sandbox, today. Thanks. -- Tom
signature.asc
Description: PGP signature
