Re: [PATCH v2 8/8] mmc: sunxi: Add runtime_pm support

[Maxime Ripard <maxime.ripard@bootlin.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4387 bytes --]

Hi Ulf,

On Thu, Mar 15, 2018 at 10:30:54AM +0100, Ulf Hansson wrote:
> On 8 March 2018 at 15:52, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > 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 <maxime.ripard@bootlin.com>
> > ---
> >  drivers/mmc/host/sunxi-mmc.c | 46 +++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 46 insertions(+)
> >
> > diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
> > index f6374066081b..0f98a5fcaade 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>
> > @@ -973,6 +974,9 @@ static void sunxi_mmc_enable_sdio_irq(struct mmc_host *mmc, int enable)
> >         unsigned long flags;
> >         u32 imask;
> >
> > +       if (enable)
> > +               pm_runtime_get_noresume(host->dev);
> > +
> >         spin_lock_irqsave(&host->lock, flags);
> >
> >         imask = mmc_readl(host, REG_IMASK);
> > @@ -985,6 +989,9 @@ static void sunxi_mmc_enable_sdio_irq(struct mmc_host *mmc, int enable)
> >         }
> >         mmc_writel(host, REG_IMASK, imask);
> >         spin_unlock_irqrestore(&host->lock, flags);
> > +
> > +       if (!enable)
> > +               pm_runtime_put_noidle(host->mmc->parent);
> >  }
> >
> >  static void sunxi_mmc_hw_reset(struct mmc_host *mmc)
> > @@ -1398,6 +1405,11 @@ static int sunxi_mmc_probe(struct platform_device *pdev)
> >         if (ret)
> >                 goto error_free_dma;
> >
> > +       pm_runtime_set_active(&pdev->dev);
> > +       pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
> > +       pm_runtime_use_autosuspend(&pdev->dev);
> > +       pm_runtime_enable(&pdev->dev);
> > +
> >         ret = mmc_add_host(mmc);
> >         if (ret)
> >                 goto error_free_dma;
> > @@ -1418,6 +1430,7 @@ 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);
> >         disable_irq(host->irq);
> >         sunxi_mmc_disable(host);
> >         dma_free_coherent(&pdev->dev, PAGE_SIZE, host->sg_cpu, host->sg_dma);
> > @@ -1426,10 +1439,43 @@ static int sunxi_mmc_remove(struct platform_device *pdev)
> >         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 = sunxi_mmc_enable(host);
> > +       if (ret)
> > +               return ret;
> > +
> > +       sunxi_mmc_power_up(mmc, &mmc->ios);
> 
> Instead of doing power up, you may need restore some ios settings,
> such as the clock rate for example.
> 
> You may also need to restore some registers in sunxi device, in case
> it's possible that the controller loses context at runtime suspend.

The thing I was precisely trying to avoid was this :)

I could save and restore registers when the MMC controller is put into
suspend, but that's pretty much exactly the same sequence than what is
done in the MMC_POWER_UP sequence in .set_ios.

So it just felt cleaner to just do the power_up sequence at resume
time. It avoids having to maintain the list of registers to save and
restore, and it involves less code overall.

It suprised me a bit that the core will not call .set_ios with
MMC_POWER_UP at resume by itself, but I guess there's a good reason
for that :)

Thanks!
Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel