| Message ID | 1403520321-2431-2-git-send-email-yuvaraj.cd@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Yuvaraj, On Mon, Jun 23, 2014 at 3:45 AM, Yuvaraj Kumar C D <yuvaraj.cd@gmail.com> wrote: > This patch makes use of mmc_regulator_get_supply() to handle > the vmmc and vqmmc regulators.Also it moves the code handling > the these regulators to dw_mci_set_ios().It turned on the vmmc > and vqmmc during MMC_POWER_UP and MMC_POWER_ON,and turned off > during MMC_POWER_OFF. > > Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.com> > --- > drivers/mmc/host/dw_mmc.c | 71 ++++++++++++++++++++++----------------------- > drivers/mmc/host/dw_mmc.h | 2 ++ > 2 files changed, 36 insertions(+), 37 deletions(-) Perhaps you could CC me on the whole series for the next version since I was involved in privately reviewing previous versions? Overall caveat for my review is that I'm nowhere near an SD/MMC expert. > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > index 1ac227c..f5cabce 100644 > --- a/drivers/mmc/host/dw_mmc.c > +++ b/drivers/mmc/host/dw_mmc.c > @@ -937,6 +937,7 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) > struct dw_mci_slot *slot = mmc_priv(mmc); > const struct dw_mci_drv_data *drv_data = slot->host->drv_data; > u32 regs; > + int ret; > > switch (ios->bus_width) { > case MMC_BUS_WIDTH_4: > @@ -975,16 +976,41 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) > > switch (ios->power_mode) { > case MMC_POWER_UP: > + if ((!IS_ERR(mmc->supply.vmmc)) && > + !test_bit(DW_MMC_CARD_POWERED, &slot->flags)) { > + ret = regulator_enable(mmc->supply.vmmc); > + if (!ret) > + set_bit(DW_MMC_CARD_POWERED, &slot->flags); > + } As per below, I'm not sure why you'd want to turn on vqmmc and vmmc at different times. Also: if you fail to turn on either of the regulators it feels like you should print a pretty nasty error message since your device will almost certainly not work. > set_bit(DW_MMC_CARD_NEED_INIT, &slot->flags); > regs = mci_readl(slot->host, PWREN); > regs |= (1 << slot->id); > mci_writel(slot->host, PWREN, regs); > break; > case MMC_POWER_OFF: > + if (!IS_ERR(mmc->supply.vqmmc) && > + test_bit(DW_MMC_IO_POWERED, &slot->flags)) { > + ret = regulator_disable(mmc->supply.vqmmc); > + if (!ret) > + clear_bit(DW_MMC_IO_POWERED, &slot->flags); > + } > + if (!IS_ERR(mmc->supply.vmmc) && > + test_bit(DW_MMC_CARD_POWERED, &slot->flags)) { > + ret = regulator_disable(mmc->supply.vmmc); > + if (!ret) > + clear_bit(DW_MMC_CARD_POWERED, &slot->flags); > + } > regs = mci_readl(slot->host, PWREN); > regs &= ~(1 << slot->id); > mci_writel(slot->host, PWREN, regs); > break; > + case MMC_POWER_ON: > + if (!IS_ERR(mmc->supply.vqmmc) && > + !test_bit(DW_MMC_IO_POWERED, &slot->flags)) { > + ret = regulator_enable(mmc->supply.vqmmc); > + if (!ret) > + set_bit(DW_MMC_IO_POWERED, &slot->flags); > + } > default: > break; > } > @@ -2067,7 +2093,13 @@ static int dw_mci_init_slot(struct dw_mci *host, unsigned int id) > mmc->f_max = freq[1]; > } > > - mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34; > + /*if there are external regulators, get them*/ > + ret = mmc_regulator_get_supply(mmc); > + if (ret == -EPROBE_DEFER) > + goto err_setup_bus; > + > + if (!mmc->ocr_avail) > + mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34; > > if (host->pdata->caps) > mmc->caps = host->pdata->caps; > @@ -2133,7 +2165,7 @@ static int dw_mci_init_slot(struct dw_mci *host, unsigned int id) > > err_setup_bus: > mmc_free_host(mmc); > - return -EINVAL; > + return ret; > } > > static void dw_mci_cleanup_slot(struct dw_mci_slot *slot, unsigned int id) > @@ -2375,24 +2407,6 @@ int dw_mci_probe(struct dw_mci *host) > } > } > > - host->vmmc = devm_regulator_get_optional(host->dev, "vmmc"); > - if (IS_ERR(host->vmmc)) { > - ret = PTR_ERR(host->vmmc); > - if (ret == -EPROBE_DEFER) > - goto err_clk_ciu; > - > - dev_info(host->dev, "no vmmc regulator found: %d\n", ret); > - host->vmmc = NULL; > - } else { > - ret = regulator_enable(host->vmmc); > - if (ret) { > - if (ret != -EPROBE_DEFER) > - dev_err(host->dev, > - "regulator_enable fail: %d\n", ret); > - goto err_clk_ciu; > - } > - } > - > host->quirks = host->pdata->quirks; > > spin_lock_init(&host->lock); > @@ -2536,8 +2550,6 @@ err_workqueue: > err_dmaunmap: > if (host->use_dma && host->dma_ops->exit) > host->dma_ops->exit(host); > - if (host->vmmc) > - regulator_disable(host->vmmc); > > err_clk_ciu: > if (!IS_ERR(host->ciu_clk)) > @@ -2573,9 +2585,6 @@ void dw_mci_remove(struct dw_mci *host) > if (host->use_dma && host->dma_ops->exit) > host->dma_ops->exit(host); > > - if (host->vmmc) > - regulator_disable(host->vmmc); > - > if (!IS_ERR(host->ciu_clk)) > clk_disable_unprepare(host->ciu_clk); > > @@ -2592,9 +2601,6 @@ EXPORT_SYMBOL(dw_mci_remove); > */ > int dw_mci_suspend(struct dw_mci *host) > { > - if (host->vmmc) > - regulator_disable(host->vmmc); > - Just to check: have you tested suspend/resume with the various combinations of "keep-power-in-suspend" and "non-removable" with your patch. I remember some of the logic being a bit complicated... > return 0; > } > EXPORT_SYMBOL(dw_mci_suspend); > @@ -2603,15 +2609,6 @@ int dw_mci_resume(struct dw_mci *host) > { > int i, ret; > > - if (host->vmmc) { > - ret = regulator_enable(host->vmmc); > - if (ret) { > - dev_err(host->dev, > - "failed to enable regulator: %d\n", ret); > - return ret; > - } > - } > - > if (!dw_mci_ctrl_all_reset(host)) { > ret = -ENODEV; > return ret; > diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h > index 738fa24..5c95d00 100644 > --- a/drivers/mmc/host/dw_mmc.h > +++ b/drivers/mmc/host/dw_mmc.h As I mentioned in my previous review, you should be removing "struct regulator *vmmc; /* Power regulator */" from "struct dw_mci" since you're no longer populating it. > @@ -225,6 +225,8 @@ struct dw_mci_slot { > unsigned long flags; > #define DW_MMC_CARD_PRESENT 0 > #define DW_MMC_CARD_NEED_INIT 1 > +#define DW_MMC_CARD_POWERED 2 > +#define DW_MMC_IO_POWERED 3 I don't really think you should have two bits here. From my understanding of SD cards there should be very little reason to have vqmmc and vmmc not powered at the same time. If vqmmc is powered but vmmc is not powered then you'll get leakage and the card can pull power through the IO lines which is bad for the card. I don't think that powering vmmc without vqmmc for a short time is terribly bad but I can't quite see a good use case. Essentially you're powering the card but not able to talk to it, right? I'm not sure what the state of the IO lines would be (either driven low or floating) since presumably the pullups on the lines are powered by vqmmc too. > 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
On 06/25/2014 03:00 AM, Doug Anderson wrote: > Yuvaraj, > > On Mon, Jun 23, 2014 at 3:45 AM, Yuvaraj Kumar C D <yuvaraj.cd@gmail.com> wrote: >> This patch makes use of mmc_regulator_get_supply() to handle >> the vmmc and vqmmc regulators.Also it moves the code handling >> the these regulators to dw_mci_set_ios().It turned on the vmmc >> and vqmmc during MMC_POWER_UP and MMC_POWER_ON,and turned off >> during MMC_POWER_OFF. >> >> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.com> >> --- >> drivers/mmc/host/dw_mmc.c | 71 ++++++++++++++++++++++----------------------- >> drivers/mmc/host/dw_mmc.h | 2 ++ >> 2 files changed, 36 insertions(+), 37 deletions(-) > > Perhaps you could CC me on the whole series for the next version since > I was involved in privately reviewing previous versions? > > Overall caveat for my review is that I'm nowhere near an SD/MMC expert. > > >> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c >> index 1ac227c..f5cabce 100644 >> --- a/drivers/mmc/host/dw_mmc.c >> +++ b/drivers/mmc/host/dw_mmc.c >> @@ -937,6 +937,7 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) >> struct dw_mci_slot *slot = mmc_priv(mmc); >> const struct dw_mci_drv_data *drv_data = slot->host->drv_data; >> u32 regs; >> + int ret; >> >> switch (ios->bus_width) { >> case MMC_BUS_WIDTH_4: >> @@ -975,16 +976,41 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) >> >> switch (ios->power_mode) { >> case MMC_POWER_UP: >> + if ((!IS_ERR(mmc->supply.vmmc)) && >> + !test_bit(DW_MMC_CARD_POWERED, &slot->flags)) { >> + ret = regulator_enable(mmc->supply.vmmc); >> + if (!ret) >> + set_bit(DW_MMC_CARD_POWERED, &slot->flags); MMC_CARD_POWERED? I didn't know why it needs. >> + } > > As per below, I'm not sure why you'd want to turn on vqmmc and vmmc at > different times. In my case, in order to prevent the H/W mis-operation, it turns on vqmmc and vmmc at different times. > > Also: if you fail to turn on either of the regulators it feels like > you should print a pretty nasty error message since your device will > almost certainly not work. > > >> set_bit(DW_MMC_CARD_NEED_INIT, &slot->flags); >> regs = mci_readl(slot->host, PWREN); >> regs |= (1 << slot->id); >> mci_writel(slot->host, PWREN, regs); >> break; >> case MMC_POWER_OFF: >> + if (!IS_ERR(mmc->supply.vqmmc) && >> + test_bit(DW_MMC_IO_POWERED, &slot->flags)) { >> + ret = regulator_disable(mmc->supply.vqmmc); >> + if (!ret) >> + clear_bit(DW_MMC_IO_POWERED, &slot->flags); >> + } >> + if (!IS_ERR(mmc->supply.vmmc) && >> + test_bit(DW_MMC_CARD_POWERED, &slot->flags)) { >> + ret = regulator_disable(mmc->supply.vmmc); >> + if (!ret) >> + clear_bit(DW_MMC_CARD_POWERED, &slot->flags); >> + } >> regs = mci_readl(slot->host, PWREN); >> regs &= ~(1 << slot->id); >> mci_writel(slot->host, PWREN, regs); >> break; >> + case MMC_POWER_ON: >> + if (!IS_ERR(mmc->supply.vqmmc) && >> + !test_bit(DW_MMC_IO_POWERED, &slot->flags)) { >> + ret = regulator_enable(mmc->supply.vqmmc); >> + if (!ret) >> + set_bit(DW_MMC_IO_POWERED, &slot->flags); >> + } >> default: >> break; >> } >> @@ -2067,7 +2093,13 @@ static int dw_mci_init_slot(struct dw_mci *host, unsigned int id) >> mmc->f_max = freq[1]; >> } >> >> - mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34; >> + /*if there are external regulators, get them*/ >> + ret = mmc_regulator_get_supply(mmc); >> + if (ret == -EPROBE_DEFER) >> + goto err_setup_bus; >> + >> + if (!mmc->ocr_avail) >> + mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34; >> >> if (host->pdata->caps) >> mmc->caps = host->pdata->caps; >> @@ -2133,7 +2165,7 @@ static int dw_mci_init_slot(struct dw_mci *host, unsigned int id) >> >> err_setup_bus: >> mmc_free_host(mmc); >> - return -EINVAL; >> + return ret; >> } >> >> static void dw_mci_cleanup_slot(struct dw_mci_slot *slot, unsigned int id) >> @@ -2375,24 +2407,6 @@ int dw_mci_probe(struct dw_mci *host) >> } >> } >> >> - host->vmmc = devm_regulator_get_optional(host->dev, "vmmc"); >> - if (IS_ERR(host->vmmc)) { >> - ret = PTR_ERR(host->vmmc); >> - if (ret == -EPROBE_DEFER) >> - goto err_clk_ciu; >> - >> - dev_info(host->dev, "no vmmc regulator found: %d\n", ret); >> - host->vmmc = NULL; >> - } else { >> - ret = regulator_enable(host->vmmc); >> - if (ret) { >> - if (ret != -EPROBE_DEFER) >> - dev_err(host->dev, >> - "regulator_enable fail: %d\n", ret); >> - goto err_clk_ciu; >> - } >> - } >> - >> host->quirks = host->pdata->quirks; >> >> spin_lock_init(&host->lock); >> @@ -2536,8 +2550,6 @@ err_workqueue: >> err_dmaunmap: >> if (host->use_dma && host->dma_ops->exit) >> host->dma_ops->exit(host); >> - if (host->vmmc) >> - regulator_disable(host->vmmc); >> >> err_clk_ciu: >> if (!IS_ERR(host->ciu_clk)) >> @@ -2573,9 +2585,6 @@ void dw_mci_remove(struct dw_mci *host) >> if (host->use_dma && host->dma_ops->exit) >> host->dma_ops->exit(host); >> >> - if (host->vmmc) >> - regulator_disable(host->vmmc); >> - >> if (!IS_ERR(host->ciu_clk)) >> clk_disable_unprepare(host->ciu_clk); >> >> @@ -2592,9 +2601,6 @@ EXPORT_SYMBOL(dw_mci_remove); >> */ >> int dw_mci_suspend(struct dw_mci *host) >> { >> - if (host->vmmc) >> - regulator_disable(host->vmmc); >> - > > Just to check: have you tested suspend/resume with the various > combinations of "keep-power-in-suspend" and "non-removable" with your > patch. I remember some of the logic being a bit complicated... > > >> return 0; >> } >> EXPORT_SYMBOL(dw_mci_suspend); >> @@ -2603,15 +2609,6 @@ int dw_mci_resume(struct dw_mci *host) >> { >> int i, ret; >> >> - if (host->vmmc) { >> - ret = regulator_enable(host->vmmc); >> - if (ret) { >> - dev_err(host->dev, >> - "failed to enable regulator: %d\n", ret); >> - return ret; >> - } >> - } >> - >> if (!dw_mci_ctrl_all_reset(host)) { >> ret = -ENODEV; >> return ret; >> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h >> index 738fa24..5c95d00 100644 >> --- a/drivers/mmc/host/dw_mmc.h >> +++ b/drivers/mmc/host/dw_mmc.h > > As I mentioned in my previous review, you should be removing "struct > regulator *vmmc; /* Power regulator */" from "struct dw_mci" since > you're no longer populating it. > > >> @@ -225,6 +225,8 @@ struct dw_mci_slot { >> unsigned long flags; >> #define DW_MMC_CARD_PRESENT 0 >> #define DW_MMC_CARD_NEED_INIT 1 >> +#define DW_MMC_CARD_POWERED 2 >> +#define DW_MMC_IO_POWERED 3 > > I don't really think you should have two bits here. From my > understanding of SD cards there should be very little reason to have > vqmmc and vmmc not powered at the same time. > > If vqmmc is powered but vmmc is not powered then you'll get leakage > and the card can pull power through the IO lines which is bad for the > card. > > I don't think that powering vmmc without vqmmc for a short time is > terribly bad but I can't quite see a good use case. Essentially > you're powering the card but not able to talk to it, right? I'm not > sure what the state of the IO lines would be (either driven low or > floating) since presumably the pullups on the lines are powered by > vqmmc too. > > >> 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 > -- 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, Jun 24, 2014 at 8:18 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote: > On 06/25/2014 03:00 AM, Doug Anderson wrote: >> Yuvaraj, >> >> On Mon, Jun 23, 2014 at 3:45 AM, Yuvaraj Kumar C D <yuvaraj.cd@gmail.com> wrote: >>> This patch makes use of mmc_regulator_get_supply() to handle >>> the vmmc and vqmmc regulators.Also it moves the code handling >>> the these regulators to dw_mci_set_ios().It turned on the vmmc >>> and vqmmc during MMC_POWER_UP and MMC_POWER_ON,and turned off >>> during MMC_POWER_OFF. >>> >>> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.com> >>> --- >>> drivers/mmc/host/dw_mmc.c | 71 ++++++++++++++++++++++----------------------- >>> drivers/mmc/host/dw_mmc.h | 2 ++ >>> 2 files changed, 36 insertions(+), 37 deletions(-) >> >> Perhaps you could CC me on the whole series for the next version since >> I was involved in privately reviewing previous versions? >> >> Overall caveat for my review is that I'm nowhere near an SD/MMC expert. >> >> >>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c >>> index 1ac227c..f5cabce 100644 >>> --- a/drivers/mmc/host/dw_mmc.c >>> +++ b/drivers/mmc/host/dw_mmc.c >>> @@ -937,6 +937,7 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) >>> struct dw_mci_slot *slot = mmc_priv(mmc); >>> const struct dw_mci_drv_data *drv_data = slot->host->drv_data; >>> u32 regs; >>> + int ret; >>> >>> switch (ios->bus_width) { >>> case MMC_BUS_WIDTH_4: >>> @@ -975,16 +976,41 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) >>> >>> switch (ios->power_mode) { >>> case MMC_POWER_UP: >>> + if ((!IS_ERR(mmc->supply.vmmc)) && >>> + !test_bit(DW_MMC_CARD_POWERED, &slot->flags)) { >>> + ret = regulator_enable(mmc->supply.vmmc); >>> + if (!ret) >>> + set_bit(DW_MMC_CARD_POWERED, &slot->flags); > > MMC_CARD_POWERED? I didn't know why it needs. I think the idea was to make sure that regulator enables/disables were balanced. ...but if the mmc core does this for us then we probably don't need to worry. >>> + } >> >> As per below, I'm not sure why you'd want to turn on vqmmc and vmmc at >> different times. > In my case, in order to prevent the H/W mis-operation, it turns on vqmmc and vmmc at different times. OK! Can you explain more? What instance is one powered but not the other? I know that we talked to an SD Card manufacturer who told us that powering their IO lines without their main power rail was a bad thing (and we in fact saw this causing problems on SD cards). I would guess that means that you're either powering vmmc before vqmmc or that vqmmc somehow doesn't directly enable pullups for IO lines. If you need vmmc before vqmmc then I'm curious about why... -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 Yuvaraj. This patch looks like similar Jaehoon's. Is it resending? Anyway, I added some comments below. On Wed, June 25, 2014, Jaehoon Chung wrote: > On 06/25/2014 03:00 AM, Doug Anderson wrote: > > Yuvaraj, > > > > On Mon, Jun 23, 2014 at 3:45 AM, Yuvaraj Kumar C D <yuvaraj.cd@gmail.com> wrote: > >> This patch makes use of mmc_regulator_get_supply() to handle > >> the vmmc and vqmmc regulators.Also it moves the code handling > >> the these regulators to dw_mci_set_ios().It turned on the vmmc > >> and vqmmc during MMC_POWER_UP and MMC_POWER_ON,and turned off > >> during MMC_POWER_OFF. > >> > >> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.com> > >> --- > >> drivers/mmc/host/dw_mmc.c | 71 ++++++++++++++++++++++----------------------- > >> drivers/mmc/host/dw_mmc.h | 2 ++ > >> 2 files changed, 36 insertions(+), 37 deletions(-) > > > > Perhaps you could CC me on the whole series for the next version since > > I was involved in privately reviewing previous versions? > > > > Overall caveat for my review is that I'm nowhere near an SD/MMC expert. > > > > > >> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > >> index 1ac227c..f5cabce 100644 > >> --- a/drivers/mmc/host/dw_mmc.c > >> +++ b/drivers/mmc/host/dw_mmc.c > >> @@ -937,6 +937,7 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) > >> struct dw_mci_slot *slot = mmc_priv(mmc); > >> const struct dw_mci_drv_data *drv_data = slot->host->drv_data; > >> u32 regs; > >> + int ret; > >> > >> switch (ios->bus_width) { > >> case MMC_BUS_WIDTH_4: > >> @@ -975,16 +976,41 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) > >> > >> switch (ios->power_mode) { > >> case MMC_POWER_UP: > >> + if ((!IS_ERR(mmc->supply.vmmc)) && > >> + !test_bit(DW_MMC_CARD_POWERED, &slot->flags)) { > >> + ret = regulator_enable(mmc->supply.vmmc); > >> + if (!ret) > >> + set_bit(DW_MMC_CARD_POWERED, &slot->flags); > > MMC_CARD_POWERED? I didn't know why it needs. Also, ios->vdd is missed. Use mmc_regulator_set_ocr() instead of regulator_enable() for vmmc. And with mmc_regulator_set_ocr(), we don't need to check additional flag for enable/disable. It does that already. > > >> + } > > > > As per below, I'm not sure why you'd want to turn on vqmmc and vmmc at > > different times. > In my case, in order to prevent the H/W mis-operation, it turns on vqmmc and vmmc at different times. > > > > > Also: if you fail to turn on either of the regulators it feels like > > you should print a pretty nasty error message since your device will > > almost certainly not work. > > > > > >> set_bit(DW_MMC_CARD_NEED_INIT, &slot->flags); > >> regs = mci_readl(slot->host, PWREN); > >> regs |= (1 << slot->id); > >> mci_writel(slot->host, PWREN, regs); > >> break; > >> case MMC_POWER_OFF: > >> + if (!IS_ERR(mmc->supply.vqmmc) && > >> + test_bit(DW_MMC_IO_POWERED, &slot->flags)) { > >> + ret = regulator_disable(mmc->supply.vqmmc); > >> + if (!ret) > >> + clear_bit(DW_MMC_IO_POWERED, &slot->flags); > >> + } > >> + if (!IS_ERR(mmc->supply.vmmc) && > >> + test_bit(DW_MMC_CARD_POWERED, &slot->flags)) { > >> + ret = regulator_disable(mmc->supply.vmmc); > >> + if (!ret) > >> + clear_bit(DW_MMC_CARD_POWERED, &slot->flags); > >> + } > >> regs = mci_readl(slot->host, PWREN); > >> regs &= ~(1 << slot->id); > >> mci_writel(slot->host, PWREN, regs); > >> break; > >> + case MMC_POWER_ON: > >> + if (!IS_ERR(mmc->supply.vqmmc) && > >> + !test_bit(DW_MMC_IO_POWERED, &slot->flags)) { You can use regulator_is_enabled() instead of flag bit, DW_MMC_IO_POWERED. Important thing is that if powering vmmc failed at MMC_POWER_UP, vqmmc should not be powered. Please consider Doug's mentions below. > >> + ret = regulator_enable(mmc->supply.vqmmc); > >> + if (!ret) > >> + set_bit(DW_MMC_IO_POWERED, &slot->flags); > >> + } > >> default: > >> break; > >> } > >> @@ -2067,7 +2093,13 @@ static int dw_mci_init_slot(struct dw_mci *host, unsigned int id) > >> mmc->f_max = freq[1]; > >> } > >> > >> - mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34; > >> + /*if there are external regulators, get them*/ > >> + ret = mmc_regulator_get_supply(mmc); > >> + if (ret == -EPROBE_DEFER) > >> + goto err_setup_bus; > >> + > >> + if (!mmc->ocr_avail) > >> + mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34; > >> > >> if (host->pdata->caps) > >> mmc->caps = host->pdata->caps; > >> @@ -2133,7 +2165,7 @@ static int dw_mci_init_slot(struct dw_mci *host, unsigned int id) > >> > >> err_setup_bus: > >> mmc_free_host(mmc); > >> - return -EINVAL; > >> + return ret; > >> } > >> > >> static void dw_mci_cleanup_slot(struct dw_mci_slot *slot, unsigned int id) > >> @@ -2375,24 +2407,6 @@ int dw_mci_probe(struct dw_mci *host) > >> } > >> } > >> > >> - host->vmmc = devm_regulator_get_optional(host->dev, "vmmc"); > >> - if (IS_ERR(host->vmmc)) { > >> - ret = PTR_ERR(host->vmmc); > >> - if (ret == -EPROBE_DEFER) > >> - goto err_clk_ciu; > >> - > >> - dev_info(host->dev, "no vmmc regulator found: %d\n", ret); > >> - host->vmmc = NULL; > >> - } else { > >> - ret = regulator_enable(host->vmmc); > >> - if (ret) { > >> - if (ret != -EPROBE_DEFER) > >> - dev_err(host->dev, > >> - "regulator_enable fail: %d\n", ret); > >> - goto err_clk_ciu; > >> - } > >> - } > >> - > >> host->quirks = host->pdata->quirks; > >> > >> spin_lock_init(&host->lock); > >> @@ -2536,8 +2550,6 @@ err_workqueue: > >> err_dmaunmap: > >> if (host->use_dma && host->dma_ops->exit) > >> host->dma_ops->exit(host); > >> - if (host->vmmc) > >> - regulator_disable(host->vmmc); > >> > >> err_clk_ciu: > >> if (!IS_ERR(host->ciu_clk)) > >> @@ -2573,9 +2585,6 @@ void dw_mci_remove(struct dw_mci *host) > >> if (host->use_dma && host->dma_ops->exit) > >> host->dma_ops->exit(host); > >> > >> - if (host->vmmc) > >> - regulator_disable(host->vmmc); > >> - > >> if (!IS_ERR(host->ciu_clk)) > >> clk_disable_unprepare(host->ciu_clk); > >> > >> @@ -2592,9 +2601,6 @@ EXPORT_SYMBOL(dw_mci_remove); > >> */ > >> int dw_mci_suspend(struct dw_mci *host) > >> { > >> - if (host->vmmc) > >> - regulator_disable(host->vmmc); > >> - > > > > Just to check: have you tested suspend/resume with the various > > combinations of "keep-power-in-suspend" and "non-removable" with your > > patch. I remember some of the logic being a bit complicated... Please check it and give some updates/result in next version. Thanks, Seungwon Jeon > > > > > >> return 0; > >> } > >> EXPORT_SYMBOL(dw_mci_suspend); > >> @@ -2603,15 +2609,6 @@ int dw_mci_resume(struct dw_mci *host) > >> { > >> int i, ret; > >> > >> - if (host->vmmc) { > >> - ret = regulator_enable(host->vmmc); > >> - if (ret) { > >> - dev_err(host->dev, > >> - "failed to enable regulator: %d\n", ret); > >> - return ret; > >> - } > >> - } > >> - > >> if (!dw_mci_ctrl_all_reset(host)) { > >> ret = -ENODEV; > >> return ret; > >> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h > >> index 738fa24..5c95d00 100644 > >> --- a/drivers/mmc/host/dw_mmc.h > >> +++ b/drivers/mmc/host/dw_mmc.h > > > > As I mentioned in my previous review, you should be removing "struct > > regulator *vmmc; /* Power regulator */" from "struct dw_mci" since > > you're no longer populating it. > > > > > >> @@ -225,6 +225,8 @@ struct dw_mci_slot { > >> unsigned long flags; > >> #define DW_MMC_CARD_PRESENT 0 > >> #define DW_MMC_CARD_NEED_INIT 1 > >> +#define DW_MMC_CARD_POWERED 2 > >> +#define DW_MMC_IO_POWERED 3 > > > > I don't really think you should have two bits here. From my > > understanding of SD cards there should be very little reason to have > > vqmmc and vmmc not powered at the same time. > > > > If vqmmc is powered but vmmc is not powered then you'll get leakage > > and the card can pull power through the IO lines which is bad for the > > card. > > > > I don't think that powering vmmc without vqmmc for a short time is > > terribly bad but I can't quite see a good use case. Essentially > > you're powering the card but not able to talk to it, right? I'm not > > sure what the state of the IO lines would be (either driven low or > > floating) since presumably the pullups on the lines are powered by > > vqmmc too. > > > > > >> 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 > > > > -- > 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, Jun 25, 2014 at 4:18 AM, Seungwon Jeon <tgih.jun@samsung.com> wrote: >> >> + case MMC_POWER_ON: >> >> + if (!IS_ERR(mmc->supply.vqmmc) && >> >> + !test_bit(DW_MMC_IO_POWERED, &slot->flags)) { > You can use regulator_is_enabled() instead of flag bit, DW_MMC_IO_POWERED. I'd be a little worried about regulator_is_enabled() since regulators are reference counted. What if someone else is sharing this regulator? The regulator might happen to be enabled when you check it but unless you add your own dw_mmc reference count they might turn it off. > Important thing is that if powering vmmc failed at MMC_POWER_UP, vqmmc should not be powered. Good point. -- 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 Thu, June 26, 2014, Doug Anderson wrote: > Seungwon, > > On Wed, Jun 25, 2014 at 4:18 AM, Seungwon Jeon <tgih.jun@samsung.com> wrote: > >> >> + case MMC_POWER_ON: > >> >> + if (!IS_ERR(mmc->supply.vqmmc) && > >> >> + !test_bit(DW_MMC_IO_POWERED, &slot->flags)) { > > You can use regulator_is_enabled() instead of flag bit, DW_MMC_IO_POWERED. > > I'd be a little worried about regulator_is_enabled() since regulators > are reference counted. What if someone else is sharing this > regulator? The regulator might happen to be enabled when you check it > but unless you add your own dw_mmc reference count they might turn it > off. Cool, that's a possibility. Some assumption may need. If mmc's core can guarantee its balance, I think we don't need to consider some flag. Thanks, Seungwon Jeon > > > Important thing is that if powering vmmc failed at MMC_POWER_UP, vqmmc should not be powered. > > Good point. > -- > 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
Doug On Tue, Jun 24, 2014 at 11:30 PM, Doug Anderson <dianders@chromium.org> wrote: > Yuvaraj, > > On Mon, Jun 23, 2014 at 3:45 AM, Yuvaraj Kumar C D <yuvaraj.cd@gmail.com> wrote: >> This patch makes use of mmc_regulator_get_supply() to handle >> the vmmc and vqmmc regulators.Also it moves the code handling >> the these regulators to dw_mci_set_ios().It turned on the vmmc >> and vqmmc during MMC_POWER_UP and MMC_POWER_ON,and turned off >> during MMC_POWER_OFF. >> >> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.com> >> --- >> drivers/mmc/host/dw_mmc.c | 71 ++++++++++++++++++++++----------------------- >> drivers/mmc/host/dw_mmc.h | 2 ++ >> 2 files changed, 36 insertions(+), 37 deletions(-) > > Perhaps you could CC me on the whole series for the next version since > I was involved in privately reviewing previous versions? It was just accidental missing you in the CC .Surely i will add you in CC for next versions. > > Overall caveat for my review is that I'm nowhere near an SD/MMC expert. > > >> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c >> index 1ac227c..f5cabce 100644 >> --- a/drivers/mmc/host/dw_mmc.c >> +++ b/drivers/mmc/host/dw_mmc.c >> @@ -937,6 +937,7 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) >> struct dw_mci_slot *slot = mmc_priv(mmc); >> const struct dw_mci_drv_data *drv_data = slot->host->drv_data; >> u32 regs; >> + int ret; >> >> switch (ios->bus_width) { >> case MMC_BUS_WIDTH_4: >> @@ -975,16 +976,41 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) >> >> switch (ios->power_mode) { >> case MMC_POWER_UP: >> + if ((!IS_ERR(mmc->supply.vmmc)) && >> + !test_bit(DW_MMC_CARD_POWERED, &slot->flags)) { >> + ret = regulator_enable(mmc->supply.vmmc); >> + if (!ret) >> + set_bit(DW_MMC_CARD_POWERED, &slot->flags); >> + } > > As per below, I'm not sure why you'd want to turn on vqmmc and vmmc at > different times. As you can see people's have different opinion on this.When i had a look at the other drivers in the subsystem which does in the same flow as above.However i will change in the next version. > > Also: if you fail to turn on either of the regulators it feels like > you should print a pretty nasty error message since your device will > almost certainly not work. Yes. I will add a error message. > > >> set_bit(DW_MMC_CARD_NEED_INIT, &slot->flags); >> regs = mci_readl(slot->host, PWREN); >> regs |= (1 << slot->id); >> mci_writel(slot->host, PWREN, regs); >> break; >> case MMC_POWER_OFF: >> + if (!IS_ERR(mmc->supply.vqmmc) && >> + test_bit(DW_MMC_IO_POWERED, &slot->flags)) { >> + ret = regulator_disable(mmc->supply.vqmmc); >> + if (!ret) >> + clear_bit(DW_MMC_IO_POWERED, &slot->flags); >> + } >> + if (!IS_ERR(mmc->supply.vmmc) && >> + test_bit(DW_MMC_CARD_POWERED, &slot->flags)) { >> + ret = regulator_disable(mmc->supply.vmmc); >> + if (!ret) >> + clear_bit(DW_MMC_CARD_POWERED, &slot->flags); >> + } >> regs = mci_readl(slot->host, PWREN); >> regs &= ~(1 << slot->id); >> mci_writel(slot->host, PWREN, regs); >> break; >> + case MMC_POWER_ON: >> + if (!IS_ERR(mmc->supply.vqmmc) && >> + !test_bit(DW_MMC_IO_POWERED, &slot->flags)) { >> + ret = regulator_enable(mmc->supply.vqmmc); >> + if (!ret) >> + set_bit(DW_MMC_IO_POWERED, &slot->flags); >> + } >> default: >> break; >> } >> @@ -2067,7 +2093,13 @@ static int dw_mci_init_slot(struct dw_mci *host, unsigned int id) >> mmc->f_max = freq[1]; >> } >> >> - mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34; >> + /*if there are external regulators, get them*/ >> + ret = mmc_regulator_get_supply(mmc); >> + if (ret == -EPROBE_DEFER) >> + goto err_setup_bus; >> + >> + if (!mmc->ocr_avail) >> + mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34; >> >> if (host->pdata->caps) >> mmc->caps = host->pdata->caps; >> @@ -2133,7 +2165,7 @@ static int dw_mci_init_slot(struct dw_mci *host, unsigned int id) >> >> err_setup_bus: >> mmc_free_host(mmc); >> - return -EINVAL; >> + return ret; >> } >> >> static void dw_mci_cleanup_slot(struct dw_mci_slot *slot, unsigned int id) >> @@ -2375,24 +2407,6 @@ int dw_mci_probe(struct dw_mci *host) >> } >> } >> >> - host->vmmc = devm_regulator_get_optional(host->dev, "vmmc"); >> - if (IS_ERR(host->vmmc)) { >> - ret = PTR_ERR(host->vmmc); >> - if (ret == -EPROBE_DEFER) >> - goto err_clk_ciu; >> - >> - dev_info(host->dev, "no vmmc regulator found: %d\n", ret); >> - host->vmmc = NULL; >> - } else { >> - ret = regulator_enable(host->vmmc); >> - if (ret) { >> - if (ret != -EPROBE_DEFER) >> - dev_err(host->dev, >> - "regulator_enable fail: %d\n", ret); >> - goto err_clk_ciu; >> - } >> - } >> - >> host->quirks = host->pdata->quirks; >> >> spin_lock_init(&host->lock); >> @@ -2536,8 +2550,6 @@ err_workqueue: >> err_dmaunmap: >> if (host->use_dma && host->dma_ops->exit) >> host->dma_ops->exit(host); >> - if (host->vmmc) >> - regulator_disable(host->vmmc); >> >> err_clk_ciu: >> if (!IS_ERR(host->ciu_clk)) >> @@ -2573,9 +2585,6 @@ void dw_mci_remove(struct dw_mci *host) >> if (host->use_dma && host->dma_ops->exit) >> host->dma_ops->exit(host); >> >> - if (host->vmmc) >> - regulator_disable(host->vmmc); >> - >> if (!IS_ERR(host->ciu_clk)) >> clk_disable_unprepare(host->ciu_clk); >> >> @@ -2592,9 +2601,6 @@ EXPORT_SYMBOL(dw_mci_remove); >> */ >> int dw_mci_suspend(struct dw_mci *host) >> { >> - if (host->vmmc) >> - regulator_disable(host->vmmc); >> - > > Just to check: have you tested suspend/resume with the various > combinations of "keep-power-in-suspend" and "non-removable" with your > patch. I remember some of the logic being a bit complicated... Tested suspend/resume with the non-removable.Will do more testing with combination of "keep-power-in-suspend" and "non-removable". > > >> return 0; >> } >> EXPORT_SYMBOL(dw_mci_suspend); >> @@ -2603,15 +2609,6 @@ int dw_mci_resume(struct dw_mci *host) >> { >> int i, ret; >> >> - if (host->vmmc) { >> - ret = regulator_enable(host->vmmc); >> - if (ret) { >> - dev_err(host->dev, >> - "failed to enable regulator: %d\n", ret); >> - return ret; >> - } >> - } >> - >> if (!dw_mci_ctrl_all_reset(host)) { >> ret = -ENODEV; >> return ret; >> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h >> index 738fa24..5c95d00 100644 >> --- a/drivers/mmc/host/dw_mmc.h >> +++ b/drivers/mmc/host/dw_mmc.h > > As I mentioned in my previous review, you should be removing "struct > regulator *vmmc; /* Power regulator */" from "struct dw_mci" since > you're no longer populating it. ok > > >> @@ -225,6 +225,8 @@ struct dw_mci_slot { >> unsigned long flags; >> #define DW_MMC_CARD_PRESENT 0 >> #define DW_MMC_CARD_NEED_INIT 1 >> +#define DW_MMC_CARD_POWERED 2 >> +#define DW_MMC_IO_POWERED 3 > > I don't really think you should have two bits here. From my > understanding of SD cards there should be very little reason to have > vqmmc and vmmc not powered at the same time. I think if i can use mmc_regulator_set_ocr(), we don't need additional flag.But for tps65090 mmc_regulator_get_ocr() and mmc_regulator_set_ocr() is failing as its a fixed-regulator. > > If vqmmc is powered but vmmc is not powered then you'll get leakage > and the card can pull power through the IO lines which is bad for the > card. > > I don't think that powering vmmc without vqmmc for a short time is > terribly bad but I can't quite see a good use case. Essentially > you're powering the card but not able to talk to it, right? I'm not > sure what the state of the IO lines would be (either driven low or > floating) since presumably the pullups on the lines are powered by > vqmmc too. > > >> 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
Yuvaraj, On Thu, Jun 26, 2014 at 4:21 AM, Yuvaraj Kumar <yuvaraj.cd@gmail.com> wrote: > Doug > > On Tue, Jun 24, 2014 at 11:30 PM, Doug Anderson <dianders@chromium.org> wrote: >> Yuvaraj, >> >> On Mon, Jun 23, 2014 at 3:45 AM, Yuvaraj Kumar C D <yuvaraj.cd@gmail.com> wrote: >>> This patch makes use of mmc_regulator_get_supply() to handle >>> the vmmc and vqmmc regulators.Also it moves the code handling >>> the these regulators to dw_mci_set_ios().It turned on the vmmc >>> and vqmmc during MMC_POWER_UP and MMC_POWER_ON,and turned off >>> during MMC_POWER_OFF. >>> >>> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.com> >>> --- >>> drivers/mmc/host/dw_mmc.c | 71 ++++++++++++++++++++++----------------------- >>> drivers/mmc/host/dw_mmc.h | 2 ++ >>> 2 files changed, 36 insertions(+), 37 deletions(-) >> >> Perhaps you could CC me on the whole series for the next version since >> I was involved in privately reviewing previous versions? > It was just accidental missing you in the CC .Surely i will add you in > CC for next versions. >> >> Overall caveat for my review is that I'm nowhere near an SD/MMC expert. >> >> >>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c >>> index 1ac227c..f5cabce 100644 >>> --- a/drivers/mmc/host/dw_mmc.c >>> +++ b/drivers/mmc/host/dw_mmc.c >>> @@ -937,6 +937,7 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) >>> struct dw_mci_slot *slot = mmc_priv(mmc); >>> const struct dw_mci_drv_data *drv_data = slot->host->drv_data; >>> u32 regs; >>> + int ret; >>> >>> switch (ios->bus_width) { >>> case MMC_BUS_WIDTH_4: >>> @@ -975,16 +976,41 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) >>> >>> switch (ios->power_mode) { >>> case MMC_POWER_UP: >>> + if ((!IS_ERR(mmc->supply.vmmc)) && >>> + !test_bit(DW_MMC_CARD_POWERED, &slot->flags)) { >>> + ret = regulator_enable(mmc->supply.vmmc); >>> + if (!ret) >>> + set_bit(DW_MMC_CARD_POWERED, &slot->flags); >>> + } >> >> As per below, I'm not sure why you'd want to turn on vqmmc and vmmc at >> different times. > As you can see people's have different opinion on this.When i had a > look at the other drivers in the subsystem which does in the same flow > as above.However i will change in the next version. Given my self proclaimed lack of SD/MMC knowledge, if others have a good reason for doing them separate then you should do it that way. So far I haven't heard that reason but I certainly could be wrong. >>> @@ -225,6 +225,8 @@ struct dw_mci_slot { >>> unsigned long flags; >>> #define DW_MMC_CARD_PRESENT 0 >>> #define DW_MMC_CARD_NEED_INIT 1 >>> +#define DW_MMC_CARD_POWERED 2 >>> +#define DW_MMC_IO_POWERED 3 >> >> I don't really think you should have two bits here. From my >> understanding of SD cards there should be very little reason to have >> vqmmc and vmmc not powered at the same time. > I think if i can use mmc_regulator_set_ocr(), we don't need additional > flag.But for tps65090 mmc_regulator_get_ocr() and > mmc_regulator_set_ocr() is failing as its a fixed-regulator. Can you explain more about what's failing? It sure looks like mmc core is supposed to handle this given comments below /* * If we're using a fixed/static regulator, don't call * regulator_set_voltage; it would fail. */ voltage = regulator_get_voltage(supply); if (!regulator_can_change_voltage(supply)) min_uV = max_uV = voltage; -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
Seungwon, On Thu, Jun 26, 2014 at 3:30 AM, Seungwon Jeon <tgih.jun@samsung.com> wrote: > Hi Doug, > > On Thu, June 26, 2014, Doug Anderson wrote: >> Seungwon, >> >> On Wed, Jun 25, 2014 at 4:18 AM, Seungwon Jeon <tgih.jun@samsung.com> wrote: >> >> >> + case MMC_POWER_ON: >> >> >> + if (!IS_ERR(mmc->supply.vqmmc) && >> >> >> + !test_bit(DW_MMC_IO_POWERED, &slot->flags)) { >> > You can use regulator_is_enabled() instead of flag bit, DW_MMC_IO_POWERED. >> >> I'd be a little worried about regulator_is_enabled() since regulators >> are reference counted. What if someone else is sharing this >> regulator? The regulator might happen to be enabled when you check it >> but unless you add your own dw_mmc reference count they might turn it >> off. > Cool, that's a possibility. Some assumption may need. > If mmc's core can guarantee its balance, I think we don't need to consider some flag. I notice that the mmc core seems to keep a flag itself for vdd (the mmc->regulator_enabled flag). That would imply that the core thought it was important to have the extra flag and that we should keep our own flag for vqmmc. -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, Jun 26, 2014 at 9:48 PM, Doug Anderson <dianders@chromium.org> wrote: > Yuvaraj, > > On Thu, Jun 26, 2014 at 4:21 AM, Yuvaraj Kumar <yuvaraj.cd@gmail.com> wrote: >> Doug >> >> On Tue, Jun 24, 2014 at 11:30 PM, Doug Anderson <dianders@chromium.org> wrote: >>> Yuvaraj, >>> >>> On Mon, Jun 23, 2014 at 3:45 AM, Yuvaraj Kumar C D <yuvaraj.cd@gmail.com> wrote: >>>> This patch makes use of mmc_regulator_get_supply() to handle >>>> the vmmc and vqmmc regulators.Also it moves the code handling >>>> the these regulators to dw_mci_set_ios().It turned on the vmmc >>>> and vqmmc during MMC_POWER_UP and MMC_POWER_ON,and turned off >>>> during MMC_POWER_OFF. >>>> >>>> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.com> >>>> --- >>>> drivers/mmc/host/dw_mmc.c | 71 ++++++++++++++++++++++----------------------- >>>> drivers/mmc/host/dw_mmc.h | 2 ++ >>>> 2 files changed, 36 insertions(+), 37 deletions(-) >>> >>> Perhaps you could CC me on the whole series for the next version since >>> I was involved in privately reviewing previous versions? >> It was just accidental missing you in the CC .Surely i will add you in >> CC for next versions. >>> >>> Overall caveat for my review is that I'm nowhere near an SD/MMC expert. >>> >>> >>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c >>>> index 1ac227c..f5cabce 100644 >>>> --- a/drivers/mmc/host/dw_mmc.c >>>> +++ b/drivers/mmc/host/dw_mmc.c >>>> @@ -937,6 +937,7 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) >>>> struct dw_mci_slot *slot = mmc_priv(mmc); >>>> const struct dw_mci_drv_data *drv_data = slot->host->drv_data; >>>> u32 regs; >>>> + int ret; >>>> >>>> switch (ios->bus_width) { >>>> case MMC_BUS_WIDTH_4: >>>> @@ -975,16 +976,41 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) >>>> >>>> switch (ios->power_mode) { >>>> case MMC_POWER_UP: >>>> + if ((!IS_ERR(mmc->supply.vmmc)) && >>>> + !test_bit(DW_MMC_CARD_POWERED, &slot->flags)) { >>>> + ret = regulator_enable(mmc->supply.vmmc); >>>> + if (!ret) >>>> + set_bit(DW_MMC_CARD_POWERED, &slot->flags); >>>> + } >>> >>> As per below, I'm not sure why you'd want to turn on vqmmc and vmmc at >>> different times. >> As you can see people's have different opinion on this.When i had a >> look at the other drivers in the subsystem which does in the same flow >> as above.However i will change in the next version. > > Given my self proclaimed lack of SD/MMC knowledge, if others have a > good reason for doing them separate then you should do it that way. > So far I haven't heard that reason but I certainly could be wrong. > > >>>> @@ -225,6 +225,8 @@ struct dw_mci_slot { >>>> unsigned long flags; >>>> #define DW_MMC_CARD_PRESENT 0 >>>> #define DW_MMC_CARD_NEED_INIT 1 >>>> +#define DW_MMC_CARD_POWERED 2 >>>> +#define DW_MMC_IO_POWERED 3 >>> >>> I don't really think you should have two bits here. From my >>> understanding of SD cards there should be very little reason to have >>> vqmmc and vmmc not powered at the same time. >> I think if i can use mmc_regulator_set_ocr(), we don't need additional >> flag.But for tps65090 mmc_regulator_get_ocr() and >> mmc_regulator_set_ocr() is failing as its a fixed-regulator. > > Can you explain more about what's failing? It sure looks like mmc > core is supposed to handle this given comments below > > /* > * If we're using a fixed/static regulator, don't call > * regulator_set_voltage; it would fail. > */ tps65090 driver does not register through fixed regulator framework.It uses normal regulator framework and supports only enable/disable/is_enabled callbacks.Also it lacks certain callbacks for list_voltage, get_voltage ,set_voltage etc. [ 2.306476] dwmmc_exynos 12220000.mmc: Failed getting OCR mask: -22 [ 2.393403] dwmmc_exynos 12220000.mmc: could not set regulator OCR (-22) For the above reason,regulator framework treats fet4 as unused regulator and disables the vmmc regulator. > voltage = regulator_get_voltage(supply); > > if (!regulator_can_change_voltage(supply)) > min_uV = max_uV = voltage; > > > -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 Fri, June 27, 2014, Doug Anderson wrote: > Seungwon, > > On Thu, Jun 26, 2014 at 3:30 AM, Seungwon Jeon <tgih.jun@samsung.com> wrote: > > Hi Doug, > > > > On Thu, June 26, 2014, Doug Anderson wrote: > >> Seungwon, > >> > >> On Wed, Jun 25, 2014 at 4:18 AM, Seungwon Jeon <tgih.jun@samsung.com> wrote: > >> >> >> + case MMC_POWER_ON: > >> >> >> + if (!IS_ERR(mmc->supply.vqmmc) && > >> >> >> + !test_bit(DW_MMC_IO_POWERED, &slot->flags)) { > >> > You can use regulator_is_enabled() instead of flag bit, DW_MMC_IO_POWERED. > >> > >> I'd be a little worried about regulator_is_enabled() since regulators > >> are reference counted. What if someone else is sharing this > >> regulator? The regulator might happen to be enabled when you check it > >> but unless you add your own dw_mmc reference count they might turn it > >> off. > > Cool, that's a possibility. Some assumption may need. > > If mmc's core can guarantee its balance, I think we don't need to consider some flag. > > I notice that the mmc core seems to keep a flag itself for vdd (the > mmc->regulator_enabled flag). That would imply that the core thought > it was important to have the extra flag and that we should keep our > own flag for vqmmc. Ok, Good. Thanks, Seungwon Jeon > > -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
Yuvaraj, On Fri, Jun 27, 2014 at 3:59 AM, Yuvaraj Kumar <yuvaraj.cd@gmail.com> wrote: >>> mmc_regulator_set_ocr() is failing as its a fixed-regulator. >> >> Can you explain more about what's failing? It sure looks like mmc >> core is supposed to handle this given comments below >> >> /* >> * If we're using a fixed/static regulator, don't call >> * regulator_set_voltage; it would fail. >> */ > tps65090 driver does not register through fixed regulator framework.It > uses normal regulator framework and supports only > enable/disable/is_enabled callbacks.Also it lacks certain callbacks > for list_voltage, get_voltage ,set_voltage etc. > [ 2.306476] dwmmc_exynos 12220000.mmc: Failed getting OCR mask: -22 > [ 2.393403] dwmmc_exynos 12220000.mmc: could not set regulator OCR (-22) > For the above reason,regulator framework treats fet4 as unused > regulator and disables the vmmc regulator. Ah. Perhaps tps65090 needs to be fixed then... I'm not sure any other drivers cared before so maybe that's why it was never caught? -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 06/27/2014 01:18 AM, Doug Anderson wrote: > Yuvaraj, > > On Thu, Jun 26, 2014 at 4:21 AM, Yuvaraj Kumar <yuvaraj.cd@gmail.com> wrote: >> Doug >> >> On Tue, Jun 24, 2014 at 11:30 PM, Doug Anderson <dianders@chromium.org> wrote: >>> Yuvaraj, >>> >>> On Mon, Jun 23, 2014 at 3:45 AM, Yuvaraj Kumar C D <yuvaraj.cd@gmail.com> wrote: >>>> This patch makes use of mmc_regulator_get_supply() to handle >>>> the vmmc and vqmmc regulators.Also it moves the code handling >>>> the these regulators to dw_mci_set_ios().It turned on the vmmc >>>> and vqmmc during MMC_POWER_UP and MMC_POWER_ON,and turned off >>>> during MMC_POWER_OFF. >>>> >>>> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.com> >>>> --- >>>> drivers/mmc/host/dw_mmc.c | 71 ++++++++++++++++++++++----------------------- >>>> drivers/mmc/host/dw_mmc.h | 2 ++ >>>> 2 files changed, 36 insertions(+), 37 deletions(-) >>> >>> Perhaps you could CC me on the whole series for the next version since >>> I was involved in privately reviewing previous versions? >> It was just accidental missing you in the CC .Surely i will add you in >> CC for next versions. >>> >>> Overall caveat for my review is that I'm nowhere near an SD/MMC expert. >>> >>> >>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c >>>> index 1ac227c..f5cabce 100644 >>>> --- a/drivers/mmc/host/dw_mmc.c >>>> +++ b/drivers/mmc/host/dw_mmc.c >>>> @@ -937,6 +937,7 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) >>>> struct dw_mci_slot *slot = mmc_priv(mmc); >>>> const struct dw_mci_drv_data *drv_data = slot->host->drv_data; >>>> u32 regs; >>>> + int ret; >>>> >>>> switch (ios->bus_width) { >>>> case MMC_BUS_WIDTH_4: >>>> @@ -975,16 +976,41 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) >>>> >>>> switch (ios->power_mode) { >>>> case MMC_POWER_UP: >>>> + if ((!IS_ERR(mmc->supply.vmmc)) && >>>> + !test_bit(DW_MMC_CARD_POWERED, &slot->flags)) { >>>> + ret = regulator_enable(mmc->supply.vmmc); >>>> + if (!ret) >>>> + set_bit(DW_MMC_CARD_POWERED, &slot->flags); >>>> + } >>> >>> As per below, I'm not sure why you'd want to turn on vqmmc and vmmc at >>> different times. >> As you can see people's have different opinion on this.When i had a >> look at the other drivers in the subsystem which does in the same flow >> as above.However i will change in the next version. > > Given my self proclaimed lack of SD/MMC knowledge, if others have a > good reason for doing them separate then you should do it that way. > So far I haven't heard that reason but I certainly could be wrong. At first time, i had believed nothing problem that it turns on vmmc and vqmmc at different time. It could have the potential problem. Best Regards, Jaehoon Chung > > >>>> @@ -225,6 +225,8 @@ struct dw_mci_slot { >>>> unsigned long flags; >>>> #define DW_MMC_CARD_PRESENT 0 >>>> #define DW_MMC_CARD_NEED_INIT 1 >>>> +#define DW_MMC_CARD_POWERED 2 >>>> +#define DW_MMC_IO_POWERED 3 >>> >>> I don't really think you should have two bits here. From my >>> understanding of SD cards there should be very little reason to have >>> vqmmc and vmmc not powered at the same time. >> I think if i can use mmc_regulator_set_ocr(), we don't need additional >> flag.But for tps65090 mmc_regulator_get_ocr() and >> mmc_regulator_set_ocr() is failing as its a fixed-regulator. > > Can you explain more about what's failing? It sure looks like mmc > core is supposed to handle this given comments below > > /* > * If we're using a fixed/static regulator, don't call > * regulator_set_voltage; it would fail. > */ > voltage = regulator_get_voltage(supply); > > if (!regulator_can_change_voltage(supply)) > min_uV = max_uV = voltage; > > > -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 Mon, Jun 30, 2014 at 5:13 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote: > On 06/27/2014 01:18 AM, Doug Anderson wrote: >> Yuvaraj, >> >> On Thu, Jun 26, 2014 at 4:21 AM, Yuvaraj Kumar <yuvaraj.cd@gmail.com> wrote: >>> Doug >>> >>> On Tue, Jun 24, 2014 at 11:30 PM, Doug Anderson <dianders@chromium.org> wrote: >>>> Yuvaraj, >>>> >>>> On Mon, Jun 23, 2014 at 3:45 AM, Yuvaraj Kumar C D <yuvaraj.cd@gmail.com> wrote: >>>>> This patch makes use of mmc_regulator_get_supply() to handle >>>>> the vmmc and vqmmc regulators.Also it moves the code handling >>>>> the these regulators to dw_mci_set_ios().It turned on the vmmc >>>>> and vqmmc during MMC_POWER_UP and MMC_POWER_ON,and turned off >>>>> during MMC_POWER_OFF. >>>>> >>>>> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.com> >>>>> --- >>>>> drivers/mmc/host/dw_mmc.c | 71 ++++++++++++++++++++++----------------------- >>>>> drivers/mmc/host/dw_mmc.h | 2 ++ >>>>> 2 files changed, 36 insertions(+), 37 deletions(-) >>>> >>>> Perhaps you could CC me on the whole series for the next version since >>>> I was involved in privately reviewing previous versions? >>> It was just accidental missing you in the CC .Surely i will add you in >>> CC for next versions. >>>> >>>> Overall caveat for my review is that I'm nowhere near an SD/MMC expert. >>>> >>>> >>>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c >>>>> index 1ac227c..f5cabce 100644 >>>>> --- a/drivers/mmc/host/dw_mmc.c >>>>> +++ b/drivers/mmc/host/dw_mmc.c >>>>> @@ -937,6 +937,7 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) >>>>> struct dw_mci_slot *slot = mmc_priv(mmc); >>>>> const struct dw_mci_drv_data *drv_data = slot->host->drv_data; >>>>> u32 regs; >>>>> + int ret; >>>>> >>>>> switch (ios->bus_width) { >>>>> case MMC_BUS_WIDTH_4: >>>>> @@ -975,16 +976,41 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) >>>>> >>>>> switch (ios->power_mode) { >>>>> case MMC_POWER_UP: >>>>> + if ((!IS_ERR(mmc->supply.vmmc)) && >>>>> + !test_bit(DW_MMC_CARD_POWERED, &slot->flags)) { >>>>> + ret = regulator_enable(mmc->supply.vmmc); >>>>> + if (!ret) >>>>> + set_bit(DW_MMC_CARD_POWERED, &slot->flags); >>>>> + } >>>> >>>> As per below, I'm not sure why you'd want to turn on vqmmc and vmmc at >>>> different times. >>> As you can see people's have different opinion on this.When i had a >>> look at the other drivers in the subsystem which does in the same flow >>> as above.However i will change in the next version. >> >> Given my self proclaimed lack of SD/MMC knowledge, if others have a >> good reason for doing them separate then you should do it that way. >> So far I haven't heard that reason but I certainly could be wrong. > > At first time, i had believed nothing problem that it turns on vmmc and vqmmc at different time. > It could have the potential problem. OK. As I said I'm nowhere near an expert on SD/MMC, so if there's something out there that says that turning vmmc on before vqmmc is the right thing to do then I guess that's the answer. I would still be very curious to get more details on what the potential problem is. Could you provide a reference to documentation that says vmmc should be on before vqmmc or describe a situation where it's important? Thanks! -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 07/01/2014 12:06 AM, Doug Anderson wrote: > Jaehoon, > > On Mon, Jun 30, 2014 at 5:13 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote: >> On 06/27/2014 01:18 AM, Doug Anderson wrote: >>> Yuvaraj, >>> >>> On Thu, Jun 26, 2014 at 4:21 AM, Yuvaraj Kumar <yuvaraj.cd@gmail.com> wrote: >>>> Doug >>>> >>>> On Tue, Jun 24, 2014 at 11:30 PM, Doug Anderson <dianders@chromium.org> wrote: >>>>> Yuvaraj, >>>>> >>>>> On Mon, Jun 23, 2014 at 3:45 AM, Yuvaraj Kumar C D <yuvaraj.cd@gmail.com> wrote: >>>>>> This patch makes use of mmc_regulator_get_supply() to handle >>>>>> the vmmc and vqmmc regulators.Also it moves the code handling >>>>>> the these regulators to dw_mci_set_ios().It turned on the vmmc >>>>>> and vqmmc during MMC_POWER_UP and MMC_POWER_ON,and turned off >>>>>> during MMC_POWER_OFF. >>>>>> >>>>>> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.com> >>>>>> --- >>>>>> drivers/mmc/host/dw_mmc.c | 71 ++++++++++++++++++++++----------------------- >>>>>> drivers/mmc/host/dw_mmc.h | 2 ++ >>>>>> 2 files changed, 36 insertions(+), 37 deletions(-) >>>>> >>>>> Perhaps you could CC me on the whole series for the next version since >>>>> I was involved in privately reviewing previous versions? >>>> It was just accidental missing you in the CC .Surely i will add you in >>>> CC for next versions. >>>>> >>>>> Overall caveat for my review is that I'm nowhere near an SD/MMC expert. >>>>> >>>>> >>>>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c >>>>>> index 1ac227c..f5cabce 100644 >>>>>> --- a/drivers/mmc/host/dw_mmc.c >>>>>> +++ b/drivers/mmc/host/dw_mmc.c >>>>>> @@ -937,6 +937,7 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) >>>>>> struct dw_mci_slot *slot = mmc_priv(mmc); >>>>>> const struct dw_mci_drv_data *drv_data = slot->host->drv_data; >>>>>> u32 regs; >>>>>> + int ret; >>>>>> >>>>>> switch (ios->bus_width) { >>>>>> case MMC_BUS_WIDTH_4: >>>>>> @@ -975,16 +976,41 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) >>>>>> >>>>>> switch (ios->power_mode) { >>>>>> case MMC_POWER_UP: >>>>>> + if ((!IS_ERR(mmc->supply.vmmc)) && >>>>>> + !test_bit(DW_MMC_CARD_POWERED, &slot->flags)) { >>>>>> + ret = regulator_enable(mmc->supply.vmmc); >>>>>> + if (!ret) >>>>>> + set_bit(DW_MMC_CARD_POWERED, &slot->flags); >>>>>> + } >>>>> >>>>> As per below, I'm not sure why you'd want to turn on vqmmc and vmmc at >>>>> different times. >>>> As you can see people's have different opinion on this.When i had a >>>> look at the other drivers in the subsystem which does in the same flow >>>> as above.However i will change in the next version. >>> >>> Given my self proclaimed lack of SD/MMC knowledge, if others have a >>> good reason for doing them separate then you should do it that way. >>> So far I haven't heard that reason but I certainly could be wrong. >> >> At first time, i had believed nothing problem that it turns on vmmc and vqmmc at different time. >> It could have the potential problem. > > OK. As I said I'm nowhere near an expert on SD/MMC, so if there's > something out there that says that turning vmmc on before vqmmc is the > right thing to do then I guess that's the answer. I would still be > very curious to get more details on what the potential problem is. > Could you provide a reference to documentation that says vmmc should > be on before vqmmc or describe a situation where it's important? Actually i didn't have any documentation. According to eMMC spec, vmmc and vqmmc should be used with same power supply. In this case, it will turn on vmmc and vqmmc at same time. If vqmmc is used with the I/O power supply, the sequence isn't important, but vmmc and vqmmc need to wait until stabling the power. I didn't know Potential problem is what problem. (Potential problem is mentioned from our H/W team member. H/W mis-operating?) I didn't have the expert H/W knowledge. Thanks! Bset Regards, Jaehoon Chung > > Thanks! > > -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
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c index 1ac227c..f5cabce 100644 --- a/drivers/mmc/host/dw_mmc.c +++ b/drivers/mmc/host/dw_mmc.c @@ -937,6 +937,7 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) struct dw_mci_slot *slot = mmc_priv(mmc); const struct dw_mci_drv_data *drv_data = slot->host->drv_data; u32 regs; + int ret; switch (ios->bus_width) { case MMC_BUS_WIDTH_4: @@ -975,16 +976,41 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) switch (ios->power_mode) { case MMC_POWER_UP: + if ((!IS_ERR(mmc->supply.vmmc)) && + !test_bit(DW_MMC_CARD_POWERED, &slot->flags)) { + ret = regulator_enable(mmc->supply.vmmc); + if (!ret) + set_bit(DW_MMC_CARD_POWERED, &slot->flags); + } set_bit(DW_MMC_CARD_NEED_INIT, &slot->flags); regs = mci_readl(slot->host, PWREN); regs |= (1 << slot->id); mci_writel(slot->host, PWREN, regs); break; case MMC_POWER_OFF: + if (!IS_ERR(mmc->supply.vqmmc) && + test_bit(DW_MMC_IO_POWERED, &slot->flags)) { + ret = regulator_disable(mmc->supply.vqmmc); + if (!ret) + clear_bit(DW_MMC_IO_POWERED, &slot->flags); + } + if (!IS_ERR(mmc->supply.vmmc) && + test_bit(DW_MMC_CARD_POWERED, &slot->flags)) { + ret = regulator_disable(mmc->supply.vmmc); + if (!ret) + clear_bit(DW_MMC_CARD_POWERED, &slot->flags); + } regs = mci_readl(slot->host, PWREN); regs &= ~(1 << slot->id); mci_writel(slot->host, PWREN, regs); break; + case MMC_POWER_ON: + if (!IS_ERR(mmc->supply.vqmmc) && + !test_bit(DW_MMC_IO_POWERED, &slot->flags)) { + ret = regulator_enable(mmc->supply.vqmmc); + if (!ret) + set_bit(DW_MMC_IO_POWERED, &slot->flags); + } default: break; } @@ -2067,7 +2093,13 @@ static int dw_mci_init_slot(struct dw_mci *host, unsigned int id) mmc->f_max = freq[1]; } - mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34; + /*if there are external regulators, get them*/ + ret = mmc_regulator_get_supply(mmc); + if (ret == -EPROBE_DEFER) + goto err_setup_bus; + + if (!mmc->ocr_avail) + mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34; if (host->pdata->caps) mmc->caps = host->pdata->caps; @@ -2133,7 +2165,7 @@ static int dw_mci_init_slot(struct dw_mci *host, unsigned int id) err_setup_bus: mmc_free_host(mmc); - return -EINVAL; + return ret; } static void dw_mci_cleanup_slot(struct dw_mci_slot *slot, unsigned int id) @@ -2375,24 +2407,6 @@ int dw_mci_probe(struct dw_mci *host) } } - host->vmmc = devm_regulator_get_optional(host->dev, "vmmc"); - if (IS_ERR(host->vmmc)) { - ret = PTR_ERR(host->vmmc); - if (ret == -EPROBE_DEFER) - goto err_clk_ciu; - - dev_info(host->dev, "no vmmc regulator found: %d\n", ret); - host->vmmc = NULL; - } else { - ret = regulator_enable(host->vmmc); - if (ret) { - if (ret != -EPROBE_DEFER) - dev_err(host->dev, - "regulator_enable fail: %d\n", ret); - goto err_clk_ciu; - } - } - host->quirks = host->pdata->quirks; spin_lock_init(&host->lock); @@ -2536,8 +2550,6 @@ err_workqueue: err_dmaunmap: if (host->use_dma && host->dma_ops->exit) host->dma_ops->exit(host); - if (host->vmmc) - regulator_disable(host->vmmc); err_clk_ciu: if (!IS_ERR(host->ciu_clk)) @@ -2573,9 +2585,6 @@ void dw_mci_remove(struct dw_mci *host) if (host->use_dma && host->dma_ops->exit) host->dma_ops->exit(host); - if (host->vmmc) - regulator_disable(host->vmmc); - if (!IS_ERR(host->ciu_clk)) clk_disable_unprepare(host->ciu_clk); @@ -2592,9 +2601,6 @@ EXPORT_SYMBOL(dw_mci_remove); */ int dw_mci_suspend(struct dw_mci *host) { - if (host->vmmc) - regulator_disable(host->vmmc); - return 0; } EXPORT_SYMBOL(dw_mci_suspend); @@ -2603,15 +2609,6 @@ int dw_mci_resume(struct dw_mci *host) { int i, ret; - if (host->vmmc) { - ret = regulator_enable(host->vmmc); - if (ret) { - dev_err(host->dev, - "failed to enable regulator: %d\n", ret); - return ret; - } - } - if (!dw_mci_ctrl_all_reset(host)) { ret = -ENODEV; return ret; diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h index 738fa24..5c95d00 100644 --- a/drivers/mmc/host/dw_mmc.h +++ b/drivers/mmc/host/dw_mmc.h @@ -225,6 +225,8 @@ struct dw_mci_slot { unsigned long flags; #define DW_MMC_CARD_PRESENT 0 #define DW_MMC_CARD_NEED_INIT 1 +#define DW_MMC_CARD_POWERED 2 +#define DW_MMC_IO_POWERED 3 int id; int last_detect_state; };
This patch makes use of mmc_regulator_get_supply() to handle the vmmc and vqmmc regulators.Also it moves the code handling the these regulators to dw_mci_set_ios().It turned on the vmmc and vqmmc during MMC_POWER_UP and MMC_POWER_ON,and turned off during MMC_POWER_OFF. Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.com> --- drivers/mmc/host/dw_mmc.c | 71 ++++++++++++++++++++++----------------------- drivers/mmc/host/dw_mmc.h | 2 ++ 2 files changed, 36 insertions(+), 37 deletions(-)