On 15 March 2018 at 11:04, Maxime Ripard <maxime.ripard@xxxxxxxxxxx> wrote:
> 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.
Well, there may be some overlap.
>
> 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.
I understand.
>
> 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 :)
It does that when it runtime PM manages the mmc/sd/sdio card, don't
confuse that with the mmc host. That's because it needs to follow the
(e)MMC/SD/SDIO spec, which don't allows you to just cut the power to
card without first informing (sending commands) the card about it.
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
