| Message ID | 1381876762-10892-2-git-send-email-dianders@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Doug, On 10/16/2013 07:39 AM, Doug Anderson wrote: > In the patch (9623b5b mmc: dw_mmc: Disable low power mode if SDIO > interrupts are used) I added code that disabled the low power mode of > dw_mmc when SDIO interrupts are used. That code worked but always > felt a little hacky because we ended up disabling low power as a side > effect of the first enable_sdio_irq() call. That wouldn't be so bad > except that disabling low power involves a complicated process of > writing to the CMD/CMDARG registers and that extra process makes it > difficult to cleanly the read-modify-write race in > dw_mci_enable_sdio_irq() (see future patch in the series). > > Change the code to take advantage of the init_card() callback of the > mmc core to know when an SDIO card has been inserted. If we've got a > SDIO card and we're configured to use SDIO Interrupts then turn off > "low power mode" right away. > > Signed-off-by: Doug Anderson <dianders@chromium.org> > --- > drivers/mmc/host/dw_mmc.c | 68 ++++++++++++++++++++++++----------------------- > drivers/mmc/host/dw_mmc.h | 1 + > 2 files changed, 36 insertions(+), 33 deletions(-) > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > index 0a6a512..1b75816 100644 > --- a/drivers/mmc/host/dw_mmc.c > +++ b/drivers/mmc/host/dw_mmc.c > @@ -27,6 +27,7 @@ > #include <linux/stat.h> > #include <linux/delay.h> > #include <linux/irq.h> > +#include <linux/mmc/card.h> > #include <linux/mmc/host.h> > #include <linux/mmc/mmc.h> > #include <linux/mmc/sdio.h> > @@ -822,7 +823,7 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit) > > /* enable clock; only low power if no SDIO */ > clk_en_a = SDMMC_CLKEN_ENABLE << slot->id; > - if (!(mci_readl(host, INTMASK) & SDMMC_INT_SDIO(slot->id))) > + if (!test_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags)) > clk_en_a |= SDMMC_CLKEN_LOW_PWR << slot->id; > mci_writel(host, CLKENA, clk_en_a); > > @@ -1050,27 +1051,37 @@ 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 the > - * description of the CLKENA register we should disable low power mode > - * for SDIO cards if we need SDIO interrupts to work. > - * > - * This function is fast if low power mode is already disabled. > - */ > -static void dw_mci_disable_low_power(struct dw_mci_slot *slot) > +static void dw_mci_init_card(struct mmc_host *mmc, struct mmc_card *card) > { > + 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); > + /* > + * Low power mode will stop the card clock when idle. According to the > + * description of the CLKENA register we should disable low power mode > + * for SDIO cards if we need SDIO interrupts to work. > + */ > + if (mmc->caps | MMC_CAP_SDIO_IRQ) { mmc->caps & MMC_CAP_SDIO_IRQ? > + const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id; > + u32 clk_en_a_old; > + u32 clk_en_a; > > - 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); > + clk_en_a_old = mci_readl(host, CLKENA); > + > + if (card->type == MMC_TYPE_SDIO || > + card->type == MMC_TYPE_SD_COMBO) { > + 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); > + clk_en_a = clk_en_a_old | clken_low_pwr; When this condition is entered? card->type is always MMC_TYPE_SDIO or MMC_TYPE_SD_COMBO, isn't? Best Regards, Jaehoon Chung > + } > + > + if (clk_en_a != clk_en_a_old) { > + mci_writel(host, CLKENA, clk_en_a); > + mci_send_cmd(slot, SDMMC_CMD_UPD_CLK | > + SDMMC_CMD_PRV_DAT_WAIT, 0); > + } > } > } > > @@ -1082,21 +1093,11 @@ 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 low power mode disabled and it will get > - * re-enabled again in dw_mci_setup_bus(). > - */ > - dw_mci_disable_low_power(slot); > - > - mci_writel(host, INTMASK, > - (int_mask | SDMMC_INT_SDIO(slot->id))); > - } else { > - mci_writel(host, INTMASK, > - (int_mask & ~SDMMC_INT_SDIO(slot->id))); > - } > + if (enb) > + int_mask |= SDMMC_INT_SDIO(slot->id); > + else > + int_mask &= ~SDMMC_INT_SDIO(slot->id); > + mci_writel(host, INTMASK, int_mask); > } > > static int dw_mci_execute_tuning(struct mmc_host *mmc, u32 opcode) > @@ -1140,6 +1141,7 @@ static const struct mmc_host_ops dw_mci_ops = { > .get_cd = dw_mci_get_cd, > .enable_sdio_irq = dw_mci_enable_sdio_irq, > .execute_tuning = dw_mci_execute_tuning, > + .init_card = dw_mci_init_card, > }; > > static void dw_mci_request_end(struct dw_mci *host, struct mmc_request *mrq) > diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h > index 6bf24ab..26d4657 100644 > --- a/drivers/mmc/host/dw_mmc.h > +++ b/drivers/mmc/host/dw_mmc.h > @@ -227,6 +227,7 @@ struct dw_mci_slot { > unsigned long flags; > #define DW_MMC_CARD_PRESENT 0 > #define DW_MMC_CARD_NEED_INIT 1 > +#define DW_MMC_CARD_NO_LOW_PWR 2 > int id; > int last_detect_state; > }; > -- 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
Jaehoon, On Fri, Oct 18, 2013 at 2:42 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote: >> - clk_en_a = mci_readl(host, CLKENA); >> + /* >> + * Low power mode will stop the card clock when idle. According to the >> + * description of the CLKENA register we should disable low power mode >> + * for SDIO cards if we need SDIO interrupts to work. >> + */ >> + if (mmc->caps | MMC_CAP_SDIO_IRQ) { > mmc->caps & MMC_CAP_SDIO_IRQ? Wow, that was an embarrassing one. Thanks. >> + const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id; >> + u32 clk_en_a_old; >> + u32 clk_en_a; >> >> - 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); >> + clk_en_a_old = mci_readl(host, CLKENA); >> + >> + if (card->type == MMC_TYPE_SDIO || >> + card->type == MMC_TYPE_SD_COMBO) { >> + 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); >> + clk_en_a = clk_en_a_old | clken_low_pwr; > When this condition is entered? card->type is always MMC_TYPE_SDIO or MMC_TYPE_SD_COMBO, isn't? Ugh, that's not intuitive. This callback is only called for SDIO cards and not MMC/SD cards? That means if you plug in an SDIO card and then eject it and plug in an SD card you won't get to low power. Hrm. I dug around a bit and couldn't find a better way to do this and then I realized that the other user of the init_card() callback has the same bug, so for the next version of the series I'm proposing a fix for mmc core to add this for all types. If you have a better suggestion, I'm all ears. -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
Hi Doug, This change looks good. There's comment below. On Sat, October 19, 2013, Doug Anderson wrote: > Jaehoon, > > On Fri, Oct 18, 2013 at 2:42 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote: > >> - clk_en_a = mci_readl(host, CLKENA); > >> + /* > >> + * Low power mode will stop the card clock when idle. According to the > >> + * description of the CLKENA register we should disable low power mode > >> + * for SDIO cards if we need SDIO interrupts to work. > >> + */ > >> + if (mmc->caps | MMC_CAP_SDIO_IRQ) { > > mmc->caps & MMC_CAP_SDIO_IRQ? > > Wow, that was an embarrassing one. Thanks. > > >> + const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id; > >> + u32 clk_en_a_old; > >> + u32 clk_en_a; > >> > >> - 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); > >> + clk_en_a_old = mci_readl(host, CLKENA); > >> + > >> + if (card->type == MMC_TYPE_SDIO || > >> + card->type == MMC_TYPE_SD_COMBO) { && card->quirks & MMC_QUIRK_BROKEN_CLK_GATING How about considering MMC_QUIRK_BROKEN_CLK_GATING? Some sdio device can work with gating clock. For this, mmc_fixup_device() should be called prior to init_card() in core(sdio.c). I guess you found that. Thanks, Seungwon Jeon > >> + 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); > >> + clk_en_a = clk_en_a_old | clken_low_pwr; > > When this condition is entered? card->type is always MMC_TYPE_SDIO or MMC_TYPE_SD_COMBO, isn't? > > Ugh, that's not intuitive. This callback is only called for SDIO > cards and not MMC/SD cards? That means if you plug in an SDIO card > and then eject it and plug in an SD card you won't get to low power. > Hrm. > > I dug around a bit and couldn't find a better way to do this and then > I realized that the other user of the init_card() callback has the > same bug, so for the next version of the series I'm proposing a fix > for mmc core to add this for all types. If you have a better > suggestion, I'm all ears. > > -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 -- 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
Seungwon, On Wed, Oct 23, 2013 at 12:25 PM, Seungwon Jeon <tgih.jun@samsung.com> wrote: >> >> + if (card->type == MMC_TYPE_SDIO || >> >> + card->type == MMC_TYPE_SD_COMBO) { > && card->quirks & MMC_QUIRK_BROKEN_CLK_GATING > How about considering MMC_QUIRK_BROKEN_CLK_GATING? > Some sdio device can work with gating clock. > For this, mmc_fixup_device() should be called prior to init_card() in core(sdio.c). > I guess you found that. By SDIO devices, are you referring to actual SDIO cards or some implementations of dw_mmc? As far as I understand in the CLKENA description in the generic documentation from Synopsys it say that for SDIO cards you must not stop the clock if interrupts must be detected. To me, that means that all dw_mmc implementations require this change if they support SDIO interrupts (hence checking for MMC_CAP_SDIO_IRQ). I guess I did make the assumption in this change that all (reasonable) SDIO cards would be using SDIO interrupts if they are available. If we could find out ahead of time if a given SDIO driver was planning to use interrupts we could do better. The old solution did better in this way and we could probably make it work (and still fix the read-modify-write race) if we thought this was really important. The code gets a little more twisted to try to avoid holding the IRQ-safe spinlock while updating the clock, but I could attempt it... NOTE: We've recently found that there are still occasions when we lose SDIO interrupts with dw_mmc and our Marvell WiFi driver, especially when those interrupts happen back-to-back. That problem appears unrelated to this one (it's what Bing was investigating when he found the original race). Anyone that has any thoughts, please let me know... -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
On Thu, October 24, 2013, Doug Anderson wrote: > Seungwon, > > On Wed, Oct 23, 2013 at 12:25 PM, Seungwon Jeon <tgih.jun@samsung.com> wrote: > >> >> + if (card->type == MMC_TYPE_SDIO || > >> >> + card->type == MMC_TYPE_SD_COMBO) { > > && card->quirks & MMC_QUIRK_BROKEN_CLK_GATING > > How about considering MMC_QUIRK_BROKEN_CLK_GATING? > > Some sdio device can work with gating clock. > > For this, mmc_fixup_device() should be called prior to init_card() in core(sdio.c). > > I guess you found that. > > By SDIO devices, are you referring to actual SDIO cards or some > implementations of dw_mmc? > > As far as I understand in the CLKENA description in the generic > documentation from Synopsys it say that for SDIO cards you must not > stop the clock if interrupts must be detected. To me, that means that > all dw_mmc implementations require this change if they support SDIO > interrupts (hence checking for MMC_CAP_SDIO_IRQ). CLKENA description in manual means that host controller can't detect the SDIO interrupt signal or wifi device can't generate the interrupt without clock? Host controller based on Synopsys supports asynchronous interrupts. It seems to depend on wifi Device. If host can do and wifi device can also work with clock gating, we may enable low-power mode. For MMC_QUIRK_BROKEN_CLK_GATING, I referred the code in 'mmc/core/quirks.c' In addition, although host can support MMC_CAP_SDIO_IRQ, some wifi drivers use OOB(Out-of-band) interrupt. That means host can apply clock gating to reduce power consumption. Thanks, Seungwon Jeon > > I guess I did make the assumption in this change that all (reasonable) > SDIO cards would be using SDIO interrupts if they are available. If > we could find out ahead of time if a given SDIO driver was planning to > use interrupts we could do better. The old solution did better in > this way and we could probably make it work (and still fix the > read-modify-write race) if we thought this was really important. The > code gets a little more twisted to try to avoid holding the IRQ-safe > spinlock while updating the clock, but I could attempt it... > > > NOTE: We've recently found that there are still occasions when we lose > SDIO interrupts with dw_mmc and our Marvell WiFi driver, especially > when those interrupts happen back-to-back. That problem appears > unrelated to this one (it's what Bing was investigating when he found > the original race). Anyone that has any thoughts, please let me > know... > > -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 -- 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
Seungwon, On Fri, Oct 25, 2013 at 2:29 AM, Seungwon Jeon <tgih.jun@samsung.com> wrote: >> By SDIO devices, are you referring to actual SDIO cards or some >> implementations of dw_mmc? >> >> As far as I understand in the CLKENA description in the generic >> documentation from Synopsys it say that for SDIO cards you must not >> stop the clock if interrupts must be detected. To me, that means that >> all dw_mmc implementations require this change if they support SDIO >> interrupts (hence checking for MMC_CAP_SDIO_IRQ). > > CLKENA description in manual means that host controller can't detect the SDIO interrupt signal > or wifi device can't generate the interrupt without clock? My reading of the "if interrupts must be detected" in the manual implies that that interrupts simply can't be detected by the controller. > Host controller based on Synopsys supports asynchronous interrupts. It seems to depend on wifi Device. > If host can do and wifi device can also work with clock gating, we may enable low-power mode. Ah, interesting! I wish I had known about this earlier and we could have actually used it in our designs. Please correct me if I'm wrong but... It looks like asynchronous interrupts is when you use a separate INT# line for your SDIO interrupts and is available only for eSDIO (embedded SDIO?), right. ...so that means it more a property of the board rather than the card itself. In other words: to use asynchronous interrupts you need to be on a SoC that supports the INT# line, you need to have it wired up to an eSDIO module, and you need the eSDIO card to support it. Assuming that I understand all of the above I'd suggest (in a future patch) that we add a property like 'dedicated-sdio-irq' to device trees. If we see this property AND we don't see MMC_QUIRK_BROKEN_CLK_GATING then we know we don't need to disable clock gating. Does that sound right? If so I'd still love to land my patch and we could add the extra logic in a separate patch. > For MMC_QUIRK_BROKEN_CLK_GATING, I referred the code in 'mmc/core/quirks.c' > In addition, although host can support MMC_CAP_SDIO_IRQ, some wifi drivers use > OOB(Out-of-band) interrupt. That means host can apply clock gating to reduce > power consumption. I think OOB interrupt is the same as asynchronous interrupt, but if I'm wrong please correct me. -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
On Tue, October 29, 2013, Doug Anderson wrote > Seungwon, > > On Fri, Oct 25, 2013 at 2:29 AM, Seungwon Jeon <tgih.jun@samsung.com> wrote: > >> By SDIO devices, are you referring to actual SDIO cards or some > >> implementations of dw_mmc? > >> > >> As far as I understand in the CLKENA description in the generic > >> documentation from Synopsys it say that for SDIO cards you must not > >> stop the clock if interrupts must be detected. To me, that means that > >> all dw_mmc implementations require this change if they support SDIO > >> interrupts (hence checking for MMC_CAP_SDIO_IRQ). > > > > CLKENA description in manual means that host controller can't detect the SDIO interrupt signal > > or wifi device can't generate the interrupt without clock? > > My reading of the "if interrupts must be detected" in the manual > implies that that interrupts simply can't be detected by the > controller. I just wanted to know your opinion because I was not convinced that. But, as far as I know, interrupt is generated by device with clock sync. If clock is stopped, device may not issue interrupt. This is the reason that interrupt is not detected. Ok. Anyway, clock should be required for synchronous interrupt. > > > > Host controller based on Synopsys supports asynchronous interrupts. It seems to depend on wifi > Device. > > If host can do and wifi device can also work with clock gating, we may enable low-power mode. > > Ah, interesting! I wish I had known about this earlier and we could > have actually used it in our designs. Please correct me if I'm wrong > but... > > It looks like asynchronous interrupts is when you use a separate INT# > line for your SDIO interrupts and is available only for eSDIO > (embedded SDIO?), right. ...so that means it more a property of the > board rather than the card itself. In other words: to use > asynchronous interrupts you need to be on a SoC that supports the INT# > line, you need to have it wired up to an eSDIO module, and you need > the eSDIO card to support it. Right. But we cannot say without the device which supports INT#. For asynchronous interrupt, device should be mounted on target board. > > Assuming that I understand all of the above I'd suggest (in a future > patch) that we add a property like 'dedicated-sdio-irq' to device > trees. If we see this property AND we don't see > MMC_QUIRK_BROKEN_CLK_GATING then we know we don't need to disable > clock gating. > > Does that sound right? If so I'd still love to land my patch and we > could add the extra logic in a separate patch. Ok. I like it. Will you send it with this series? If not, existing MMC_QUIRK_BROKEN_CLK_GATING could be considered at this time. And, could you modify your comment message more definitely? > > > > For MMC_QUIRK_BROKEN_CLK_GATING, I referred the code in 'mmc/core/quirks.c' > > In addition, although host can support MMC_CAP_SDIO_IRQ, some wifi drivers use > > OOB(Out-of-band) interrupt. That means host can apply clock gating to reduce > > power consumption. > > I think OOB interrupt is the same as asynchronous interrupt, but if > I'm wrong please correct me. Yes. Eventually both mechanisms are asynchronous. Additionally, OOB I mentioned comes from some wlan driver commit & implementation. I guess it doesn't indicate OOB of SDIO specification 4.0 and it's not same. Thanks, Seungwon Jeon -- 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/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c index 0a6a512..1b75816 100644 --- a/drivers/mmc/host/dw_mmc.c +++ b/drivers/mmc/host/dw_mmc.c @@ -27,6 +27,7 @@ #include <linux/stat.h> #include <linux/delay.h> #include <linux/irq.h> +#include <linux/mmc/card.h> #include <linux/mmc/host.h> #include <linux/mmc/mmc.h> #include <linux/mmc/sdio.h> @@ -822,7 +823,7 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit) /* enable clock; only low power if no SDIO */ clk_en_a = SDMMC_CLKEN_ENABLE << slot->id; - if (!(mci_readl(host, INTMASK) & SDMMC_INT_SDIO(slot->id))) + if (!test_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags)) clk_en_a |= SDMMC_CLKEN_LOW_PWR << slot->id; mci_writel(host, CLKENA, clk_en_a); @@ -1050,27 +1051,37 @@ 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 the - * description of the CLKENA register we should disable low power mode - * for SDIO cards if we need SDIO interrupts to work. - * - * This function is fast if low power mode is already disabled. - */ -static void dw_mci_disable_low_power(struct dw_mci_slot *slot) +static void dw_mci_init_card(struct mmc_host *mmc, struct mmc_card *card) { + 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); + /* + * Low power mode will stop the card clock when idle. According to the + * description of the CLKENA register we should disable low power mode + * for SDIO cards if we need SDIO interrupts to work. + */ + if (mmc->caps | MMC_CAP_SDIO_IRQ) { + const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id; + u32 clk_en_a_old; + u32 clk_en_a; - 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); + clk_en_a_old = mci_readl(host, CLKENA); + + if (card->type == MMC_TYPE_SDIO || + card->type == MMC_TYPE_SD_COMBO) { + 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); + clk_en_a = clk_en_a_old | clken_low_pwr; + } + + if (clk_en_a != clk_en_a_old) { + mci_writel(host, CLKENA, clk_en_a); + mci_send_cmd(slot, SDMMC_CMD_UPD_CLK | + SDMMC_CMD_PRV_DAT_WAIT, 0); + } } } @@ -1082,21 +1093,11 @@ 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 low power mode disabled and it will get - * re-enabled again in dw_mci_setup_bus(). - */ - dw_mci_disable_low_power(slot); - - mci_writel(host, INTMASK, - (int_mask | SDMMC_INT_SDIO(slot->id))); - } else { - mci_writel(host, INTMASK, - (int_mask & ~SDMMC_INT_SDIO(slot->id))); - } + if (enb) + int_mask |= SDMMC_INT_SDIO(slot->id); + else + int_mask &= ~SDMMC_INT_SDIO(slot->id); + mci_writel(host, INTMASK, int_mask); } static int dw_mci_execute_tuning(struct mmc_host *mmc, u32 opcode) @@ -1140,6 +1141,7 @@ static const struct mmc_host_ops dw_mci_ops = { .get_cd = dw_mci_get_cd, .enable_sdio_irq = dw_mci_enable_sdio_irq, .execute_tuning = dw_mci_execute_tuning, + .init_card = dw_mci_init_card, }; static void dw_mci_request_end(struct dw_mci *host, struct mmc_request *mrq) diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h index 6bf24ab..26d4657 100644 --- a/drivers/mmc/host/dw_mmc.h +++ b/drivers/mmc/host/dw_mmc.h @@ -227,6 +227,7 @@ struct dw_mci_slot { unsigned long flags; #define DW_MMC_CARD_PRESENT 0 #define DW_MMC_CARD_NEED_INIT 1 +#define DW_MMC_CARD_NO_LOW_PWR 2 int id; int last_detect_state; };
In the patch (9623b5b mmc: dw_mmc: Disable low power mode if SDIO interrupts are used) I added code that disabled the low power mode of dw_mmc when SDIO interrupts are used. That code worked but always felt a little hacky because we ended up disabling low power as a side effect of the first enable_sdio_irq() call. That wouldn't be so bad except that disabling low power involves a complicated process of writing to the CMD/CMDARG registers and that extra process makes it difficult to cleanly the read-modify-write race in dw_mci_enable_sdio_irq() (see future patch in the series). Change the code to take advantage of the init_card() callback of the mmc core to know when an SDIO card has been inserted. If we've got a SDIO card and we're configured to use SDIO Interrupts then turn off "low power mode" right away. Signed-off-by: Doug Anderson <dianders@chromium.org> --- drivers/mmc/host/dw_mmc.c | 68 ++++++++++++++++++++++++----------------------- drivers/mmc/host/dw_mmc.h | 1 + 2 files changed, 36 insertions(+), 33 deletions(-)