Re: [PATCH net-next v12 3/6] net: pcs: xpcs: add Rockchip RK3568 platform glue driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] net: pcs: xpcs: xpcs_rk_system_suspend() relies on unset active_wakeup
--

commit 51040950622ac8bc620e18e37d9000b98191388d
Author: Coia Prant <coiaprant@gmail.com>

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

This commit adds a platform glue driver for the Synopsys DesignWare XPCS
on Rockchip RK3568 SoCs, creating a virtual MDIO bus over APB3 registers
and managing the SerDes PHY, clocks, and power domain.

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

> diff --git a/drivers/net/pcs/pcs-xpcs-rk.c b/drivers/net/pcs/pcs-xpcs-rk.c
> new file mode 100644
> index 0000000000000..698a43b753341
> --- /dev/null
> +++ b/drivers/net/pcs/pcs-xpcs-rk.c
> @@ -0,0 +1,598 @@

[ ... ]

> +static int xpcs_rk_system_suspend(struct device *dev)
> +{
> +	/*
> +	 * Keep the PD_PIPE power domain on during system suspend.
> +	 *
> +	 * PD_PIPE is shared with SATA/PCIe and would be powered down by
> +	 * genpd once all its consumers are suspended, killing the SerDes
> +	 * and breaking MAC WoL.  Mark the XPCS as part of the wakeup path
> +	 * so genpd keeps the domain on.  Unconditional because the XPCS
> +	 * core has no callback to convey the MAC WoL state.
> +	 */
> +	device_set_wakeup_path(dev);
> +	return 0;
> +}
> +
> +static int xpcs_rk_system_resume(struct device *dev)
> +{
> +	return 0;
> +}

[Severity: Medium]
Does calling device_set_wakeup_path(dev) in xpcs_rk_system_suspend()
actually keep the RK3568_PD_PIPE power domain powered on during system
suspend?

The commit message notes that genpd leaves the domain powered because the
Rockchip power domain driver sets GENPD_FLAG_ACTIVE_WAKEUP on PD_PIPE.
However, in drivers/pmdomain/rockchip/pm-domains.c, rk3568_pm_domains
defines RK3568_PD_PIPE with wakeup set to false:

drivers/pmdomain/rockchip/pm-domains.c:
	[RK3568_PD_PIPE]	= DOMAIN_RK3568("pipe", BIT(8), BIT(11), false, false),

In rockchip_pm_add_one_domain(), GENPD_FLAG_ACTIVE_WAKEUP is only set when
pd_info->active_wakeup is true:

	pd->genpd.flags = GENPD_FLAG_PM_CLK | GENPD_FLAG_NO_STAY_ON;
	if (pd_info->active_wakeup)
		pd->genpd.flags |= GENPD_FLAG_ACTIVE_WAKEUP;

When the system suspends via genpd_suspend_noirq() -> genpd_finish_suspend()
in drivers/pmdomain/core.c, genpd checks:

	if (device_awake_path(dev) && genpd_is_active_wakeup(genpd) &&
	    !device_out_band_wakeup(dev))
		return 0;

Because genpd_is_active_wakeup(genpd) is false for RK3568_PD_PIPE, genpd
ignores device_awake_path(dev) and powers off the domain once all consumers
are suspended. This powers down the shared SerDes PHY and XPCS controller,
breaking MAC Wake-on-LAN and losing hardware state across suspend and
resume since xpcs_rk_system_resume() is a no-op.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017054406.1828061-1-coiaprant@gmail.com?part=3