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@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> > ---
> > 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:
signature.asc
Description: PGP signature
