[PATCH net-next v15 9/9] net: stmmac: qcom-ethqos: add support for sa8255p

[netdev-bot+sashiko at kernel.org]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] With Wake-on-LAN armed, system suspend on sa8255p powers off
  the MAC's SCMI "core"/"mdio" domains, and on resume the EMAC wrapper…
- [Medium] The SCMI variant has no CSR/application clock, and nothing
  substitutes a rate for it.
- [Low] The new sa8255p (SCMI) variant accepts RGMII phy-modes but
  silently misconfigures them.
- [Low] The new comment in ethqos_scmi_setup() and the commit message
  misdescribe what the code does.

--

net: stmmac: qcom-ethqos: add support for sa8255p

This adds a new sa8255p variant to the qcom-ethqos glue driver. On this
variant, firmware handles power management over SCMI, and Linux models it
with power domains. It adds a new emac data variant with a fixed PTP clock
rate, plus a separate ethqos_scmi_setup() callback.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index 509a5368e69a8..3d4bc950a72ac 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c

[ ... ]

> @@ -756,6 +788,80 @@ static int ethqos_hlos_setup(struct qcom_ethqos *ethqos,
>  	return 0;
>  }
>  
> +static const char *const ethqos_scmi_pd_names[] = { "core", "mdio" };
> +
> +static int ethqos_scmi_setup(struct qcom_ethqos *ethqos,
> +			     struct plat_stmmacenet_data *plat_dat)
> +{
> +	const struct dev_pm_domain_attach_data pd_data = {
> +		.pd_names	= ethqos_scmi_pd_names,
> +		.num_pd_names	= ARRAY_SIZE(ethqos_scmi_pd_names),
> +		.pd_flags	= PD_FLAG_DEV_LINK_ON,
> +	};
> +
> +	struct platform_device *pdev = ethqos->pdev;
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	ret = devm_pm_domain_attach_list(dev, &pd_data, &ethqos->pds);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to attach power domains\n");

[Severity: Medium]
What happens to these two domains during system suspend when Wake-on-LAN
is armed?

devm_pm_domain_attach_list() creates virtual genpd devices through
genpd_dev_pm_attach_by_id(). Those devices have no parent:

drivers/pmdomain/core.c:genpd_dev_pm_attach_by_id() {
    ...
	dev_set_name(virt_dev, "genpd:%u:%s", index, dev_name(dev));
	virt_dev->bus = &genpd_bus_type;
	virt_dev->release = genpd_release_dev;
	virt_dev->of_node = of_node_get(dev->of_node);
    ...
}

dpm_propagate_wakeup_to_parent() only passes wakeup_path to dev->parent.
It does not reach device-link suppliers, so device_awake_path(virt_dev)
stays false.

SCMI domains set GENPD_FLAG_ACTIVE_WAKEUP. Even so, genpd_finish_suspend()
falls through to genpd_sync_power_off():

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

qcom_ethqos_probe() also sets plat_dat->pmt = true for this variant, so
WoL can be enabled with ethtool. With priv->wolopts set,
stmmac_pltfr_noirq_suspend() and stmmac_pltfr_noirq_resume() skip
pm_runtime_force_suspend() and pm_runtime_force_resume():

	if (!priv->wolopts) {
		/* enable the clk previously disabled */
		ret = pm_runtime_force_resume(dev);

As a result, ethqos_clks_config(true) does not run on resume.

If firmware really power-cycles the MAC and wrapper when the "core" and
"mdio" domains are turned off, can WoL fail to wake the system? Would the
SGMII loopback and FUNC_CLK_EN bits at rgmii_base also be lost, so that
stmmac_resume()->stmmac_hw_setup() hits the DMA SW-reset timeout that the
comment below describes?

On the HLOS variants, the single GDSC is the MAC's own pm_domain, so the
awake-path check applies to the MAC device itself. This looks specific to
the multi-domain attach used here. The kernel tree can't show whether
firmware actually removes power on that request.

> +
> +	/*
> +	 * The SerDes lane, its clocks and the MAC AXI/AHB clocks are owned by
> +	 * firmware and brought up through the SCMI power domains above. The
> +	 * MAC wrapper itself, however is in the kernel's register space: the
> +	 * wrapper bit that loops the PHY TX clock into the MAC's clk_rx_i -
> +	 * needed because no recovered RX clock exists yet - is not
> +	 * configured by firmware. Without it, clk_rx_i never toggles and the
> +	 * DMA SW-reset polled in dwmac4_dma_reset() never completes.

[Severity: Low]
Is this comment right about where the SerDes power comes from?

The sa8255p SerDes PHY binding (qcom,sa8255p-dwmac-sgmii-phy.yaml)
requires a power-domain of its own. phy-qcom-sgmii-eth-scmi.c votes for
it through the PHY device's runtime PM (devm_pm_runtime_enable() before
devm_phy_create()), and set_mode() sets its performance state.

qcom_ethqos_probe() still gets the "serdes" phy and installs
qcom_ethqos_serdes_powerup(). The PHY TX clock that is looped into
clk_rx_i therefore comes from serdes_powerup() in stmmac_open(), before
__stmmac_open()->stmmac_hw_setup(). It does not come from the "core" and
"mdio" domains attached above.

The current ordering works. Could the comment name the SerDes PHY's own
domain instead, so later changes to the SerDes or PM ordering are not
misled?

The commit message also says only:

  Unlike the previously supported variants, this one's power management
  is done in the firmware over SCMI. This is modeled in linux using power
  domains so add a new emac data variant and a separate setup callback.

Could it also mention the following?

  - The kernel still maps and programs the "rgmii" wrapper registers
    (SGMII loopback and FUNC_CLK_EN) at probe and on every runtime resume.
  - set_clk_tx_rate and dump_debug_regs are dropped for this variant.
  - A fixed 230.4 MHz PTP reference rate is hardcoded in
    emac_v4_0_0_scmi_data.

> +	 *
> +	 * Map the wrapper and program the same loopback/functional clock bits
> +	 * the non-firmware platforms rely on (see ethqos_clks_config) so the
> +	 * RX clock is present by the time the DMA engine is reset.
> +	 */

[ ... ]

> +	ret = devm_add_action_or_reset(dev, ethqos_clks_disable, ethqos);
> +	if (ret)
> +		return ret;
> +
> +	plat_dat->clk_ptp_rate = ethqos->data->ptp_clk_rate;

[Severity: Medium]
The PTP rate gets a fixed value here, but what about the CSR clock?

The sa8255p binding has no clocks property. stmmac_probe_config_dt()
therefore fails devm_clk_get("stmmaceth") and leaves plat->stmmac_clk
NULL:

	plat->stmmac_clk = devm_clk_get(&pdev->dev,
					STMMAC_RESOURCE_NAME);
	if (IS_ERR(plat->stmmac_clk)) {
		dev_warn(&pdev->dev, "Cannot get CSR clock\n");
		plat->stmmac_clk = NULL;

dwmac4_core_init() then runs this on every stmmac_hw_setup():

	clk_rate = clk_get_rate(priv->plat->stmmac_clk);
	writel((clk_rate / 1000000) - 1, ioaddr + GMAC4_MAC_ONEUS_TIC_COUNTER);

With clk_rate == 0, won't this write 0xFFFFFFFF into
GMAC4_MAC_ONEUS_TIC_COUNTER? That would break the LPI/EEE timers, which
count in microseconds.

plat_dat->clk_ref_rate is not set either, so stmmac_usec2riwt() and
stmmac_riwt2usec() return 0:

	unsigned long clk = clk_get_rate(priv->plat->stmmac_clk);

	if (!clk) {
		clk = priv->plat->clk_ref_rate;
		if (!clk)
			return 0;

Would ethtool -C rx-usecs then fail the MIN_DMA_RIWT check, and would
rx-usecs be reported as 0?

> +
> +	switch (ethqos->phy_mode) {
> +	case PHY_INTERFACE_MODE_2500BASEX:
> +	case PHY_INTERFACE_MODE_SGMII:
> +		plat_dat->fix_mac_speed = ethqos_fix_mac_speed_sgmii;
> +		break;
> +	default:
> +		break;
> +	}

[Severity: Low]
What happens here if the device tree sets an RGMII phy-mode on a
qcom,sa8255p-ethqos node?

qcom_ethqos_probe() accepts PHY_INTERFACE_MODE_RGMII, _ID, _RXID and _TXID
for every compatible, and the sa8255p binding does not constrain
phy-mode. For RGMII, this switch takes the default branch, so none of the
following happens:

  - no fix_mac_speed is installed
  - no set_clk_tx_rate is installed
  - no RGMII IO macro or DLL programming is done (emac_v4_0_0_scmi_data
    has no rgmii_por)

qcom_ethqos_set_sgmii_loopback() also returns early for non-SGMII modes.

The SCMI SerDes provider rejects RGMII as well:

drivers/phy/qualcomm/phy-qcom-sgmii-eth-scmi.c:qcom_dwmac_sgmii_phy_scmi_validate() {
	switch (submode) {
	case PHY_INTERFACE_MODE_SGMII:
	case PHY_INTERFACE_MODE_1000BASEX:
	case PHY_INTERFACE_MODE_2500BASEX:
		return 0;
	default:
		return -EINVAL;
    ...
}

With a phys property, qcom_ethqos_serdes_powerup()->phy_set_mode_ext()
fails at open time. Without one, the MAC runs in RGMII mode with no
wrapper configuration. In both cases probe succeeds, but the interface
never works.

Would it be better for ethqos_scmi_setup() to return -EINVAL for modes
that don't use the SerDes, or for the binding to limit phy-mode to an
enum, or both?

> +
> +	return 0;
> +}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260923-qcom-sa8255p-emac-v15-0-e82f33720737%40oss.qualcomm.com