On Thu, 30 Sept 2021 at 08:57, Tony Lindgren <tony@xxxxxxxxxxx> wrote:
>
> Implement PM runtime functions and enable MMC_CAP_AGGRESSIVE_PM.
I suggest you split this change into two pieces. MMC_CAP_AGGRESSIVE_PM
is about enabling runtime PM management for the eMMC/SD card device,
which is perfectly fine to use independently of whether runtime PM is
supported for the host device.
>
> Note that we save context in probe to avoid restoring invalid context
> on the first resume. For system suspend, we have the new PM runtime
> functions do most of the work.
>
> Signed-off-by: Tony Lindgren <tony@xxxxxxxxxxx>
> ---
> drivers/mmc/host/sdhci-omap.c | 66 +++++++++++++++++++++++++++++------
> 1 file changed, 56 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c
> --- a/drivers/mmc/host/sdhci-omap.c
> +++ b/drivers/mmc/host/sdhci-omap.c
> @@ -117,6 +117,9 @@ struct sdhci_omap_host {
>
> struct pinctrl *pinctrl;
> struct pinctrl_state **pinctrl_state;
> + unsigned long context_valid:1;
> + unsigned long is_runtime_suspended:1;
> + unsigned long needs_resume:1;
> bool is_tuning;
>
> /* Offset for omap specific registers from base */
> @@ -1207,6 +1210,8 @@ static const struct soc_device_attribute sdhci_omap_soc_devices[] = {
> }
> };
>
> +static void sdhci_omap_context_save(struct sdhci_omap_host *omap_host);
> +
> static int sdhci_omap_probe(struct platform_device *pdev)
> {
> int ret;
> @@ -1338,6 +1343,8 @@ static int sdhci_omap_probe(struct platform_device *pdev)
> /* R1B responses is required to properly manage HW busy detection. */
> mmc->caps |= MMC_CAP_NEED_RSP_BUSY;
>
> + mmc->caps |= MMC_CAP_AGGRESSIVE_PM;
> +
> ret = sdhci_setup_host(host);
> if (ret)
> goto err_put_sync;
> @@ -1350,6 +1357,11 @@ static int sdhci_omap_probe(struct platform_device *pdev)
> if (ret)
> goto err_cleanup_host;
>
> + sdhci_omap_context_save(omap_host);
> + omap_host->context_valid = 1;
Looks like you can remove this flag, it's not being used.
> +
> + pm_runtime_put_sync(dev);
I recommend to use the PM runtime autosuspend feature, as to avoid an
initial latency for every I/O request to the host driver. The mmc core
already supports that, see mmc_release_host().
The typical default timeout value for autosuspend, is usually set
~50-200ms, by host drivers (if I recall correctly).
> +
> return 0;
>
> err_cleanup_host:
> @@ -1371,6 +1383,7 @@ static int sdhci_omap_remove(struct platform_device *pdev)
> struct device *dev = &pdev->dev;
> struct sdhci_host *host = platform_get_drvdata(pdev);
>
> + pm_runtime_get_sync(dev);
> sdhci_remove_host(host, true);
> pm_runtime_put_sync(dev);
There is no guarantee that this triggers a call to
->sdhci_omap_runtime_suspend(), which I guess is what we want.
Userspace via sysfs may have increase the RPM usage count
(pm_runtime_forbid(), for example.
To address this, I would call pm_runtime_disable() first and then
explicitly put the device into low power state, rather than relying on
runtime PM to do it. Another option could be to use
pm_runtime_force_suspend().
> pm_runtime_disable(dev);
> @@ -1402,42 +1415,75 @@ static void sdhci_omap_context_restore(struct sdhci_omap_host *omap_host)
> sdhci_omap_writel(omap_host, SDHCI_OMAP_ISE, omap_host->ise);
> }
>
> -static int __maybe_unused sdhci_omap_suspend(struct device *dev)
> +static int __maybe_unused sdhci_omap_runtime_suspend(struct device *dev)
> {
> struct sdhci_host *host = dev_get_drvdata(dev);
> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host);
>
> - sdhci_suspend_host(host);
> -
Shouldn't you call sdhci_runtime_suspend_host() somewhere here?
> sdhci_omap_context_save(omap_host);
>
> pinctrl_pm_select_idle_state(dev);
>
> - pm_runtime_force_suspend(dev);
> + omap_host->is_runtime_suspended = 1;
>
> return 0;
> }
>
> -static int __maybe_unused sdhci_omap_resume(struct device *dev)
> +static int __maybe_unused sdhci_omap_runtime_resume(struct device *dev)
> {
> struct sdhci_host *host = dev_get_drvdata(dev);
> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host);
>
> - pm_runtime_force_resume(dev);
> -
> pinctrl_pm_select_default_state(dev);
>
> - sdhci_omap_context_restore(omap_host);
> + if (omap_host->context_valid)
> + sdhci_omap_context_restore(omap_host);
> +
> + omap_host->is_runtime_suspended = 0;
Shouldn't you call sdhci_runtime_resume_host() somewhere here?
> +
> + return 0;
> +}
> +
> +static int __maybe_unused sdhci_omap_suspend(struct device *dev)
> +{
> + struct sdhci_host *host = dev_get_drvdata(dev);
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host);
> +
> + if (omap_host->is_runtime_suspended)
> + return 0;
So if the host is already runtime suspended, it's okay to just leave it as is?
In a way that sounds like you could call pm_runtime_force_suspend()
instead, assuming the sdhci_omap_runtime_suspend() can be extended to
do the right thing for system suspend as well.
It looks a bit odd that sdhci_suspend_host() is called only when the
host is runtime resumed. Perhaps you can elaborate a bit more on why
this is, so I can understand better what you want to achieve here.
> +
> + sdhci_suspend_host(host);
> + sdhci_omap_runtime_suspend(dev);
> + omap_host->needs_resume = 1;
>
> + return 0;
> +}
> +
> +static int __maybe_unused sdhci_omap_resume(struct device *dev)
> +{
> + struct sdhci_host *host = dev_get_drvdata(dev);
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host);
> +
> + if (!omap_host->needs_resume)
> + return 0;
> +
> + sdhci_omap_runtime_resume(dev);
> sdhci_resume_host(host);
> + omap_host->needs_resume = 0;
>
> return 0;
> }
> #endif
> -static SIMPLE_DEV_PM_OPS(sdhci_omap_dev_pm_ops, sdhci_omap_suspend,
> - sdhci_omap_resume);
> +
> +static const struct dev_pm_ops sdhci_omap_dev_pm_ops = {
> + SET_RUNTIME_PM_OPS(sdhci_omap_runtime_suspend,
> + sdhci_omap_runtime_resume, NULL)
> + SET_SYSTEM_SLEEP_PM_OPS(sdhci_omap_suspend, sdhci_omap_resume)
> +};
>
> static struct platform_driver sdhci_omap_driver = {
> .probe = sdhci_omap_probe,
> --
> 2.33.0
Kind regards
Uffe
