Re: [PATCH v5 8/8] net: stmmac: qcom-ethqos: add support for sa8255p

[Simon Horman <horms@kernel.org>]

On Fri, Nov 07, 2025 at 11:29:58AM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

...

> +static int qcom_ethqos_pd_init(struct platform_device *pdev, 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;
> +}
> +
> +static void qcom_ethqos_pd_exit(struct platform_device *pdev, void *data)
> +{
> +	struct qcom_ethqos *ethqos = data;
> +
> +	qcom_ethqos_domain_off(ethqos, ETHQOS_PD_MDIO);
> +	qcom_ethqos_domain_off(ethqos, ETHQOS_PD_CORE);
> +}
> +
>  static void ethqos_ptp_clk_freq_config(struct stmmac_priv *priv)
>  {
>  	struct plat_stmmacenet_data *plat_dat = priv->plat;

...

> @@ -852,28 +993,63 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>  	ethqos->rgmii_config_loopback_en = drv_data->rgmii_config_loopback_en;
>  	ethqos->has_emac_ge_3 = drv_data->has_emac_ge_3;
>  	ethqos->needs_sgmii_loopback = drv_data->needs_sgmii_loopback;
> -
> -	ethqos->pm.link_clk = devm_clk_get(dev, clk_name);
> -	if (IS_ERR(ethqos->pm.link_clk))
> -		return dev_err_probe(dev, PTR_ERR(ethqos->pm.link_clk),
> -				     "Failed to get link_clk\n");
> -
> -	ret = ethqos_clks_config(ethqos, true);
> -	if (ret)
> -		return ret;
> -
> -	ret = devm_add_action_or_reset(dev, ethqos_clks_disable, ethqos);
> -	if (ret)
> -		return ret;
> -
> -	ethqos->pm.serdes_phy = devm_phy_optional_get(dev, "serdes");
> -	if (IS_ERR(ethqos->pm.serdes_phy))
> -		return dev_err_probe(dev, PTR_ERR(ethqos->pm.serdes_phy),
> -				     "Failed to get serdes phy\n");
> -
> -	ethqos->set_serdes_speed = ethqos_set_serdes_speed_phy;
>  	ethqos->serdes_speed = SPEED_1000;
> -	ethqos_update_link_clk(ethqos, SPEED_1000);
> +
> +	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;

Hi Bartosz,

It seems that the intention of this is to ensure
that domains turned on by qcom_ethqos_pd_init()
are turned off again on exit or clean-up in error paths.

> +		plat_dat->init = qcom_ethqos_pd_init;
> +		plat_dat->clk_ptp_rate = pm_data->clk_ptp_rate;
> +
> +		ret = qcom_ethqos_pd_init(pdev, ethqos);
> +		if (ret)
> +			return ret;

And here those domains are turned on.

> +
> +		ret = qcom_ethqos_domain_on(ethqos, ETHQOS_PD_SERDES);
> +		if (ret)

But it seems that if we reach this error path then the cleanup is not
performed. This is because plat_dat and thus it's exit callback are
registered until the call to devm_stmmac_pltfr_probe() towards the end of
this function.

Sorry if I'm on the wrong track here. I did dig into it.
But this was flagged by Claude Code running
https://github.com/masoncl/review-prompts/

> +			return dev_err_probe(dev, ret,
> +					     "Failed to enable the serdes power domain\n");

...