Re: [PATCH] sdhci: wakeup from runtime PM

[Ulf Hansson]

On 5 April 2016 at 14:40, Adrian Hunter <adrian.hunter@xxxxxxxxx> wrote:
> On 25/03/16 18:05, Ludovic Desroches wrote:
>> Hi,
>>
>> When not using the SDHCI controller, it is logical to save power by suspending
>> it. The issue is that SDHCI assumes that the controller is completely disabled.
>> It means the only way to wake up on a card event is to have a gpio for the card
>> detection (so two pins for the same signal). A possible workaround is to use
>> polling but the controller will be resumed/suspended between each attempts.
>>
>> We have already discussed a long time about this and it seems we don't agree.
>> In my opinion, even if I can't disable all clocks, I should use runtime PM
>> to save some power.
>>
>> I propose two patches, one which is a draft to try to solve it at sdhci level
>> and one at sdhci-of-at91 level.
>>
>> Concerning the first one, I don't understand why we need to reject irqs if
>> runtime_suspended is true.
>
> The interrupt handler might be called because the interrupt is shared i.e.
> the interrupt is for a different device.  In that case the host controller
> might be off and the registers inaccessible.  In that case we cannot even
> look at the interrupt register to determine if we were expecting the interrupt.
>
>>                            Only SDHCI_INT_CARD_INT irq is enabled so why we
>> should have other IRQs than this one?
>
> In the case of SDIO Card interrupt, it is delivered via the host controller,
> so we have to assume the registers are accessible if we are
> runtime-suspended with the SDIO IRQ enabled.
>
>>
>> Since you were not in favour of allowing to wakeup on SDHCI_INT_CARD_INSERT or
>> SDHCI_INT_CARD_REMOVE, I assume you won't take it so I
>> solved my issue only by modifying my driver.
>
> I don't mind allowing card detect interrupts while runtime suspended, but we
> need a flag so that:
> - runtime suspend leaves the insert/remove interrupts enabled
> - irq handler knows it can access registers

To me, this seem like the wrong way of how to configure wake-ups for
these kind of devices.

I don't think the regular IRQs shall be enabled and the driver
shouldn't assume the registers are accessible without first runtime
resuming the device.

> - irq thread handler knows to runtime resume before doing anything else
>
> But it seems like you need to persuade Ulf first.

For a more thorough explanation to why I don't like, please have a
look at my comment for another related thread on the mmc list.
http://www.spinics.net/lists/linux-mmc/msg36132.html

Kind regards
Uffe