Re: [PATCH] mmc: sdhci-of-at91: fix wakeup issue when using runtime pm

[Ulf Hansson]

On 10 March 2016 at 11:30, Ludovic Desroches
 wrote:
> It was impossible to wake-up on card detect event because when sdhci
> controller is runtime suspend, it is assumed that all the clocks are
> disabled so we can't get irqs.
> If the device is removable and there is no gpio to manage the card
> detection then card detection polling is used.
>
> Signed-off-by: Ludovic Desroches 
> ---
>
> Hi Ulf, Adrian,
>
> Following the discussion, I need to fix my issue. I think we could both agree
> on this patch that I see more as temporary workaround.
>
> I will try to change the muxing of the card detect pio in order to no more
> use the sdhci controller to manage it but a gpio. If it do not work then
> I may send another patch in the spirit of the previous one (keeping one clock
> enabled and not calling sdhci_runtime_suspend_host()).
>
> Regards
>
>  drivers/mmc/host/sdhci-of-at91.c | 16 
>  1 file changed, 16 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci-of-at91.c 
> b/drivers/mmc/host/sdhci-of-at91.c
> index 35c02fc..dac8508 100644
> --- a/drivers/mmc/host/sdhci-of-at91.c
> +++ b/drivers/mmc/host/sdhci-of-at91.c
> @@ -18,6 +18,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -204,6 +205,21 @@ static int sdhci_at91_probe(struct platform_device *pdev)
> if (ret)
> goto pm_runtime_disable;
>
> +   /*
> +* When calling sdhci_runtime_suspend_host(), the sdhci layer makes
> +* the assumption that all the clocks of the controller are disabled.
> +* It means we can't get irq from it when it is runtime suspended.
> +* For that reason, it is not planned to wake-up on a card detect irq
> +* from the controller.
> +* If we want to use runtime PM and to be able to wake-up on card
> +* insertion, we have to use a GPIO for the card detection or we can
> +* use polling for the card detection. Be aware that using polling
> +* will resume/suspend the controller between each attempt.
> +*/
> +   if (!(host->mmc->caps & MMC_CAP_NONREMOVABLE) &&
> +   IS_ERR_VALUE(mmc_gpio_get_cd(host->mmc)))
> +   host->mmc->caps |= MMC_CAP_NEEDS_POLL;

Perhaps also clearing SDHCI_QUIRK_BROKEN_CARD_DETECTION, as this
doesn't play well if this is set via parsing the DTS.

> +
> pm_runtime_put_autosuspend(>dev);
>
> return 0;
> --
> 2.5.0
>

Otherwise this looks okay to me.

Kind regards
Uffe

Re: [PATCH] mmc: sdhci-of-at91: fix wakeup issue when using runtime pm

[Ulf Hansson]

On 10 March 2016 at 11:30, Ludovic Desroches
 wrote:
> It was impossible to wake-up on card detect event because when sdhci
> controller is runtime suspend, it is assumed that all the clocks are
> disabled so we can't get irqs.
> If the device is removable and there is no gpio to manage the card
> detection then card detection polling is used.
>
> Signed-off-by: Ludovic Desroches 
> ---
>
> Hi Ulf, Adrian,
>
> Following the discussion, I need to fix my issue. I think we could both agree
> on this patch that I see more as temporary workaround.
>
> I will try to change the muxing of the card detect pio in order to no more
> use the sdhci controller to manage it but a gpio. If it do not work then
> I may send another patch in the spirit of the previous one (keeping one clock
> enabled and not calling sdhci_runtime_suspend_host()).
>
> Regards
>
>  drivers/mmc/host/sdhci-of-at91.c | 16 
>  1 file changed, 16 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci-of-at91.c 
> b/drivers/mmc/host/sdhci-of-at91.c
> index 35c02fc..dac8508 100644
> --- a/drivers/mmc/host/sdhci-of-at91.c
> +++ b/drivers/mmc/host/sdhci-of-at91.c
> @@ -18,6 +18,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -204,6 +205,21 @@ static int sdhci_at91_probe(struct platform_device *pdev)
> if (ret)
> goto pm_runtime_disable;
>
> +   /*
> +* When calling sdhci_runtime_suspend_host(), the sdhci layer makes
> +* the assumption that all the clocks of the controller are disabled.
> +* It means we can't get irq from it when it is runtime suspended.
> +* For that reason, it is not planned to wake-up on a card detect irq
> +* from the controller.
> +* If we want to use runtime PM and to be able to wake-up on card
> +* insertion, we have to use a GPIO for the card detection or we can
> +* use polling for the card detection. Be aware that using polling
> +* will resume/suspend the controller between each attempt.
> +*/
> +   if (!(host->mmc->caps & MMC_CAP_NONREMOVABLE) &&
> +   IS_ERR_VALUE(mmc_gpio_get_cd(host->mmc)))
> +   host->mmc->caps |= MMC_CAP_NEEDS_POLL;

Perhaps also clearing SDHCI_QUIRK_BROKEN_CARD_DETECTION, as this
doesn't play well if this is set via parsing the DTS.

> +
> pm_runtime_put_autosuspend(>dev);
>
> return 0;
> --
> 2.5.0
>

Otherwise this looks okay to me.

Kind regards
Uffe

Re: [PATCH] mmc: sdhci-of-at91: fix wakeup issue when using runtime pm

[Ludovic Desroches]

On Thu, Mar 10, 2016 at 11:30:58AM +0100, Ludovic Desroches wrote:
> It was impossible to wake-up on card detect event because when sdhci
> controller is runtime suspend, it is assumed that all the clocks are
> disabled so we can't get irqs.
> If the device is removable and there is no gpio to manage the card
> detection then card detection polling is used.
> 
> Signed-off-by: Ludovic Desroches 
> ---
> 
> Hi Ulf, Adrian,
> 
> Following the discussion, I need to fix my issue. I think we could both agree
> on this patch that I see more as temporary workaround.
> 
> I will try to change the muxing of the card detect pio in order to no more
> use the sdhci controller to manage it but a gpio. If it do not work then
> I may send another patch in the spirit of the previous one (keeping one clock
> enabled and not calling sdhci_runtime_suspend_host()).

I have changed the muxing for the card detect signal to get it on a gpio
instead of the card detect pio (peripheral io) of the controller. I thought
it can work because we have a 'force card detect' bit. Unfortunately, it is
resetted when we perform a software reset. Moreover I need to keep the
card detect pio, because it used to perform fast startup from ultra low power
modes.

Maybe it could work by using an extra pin but it invovles to do hardawre
changes on the boards and needing two pins for one signal is not acceptable.

I'll do more tests to use runtime pm without polling for card detection
and without using sdhci_runtime_suspend_host().


> 
> Regards
> 
>  drivers/mmc/host/sdhci-of-at91.c | 16 
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-of-at91.c 
> b/drivers/mmc/host/sdhci-of-at91.c
> index 35c02fc..dac8508 100644
> --- a/drivers/mmc/host/sdhci-of-at91.c
> +++ b/drivers/mmc/host/sdhci-of-at91.c
> @@ -18,6 +18,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -204,6 +205,21 @@ static int sdhci_at91_probe(struct platform_device *pdev)
>   if (ret)
>   goto pm_runtime_disable;
>  
> + /*
> +  * When calling sdhci_runtime_suspend_host(), the sdhci layer makes
> +  * the assumption that all the clocks of the controller are disabled.
> +  * It means we can't get irq from it when it is runtime suspended.
> +  * For that reason, it is not planned to wake-up on a card detect irq
> +  * from the controller.
> +  * If we want to use runtime PM and to be able to wake-up on card
> +  * insertion, we have to use a GPIO for the card detection or we can
> +  * use polling for the card detection. Be aware that using polling
> +  * will resume/suspend the controller between each attempt.
> +  */
> + if (!(host->mmc->caps & MMC_CAP_NONREMOVABLE) &&
> + IS_ERR_VALUE(mmc_gpio_get_cd(host->mmc)))
> + host->mmc->caps |= MMC_CAP_NEEDS_POLL;
> +
>   pm_runtime_put_autosuspend(>dev);
>  
>   return 0;
> -- 
> 2.5.0
> 

Re: [PATCH] mmc: sdhci-of-at91: fix wakeup issue when using runtime pm

[Ludovic Desroches]

On Thu, Mar 10, 2016 at 11:30:58AM +0100, Ludovic Desroches wrote:
> It was impossible to wake-up on card detect event because when sdhci
> controller is runtime suspend, it is assumed that all the clocks are
> disabled so we can't get irqs.
> If the device is removable and there is no gpio to manage the card
> detection then card detection polling is used.
> 
> Signed-off-by: Ludovic Desroches 
> ---
> 
> Hi Ulf, Adrian,
> 
> Following the discussion, I need to fix my issue. I think we could both agree
> on this patch that I see more as temporary workaround.
> 
> I will try to change the muxing of the card detect pio in order to no more
> use the sdhci controller to manage it but a gpio. If it do not work then
> I may send another patch in the spirit of the previous one (keeping one clock
> enabled and not calling sdhci_runtime_suspend_host()).

I have changed the muxing for the card detect signal to get it on a gpio
instead of the card detect pio (peripheral io) of the controller. I thought
it can work because we have a 'force card detect' bit. Unfortunately, it is
resetted when we perform a software reset. Moreover I need to keep the
card detect pio, because it used to perform fast startup from ultra low power
modes.

Maybe it could work by using an extra pin but it invovles to do hardawre
changes on the boards and needing two pins for one signal is not acceptable.

I'll do more tests to use runtime pm without polling for card detection
and without using sdhci_runtime_suspend_host().


> 
> Regards
> 
>  drivers/mmc/host/sdhci-of-at91.c | 16 
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-of-at91.c 
> b/drivers/mmc/host/sdhci-of-at91.c
> index 35c02fc..dac8508 100644
> --- a/drivers/mmc/host/sdhci-of-at91.c
> +++ b/drivers/mmc/host/sdhci-of-at91.c
> @@ -18,6 +18,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -204,6 +205,21 @@ static int sdhci_at91_probe(struct platform_device *pdev)
>   if (ret)
>   goto pm_runtime_disable;
>  
> + /*
> +  * When calling sdhci_runtime_suspend_host(), the sdhci layer makes
> +  * the assumption that all the clocks of the controller are disabled.
> +  * It means we can't get irq from it when it is runtime suspended.
> +  * For that reason, it is not planned to wake-up on a card detect irq
> +  * from the controller.
> +  * If we want to use runtime PM and to be able to wake-up on card
> +  * insertion, we have to use a GPIO for the card detection or we can
> +  * use polling for the card detection. Be aware that using polling
> +  * will resume/suspend the controller between each attempt.
> +  */
> + if (!(host->mmc->caps & MMC_CAP_NONREMOVABLE) &&
> + IS_ERR_VALUE(mmc_gpio_get_cd(host->mmc)))
> + host->mmc->caps |= MMC_CAP_NEEDS_POLL;
> +
>   pm_runtime_put_autosuspend(>dev);
>  
>   return 0;
> -- 
> 2.5.0
> 

Re: [PATCH] mmc: sdhci-of-at91: fix wakeup issue when using runtime pm

[Ludovic Desroches]

+ PM mailing list since the discussion is mixing PM and sdhci

On Tue, Mar 08, 2016 at 10:56:31PM +0100, Ulf Hansson wrote:
> +Ludovic
> 
> On 8 March 2016 at 22:54, Ulf Hansson  wrote:
> > On 4 March 2016 at 14:48, Ludovic Desroches  
> > wrote:

[snip]

> >>
> >> Coming back to the initial discussion and patch which were motivated by
> >> the fact that after runtime suspend I can't wake-up on card detect event,
> >> I have the feeling we don't have the same assumption about runtime PM.
> >>
> >> From what you and Adrian told me, I should not use runtime PM if I have
> >> no way to wake-up. In your minds, the way to wake-up is to use an
> >> externel GPIO because the controller will be 'totally' disabled, isn't it?
> >
> > I agree to the first part here.
> >
> > Although, as you also have the option to use polling for card detect,
> > this actually means you don't really *need* to have a wakeup
> > configured. Especially in the case where you don't have GPIO card
> > detect.
> >
> > In that way, *all* the clocks can gated in between the polling
> > attempts, thus you will save power even in the polling mode
> > configuration and when runtime PM is enabled.
> >

As you said, I can gate all the clocks but only between attempts.
Regularly, I will resume and suspend my controller to perform the card
detection. I don't know if it is a better solution than keeping one
clock enabled and waiting for an interrupt to enable the other ones.

> >>
> >> On my side, runtime PM allows me to save power when the sdhci controller
> >> is not used. If I can disable two clocks out of three, I should use
> >> runtime PM. Do you agree?
> >>
> >> If not, tell me how I can convince you :) Otherwise, next step is to rework
> >> my patch but I think I have no other solution that not calling
> >> sdhci_runtime_suspend_host if I expect to use the card detect irq of the
> >> controller.
> >
> > So, to summarize.
> >
> > I think the best fix is to add a clever check in ->probe() and then
> > enable polling when you can't rely on GPIO card detect IRQ.
> >
> > Moreover, to have a robust solution, you also need to clear
> > SDHCI_QUIRK_BROKEN_CARD_DETECTION for your sdhci variant, as otherwise
> > the "broken-cd" DT binding could wrongly be used for this variant.
> >
> > Does that make sense?


Well I don't agree totally because I have the feeling to use a
workaround because if sdhci_runtime_suspend_host has been called the
controlled is considered as 'disabled' then we won't deal with its IRQs.

I would like to move forward on this issue so it could be a trade-off.

Since the sdhci layer is going to be reworked, maybe it could be
interesting to have another approach of runtime PM to get more
flexibility.


Regards

Ludovic

Re: [PATCH] mmc: sdhci-of-at91: fix wakeup issue when using runtime pm

[Ludovic Desroches]

+ PM mailing list since the discussion is mixing PM and sdhci

On Tue, Mar 08, 2016 at 10:56:31PM +0100, Ulf Hansson wrote:
> +Ludovic
> 
> On 8 March 2016 at 22:54, Ulf Hansson  wrote:
> > On 4 March 2016 at 14:48, Ludovic Desroches  
> > wrote:

[snip]

> >>
> >> Coming back to the initial discussion and patch which were motivated by
> >> the fact that after runtime suspend I can't wake-up on card detect event,
> >> I have the feeling we don't have the same assumption about runtime PM.
> >>
> >> From what you and Adrian told me, I should not use runtime PM if I have
> >> no way to wake-up. In your minds, the way to wake-up is to use an
> >> externel GPIO because the controller will be 'totally' disabled, isn't it?
> >
> > I agree to the first part here.
> >
> > Although, as you also have the option to use polling for card detect,
> > this actually means you don't really *need* to have a wakeup
> > configured. Especially in the case where you don't have GPIO card
> > detect.
> >
> > In that way, *all* the clocks can gated in between the polling
> > attempts, thus you will save power even in the polling mode
> > configuration and when runtime PM is enabled.
> >

As you said, I can gate all the clocks but only between attempts.
Regularly, I will resume and suspend my controller to perform the card
detection. I don't know if it is a better solution than keeping one
clock enabled and waiting for an interrupt to enable the other ones.

> >>
> >> On my side, runtime PM allows me to save power when the sdhci controller
> >> is not used. If I can disable two clocks out of three, I should use
> >> runtime PM. Do you agree?
> >>
> >> If not, tell me how I can convince you :) Otherwise, next step is to rework
> >> my patch but I think I have no other solution that not calling
> >> sdhci_runtime_suspend_host if I expect to use the card detect irq of the
> >> controller.
> >
> > So, to summarize.
> >
> > I think the best fix is to add a clever check in ->probe() and then
> > enable polling when you can't rely on GPIO card detect IRQ.
> >
> > Moreover, to have a robust solution, you also need to clear
> > SDHCI_QUIRK_BROKEN_CARD_DETECTION for your sdhci variant, as otherwise
> > the "broken-cd" DT binding could wrongly be used for this variant.
> >
> > Does that make sense?


Well I don't agree totally because I have the feeling to use a
workaround because if sdhci_runtime_suspend_host has been called the
controlled is considered as 'disabled' then we won't deal with its IRQs.

I would like to move forward on this issue so it could be a trade-off.

Since the sdhci layer is going to be reworked, maybe it could be
interesting to have another approach of runtime PM to get more
flexibility.


Regards

Ludovic

Re: [PATCH] mmc: sdhci-of-at91: fix wakeup issue when using runtime pm

[Ulf Hansson]

+Ludovic

On 8 March 2016 at 22:54, Ulf Hansson  wrote:
> On 4 March 2016 at 14:48, Ludovic Desroches  
> wrote:
>> Hi Ulf,
>>
>> On Fri, Mar 04, 2016 at 10:09:37AM +0100, Ulf Hansson wrote:
>>> On 17 February 2016 at 11:35, Ludovic Desroches
>>
>> [snip]
>>
>>> > I am wondering if I should take account of sdio irq enabled or not here.
>>> >
>>> > I have a sdio device which drives me crazy because of power management.
>>> > The driver of this device is in staging, it is wilc1000. It seems that I
>>> > am stuck because the sdio irq are not received. If I don't disable the
>>> > clock of the controller (hclock), I should receive the sdio IRQ as I
>>> > receive card detect ones, isn't it?
>>> >
>>> > It doesn't work, it seems I have also to not disabled mainck and gck
>>> > which are clocks needed to generate the clock sent to the sdio device.
>>> > If none of the clocks have to be disabled, where it has to be managed?
>>>
>>> That's a typical issue for SDIO IRQs, especially when the controller
>>> HW manages IRQs (there are other ways to deal with SDIO IRQs as well).
>>>
>>> Currently, the simplest way to deal with this in the driver is to do a
>>> pm_runtime_get_sync() when the SDIO IRQ gets enabled, and
>>> pm_runtime_put() when it gets disabled.
>>
>> Which driver? sdio device driver or controller driver?
>
> In the mmc controller driver.
>
>>
>>> >
>>> > Do I have to anticipate this use case in the driver of my sdhci
>>> > controller or does it have to be managed in the sdio device driver? They
>>> > are using sdio_claim/release_host to suspend or resume the host but
>>> > maybe they use it in a bad way.
>>>
>>> The wilc100 SDIO func driver should *not* keep the host claimed to
>>> deal with SDIO irqs. Only when it configures them.
>>>
>>> Instead, you need to deal with this in the sdhci driver, when you get
>>> the call to enable/disable SDIO IRQs.
>>>
>>
>> Do you mean in sdhci_enable_sdio_irq?
>
> I am not sure exactly where to check. As it may be depending on the
> sdhci variant and the SoC you probably need to deal with this in
> non-common sdhci code.
>
>>
>>> Moreover, from a system PM point of view. If the wilc100 SDIO func
>>> driver wants the platform to wake up on SDIO IRQs, it needs to set
>>> MMC_PM_KEEP_POWER and MMC_PM_WAKE_SDIO_IRQ from its ->suspend()
>>> callback.
>>>
>>> In that way, your sdhci driver can act accordingly from its system PM
>>> callbacks. In other words, depending on MMC_PM_KEEP_POWER and
>>> MMC_PM_WAKE_SDIO_IRQ to *not* call pm_runtime_force_suspend().
>>>
>>
>> Ok, it makes sense.
>>
>>
>> Coming back to the initial discussion and patch which were motivated by
>> the fact that after runtime suspend I can't wake-up on card detect event,
>> I have the feeling we don't have the same assumption about runtime PM.
>>
>> From what you and Adrian told me, I should not use runtime PM if I have
>> no way to wake-up. In your minds, the way to wake-up is to use an
>> externel GPIO because the controller will be 'totally' disabled, isn't it?
>
> I agree to the first part here.
>
> Although, as you also have the option to use polling for card detect,
> this actually means you don't really *need* to have a wakeup
> configured. Especially in the case where you don't have GPIO card
> detect.
>
> In that way, *all* the clocks can gated in between the polling
> attempts, thus you will save power even in the polling mode
> configuration and when runtime PM is enabled.
>
>>
>> On my side, runtime PM allows me to save power when the sdhci controller
>> is not used. If I can disable two clocks out of three, I should use
>> runtime PM. Do you agree?
>>
>> If not, tell me how I can convince you :) Otherwise, next step is to rework
>> my patch but I think I have no other solution that not calling
>> sdhci_runtime_suspend_host if I expect to use the card detect irq of the
>> controller.
>
> So, to summarize.
>
> I think the best fix is to add a clever check in ->probe() and then
> enable polling when you can't rely on GPIO card detect IRQ.
>
> Moreover, to have a robust solution, you also need to clear
> SDHCI_QUIRK_BROKEN_CARD_DETECTION for your sdhci variant, as otherwise
> the "broken-cd" DT binding could wrongly be used for this variant.
>
> Does that make sense?
>
> Kind regards
> Uffe

Re: [PATCH] mmc: sdhci-of-at91: fix wakeup issue when using runtime pm

[Ulf Hansson]

+Ludovic

On 8 March 2016 at 22:54, Ulf Hansson  wrote:
> On 4 March 2016 at 14:48, Ludovic Desroches  
> wrote:
>> Hi Ulf,
>>
>> On Fri, Mar 04, 2016 at 10:09:37AM +0100, Ulf Hansson wrote:
>>> On 17 February 2016 at 11:35, Ludovic Desroches
>>
>> [snip]
>>
>>> > I am wondering if I should take account of sdio irq enabled or not here.
>>> >
>>> > I have a sdio device which drives me crazy because of power management.
>>> > The driver of this device is in staging, it is wilc1000. It seems that I
>>> > am stuck because the sdio irq are not received. If I don't disable the
>>> > clock of the controller (hclock), I should receive the sdio IRQ as I
>>> > receive card detect ones, isn't it?
>>> >
>>> > It doesn't work, it seems I have also to not disabled mainck and gck
>>> > which are clocks needed to generate the clock sent to the sdio device.
>>> > If none of the clocks have to be disabled, where it has to be managed?
>>>
>>> That's a typical issue for SDIO IRQs, especially when the controller
>>> HW manages IRQs (there are other ways to deal with SDIO IRQs as well).
>>>
>>> Currently, the simplest way to deal with this in the driver is to do a
>>> pm_runtime_get_sync() when the SDIO IRQ gets enabled, and
>>> pm_runtime_put() when it gets disabled.
>>
>> Which driver? sdio device driver or controller driver?
>
> In the mmc controller driver.
>
>>
>>> >
>>> > Do I have to anticipate this use case in the driver of my sdhci
>>> > controller or does it have to be managed in the sdio device driver? They
>>> > are using sdio_claim/release_host to suspend or resume the host but
>>> > maybe they use it in a bad way.
>>>
>>> The wilc100 SDIO func driver should *not* keep the host claimed to
>>> deal with SDIO irqs. Only when it configures them.
>>>
>>> Instead, you need to deal with this in the sdhci driver, when you get
>>> the call to enable/disable SDIO IRQs.
>>>
>>
>> Do you mean in sdhci_enable_sdio_irq?
>
> I am not sure exactly where to check. As it may be depending on the
> sdhci variant and the SoC you probably need to deal with this in
> non-common sdhci code.
>
>>
>>> Moreover, from a system PM point of view. If the wilc100 SDIO func
>>> driver wants the platform to wake up on SDIO IRQs, it needs to set
>>> MMC_PM_KEEP_POWER and MMC_PM_WAKE_SDIO_IRQ from its ->suspend()
>>> callback.
>>>
>>> In that way, your sdhci driver can act accordingly from its system PM
>>> callbacks. In other words, depending on MMC_PM_KEEP_POWER and
>>> MMC_PM_WAKE_SDIO_IRQ to *not* call pm_runtime_force_suspend().
>>>
>>
>> Ok, it makes sense.
>>
>>
>> Coming back to the initial discussion and patch which were motivated by
>> the fact that after runtime suspend I can't wake-up on card detect event,
>> I have the feeling we don't have the same assumption about runtime PM.
>>
>> From what you and Adrian told me, I should not use runtime PM if I have
>> no way to wake-up. In your minds, the way to wake-up is to use an
>> externel GPIO because the controller will be 'totally' disabled, isn't it?
>
> I agree to the first part here.
>
> Although, as you also have the option to use polling for card detect,
> this actually means you don't really *need* to have a wakeup
> configured. Especially in the case where you don't have GPIO card
> detect.
>
> In that way, *all* the clocks can gated in between the polling
> attempts, thus you will save power even in the polling mode
> configuration and when runtime PM is enabled.
>
>>
>> On my side, runtime PM allows me to save power when the sdhci controller
>> is not used. If I can disable two clocks out of three, I should use
>> runtime PM. Do you agree?
>>
>> If not, tell me how I can convince you :) Otherwise, next step is to rework
>> my patch but I think I have no other solution that not calling
>> sdhci_runtime_suspend_host if I expect to use the card detect irq of the
>> controller.
>
> So, to summarize.
>
> I think the best fix is to add a clever check in ->probe() and then
> enable polling when you can't rely on GPIO card detect IRQ.
>
> Moreover, to have a robust solution, you also need to clear
> SDHCI_QUIRK_BROKEN_CARD_DETECTION for your sdhci variant, as otherwise
> the "broken-cd" DT binding could wrongly be used for this variant.
>
> Does that make sense?
>
> Kind regards
> Uffe

Re: [PATCH] mmc: sdhci-of-at91: fix wakeup issue when using runtime pm

[Ulf Hansson]

On 4 March 2016 at 14:48, Ludovic Desroches  wrote:
> Hi Ulf,
>
> On Fri, Mar 04, 2016 at 10:09:37AM +0100, Ulf Hansson wrote:
>> On 17 February 2016 at 11:35, Ludovic Desroches
>
> [snip]
>
>> > I am wondering if I should take account of sdio irq enabled or not here.
>> >
>> > I have a sdio device which drives me crazy because of power management.
>> > The driver of this device is in staging, it is wilc1000. It seems that I
>> > am stuck because the sdio irq are not received. If I don't disable the
>> > clock of the controller (hclock), I should receive the sdio IRQ as I
>> > receive card detect ones, isn't it?
>> >
>> > It doesn't work, it seems I have also to not disabled mainck and gck
>> > which are clocks needed to generate the clock sent to the sdio device.
>> > If none of the clocks have to be disabled, where it has to be managed?
>>
>> That's a typical issue for SDIO IRQs, especially when the controller
>> HW manages IRQs (there are other ways to deal with SDIO IRQs as well).
>>
>> Currently, the simplest way to deal with this in the driver is to do a
>> pm_runtime_get_sync() when the SDIO IRQ gets enabled, and
>> pm_runtime_put() when it gets disabled.
>
> Which driver? sdio device driver or controller driver?

In the mmc controller driver.

>
>> >
>> > Do I have to anticipate this use case in the driver of my sdhci
>> > controller or does it have to be managed in the sdio device driver? They
>> > are using sdio_claim/release_host to suspend or resume the host but
>> > maybe they use it in a bad way.
>>
>> The wilc100 SDIO func driver should *not* keep the host claimed to
>> deal with SDIO irqs. Only when it configures them.
>>
>> Instead, you need to deal with this in the sdhci driver, when you get
>> the call to enable/disable SDIO IRQs.
>>
>
> Do you mean in sdhci_enable_sdio_irq?

I am not sure exactly where to check. As it may be depending on the
sdhci variant and the SoC you probably need to deal with this in
non-common sdhci code.

>
>> Moreover, from a system PM point of view. If the wilc100 SDIO func
>> driver wants the platform to wake up on SDIO IRQs, it needs to set
>> MMC_PM_KEEP_POWER and MMC_PM_WAKE_SDIO_IRQ from its ->suspend()
>> callback.
>>
>> In that way, your sdhci driver can act accordingly from its system PM
>> callbacks. In other words, depending on MMC_PM_KEEP_POWER and
>> MMC_PM_WAKE_SDIO_IRQ to *not* call pm_runtime_force_suspend().
>>
>
> Ok, it makes sense.
>
>
> Coming back to the initial discussion and patch which were motivated by
> the fact that after runtime suspend I can't wake-up on card detect event,
> I have the feeling we don't have the same assumption about runtime PM.
>
> From what you and Adrian told me, I should not use runtime PM if I have
> no way to wake-up. In your minds, the way to wake-up is to use an
> externel GPIO because the controller will be 'totally' disabled, isn't it?

I agree to the first part here.

Although, as you also have the option to use polling for card detect,
this actually means you don't really *need* to have a wakeup
configured. Especially in the case where you don't have GPIO card
detect.

In that way, *all* the clocks can gated in between the polling
attempts, thus you will save power even in the polling mode
configuration and when runtime PM is enabled.

>
> On my side, runtime PM allows me to save power when the sdhci controller
> is not used. If I can disable two clocks out of three, I should use
> runtime PM. Do you agree?
>
> If not, tell me how I can convince you :) Otherwise, next step is to rework
> my patch but I think I have no other solution that not calling
> sdhci_runtime_suspend_host if I expect to use the card detect irq of the
> controller.

So, to summarize.

I think the best fix is to add a clever check in ->probe() and then
enable polling when you can't rely on GPIO card detect IRQ.

Moreover, to have a robust solution, you also need to clear
SDHCI_QUIRK_BROKEN_CARD_DETECTION for your sdhci variant, as otherwise
the "broken-cd" DT binding could wrongly be used for this variant.

Does that make sense?

Kind regards
Uffe

Re: [PATCH] mmc: sdhci-of-at91: fix wakeup issue when using runtime pm

[Ulf Hansson]

On 4 March 2016 at 14:48, Ludovic Desroches  wrote:
> Hi Ulf,
>
> On Fri, Mar 04, 2016 at 10:09:37AM +0100, Ulf Hansson wrote:
>> On 17 February 2016 at 11:35, Ludovic Desroches
>
> [snip]
>
>> > I am wondering if I should take account of sdio irq enabled or not here.
>> >
>> > I have a sdio device which drives me crazy because of power management.
>> > The driver of this device is in staging, it is wilc1000. It seems that I
>> > am stuck because the sdio irq are not received. If I don't disable the
>> > clock of the controller (hclock), I should receive the sdio IRQ as I
>> > receive card detect ones, isn't it?
>> >
>> > It doesn't work, it seems I have also to not disabled mainck and gck
>> > which are clocks needed to generate the clock sent to the sdio device.
>> > If none of the clocks have to be disabled, where it has to be managed?
>>
>> That's a typical issue for SDIO IRQs, especially when the controller
>> HW manages IRQs (there are other ways to deal with SDIO IRQs as well).
>>
>> Currently, the simplest way to deal with this in the driver is to do a
>> pm_runtime_get_sync() when the SDIO IRQ gets enabled, and
>> pm_runtime_put() when it gets disabled.
>
> Which driver? sdio device driver or controller driver?

In the mmc controller driver.

>
>> >
>> > Do I have to anticipate this use case in the driver of my sdhci
>> > controller or does it have to be managed in the sdio device driver? They
>> > are using sdio_claim/release_host to suspend or resume the host but
>> > maybe they use it in a bad way.
>>
>> The wilc100 SDIO func driver should *not* keep the host claimed to
>> deal with SDIO irqs. Only when it configures them.
>>
>> Instead, you need to deal with this in the sdhci driver, when you get
>> the call to enable/disable SDIO IRQs.
>>
>
> Do you mean in sdhci_enable_sdio_irq?

I am not sure exactly where to check. As it may be depending on the
sdhci variant and the SoC you probably need to deal with this in
non-common sdhci code.

>
>> Moreover, from a system PM point of view. If the wilc100 SDIO func
>> driver wants the platform to wake up on SDIO IRQs, it needs to set
>> MMC_PM_KEEP_POWER and MMC_PM_WAKE_SDIO_IRQ from its ->suspend()
>> callback.
>>
>> In that way, your sdhci driver can act accordingly from its system PM
>> callbacks. In other words, depending on MMC_PM_KEEP_POWER and
>> MMC_PM_WAKE_SDIO_IRQ to *not* call pm_runtime_force_suspend().
>>
>
> Ok, it makes sense.
>
>
> Coming back to the initial discussion and patch which were motivated by
> the fact that after runtime suspend I can't wake-up on card detect event,
> I have the feeling we don't have the same assumption about runtime PM.
>
> From what you and Adrian told me, I should not use runtime PM if I have
> no way to wake-up. In your minds, the way to wake-up is to use an
> externel GPIO because the controller will be 'totally' disabled, isn't it?

I agree to the first part here.

Although, as you also have the option to use polling for card detect,
this actually means you don't really *need* to have a wakeup
configured. Especially in the case where you don't have GPIO card
detect.

In that way, *all* the clocks can gated in between the polling
attempts, thus you will save power even in the polling mode
configuration and when runtime PM is enabled.

>
> On my side, runtime PM allows me to save power when the sdhci controller
> is not used. If I can disable two clocks out of three, I should use
> runtime PM. Do you agree?
>
> If not, tell me how I can convince you :) Otherwise, next step is to rework
> my patch but I think I have no other solution that not calling
> sdhci_runtime_suspend_host if I expect to use the card detect irq of the
> controller.

So, to summarize.

I think the best fix is to add a clever check in ->probe() and then
enable polling when you can't rely on GPIO card detect IRQ.

Moreover, to have a robust solution, you also need to clear
SDHCI_QUIRK_BROKEN_CARD_DETECTION for your sdhci variant, as otherwise
the "broken-cd" DT binding could wrongly be used for this variant.

Does that make sense?

Kind regards
Uffe

Re: [PATCH] mmc: sdhci-of-at91: fix wakeup issue when using runtime pm

[Ludovic Desroches]

Hi Ulf,

On Fri, Mar 04, 2016 at 10:09:37AM +0100, Ulf Hansson wrote:
> On 17 February 2016 at 11:35, Ludovic Desroches

[snip]

> > I am wondering if I should take account of sdio irq enabled or not here.
> >
> > I have a sdio device which drives me crazy because of power management.
> > The driver of this device is in staging, it is wilc1000. It seems that I
> > am stuck because the sdio irq are not received. If I don't disable the
> > clock of the controller (hclock), I should receive the sdio IRQ as I
> > receive card detect ones, isn't it?
> >
> > It doesn't work, it seems I have also to not disabled mainck and gck
> > which are clocks needed to generate the clock sent to the sdio device.
> > If none of the clocks have to be disabled, where it has to be managed?
> 
> That's a typical issue for SDIO IRQs, especially when the controller
> HW manages IRQs (there are other ways to deal with SDIO IRQs as well).
> 
> Currently, the simplest way to deal with this in the driver is to do a
> pm_runtime_get_sync() when the SDIO IRQ gets enabled, and
> pm_runtime_put() when it gets disabled.

Which driver? sdio device driver or controller driver?

> >
> > Do I have to anticipate this use case in the driver of my sdhci
> > controller or does it have to be managed in the sdio device driver? They
> > are using sdio_claim/release_host to suspend or resume the host but
> > maybe they use it in a bad way.
> 
> The wilc100 SDIO func driver should *not* keep the host claimed to
> deal with SDIO irqs. Only when it configures them.
> 
> Instead, you need to deal with this in the sdhci driver, when you get
> the call to enable/disable SDIO IRQs.
> 

Do you mean in sdhci_enable_sdio_irq?

> Moreover, from a system PM point of view. If the wilc100 SDIO func
> driver wants the platform to wake up on SDIO IRQs, it needs to set
> MMC_PM_KEEP_POWER and MMC_PM_WAKE_SDIO_IRQ from its ->suspend()
> callback.
> 
> In that way, your sdhci driver can act accordingly from its system PM
> callbacks. In other words, depending on MMC_PM_KEEP_POWER and
> MMC_PM_WAKE_SDIO_IRQ to *not* call pm_runtime_force_suspend().
> 

Ok, it makes sense.


Coming back to the initial discussion and patch which were motivated by
the fact that after runtime suspend I can't wake-up on card detect event,
I have the feeling we don't have the same assumption about runtime PM.

>From what you and Adrian told me, I should not use runtime PM if I have
no way to wake-up. In your minds, the way to wake-up is to use an
externel GPIO because the controller will be 'totally' disabled, isn't it?

On my side, runtime PM allows me to save power when the sdhci controller
is not used. If I can disable two clocks out of three, I should use
runtime PM. Do you agree?

If not, tell me how I can convince you :) Otherwise, next step is to rework
my patch but I think I have no other solution that not calling
sdhci_runtime_suspend_host if I expect to use the card detect irq of the
controller.

Regards

Ludovic

Re: [PATCH] mmc: sdhci-of-at91: fix wakeup issue when using runtime pm

[Ludovic Desroches]

Hi Ulf,

On Fri, Mar 04, 2016 at 10:09:37AM +0100, Ulf Hansson wrote:
> On 17 February 2016 at 11:35, Ludovic Desroches

[snip]

> > I am wondering if I should take account of sdio irq enabled or not here.
> >
> > I have a sdio device which drives me crazy because of power management.
> > The driver of this device is in staging, it is wilc1000. It seems that I
> > am stuck because the sdio irq are not received. If I don't disable the
> > clock of the controller (hclock), I should receive the sdio IRQ as I
> > receive card detect ones, isn't it?
> >
> > It doesn't work, it seems I have also to not disabled mainck and gck
> > which are clocks needed to generate the clock sent to the sdio device.
> > If none of the clocks have to be disabled, where it has to be managed?
> 
> That's a typical issue for SDIO IRQs, especially when the controller
> HW manages IRQs (there are other ways to deal with SDIO IRQs as well).
> 
> Currently, the simplest way to deal with this in the driver is to do a
> pm_runtime_get_sync() when the SDIO IRQ gets enabled, and
> pm_runtime_put() when it gets disabled.

Which driver? sdio device driver or controller driver?

> >
> > Do I have to anticipate this use case in the driver of my sdhci
> > controller or does it have to be managed in the sdio device driver? They
> > are using sdio_claim/release_host to suspend or resume the host but
> > maybe they use it in a bad way.
> 
> The wilc100 SDIO func driver should *not* keep the host claimed to
> deal with SDIO irqs. Only when it configures them.
> 
> Instead, you need to deal with this in the sdhci driver, when you get
> the call to enable/disable SDIO IRQs.
> 

Do you mean in sdhci_enable_sdio_irq?

> Moreover, from a system PM point of view. If the wilc100 SDIO func
> driver wants the platform to wake up on SDIO IRQs, it needs to set
> MMC_PM_KEEP_POWER and MMC_PM_WAKE_SDIO_IRQ from its ->suspend()
> callback.
> 
> In that way, your sdhci driver can act accordingly from its system PM
> callbacks. In other words, depending on MMC_PM_KEEP_POWER and
> MMC_PM_WAKE_SDIO_IRQ to *not* call pm_runtime_force_suspend().
> 

Ok, it makes sense.


Coming back to the initial discussion and patch which were motivated by
the fact that after runtime suspend I can't wake-up on card detect event,
I have the feeling we don't have the same assumption about runtime PM.

>From what you and Adrian told me, I should not use runtime PM if I have
no way to wake-up. In your minds, the way to wake-up is to use an
externel GPIO because the controller will be 'totally' disabled, isn't it?

On my side, runtime PM allows me to save power when the sdhci controller
is not used. If I can disable two clocks out of three, I should use
runtime PM. Do you agree?

If not, tell me how I can convince you :) Otherwise, next step is to rework
my patch but I think I have no other solution that not calling
sdhci_runtime_suspend_host if I expect to use the card detect irq of the
controller.

Regards

Ludovic

Re: [PATCH] mmc: sdhci-of-at91: fix wakeup issue when using runtime pm

[Ulf Hansson]

+ Ludovic

On 4 March 2016 at 10:09, Ulf Hansson  wrote:
> On 17 February 2016 at 11:35, Ludovic Desroches
>  wrote:
>> On Tue, Feb 16, 2016 at 04:22:04PM +0100, Ludovic Desroches wrote:
>>> On Tue, Feb 16, 2016 at 03:38:29PM +0100, Ulf Hansson wrote:
>>> > On 13 February 2016 at 10:56, Ludovic Desroches
>>> >  wrote:
>>> > > When suspending the sdhci host, the only hardware event that could wake
>>> > > up the host is a sdio irq if they are enabled. If we want to wakeup on
>>> > > card detect events, a gpio as to be used.
>>> > > If we don't want to use a gpio but the card detect pio of the controller
>>> > > then we need to keep enabled the clock of the controller interface to
>>> > > get the interrupt and to not set the host in a suspended state to have 
>>> > > the
>>> > > interrupt handled.
>>> > >
>>> > > Signed-off-by: Ludovic Desroches 
>>> > > ---
>>> > >  drivers/mmc/host/sdhci-of-at91.c | 46 
>>> > > ++--
>>> > >  1 file changed, 35 insertions(+), 11 deletions(-)
>>> > >
>>> > > diff --git a/drivers/mmc/host/sdhci-of-at91.c 
>>> > > b/drivers/mmc/host/sdhci-of-at91.c
>>> > > index efec736..2159c6e 100644
>>> > > --- a/drivers/mmc/host/sdhci-of-at91.c
>>> > > +++ b/drivers/mmc/host/sdhci-of-at91.c
>>> > > @@ -18,6 +18,7 @@
>>> > >  #include 
>>> > >  #include 
>>> > >  #include 
>>> > > +#include 
>>> > >  #include 
>>> > >  #include 
>>> > >  #include 
>>> > > @@ -45,7 +46,6 @@ static const struct sdhci_ops sdhci_at91_sama5d2_ops 
>>> > > = {
>>> > >
>>> > >  static const struct sdhci_pltfm_data soc_data_sama5d2 = {
>>> > > .ops = _at91_sama5d2_ops,
>>> > > -   .quirks = SDHCI_QUIRK_BROKEN_CARD_DETECTION,
>>> >
>>> > You probably have some leftovers from earlier local changes, as this
>>> > isn't going to apply to my next branch.
>>> >
>>>
>>> Yes, it is based on the first patch of this thread, it was only to
>>> discuss about it.
>>>
>>> > > .quirks2 = SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST,
>>> > >  };
>>> > >
>>> > > @@ -55,17 +55,37 @@ static const struct of_device_id 
>>> > > sdhci_at91_dt_match[] = {
>>> > >  };
>>> > >
>>> > >  #ifdef CONFIG_PM
>>> > > +static bool sdhci_at91_use_sdhci_runtime(struct sdhci_host *host)
>>> > > +{
>>> > > +   u32 caps = host->mmc->caps;
>>> > > +
>>> > > +   return (caps & MMC_CAP_NONREMOVABLE) ||
>>> > > +  (!IS_ERR_VALUE(mmc_gpio_get_cd(host->mmc)));
>>
>>
>> I am wondering if I should take account of sdio irq enabled or not here.
>>
>> I have a sdio device which drives me crazy because of power management.
>> The driver of this device is in staging, it is wilc1000. It seems that I
>> am stuck because the sdio irq are not received. If I don't disable the
>> clock of the controller (hclock), I should receive the sdio IRQ as I
>> receive card detect ones, isn't it?
>>
>> It doesn't work, it seems I have also to not disabled mainck and gck
>> which are clocks needed to generate the clock sent to the sdio device.
>> If none of the clocks have to be disabled, where it has to be managed?
>
> That's a typical issue for SDIO IRQs, especially when the controller
> HW manages IRQs (there are other ways to deal with SDIO IRQs as well).
>
> Currently, the simplest way to deal with this in the driver is to do a
> pm_runtime_get_sync() when the SDIO IRQ gets enabled, and
> pm_runtime_put() when it gets disabled.
>
>>
>> Do I have to anticipate this use case in the driver of my sdhci
>> controller or does it have to be managed in the sdio device driver? They
>> are using sdio_claim/release_host to suspend or resume the host but
>> maybe they use it in a bad way.
>
> The wilc100 SDIO func driver should *not* keep the host claimed to
> deal with SDIO irqs. Only when it configures them.
>
> Instead, you need to deal with this in the sdhci driver, when you get
> the call to enable/disable SDIO IRQs.
>
> Moreover, from a system PM point of view. If the wilc100 SDIO func
> driver wants the platform to wake up on SDIO IRQs, it needs to set
> MMC_PM_KEEP_POWER and MMC_PM_WAKE_SDIO_IRQ from its ->suspend()
> callback.
>
> In that way, your sdhci driver can act accordingly from its system PM
> callbacks. In other words, depending on MMC_PM_KEEP_POWER and
> MMC_PM_WAKE_SDIO_IRQ to *not* call pm_runtime_force_suspend().
>
> [...]
>
> Kind regards
> Uffe

Re: [PATCH] mmc: sdhci-of-at91: fix wakeup issue when using runtime pm

[Ulf Hansson]

+ Ludovic

On 4 March 2016 at 10:09, Ulf Hansson  wrote:
> On 17 February 2016 at 11:35, Ludovic Desroches
>  wrote:
>> On Tue, Feb 16, 2016 at 04:22:04PM +0100, Ludovic Desroches wrote:
>>> On Tue, Feb 16, 2016 at 03:38:29PM +0100, Ulf Hansson wrote:
>>> > On 13 February 2016 at 10:56, Ludovic Desroches
>>> >  wrote:
>>> > > When suspending the sdhci host, the only hardware event that could wake
>>> > > up the host is a sdio irq if they are enabled. If we want to wakeup on
>>> > > card detect events, a gpio as to be used.
>>> > > If we don't want to use a gpio but the card detect pio of the controller
>>> > > then we need to keep enabled the clock of the controller interface to
>>> > > get the interrupt and to not set the host in a suspended state to have 
>>> > > the
>>> > > interrupt handled.
>>> > >
>>> > > Signed-off-by: Ludovic Desroches 
>>> > > ---
>>> > >  drivers/mmc/host/sdhci-of-at91.c | 46 
>>> > > ++--
>>> > >  1 file changed, 35 insertions(+), 11 deletions(-)
>>> > >
>>> > > diff --git a/drivers/mmc/host/sdhci-of-at91.c 
>>> > > b/drivers/mmc/host/sdhci-of-at91.c
>>> > > index efec736..2159c6e 100644
>>> > > --- a/drivers/mmc/host/sdhci-of-at91.c
>>> > > +++ b/drivers/mmc/host/sdhci-of-at91.c
>>> > > @@ -18,6 +18,7 @@
>>> > >  #include 
>>> > >  #include 
>>> > >  #include 
>>> > > +#include 
>>> > >  #include 
>>> > >  #include 
>>> > >  #include 
>>> > > @@ -45,7 +46,6 @@ static const struct sdhci_ops sdhci_at91_sama5d2_ops 
>>> > > = {
>>> > >
>>> > >  static const struct sdhci_pltfm_data soc_data_sama5d2 = {
>>> > > .ops = _at91_sama5d2_ops,
>>> > > -   .quirks = SDHCI_QUIRK_BROKEN_CARD_DETECTION,
>>> >
>>> > You probably have some leftovers from earlier local changes, as this
>>> > isn't going to apply to my next branch.
>>> >
>>>
>>> Yes, it is based on the first patch of this thread, it was only to
>>> discuss about it.
>>>
>>> > > .quirks2 = SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST,
>>> > >  };
>>> > >
>>> > > @@ -55,17 +55,37 @@ static const struct of_device_id 
>>> > > sdhci_at91_dt_match[] = {
>>> > >  };
>>> > >
>>> > >  #ifdef CONFIG_PM
>>> > > +static bool sdhci_at91_use_sdhci_runtime(struct sdhci_host *host)
>>> > > +{
>>> > > +   u32 caps = host->mmc->caps;
>>> > > +
>>> > > +   return (caps & MMC_CAP_NONREMOVABLE) ||
>>> > > +  (!IS_ERR_VALUE(mmc_gpio_get_cd(host->mmc)));
>>
>>
>> I am wondering if I should take account of sdio irq enabled or not here.
>>
>> I have a sdio device which drives me crazy because of power management.
>> The driver of this device is in staging, it is wilc1000. It seems that I
>> am stuck because the sdio irq are not received. If I don't disable the
>> clock of the controller (hclock), I should receive the sdio IRQ as I
>> receive card detect ones, isn't it?
>>
>> It doesn't work, it seems I have also to not disabled mainck and gck
>> which are clocks needed to generate the clock sent to the sdio device.
>> If none of the clocks have to be disabled, where it has to be managed?
>
> That's a typical issue for SDIO IRQs, especially when the controller
> HW manages IRQs (there are other ways to deal with SDIO IRQs as well).
>
> Currently, the simplest way to deal with this in the driver is to do a
> pm_runtime_get_sync() when the SDIO IRQ gets enabled, and
> pm_runtime_put() when it gets disabled.
>
>>
>> Do I have to anticipate this use case in the driver of my sdhci
>> controller or does it have to be managed in the sdio device driver? They
>> are using sdio_claim/release_host to suspend or resume the host but
>> maybe they use it in a bad way.
>
> The wilc100 SDIO func driver should *not* keep the host claimed to
> deal with SDIO irqs. Only when it configures them.
>
> Instead, you need to deal with this in the sdhci driver, when you get
> the call to enable/disable SDIO IRQs.
>
> Moreover, from a system PM point of view. If the wilc100 SDIO func
> driver wants the platform to wake up on SDIO IRQs, it needs to set
> MMC_PM_KEEP_POWER and MMC_PM_WAKE_SDIO_IRQ from its ->suspend()
> callback.
>
> In that way, your sdhci driver can act accordingly from its system PM
> callbacks. In other words, depending on MMC_PM_KEEP_POWER and
> MMC_PM_WAKE_SDIO_IRQ to *not* call pm_runtime_force_suspend().
>
> [...]
>
> Kind regards
> Uffe

Re: [PATCH] mmc: sdhci-of-at91: fix wakeup issue when using runtime pm

[Ulf Hansson]

On 17 February 2016 at 11:35, Ludovic Desroches
 wrote:
> On Tue, Feb 16, 2016 at 04:22:04PM +0100, Ludovic Desroches wrote:
>> On Tue, Feb 16, 2016 at 03:38:29PM +0100, Ulf Hansson wrote:
>> > On 13 February 2016 at 10:56, Ludovic Desroches
>> >  wrote:
>> > > When suspending the sdhci host, the only hardware event that could wake
>> > > up the host is a sdio irq if they are enabled. If we want to wakeup on
>> > > card detect events, a gpio as to be used.
>> > > If we don't want to use a gpio but the card detect pio of the controller
>> > > then we need to keep enabled the clock of the controller interface to
>> > > get the interrupt and to not set the host in a suspended state to have 
>> > > the
>> > > interrupt handled.
>> > >
>> > > Signed-off-by: Ludovic Desroches 
>> > > ---
>> > >  drivers/mmc/host/sdhci-of-at91.c | 46 
>> > > ++--
>> > >  1 file changed, 35 insertions(+), 11 deletions(-)
>> > >
>> > > diff --git a/drivers/mmc/host/sdhci-of-at91.c 
>> > > b/drivers/mmc/host/sdhci-of-at91.c
>> > > index efec736..2159c6e 100644
>> > > --- a/drivers/mmc/host/sdhci-of-at91.c
>> > > +++ b/drivers/mmc/host/sdhci-of-at91.c
>> > > @@ -18,6 +18,7 @@
>> > >  #include 
>> > >  #include 
>> > >  #include 
>> > > +#include 
>> > >  #include 
>> > >  #include 
>> > >  #include 
>> > > @@ -45,7 +46,6 @@ static const struct sdhci_ops sdhci_at91_sama5d2_ops = 
>> > > {
>> > >
>> > >  static const struct sdhci_pltfm_data soc_data_sama5d2 = {
>> > > .ops = _at91_sama5d2_ops,
>> > > -   .quirks = SDHCI_QUIRK_BROKEN_CARD_DETECTION,
>> >
>> > You probably have some leftovers from earlier local changes, as this
>> > isn't going to apply to my next branch.
>> >
>>
>> Yes, it is based on the first patch of this thread, it was only to
>> discuss about it.
>>
>> > > .quirks2 = SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST,
>> > >  };
>> > >
>> > > @@ -55,17 +55,37 @@ static const struct of_device_id 
>> > > sdhci_at91_dt_match[] = {
>> > >  };
>> > >
>> > >  #ifdef CONFIG_PM
>> > > +static bool sdhci_at91_use_sdhci_runtime(struct sdhci_host *host)
>> > > +{
>> > > +   u32 caps = host->mmc->caps;
>> > > +
>> > > +   return (caps & MMC_CAP_NONREMOVABLE) ||
>> > > +  (!IS_ERR_VALUE(mmc_gpio_get_cd(host->mmc)));
>
>
> I am wondering if I should take account of sdio irq enabled or not here.
>
> I have a sdio device which drives me crazy because of power management.
> The driver of this device is in staging, it is wilc1000. It seems that I
> am stuck because the sdio irq are not received. If I don't disable the
> clock of the controller (hclock), I should receive the sdio IRQ as I
> receive card detect ones, isn't it?
>
> It doesn't work, it seems I have also to not disabled mainck and gck
> which are clocks needed to generate the clock sent to the sdio device.
> If none of the clocks have to be disabled, where it has to be managed?

That's a typical issue for SDIO IRQs, especially when the controller
HW manages IRQs (there are other ways to deal with SDIO IRQs as well).

Currently, the simplest way to deal with this in the driver is to do a
pm_runtime_get_sync() when the SDIO IRQ gets enabled, and
pm_runtime_put() when it gets disabled.

>
> Do I have to anticipate this use case in the driver of my sdhci
> controller or does it have to be managed in the sdio device driver? They
> are using sdio_claim/release_host to suspend or resume the host but
> maybe they use it in a bad way.

The wilc100 SDIO func driver should *not* keep the host claimed to
deal with SDIO irqs. Only when it configures them.

Instead, you need to deal with this in the sdhci driver, when you get
the call to enable/disable SDIO IRQs.

Moreover, from a system PM point of view. If the wilc100 SDIO func
driver wants the platform to wake up on SDIO IRQs, it needs to set
MMC_PM_KEEP_POWER and MMC_PM_WAKE_SDIO_IRQ from its ->suspend()
callback.

In that way, your sdhci driver can act accordingly from its system PM
callbacks. In other words, depending on MMC_PM_KEEP_POWER and
MMC_PM_WAKE_SDIO_IRQ to *not* call pm_runtime_force_suspend().

[...]

Kind regards
Uffe

Re: [PATCH] mmc: sdhci-of-at91: fix wakeup issue when using runtime pm

[Ulf Hansson]

On 17 February 2016 at 11:35, Ludovic Desroches
 wrote:
> On Tue, Feb 16, 2016 at 04:22:04PM +0100, Ludovic Desroches wrote:
>> On Tue, Feb 16, 2016 at 03:38:29PM +0100, Ulf Hansson wrote:
>> > On 13 February 2016 at 10:56, Ludovic Desroches
>> >  wrote:
>> > > When suspending the sdhci host, the only hardware event that could wake
>> > > up the host is a sdio irq if they are enabled. If we want to wakeup on
>> > > card detect events, a gpio as to be used.
>> > > If we don't want to use a gpio but the card detect pio of the controller
>> > > then we need to keep enabled the clock of the controller interface to
>> > > get the interrupt and to not set the host in a suspended state to have 
>> > > the
>> > > interrupt handled.
>> > >
>> > > Signed-off-by: Ludovic Desroches 
>> > > ---
>> > >  drivers/mmc/host/sdhci-of-at91.c | 46 
>> > > ++--
>> > >  1 file changed, 35 insertions(+), 11 deletions(-)
>> > >
>> > > diff --git a/drivers/mmc/host/sdhci-of-at91.c 
>> > > b/drivers/mmc/host/sdhci-of-at91.c
>> > > index efec736..2159c6e 100644
>> > > --- a/drivers/mmc/host/sdhci-of-at91.c
>> > > +++ b/drivers/mmc/host/sdhci-of-at91.c
>> > > @@ -18,6 +18,7 @@
>> > >  #include 
>> > >  #include 
>> > >  #include 
>> > > +#include 
>> > >  #include 
>> > >  #include 
>> > >  #include 
>> > > @@ -45,7 +46,6 @@ static const struct sdhci_ops sdhci_at91_sama5d2_ops = 
>> > > {
>> > >
>> > >  static const struct sdhci_pltfm_data soc_data_sama5d2 = {
>> > > .ops = _at91_sama5d2_ops,
>> > > -   .quirks = SDHCI_QUIRK_BROKEN_CARD_DETECTION,
>> >
>> > You probably have some leftovers from earlier local changes, as this
>> > isn't going to apply to my next branch.
>> >
>>
>> Yes, it is based on the first patch of this thread, it was only to
>> discuss about it.
>>
>> > > .quirks2 = SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST,
>> > >  };
>> > >
>> > > @@ -55,17 +55,37 @@ static const struct of_device_id 
>> > > sdhci_at91_dt_match[] = {
>> > >  };
>> > >
>> > >  #ifdef CONFIG_PM
>> > > +static bool sdhci_at91_use_sdhci_runtime(struct sdhci_host *host)
>> > > +{
>> > > +   u32 caps = host->mmc->caps;
>> > > +
>> > > +   return (caps & MMC_CAP_NONREMOVABLE) ||
>> > > +  (!IS_ERR_VALUE(mmc_gpio_get_cd(host->mmc)));
>
>
> I am wondering if I should take account of sdio irq enabled or not here.
>
> I have a sdio device which drives me crazy because of power management.
> The driver of this device is in staging, it is wilc1000. It seems that I
> am stuck because the sdio irq are not received. If I don't disable the
> clock of the controller (hclock), I should receive the sdio IRQ as I
> receive card detect ones, isn't it?
>
> It doesn't work, it seems I have also to not disabled mainck and gck
> which are clocks needed to generate the clock sent to the sdio device.
> If none of the clocks have to be disabled, where it has to be managed?

That's a typical issue for SDIO IRQs, especially when the controller
HW manages IRQs (there are other ways to deal with SDIO IRQs as well).

Currently, the simplest way to deal with this in the driver is to do a
pm_runtime_get_sync() when the SDIO IRQ gets enabled, and
pm_runtime_put() when it gets disabled.

>
> Do I have to anticipate this use case in the driver of my sdhci
> controller or does it have to be managed in the sdio device driver? They
> are using sdio_claim/release_host to suspend or resume the host but
> maybe they use it in a bad way.

The wilc100 SDIO func driver should *not* keep the host claimed to
deal with SDIO irqs. Only when it configures them.

Instead, you need to deal with this in the sdhci driver, when you get
the call to enable/disable SDIO IRQs.

Moreover, from a system PM point of view. If the wilc100 SDIO func
driver wants the platform to wake up on SDIO IRQs, it needs to set
MMC_PM_KEEP_POWER and MMC_PM_WAKE_SDIO_IRQ from its ->suspend()
callback.

In that way, your sdhci driver can act accordingly from its system PM
callbacks. In other words, depending on MMC_PM_KEEP_POWER and
MMC_PM_WAKE_SDIO_IRQ to *not* call pm_runtime_force_suspend().

[...]

Kind regards
Uffe

Re: [PATCH] mmc: sdhci-of-at91: fix wakeup issue when using runtime pm

[Ludovic Desroches]

On Sat, Feb 13, 2016 at 10:56:40AM +0100, Ludovic Desroches wrote:
> When suspending the sdhci host, the only hardware event that could wake
> up the host is a sdio irq if they are enabled. If we want to wakeup on
> card detect events, a gpio as to be used.
> If we don't want to use a gpio but the card detect pio of the controller
> then we need to keep enabled the clock of the controller interface to
> get the interrupt and to not set the host in a suspended state to have the
> interrupt handled.
> 

How can I move forward on this topic? I have several issues related to
runtime PM (card detection, wifi module which doesn't like to get clocks
disabled) but I am not in favour to get rid of it.

Regards

Ludovic

> Signed-off-by: Ludovic Desroches 
> ---
>  drivers/mmc/host/sdhci-of-at91.c | 46 
> ++--
>  1 file changed, 35 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-at91.c 
> b/drivers/mmc/host/sdhci-of-at91.c
> index efec736..2159c6e 100644
> --- a/drivers/mmc/host/sdhci-of-at91.c
> +++ b/drivers/mmc/host/sdhci-of-at91.c
> @@ -18,6 +18,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -45,7 +46,6 @@ static const struct sdhci_ops sdhci_at91_sama5d2_ops = {
>  
>  static const struct sdhci_pltfm_data soc_data_sama5d2 = {
>   .ops = _at91_sama5d2_ops,
> - .quirks = SDHCI_QUIRK_BROKEN_CARD_DETECTION,
>   .quirks2 = SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST,
>  };
>  
> @@ -55,17 +55,37 @@ static const struct of_device_id sdhci_at91_dt_match[] = {
>  };
>  
>  #ifdef CONFIG_PM
> +static bool sdhci_at91_use_sdhci_runtime(struct sdhci_host *host)
> +{
> + u32 caps = host->mmc->caps;
> +
> + return (caps & MMC_CAP_NONREMOVABLE) ||
> +(!IS_ERR_VALUE(mmc_gpio_get_cd(host->mmc)));
> +}
> +
>  static int sdhci_at91_runtime_suspend(struct device *dev)
>  {
>   struct sdhci_host *host = dev_get_drvdata(dev);
>   struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>   struct sdhci_at91_priv *priv = pltfm_host->priv;
> - int ret;
> + int ret = 0;
>  
> - ret = sdhci_runtime_suspend_host(host);
> + /*
> +  * If we call sdhci_runtime_suspend(), we could wakeup only if sdio
> +  * irqs are enabled. If we want to wakeup on a card detect event there
> +  * are two options:
> +  * - do not call sdhci_runtime_suspend() but save power by disabling
> +  *   all clocks excepting the one for the controller interface.
> +  * - call sdhci_runtime_suspend(), save maximum power by disabling
> +  *   all clocks but use a gpio for the card detect signal to have a way
> +  *   to wakeup.
> +  */
> + if (sdhci_at91_use_sdhci_runtime(host)) {
> + ret = sdhci_runtime_suspend_host(host);
> + clk_disable_unprepare(priv->hclock);
> + }
>  
>   clk_disable_unprepare(priv->gck);
> - clk_disable_unprepare(priv->hclock);
>   clk_disable_unprepare(priv->mainck);
>  
>   return ret;
> @@ -84,19 +104,23 @@ static int sdhci_at91_runtime_resume(struct device *dev)
>   return ret;
>   }
>  
> - ret = clk_prepare_enable(priv->hclock);
> - if (ret) {
> - dev_err(dev, "can't enable hclock\n");
> - return ret;
> - }
> -
>   ret = clk_prepare_enable(priv->gck);
>   if (ret) {
>   dev_err(dev, "can't enable gck\n");
>   return ret;
>   }
>  
> - return sdhci_runtime_resume_host(host);
> + if (sdhci_at91_use_sdhci_runtime(host)) {
> + ret = clk_prepare_enable(priv->hclock);
> + if (ret) {
> + dev_err(dev, "can't enable hclock\n");
> + return ret;
> + }
> +
> + ret = sdhci_runtime_resume_host(host);
> + }
> +
> + return ret;
>  }
>  #endif /* CONFIG_PM */
>  
> -- 
> 2.7.0
> 

Re: [PATCH] mmc: sdhci-of-at91: fix wakeup issue when using runtime pm

[Ludovic Desroches]

On Sat, Feb 13, 2016 at 10:56:40AM +0100, Ludovic Desroches wrote:
> When suspending the sdhci host, the only hardware event that could wake
> up the host is a sdio irq if they are enabled. If we want to wakeup on
> card detect events, a gpio as to be used.
> If we don't want to use a gpio but the card detect pio of the controller
> then we need to keep enabled the clock of the controller interface to
> get the interrupt and to not set the host in a suspended state to have the
> interrupt handled.
> 

How can I move forward on this topic? I have several issues related to
runtime PM (card detection, wifi module which doesn't like to get clocks
disabled) but I am not in favour to get rid of it.

Regards

Ludovic

> Signed-off-by: Ludovic Desroches 
> ---
>  drivers/mmc/host/sdhci-of-at91.c | 46 
> ++--
>  1 file changed, 35 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-at91.c 
> b/drivers/mmc/host/sdhci-of-at91.c
> index efec736..2159c6e 100644
> --- a/drivers/mmc/host/sdhci-of-at91.c
> +++ b/drivers/mmc/host/sdhci-of-at91.c
> @@ -18,6 +18,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -45,7 +46,6 @@ static const struct sdhci_ops sdhci_at91_sama5d2_ops = {
>  
>  static const struct sdhci_pltfm_data soc_data_sama5d2 = {
>   .ops = _at91_sama5d2_ops,
> - .quirks = SDHCI_QUIRK_BROKEN_CARD_DETECTION,
>   .quirks2 = SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST,
>  };
>  
> @@ -55,17 +55,37 @@ static const struct of_device_id sdhci_at91_dt_match[] = {
>  };
>  
>  #ifdef CONFIG_PM
> +static bool sdhci_at91_use_sdhci_runtime(struct sdhci_host *host)
> +{
> + u32 caps = host->mmc->caps;
> +
> + return (caps & MMC_CAP_NONREMOVABLE) ||
> +(!IS_ERR_VALUE(mmc_gpio_get_cd(host->mmc)));
> +}
> +
>  static int sdhci_at91_runtime_suspend(struct device *dev)
>  {
>   struct sdhci_host *host = dev_get_drvdata(dev);
>   struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>   struct sdhci_at91_priv *priv = pltfm_host->priv;
> - int ret;
> + int ret = 0;
>  
> - ret = sdhci_runtime_suspend_host(host);
> + /*
> +  * If we call sdhci_runtime_suspend(), we could wakeup only if sdio
> +  * irqs are enabled. If we want to wakeup on a card detect event there
> +  * are two options:
> +  * - do not call sdhci_runtime_suspend() but save power by disabling
> +  *   all clocks excepting the one for the controller interface.
> +  * - call sdhci_runtime_suspend(), save maximum power by disabling
> +  *   all clocks but use a gpio for the card detect signal to have a way
> +  *   to wakeup.
> +  */
> + if (sdhci_at91_use_sdhci_runtime(host)) {
> + ret = sdhci_runtime_suspend_host(host);
> + clk_disable_unprepare(priv->hclock);
> + }
>  
>   clk_disable_unprepare(priv->gck);
> - clk_disable_unprepare(priv->hclock);
>   clk_disable_unprepare(priv->mainck);
>  
>   return ret;
> @@ -84,19 +104,23 @@ static int sdhci_at91_runtime_resume(struct device *dev)
>   return ret;
>   }
>  
> - ret = clk_prepare_enable(priv->hclock);
> - if (ret) {
> - dev_err(dev, "can't enable hclock\n");
> - return ret;
> - }
> -
>   ret = clk_prepare_enable(priv->gck);
>   if (ret) {
>   dev_err(dev, "can't enable gck\n");
>   return ret;
>   }
>  
> - return sdhci_runtime_resume_host(host);
> + if (sdhci_at91_use_sdhci_runtime(host)) {
> + ret = clk_prepare_enable(priv->hclock);
> + if (ret) {
> + dev_err(dev, "can't enable hclock\n");
> + return ret;
> + }
> +
> + ret = sdhci_runtime_resume_host(host);
> + }
> +
> + return ret;
>  }
>  #endif /* CONFIG_PM */
>  
> -- 
> 2.7.0
> 

Re: [PATCH] mmc: sdhci-of-at91: fix wakeup issue when using runtime pm

[Ludovic Desroches]

On Tue, Feb 16, 2016 at 04:22:04PM +0100, Ludovic Desroches wrote:
> On Tue, Feb 16, 2016 at 03:38:29PM +0100, Ulf Hansson wrote:
> > On 13 February 2016 at 10:56, Ludovic Desroches
> >  wrote:
> > > When suspending the sdhci host, the only hardware event that could wake
> > > up the host is a sdio irq if they are enabled. If we want to wakeup on
> > > card detect events, a gpio as to be used.
> > > If we don't want to use a gpio but the card detect pio of the controller
> > > then we need to keep enabled the clock of the controller interface to
> > > get the interrupt and to not set the host in a suspended state to have the
> > > interrupt handled.
> > >
> > > Signed-off-by: Ludovic Desroches 
> > > ---
> > >  drivers/mmc/host/sdhci-of-at91.c | 46 
> > > ++--
> > >  1 file changed, 35 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/mmc/host/sdhci-of-at91.c 
> > > b/drivers/mmc/host/sdhci-of-at91.c
> > > index efec736..2159c6e 100644
> > > --- a/drivers/mmc/host/sdhci-of-at91.c
> > > +++ b/drivers/mmc/host/sdhci-of-at91.c
> > > @@ -18,6 +18,7 @@
> > >  #include 
> > >  #include 
> > >  #include 
> > > +#include 
> > >  #include 
> > >  #include 
> > >  #include 
> > > @@ -45,7 +46,6 @@ static const struct sdhci_ops sdhci_at91_sama5d2_ops = {
> > >
> > >  static const struct sdhci_pltfm_data soc_data_sama5d2 = {
> > > .ops = _at91_sama5d2_ops,
> > > -   .quirks = SDHCI_QUIRK_BROKEN_CARD_DETECTION,
> > 
> > You probably have some leftovers from earlier local changes, as this
> > isn't going to apply to my next branch.
> > 
> 
> Yes, it is based on the first patch of this thread, it was only to
> discuss about it.
> 
> > > .quirks2 = SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST,
> > >  };
> > >
> > > @@ -55,17 +55,37 @@ static const struct of_device_id 
> > > sdhci_at91_dt_match[] = {
> > >  };
> > >
> > >  #ifdef CONFIG_PM
> > > +static bool sdhci_at91_use_sdhci_runtime(struct sdhci_host *host)
> > > +{
> > > +   u32 caps = host->mmc->caps;
> > > +
> > > +   return (caps & MMC_CAP_NONREMOVABLE) ||
> > > +  (!IS_ERR_VALUE(mmc_gpio_get_cd(host->mmc)));


I am wondering if I should take account of sdio irq enabled or not here.

I have a sdio device which drives me crazy because of power management.
The driver of this device is in staging, it is wilc1000. It seems that I
am stuck because the sdio irq are not received. If I don't disable the
clock of the controller (hclock), I should receive the sdio IRQ as I
receive card detect ones, isn't it?

It doesn't work, it seems I have also to not disabled mainck and gck
which are clocks needed to generate the clock sent to the sdio device.
If none of the clocks have to be disabled, where it has to be managed?

Do I have to anticipate this use case in the driver of my sdhci
controller or does it have to be managed in the sdio device driver? They
are using sdio_claim/release_host to suspend or resume the host but
maybe they use it in a bad way.

> > > +}
> > > +
> > >  static int sdhci_at91_runtime_suspend(struct device *dev)
> > >  {
> > > struct sdhci_host *host = dev_get_drvdata(dev);
> > > struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > > struct sdhci_at91_priv *priv = pltfm_host->priv;
> > > -   int ret;
> > > +   int ret = 0;
> > >
> > > -   ret = sdhci_runtime_suspend_host(host);
> > > +   /*
> > > +* If we call sdhci_runtime_suspend(), we could wakeup only if 
> > > sdio
> > > +* irqs are enabled. If we want to wakeup on a card detect event 
> > > there
> > > +* are two options:
> > > +* - do not call sdhci_runtime_suspend() but save power by 
> > > disabling
> > > +*   all clocks excepting the one for the controller interface.
> > > +* - call sdhci_runtime_suspend(), save maximum power by disabling
> > > +*   all clocks but use a gpio for the card detect signal to have 
> > > a way
> > > +*   to wakeup.
> > > +*/
> > > +   if (sdhci_at91_use_sdhci_runtime(host)) {
> > 
> > I am not sure this approach is safe, particularly for cases when
> > sdhci_runtime_suspend() isn't invoked.
> > 
> > As I understand it, in those cases potentially the sdhci's IRQ handler
> > may be invoked to serve even other IRQs than a card detect IRQ. Doing
> > that while being runtime suspended doesn't seem like a good idea. Will
> > it even work?
> > 
> 
> Yes it is the idea. It will allow to save some power. I don't see how I can 
> use
> runtime PM if I need to invoke sdhci_runtime_suspend_host() without modifying
> sdhci layer to handle card detect irq.
> 
> I was also afraid to have some side effects but it works, at least for the
> card detect case.
> 
> > > +   ret = sdhci_runtime_suspend_host(host);
> > > +   clk_disable_unprepare(priv->hclock);
> > > +   }
> > >
> > > 

Re: [PATCH] mmc: sdhci-of-at91: fix wakeup issue when using runtime pm

[Ludovic Desroches]

On Tue, Feb 16, 2016 at 04:22:04PM +0100, Ludovic Desroches wrote:
> On Tue, Feb 16, 2016 at 03:38:29PM +0100, Ulf Hansson wrote:
> > On 13 February 2016 at 10:56, Ludovic Desroches
> >  wrote:
> > > When suspending the sdhci host, the only hardware event that could wake
> > > up the host is a sdio irq if they are enabled. If we want to wakeup on
> > > card detect events, a gpio as to be used.
> > > If we don't want to use a gpio but the card detect pio of the controller
> > > then we need to keep enabled the clock of the controller interface to
> > > get the interrupt and to not set the host in a suspended state to have the
> > > interrupt handled.
> > >
> > > Signed-off-by: Ludovic Desroches 
> > > ---
> > >  drivers/mmc/host/sdhci-of-at91.c | 46 
> > > ++--
> > >  1 file changed, 35 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/mmc/host/sdhci-of-at91.c 
> > > b/drivers/mmc/host/sdhci-of-at91.c
> > > index efec736..2159c6e 100644
> > > --- a/drivers/mmc/host/sdhci-of-at91.c
> > > +++ b/drivers/mmc/host/sdhci-of-at91.c
> > > @@ -18,6 +18,7 @@
> > >  #include 
> > >  #include 
> > >  #include 
> > > +#include 
> > >  #include 
> > >  #include 
> > >  #include 
> > > @@ -45,7 +46,6 @@ static const struct sdhci_ops sdhci_at91_sama5d2_ops = {
> > >
> > >  static const struct sdhci_pltfm_data soc_data_sama5d2 = {
> > > .ops = _at91_sama5d2_ops,
> > > -   .quirks = SDHCI_QUIRK_BROKEN_CARD_DETECTION,
> > 
> > You probably have some leftovers from earlier local changes, as this
> > isn't going to apply to my next branch.
> > 
> 
> Yes, it is based on the first patch of this thread, it was only to
> discuss about it.
> 
> > > .quirks2 = SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST,
> > >  };
> > >
> > > @@ -55,17 +55,37 @@ static const struct of_device_id 
> > > sdhci_at91_dt_match[] = {
> > >  };
> > >
> > >  #ifdef CONFIG_PM
> > > +static bool sdhci_at91_use_sdhci_runtime(struct sdhci_host *host)
> > > +{
> > > +   u32 caps = host->mmc->caps;
> > > +
> > > +   return (caps & MMC_CAP_NONREMOVABLE) ||
> > > +  (!IS_ERR_VALUE(mmc_gpio_get_cd(host->mmc)));


I am wondering if I should take account of sdio irq enabled or not here.

I have a sdio device which drives me crazy because of power management.
The driver of this device is in staging, it is wilc1000. It seems that I
am stuck because the sdio irq are not received. If I don't disable the
clock of the controller (hclock), I should receive the sdio IRQ as I
receive card detect ones, isn't it?

It doesn't work, it seems I have also to not disabled mainck and gck
which are clocks needed to generate the clock sent to the sdio device.
If none of the clocks have to be disabled, where it has to be managed?

Do I have to anticipate this use case in the driver of my sdhci
controller or does it have to be managed in the sdio device driver? They
are using sdio_claim/release_host to suspend or resume the host but
maybe they use it in a bad way.

> > > +}
> > > +
> > >  static int sdhci_at91_runtime_suspend(struct device *dev)
> > >  {
> > > struct sdhci_host *host = dev_get_drvdata(dev);
> > > struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > > struct sdhci_at91_priv *priv = pltfm_host->priv;
> > > -   int ret;
> > > +   int ret = 0;
> > >
> > > -   ret = sdhci_runtime_suspend_host(host);
> > > +   /*
> > > +* If we call sdhci_runtime_suspend(), we could wakeup only if 
> > > sdio
> > > +* irqs are enabled. If we want to wakeup on a card detect event 
> > > there
> > > +* are two options:
> > > +* - do not call sdhci_runtime_suspend() but save power by 
> > > disabling
> > > +*   all clocks excepting the one for the controller interface.
> > > +* - call sdhci_runtime_suspend(), save maximum power by disabling
> > > +*   all clocks but use a gpio for the card detect signal to have 
> > > a way
> > > +*   to wakeup.
> > > +*/
> > > +   if (sdhci_at91_use_sdhci_runtime(host)) {
> > 
> > I am not sure this approach is safe, particularly for cases when
> > sdhci_runtime_suspend() isn't invoked.
> > 
> > As I understand it, in those cases potentially the sdhci's IRQ handler
> > may be invoked to serve even other IRQs than a card detect IRQ. Doing
> > that while being runtime suspended doesn't seem like a good idea. Will
> > it even work?
> > 
> 
> Yes it is the idea. It will allow to save some power. I don't see how I can 
> use
> runtime PM if I need to invoke sdhci_runtime_suspend_host() without modifying
> sdhci layer to handle card detect irq.
> 
> I was also afraid to have some side effects but it works, at least for the
> card detect case.
> 
> > > +   ret = sdhci_runtime_suspend_host(host);
> > > +   clk_disable_unprepare(priv->hclock);
> > > +   }
> > >
> > > clk_disable_unprepare(priv->gck);
> > > -   

Re: [PATCH] mmc: sdhci-of-at91: fix wakeup issue when using runtime pm

[Ludovic Desroches]

On Tue, Feb 16, 2016 at 03:38:29PM +0100, Ulf Hansson wrote:
> On 13 February 2016 at 10:56, Ludovic Desroches
>  wrote:
> > When suspending the sdhci host, the only hardware event that could wake
> > up the host is a sdio irq if they are enabled. If we want to wakeup on
> > card detect events, a gpio as to be used.
> > If we don't want to use a gpio but the card detect pio of the controller
> > then we need to keep enabled the clock of the controller interface to
> > get the interrupt and to not set the host in a suspended state to have the
> > interrupt handled.
> >
> > Signed-off-by: Ludovic Desroches 
> > ---
> >  drivers/mmc/host/sdhci-of-at91.c | 46 
> > ++--
> >  1 file changed, 35 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-of-at91.c 
> > b/drivers/mmc/host/sdhci-of-at91.c
> > index efec736..2159c6e 100644
> > --- a/drivers/mmc/host/sdhci-of-at91.c
> > +++ b/drivers/mmc/host/sdhci-of-at91.c
> > @@ -18,6 +18,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -45,7 +46,6 @@ static const struct sdhci_ops sdhci_at91_sama5d2_ops = {
> >
> >  static const struct sdhci_pltfm_data soc_data_sama5d2 = {
> > .ops = _at91_sama5d2_ops,
> > -   .quirks = SDHCI_QUIRK_BROKEN_CARD_DETECTION,
> 
> You probably have some leftovers from earlier local changes, as this
> isn't going to apply to my next branch.
> 

Yes, it is based on the first patch of this thread, it was only to
discuss about it.

> > .quirks2 = SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST,
> >  };
> >
> > @@ -55,17 +55,37 @@ static const struct of_device_id sdhci_at91_dt_match[] 
> > = {
> >  };
> >
> >  #ifdef CONFIG_PM
> > +static bool sdhci_at91_use_sdhci_runtime(struct sdhci_host *host)
> > +{
> > +   u32 caps = host->mmc->caps;
> > +
> > +   return (caps & MMC_CAP_NONREMOVABLE) ||
> > +  (!IS_ERR_VALUE(mmc_gpio_get_cd(host->mmc)));
> > +}
> > +
> >  static int sdhci_at91_runtime_suspend(struct device *dev)
> >  {
> > struct sdhci_host *host = dev_get_drvdata(dev);
> > struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > struct sdhci_at91_priv *priv = pltfm_host->priv;
> > -   int ret;
> > +   int ret = 0;
> >
> > -   ret = sdhci_runtime_suspend_host(host);
> > +   /*
> > +* If we call sdhci_runtime_suspend(), we could wakeup only if sdio
> > +* irqs are enabled. If we want to wakeup on a card detect event 
> > there
> > +* are two options:
> > +* - do not call sdhci_runtime_suspend() but save power by disabling
> > +*   all clocks excepting the one for the controller interface.
> > +* - call sdhci_runtime_suspend(), save maximum power by disabling
> > +*   all clocks but use a gpio for the card detect signal to have a 
> > way
> > +*   to wakeup.
> > +*/
> > +   if (sdhci_at91_use_sdhci_runtime(host)) {
> 
> I am not sure this approach is safe, particularly for cases when
> sdhci_runtime_suspend() isn't invoked.
> 
> As I understand it, in those cases potentially the sdhci's IRQ handler
> may be invoked to serve even other IRQs than a card detect IRQ. Doing
> that while being runtime suspended doesn't seem like a good idea. Will
> it even work?
> 

Yes it is the idea. It will allow to save some power. I don't see how I can use
runtime PM if I need to invoke sdhci_runtime_suspend_host() without modifying
sdhci layer to handle card detect irq.

I was also afraid to have some side effects but it works, at least for the
card detect case.

> > +   ret = sdhci_runtime_suspend_host(host);
> > +   clk_disable_unprepare(priv->hclock);
> > +   }
> >
> > clk_disable_unprepare(priv->gck);
> > -   clk_disable_unprepare(priv->hclock);
> > clk_disable_unprepare(priv->mainck);
> >
> > return ret;
> > @@ -84,19 +104,23 @@ static int sdhci_at91_runtime_resume(struct device 
> > *dev)
> > return ret;
> > }
> >
> > -   ret = clk_prepare_enable(priv->hclock);
> > -   if (ret) {
> > -   dev_err(dev, "can't enable hclock\n");
> > -   return ret;
> > -   }
> > -
> > ret = clk_prepare_enable(priv->gck);
> > if (ret) {
> > dev_err(dev, "can't enable gck\n");
> > return ret;
> > }
> >
> > -   return sdhci_runtime_resume_host(host);
> > +   if (sdhci_at91_use_sdhci_runtime(host)) {
> > +   ret = clk_prepare_enable(priv->hclock);
> > +   if (ret) {
> > +   dev_err(dev, "can't enable hclock\n");
> > +   return ret;
> > +   }
> > +
> > +   ret = sdhci_runtime_resume_host(host);
> > +   }
> > +
> > +   return ret;
> >  }
> >  #endif /* CONFIG_PM */

Regards

Re: [PATCH] mmc: sdhci-of-at91: fix wakeup issue when using runtime pm

[Ludovic Desroches]

On Tue, Feb 16, 2016 at 03:38:29PM +0100, Ulf Hansson wrote:
> On 13 February 2016 at 10:56, Ludovic Desroches
>  wrote:
> > When suspending the sdhci host, the only hardware event that could wake
> > up the host is a sdio irq if they are enabled. If we want to wakeup on
> > card detect events, a gpio as to be used.
> > If we don't want to use a gpio but the card detect pio of the controller
> > then we need to keep enabled the clock of the controller interface to
> > get the interrupt and to not set the host in a suspended state to have the
> > interrupt handled.
> >
> > Signed-off-by: Ludovic Desroches 
> > ---
> >  drivers/mmc/host/sdhci-of-at91.c | 46 
> > ++--
> >  1 file changed, 35 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-of-at91.c 
> > b/drivers/mmc/host/sdhci-of-at91.c
> > index efec736..2159c6e 100644
> > --- a/drivers/mmc/host/sdhci-of-at91.c
> > +++ b/drivers/mmc/host/sdhci-of-at91.c
> > @@ -18,6 +18,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -45,7 +46,6 @@ static const struct sdhci_ops sdhci_at91_sama5d2_ops = {
> >
> >  static const struct sdhci_pltfm_data soc_data_sama5d2 = {
> > .ops = _at91_sama5d2_ops,
> > -   .quirks = SDHCI_QUIRK_BROKEN_CARD_DETECTION,
> 
> You probably have some leftovers from earlier local changes, as this
> isn't going to apply to my next branch.
> 

Yes, it is based on the first patch of this thread, it was only to
discuss about it.

> > .quirks2 = SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST,
> >  };
> >
> > @@ -55,17 +55,37 @@ static const struct of_device_id sdhci_at91_dt_match[] 
> > = {
> >  };
> >
> >  #ifdef CONFIG_PM
> > +static bool sdhci_at91_use_sdhci_runtime(struct sdhci_host *host)
> > +{
> > +   u32 caps = host->mmc->caps;
> > +
> > +   return (caps & MMC_CAP_NONREMOVABLE) ||
> > +  (!IS_ERR_VALUE(mmc_gpio_get_cd(host->mmc)));
> > +}
> > +
> >  static int sdhci_at91_runtime_suspend(struct device *dev)
> >  {
> > struct sdhci_host *host = dev_get_drvdata(dev);
> > struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > struct sdhci_at91_priv *priv = pltfm_host->priv;
> > -   int ret;
> > +   int ret = 0;
> >
> > -   ret = sdhci_runtime_suspend_host(host);
> > +   /*
> > +* If we call sdhci_runtime_suspend(), we could wakeup only if sdio
> > +* irqs are enabled. If we want to wakeup on a card detect event 
> > there
> > +* are two options:
> > +* - do not call sdhci_runtime_suspend() but save power by disabling
> > +*   all clocks excepting the one for the controller interface.
> > +* - call sdhci_runtime_suspend(), save maximum power by disabling
> > +*   all clocks but use a gpio for the card detect signal to have a 
> > way
> > +*   to wakeup.
> > +*/
> > +   if (sdhci_at91_use_sdhci_runtime(host)) {
> 
> I am not sure this approach is safe, particularly for cases when
> sdhci_runtime_suspend() isn't invoked.
> 
> As I understand it, in those cases potentially the sdhci's IRQ handler
> may be invoked to serve even other IRQs than a card detect IRQ. Doing
> that while being runtime suspended doesn't seem like a good idea. Will
> it even work?
> 

Yes it is the idea. It will allow to save some power. I don't see how I can use
runtime PM if I need to invoke sdhci_runtime_suspend_host() without modifying
sdhci layer to handle card detect irq.

I was also afraid to have some side effects but it works, at least for the
card detect case.

> > +   ret = sdhci_runtime_suspend_host(host);
> > +   clk_disable_unprepare(priv->hclock);
> > +   }
> >
> > clk_disable_unprepare(priv->gck);
> > -   clk_disable_unprepare(priv->hclock);
> > clk_disable_unprepare(priv->mainck);
> >
> > return ret;
> > @@ -84,19 +104,23 @@ static int sdhci_at91_runtime_resume(struct device 
> > *dev)
> > return ret;
> > }
> >
> > -   ret = clk_prepare_enable(priv->hclock);
> > -   if (ret) {
> > -   dev_err(dev, "can't enable hclock\n");
> > -   return ret;
> > -   }
> > -
> > ret = clk_prepare_enable(priv->gck);
> > if (ret) {
> > dev_err(dev, "can't enable gck\n");
> > return ret;
> > }
> >
> > -   return sdhci_runtime_resume_host(host);
> > +   if (sdhci_at91_use_sdhci_runtime(host)) {
> > +   ret = clk_prepare_enable(priv->hclock);
> > +   if (ret) {
> > +   dev_err(dev, "can't enable hclock\n");
> > +   return ret;
> > +   }
> > +
> > +   ret = sdhci_runtime_resume_host(host);
> > +   }
> > +
> > +   return ret;
> >  }
> >  #endif /* CONFIG_PM */

Regards

Ludovic

Re: [PATCH] mmc: sdhci-of-at91: fix wakeup issue when using runtime pm

[Ulf Hansson]

On 13 February 2016 at 10:56, Ludovic Desroches
 wrote:
> When suspending the sdhci host, the only hardware event that could wake
> up the host is a sdio irq if they are enabled. If we want to wakeup on
> card detect events, a gpio as to be used.
> If we don't want to use a gpio but the card detect pio of the controller
> then we need to keep enabled the clock of the controller interface to
> get the interrupt and to not set the host in a suspended state to have the
> interrupt handled.
>
> Signed-off-by: Ludovic Desroches 
> ---
>  drivers/mmc/host/sdhci-of-at91.c | 46 
> ++--
>  1 file changed, 35 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-of-at91.c 
> b/drivers/mmc/host/sdhci-of-at91.c
> index efec736..2159c6e 100644
> --- a/drivers/mmc/host/sdhci-of-at91.c
> +++ b/drivers/mmc/host/sdhci-of-at91.c
> @@ -18,6 +18,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -45,7 +46,6 @@ static const struct sdhci_ops sdhci_at91_sama5d2_ops = {
>
>  static const struct sdhci_pltfm_data soc_data_sama5d2 = {
> .ops = _at91_sama5d2_ops,
> -   .quirks = SDHCI_QUIRK_BROKEN_CARD_DETECTION,

You probably have some leftovers from earlier local changes, as this
isn't going to apply to my next branch.

> .quirks2 = SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST,
>  };
>
> @@ -55,17 +55,37 @@ static const struct of_device_id sdhci_at91_dt_match[] = {
>  };
>
>  #ifdef CONFIG_PM
> +static bool sdhci_at91_use_sdhci_runtime(struct sdhci_host *host)
> +{
> +   u32 caps = host->mmc->caps;
> +
> +   return (caps & MMC_CAP_NONREMOVABLE) ||
> +  (!IS_ERR_VALUE(mmc_gpio_get_cd(host->mmc)));
> +}
> +
>  static int sdhci_at91_runtime_suspend(struct device *dev)
>  {
> struct sdhci_host *host = dev_get_drvdata(dev);
> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> struct sdhci_at91_priv *priv = pltfm_host->priv;
> -   int ret;
> +   int ret = 0;
>
> -   ret = sdhci_runtime_suspend_host(host);
> +   /*
> +* If we call sdhci_runtime_suspend(), we could wakeup only if sdio
> +* irqs are enabled. If we want to wakeup on a card detect event there
> +* are two options:
> +* - do not call sdhci_runtime_suspend() but save power by disabling
> +*   all clocks excepting the one for the controller interface.
> +* - call sdhci_runtime_suspend(), save maximum power by disabling
> +*   all clocks but use a gpio for the card detect signal to have a 
> way
> +*   to wakeup.
> +*/
> +   if (sdhci_at91_use_sdhci_runtime(host)) {

I am not sure this approach is safe, particularly for cases when
sdhci_runtime_suspend() isn't invoked.

As I understand it, in those cases potentially the sdhci's IRQ handler
may be invoked to serve even other IRQs than a card detect IRQ. Doing
that while being runtime suspended doesn't seem like a good idea. Will
it even work?

> +   ret = sdhci_runtime_suspend_host(host);
> +   clk_disable_unprepare(priv->hclock);
> +   }
>
> clk_disable_unprepare(priv->gck);
> -   clk_disable_unprepare(priv->hclock);
> clk_disable_unprepare(priv->mainck);
>
> return ret;
> @@ -84,19 +104,23 @@ static int sdhci_at91_runtime_resume(struct device *dev)
> return ret;
> }
>
> -   ret = clk_prepare_enable(priv->hclock);
> -   if (ret) {
> -   dev_err(dev, "can't enable hclock\n");
> -   return ret;
> -   }
> -
> ret = clk_prepare_enable(priv->gck);
> if (ret) {
> dev_err(dev, "can't enable gck\n");
> return ret;
> }
>
> -   return sdhci_runtime_resume_host(host);
> +   if (sdhci_at91_use_sdhci_runtime(host)) {
> +   ret = clk_prepare_enable(priv->hclock);
> +   if (ret) {
> +   dev_err(dev, "can't enable hclock\n");
> +   return ret;
> +   }
> +
> +   ret = sdhci_runtime_resume_host(host);
> +   }
> +
> +   return ret;
>  }
>  #endif /* CONFIG_PM */
>
> --
> 2.7.0
>

Kind regards
Uffe

Re: [PATCH] mmc: sdhci-of-at91: fix wakeup issue when using runtime pm

[Ulf Hansson]

On 13 February 2016 at 10:56, Ludovic Desroches
 wrote:
> When suspending the sdhci host, the only hardware event that could wake
> up the host is a sdio irq if they are enabled. If we want to wakeup on
> card detect events, a gpio as to be used.
> If we don't want to use a gpio but the card detect pio of the controller
> then we need to keep enabled the clock of the controller interface to
> get the interrupt and to not set the host in a suspended state to have the
> interrupt handled.
>
> Signed-off-by: Ludovic Desroches 
> ---
>  drivers/mmc/host/sdhci-of-at91.c | 46 
> ++--
>  1 file changed, 35 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-of-at91.c 
> b/drivers/mmc/host/sdhci-of-at91.c
> index efec736..2159c6e 100644
> --- a/drivers/mmc/host/sdhci-of-at91.c
> +++ b/drivers/mmc/host/sdhci-of-at91.c
> @@ -18,6 +18,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -45,7 +46,6 @@ static const struct sdhci_ops sdhci_at91_sama5d2_ops = {
>
>  static const struct sdhci_pltfm_data soc_data_sama5d2 = {
> .ops = _at91_sama5d2_ops,
> -   .quirks = SDHCI_QUIRK_BROKEN_CARD_DETECTION,

You probably have some leftovers from earlier local changes, as this
isn't going to apply to my next branch.

> .quirks2 = SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST,
>  };
>
> @@ -55,17 +55,37 @@ static const struct of_device_id sdhci_at91_dt_match[] = {
>  };
>
>  #ifdef CONFIG_PM
> +static bool sdhci_at91_use_sdhci_runtime(struct sdhci_host *host)
> +{
> +   u32 caps = host->mmc->caps;
> +
> +   return (caps & MMC_CAP_NONREMOVABLE) ||
> +  (!IS_ERR_VALUE(mmc_gpio_get_cd(host->mmc)));
> +}
> +
>  static int sdhci_at91_runtime_suspend(struct device *dev)
>  {
> struct sdhci_host *host = dev_get_drvdata(dev);
> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> struct sdhci_at91_priv *priv = pltfm_host->priv;
> -   int ret;
> +   int ret = 0;
>
> -   ret = sdhci_runtime_suspend_host(host);
> +   /*
> +* If we call sdhci_runtime_suspend(), we could wakeup only if sdio
> +* irqs are enabled. If we want to wakeup on a card detect event there
> +* are two options:
> +* - do not call sdhci_runtime_suspend() but save power by disabling
> +*   all clocks excepting the one for the controller interface.
> +* - call sdhci_runtime_suspend(), save maximum power by disabling
> +*   all clocks but use a gpio for the card detect signal to have a 
> way
> +*   to wakeup.
> +*/
> +   if (sdhci_at91_use_sdhci_runtime(host)) {

I am not sure this approach is safe, particularly for cases when
sdhci_runtime_suspend() isn't invoked.

As I understand it, in those cases potentially the sdhci's IRQ handler
may be invoked to serve even other IRQs than a card detect IRQ. Doing
that while being runtime suspended doesn't seem like a good idea. Will
it even work?

> +   ret = sdhci_runtime_suspend_host(host);
> +   clk_disable_unprepare(priv->hclock);
> +   }
>
> clk_disable_unprepare(priv->gck);
> -   clk_disable_unprepare(priv->hclock);
> clk_disable_unprepare(priv->mainck);
>
> return ret;
> @@ -84,19 +104,23 @@ static int sdhci_at91_runtime_resume(struct device *dev)
> return ret;
> }
>
> -   ret = clk_prepare_enable(priv->hclock);
> -   if (ret) {
> -   dev_err(dev, "can't enable hclock\n");
> -   return ret;
> -   }
> -
> ret = clk_prepare_enable(priv->gck);
> if (ret) {
> dev_err(dev, "can't enable gck\n");
> return ret;
> }
>
> -   return sdhci_runtime_resume_host(host);
> +   if (sdhci_at91_use_sdhci_runtime(host)) {
> +   ret = clk_prepare_enable(priv->hclock);
> +   if (ret) {
> +   dev_err(dev, "can't enable hclock\n");
> +   return ret;
> +   }
> +
> +   ret = sdhci_runtime_resume_host(host);
> +   }
> +
> +   return ret;
>  }
>  #endif /* CONFIG_PM */
>
> --
> 2.7.0
>

Kind regards
Uffe