Re: [PATCH V2 11/15] mmc: sdhci: Change to new way of doing re-tuning

[Adrian Hunter <adrian.hunter@intel.com>]

On 27/03/15 11:54, Adrian Hunter wrote:
> On 26/03/15 18:06, Ulf Hansson wrote:
>> On 25 March 2015 at 14:48, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>
>>> On 24/03/15 23:12, Ulf Hansson wrote:
>>>> On 23 March 2015 at 22:11, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>> On 23/03/2015 5:02 p.m., Ulf Hansson wrote:
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>>
>>>>>>> I have no locking issues, so I am not sure what you mean here.
>>>>>>
>>>>>>
>>>>>> Okay, I should have stated race conditions.
>>>>>
>>>>>
>>>>> Which I resolved using runtime get / put calls.
>>>>
>>>> Yes, I noticed that and it works! Though, I would rather avoid to add
>>>> yet another pair of host ops callbacks for this.
>>>>
>>>> What do you think if these options instead?
>>>> 1) Do runtime PM get/put from the host ops ->enable|disable()
>>>> callbacks. As omap_hsmmc does.
>>>> 2) Or even better, do runtime PM get/put directly of the host device
>>>> from mmc_claim|release_host().
>>>
>>> Claiming the host is not directly related to host controller runtime pm. It
>>> is possible to imagine cases for claiming the host for synchronization
>>> reasons that do not need the host controller powered up. And card drivers
>>> could reasonably assume that they can claim the host for long periods
>>> without affecting the runtime pm of the host controller.
>>
>>
>> Yes, it _may_ power up the host controller sometimes when not needed.
>> I don't think this will be an issue, mostly because it should be rare
>> and not happening frequently - if ever.
>>
>> The only users of mmc_claim_host() for SD/MMC is the core itself, so I
>> don't see any issue with misbehaving "card drivers" here.
>>
>> SDIO is again different, since it's up to the SDIO func drivers to
>> behave - as you stated. But, if they don't they anyway need to be
>> fixed, right!?
> 
> Be aware that that links the holding of re-tuning with claiming the host.
> It will then not be allowed for re-tuning to be held when the host is
> released. Will we have to add to mmc_release_host():
> 
> 	WARN_ON(host->hold_count);
> 
> That could be a problem. Say you wanted to start bkops and then release the
> host so that a different context could issue an HPI. That wouldn't be
> allowed anymore.

On the other hand, we would need to prevent the host controller runtime
suspending in that case anyway.

> 
> Generally it is impossible to see all ends, which begs the question: why
> link things if you don't have to?

So I correct myself. Any time we would need to hold re-tuning but release
the host would anyway require preventing runtime suspend of the host
controller. So the requirement:	"don't allow the host controller to runtime
suspend while re-tuning is held" is covered by the requirement "don't allow
the host controller to runtime suspend when it is doing something".

> 
>>
>>>
>>> Currently we have that the host controller driver is the exclusive owner of
>>> runtime pm for the host controller. So the first thing is to decide if we
>>> want to keep that or let the core be involved as well. I would argue for
>>> sticking with the status quo. Let the host controller driver know what is
>>> going on and leave it to do the right thing with its own power management.
>>
>> The problem I see with the current solution, is that we will be
>> scheduling a runtime PM suspend for each an every request towards the
>> host.
>>
>> It's ineffective, due to that we will unnecessary involve the runtime
>> PM core, thus increasing CPU utilization - when we actually don't need
>> to.
>>
>> I will send a patch for this tomorrow, let's discuss it separately.
> 
> Yes please let's keep that separate.
> 
>>
>> [...]
>>
>>>>
>>>> I have thought more about this since yesterday and I somewhat agree
>>>> with your suggestion. Especially in that sense that I think we should
>>>> consider Neil's issue as an "idle operation" for SDIO.
>>>>
>>>> For "idle operations" for MMC/SD cards, runtime PM is already
>>>> deployed. So, I think it's just a matter of extending that support to
>>>> cover more "idle operations" besides the MMC_CAP_AGGRESSIVE_PM thing.
>>>>
>>>> What we need to figure out is how to make this work for SDIO - and
>>>> that's when it becomes more complex. I really would like us to avoid
>>>> exporting new SDIO APIs, unless really needed.
>>>>
>>>> Today runtime PM is deployed by the following SDIO func drivers:
>>>> drivers/net/wireless/libertas/if_sdio.c
>>>> drivers/net/wireless/ti/wl1251/sdio.c
>>>> drivers/net/wireless/ti/wlcore/sdio.c
>>>>
>>>> We _could_ consider to convert above drivers to use something else
>>>> than runtime PM to control the power to the card. I think that would
>>>> be the ideal solution, since then we can deploy runtime PM for SDIO
>>>> fairly similar to how MMC/SDIO is done. That means doing runtime PM
>>>> get/put of the device for the struct mmc_card, inside the mmc core
>>>> while handling SDIO requests.
>>>>
>>>> The above requires some work, both in the mmc core and in the SDIO
>>>> func drivers. The nice thing is that we don't need to export new APIs
>>>> to the SDIO func drivers and we can keep the complexity of dealing
>>>> with "idle operations" inside the mmc core.
>>>>
>>>> Thoughts?
>>>
>>> There isn't necessarily any link between "idle operations" and runtime pm.
>>
>> I think this is exactly what runtime PM is designed for so I don't
>> want us to re-invent something that is specific for mmc.
> 
> Need to remember that PM can theoretically be configured out, which makes
> using it for bkops seem inappropriate.
> 
>>
>>>
>>> For example for eMMC background operations I would expect to see:
>>>
>>> - queue is empty so block driver enables "idle operations".
>>> - core waits (delayed work?) a few milliseconds and then starts bkops.
>>> - a new I/O request arrives, block driver wakes up and tells the core to
>>> stop "idle operations" ASAP, and waits until it does.
>>> - or, the core notifies (callback perhaps) the block driver that "idle
>>> operations" are finished, so the block driver can runtime-put the card
>>>
>>> Also need to stop "idle operations" for system suspend, maybe other places.
>>
>> Conceptual wise, I fully agree. Though, I want to make use of runtime PM.
> 
> So long as it is power management. I am not sure bkops falls into that category.
> 
>>
>> For the eMMC/SD case, the runtime PM suspend callbacks (specified per
>> bus_ops) should be able to act as the "trigger" point to kick off
>> "idle operations".
>>
>> Now, the thing to figure out, is how to execute "idle operations" and
>> at the same time being able to interrupt/abort them from another
>> context. An option for how to deal with this, could be to schedule a
>> "delayed work" from the runtime PM suspend callback. But if we can
>> avoid it, I think we should.
> 
> Whatever the context that runs the idle operations, for the case where a
> driver wants to stop the idle operations ASAP, it would be nice if it could
> simply take over control - particularly if the idle operation is anyway
> waiting for something. So the HPI or whatever is done in the driver context
> directly and the idle operation context (if there even is one at that stage)
> just exits.
> 
>>
>>>
>>> Now in Neil's case there is a relation to runtime pm in that it would be
>>> nice if the switch to 1-bit mode was delayed by the host controllers
>>> autosuspend_delay. But potentially the host controller driver could
>>> routinely set the delay so that it matches the autosuspend_delay anyway.
>>>
>>> Currently the SDIO function drivers all use sdio_claim_host(). So for Neil's
>>> case I would see:
>>>
>>> - host controller driver sees the switch to 4-bit mode and does a runtime
>>> "get" to prevent runtime suspend
>>> - sdio_release_host enables "idle operations"
>>> - the core sees that someone has requested a switch to 1-bit mode after a
>>> certain delay, so it waits that delay (delayed work?) and does the switch
>>> - host controller driver sees the switch to 1-bit mode and runtime suspends
>>> via a pm_runtime_put
>>> - sdio_claim_host tells the core to stop "idle operations" ASAP and waits
>>> until it has
>>> - host controller driver sees the switch to 4-bit mode and does a runtime
>>> "get" to prevent runtime suspend
>>
>> That seems like a way forward!
>>
>> Still I rather would like the above mentioned SDIO func drivers to use
>> something else than runtime PM to control the power to the cards, as I
>> suggested. But that might be too much to fix!?
> 
> Let's keep that a separate issue.
> 
> 
>