On Mon, May 11, 2015 at 4:50 PM, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote: > On 11 May 2015 at 14:02, Eliad Peller <eliad@xxxxxxxxxx> wrote: >> On Mon, May 11, 2015 at 2:47 PM, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote: >>> On 11 May 2015 at 13:07, Eliad Peller <eliad@xxxxxxxxxx> wrote: >>>> On Mon, May 11, 2015 at 11:48 AM, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote: >>>>> On 5 May 2015 at 18:03, Eliad Peller <eliad@xxxxxxxxxx> wrote: >>>>>> mmc_power_restore_host() calls mmc_power_up(), which >>>>>> returns immediately if power is already on. >>>>>> >>>>>> However, it still calls host->bus_ops->power_restore, >>>>>> which might result in various errors if the bus_ops >>>>>> doesn't handle it well (e.g. failing to run init >>>>>> sequence twice) >>>>>> >>>>>> Simply bail out in this case, without further calling >>>>>> bus_ops->power_restore. >>>>>> >>>>>> Specifically, this solves issue with wl18xx sdio card, >>>>>> where the mmc core powers on the card on resume (while >>>>>> MMC_PM_KEEP_POWER is not set), and the wl18xx device >>>>>> driver calls mmc_power_restore_host() once more. >>>>> >>>>> Could you elaborate on why that driver calls mmc_power_restore_host() >>>>> after the system PM suspend sequence? I am trying to understand the >>>>> use case. >>>>> >>>> The driver assumes control over the mmc power, in order to save power >>>> when no interface is up. >>> >>> Makes sense! >>> >>>> It basically uses runtime_pm for it, but calls the power functions >>>> explicitly if pm_runtime returned non-zero (this is needed for some >>>> corner cases, e.g. runtime pm is disabled). >>> >>> I have some ideas about changing the way runtime PM shall be used for >>> SDIO func drivers. Instead of using it to control power to the SDIO >>> card, it should be used to deal with "idle operations". >>> >>> That would mean SDIO func drivers would use only >>> mmc_power_save|restore_host() APIs, to control the power to the SDIO >>> card. >>> >>> Do you see any issues with such an approach? >>> >> actually, the current driver code (to use runtime_pm, and then call >> the power functions explicitly in some cases) looks a bit weird. >> so your approach makes sense to me. >> > > Okay, good. > >>>> >>>> On suspend (if wowlan is not configured), all the wlan interfaces are >>>> taken down, and the driver powers off the device. >>>> On resume, the interfaces are taken up again, and the driver powers on >>>> the device, by calling mmc_power_restore_host(). >>> >>> That raises the following question. >>> >>> When mmc_power_save_host() has been called for an SDIO card, should >>> really the mmc core restore power to that card during system PM >>> resume? Isn't it better to leave that to the SDIO func driver? >>> >> yes. but i didn't want to make a major change :) > > I think we need to. :-) > >> >> note that the suspend/resume flow used to work properly. >> iirc, we did some bisect when the issue first showed up, which showed >> it started by: >> 7459026 mmc: core: Push common suspend|resume code into each bus_ops >> (the reason might have been different, but it did used to work before >> this patch) > > Yes, I that's my fault and I am terribly sorry about that. I will try > to fix it asap. > > Unfortunate I don't have HW to test this, but hopefully you or someone > else can help out doing that? > sure. i'll be able to test it. thanks for taking it :) Eliad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
