* [PATCH] mmc: sdhci-pxav3: use managed clock enablement
@ 2026-09-01 0:42 Rosen Penev
2026-09-09 7:51 ` Adrian Hunter
0 siblings, 1 reply; 3+ messages in thread
From: Rosen Penev @ 2026-09-01 0:42 UTC (permalink / raw)
To: linux-mmc; +Cc: Adrian Hunter, Ulf Hansson, open list
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.
clk_prepare_enable() and friends are NULL safe, so no need to check if
the optional clock is present.
Also 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().
Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
drivers/mmc/host/sdhci-pxav3.c | 42 +++++++++++++++-------------------
1 file changed, 19 insertions(+), 23 deletions(-)
diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
index d082c4e21aa9..db9dfa92cb57 100644
--- a/drivers/mmc/host/sdhci-pxav3.c
+++ b/drivers/mmc/host/sdhci-pxav3.c
@@ -414,19 +414,18 @@ 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_enabled(dev, "io");
if (IS_ERR(pxa->clk_io))
- pxa->clk_io = devm_clk_get(dev, NULL);
+ pxa->clk_io = devm_clk_get_enabled(dev, NULL);
if (IS_ERR(pxa->clk_io)) {
dev_err(dev, "failed to get io clock\n");
return PTR_ERR(pxa->clk_io);
}
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 */
@@ -435,17 +434,17 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
if (of_device_is_compatible(np, "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;
@@ -500,27 +499,18 @@ 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 sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
- struct sdhci_pxa *pxa = sdhci_pltfm_priv(pltfm_host);
pm_runtime_get_sync(&pdev->dev);
pm_runtime_disable(&pdev->dev);
pm_runtime_put_noidle(&pdev->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)
@@ -561,8 +551,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;
}
@@ -572,10 +561,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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] mmc: sdhci-pxav3: use managed clock enablement
2026-09-01 0:42 [PATCH] mmc: sdhci-pxav3: use managed clock enablement Rosen Penev
@ 2026-09-09 7:51 ` Adrian Hunter
2026-09-09 21:39 ` Rosen Penev
0 siblings, 1 reply; 3+ messages in thread
From: Adrian Hunter @ 2026-09-09 7:51 UTC (permalink / raw)
To: Rosen Penev, linux-mmc
Cc: Ulf Hansson, open list, Karel Balej, Duje Mihanović
On 01/09/2026 03:42, 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.
>
> clk_prepare_enable() and friends are NULL safe, so no need to check if
> the optional clock is present.
>
> Also 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().
>
> Assisted-by: opencode:big-pickle
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
> drivers/mmc/host/sdhci-pxav3.c | 42 +++++++++++++++-------------------
> 1 file changed, 19 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
> index d082c4e21aa9..db9dfa92cb57 100644
> --- a/drivers/mmc/host/sdhci-pxav3.c
> +++ b/drivers/mmc/host/sdhci-pxav3.c
> @@ -414,19 +414,18 @@ 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_enabled(dev, "io");
> if (IS_ERR(pxa->clk_io))
> - pxa->clk_io = devm_clk_get(dev, NULL);
> + pxa->clk_io = devm_clk_get_enabled(dev, NULL);
Sashiko has a point about getting the wrong clock:
https://sashiko.dev/#/patchset/20260901004243.50052-1-rosenp%40gmail.com?part=1
> if (IS_ERR(pxa->clk_io)) {
> dev_err(dev, "failed to get io clock\n");
> return PTR_ERR(pxa->clk_io);
> }
> 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 */
> @@ -435,17 +434,17 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
> if (of_device_is_compatible(np, "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;
> @@ -500,27 +499,18 @@ 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 sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> - struct sdhci_pxa *pxa = sdhci_pltfm_priv(pltfm_host);
>
> pm_runtime_get_sync(&pdev->dev);
> pm_runtime_disable(&pdev->dev);
> pm_runtime_put_noidle(&pdev->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)
> @@ -561,8 +551,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;
> }
> @@ -572,10 +561,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;
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] mmc: sdhci-pxav3: use managed clock enablement
2026-09-09 7:51 ` Adrian Hunter
@ 2026-09-09 21:39 ` Rosen Penev
0 siblings, 0 replies; 3+ messages in thread
From: Rosen Penev @ 2026-09-09 21:39 UTC (permalink / raw)
To: Adrian Hunter
Cc: linux-mmc, Ulf Hansson, open list, Karel Balej, Duje Mihanović
On Wed, Sep 9, 2026 at 12:51 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 01/09/2026 03:42, 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.
> >
> > clk_prepare_enable() and friends are NULL safe, so no need to check if
> > the optional clock is present.
> >
> > Also 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().
> >
> > Assisted-by: opencode:big-pickle
> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > ---
> > drivers/mmc/host/sdhci-pxav3.c | 42 +++++++++++++++-------------------
> > 1 file changed, 19 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
> > index d082c4e21aa9..db9dfa92cb57 100644
> > --- a/drivers/mmc/host/sdhci-pxav3.c
> > +++ b/drivers/mmc/host/sdhci-pxav3.c
> > @@ -414,19 +414,18 @@ 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_enabled(dev, "io");
> > if (IS_ERR(pxa->clk_io))
> > - pxa->clk_io = devm_clk_get(dev, NULL);
> > + pxa->clk_io = devm_clk_get_enabled(dev, NULL);
>
> Sashiko has a point about getting the wrong clock:
>
> https://sashiko.dev/#/patchset/20260901004243.50052-1-rosenp%40gmail.com?part=1
It's technically correct but meaningless. All dts files specify the
clock properly. Both paths attempt to enable the same clock.
>
> > if (IS_ERR(pxa->clk_io)) {
> > dev_err(dev, "failed to get io clock\n");
> > return PTR_ERR(pxa->clk_io);
> > }
> > 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 */
> > @@ -435,17 +434,17 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
> > if (of_device_is_compatible(np, "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;
> > @@ -500,27 +499,18 @@ 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 sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > - struct sdhci_pxa *pxa = sdhci_pltfm_priv(pltfm_host);
> >
> > pm_runtime_get_sync(&pdev->dev);
> > pm_runtime_disable(&pdev->dev);
> > pm_runtime_put_noidle(&pdev->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)
> > @@ -561,8 +551,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;
> > }
> > @@ -572,10 +561,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;
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-09-09 21:40 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-09-01 0:42 [PATCH] mmc: sdhci-pxav3: use managed clock enablement Rosen Penev
2026-09-09 7:51 ` Adrian Hunter
2026-09-09 21:39 ` Rosen Penev
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®