Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: Linux: Kernel

[PATCH] mmc: dw_mmc: Disable low power mode if SDIO interrupts are used

 

 

Linux kernel RSS feed   Index | Next | Previous | View Threaded


dianders at chromium

Jul 20, 2012, 11:13 AM

Post #1 of 5 (129 views)
Permalink
[PATCH] mmc: dw_mmc: Disable low power mode if SDIO interrupts are used

The documentation for the dw_mmc part says that the low power
mode should normally only be set for MMC and SD memory and should
be turned off for SDIO cards that need interrupts detected.

The best place I could find to do this is when the SDIO interrupt
was first enabled. I rely on the fact that dw_mci_setup_bus()
will be called when it's time to reenable.

Change-Id: Id0e33a4e3a0a77ce8a5053b6e73197d53a5d46bb
Signed-off-by: Doug Anderson <dianders [at] chromium>
---
drivers/mmc/host/dw_mmc.c | 33 +++++++++++++++++++++++++++++++++
1 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 72dc3cd..0cb2756 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -862,6 +862,31 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
return present;
}

+/*
+ * Disable lower power mode.
+ *
+ * Low power mode will stop the card clock when idle. According to
+ * documentation we should disable low power mode for SDIO cards if we
+ * need interrupts to work.
+ *
+ * This function is fast if the power mode is already disabled.
+ */
+static void dw_mci_disable_low_power(struct mmc_host *mmc)
+{
+ struct dw_mci_slot *slot = mmc_priv(mmc);
+ struct dw_mci *host = slot->host;
+ u32 clk_en_a;
+ const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
+
+ clk_en_a = mci_readl(host, CLKENA);
+
+ if (clk_en_a & clken_low_pwr) {
+ mci_writel(host, CLKENA, clk_en_a & ~clken_low_pwr);
+ mci_send_cmd(slot, SDMMC_CMD_UPD_CLK |
+ SDMMC_CMD_PRV_DAT_WAIT, 0);
+ }
+}
+
static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb)
{
struct dw_mci_slot *slot = mmc_priv(mmc);
@@ -871,6 +896,14 @@ static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb)
/* Enable/disable Slot Specific SDIO interrupt */
int_mask = mci_readl(host, INTMASK);
if (enb) {
+ /*
+ * Turn off low power mode if it was enabled. This is a bit of
+ * a heavy operation and we disable / enable IRQs a lot, so
+ * we'll leave them disabled; they will get re-enabled again in
+ * dw_mci_setup_bus().
+ */
+ dw_mci_disable_low_power(mmc);
+
mci_writel(host, INTMASK,
(int_mask | SDMMC_INT_SDIO(slot->id)));
} else {
--
1.7.7.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


will.newton at gmail

Jul 21, 2012, 3:40 AM

Post #2 of 5 (125 views)
Permalink
Re: [PATCH] mmc: dw_mmc: Disable low power mode if SDIO interrupts are used [In reply to]

On Fri, Jul 20, 2012 at 7:13 PM, Doug Anderson <dianders [at] chromium> wrote:
> The documentation for the dw_mmc part says that the low power
> mode should normally only be set for MMC and SD memory and should
> be turned off for SDIO cards that need interrupts detected.
>
> The best place I could find to do this is when the SDIO interrupt
> was first enabled. I rely on the fact that dw_mci_setup_bus()
> will be called when it's time to reenable.
>
> Change-Id: Id0e33a4e3a0a77ce8a5053b6e73197d53a5d46bb
> Signed-off-by: Doug Anderson <dianders [at] chromium>
> ---
> drivers/mmc/host/dw_mmc.c | 33 +++++++++++++++++++++++++++++++++
> 1 files changed, 33 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 72dc3cd..0cb2756 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -862,6 +862,31 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
> return present;
> }
>
> +/*
> + * Disable lower power mode.
> + *
> + * Low power mode will stop the card clock when idle. According to
> + * documentation we should disable low power mode for SDIO cards if we
> + * need interrupts to work.
> + *
> + * This function is fast if the power mode is already disabled.
> + */
> +static void dw_mci_disable_low_power(struct mmc_host *mmc)
> +{
> + struct dw_mci_slot *slot = mmc_priv(mmc);
> + struct dw_mci *host = slot->host;
> + u32 clk_en_a;
> + const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
> +
> + clk_en_a = mci_readl(host, CLKENA);
> +
> + if (clk_en_a & clken_low_pwr) {
> + mci_writel(host, CLKENA, clk_en_a & ~clken_low_pwr);
> + mci_send_cmd(slot, SDMMC_CMD_UPD_CLK |
> + SDMMC_CMD_PRV_DAT_WAIT, 0);
> + }
> +}
> +
> static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb)
> {
> struct dw_mci_slot *slot = mmc_priv(mmc);
> @@ -871,6 +896,14 @@ static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb)
> /* Enable/disable Slot Specific SDIO interrupt */
> int_mask = mci_readl(host, INTMASK);
> if (enb) {
> + /*
> + * Turn off low power mode if it was enabled. This is a bit of
> + * a heavy operation and we disable / enable IRQs a lot, so
> + * we'll leave them disabled; they will get re-enabled again in
> + * dw_mci_setup_bus().
> + */
> + dw_mci_disable_low_power(mmc);
> +
> mci_writel(host, INTMASK,
> (int_mask | SDMMC_INT_SDIO(slot->id)));
> } else {
> --
> 1.7.7.3

Is it safe to just disable low power here or could the setting be
overwritten in setup_bus?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


dianders at chromium

Jul 22, 2012, 7:48 PM

Post #3 of 5 (125 views)
Permalink
Re: [PATCH] mmc: dw_mmc: Disable low power mode if SDIO interrupts are used [In reply to]

On Sat, Jul 21, 2012 at 3:40 AM, Will Newton <will.newton [at] gmail> wrote:
>> static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb)
>> {
>> struct dw_mci_slot *slot = mmc_priv(mmc);
>> @@ -871,6 +896,14 @@ static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb)
>> /* Enable/disable Slot Specific SDIO interrupt */
>> int_mask = mci_readl(host, INTMASK);
>> if (enb) {
>> + /*
>> + * Turn off low power mode if it was enabled. This is a bit of
>> + * a heavy operation and we disable / enable IRQs a lot, so
>> + * we'll leave them disabled; they will get re-enabled again in
>> + * dw_mci_setup_bus().
>> + */
>> + dw_mci_disable_low_power(mmc);
>> +
>
> Is it safe to just disable low power here or could the setting be
> overwritten in setup_bus?

Very good question. In my current setup I don't see setup_bus()
called during normal operation. If it were, my kernel messages would
be constantly spammed with messages like:
Bus speed (slot %d) = %dHz (slot req %dHz, actual %dHZ div = %d)

...and they're not. Things may be different with different SDIO cards perhaps?

In any case, it's pretty easy for me to spin the patch so that we
don't clobber the low power bit in setup_bus() if SDIO interrupts are
enabled. That makes a lot of sense, though I'd need to make sure that
low power mode does eventually get set again if someone ejects the
SDIO card and puts in a non-SDIO card.

I'll spin the patch tomorrow when I can test it properly and also
address some commenting concerns another engineer at chromium.org had.

It still feels to me like there ought to be a better place to put this
code. I'd rather disable low power mode as soon as we detect an SDIO
card. I spent time searching and the best I could find was
dw_mci_enable_sdio_irq(), but I'm all ears if someone has a better
idea! :) Certainly this code needs to go somewhere if we want SDIO
interrupts to be reliable.

Thanks for your feeback! :)

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


will.newton at gmail

Jul 23, 2012, 2:19 AM

Post #4 of 5 (125 views)
Permalink
Re: [PATCH] mmc: dw_mmc: Disable low power mode if SDIO interrupts are used [In reply to]

On Mon, Jul 23, 2012 at 3:48 AM, Doug Anderson <dianders [at] chromium> wrote:
> On Sat, Jul 21, 2012 at 3:40 AM, Will Newton <will.newton [at] gmail> wrote:
>>> static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb)
>>> {
>>> struct dw_mci_slot *slot = mmc_priv(mmc);
>>> @@ -871,6 +896,14 @@ static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb)
>>> /* Enable/disable Slot Specific SDIO interrupt */
>>> int_mask = mci_readl(host, INTMASK);
>>> if (enb) {
>>> + /*
>>> + * Turn off low power mode if it was enabled. This is a bit of
>>> + * a heavy operation and we disable / enable IRQs a lot, so
>>> + * we'll leave them disabled; they will get re-enabled again in
>>> + * dw_mci_setup_bus().
>>> + */
>>> + dw_mci_disable_low_power(mmc);
>>> +
>>
>> Is it safe to just disable low power here or could the setting be
>> overwritten in setup_bus?
>
> Very good question. In my current setup I don't see setup_bus()
> called during normal operation. If it were, my kernel messages would
> be constantly spammed with messages like:
> Bus speed (slot %d) = %dHz (slot req %dHz, actual %dHZ div = %d)
>
> ...and they're not. Things may be different with different SDIO cards perhaps?

Yeah I think setup_bus should only setup the card clock once at
startup but it may also be required on resume?

I should probably mention I have not tested this driver with any SDIO
devices, although I believe there are other people out there who do!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


dianders at chromium

Jul 23, 2012, 10:00 AM

Post #5 of 5 (124 views)
Permalink
Re: [PATCH] mmc: dw_mmc: Disable low power mode if SDIO interrupts are used [In reply to]

On Mon, Jul 23, 2012 at 2:19 AM, Will Newton <will.newton [at] gmail> wrote:
>> Very good question. In my current setup I don't see setup_bus()
>> called during normal operation. If it were, my kernel messages would
>> be constantly spammed with messages like:
>> Bus speed (slot %d) = %dHz (slot req %dHz, actual %dHZ div = %d)
>>
>> ...and they're not. Things may be different with different SDIO cards perhaps?
>
> Yeah I think setup_bus should only setup the card clock once at
> startup but it may also be required on resume?

We just got suspend/resume working yesterday, so I can now test this! :)

With our current driver (which had some modifications to allow for
MMC_PM_KEEP_POWER that I assume will be posted before too long), I did
some testing with printk. On my system I found that
dw_mci_setup_bus() is always called with SDIO interrupts turned off,
even during the resume path. That means my previous posted patch is
OK.

I also looked more closely at the resume path. I see this in the
current upstream code in the resume function:

mci_writel(host, INTMASK, SDMMC_INT_CMD_DONE | SDMMC_INT_DATA_OVER |
SDMMC_INT_TXDR | SDMMC_INT_RXDR |
DW_MCI_ERROR_FLAGS | SDMMC_INT_CD);

This will clobber SDIO interrupts. That means that if we have any
hope of SDIO interrupts working, someone will need to call
dw_mci_enable_sdio_irq() which will re-disable low power mode. This
also points to my previous patch being OK.


...but putting the extra check in setup_bus() still doesn't hurt,
though, so I'll post that shortly. I have looked into the SDIO code
and see that when the sdio_irq_thread exits it always disables SDIO
interrupts. That means that I can still rely on setup_bus to properly
re-enable low power mode when it's called after an SDIO module is
removed. :)


> I should probably mention I have not tested this driver with any SDIO
> devices, although I believe there are other people out there who do!

Agreed. Given that I've seen recent patches (authored May 14th 2012,
for instance) fixing major SDIO issues with this driver, I'd conclude:

* Use of this driver for SDIO is very new and there may still be bugs.

* If others are using SDIO interrupts and haven't seen this issue,
they've got something different about their system. Perhaps the SDIO
module they're using behaves in a way that SDIO interrupts always come
in at the same time as some other source? ...or maybe they do some
type of periodic polling and are thus OK with missing some interrupts?
The exynos manual that includes the dw mmc controller is very clear
that you can't use low power mode and SDIO interrupts and it was
definitely failing for us.

I've added the author of the most recent SDIO patch to this email
thread. Sorry for missing that before. Kyoungil: do you have any
comments on this?


-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Linux kernel RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact Gossamer Threads
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.