From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752050AbcBKLF7 (ORCPT ); Thu, 11 Feb 2016 06:05:59 -0500 Received: from eusmtp01.atmel.com ([212.144.249.242]:26526 "EHLO eusmtp01.atmel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751212AbcBKLF5 (ORCPT ); Thu, 11 Feb 2016 06:05:57 -0500 Date: Thu, 11 Feb 2016 12:05:53 +0100 From: Ludovic Desroches To: Ulf Hansson CC: Adrian Hunter , linux-mmc , "linux-kernel@vger.kernel.org" , Nicolas Ferre , Ludovic Desroches Subject: Re: [RFC PATCH] mmc: sdhci-of-at91: don't put device in suspend after probe Message-ID: <20160211110553.GH14937@odux.rfo.atmel.com> Mail-Followup-To: Ulf Hansson , Adrian Hunter , linux-mmc , "linux-kernel@vger.kernel.org" , Nicolas Ferre References: <1455098283-14500-1-git-send-email-ludovic.desroches@atmel.com> <56BB2414.1040606@intel.com> <20160210125139.GB14937@odux.rfo.atmel.com> <20160211090856.GG14937@odux.rfo.atmel.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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