[PATCHv3] mmc: sdhci-pxav3: use managed clock enablement

[PATCHv3] mmc: sdhci-pxav3: use managed clock enablement

[Rosen Penev]

Replace devm_clk_get() plus clk_prepare_enable() with the managed
variants devm_clk_get_enabled() and devm_clk_get_optional_enabled().
The core clock becomes optional, which removes the IS_ERR() guards
in runtime suspend/resume and lets devms unwind the clocks, so the
manual clk_disable_unprepare() calls in the probe error paths and
remove are dropped. The NULL version of clk_io was removed as all dts
files that use sdhci-pxav3 specify io as a clock name. Strictly speaking
that could break pdata users but all of that is slated for removal.

clk_prepare_enable() and friends are NULL safe, so no need to check if
the optional clock is present.

Handle failures of clk_prepare_enable() in runtime resume instead
of ignoring them: a failed enable would leave controller register
accesses hitting unclocked hardware. Bail out before resuming the host
and unwind clk_io when enabling clk_core fails. This also drops the
now unused pltfm_host variable from remove().

Handle result of the forced resumes in remove(), suspend() and resume().
Use pm_runtime_resume_and_get(), which does not bump the usage counter on
error.

Assisted-by: LLM
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 v3: fix sashiko errors.
 v2: fix resume paths.
 drivers/mmc/host/sdhci-pxav3.c | 91 ++++++++++++++++++++++------------
 1 file changed, 59 insertions(+), 32 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
index f39764ffd4a0..fda1eafe5685 100644
--- a/drivers/mmc/host/sdhci-pxav3.c
+++ b/drivers/mmc/host/sdhci-pxav3.c
@@ -413,19 +413,16 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
 	pltfm_host = sdhci_priv(host);
 	pxa = sdhci_pltfm_priv(pltfm_host);
 
-	pxa->clk_io = devm_clk_get(dev, "io");
+	pxa->clk_io = devm_clk_get_optional_enabled(dev, "io");
+	if (!pxa->clk_io)
+		pxa->clk_io = devm_clk_get_enabled(dev, NULL);
 	if (IS_ERR(pxa->clk_io))
-		pxa->clk_io = devm_clk_get(dev, NULL);
-	if (IS_ERR(pxa->clk_io)) {
-		dev_err(dev, "failed to get io clock\n");
-		return PTR_ERR(pxa->clk_io);
-	}
+		return dev_err_probe(dev, PTR_ERR(pxa->clk_io), "failed to get io clock\n");
 	pltfm_host->clk = pxa->clk_io;
-	clk_prepare_enable(pxa->clk_io);
 
-	pxa->clk_core = devm_clk_get(dev, "core");
-	if (!IS_ERR(pxa->clk_core))
-		clk_prepare_enable(pxa->clk_core);
+	pxa->clk_core = devm_clk_get_optional_enabled(dev, "core");
+	if (IS_ERR(pxa->clk_core))
+		return PTR_ERR(pxa->clk_core);
 
 	host->mmc->caps |= MMC_CAP_NEED_RSP_BUSY;
 	/* enable 1/8V DDR capable */
@@ -434,17 +431,17 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
 	if (device_is_compatible(dev, "marvell,armada-380-sdhci")) {
 		ret = armada_38x_quirks(pdev, host);
 		if (ret < 0)
-			goto err_mbus_win;
+			return ret;
 		ret = mv_conf_mbus_windows(pdev, mv_mbus_dram_info());
 		if (ret < 0)
-			goto err_mbus_win;
+			return ret;
 	}
 
 	match = of_match_device(of_match_ptr(sdhci_pxav3_of_match), &pdev->dev);
 	if (match) {
 		ret = mmc_of_parse(host->mmc);
 		if (ret)
-			goto err_of_parse;
+			return ret;
 		sdhci_get_of_property(pdev);
 		pdata = pxav3_get_mmc_pdata(dev);
 		pdev->dev.platform_data = pdata;
@@ -499,35 +496,56 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
 err_add_host:
 	pm_runtime_disable(&pdev->dev);
 	pm_runtime_put_noidle(&pdev->dev);
-err_of_parse:
-err_mbus_win:
-	clk_disable_unprepare(pxa->clk_io);
-	clk_disable_unprepare(pxa->clk_core);
 	return ret;
 }
 
 static void sdhci_pxav3_remove(struct platform_device *pdev)
 {
 	struct sdhci_host *host = platform_get_drvdata(pdev);
+	struct device *dev = &pdev->dev;
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct sdhci_pxa *pxa = sdhci_pltfm_priv(pltfm_host);
+	int ret;
 
-	pm_runtime_get_sync(&pdev->dev);
-	pm_runtime_disable(&pdev->dev);
-	pm_runtime_put_noidle(&pdev->dev);
+	ret = pm_runtime_resume_and_get(dev);
+	pm_runtime_disable(dev);
+	if (ret) {
+		/*
+		 * Runtime resume failed, so the controller may still be
+		 * unclocked.  Restore the clocks so the register accesses in
+		 * sdhci_remove_host() do not hit unclocked hardware.
+		 */
+		dev_warn(dev, "failed to resume controller, %pe\n",
+			 ERR_PTR(ret));
+		ret = clk_prepare_enable(pxa->clk_io);
+		if (ret) {
+			dev_warn(dev, "failed to re-enable io clock, %pe\n",
+				 ERR_PTR(ret));
+			return;
+		}
+		ret = clk_prepare_enable(pxa->clk_core);
+		if (ret) {
+			clk_disable_unprepare(pxa->clk_io);
+			dev_warn(dev, "failed to re-enable core clock, %pe\n",
+				 ERR_PTR(ret));
+			return;
+		}
+	} else {
+		pm_runtime_put_noidle(dev);
+	}
 
 	sdhci_remove_host(host, 1);
-
-	clk_disable_unprepare(pxa->clk_io);
-	clk_disable_unprepare(pxa->clk_core);
 }
 
 static int sdhci_pxav3_suspend(struct device *dev)
 {
-	int ret;
 	struct sdhci_host *host = dev_get_drvdata(dev);
+	int ret;
+
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret)
+		return ret;
 
-	pm_runtime_get_sync(dev);
 	if (host->tuning_mode != SDHCI_TUNING_MODE_3)
 		mmc_retune_needed(host->mmc);
 	ret = sdhci_suspend_host(host);
@@ -538,10 +556,13 @@ static int sdhci_pxav3_suspend(struct device *dev)
 
 static int sdhci_pxav3_resume(struct device *dev)
 {
-	int ret;
 	struct sdhci_host *host = dev_get_drvdata(dev);
+	int ret;
+
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret)
+		return ret;
 
-	pm_runtime_get_sync(dev);
 	ret = sdhci_resume_host(host);
 	pm_runtime_put_autosuspend(dev);
 
@@ -560,8 +581,7 @@ static int sdhci_pxav3_runtime_suspend(struct device *dev)
 		mmc_retune_needed(host->mmc);
 
 	clk_disable_unprepare(pxa->clk_io);
-	if (!IS_ERR(pxa->clk_core))
-		clk_disable_unprepare(pxa->clk_core);
+	clk_disable_unprepare(pxa->clk_core);
 
 	return 0;
 }
@@ -571,10 +591,17 @@ static int sdhci_pxav3_runtime_resume(struct device *dev)
 	struct sdhci_host *host = dev_get_drvdata(dev);
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct sdhci_pxa *pxa = sdhci_pltfm_priv(pltfm_host);
+	int ret;
 
-	clk_prepare_enable(pxa->clk_io);
-	if (!IS_ERR(pxa->clk_core))
-		clk_prepare_enable(pxa->clk_core);
+	ret = clk_prepare_enable(pxa->clk_io);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(pxa->clk_core);
+	if (ret) {
+		clk_disable_unprepare(pxa->clk_io);
+		return ret;
+	}
 
 	sdhci_runtime_resume_host(host, 0);
 	return 0;
-- 
2.55.0

Re: [PATCHv3] mmc: sdhci-pxav3: use managed clock enablement

[Adrian Hunter]

On 23/09/2026 21:27, Rosen Penev wrote:
> Replace devm_clk_get() plus clk_prepare_enable() with the managed
> variants devm_clk_get_enabled() and devm_clk_get_optional_enabled().
> The core clock becomes optional, which removes the IS_ERR() guards
> in runtime suspend/resume and lets devms unwind the clocks, so the
> manual clk_disable_unprepare() calls in the probe error paths and
> remove are dropped. The NULL version of clk_io was removed as all dts
> files that use sdhci-pxav3 specify io as a clock name. Strictly speaking
> that could break pdata users but all of that is slated for removal.
> 
> clk_prepare_enable() and friends are NULL safe, so no need to check if
> the optional clock is present.
> 
> Handle failures of clk_prepare_enable() in runtime resume instead
> of ignoring them: a failed enable would leave controller register
> accesses hitting unclocked hardware. Bail out before resuming the host
> and unwind clk_io when enabling clk_core fails. This also drops the
> now unused pltfm_host variable from remove().
> 
> Handle result of the forced resumes in remove(), suspend() and resume().
> Use pm_runtime_resume_and_get(), which does not bump the usage counter on
> error.
> 
> Assisted-by: LLM
> Signed-off-by: Rosen Penev <rosenp@gmail.com>

mmc core and SDHCI do not withstand runtime resume failure anyway.
Of the changes outside sdhci_pxav3_probe(), only the ones that follow
directly from devm and the optional core clock are worth keeping.
So more like v1 plus the sdhci-pxav2-style io clock lookup.

> ---
>  v3: fix sashiko errors.
>  v2: fix resume paths.
>  drivers/mmc/host/sdhci-pxav3.c | 91 ++++++++++++++++++++++------------
>  1 file changed, 59 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
> index f39764ffd4a0..fda1eafe5685 100644
> --- a/drivers/mmc/host/sdhci-pxav3.c
> +++ b/drivers/mmc/host/sdhci-pxav3.c
> @@ -413,19 +413,16 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
>  	pltfm_host = sdhci_priv(host);
>  	pxa = sdhci_pltfm_priv(pltfm_host);
>  
> -	pxa->clk_io = devm_clk_get(dev, "io");
> +	pxa->clk_io = devm_clk_get_optional_enabled(dev, "io");
> +	if (!pxa->clk_io)
> +		pxa->clk_io = devm_clk_get_enabled(dev, NULL);
>  	if (IS_ERR(pxa->clk_io))
> -		pxa->clk_io = devm_clk_get(dev, NULL);
> -	if (IS_ERR(pxa->clk_io)) {
> -		dev_err(dev, "failed to get io clock\n");
> -		return PTR_ERR(pxa->clk_io);
> -	}
> +		return dev_err_probe(dev, PTR_ERR(pxa->clk_io), "failed to get io clock\n");
>  	pltfm_host->clk = pxa->clk_io;
> -	clk_prepare_enable(pxa->clk_io);
>  
> -	pxa->clk_core = devm_clk_get(dev, "core");
> -	if (!IS_ERR(pxa->clk_core))
> -		clk_prepare_enable(pxa->clk_core);
> +	pxa->clk_core = devm_clk_get_optional_enabled(dev, "core");
> +	if (IS_ERR(pxa->clk_core))
> +		return PTR_ERR(pxa->clk_core);
>  
>  	host->mmc->caps |= MMC_CAP_NEED_RSP_BUSY;
>  	/* enable 1/8V DDR capable */
> @@ -434,17 +431,17 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
>  	if (device_is_compatible(dev, "marvell,armada-380-sdhci")) {
>  		ret = armada_38x_quirks(pdev, host);
>  		if (ret < 0)
> -			goto err_mbus_win;
> +			return ret;
>  		ret = mv_conf_mbus_windows(pdev, mv_mbus_dram_info());
>  		if (ret < 0)
> -			goto err_mbus_win;
> +			return ret;
>  	}
>  
>  	match = of_match_device(of_match_ptr(sdhci_pxav3_of_match), &pdev->dev);
>  	if (match) {
>  		ret = mmc_of_parse(host->mmc);
>  		if (ret)
> -			goto err_of_parse;
> +			return ret;
>  		sdhci_get_of_property(pdev);
>  		pdata = pxav3_get_mmc_pdata(dev);
>  		pdev->dev.platform_data = pdata;
> @@ -499,35 +496,56 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
>  err_add_host:
>  	pm_runtime_disable(&pdev->dev);
>  	pm_runtime_put_noidle(&pdev->dev);
> -err_of_parse:
> -err_mbus_win:
> -	clk_disable_unprepare(pxa->clk_io);
> -	clk_disable_unprepare(pxa->clk_core);
>  	return ret;
>  }
>  
>  static void sdhci_pxav3_remove(struct platform_device *pdev)
>  {
>  	struct sdhci_host *host = platform_get_drvdata(pdev);
> +	struct device *dev = &pdev->dev;
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>  	struct sdhci_pxa *pxa = sdhci_pltfm_priv(pltfm_host);
> +	int ret;
>  
> -	pm_runtime_get_sync(&pdev->dev);
> -	pm_runtime_disable(&pdev->dev);
> -	pm_runtime_put_noidle(&pdev->dev);
> +	ret = pm_runtime_resume_and_get(dev);
> +	pm_runtime_disable(dev);
> +	if (ret) {
> +		/*
> +		 * Runtime resume failed, so the controller may still be
> +		 * unclocked.  Restore the clocks so the register accesses in
> +		 * sdhci_remove_host() do not hit unclocked hardware.
> +		 */
> +		dev_warn(dev, "failed to resume controller, %pe\n",
> +			 ERR_PTR(ret));
> +		ret = clk_prepare_enable(pxa->clk_io);
> +		if (ret) {
> +			dev_warn(dev, "failed to re-enable io clock, %pe\n",
> +				 ERR_PTR(ret));
> +			return;
> +		}
> +		ret = clk_prepare_enable(pxa->clk_core);
> +		if (ret) {
> +			clk_disable_unprepare(pxa->clk_io);
> +			dev_warn(dev, "failed to re-enable core clock, %pe\n",
> +				 ERR_PTR(ret));
> +			return;
> +		}
> +	} else {
> +		pm_runtime_put_noidle(dev);
> +	}
>  
>  	sdhci_remove_host(host, 1);
> -
> -	clk_disable_unprepare(pxa->clk_io);
> -	clk_disable_unprepare(pxa->clk_core);
>  }
>  
>  static int sdhci_pxav3_suspend(struct device *dev)
>  {
> -	int ret;
>  	struct sdhci_host *host = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret)
> +		return ret;
>  
> -	pm_runtime_get_sync(dev);
>  	if (host->tuning_mode != SDHCI_TUNING_MODE_3)
>  		mmc_retune_needed(host->mmc);
>  	ret = sdhci_suspend_host(host);
> @@ -538,10 +556,13 @@ static int sdhci_pxav3_suspend(struct device *dev)
>  
>  static int sdhci_pxav3_resume(struct device *dev)
>  {
> -	int ret;
>  	struct sdhci_host *host = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret)
> +		return ret;
>  
> -	pm_runtime_get_sync(dev);
>  	ret = sdhci_resume_host(host);
>  	pm_runtime_put_autosuspend(dev);
>  
> @@ -560,8 +581,7 @@ static int sdhci_pxav3_runtime_suspend(struct device *dev)
>  		mmc_retune_needed(host->mmc);
>  
>  	clk_disable_unprepare(pxa->clk_io);
> -	if (!IS_ERR(pxa->clk_core))
> -		clk_disable_unprepare(pxa->clk_core);
> +	clk_disable_unprepare(pxa->clk_core);
>  
>  	return 0;
>  }
> @@ -571,10 +591,17 @@ static int sdhci_pxav3_runtime_resume(struct device *dev)
>  	struct sdhci_host *host = dev_get_drvdata(dev);
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>  	struct sdhci_pxa *pxa = sdhci_pltfm_priv(pltfm_host);
> +	int ret;
>  
> -	clk_prepare_enable(pxa->clk_io);
> -	if (!IS_ERR(pxa->clk_core))
> -		clk_prepare_enable(pxa->clk_core);
> +	ret = clk_prepare_enable(pxa->clk_io);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_prepare_enable(pxa->clk_core);
> +	if (ret) {
> +		clk_disable_unprepare(pxa->clk_io);
> +		return ret;
> +	}
>  
>  	sdhci_runtime_resume_host(host, 0);
>  	return 0;