Hi Akash,
> 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.
I would typically expect a driver, even if explicitly labelled not
ready for merge, to enable people to see how this would work.
> 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.
Do you happen to know if there's other drivers in U-Boot currently
that are poking the PCS interfaces directly? Not asking you to convert
them, more just wondering if it's been hacked up elsewhere.
> What is in the series
> ---------------------
> 1/2 net: pcs: Add generic PCS interface
> 2/2 net: pcs: Add Synopsys DesignWare XPCS driver
Is the DW driver based on the proposed Linux driver for this? [1]
> 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.
Can you explain why you chose to depend on UCLASS_MISC for the
functionality other than scope? It seems like
Hi Akash,
> 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.
I would typically expect a driver, even if explicitly labelled not
ready for merge, to enable people to see how this would work.
> 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.
Do you happen to know if there's other drivers in U-Boot currently
that are poking the PCS interfaces directly? Not asking you to convert
them, more just wondering if it's been hacked up elsewhere.
> What is in the series
> ---------------------
> 1/2 net: pcs: Add generic PCS interface
> 2/2 net: pcs: Add Synopsys DesignWare XPCS driver
Is the DW driver based on the proposed Linux driver for this? [1]
> 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.
Can you explain why you chose to depend on UCLASS_MISC for the
functionality other than scope? I think I would sooner see a
UCLASS_PCS now rather than a lot of driver duplicated work that will
need to be fixed later. Maybe work with the Airoha maintainer to
convert that driver as an example, the Airoha support is quite new.
> 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?
I would sooner we go straight to UCLASS_PCS, it might be a bit more
work now, but will be less maintenance overall.
> 2. Are struct pcs_config and struct pcs_state enough for other PCS
> drivers, or is something missing?
I would like to see what other drivers poke the PCS stuff in tree as I
doubt this is just Airoha.
> 3. Auto-negotiation completion is polled from get_state() and the
> interrupt is not used. Is that fine for U-Boot?
I suspect it's probably fine, does get state use the event loop interface?
> Testing
> -------
> Not tested on hardware yet.
> Build tested with CONFIG_PCS_XPCS=y on qemu_arm64.
> checkpatch.pl --strict reports no errors.
I would like to see actual tests as part of the patch series.
> Not done
Are you intending to do this list below? It's not clear.
> --------
> - Interface modes other than the three above. More modes are planned
> once this is approved.
Please outline where you expect those to go, it helps provide context
to the actual reviewers rather than telling a small part of the story,
it also may show that a PCS uclass is more appropriate than using
misc.
> - 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
How is that done? How is it documented? Again the sample driver would
likely show this.
> - SPDX tags and a MAINTAINERS entry (checkpatch warnings)
> - Sandbox emulation and a dm test
> - A dedicated UCLASS_PCS
Peter
[1] https://lwn.net/Articles/1099457/
