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

Jakub Kicinski [off-list ref] 于2026年9月13日周日 04:13写道：

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: pcs: xpcs: add Rockchip RK3568 platform glue driver

This adds a glue driver for the Synopsys DesignWare XPCS found on RK3568,
which is reached through APB3 memory-mapped registers instead of a real
MDIO bus.  The driver registers a virtual MDIO bus that remaps the four
MII ports onto their respective MMDs, and exports xpcs_rk_create() so a
MAC driver can obtain a dw_xpcs instance.

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

A few questions below.

quoted

diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
index e417fd66f660a..88bc40791e69f 100644
--- a/drivers/net/pcs/Kconfig
+++ b/drivers/net/pcs/Kconfig

@@ -12,6 +12,29 @@ 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.

[Severity: Medium]
Should this new user-visible symbol be part of a patch whose changelog only
describes adding Rockchip RK3568 support?

Two side effects of introducing PCS_XPCS_PLATFORM are not mentioned
anywhere in the commit message:

  - the "snps,dw-xpcs" platform glue moves out of pcs_xpcs.ko into a new
    pcs-xpcs-plat.ko, which changes module packaging for every existing
    memory-mapped DW XPCS user
  - the glue becomes optional, so a config with PCS_XPCS=y and
    PCS_XPCS_PLATFORM=n silently loses "snps,dw-xpcs" support

Since this restructuring of shared code is independent of the Rockchip
glue, could it be split into its own preparatory patch with the effects
described?

You're right that this restructuring is independent of the Rockchip
glue, but the "silently loses snps,dw-xpcs support" concern is not
accurate: PCS_XPCS_PLATFORM has `default PCS_XPCS`, so when PCS_XPCS
is enabled, the platform glue is also enabled by default. A user would
have to explicitly set CONFIG_PCS_XPCS_PLATFORM=n to lose it.

The module packaging change (snps,dw-xpcs moving from pcs_xpcs.ko to
pcs-xpcs-plat.ko) is real, though. I'll mention it in the changelog.

I'll keep the Kconfig change in this patch since it is small and
directly related to making the Rockchip glue a separate module, but
I'll describe the module packaging side effect explicitly.

quoted

+
+config PCS_XPCS_ROCKCHIP
+     tristate "Rockchip XPCS controller support"
+     default ARCH_ROCKCHIP
+     depends on OF && (ARCH_ROCKCHIP || COMPILE_TEST)

[Severity: Medium]
Do the dependencies here need GENERIC_PHY and PM_GENERIC_DOMAINS as well?

xpcs_rk_probe() cannot make forward progress without either of them.  With
CONFIG_GENERIC_PHY=n, the first probe step hits the stub:

include/linux/phy/phy.h:
        static inline struct phy *devm_phy_get(struct device *dev,
                                               const char *string)
        {
                return ERR_PTR(-ENOSYS);
        }

so xpcs_rk_serdes_phy_init() always fails through dev_err_probe().

With CONFIG_PM_GENERIC_DOMAINS=n (which covers every CONFIG_PM=n build),
xpcs_rk_serdes_phy_poweron() hits:

include/linux/pm_domain.h:
        static inline int dev_pm_genpd_rpm_always_on(struct device *dev, bool on)
        {
                return -EOPNOTSUPP;
        }

and the driver treats that as fatal.  Since the symbol is also
default ARCH_ROCKCHIP, it can be built and matched in configurations where
it can never bind.  Would adding depends on GENERIC_PHY and
PM_GENERIC_DOMAINS be appropriate?

Sure, I will add it.

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]
Is the mandatory "serdes" PHY consistent with the device tree added by the
rest of the series?

The binding added in this series lists phys and phy-names under required:

Documentation/devicetree/bindings/net/pcs/rockchip-dwxpcs.yaml:
        required:
          - compatible
          - reg
          ...
          - phys
          - phy-names
          - power-domains

but the SoC node has neither property:

arch/arm64/boot/dts/rockchip/rk3568.dtsi:
        xpcs: 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";

dtschema validates nodes regardless of status, so every RK3568 DTB other
than the one board patched later in the series should report that phys is
a required property.  Any board that enables the node without adding
phys/phy-names also gets an unconditional probe failure here, since there
is no devm_phy_get_optional() path.  Should the PHY properties move to the
SoC dtsi, or should they be dropped from required:?

As replied earlier, the phys/phy-names properties cannot move into
rk3568.dtsi because the SerDes connection is board-specific. Boards that
enable XPCS must provide them.

quoted

+static int xpcs_rk_init_clk(struct dw_xpcs_rk *pxpcs)
+{
+     struct device *dev = &pxpcs->pdev->dev;
+     int ret;
+
+     pxpcs->csr_clk = devm_clk_get(dev, "csr");
+     if (IS_ERR(pxpcs->csr_clk))
+             return dev_err_probe(dev, PTR_ERR(pxpcs->csr_clk),
+                                      "Failed to get CSR clock\n");
+
+     pxpcs->eee_clk = devm_clk_get(dev, "eee");
+     if (IS_ERR(pxpcs->eee_clk))
+             return dev_err_probe(dev, PTR_ERR(pxpcs->eee_clk),
+                                      "Failed to get EEE clock\n");
+
+     ret = clk_prepare_enable(pxpcs->eee_clk);
+     if (ret) {
+             dev_err(dev, "Failed to enable EEE clock\n");
+             return ret;
+     }

[Severity: Low]
The EEE clock is acquired and permanently enabled, but nothing ever
derives the XPCS 100 ns multiplier from its rate.  Should this driver call
xpcs_config_eee_mult_fact() the way dwmac-intel.c does?

Without it xpcs->eee_mult_fact stays 0, and the core programs that value
verbatim when EEE is turned on:

drivers/net/pcs/pcs-xpcs.c:
        /* Configure the eee_mult_fact so that
         * clk_eee_time_period * (mult_fact + 1) is within the range
         * 80 to 120ns.
         */
        val = ... FIELD_PREP(DW_VR_MII_EEE_MULT_FACT_100NS,
                             xpcs->eee_mult_fact);

With the RK3568 EEE clock parents (125/200 MHz, so a 5-8 ns period), a
multiplier of 0 puts the LPI timing well below that range.  The register
reset value is also 0, so nothing valid is being overwritten, but is the
timing expected to be programmed here?

mainline's clk-rk3568.c registers CLK_XPCS_EEE with a parent mux
between gpll200 (200 MHz) and cpll125 (125 MHz), controlled by
CRU_CLKSEL_CON29 bit 13. The reset value is 0 (gpll200, 200 MHz), and
no in-tree DTS changes it, so the default multiplier is 19.

To stay correct if a board changes the mux, I'll derive the value at
runtime in xpcs_rk_init_clk() where the EEE clock is already obtained:

    pxpcs->eee_mult_fact =
        DIV_ROUND_CLOSEST(100 * clk_get_rate(pxpcs->eee_clk),
                          1000000000UL) - 1;

and call xpcs_config_eee_mult_fact() in xpcs_rk_create() right after
xpcs_create_mdiodev() succeeds, since that is where the dw_xpcs
pointer becomes available.

This yields 19 for 200 MHz (100 ns) and 12 for 125 MHz (104 ns), both
within the required 80..120 ns range.

quoted

+struct dw_xpcs *xpcs_rk_create(struct device *dev, struct device_node *np)
+{

[ ... ]

quoted

+     pdev = of_find_device_by_node(pcs_np);
+     of_node_put(pcs_np);
+     if (!pdev)
+             return ERR_PTR(-EPROBE_DEFER);
+
+     device_lock(&pdev->dev);
+     pxpcs = platform_get_drvdata(pdev);
+     if (!pxpcs || !pxpcs->bus) {
+             device_unlock(&pdev->dev);
+             put_device(&pdev->dev);
+             return ERR_PTR(-EPROBE_DEFER);
+     }
+
+     xpcs = xpcs_create_mdiodev(pxpcs->bus, (int)port);
+     device_unlock(&pdev->dev);
+     if (IS_ERR(xpcs)) {
+             put_device(&pdev->dev);
+             return xpcs;
+     }
+
+     if (!device_link_add(dev, &pdev->dev, DL_FLAG_AUTOREMOVE_CONSUMER)) {

[Severity: Medium]
Can the mii_bus be freed under the new dw_xpcs in the window between
device_unlock() and device_link_add()?

The bus is devm managed, so it goes away when the XPCS platform driver
unbinds:

xpcs_rk_init_bus()
        bus = devm_mdiobus_alloc_size(dev, 0);
        ...
        ret = devm_mdiobus_register(dev, bus);

and xpcs_create_mdiodev() takes no reference on it:

drivers/net/phy/mdio_device.c:mdio_device_create()
        mdiodev->bus = bus;
        mdiodev->addr = addr;

so the managed device link is the only thing keeping the supplier binding
alive.  With the lock already dropped, an unbind through sysfs or an
rmmod of pcs-xpcs-rk can run device_release_driver() before the link
exists.  put_device() keeps the struct device around but not the driver
binding or its devm resources, leaving mdiodev->bus and the ioremapped
window pointing at freed memory.  The xpcs_destroy() call on the
device_link_add() failure path would then touch it too.

Would creating the device link while still holding device_lock(), or
otherwise pinning the supplier before unlocking, close this?

I've addressed this by keeping device_link_add() inside the
device_lock() critical section. device_link_add() uses global locks
(device_links_lock and dpm_list_mtx), not the supplier's device_lock,
so calling it while holding the lock is safe.

quoted

+EXPORT_SYMBOL_GPL(xpcs_rk_create);

[Severity: Low]
As of this commit alone, xpcs_rk_create() has no in-tree caller, so the
driver only probes, pins the power domain and SerDes, and registers the
virtual MDIO bus without ever producing the phylink_pcs the changelog
describes.

This resolves later in the same series: rk_pcs_init() in
drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c calls
xpcs_rk_create(priv->device, pcs_node), rk_gmac_probe() installs
pcs_init/pcs_exit/select_pcs, and DWMAC_ROCKCHIP gains
select PCS_XPCS_ROCKCHIP.  Noting it only in case the ordering was
intentional.

This ordering is intentional. Patch 7 adds the XPCS glue driver
(provider), and the consumer (rk_pcs_init() in dwmac-rk) is added in
the next patch. This is the standard way to split provider and consumer
patches: each commit compiles on its own, but the full functionality
only comes together at the end of the series.

The exported symbol has no caller until the next patch, which is
expected for this kind of split.

Thanks,
Coia