Re: [PATCH net-next v5 07/11] net: pcs: xpcs: add Rockchip RK3568 platform glue driver

[off-list ref] 于2026年9月15日周二 04:24写道：

Thank you for your contribution! Sashiko AI review found 6 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 3 · Low: 2

- [High] Incorrect MMD write remapping for MII vendor registers breaks
  configuration for ports 1, 2, and 3.
- [Medium] The patch silently re-packages the pre-existing generic
  Synopsys XPCS platform driver.
- [Medium] xpcs_rk_init_clk() (drivers/net/pcs/pcs-xpcs-rk.c:361-362)
  computes `pxpcs->eee_mult_fact = DIV_ROUND_CLOSEST(100 *…
- [Medium] Acquiring `device_lock()` of another device from within a
  probe path causes a lockdep warning (recursive locking).
- [Low] Concern that the newly exported xpcs_rk_create()
  (include/linux/pcs/pcs-xpcs-rk.h) has no in-tree caller and no Kconfig…
- [Low] The rockchip,rk3568-xpcs binding lists `phys` and `phy-names` in
  its `required:` list…

--

net: pcs: xpcs: add Rockchip RK3568 platform glue driver

Adds a platform glue driver for the DesignWare XPCS block found on RK3568,
exposing the APB3-mapped XPCS registers as a virtual MDIO bus with per-port
MMD remapping so the generic pcs-xpcs core can create a phylink_pcs.  Also
adds new Kconfig symbols for the generic and Rockchip XPCS glue and an
exported xpcs_rk_create() helper for MAC drivers.

Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part2%20V1.1-20210301.pdf (Page 2078)

quoted

diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
index e417fd66f660a..080538f193d59 100644
--- a/drivers/net/pcs/Kconfig
+++ b/drivers/net/pcs/Kconfig

@@ -12,6 +12,31 @@ config PCS_XPCS
        This module provides a driver and helper functions for Synopsys
        DesignWare XPCS controllers.

+if PCS_XPCS
+
+config PCS_XPCS_PLATFORM
+     tristate "Generic XPCS controller support"
+     default PCS_XPCS
+     help
+       Generic DWXPCS driver for platforms that don't require any
+       platform specific code to function or is using platform
+       data for setup.
+
+       If you have a controller with this interface, say Y or M here.
+
+config PCS_XPCS_ROCKCHIP
+     tristate "Rockchip XPCS controller support"
+     default ARCH_ROCKCHIP
+     depends on OF && (ARCH_ROCKCHIP || COMPILE_TEST)
+     depends on GENERIC_PHY || COMPILE_TEST
+     depends on PM_GENERIC_DOMAINS || COMPILE_TEST
+     help
+       Support for XPCS controller on Rockchip RK356x SoC.
+
+       If you have a Rockchip SoC with this interface, say Y or M here.
+
+endif # PCS_XPCS
+
 config PCS_LYNX
      tristate
      help

diff --git a/drivers/net/pcs/Makefile b/drivers/net/pcs/Makefile
index 4f7920618b900..f9f6cf2578d72 100644
--- a/drivers/net/pcs/Makefile
+++ b/drivers/net/pcs/Makefile

@@ -1,10 +1,11 @@
 # SPDX-License-Identifier: GPL-2.0
 # Makefile for Linux PCS drivers

-pcs_xpcs-$(CONFIG_PCS_XPCS)  := pcs-xpcs.o pcs-xpcs-plat.o \
-                                pcs-xpcs-nxp.o pcs-xpcs-wx.o
+pcs_xpcs-$(CONFIG_PCS_XPCS)  := pcs-xpcs.o pcs-xpcs-nxp.o pcs-xpcs-wx.o

 obj-$(CONFIG_PCS_XPCS)               += pcs_xpcs.o
+obj-$(CONFIG_PCS_XPCS_PLATFORM) += pcs-xpcs-plat.o
+obj-$(CONFIG_PCS_XPCS_ROCKCHIP) += pcs-xpcs-rk.o

[Severity: Medium]
Should this repackaging of the pre-existing generic platform glue be a
separate patch, or at least be described in the commit message?

pcs-xpcs-plat.o is moved out of the pcs_xpcs composite object and built as
its own module, and the new PCS_XPCS_PLATFORM tristate makes the snps,dw-xpcs
platform glue user-selectable where it was previously unconditional under
PCS_XPCS.  So the snps,dw-xpcs platform driver ships in pcs-xpcs-plat.ko
instead of pcs_xpcs.ko after this patch.

The commit message only describes adding the Rockchip glue driver and does
not mention this change to existing functionality.  Could the Makefile and
Kconfig refactor be split out with its own rationale?

We have added a default value for existing users when selecting PCS_XPCS.

quoted

+static int xpcs_rk_serdes_phy_init(struct dw_xpcs_rk *pxpcs)
+{
+     struct device *dev = &pxpcs->pdev->dev;
+
+     pxpcs->serdes_phy = devm_phy_get(dev, "serdes");
+     if (IS_ERR(pxpcs->serdes_phy))
+             return dev_err_probe(dev, PTR_ERR(pxpcs->serdes_phy),
+                                     "Failed to get SerDes PHY\n");
+
+     return 0;
+}

[Severity: Low]
xpcs_rk_serdes_phy_init() treats the "serdes" phy as mandatory and fails
probe when it cannot be resolved, and the binding added later in this series
(Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml) lists
phys and phy-names under required:

  required:
    - compatible
    - reg
    ...
    - phys
    - phy-names
    - power-domains

The SoC node added in arch/arm64/boot/dts/rockchip/rk3568.dtsi only has:

        xpcs: ethernet-pcs@fda00000 {
                compatible = "rockchip,rk3568-xpcs";
                #address-cells = <1>;
                #size-cells = <0>;
                reg = <0x0 0xfda00000 0x0 0x200000>;
                clocks = <&cru PCLK_XPCS>, <&cru CLK_XPCS_EEE>;
                clock-names = "csr", "eee";
                power-domains = <&power RK3568_PD_PIPE>;
                status = "disabled";

CHECK_DTBS validates nodes regardless of status, so does every RK3568 DTB
now report missing phys/phy-names for this node?  Only
rk3568-photonicat.dts adds them, so a board that just flips this node to
status = "okay" would also fail probe with "Failed to get SerDes PHY".

Should the SoC node carry phys/phy-names, or should the binding not mark
them required at the SoC level?

As replied earlier, the SerDes connection is board-specific (some
boards use combphy1, others combphy2), so phys/phy-names cannot move
into the SoC dtsi. dtbs_check skips required-property validation for
status = "disabled" nodes, so boards that don't enable XPCS produce no
warnings.

quoted

+static int xpcs_rk_init_clk(struct dw_xpcs_rk *pxpcs)
+{

[ ... ]

quoted

+     /*
+      * Compute the multiplier for the EEE clock so that
+      * clk_eee_period * (mult_fact + 1) falls within 80..120 ns.
+      *
+      * On RK3568, clk_xpcs_eee is muxed between gpll200 (200 MHz, 5 ns)
+      * and cpll125 (125 MHz, 8 ns), selected by CRU_CLKSEL_CON29 bit 13.
+      * The reset value is 0 (200 MHz), but derive the value at runtime to
+      * stay correct if the mux is changed by a board.
+      */
+     pxpcs->eee_mult_fact = DIV_ROUND_CLOSEST(100 * clk_get_rate(pxpcs->eee_clk),
+                                              1000000000UL) - 1;

[Severity: Medium]
Can this value overflow the 4-bit register field it ends up in?

For the reset configuration named in the comment (200 MHz, 5 ns) this
computes 100 * 200000000 / 1000000000 - 1 = 19.  That is passed on
unchecked:

        xpcs_config_eee_mult_fact(xpcs, pxpcs->eee_mult_fact);

and the core programs it in xpcs_config_eee():

        drivers/net/pcs/pcs-xpcs.c:
                      FIELD_PREP(DW_VR_MII_EEE_MULT_FACT_100NS,
                                 xpcs->eee_mult_fact);

with

        drivers/net/pcs/pcs-xpcs.h:
        #define DW_VR_MII_EEE_MULT_FACT_100NS           GENMASK(11, 8)

FIELD_PREP on a runtime value does no range check, so 19 (0x13) becomes 3.
The hardware is then told the 100 ns reference is 4 EEE clock periods
(20 ns) rather than the 80..120 ns the comment and the kernel-doc of
xpcs_config_eee_mult_fact() require, which affects all derived LPI timers
once EEE is enabled.  Since the field caps the count at 16 periods, is 15
the only representable value for a 5 ns clock, and should this be clamped?

Two more things about the same expression:

- 100 * clk_get_rate() is evaluated in unsigned long, so on 32-bit builds
  100 * 200000000 wraps to 2820130816 and yields mult_fact 2.  Would
  mul_u64_u32_div() or a u64 intermediate be better here?

- If clk_get_rate() returns 0, 0 - 1 stored in a u8 becomes 255, giving
  field value 15.  Is a zero rate worth rejecting?

You're right, and this is a real bug. Two problems:

1. 100 * clk_get_rate() overflows unsigned long on 32-bit builds.
2. DW_VR_MII_EEE_MULT_FACT_100NS is a 4-bit field (GENMASK(11, 8)),
   so the value is capped at 15. For 200 MHz the formula yields 19,
   which FIELD_PREP truncates to 3.

It will be fixed in the next version.

quoted

+struct dw_xpcs *xpcs_rk_create(struct device *dev, struct device_node *np)
+{
+     struct platform_device *pdev;
+     struct device_node *pcs_np;
+     struct dw_xpcs_rk *pxpcs;
+     struct dw_xpcs *xpcs;
+     u32 port;
+
+     if (!of_device_is_available(np))
+             return ERR_PTR(-ENODEV);
+
+     if (of_property_read_u32(np, "reg", &port))
+             return ERR_PTR(-EINVAL);
+
+     if (!xpcs_rk_mdio_addr_validate((int)port))
+             return ERR_PTR(-EINVAL);
+
+     /* The XPCS pdev is attached to the parent node */
+     pcs_np = of_get_parent(np);
+     if (!pcs_np)
+             return ERR_PTR(-ENODEV);
+
+     if (!of_device_is_available(pcs_np)) {
+             of_node_put(pcs_np);
+             return ERR_PTR(-ENODEV);
+     }
+
+     if (!of_match_node(xpcs_rk_of_ids, pcs_np)) {
+             of_node_put(pcs_np);
+             return ERR_PTR(-EINVAL);
+     }
+
+     pdev = of_find_device_by_node(pcs_np);
+     of_node_put(pcs_np);
+     if (!pdev)
+             return ERR_PTR(-EPROBE_DEFER);
+
+     device_lock(&pdev->dev);

[Severity: Medium]
xpcs_rk_create() is called from a MAC driver's probe path, which is
already running under device_lock() of the MAC device taken by
really_probe().  Taking device_lock(&pdev->dev) here nests a second
struct device mutex, and since all device mutexes share one lockdep
class unless annotated, lockdep will report "possible recursive
locking detected" on the first probe.  That is a splat in the logs on
any CONFIG_PROVE_LOCKING build, and a panic with panic_on_warn set.

Holding the XPCS device's lock across xpcs_create_mdiodev() and
device_link_add() is also unusual - device_link_add() does its own
locking and the driver core normally expects supplier device locks not
to be held by consumers.

What race is the lock actually protecting against?  pxpcs and
pxpcs->bus are published by xpcs_rk_probe() via dev_set_drvdata() and
xpcs_rk_init_bus(), and the !pxpcs || !pxpcs->bus case already returns
-EPROBE_DEFER, so the consumer retries later; the device link then
keeps the supplier around.  Can the device_lock()/device_unlock() pairs
simply be dropped, or is something else needed here (e.g. checking that
the supplier is bound) to close the window you had in mind?

Agreed. I'll drop device_lock() and establish the device link first,
then read platform_get_drvdata(). DL_FLAG_MANAGED keeps the supplier
bound while the consumer is, so no extra locking is needed. Failure
paths will call device_link_remove() explicitly, since
DL_FLAG_AUTOREMOVE_CONSUMER only removes the link when the consumer
device is released.

[Severity: High]
This block in xpcs_rk_mdio_write_remapping() ignores addr entirely:
any write to DW_VR_MII_AN_CTRL, DW_VR_MII_AN_INTR_STS,
DW_VR_MII_EEE_MCTRL0, DW_VR_MII_EEE_MCTRL1 or DW_VR_MII_DIG_CTRL2 is
routed to ROCKCHIP_MMD_MII, which per the commit message is port 0's
MMD (7), regardless of which of the four ports the MDIO address
selects.

So when the core configures a PCS instance created with addr 1, 2 or 3
- xpcs_config_aneg_c37_sgmii() writing DW_VR_MII_AN_CTRL and
DW_VR_MII_DIG_CTRL2, xpcs_config_eee() writing DW_VR_MII_EEE_MCTRL0/1,
and the interrupt-status clear on DW_VR_MII_AN_INTR_STS - the write
lands on port 0 instead.  That has two effects: the intended port is
never configured (so its link stays broken), and port 0's
auto-negotiation/EEE state is silently clobbered by an unrelated
interface, including its pending AN interrupt status.

The read path is not symmetric either: xpcs_rk_mdio_read_remapping()
remaps per addr with the comment "read remapping to MII is performed by
HW", so a write to these registers on port 2 followed by a read back
would not return what was written.  Is that asymmetry deliberate?

Does the TRM say these vendor registers physically exist only in MMD 7
and are shared by all four MII ports?  If so, redirecting writes
silently still leaves per-port configuration impossible, and the driver
probably needs to say so explicitly (reject the access, or document how
the shared block is meant to be programmed) rather than aliasing ports
1..3 onto port 0.  If instead each port has its own copy of these
registers, this switch should be dropped so the addr-based mapping
below applies.  Which is it?

ROCKCHIP_MMD_MII is a management port. The AN_CTRL / AN_INTR_STS /
EEE_MCTRL0/1 / DIG_CTRL2 bits are read-only mirrors on ports 1-3, so
writes can only land in MMD 7. The read path is redirected by hardware
to the port's mirror, which is why the write path is asymmetric.

On RK3568 there is only one SGMII link, so only one port is active at
a time and the aliasing is harmless. QSGMII is not supported by this
driver.