| Message ID | 1430841827-7834-1-git-send-email-eliad@wizery.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 5 May 2015 at 18:03, Eliad Peller <eliad@wizery.com> 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. Kind regards Uffe > > Signed-off-by: Eliad Peller <eliad@wizery.com> > Signed-off-by: Ido Yariv <ido@wizery.com> > --- > drivers/mmc/core/core.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c > index c296bc0..797b574 100644 > --- a/drivers/mmc/core/core.c > +++ b/drivers/mmc/core/core.c > @@ -2604,6 +2604,11 @@ int mmc_power_restore_host(struct mmc_host *host) > return -EINVAL; > } > > + if (host->ios.power_mode == MMC_POWER_ON) { > + mmc_bus_put(host); > + return 0; > + } > + > mmc_power_up(host, host->card->ocr); > ret = host->bus_ops->power_restore(host); > > -- > 1.8.5.2.229.g4448466.dirty > -- 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
On Mon, May 11, 2015 at 11:48 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 5 May 2015 at 18:03, Eliad Peller <eliad@wizery.com> 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. 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). 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(). Eliad. -- 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
On 11 May 2015 at 13:07, Eliad Peller <eliad@wizery.com> wrote: > On Mon, May 11, 2015 at 11:48 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> On 5 May 2015 at 18:03, Eliad Peller <eliad@wizery.com> 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? > > 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? Kind regards Uffe -- 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
On Mon, May 11, 2015 at 2:47 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 11 May 2015 at 13:07, Eliad Peller <eliad@wizery.com> wrote: >> On Mon, May 11, 2015 at 11:48 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>> On 5 May 2015 at 18:03, Eliad Peller <eliad@wizery.com> 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. >> >> 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 :) 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) Eliad. -- 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
On 11 May 2015 at 14:02, Eliad Peller <eliad@wizery.com> wrote: > On Mon, May 11, 2015 at 2:47 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> On 11 May 2015 at 13:07, Eliad Peller <eliad@wizery.com> wrote: >>> On Mon, May 11, 2015 at 11:48 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>>> On 5 May 2015 at 18:03, Eliad Peller <eliad@wizery.com> 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? Kind regards Uffe -- 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
On Mon, May 11, 2015 at 4:50 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 11 May 2015 at 14:02, Eliad Peller <eliad@wizery.com> wrote: >> On Mon, May 11, 2015 at 2:47 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>> On 11 May 2015 at 13:07, Eliad Peller <eliad@wizery.com> wrote: >>>> On Mon, May 11, 2015 at 11:48 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>>>> On 5 May 2015 at 18:03, Eliad Peller <eliad@wizery.com> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index c296bc0..797b574 100644 --- a/drivers/mmc/core/core.c +++ b/drivers/mmc/core/core.c @@ -2604,6 +2604,11 @@ int mmc_power_restore_host(struct mmc_host *host) return -EINVAL; } + if (host->ios.power_mode == MMC_POWER_ON) { + mmc_bus_put(host); + return 0; + } + mmc_power_up(host, host->card->ocr); ret = host->bus_ops->power_restore(host);