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@xxxxxxxxx> 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@xxxxxxxxx>
> >> > ---
> >> >
> >> > 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