Re: [RESEND,net-next,v6,7/7] net: stmmac: qcom-ethqos: add support for sa8255p

[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 pausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: stmmac: qcom-ethqos: add support for sa8255p

This commit adds support for the SA8255P SoC which uses power domains
managed via SCMI firmware, rather than the traditional clock/phy-based
power management used by other variants.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index 8ba57bba3f2e..54f8ef3cfd7d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c

[ ... ]

> +static void ethqos_disable_serdes(void *data)
> +{
> +	struct qcom_ethqos *ethqos = data;
> +
> +	qcom_ethqos_domain_on(ethqos, ETHQOS_PD_SERDES);
                          ^^

Should this be qcom_ethqos_domain_off() instead of qcom_ethqos_domain_on()?

This function is registered as a cleanup action via devm_add_action_or_reset()
to undo the qcom_ethqos_domain_on(ETHQOS_PD_SERDES) call during probe. Using
domain_on here would increment the PM runtime usage counter again rather than
decrementing it. The serdes power domain would never be properly turned off
on driver removal.

Compare with qcom_ethqos_pd_serdes_powerdown() which correctly uses
qcom_ethqos_domain_off().

> +}

[ ... ]

> +static int qcom_ethqos_pd_init(struct device *dev, void *priv)
> +{
> +	struct qcom_ethqos *ethqos = priv;
> +	int ret;
> +
> +	/*
> +	 * Enable functional clock to prevent DMA reset after timeout due
> +	 * to no PHY clock being enabled after the hardware block has been
> +	 * power cycled. The actual configuration will be adjusted once
> +	 * ethqos_fix_mac_speed() is called.
> +	 */
> +	ethqos_set_func_clk_en(ethqos);
> +
> +	ret = qcom_ethqos_domain_on(ethqos, ETHQOS_PD_CORE);
> +	if (ret)
> +		return ret;
> +
> +	ret = qcom_ethqos_domain_on(ethqos, ETHQOS_PD_MDIO);
> +	if (ret) {
> +		qcom_ethqos_domain_off(ethqos, ETHQOS_PD_CORE);
> +		return ret;
> +	}
> +
> +	return 0;
> +}

[ ... ]

>  	if (pm_data && pm_data->use_domains) {
>  		ethqos->set_serdes_speed = ethqos_set_serdes_speed_pd;
>
> +		ret = devm_pm_domain_attach_list(dev, &pm_data->pd,
> +						 &ethqos->pd.pd_list);
> +		if (ret < 0)
> +			return dev_err_probe(dev, ret, "Failed to attach power domains\n");
> +
> +		plat_dat->clks_config = ethqos_pd_clks_config;
> +		plat_dat->serdes_powerup = qcom_ethqos_pd_serdes_powerup;
> +		plat_dat->serdes_powerdown = qcom_ethqos_pd_serdes_powerdown;
> +		plat_dat->exit = qcom_ethqos_pd_exit;
> +		plat_dat->init = qcom_ethqos_pd_init;
                             ^^^^^^^^^^^^^^^^^^^^^^^

> +		plat_dat->clk_ptp_rate = pm_data->clk_ptp_rate;
> +
> +		ret = qcom_ethqos_pd_init(dev, ethqos);
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Is qcom_ethqos_pd_init() being called twice?

It is assigned to plat_dat->init above, and then called directly here. Later,
devm_stmmac_pltfr_probe() -> stmmac_pltfr_probe() -> stmmac_dvr_probe() will
call plat_dat->init() again (see stmmac_main.c stmmac_dvr_probe function).

Each call to qcom_ethqos_pd_init() increments the PM runtime usage counter
for ETHQOS_PD_CORE and ETHQOS_PD_MDIO via pm_runtime_resume_and_get(). With
two calls, the refcount would be 2 for each domain, but qcom_ethqos_pd_exit()
only decrements once per domain. This would leave the power domains with a
dangling reference.

Perhaps the direct call to qcom_ethqos_pd_init() here should be removed, and
let stmmac_dvr_probe() invoke it through the plat_dat->init callback?

> +		if (ret)
> +			return ret;

[ ... ]