diff mbox

[1/3] mmc: dw_mmc: use mmc_regulator_get_supply to handle regulators

Message ID 1403520321-2431-2-git-send-email-yuvaraj.cd@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Yuvaraj CD June 23, 2014, 10:45 a.m. UTC
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(-)

Comments

Douglas Anderson June 24, 2014, 6 p.m. UTC | #1
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-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jaehoon Chung June 25, 2014, 3:18 a.m. UTC | #2
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-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Douglas Anderson June 25, 2014, 4 a.m. UTC | #3
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-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Seungwon Jeon June 25, 2014, 11:18 a.m. UTC | #4
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-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Douglas Anderson June 25, 2014, 5:28 p.m. UTC | #5
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-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Seungwon Jeon June 26, 2014, 10:30 a.m. UTC | #6
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-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yuvaraj CD June 26, 2014, 11:21 a.m. UTC | #7
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-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Douglas Anderson June 26, 2014, 4:18 p.m. UTC | #8
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-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Douglas Anderson June 26, 2014, 4:20 p.m. UTC | #9
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-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yuvaraj CD June 27, 2014, 10:59 a.m. UTC | #10
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-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Seungwon Jeon June 27, 2014, 11:19 a.m. UTC | #11
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-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Douglas Anderson June 27, 2014, 10:47 p.m. UTC | #12
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-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jaehoon Chung June 30, 2014, 12:13 p.m. UTC | #13
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-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Douglas Anderson June 30, 2014, 3:06 p.m. UTC | #14
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-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jaehoon Chung July 1, 2014, 4:25 a.m. UTC | #15
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-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 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;
 };