diff mbox

mmc: dw_mmc: Don't allow Runtime PM for SDIO cards

Message ID 20170411225543.987-1-dianders@chromium.org
State New, archived
Headers show

Commit Message

Doug Anderson April 11, 2017, 10:55 p.m. UTC
According to the SDIO standard interrupts are normally signalled in a
very complicated way.  They require the card clock to be running and
require the controller to be paying close attention to the signals
coming from the card.  This simply can't happen with the clock stopped
or with the controller in a low power mode.

To that end, we'll disable runtime_pm when we detect that an SDIO card
was inserted.  This is much like with what we do with the special
"SDMMC_CLKEN_LOW_PWR" bit that dw_mmc supports.

NOTE: we specifically do this Runtime PM disabling at card init time
rather than in the enable_sdio_irq() callback.  This is _different_
than how SDHCI does it.  Why do we do it differently?

- Unlike SDHCI, dw_mmc uses the standard sdio_irq code in Linux (AKA
  dw_mmc doesn't set MMC_CAP2_SDIO_IRQ_NOTHREAD).
- Because we use the standard sdio_irq code:
  - We see a constant stream of enable_sdio_irq(0) and
    enable_sdio_irq(1) calls.  This is because the standard code
    disables interrupts while processing and re-enables them after.
  - While interrupts are disabled, there's technically a period where
    we could get runtime disabled while processing interrupts.
  - If we are runtime disabled while processing interrupts, we'll
    reset the controller at resume time (see dw_mci_runtime_resume),
    which seems like a terrible idea because we could possibly have
    another interrupt pending.

To fix the above isues we'd want to put something in the standard
sdio_irq code that makes sure to call pm_runtime get/put when
interrupts are being actively being processed.  That's possible to do,
but it seems like a more complicated mechanism when we really just
want the runtime pm disabled always for SDIO cards given that all the
other bits needed to get Runtime PM vs. SDIO just aren't there.

NOTE: at some point in time someone might come up with a fancy way to
do SDIO interrupts and still allow (some) amount of runtime PM.
Technically we could turn off the card clock if we used an alternate
way of signaling SDIO interrupts (and out of band interrupt is one way
to do this).  We probably wouldn't actually want to fully runtime
suspend in this case though--at least not with the current
dw_mci_runtime_resume() which basically fully resets the controller at
resume time.

Fixes: e9ed8835e990 ("mmc: dw_mmc: add runtime PM callback")
Cc: <stable@vger.kernel.org>
Reported-by: Brian Norris <briannorris@chromium.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
 drivers/mmc/host/dw_mmc.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Comments

Jaehoon Chung April 12, 2017, 5:01 a.m. UTC | #1
On 04/12/2017 07:55 AM, Douglas Anderson wrote:
> According to the SDIO standard interrupts are normally signalled in a
> very complicated way.  They require the card clock to be running and
> require the controller to be paying close attention to the signals
> coming from the card.  This simply can't happen with the clock stopped
> or with the controller in a low power mode.
> 
> To that end, we'll disable runtime_pm when we detect that an SDIO card
> was inserted.  This is much like with what we do with the special
> "SDMMC_CLKEN_LOW_PWR" bit that dw_mmc supports.
> 
> NOTE: we specifically do this Runtime PM disabling at card init time
> rather than in the enable_sdio_irq() callback.  This is _different_
> than how SDHCI does it.  Why do we do it differently?
> 
> - Unlike SDHCI, dw_mmc uses the standard sdio_irq code in Linux (AKA
>   dw_mmc doesn't set MMC_CAP2_SDIO_IRQ_NOTHREAD).
> - Because we use the standard sdio_irq code:
>   - We see a constant stream of enable_sdio_irq(0) and
>     enable_sdio_irq(1) calls.  This is because the standard code
>     disables interrupts while processing and re-enables them after.
>   - While interrupts are disabled, there's technically a period where
>     we could get runtime disabled while processing interrupts.
>   - If we are runtime disabled while processing interrupts, we'll
>     reset the controller at resume time (see dw_mci_runtime_resume),
>     which seems like a terrible idea because we could possibly have
>     another interrupt pending.
> 
> To fix the above isues we'd want to put something in the standard
> sdio_irq code that makes sure to call pm_runtime get/put when
> interrupts are being actively being processed.  That's possible to do,
> but it seems like a more complicated mechanism when we really just
> want the runtime pm disabled always for SDIO cards given that all the
> other bits needed to get Runtime PM vs. SDIO just aren't there.
> 
> NOTE: at some point in time someone might come up with a fancy way to
> do SDIO interrupts and still allow (some) amount of runtime PM.
> Technically we could turn off the card clock if we used an alternate
> way of signaling SDIO interrupts (and out of band interrupt is one way
> to do this).  We probably wouldn't actually want to fully runtime
> suspend in this case though--at least not with the current
> dw_mci_runtime_resume() which basically fully resets the controller at
> resume time.
> 
> Fixes: e9ed8835e990 ("mmc: dw_mmc: add runtime PM callback")
> Cc: <stable@vger.kernel.org>
> Reported-by: Brian Norris <briannorris@chromium.org>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Acked-by: Jaehoon Chung <jh80.chung@samsung.com>

Best Regards,
Jaehoon Chung

> ---
>  drivers/mmc/host/dw_mmc.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 249ded65192e..e45129f48174 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -23,6 +23,7 @@
>  #include <linux/ioport.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/seq_file.h>
>  #include <linux/slab.h>
>  #include <linux/stat.h>
> @@ -1620,10 +1621,16 @@ static void dw_mci_init_card(struct mmc_host *mmc, struct mmc_card *card)
>  
>  		if (card->type == MMC_TYPE_SDIO ||
>  		    card->type == MMC_TYPE_SD_COMBO) {
> -			set_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
> +			if (!test_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags)) {
> +				pm_runtime_get_noresume(mmc->parent);
> +				set_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
> +			}
>  			clk_en_a = clk_en_a_old & ~clken_low_pwr;
>  		} else {
> -			clear_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
> +			if (test_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags)) {
> +				pm_runtime_put_noidle(mmc->parent);
> +				clear_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
> +			}
>  			clk_en_a = clk_en_a_old | clken_low_pwr;
>  		}
>  
> 

--
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
Shawn Lin April 13, 2017, 8:33 a.m. UTC | #2
On 2017/4/12 6:55, Douglas Anderson wrote:
> According to the SDIO standard interrupts are normally signalled in a
> very complicated way.  They require the card clock to be running and
> require the controller to be paying close attention to the signals
> coming from the card.  This simply can't happen with the clock stopped
> or with the controller in a low power mode.
>
> To that end, we'll disable runtime_pm when we detect that an SDIO card
> was inserted.  This is much like with what we do with the special
> "SDMMC_CLKEN_LOW_PWR" bit that dw_mmc supports.
>
> NOTE: we specifically do this Runtime PM disabling at card init time
> rather than in the enable_sdio_irq() callback.  This is _different_
> than how SDHCI does it.  Why do we do it differently?
>
> - Unlike SDHCI, dw_mmc uses the standard sdio_irq code in Linux (AKA
>   dw_mmc doesn't set MMC_CAP2_SDIO_IRQ_NOTHREAD).
> - Because we use the standard sdio_irq code:
>   - We see a constant stream of enable_sdio_irq(0) and
>     enable_sdio_irq(1) calls.  This is because the standard code
>     disables interrupts while processing and re-enables them after.
>   - While interrupts are disabled, there's technically a period where
>     we could get runtime disabled while processing interrupts.
>   - If we are runtime disabled while processing interrupts, we'll
>     reset the controller at resume time (see dw_mci_runtime_resume),
>     which seems like a terrible idea because we could possibly have
>     another interrupt pending.
>
> To fix the above isues we'd want to put something in the standard
> sdio_irq code that makes sure to call pm_runtime get/put when
> interrupts are being actively being processed.  That's possible to do,
> but it seems like a more complicated mechanism when we really just
> want the runtime pm disabled always for SDIO cards given that all the
> other bits needed to get Runtime PM vs. SDIO just aren't there.
>
> NOTE: at some point in time someone might come up with a fancy way to
> do SDIO interrupts and still allow (some) amount of runtime PM.
> Technically we could turn off the card clock if we used an alternate
> way of signaling SDIO interrupts (and out of band interrupt is one way
> to do this).  We probably wouldn't actually want to fully runtime
> suspend in this case though--at least not with the current
> dw_mci_runtime_resume() which basically fully resets the controller at
> resume time.
>
> Fixes: e9ed8835e990 ("mmc: dw_mmc: add runtime PM callback")
> Cc: <stable@vger.kernel.org>
> Reported-by: Brian Norris <briannorris@chromium.org>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Thanks, Doug, for this fix.

Reviewed-by: Shawn Lin <shawn.lin@rock-chips.com>


> ---
>  drivers/mmc/host/dw_mmc.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 249ded65192e..e45129f48174 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -23,6 +23,7 @@
>  #include <linux/ioport.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/seq_file.h>
>  #include <linux/slab.h>
>  #include <linux/stat.h>
> @@ -1620,10 +1621,16 @@ static void dw_mci_init_card(struct mmc_host *mmc, struct mmc_card *card)
>
>  		if (card->type == MMC_TYPE_SDIO ||
>  		    card->type == MMC_TYPE_SD_COMBO) {
> -			set_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
> +			if (!test_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags)) {
> +				pm_runtime_get_noresume(mmc->parent);
> +				set_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
> +			}
>  			clk_en_a = clk_en_a_old & ~clken_low_pwr;
>  		} else {
> -			clear_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
> +			if (test_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags)) {
> +				pm_runtime_put_noidle(mmc->parent);
> +				clear_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
> +			}
>  			clk_en_a = clk_en_a_old | clken_low_pwr;
>  		}
>
>

--
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
Ulf Hansson April 13, 2017, 9:32 a.m. UTC | #3
On 12 April 2017 at 00:55, Douglas Anderson <dianders@chromium.org> wrote:
> According to the SDIO standard interrupts are normally signalled in a
> very complicated way.  They require the card clock to be running and
> require the controller to be paying close attention to the signals
> coming from the card.  This simply can't happen with the clock stopped
> or with the controller in a low power mode.

Right - unless we have a possibility to re-route the SDIO irq line to
GPIO irq for wake-up when the controller enters low power state.

>
> To that end, we'll disable runtime_pm when we detect that an SDIO card
> was inserted.  This is much like with what we do with the special
> "SDMMC_CLKEN_LOW_PWR" bit that dw_mmc supports.

I wonder whether this is related to SDIO IRQs but not generic to SDIO.

For example, when we have a WIFI chip with a dedicated and separate
IRQ line, enabling SDMMC_CLKEN_LOW_PWR could make sense.

>
> NOTE: we specifically do this Runtime PM disabling at card init time
> rather than in the enable_sdio_irq() callback.  This is _different_
> than how SDHCI does it.  Why do we do it differently?
>
> - Unlike SDHCI, dw_mmc uses the standard sdio_irq code in Linux (AKA
>   dw_mmc doesn't set MMC_CAP2_SDIO_IRQ_NOTHREAD).
> - Because we use the standard sdio_irq code:
>   - We see a constant stream of enable_sdio_irq(0) and
>     enable_sdio_irq(1) calls.  This is because the standard code
>     disables interrupts while processing and re-enables them after.
>   - While interrupts are disabled, there's technically a period where
>     we could get runtime disabled while processing interrupts.
>   - If we are runtime disabled while processing interrupts, we'll
>     reset the controller at resume time (see dw_mci_runtime_resume),
>     which seems like a terrible idea because we could possibly have
>     another interrupt pending.

Thanks for the detailed clarification.

>
> To fix the above isues we'd want to put something in the standard
> sdio_irq code that makes sure to call pm_runtime get/put when
> interrupts are being actively being processed.  That's possible to do,
> but it seems like a more complicated mechanism when we really just
> want the runtime pm disabled always for SDIO cards given that all the
> other bits needed to get Runtime PM vs. SDIO just aren't there.
>
> NOTE: at some point in time someone might come up with a fancy way to
> do SDIO interrupts and still allow (some) amount of runtime PM.
> Technically we could turn off the card clock if we used an alternate
> way of signaling SDIO interrupts (and out of band interrupt is one way
> to do this).  We probably wouldn't actually want to fully runtime
> suspend in this case though--at least not with the current
> dw_mci_runtime_resume() which basically fully resets the controller at
> resume time.

I understand this "quick" fix in dw_mmc takes care of the problem.
However, allow me to try to make this a bit more generic. I intend to
post something within a few days. If I fail, let's go with this
solution.

Kind regards
Uffe

>
> Fixes: e9ed8835e990 ("mmc: dw_mmc: add runtime PM callback")
> Cc: <stable@vger.kernel.org>
> Reported-by: Brian Norris <briannorris@chromium.org>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>  drivers/mmc/host/dw_mmc.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 249ded65192e..e45129f48174 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -23,6 +23,7 @@
>  #include <linux/ioport.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/seq_file.h>
>  #include <linux/slab.h>
>  #include <linux/stat.h>
> @@ -1620,10 +1621,16 @@ static void dw_mci_init_card(struct mmc_host *mmc, struct mmc_card *card)
>
>                 if (card->type == MMC_TYPE_SDIO ||
>                     card->type == MMC_TYPE_SD_COMBO) {
> -                       set_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
> +                       if (!test_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags)) {
> +                               pm_runtime_get_noresume(mmc->parent);
> +                               set_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
> +                       }
>                         clk_en_a = clk_en_a_old & ~clken_low_pwr;
>                 } else {
> -                       clear_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
> +                       if (test_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags)) {
> +                               pm_runtime_put_noidle(mmc->parent);
> +                               clear_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
> +                       }
>                         clk_en_a = clk_en_a_old | clken_low_pwr;
>                 }
>
> --
> 2.12.2.715.g7642488e1d-goog
>
--
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
Doug Anderson April 13, 2017, 3:05 p.m. UTC | #4
Hi,

On Thu, Apr 13, 2017 at 2:32 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 12 April 2017 at 00:55, Douglas Anderson <dianders@chromium.org> wrote:
>> According to the SDIO standard interrupts are normally signalled in a
>> very complicated way.  They require the card clock to be running and
>> require the controller to be paying close attention to the signals
>> coming from the card.  This simply can't happen with the clock stopped
>> or with the controller in a low power mode.
>
> Right - unless we have a possibility to re-route the SDIO irq line to
> GPIO irq for wake-up when the controller enters low power state.

Yup.  Hence the "normally signalled".  ;)


>> To that end, we'll disable runtime_pm when we detect that an SDIO card
>> was inserted.  This is much like with what we do with the special
>> "SDMMC_CLKEN_LOW_PWR" bit that dw_mmc supports.
>
> I wonder whether this is related to SDIO IRQs but not generic to SDIO.
>
> For example, when we have a WIFI chip with a dedicated and separate
> IRQ line, enabling SDMMC_CLKEN_LOW_PWR could make sense.

Yes, I believe this to be the case too.  It has always been on my TODO
list to see if I could figure out how to make this work, but it never
rose to the top.

One thing I wasn't sure about: is SDIO truly stateless enough that you
could fully power down the controller while you were waiting for an
interrupt?  I've never dug down that far into the protocol.

For certain, though, you wouldn't want to remove VMMC from an SDIO
card while waiting for an interrupt.  This contrasts to an SD or eMMC
memory card where you could plausibly do that if you're not actively
reading from or writing to the card (though I think some cards may do
background garbage collection, so maybe?? you could interfere with
that?)


>> To fix the above isues we'd want to put something in the standard
>> sdio_irq code that makes sure to call pm_runtime get/put when
>> interrupts are being actively being processed.  That's possible to do,
>> but it seems like a more complicated mechanism when we really just
>> want the runtime pm disabled always for SDIO cards given that all the
>> other bits needed to get Runtime PM vs. SDIO just aren't there.
>>
>> NOTE: at some point in time someone might come up with a fancy way to
>> do SDIO interrupts and still allow (some) amount of runtime PM.
>> Technically we could turn off the card clock if we used an alternate
>> way of signaling SDIO interrupts (and out of band interrupt is one way
>> to do this).  We probably wouldn't actually want to fully runtime
>> suspend in this case though--at least not with the current
>> dw_mci_runtime_resume() which basically fully resets the controller at
>> resume time.
>
> I understand this "quick" fix in dw_mmc takes care of the problem.
> However, allow me to try to make this a bit more generic. I intend to
> post something within a few days. If I fail, let's go with this
> solution.

Most certainly!  If you can come up with something better then I'd be happy!

-Doug
--
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
Ulf Hansson April 18, 2017, 7:15 p.m. UTC | #5
On 12 April 2017 at 00:55, Douglas Anderson <dianders@chromium.org> wrote:
> According to the SDIO standard interrupts are normally signalled in a
> very complicated way.  They require the card clock to be running and
> require the controller to be paying close attention to the signals
> coming from the card.  This simply can't happen with the clock stopped
> or with the controller in a low power mode.
>
> To that end, we'll disable runtime_pm when we detect that an SDIO card
> was inserted.  This is much like with what we do with the special
> "SDMMC_CLKEN_LOW_PWR" bit that dw_mmc supports.
>
> NOTE: we specifically do this Runtime PM disabling at card init time
> rather than in the enable_sdio_irq() callback.  This is _different_
> than how SDHCI does it.  Why do we do it differently?
>
> - Unlike SDHCI, dw_mmc uses the standard sdio_irq code in Linux (AKA
>   dw_mmc doesn't set MMC_CAP2_SDIO_IRQ_NOTHREAD).
> - Because we use the standard sdio_irq code:
>   - We see a constant stream of enable_sdio_irq(0) and
>     enable_sdio_irq(1) calls.  This is because the standard code
>     disables interrupts while processing and re-enables them after.
>   - While interrupts are disabled, there's technically a period where
>     we could get runtime disabled while processing interrupts.
>   - If we are runtime disabled while processing interrupts, we'll
>     reset the controller at resume time (see dw_mci_runtime_resume),
>     which seems like a terrible idea because we could possibly have
>     another interrupt pending.
>
> To fix the above isues we'd want to put something in the standard
> sdio_irq code that makes sure to call pm_runtime get/put when
> interrupts are being actively being processed.  That's possible to do,
> but it seems like a more complicated mechanism when we really just
> want the runtime pm disabled always for SDIO cards given that all the
> other bits needed to get Runtime PM vs. SDIO just aren't there.
>
> NOTE: at some point in time someone might come up with a fancy way to
> do SDIO interrupts and still allow (some) amount of runtime PM.
> Technically we could turn off the card clock if we used an alternate
> way of signaling SDIO interrupts (and out of band interrupt is one way
> to do this).  We probably wouldn't actually want to fully runtime
> suspend in this case though--at least not with the current
> dw_mci_runtime_resume() which basically fully resets the controller at
> resume time.
>
> Fixes: e9ed8835e990 ("mmc: dw_mmc: add runtime PM callback")
> Cc: <stable@vger.kernel.org>
> Reported-by: Brian Norris <briannorris@chromium.org>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Thanks, applied for fixes!

I think this is the easiest way to fix the problem for stable and for 4.11.
However I think we should be able to revert this change for 4.12 or
perhaps for 4.13, once we find a more fine-grained solution. As you
probably noticed, I also gave that a try [1]. If you want to test it,
don't forget to revert $subject patch before testing.

Kind regards
Uffe

[1]
https://patchwork.kernel.org/patch/9685461/
https://patchwork.kernel.org/patch/9685463/
https://patchwork.kernel.org/patch/9685465/

> ---
>  drivers/mmc/host/dw_mmc.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 249ded65192e..e45129f48174 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -23,6 +23,7 @@
>  #include <linux/ioport.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/seq_file.h>
>  #include <linux/slab.h>
>  #include <linux/stat.h>
> @@ -1620,10 +1621,16 @@ static void dw_mci_init_card(struct mmc_host *mmc, struct mmc_card *card)
>
>                 if (card->type == MMC_TYPE_SDIO ||
>                     card->type == MMC_TYPE_SD_COMBO) {
> -                       set_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
> +                       if (!test_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags)) {
> +                               pm_runtime_get_noresume(mmc->parent);
> +                               set_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
> +                       }
>                         clk_en_a = clk_en_a_old & ~clken_low_pwr;
>                 } else {
> -                       clear_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
> +                       if (test_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags)) {
> +                               pm_runtime_put_noidle(mmc->parent);
> +                               clear_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
> +                       }
>                         clk_en_a = clk_en_a_old | clken_low_pwr;
>                 }
>
> --
> 2.12.2.715.g7642488e1d-goog
>
--
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
Brian Norris April 19, 2017, 5:55 p.m. UTC | #6
I'm a little late as this already landed (and I trusted Doug was
handling this just fine), but anyway:

On Tue, Apr 18, 2017 at 09:15:29PM +0200, Ulf Hansson wrote:
> Thanks, applied for fixes!

Yes, thanks :)

FWIW:

Tested-by: Brian Norris <briannorris@chromium.org>

Also, for some reason the eMMC crashes are now harder to reproduce.
Perhaps I was just (un)lucky the first few times I tested. Maybe it's an
existing issue and hasn't gotten much worse, except for possibly the
additional churn that runtime PM gives it.

But the SDIO/Wifi issues were definitely the primary thing that prompted
my original report, so I'm much happier now.

Brian
--
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 mbox

Patch

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 249ded65192e..e45129f48174 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -23,6 +23,7 @@ 
 #include <linux/ioport.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/seq_file.h>
 #include <linux/slab.h>
 #include <linux/stat.h>
@@ -1620,10 +1621,16 @@  static void dw_mci_init_card(struct mmc_host *mmc, struct mmc_card *card)
 
 		if (card->type == MMC_TYPE_SDIO ||
 		    card->type == MMC_TYPE_SD_COMBO) {
-			set_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
+			if (!test_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags)) {
+				pm_runtime_get_noresume(mmc->parent);
+				set_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
+			}
 			clk_en_a = clk_en_a_old & ~clken_low_pwr;
 		} else {
-			clear_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
+			if (test_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags)) {
+				pm_runtime_put_noidle(mmc->parent);
+				clear_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
+			}
 			clk_en_a = clk_en_a_old | clken_low_pwr;
 		}