Re: [PATCH v1 1/3]mmc: implemented runtime pm for mmc host

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



2011/1/25 Tardy, Pierre <pierre.tardy@xxxxxxxxx>:

> Actually in sdhci, you power off the MMC IP block, you power OFF the
> MCLK (at least on our platform)
>
> I don't know other platform where mmc clock is not controlled more or
> less directlty by the sdhc IP.

This is the case with host/mmci.c.

It is taking a clock inside the driver (MCLK), but since it's an AMBA
primecell, you can see that when it's probed it also requests and
optional IP clock in drivers/amba/bus.c.

The ARM reference platforms are wired with two different clocks
connected to each of these.

There are platforms, such as the U300 that just connect the two
into one terminal though. To get a chance to handle the abstraction
for both cases these two clocks refer to the same struct clk in the
clock tree.

>> Further this patch cannot be applied as-is.
>> It needs a clause where it will wait for the clock gating to
>> be sync:ed *before* calling the suspend hook.
>
> That's the purpose of those hunks:
>
> iff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 8c3769b..27931f6 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -20,6 +20,7 @@
>  #include <linux/slab.h>
>  #include <linux/scatterlist.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/pm_runtime.h>
>
>  #include <linux/leds.h>
>
> @@ -1221,6 +1222,7 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>  {
>        struct sdhci_host *host;
>        unsigned long flags;
> +       unsigned int lastclock;
>        u8 ctrl;
>
>        host = mmc_priv(mmc);
> @@ -1231,6 +1233,27 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>                goto out;
>
>        /*
> +        * get/put runtime_pm usage counter at ios->clock transitions
> +        * We need to do it before any other chip access, as sdhci could
> +        * be power gated
> +        */
> +       lastclock = host->iosclock;
> +       host->iosclock = ios->clock;
> +       if (lastclock == 0 && ios->clock != 0) {
> +               spin_unlock_irqrestore(&host->lock, flags);
> +               pm_runtime_get_sync(host->mmc->parent);
> +               spin_lock_irqsave(&host->lock, flags);
> +       } else if (lastclock != 0 && ios->clock == 0) {
> +               spin_unlock_irqrestore(&host->lock, flags);
> +               pm_runtime_mark_last_busy(host->mmc->parent);
> +               pm_runtime_put_autosuspend(host->mmc->parent);
> +               spin_lock_irqsave(&host->lock, flags);
> +       }
> +       /* no need to configure the rest.. */
> +       if (host->iosclock == 0)
> +               goto out;
> +
> +       /*
>         * Reset the chip on each power off.
>         * Should clear out any weird states.
>         */

Aha I get it. So it uses the freq shift from the existing clock gate
code to hint rpm, that's actually how I envisioned that this would
work for this case.

Now I really started liking this patch.
Acked-by: Linus Walleij <linus.walleij@xxxxxxxxxxxxxx>

> @@ -1303,6 +1326,8 @@ static int sdhci_get_ro(struct mmc_host *mmc)
>
> I'm wondering if this code could be generic to all drivers, and if clock gating could not be taking/releasing reference counter on the mmc_bus whenever there is a clock gating transition?
> We could condition that on some MMC_CAP_POWERGATE_WILL_CLKGATE capability flag.

Time will tell I guess, the code in each driver will surely be similar
to the above, but I already see some deviations, e.g. some HW
will need to delay the actual action taken, some may want to
manipulate a register or regulator etc, so I'd leave it open-ended
like this.

Yours,
Linus Walleij
--
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


[Index of Archives]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux