Hi Ulf,

On Thu, Dec 21, 2017 at 01:21:51PM +0100, Ulf Hansson wrote:

On 20 December 2017 at 11:50, Maxime Ripard
[off-list ref] wrote:

quoted

So far, even if our card was not in use, we didn't shut down our main
clock, which meant that it was still output on the MMC bus.

While this obviously means that we could save some power there, it also
created issues when it comes with EMC control since we'll have a perfect
peak at the card clock rate.

Let's implement runtime_pm with autosuspend so that we will shut down the
clock when it's not been in use for quite some time.

Signed-off-by: Maxime Ripard <redacted>
---
 drivers/mmc/host/sunxi-mmc.c | 90 ++++++++++++++++++++++++-------------
 1 file changed, 60 insertions(+), 30 deletions(-)

diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index 3ce46ebd3488..c6a0bd0e0476 100644
--- a/drivers/mmc/host/sunxi-mmc.c
+++ b/drivers/mmc/host/sunxi-mmc.c

@@ -35,6 +35,7 @@
 #include <linux/of_gpio.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>
 #include <linux/reset.h>
 #include <linux/scatterlist.h>

@@ -1217,29 +1218,11 @@ static int sunxi_mmc_resource_request(struct sunxi_mmc_host *host,
                return ret;
        }

-       ret = clk_prepare_enable(host->clk_mmc);
-       if (ret) {
-               dev_err(&pdev->dev, "Enable mmc clk err %d\n", ret);
-               goto error_disable_clk_ahb;
-       }
-
-       ret = clk_prepare_enable(host->clk_output);
-       if (ret) {
-               dev_err(&pdev->dev, "Enable output clk err %d\n", ret);
-               goto error_disable_clk_mmc;
-       }
-
-       ret = clk_prepare_enable(host->clk_sample);
-       if (ret) {
-               dev_err(&pdev->dev, "Enable sample clk err %d\n", ret);
-               goto error_disable_clk_output;
-       }
-

Actually, I think you should keep all the above. Perhaps you may want
to move it to a separate helper function though, which the
->runtime_resume() callbacks can invoke as well.

More reasons to why, see the comment in the ->probe() function.

quoted

        if (!IS_ERR(host->reset)) {
                ret = reset_control_reset(host->reset);
                if (ret) {
                        dev_err(&pdev->dev, "reset err %d\n", ret);
-                       goto error_disable_clk_sample;
+                       goto error_disable_clk_ahb;
                }
        }

@@ -1258,12 +1241,6 @@ static int sunxi_mmc_resource_request(struct sunxi_mmc_host *host,
 error_assert_reset:
        if (!IS_ERR(host->reset))
                reset_control_assert(host->reset);
-error_disable_clk_sample:
-       clk_disable_unprepare(host->clk_sample);
-error_disable_clk_output:
-       clk_disable_unprepare(host->clk_output);
-error_disable_clk_mmc:
-       clk_disable_unprepare(host->clk_mmc);
 error_disable_clk_ahb:
        clk_disable_unprepare(host->clk_ahb);
        return ret;

@@ -1280,6 +1257,7 @@ static int sunxi_mmc_probe(struct platform_device *pdev)
                dev_err(&pdev->dev, "mmc alloc host failed\n");
                return -ENOMEM;
        }
+       platform_set_drvdata(pdev, mmc);

        host = mmc_priv(mmc);
        host->mmc = mmc;

@@ -1340,12 +1318,16 @@ static int sunxi_mmc_probe(struct platform_device *pdev)
        if (ret)
                goto error_free_dma;

+       pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
+       pm_runtime_use_autosuspend(&pdev->dev);
+       pm_runtime_enable(&pdev->dev);
+

This means in case you don't have CONFIG_PM set when compiling this
driver, the clocks will never be enabled using runtime PM.

I think it's good practice to deal with this, therefore I think you
should enable the clocks as before, and instead indicate that the
device is already runtime resumed.

In other words, before you call pm_runtime_enable(), invoke
pm_runtime_set_active().

That's a very good point, I'll do that in the next iteration.

quoted

        ret = mmc_add_host(mmc);
        if (ret)
                goto error_free_dma;

        dev_info(&pdev->dev, "base:0x%p irq:%u\n", host->reg_base, host->irq);
-       platform_set_drvdata(pdev, mmc);
+
        return 0;

 error_free_dma:

@@ -1361,27 +1343,75 @@ static int sunxi_mmc_remove(struct platform_device *pdev)
        struct sunxi_mmc_host *host = mmc_priv(mmc);

        mmc_remove_host(mmc);
+       pm_runtime_force_suspend(&pdev->dev);

Do you need the clocks to be enabled, while calling disable_irq() and
sunxi_mmc_reset_host()?

In such case you need to call pm_runtime_get_sync() here. Then move
pm_runtime_force_suspend() a few lines below, and later call
pm_runtime_put_noidle().

We don't for disable_irq, but we need it for
sunxi_mmc_reset_host. I'll do what you suggest.

quoted

        disable_irq(host->irq);
        sunxi_mmc_reset_host(host);

        if (!IS_ERR(host->reset))
                reset_control_assert(host->reset);

-       clk_disable_unprepare(host->clk_sample);
+       dma_free_coherent(&pdev->dev, PAGE_SIZE, host->sg_cpu, host->sg_dma);
+       mmc_free_host(mmc);
+
+       return 0;
+}
+
+static int sunxi_mmc_runtime_resume(struct device *dev)
+{
+       struct mmc_host *mmc = dev_get_drvdata(dev);
+       struct sunxi_mmc_host *host = mmc_priv(mmc);
+       int ret;
+
+       ret = clk_prepare_enable(host->clk_mmc);
+       if (ret) {
+               dev_err(dev, "Enable mmc clk err %d\n", ret);
+               return ret;
+       }
+
+       ret = clk_prepare_enable(host->clk_output);
+       if (ret) {
+               dev_err(dev, "Enable output clk err %d\n", ret);
+               goto error_disable_clk_mmc;
+       }
+
+       ret = clk_prepare_enable(host->clk_sample);
+       if (ret) {
+               dev_err(dev, "Enable sample clk err %d\n", ret);
+               goto error_disable_clk_output;
+       }
+
+       return 0;
+
+error_disable_clk_output:
        clk_disable_unprepare(host->clk_output);
+error_disable_clk_mmc:
        clk_disable_unprepare(host->clk_mmc);
-       clk_disable_unprepare(host->clk_ahb);
+       return ret;
+}

-       dma_free_coherent(&pdev->dev, PAGE_SIZE, host->sg_cpu, host->sg_dma);
-       mmc_free_host(mmc);
+static int sunxi_mmc_runtime_suspend(struct device *dev)
+{
+       struct mmc_host *mmc = dev_get_drvdata(dev);
+       struct sunxi_mmc_host *host = mmc_priv(mmc);
+
+       clk_disable_unprepare(host->clk_sample);
+       clk_disable_unprepare(host->clk_output);
+       clk_disable_unprepare(host->clk_mmc);

        return 0;
 }

+static const struct dev_pm_ops sunxi_mmc_pm_ops = {
+       SET_RUNTIME_PM_OPS(sunxi_mmc_runtime_suspend,
+                          sunxi_mmc_runtime_resume,
+                          NULL)
+};
+
 static struct platform_driver sunxi_mmc_driver = {
        .driver = {
                .name   = "sunxi-mmc",
                .of_match_table = of_match_ptr(sunxi_mmc_of_match),
+               .pm = &sunxi_mmc_pm_ops,
        },
        .probe          = sunxi_mmc_probe,
        .remove         = sunxi_mmc_remove,
--
git-series 0.9.1

Otherwise this looks good to me!

BTW, isn't system wide suspend/resume() also important to save power
for? That's rather simple to implement, after you have enabled runtime
PM support.

We don't have (unfortunately) any kind of suspend support at the
moment, so I guess that'll come as a single step when we'll have it :)

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180104/1e76c357/attachment.sig>