Message-ID: <20160211110553.GH14937@odux.rfo.atmel.com>
Date:	Thu, 11 Feb 2016 12:05:53 +0100
From:	Ludovic Desroches <ludovic.desroches@...el.com>
To:	Ulf Hansson <ulf.hansson@...aro.org>
CC:	Adrian Hunter <adrian.hunter@...el.com>,
	linux-mmc <linux-mmc@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Nicolas Ferre <nicolas.ferre@...el.com>,
	Ludovic Desroches <ludovic.desroches@...el.com>
Subject: Re: [RFC PATCH] mmc: sdhci-of-at91: don't put device in suspend
 after probe

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

Powered by blists - more mailing lists