[RFC PATCH] mmc: sdhci-of-at91: don't put device in suspend after probe

[RFC PATCH] mmc: sdhci-of-at91: don't put device in suspend after probe

[Ludovic Desroches]

By putting the device in suspend at the end of the probe, it is
impossible to wake up on non software event such as card
insertion/removal.

Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
---

Hi,

Since I had no feedback on this topic:
http://permalink.gmane.org/gmane.linux.kernel.mmc/35160

I would like to no more put the device in suspend at the end of the probe. If
my device is suspended at the end of the probe, I have no issue to resume on
a software event such as mounting my sdcard but hardware event such as card
insertion and removal do not trigger a resume.

It seems there are only two sdhci drivers using runtime pm so maybe nobody has
noticed this issue.

Regards

Ludovic

 drivers/mmc/host/sdhci-of-at91.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
index 9cb86fb..ae24dea 100644
--- a/drivers/mmc/host/sdhci-of-at91.c
+++ b/drivers/mmc/host/sdhci-of-at91.c
@@ -210,8 +210,6 @@ static int sdhci_at91_probe(struct platform_device *pdev)
 	if (ret)
 		goto pm_runtime_disable;
 
-	pm_runtime_put_autosuspend(&pdev->dev);
-
 	return 0;
 
 pm_runtime_disable:
-- 
2.7.0

Re: [RFC PATCH] mmc: sdhci-of-at91: don't put device in suspend after probe

[Adrian Hunter]

On 10/02/16 11:58, Ludovic Desroches wrote:
> By putting the device in suspend at the end of the probe, it is
> impossible to wake up on non software event such as card
> insertion/removal.
> 
> Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
> ---
> 
> Hi,
> 
> Since I had no feedback on this topic:
> http://permalink.gmane.org/gmane.linux.kernel.mmc/35160
> 
> I would like to no more put the device in suspend at the end of the probe. If
> my device is suspended at the end of the probe, I have no issue to resume on
> a software event such as mounting my sdcard but hardware event such as card
> insertion and removal do not trigger a resume.

You can't use runtime PM unless you have a way to wake-up.

Currently, sdhci disables card detect interrupts when runtime suspended,
and drivers use a card-detect GPIO to wake-up.


> 
> It seems there are only two sdhci drivers using runtime pm so maybe nobody has
> noticed this issue.
> 
> Regards
> 
> Ludovic
> 
>  drivers/mmc/host/sdhci-of-at91.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
> index 9cb86fb..ae24dea 100644
> --- a/drivers/mmc/host/sdhci-of-at91.c
> +++ b/drivers/mmc/host/sdhci-of-at91.c
> @@ -210,8 +210,6 @@ static int sdhci_at91_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto pm_runtime_disable;
>  
> -	pm_runtime_put_autosuspend(&pdev->dev);
> -
>  	return 0;
>  
>  pm_runtime_disable:
> 

Re: [RFC PATCH] mmc: sdhci-of-at91: don't put device in suspend after probe

[Ludovic Desroches]

Hi Adrian,

On Wed, Feb 10, 2016 at 01:50:44PM +0200, Adrian Hunter wrote:
> On 10/02/16 11:58, Ludovic Desroches wrote:
> > By putting the device in suspend at the end of the probe, it is
> > impossible to wake up on non software event such as card
> > insertion/removal.
> > 
> > Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
> > ---
> > 
> > Hi,
> > 
> > Since I had no feedback on this topic:
> > http://permalink.gmane.org/gmane.linux.kernel.mmc/35160
> > 
> > I would like to no more put the device in suspend at the end of the probe. If
> > my device is suspended at the end of the probe, I have no issue to resume on
> > a software event such as mounting my sdcard but hardware event such as card
> > insertion and removal do not trigger a resume.
> 
> You can't use runtime PM unless you have a way to wake-up.
> 

Thanks for your feedback. I am a bit disappointed since Ulf advised me to use
runtime PM instead of system PM.

> Currently, sdhci disables card detect interrupts when runtime suspended,
> and drivers use a card-detect GPIO to wake-up.
> 

It is what I have seen going through the sdhci layer. So next question is:
is it normal to not take care of card detect interrupts? We keep enabled
some IRQs probably for SDIO modules IRQ but not for card detection. I
don't understand the reason.

> 
> > 
> > It seems there are only two sdhci drivers using runtime pm so maybe nobody has
> > noticed this issue.
> > 
> > Regards
> > 
> > Ludovic
> > 
> >  drivers/mmc/host/sdhci-of-at91.c | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
> > index 9cb86fb..ae24dea 100644
> > --- a/drivers/mmc/host/sdhci-of-at91.c
> > +++ b/drivers/mmc/host/sdhci-of-at91.c
> > @@ -210,8 +210,6 @@ static int sdhci_at91_probe(struct platform_device *pdev)
> >  	if (ret)
> >  		goto pm_runtime_disable;
> >  
> > -	pm_runtime_put_autosuspend(&pdev->dev);
> > -
> >  	return 0;
> >  
> >  pm_runtime_disable:
> > 
> 

Re: [RFC PATCH] mmc: sdhci-of-at91: don't put device in suspend after probe

[Adrian Hunter]

On 10/02/16 14:51, Ludovic Desroches wrote:
> Hi Adrian,
> 
> On Wed, Feb 10, 2016 at 01:50:44PM +0200, Adrian Hunter wrote:
>> On 10/02/16 11:58, Ludovic Desroches wrote:
>>> By putting the device in suspend at the end of the probe, it is
>>> impossible to wake up on non software event such as card
>>> insertion/removal.
>>>
>>> Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
>>> ---
>>>
>>> Hi,
>>>
>>> Since I had no feedback on this topic:
>>> http://permalink.gmane.org/gmane.linux.kernel.mmc/35160
>>>
>>> I would like to no more put the device in suspend at the end of the probe. If
>>> my device is suspended at the end of the probe, I have no issue to resume on
>>> a software event such as mounting my sdcard but hardware event such as card
>>> insertion and removal do not trigger a resume.
>>
>> You can't use runtime PM unless you have a way to wake-up.
>>
> 
> Thanks for your feedback. I am a bit disappointed since Ulf advised me to use
> runtime PM instead of system PM.
> 
>> Currently, sdhci disables card detect interrupts when runtime suspended,
>> and drivers use a card-detect GPIO to wake-up.
>>
> 
> It is what I have seen going through the sdhci layer. So next question is:
> is it normal to not take care of card detect interrupts? We keep enabled
> some IRQs probably for SDIO modules IRQ but not for card detection. I
> don't understand the reason.

Does sdhci-of-at91.c generate card detect interrupts while runtime suspended?

Re: [RFC PATCH] mmc: sdhci-of-at91: don't put device in suspend after probe

[Ludovic Desroches]

On Wed, Feb 10, 2016 at 03:00:09PM +0200, Adrian Hunter wrote:
> On 10/02/16 14:51, Ludovic Desroches wrote:
> > Hi Adrian,
> > 
> > On Wed, Feb 10, 2016 at 01:50:44PM +0200, Adrian Hunter wrote:
> >> On 10/02/16 11:58, Ludovic Desroches wrote:
> >>> By putting the device in suspend at the end of the probe, it is
> >>> impossible to wake up on non software event such as card
> >>> insertion/removal.
> >>>
> >>> Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
> >>> ---
> >>>
> >>> Hi,
> >>>
> >>> Since I had no feedback on this topic:
> >>> http://permalink.gmane.org/gmane.linux.kernel.mmc/35160
> >>>
> >>> I would like to no more put the device in suspend at the end of the probe. If
> >>> my device is suspended at the end of the probe, I have no issue to resume on
> >>> a software event such as mounting my sdcard but hardware event such as card
> >>> insertion and removal do not trigger a resume.
> >>
> >> You can't use runtime PM unless you have a way to wake-up.
> >>
> > 
> > Thanks for your feedback. I am a bit disappointed since Ulf advised me to use
> > runtime PM instead of system PM.
> > 
> >> Currently, sdhci disables card detect interrupts when runtime suspended,
> >> and drivers use a card-detect GPIO to wake-up.
> >>
> > 
> > It is what I have seen going through the sdhci layer. So next question is:
> > is it normal to not take care of card detect interrupts? We keep enabled
> > some IRQs probably for SDIO modules IRQ but not for card detection. I
> > don't understand the reason.
> 
> Does sdhci-of-at91.c generate card detect interrupts while runtime suspended?

It could if I keep one or two clocks enabled (need extra tests to be
sure about the clock needed to get this interrupt). I have tried to modify
quickly sdhci_runtime_suspend_host() to enable card insertion/removal,
there is an irq generated but the kernel complains this irq is not
handled. I didn't dig further, it was only to do a simple test.

> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] mmc: sdhci-of-at91: don't put device in suspend after probe

[Adrian Hunter]

On 10/02/16 16:16, Ludovic Desroches wrote:
> On Wed, Feb 10, 2016 at 03:00:09PM +0200, Adrian Hunter wrote:
>> On 10/02/16 14:51, Ludovic Desroches wrote:
>>> Hi Adrian,
>>>
>>> On Wed, Feb 10, 2016 at 01:50:44PM +0200, Adrian Hunter wrote:
>>>> On 10/02/16 11:58, Ludovic Desroches wrote:
>>>>> By putting the device in suspend at the end of the probe, it is
>>>>> impossible to wake up on non software event such as card
>>>>> insertion/removal.
>>>>>
>>>>> Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
>>>>> ---
>>>>>
>>>>> Hi,
>>>>>
>>>>> Since I had no feedback on this topic:
>>>>> http://permalink.gmane.org/gmane.linux.kernel.mmc/35160
>>>>>
>>>>> I would like to no more put the device in suspend at the end of the probe. If
>>>>> my device is suspended at the end of the probe, I have no issue to resume on
>>>>> a software event such as mounting my sdcard but hardware event such as card
>>>>> insertion and removal do not trigger a resume.
>>>>
>>>> You can't use runtime PM unless you have a way to wake-up.
>>>>
>>>
>>> Thanks for your feedback. I am a bit disappointed since Ulf advised me to use
>>> runtime PM instead of system PM.
>>>
>>>> Currently, sdhci disables card detect interrupts when runtime suspended,
>>>> and drivers use a card-detect GPIO to wake-up.
>>>>
>>>
>>> It is what I have seen going through the sdhci layer. So next question is:
>>> is it normal to not take care of card detect interrupts? We keep enabled
>>> some IRQs probably for SDIO modules IRQ but not for card detection. I
>>> don't understand the reason.
>>
>> Does sdhci-of-at91.c generate card detect interrupts while runtime suspended?
> 
> It could if I keep one or two clocks enabled (need extra tests to be
> sure about the clock needed to get this interrupt). I have tried to modify
> quickly sdhci_runtime_suspend_host() to enable card insertion/removal,
> there is an irq generated but the kernel complains this irq is not
> handled. I didn't dig further, it was only to do a simple test.

sdhci_irq() returns immediately if (host->runtime_suspended &&
!sdhci_sdio_irq_enabled(host))

Re: [RFC PATCH] mmc: sdhci-of-at91: don't put device in suspend after probe

[Ludovic Desroches]

On Wed, Feb 10, 2016 at 04:20:43PM +0200, Adrian Hunter wrote:
> On 10/02/16 16:16, Ludovic Desroches wrote:
> > On Wed, Feb 10, 2016 at 03:00:09PM +0200, Adrian Hunter wrote:
> >> On 10/02/16 14:51, Ludovic Desroches wrote:
> >>> Hi Adrian,
> >>>
> >>> On Wed, Feb 10, 2016 at 01:50:44PM +0200, Adrian Hunter wrote:
> >>>> On 10/02/16 11:58, Ludovic Desroches wrote:
> >>>>> By putting the device in suspend at the end of the probe, it is
> >>>>> impossible to wake up on non software event such as card
> >>>>> insertion/removal.
> >>>>>
> >>>>> Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
> >>>>> ---
> >>>>>
> >>>>> Hi,
> >>>>>
> >>>>> Since I had no feedback on this topic:
> >>>>> http://permalink.gmane.org/gmane.linux.kernel.mmc/35160
> >>>>>
> >>>>> I would like to no more put the device in suspend at the end of the probe. If
> >>>>> my device is suspended at the end of the probe, I have no issue to resume on
> >>>>> a software event such as mounting my sdcard but hardware event such as card
> >>>>> insertion and removal do not trigger a resume.
> >>>>
> >>>> You can't use runtime PM unless you have a way to wake-up.
> >>>>
> >>>
> >>> Thanks for your feedback. I am a bit disappointed since Ulf advised me to use
> >>> runtime PM instead of system PM.
> >>>
> >>>> Currently, sdhci disables card detect interrupts when runtime suspended,
> >>>> and drivers use a card-detect GPIO to wake-up.
> >>>>
> >>>
> >>> It is what I have seen going through the sdhci layer. So next question is:
> >>> is it normal to not take care of card detect interrupts? We keep enabled
> >>> some IRQs probably for SDIO modules IRQ but not for card detection. I
> >>> don't understand the reason.
> >>
> >> Does sdhci-of-at91.c generate card detect interrupts while runtime suspended?
> > 
> > It could if I keep one or two clocks enabled (need extra tests to be
> > sure about the clock needed to get this interrupt). I have tried to modify
> > quickly sdhci_runtime_suspend_host() to enable card insertion/removal,
> > there is an irq generated but the kernel complains this irq is not
> > handled. I didn't dig further, it was only to do a simple test.
> 
> sdhci_irq() returns immediately if (host->runtime_suspended &&
> !sdhci_sdio_irq_enabled(host))

Ok so do I have to rework this part to allow wake up on card detect or
do I have to remove runtime PM from my driver?

> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] mmc: sdhci-of-at91: don't put device in suspend after probe

[Ulf Hansson]

On 10 February 2016 at 13:51, Ludovic Desroches
<ludovic.desroches@atmel.com> wrote:
> Hi Adrian,
>
> On Wed, Feb 10, 2016 at 01:50:44PM +0200, Adrian Hunter wrote:
>> On 10/02/16 11:58, Ludovic Desroches wrote:
>> > By putting the device in suspend at the end of the probe, it is
>> > impossible to wake up on non software event such as card
>> > insertion/removal.
>> >
>> > Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
>> > ---
>> >
>> > Hi,
>> >
>> > Since I had no feedback on this topic:
>> > http://permalink.gmane.org/gmane.linux.kernel.mmc/35160
>> >
>> > I would like to no more put the device in suspend at the end of the probe. If
>> > my device is suspended at the end of the probe, I have no issue to resume on
>> > a software event such as mounting my sdcard but hardware event such as card
>> > insertion and removal do not trigger a resume.
>>
>> You can't use runtime PM unless you have a way to wake-up.
>>
>
> Thanks for your feedback. I am a bit disappointed since Ulf advised me to use
> runtime PM instead of system PM.

Sorry about that, but of course I can't know exactly how your hardware work.

>
>> Currently, sdhci disables card detect interrupts when runtime suspended,
>> and drivers use a card-detect GPIO to wake-up.
>>
>
> It is what I have seen going through the sdhci layer. So next question is:
> is it normal to not take care of card detect interrupts? We keep enabled
> some IRQs probably for SDIO modules IRQ but not for card detection. I
> don't understand the reason.

If SDIO IRQ is enabled the mmc controller needs to stay runtime
resumed (as it's the mmc controller that monitors the IRQ), unless you
can re-configure the SDIO IRQ as a wakeup. For example by re-routing
it to a GPIO irq.
Whether this wakeup configuration can stay the same between system PM
and runtime PM is SoC dependent.

Regarding card detects in runtime PM:

If you have an option to use GPIO IRQs or it's possible to configure
the card detect IRQ as a wakeup in any other way, runtime PM works
fine.

Now, when the card detect *can't* be configured as a wakeup in runtime
suspend mode, there are two options.

1) Rely on using MMC_CAP_NEEDS_POLL.
2) Prevent runtime PM.

Which option that's preferred is SoC/ mmc controller dependent.
Although but please consider below recommendations.

- In some cases using polling works really well, as the the mmc core
get fast/easy information about whether a card is inserted or not, via
the ->get_cd() host ops.

- In some cases ->get_cd() is broken (or not implemented) and always
returns a value indicating a card is inserted. That means external
regulators for the card must be enabled and a card initialization
sequence needs to be executed to find out whether a card was *really*
inserted.

So to conclude, if the controller supports card detection but without
wakeup support and the polling mode sucks, then it probably better to
prevent runtime PM. Otherwise polling is probably better.

Regarding card detect in system PM:
If the SoC supports card detect IRQs being configured as wakeup, it's
recommended to do that!

If it can't support wakeup, no worries! The mmc core will run a check
for card insert/removal after a system PM resume sequence is
completed.

>
>>
>> >
>> > It seems there are only two sdhci drivers using runtime pm so maybe nobody has
>> > noticed this issue.

In more general, I wouldn't be surprised if the PM related code in
sdhci is fragile/broken for some SoC/sdhci variants. Although, it's
just an impression I get by studying the code.

>> >
>> > Regards
>> >
>> > Ludovic
>> >
>> >  drivers/mmc/host/sdhci-of-at91.c | 2 --
>> >  1 file changed, 2 deletions(-)
>> >
>> > diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
>> > index 9cb86fb..ae24dea 100644
>> > --- a/drivers/mmc/host/sdhci-of-at91.c
>> > +++ b/drivers/mmc/host/sdhci-of-at91.c
>> > @@ -210,8 +210,6 @@ static int sdhci_at91_probe(struct platform_device *pdev)
>> >     if (ret)
>> >             goto pm_runtime_disable;
>> >
>> > -   pm_runtime_put_autosuspend(&pdev->dev);
>> > -

1) Before doing this consider the option to use polling. (MMC_CAP_NEEDS_POLL).

2) The driver may also handle non-removable cards (depending on the
SoC configuration). In those cases it's perfectly okay to use runtime
PM. So, I think you need some conditional check before deciding to
prevent runtime PM. And don't forget to restore the usage count in
->remove().

>> >     return 0;
>> >
>> >  pm_runtime_disable:
>> >
>>

Kind regards
Uffe

Re: [RFC PATCH] mmc: sdhci-of-at91: don't put device in suspend after probe

[Ludovic Desroches]

Hi Ulf,

On Wed, Feb 10, 2016 at 04:56:11PM +0100, Ulf Hansson wrote:
> On 10 February 2016 at 13:51, Ludovic Desroches
> <ludovic.desroches@atmel.com> wrote:
> > Hi Adrian,
> >
> > On Wed, Feb 10, 2016 at 01:50:44PM +0200, Adrian Hunter wrote:
> >> On 10/02/16 11:58, Ludovic Desroches wrote:
> >> > By putting the device in suspend at the end of the probe, it is
> >> > impossible to wake up on non software event such as card
> >> > insertion/removal.
> >> >
> >> > Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
> >> > ---
> >> >
> >> > Hi,
> >> >
> >> > Since I had no feedback on this topic:
> >> > http://permalink.gmane.org/gmane.linux.kernel.mmc/35160
> >> >
> >> > I would like to no more put the device in suspend at the end of the probe. If
> >> > my device is suspended at the end of the probe, I have no issue to resume on
> >> > a software event such as mounting my sdcard but hardware event such as card
> >> > insertion and removal do not trigger a resume.
> >>
> >> You can't use runtime PM unless you have a way to wake-up.
> >>
> >
> > Thanks for your feedback. I am a bit disappointed since Ulf advised me to use
> > runtime PM instead of system PM.
> 
> Sorry about that, but of course I can't know exactly how your hardware work.
> 

Of course you can't know the hardware of all SoC vendors! No problem
about that. In my mind it is not dependant of how my hardware works (I mean at
SoC level), if someone decides to not use the card detect of the host and to
use a gpio instead, of course he can.

> >
> >> Currently, sdhci disables card detect interrupts when runtime suspended,
> >> and drivers use a card-detect GPIO to wake-up.
> >>
> >
> > It is what I have seen going through the sdhci layer. So next question is:
> > is it normal to not take care of card detect interrupts? We keep enabled
> > some IRQs probably for SDIO modules IRQ but not for card detection. I
> > don't understand the reason.
> 
> If SDIO IRQ is enabled the mmc controller needs to stay runtime
> resumed (as it's the mmc controller that monitors the IRQ), unless you
> can re-configure the SDIO IRQ as a wakeup. For example by re-routing
> it to a GPIO irq.
> Whether this wakeup configuration can stay the same between system PM
> and runtime PM is SoC dependent.

I don't know if we are talking about the same thing. In
sdhci_runtime_suspend_host(), we set SDHCI_INT_CARD_INT as a kind of
wakeup irq before considering the host as suspended
(host->runtime_suspended = true), isn't it?

Why not adding SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE?

In my case, it allows me to use runtime PM to disable two clocks and
keep one enabled (I need this one to get the irq) when suspending the device.

> 
> Regarding card detects in runtime PM:
> 
> If you have an option to use GPIO IRQs or it's possible to configure
> the card detect IRQ as a wakeup in any other way, runtime PM works
> fine.
> 

It will depend of the customer but I am not sure I'll want to use a pio
as a gpio for this if there is a pio routed to the sdhci controller.

> Now, when the card detect *can't* be configured as a wakeup in runtime
> suspend mode, there are two options.
> 
> 1) Rely on using MMC_CAP_NEEDS_POLL.
> 2) Prevent runtime PM.
> 
> Which option that's preferred is SoC/ mmc controller dependent.
> Although but please consider below recommendations.
> 
> - In some cases using polling works really well, as the the mmc core
> get fast/easy information about whether a card is inserted or not, via
> the ->get_cd() host ops.
> 
> - In some cases ->get_cd() is broken (or not implemented) and always
> returns a value indicating a card is inserted. That means external
> regulators for the card must be enabled and a card initialization
> sequence needs to be executed to find out whether a card was *really*
> inserted.
> 
> So to conclude, if the controller supports card detection but without
> wakeup support and the polling mode sucks, then it probably better to
> prevent runtime PM. Otherwise polling is probably better.
> 

It is weird to claim that I need polling since I have a working card
detect.

> Regarding card detect in system PM:
> If the SoC supports card detect IRQs being configured as wakeup, it's
> recommended to do that!
> 
> If it can't support wakeup, no worries! The mmc core will run a check
> for card insert/removal after a system PM resume sequence is
> completed.
> 
> >
> >>
> >> >
> >> > It seems there are only two sdhci drivers using runtime pm so maybe nobody has
> >> > noticed this issue.
> 
> In more general, I wouldn't be surprised if the PM related code in
> sdhci is fragile/broken for some SoC/sdhci variants. Although, it's
> just an impression I get by studying the code.
> 
> >> >
> >> > Regards
> >> >
> >> > Ludovic
> >> >
> >> >  drivers/mmc/host/sdhci-of-at91.c | 2 --
> >> >  1 file changed, 2 deletions(-)
> >> >
> >> > diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
> >> > index 9cb86fb..ae24dea 100644
> >> > --- a/drivers/mmc/host/sdhci-of-at91.c
> >> > +++ b/drivers/mmc/host/sdhci-of-at91.c
> >> > @@ -210,8 +210,6 @@ static int sdhci_at91_probe(struct platform_device *pdev)
> >> >     if (ret)
> >> >             goto pm_runtime_disable;
> >> >
> >> > -   pm_runtime_put_autosuspend(&pdev->dev);
> >> > -
> 
> 1) Before doing this consider the option to use polling. (MMC_CAP_NEEDS_POLL).
> 
> 2) The driver may also handle non-removable cards (depending on the
> SoC configuration). In those cases it's perfectly okay to use runtime
> PM. So, I think you need some conditional check before deciding to
> prevent runtime PM. And don't forget to restore the usage count in
> ->remove().
> 

I have to think more about the other cases. I know we have some issues
with a wifi SDIO module and runtime PM but I have to investigate on this
topic but I suspect something as the wifi driver doesn't claim it is
using the sdhci controller.

> >> >     return 0;
> >> >
> >> >  pm_runtime_disable:
> >> >
> >>
> 
> Kind regards
> Uffe

Regards

Ludovic

Re: [RFC PATCH] mmc: sdhci-of-at91: don't put device in suspend after probe

[Ulf Hansson]

[...]

>> >
>> >> Currently, sdhci disables card detect interrupts when runtime suspended,
>> >> and drivers use a card-detect GPIO to wake-up.
>> >>
>> >
>> > It is what I have seen going through the sdhci layer. So next question is:
>> > is it normal to not take care of card detect interrupts? We keep enabled
>> > some IRQs probably for SDIO modules IRQ but not for card detection. I
>> > don't understand the reason.
>>
>> If SDIO IRQ is enabled the mmc controller needs to stay runtime
>> resumed (as it's the mmc controller that monitors the IRQ), unless you
>> can re-configure the SDIO IRQ as a wakeup. For example by re-routing
>> it to a GPIO irq.
>> Whether this wakeup configuration can stay the same between system PM
>> and runtime PM is SoC dependent.
>
> I don't know if we are talking about the same thing. In
> sdhci_runtime_suspend_host(), we set SDHCI_INT_CARD_INT as a kind of
> wakeup irq before considering the host as suspended
> (host->runtime_suspended = true), isn't it?

No, you have got this wrong. The card detect IRQ is disabled at
sdhci_runtime_suspend_host().

It's not related to SDIO irq, as in that case the controller can't be
runtime suspended (unless wakeup is supported).

>
> Why not adding SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE?
>
> In my case, it allows me to use runtime PM to disable two clocks and
> keep one enabled (I need this one to get the irq) when suspending the device.

That seems like a very special case and normally not how runtime PM is used.

So, if you only want to disable|enable some clocks from runtime PM, I
suggest you keep that out of the library function
sdhci_runtime_suspend_host() and deal with that in your driver
instead.

>
>>
>> Regarding card detects in runtime PM:
>>
>> If you have an option to use GPIO IRQs or it's possible to configure
>> the card detect IRQ as a wakeup in any other way, runtime PM works
>> fine.
>>
>
> It will depend of the customer but I am not sure I'll want to use a pio
> as a gpio for this if there is a pio routed to the sdhci controller.

That all has to do with how the SoC is designed from power management
point of view.

In general, it's a good idea to have card detect on GPIO, as it allows
to put other unused parts of the silicon into a low power state.

>
>> Now, when the card detect *can't* be configured as a wakeup in runtime
>> suspend mode, there are two options.
>>
>> 1) Rely on using MMC_CAP_NEEDS_POLL.
>> 2) Prevent runtime PM.
>>
>> Which option that's preferred is SoC/ mmc controller dependent.
>> Although but please consider below recommendations.
>>
>> - In some cases using polling works really well, as the the mmc core
>> get fast/easy information about whether a card is inserted or not, via
>> the ->get_cd() host ops.
>>
>> - In some cases ->get_cd() is broken (or not implemented) and always
>> returns a value indicating a card is inserted. That means external
>> regulators for the card must be enabled and a card initialization
>> sequence needs to be executed to find out whether a card was *really*
>> inserted.
>>
>> So to conclude, if the controller supports card detection but without
>> wakeup support and the polling mode sucks, then it probably better to
>> prevent runtime PM. Otherwise polling is probably better.
>>
>
> It is weird to claim that I need polling since I have a working card
> detect.

It's not, if you really care about saving power.

Although, as I stated, which solution that's best, depends on the SoC.

[...]

So, we have a regression to fix here. I can propose a patch adopting
the above recommendations!?

That's solution doesn’t have to stay long term, as you can try to
optimize it later on to what suits your SoC the best.

Kind regards
Uffe

Re: [RFC PATCH] mmc: sdhci-of-at91: don't put device in suspend after probe

[Ludovic Desroches]

On Thu, Feb 11, 2016 at 10:46:08AM +0100, Ulf Hansson wrote:
> [...]
> 
> >> >
> >> >> Currently, sdhci disables card detect interrupts when runtime suspended,
> >> >> and drivers use a card-detect GPIO to wake-up.
> >> >>
> >> >
> >> > It is what I have seen going through the sdhci layer. So next question is:
> >> > is it normal to not take care of card detect interrupts? We keep enabled
> >> > some IRQs probably for SDIO modules IRQ but not for card detection. I
> >> > don't understand the reason.
> >>
> >> If SDIO IRQ is enabled the mmc controller needs to stay runtime
> >> resumed (as it's the mmc controller that monitors the IRQ), unless you
> >> can re-configure the SDIO IRQ as a wakeup. For example by re-routing
> >> it to a GPIO irq.
> >> Whether this wakeup configuration can stay the same between system PM
> >> and runtime PM is SoC dependent.
> >
> > I don't know if we are talking about the same thing. In
> > sdhci_runtime_suspend_host(), we set SDHCI_INT_CARD_INT as a kind of
> > wakeup irq before considering the host as suspended
> > (host->runtime_suspended = true), isn't it?
> 
> No, you have got this wrong. The card detect IRQ is disabled at
> sdhci_runtime_suspend_host().
> 

Ok for card detect. For my knowledge, what is the purpose of enabling
SHDCI_INT_CARD_INT?

> It's not related to SDIO irq, as in that case the controller can't be
> runtime suspended (unless wakeup is supported).
> 
> >
> > Why not adding SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE?
> >
> > In my case, it allows me to use runtime PM to disable two clocks and
> > keep one enabled (I need this one to get the irq) when suspending the device.
> 
> That seems like a very special case and normally not how runtime PM is used.
> 
> So, if you only want to disable|enable some clocks from runtime PM, I
> suggest you keep that out of the library function
> sdhci_runtime_suspend_host() and deal with that in your driver
> instead.
> 
> >
> >>
> >> Regarding card detects in runtime PM:
> >>
> >> If you have an option to use GPIO IRQs or it's possible to configure
> >> the card detect IRQ as a wakeup in any other way, runtime PM works
> >> fine.
> >>
> >
> > It will depend of the customer but I am not sure I'll want to use a pio
> > as a gpio for this if there is a pio routed to the sdhci controller.
> 
> That all has to do with how the SoC is designed from power management
> point of view.
> 

I don't agree this point. We have connected the card detect to the controller 
on our Xplained board but someone can do another board and use a gpio.

So depending on the board we should use runtime PM or not. As you
suggested, I have to find a clever way to know when I can use runtime PM
or not: non removable device, gpio cd, etc.

> In general, it's a good idea to have card detect on GPIO, as it allows
> to put other unused parts of the silicon into a low power state.
> 
> >
> >> Now, when the card detect *can't* be configured as a wakeup in runtime
> >> suspend mode, there are two options.
> >>
> >> 1) Rely on using MMC_CAP_NEEDS_POLL.
> >> 2) Prevent runtime PM.
> >>
> >> Which option that's preferred is SoC/ mmc controller dependent.
> >> Although but please consider below recommendations.
> >>
> >> - In some cases using polling works really well, as the the mmc core
> >> get fast/easy information about whether a card is inserted or not, via
> >> the ->get_cd() host ops.
> >>
> >> - In some cases ->get_cd() is broken (or not implemented) and always
> >> returns a value indicating a card is inserted. That means external
> >> regulators for the card must be enabled and a card initialization
> >> sequence needs to be executed to find out whether a card was *really*
> >> inserted.
> >>
> >> So to conclude, if the controller supports card detection but without
> >> wakeup support and the polling mode sucks, then it probably better to
> >> prevent runtime PM. Otherwise polling is probably better.
> >>
> >
> > It is weird to claim that I need polling since I have a working card
> > detect.
> 
> It's not, if you really care about saving power.
> 
> Although, as I stated, which solution that's best, depends on the SoC.
> 
> [...]
> 
> So, we have a regression to fix here. I can propose a patch adopting
> the above recommendations!?
> 
> That's solution doesn’t have to stay long term, as you can try to
> optimize it later on to what suits your SoC the best.

Ok I'll have a try with MMC_CAP_NEEDS_POLL and after the investigation
about the sdio module I'll try to propose something better.

Regards

Ludovic