Re: [PATCH v2 2/6] mmc: sdhci-of-at91: add option to keep clocks enabled

[Adrian Hunter <adrian.hunter@intel.com>]

On 07/09/2026 16:25, Robert Marko wrote:
> sdhci_at91_set_clks_presets() both enables the controller clocks and
> programs its capabilities and preset registers. This prevents callers from
> restoring the registers without changing the clock enable counts.
> 
> Move clock enablement to callers and add a SoC data flag for controllers
> that must keep their clocks enabled. Use it in the runtime PM paths while
> keeping register restoration separate from clock enablement.
> 
> Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> 
>  drivers/mmc/host/sdhci-of-at91.c | 39 +++++++++++++++++++-------------
>  1 file changed, 23 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
> index 7c4ac65f247d..b7e2a89da348 100644
> --- a/drivers/mmc/host/sdhci-of-at91.c
> +++ b/drivers/mmc/host/sdhci-of-at91.c
> @@ -38,6 +38,7 @@
>  struct sdhci_at91_soc_data {
>  	const struct sdhci_pltfm_data *pdata;
>  	bool baseclk_is_generated_internally;
> +	bool keep_clks_on;

It can be a bit easier to read when conditions do not have to be
inverted, like

	if (priv->soc_data->suspend_clks)

instead of:
	
	if (!priv->soc_data->keep_clks_on)


>  	unsigned int divider_for_baseclk;
>  };
>  
> @@ -164,7 +165,7 @@ static const struct of_device_id sdhci_at91_dt_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, sdhci_at91_dt_match);
>  
> -static int sdhci_at91_set_clks_presets(struct device *dev)
> +static void sdhci_at91_set_clks_presets(struct device *dev)
>  {
>  	struct sdhci_host *host = dev_get_drvdata(dev);
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> @@ -174,7 +175,6 @@ static int sdhci_at91_set_clks_presets(struct device *dev)
>  	unsigned int			gck_rate, clk_base_rate;
>  	unsigned int			preset_div;
>  
> -	clk_prepare_enable(priv->hclock);

Might just as well pass in a parameter and:

	if (prepare_clks)
		clk_prepare_enable(priv->hclock);

>  	caps0 = readl(host->ioaddr + SDHCI_CAPABILITIES);
>  	caps1 = readl(host->ioaddr + SDHCI_CAPABILITIES_1);
>  
> @@ -222,11 +222,6 @@ static int sdhci_at91_set_clks_presets(struct device *dev)
>  	preset_div = DIV_ROUND_UP(gck_rate, 50000000) - 1;
>  	writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div,
>  	       host->ioaddr + SDHCI_PRESET_FOR_DDR50);
> -
> -	clk_prepare_enable(priv->mainck);
> -	clk_prepare_enable(priv->gck);

Ditto

> -
> -	return 0;
>  }
>  
>  static int sdhci_at91_suspend(struct device *dev)
> @@ -254,9 +249,11 @@ static int sdhci_at91_runtime_suspend(struct device *dev)
>  	if (host->tuning_mode != SDHCI_TUNING_MODE_3)
>  		mmc_retune_needed(host->mmc);
>  
> -	clk_disable_unprepare(priv->gck);
> -	clk_disable_unprepare(priv->hclock);
> -	clk_disable_unprepare(priv->mainck);
> +	if (!priv->soc_data->keep_clks_on) {
> +		clk_disable_unprepare(priv->gck);
> +		clk_disable_unprepare(priv->hclock);
> +		clk_disable_unprepare(priv->mainck);
> +	}
>  
>  	return 0;
>  }
> @@ -269,14 +266,23 @@ static int sdhci_at91_runtime_resume(struct device *dev)
>  	int ret;
>  
>  	if (priv->restore_needed) {
> -		ret = sdhci_at91_set_clks_presets(dev);
> -		if (ret)
> -			return ret;
> +		if (!priv->soc_data->keep_clks_on)
> +			clk_prepare_enable(priv->hclock);
> +
> +		sdhci_at91_set_clks_presets(dev);
> +
> +		if (!priv->soc_data->keep_clks_on) {
> +			clk_prepare_enable(priv->mainck);
> +			clk_prepare_enable(priv->gck);
> +		}

Then that all becomes just:

		sdhci_at91_set_clks_presets(dev, !priv->soc_data->keep_clks_on);

>  
>  		priv->restore_needed = false;
>  		goto out;
>  	}
>  
> +	if (priv->soc_data->keep_clks_on)
> +		goto out;
> +
>  	ret = clk_prepare_enable(priv->mainck);
>  	if (ret) {
>  		dev_err(dev, "can't enable mainck\n");
> @@ -344,9 +350,10 @@ static int sdhci_at91_probe(struct platform_device *pdev)
>  		return dev_err_probe(&pdev->dev, PTR_ERR(priv->gck),
>  				     "failed to get multclk\n");
>  
> -	ret = sdhci_at91_set_clks_presets(&pdev->dev);
> -	if (ret)
> -		return ret;
> +	clk_prepare_enable(priv->hclock);
> +	sdhci_at91_set_clks_presets(&pdev->dev);
> +	clk_prepare_enable(priv->mainck);
> +	clk_prepare_enable(priv->gck);

And that all becomes just:

	sdhci_at91_set_clks_presets(&pdev->dev, true);

>  
>  	priv->restore_needed = false;
>