Re: [PATCH 1/2] MMC Agressive clocking framework v5

[Linus Walleij <linus.ml.walleij@gmail.com>]

2009/7/23 David Vrabel <david.vrabel@csr.com>:
> Linus Walleij wrote:
>> 2009/7/22 David Vrabel <david.vrabel@csr.com>:

>>> 1. With some controllers (e.g., PXA270 I think) turning the clock on and
>>> off is slow.  This means if you're doing back-to-back commands you
>>> should leave the clock on for best performance.
>>
>> OK, when I've been testing this using the default workqueue and
>> schedule_work() covered these cases. Back-on-back commands
>> seemingly doesn't allow the timeout work to schedule, but I might be
>> overseeing the case of several CPU:s there though :-/
>
> Ok, with a configurable timeout your scheme is fine.  The timeout can be
> extended beyond 8 SDCLKs if this is beneficial.

Yep I have that in debugfs, but when I look at Adrians code I see he instead
added a disable delay field to the host struct so let's use his patch
instead then.

> I'm not sure what the best way to add this would be.  You could:
>
> 1. Have a special clock frequency to mean idle and fix up all existing
> controller drivers to interpret this as 400 kHz unless you know the
> controller handles SDIO interrupts with no SDCLK.
>
> or:
>
> 2. Add an additional controller method (set_bus_state?) and only provide
> this on controller drivers you're interested in.

As discussed with Adrian this is what his patch does (adding a new host->ops
function for enable/disable) so let's use his patch.

>>> 3. Regardless of point 1 above.  Using a workqueue item in this way
>>> seems overkill.  Consider using a timer and simply calling mod_timer()
>>> at the start of every command.  When the timer expires, idle the clock.
>>>  You will probably need a "command in progress" bit to ensure you don't
>>> idle the clock if the timer expires in the middle of a command.
>>
>> I would agree if I created a new workqueue, but the timeout of this
>> particular workqueue is unimportant and that's why I'm using the
>> global workqueue and just schedule_work(). This means no extra
>> overhead, no extra thread and basically does the exact same thing.
>
> You currently queue a work item and wake the workqueue every command.
> This is considerably more overhead (when doing back-to-back commands)
> than simply calling mod_timer().
>
> You also potentially delay for a considerable amount of time in the work
> item.

Yep that's the idea almost... But let's raise the timer debate again with
Adrian's patch instead :-)

Linus Walleij