2009/6/15 Pierre Ossman <pierre@xxxxxxxxx>:
> On Tue, 2 Jun 2009 14:36:28 +0200
> Linus Walleij <linus.ml.walleij@xxxxxxxxx> wrote:
>
>> This patch modified the MMC core code to optionally call the
>> set_ios() operation on the driver with the clock frequency set
>> to 0 to gate the hardware block clock (and thus the MCI clock)
>> for an MMC host controller after a grace period of at least 8
>> MCLK cycles. It is inspired by existing clock gating code found
>> in the OMAP and Atmel drivers and brings this up to the host
>> abstraction. Gating is performed before and after any MMC request
>> or IOS operation, the other optional host operations will not
>> ungate/gate the clock since they do not necessary touch the
>> actual host controller hardware.
>>
>> It exemplifies by implementing this for the MMCI/PL180 MMC/SD
>> host controller, but it should be simple to switch OMAP and
>> Atmel over to using this instead.
>>
>> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxxxxxx>
>> ---
>
> This looks pretty good. We might want to make it more runtime
> configurable in the future, but this lays a nice groundwork. It's also
> nicely commented to boot. :)
>
> The only thing that concerns me is the locking and reference counting.
> Wouldn't it be sufficient to enable the clock around requests?
Actually that's all it does now, I had it also in mmc_rescan() but don't
think that's necessary (will do some more deep testing before sending
the updated patch).
> Or
> when the host lock is grabbed? Either version would remove the need for
> both clk_users and clk_lock. I think it would also remove a lot of the
> special logic you have.
I have renamed clk_users to clk_requests because that's what it counts
now. Its still needed however: the problem here is that we need to keep
the clock running a number of cycles after the last request, so we solve it
by increasing .clk_requests by one for every request, then decreasing by
one for every request that ends.
Of course requests are serialized, but the request counter is used for the
disablement work to know if any new request came in when we were
delaying for the clk_disable() call, this work will actually not be scheduled
until after a few requests end and provides the necessary hysteresis.
The requests count would not be needed unless we were splitting of a
separate work, but we have to do that in order to have a burst of requests
serviced without waiting 8 MCI clocks inbetween each of them.
The idea here is just to delay set_ios() with freq = 0 until we know for
sure that the current burst of requests is ended.
When I put in some prints I see that there are lots of requests coming
in bursts so this forms a nice "clock on" window around them.
>> diff --git a/drivers/mmc/core/Kconfig b/drivers/mmc/core/Kconfig
>> index ab37a6d..6ae2156 100644
>> --- a/drivers/mmc/core/Kconfig
>> +++ b/drivers/mmc/core/Kconfig
>> @@ -14,3 +14,14 @@ config MMC_UNSAFE_RESUME
>> This option is usually just for embedded systems which use
>> a MMC/SD card for rootfs. Most people should say N here.
>>
>> +config MMC_CLKGATE
>> + bool "MMC host clock gaing (EXPERIMENTAL)"
>> + depends on EXPERIMENTAL
>> + help
>> + This will attempt to agressively gate the clock to the MMC host,
>> + which typically also will gate the MCI clock to the card. This
>> + is done to save power due to gating off the logic and bus noise
>> + when MMC is not in use. Your host driver has to support this in
>> + order for it to be of any use.
>
> The last sense isn't true anymore, is it?
I rewrote it a bit: all drivers that want to perform clock gating still have
to handle the freq field being set to 0 and take apropriate action. This
is not the case when you don't enable clock gating, you get a few
requests with freq set to 0 in the initialization code but once it's set
to something it never goes to 0 again. Most drivers aren't written to
handle frequency 0, some will probably even get a division by 0
error if you try it (just a guess).
>> +
>> + Of unsure, say N.
>
> "If" :)
>
>> +/*
>> + * Internal work. Work to disable the clock at some later point.
>> + */
>> +static void mmc_clk_disable_work(struct work_struct *work)
>> +{
>> + struct mmc_host *host = container_of(work, struct mmc_host,
>> + clk_disable_work);
>> +
>> + mmc_clk_disable_delayed(host);
>> +}
>
> I think I did a bad job explaining my comments about this the last
> time. What I was trying to say was that why have this
> mmc_clk_disable_work() when you could give mmc_clk_disable_delayed()
> directly to the workqueue?
Because mmc_clk_disable_delayed() is also called in mmc_clk_exit()
like this:
if (cancel_work_sync(&host->clk_disable_work))
mmc_clk_disable_delayed(host);
This is if we cancel a pending clock gating and need to make sure that
the clock is eventually gated off before stopping it. In this case if
cancel_work_sync() returns non-zero the work is cancelled but its
pending task still need to be fulfilled so we need to call
mmc_clk_disable_delayed() outside the work, and we cannot just
call the ios with freq set to 0 because we have no idea as to whether
8 MCI cycles (or whatever you configure) have actually passed since
the last request.
>> +/*
>> + * mmc_clk_remove - shut down clock gating code
>> + * @host: host with potential hardware clock to control
>> + */
>> +static inline void mmc_clk_remove(struct mmc_host *host)
>> +{
>> + if (cancel_work_sync(&host->clk_disable_work))
>> + mmc_clk_disable_delayed(host);
>> + BUG_ON(host->clk_users > 0);
>> +}
>
> I'm not sure why you call mmc_clk_disable_delayed() here. Is the clock
> properly enabled again when this function exits? It should be, but the
> disable call there has me confused.
Some previous request may have made the clock go on, then the
request has completed, but the 8 clock cycles have not yet passed.
So we cannot exit MMC until they have finished.
This way the clock gating framework will assure that the set_ios()
is called with freq = 0 before exiting MMC.
>> @@ -80,6 +235,8 @@ struct mmc_host *mmc_alloc_host(int extra, struct
>> device *dev)
>> host->class_dev.class = &mmc_host_class;
>> device_initialize(&host->class_dev);
>>
>> + mmc_clk_alloc(host);
>> +
>> spin_lock_init(&host->lock);
>> init_waitqueue_head(&host->wq);
>> INIT_DELAYED_WORK(&host->detect, mmc_rescan);
>> @@ -156,6 +313,8 @@ void mmc_remove_host(struct mmc_host *host)
>> device_del(&host->class_dev);
>>
>> led_trigger_unregister_simple(host->led);
>> +
>> + mmc_clk_remove(host);
>> }
>>
>> EXPORT_SYMBOL(mmc_remove_host);
>
> alloc and remove don't form a nice pair. I suggest add/remove or
> perhaps init/exit.
OK renamed it *init *exit.
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-embedded" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
