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

[Jakub Kicinski <kuba@kernel.org>]

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.

> 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?

> +
> +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?

> +	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
>  obj-$(CONFIG_PCS_LYNX)		+= pcs-lynx.o
>  obj-$(CONFIG_PCS_MTK_LYNXI)	+= pcs-mtk-lynxi.o
>  obj-$(CONFIG_PCS_RZN1_MIIC)	+= pcs-rzn1-miic.o
> diff --git a/drivers/net/pcs/pcs-xpcs-rk.c b/drivers/net/pcs/pcs-xpcs-rk.c
> new file mode 100644
> index 0000000000000..68bcaa1a5cbbf
> --- /dev/null
> +++ b/drivers/net/pcs/pcs-xpcs-rk.c
> @@ -0,0 +1,543 @@

[ ... ]

> +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:?

[ ... ]

> +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?

[ ... ]

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

[ ... ]

> +	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?

[ ... ]

> +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.

[ ... ]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip