| Message ID | 1417563767-32181-4-git-send-email-dianders@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, Doug. On 12/03/2014 08:42 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 do this right at bootup. > > Signed-off-by: Doug Anderson <dianders@chromium.org> > --- > Changes in v5: None > Changes in v3: None > Changes in v2: > - Fixed "|" to "&". > > drivers/mmc/host/dw_mmc.c | 69 ++++++++++++++++++++++++----------------------- > drivers/mmc/host/dw_mmc.h | 1 + > 2 files changed, 36 insertions(+), 34 deletions(-) > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > index 67c0451..ae10a02 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/sd.h> > @@ -926,7 +927,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->sdio_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); > > @@ -1245,27 +1246,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 { I wonder this point. When entered at this point? MMC_CAP_SDIO_IRQ is sdio capability. and card->type is also related with SDIO, isn't? Best Regards, Jaehoon Chung > + 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); > + } > } > } > > @@ -1277,21 +1288,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->sdio_id))); > - } else { > - mci_writel(host, INTMASK, > - (int_mask & ~SDMMC_INT_SDIO(slot->sdio_id))); > - } > + if (enb) > + int_mask |= SDMMC_INT_SDIO(slot->sdio_id); > + else > + int_mask &= ~SDMMC_INT_SDIO(slot->sdio_id); > + mci_writel(host, INTMASK, int_mask); > } > > static int dw_mci_execute_tuning(struct mmc_host *mmc, u32 opcode) > @@ -1337,7 +1338,7 @@ static const struct mmc_host_ops dw_mci_ops = { > .execute_tuning = dw_mci_execute_tuning, > .card_busy = dw_mci_card_busy, > .start_signal_voltage_switch = dw_mci_switch_voltage, > - > + .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 0d0f7a2..d27d4c0 100644 > --- a/drivers/mmc/host/dw_mmc.h > +++ b/drivers/mmc/host/dw_mmc.h > @@ -244,6 +244,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 sdio_id; > }; > -- 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 Tue, Dec 2, 2014 at 4:17 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote: >> @@ -1245,27 +1246,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 { > > I wonder this point. When entered at this point? > > MMC_CAP_SDIO_IRQ is sdio capability. and card->type is also related with SDIO, isn't? Right. As I understand it: * You can certainly add MMC_CAP_SDIO_IRQ to an external card slot that might have an MMC card, and SD card, or an SDIO card. You still want the LOW_PWR mode for MMC/SD cards but don't want it for SDIO cards in that case. * If you don't set MMC_CAP_SDIO_IRQ then presumably you've got some problem where the SDIO interrupt is broken on your board. That means you're using polling mode to find interrupts. As I understand it that would allow you to use LOW_PWR since you don't need to detect SDIO interrupts. I know on Marvell WiFi modules I tested this seemed to work OK, but I doubt anyone is really running a production device in this way. Basically I was trying to mirror the functionality of the old code as closely as possible. In the old code if you were using polling mode then dw_mci_enable_sdio_irq() should never be called and you'd never go out of low power mode. -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 On 12/03/2014 09:36 AM, Doug Anderson wrote: > Jaehoon, > > On Tue, Dec 2, 2014 at 4:17 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote: >>> @@ -1245,27 +1246,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 { >> >> I wonder this point. When entered at this point? >> >> MMC_CAP_SDIO_IRQ is sdio capability. and card->type is also related with SDIO, isn't? > > Right. As I understand it: > > * You can certainly add MMC_CAP_SDIO_IRQ to an external card slot that > might have an MMC card, and SD card, or an SDIO card. You still want > the LOW_PWR mode for MMC/SD cards but don't want it for SDIO cards in > that case. Ok, I understood your purpose. Your mean is MMC_CAP_SIDO_IRQ can be set to MMC or SD...not only SDIO card. It's reasonable. But i think it doesn't need to set LOW_PWR_mode for SD/eMMC at here. it should be already set to Low-power mode. And SDIO case should not enter at here. > > * If you don't set MMC_CAP_SDIO_IRQ then presumably you've got some > problem where the SDIO interrupt is broken on your board. That means > you're using polling mode to find interrupts. As I understand it that > would allow you to use LOW_PWR since you don't need to detect SDIO > interrupts. I know on Marvell WiFi modules I tested this seemed to > work OK, but I doubt anyone is really running a production device in > this way. No, we needs to set MMC_CAP_SDIO_IRQ, if i didn't set to it, i will also see the problem. Since I knew which slot is used as SDIO card, i thought MMC_CAP_SDIO_IRQ is set for only sdio card. Then it would be check the twice whether card is SDIO or not. Actually, i didn't test WiFi module with this patch. but concept is not problem. Best Regards, Jaehoon Chung > > > Basically I was trying to mirror the functionality of the old code as > closely as possible. In the old code if you were using polling mode > then dw_mci_enable_sdio_irq() should never be called and you'd never > go out of low power mode. > > -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
Jaehoon, On Tue, Dec 2, 2014 at 5:06 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote: > Hi Doug > > On 12/03/2014 09:36 AM, Doug Anderson wrote: >> Jaehoon, >> >> On Tue, Dec 2, 2014 at 4:17 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote: >>>> @@ -1245,27 +1246,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 { >>> >>> I wonder this point. When entered at this point? >>> >>> MMC_CAP_SDIO_IRQ is sdio capability. and card->type is also related with SDIO, isn't? >> >> Right. As I understand it: >> >> * You can certainly add MMC_CAP_SDIO_IRQ to an external card slot that >> might have an MMC card, and SD card, or an SDIO card. You still want >> the LOW_PWR mode for MMC/SD cards but don't want it for SDIO cards in >> that case. > > Ok, I understood your purpose. > Your mean is MMC_CAP_SIDO_IRQ can be set to MMC or SD...not only SDIO card. > It's reasonable. > But i think it doesn't need to set LOW_PWR_mode for SD/eMMC at here. it should be already set to Low-power mode. > And SDIO case should not enter at here. Are you sure? What about if you plug in an SDIO card, then unplug it, then plug in an SD card? >> * If you don't set MMC_CAP_SDIO_IRQ then presumably you've got some >> problem where the SDIO interrupt is broken on your board. That means >> you're using polling mode to find interrupts. As I understand it that >> would allow you to use LOW_PWR since you don't need to detect SDIO >> interrupts. I know on Marvell WiFi modules I tested this seemed to >> work OK, but I doubt anyone is really running a production device in >> this way. > > No, we needs to set MMC_CAP_SDIO_IRQ, if i didn't set to it, i will also see the problem. > Since I knew which slot is used as SDIO card, i thought MMC_CAP_SDIO_IRQ is set for only sdio card. > Then it would be check the twice whether card is SDIO or not. > > Actually, i didn't test WiFi module with this patch. but concept is not problem. Yup, I think you were only thinking about the embedded case where you know that an SDIO card will be plugged into the slot. In the case where you have an exposed slot it is certainly possible to expect someone might plug in an SD card or an SDIO card. ...or even a combo card! -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
Doug. On 12/03/2014 10:12 AM, Doug Anderson wrote: > Jaehoon, > > On Tue, Dec 2, 2014 at 5:06 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote: >> Hi Doug >> >> On 12/03/2014 09:36 AM, Doug Anderson wrote: >>> Jaehoon, >>> >>> On Tue, Dec 2, 2014 at 4:17 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote: >>>>> @@ -1245,27 +1246,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 { >>>> >>>> I wonder this point. When entered at this point? >>>> >>>> MMC_CAP_SDIO_IRQ is sdio capability. and card->type is also related with SDIO, isn't? >>> >>> Right. As I understand it: >>> >>> * You can certainly add MMC_CAP_SDIO_IRQ to an external card slot that >>> might have an MMC card, and SD card, or an SDIO card. You still want >>> the LOW_PWR mode for MMC/SD cards but don't want it for SDIO cards in >>> that case. >> >> Ok, I understood your purpose. >> Your mean is MMC_CAP_SIDO_IRQ can be set to MMC or SD...not only SDIO card. >> It's reasonable. >> But i think it doesn't need to set LOW_PWR_mode for SD/eMMC at here. it should be already set to Low-power mode. >> And SDIO case should not enter at here. > > Are you sure? What about if you plug in an SDIO card, then unplug it, > then plug in an SD card? Oh.. :) I missed this case. thank you for pointing out. I considered only non-removable case, it's stupid... Best Regards, Jaehoon Chung > >>> * If you don't set MMC_CAP_SDIO_IRQ then presumably you've got some >>> problem where the SDIO interrupt is broken on your board. That means >>> you're using polling mode to find interrupts. As I understand it that >>> would allow you to use LOW_PWR since you don't need to detect SDIO >>> interrupts. I know on Marvell WiFi modules I tested this seemed to >>> work OK, but I doubt anyone is really running a production device in >>> this way. >> >> No, we needs to set MMC_CAP_SDIO_IRQ, if i didn't set to it, i will also see the problem. >> Since I knew which slot is used as SDIO card, i thought MMC_CAP_SDIO_IRQ is set for only sdio card. >> Then it would be check the twice whether card is SDIO or not. >> >> Actually, i didn't test WiFi module with this patch. but concept is not problem. > > Yup, I think you were only thinking about the embedded case where you > know that an SDIO card will be plugged into the slot. In the case > where you have an exposed slot it is certainly possible to expect > someone might plug in an SD card or an SDIO card. ...or even a combo > card! > > -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
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c index 67c0451..ae10a02 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/sd.h> @@ -926,7 +927,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->sdio_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); @@ -1245,27 +1246,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); + } } } @@ -1277,21 +1288,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->sdio_id))); - } else { - mci_writel(host, INTMASK, - (int_mask & ~SDMMC_INT_SDIO(slot->sdio_id))); - } + if (enb) + int_mask |= SDMMC_INT_SDIO(slot->sdio_id); + else + int_mask &= ~SDMMC_INT_SDIO(slot->sdio_id); + mci_writel(host, INTMASK, int_mask); } static int dw_mci_execute_tuning(struct mmc_host *mmc, u32 opcode) @@ -1337,7 +1338,7 @@ static const struct mmc_host_ops dw_mci_ops = { .execute_tuning = dw_mci_execute_tuning, .card_busy = dw_mci_card_busy, .start_signal_voltage_switch = dw_mci_switch_voltage, - + .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 0d0f7a2..d27d4c0 100644 --- a/drivers/mmc/host/dw_mmc.h +++ b/drivers/mmc/host/dw_mmc.h @@ -244,6 +244,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 sdio_id; };
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 do this right at bootup. Signed-off-by: Doug Anderson <dianders@chromium.org> --- Changes in v5: None Changes in v3: None Changes in v2: - Fixed "|" to "&". drivers/mmc/host/dw_mmc.c | 69 ++++++++++++++++++++++++----------------------- drivers/mmc/host/dw_mmc.h | 1 + 2 files changed, 36 insertions(+), 34 deletions(-)