Re: [RFC] mmc: core: add the capability for broken voltage

[Kyungmin Park <kmpark@infradead.org>]

On 1/19/12, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 18/01/12 15:03, Kyungmin Park wrote:
>> On Wed, Jan 18, 2012 at 5:56 PM, Adrian Hunter <adrian.hunter@intel.com>
>> wrote:
>>> On 18/01/12 10:13, Kyungmin Park wrote:
>>>> On 1/18/12, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>> On 18/01/12 01:58, Kyungmin Park wrote:
>>>>>> On 1/17/12, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>>>> On 17/01/12 11:53, Kyungmin Park wrote:
>>>>>>>> On 1/17/12, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>>>>>> On 17/01/12 11:05, Kyungmin Park wrote:
>>>>>>>>>> On 1/17/12, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>>>>>>>> On 16/01/12 10:49, Jaehoon Chung wrote:
>>>>>>>>>>>> This patch is added the MMC_CAP2_BROKEN_VOLTAGE.
>>>>>>>>>>>>
>>>>>>>>>>>> if the voltage didn't satisfy between min_uV and max_uV,
>>>>>>>>>>>
>>>>>>>>>>> Why is the fixed voltage not in the acceptable range for the
>>>>>>>>>>> card?
>>>>>>>>>>> Doesn't that risk breaking the card?
>>>>>>>>>> Hi Adrian,
>>>>>>>>>>
>>>>>>>>>> I don't know, they uses the not supported voltage. but it's worked
>>>>>>>>>> properly for long time.
>>>>>>>>>
>>>>>>>>> I wonder if there is a generic problem here that this fix hides.
>>>>>>>>> It would be nice to have an explanation.  Do you know:
>>>>>>>>>  - what is the fixed voltage?
>>>>>>>> Now 2.8V is supplied by gpio so make it fixed regulator
>>>>>>>>>  - is it supplying VCC or VCCQ?
>>>>>>>> VDD in our schematic.
>>>>>>>
>>>>>>> Is it the NAND core voltage or the I/O voltage?
>>>>>> In case of eMMC v4.41, it uses the same voltage, VDD is used for MMC
>>>>>> I/O block and controller and another VDDF is used for flash.
>>>>>
>>>>> It seems to me you just need to override the value of
>>>>> mmc->ocr_avail_mmc
>>>>> calculated in sdhci_add_host() in sdhci.c to reflect the fact that the
>>>>> only
>>>>> voltage available is 2.8V.
>>>>
>>>> current sdhci ocr_avail is 0x300080 and wanted ocr is 0x10000 so it
>>>> doesn't support it.
>>>>
>>>> [    0.984754] sdhci_add_host[2888] ocr_avail 0x300080
>>>> [    0.989909] sdhci_add_host[2889] host->ocr_avail_mmc 0x10000
>>>
>>> Yes because sdhci_add_host() ANDs the values i.e.
>>>
>>>        if (host->ocr_avail_mmc)
>>>                mmc->ocr_avail_mmc &= host->ocr_avail_mmc;
>> Right, so it's meaningless set the host->ocr_avail_mmc. basically
>> sdhci doesn't support this voltage.
>
> That is not entirely true.  SD and MMC define voltage thresholds in such a
> way that a 2.7V card is within the thresholds of a 3.0V host controller.
> And a 3.6V card is within the thresholds of a 3.3V host controller.
>
> I would do something like below.  If you agree, I will send a patch.
Now it's has only (caps[0] & SDHCI_CAN_VDD_330) and don't have SDHCI_CAN_VDD_300
it doesn't support the 2.8V.

sdhci_add_host[2917] ocr_avail_mmc 0xf80080.

Thank you,
Kyungmin Park

Note that. maybe MAX_VOLTAGE instead of LAX_VOLTAGE.
>
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 8d66706..a7cb3f2 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1179,20 +1179,43 @@ static int sdhci_set_power(struct sdhci_host *host,
> unsigned short power)
>         u8 pwr = 0;
>
>         if (power != (unsigned short)-1) {
> -               switch (1 << power) {
> -               case MMC_VDD_165_195:
> -                       pwr = SDHCI_POWER_180;
> -                       break;
> -               case MMC_VDD_29_30:
> -               case MMC_VDD_30_31:
> -                       pwr = SDHCI_POWER_300;
> -                       break;
> -               case MMC_VDD_32_33:
> -               case MMC_VDD_33_34:
> -                       pwr = SDHCI_POWER_330;
> -                       break;
> -               default:
> -                       BUG();
> +               if (host->quirks2 & SDHCI_QUIRK2_LAX_VOLTAGE) {
> +                       switch (1 << power) {
> +                       case MMC_VDD_165_195:
> +                               pwr = SDHCI_POWER_180;
> +                               break;
> +                       case MMC_VDD_27_28:
> +                       case MMC_VDD_28_29:
> +                       case MMC_VDD_29_30:
> +                       case MMC_VDD_30_31:
> +                               pwr = SDHCI_POWER_300;
> +                               break;
> +                       case MMC_VDD_31_32:
> +                       case MMC_VDD_32_33:
> +                       case MMC_VDD_33_34:
> +                       case MMC_VDD_34_35:
> +                       case MMC_VDD_35_36:
> +                               pwr = SDHCI_POWER_330;
> +                               break;
> +                       default:
> +                               BUG();
> +                       }
> +               } else {
> +                       switch (1 << power) {
> +                       case MMC_VDD_165_195:
> +                               pwr = SDHCI_POWER_180;
> +                               break;
> +                       case MMC_VDD_29_30:
> +                       case MMC_VDD_30_31:
> +                               pwr = SDHCI_POWER_300;
> +                               break;
> +                       case MMC_VDD_32_33:
> +                       case MMC_VDD_33_34:
> +                               pwr = SDHCI_POWER_330;
> +                               break;
> +                       default:
> +                               BUG();
> +                       }
>                 }
>         }
>
> @@ -2829,6 +2852,9 @@ int sdhci_add_host(struct sdhci_host *host)
>                 int max_current_330;
>
>                 ocr_avail |= MMC_VDD_32_33 | MMC_VDD_33_34;
> +               if (host->quirks2 & SDHCI_QUIRK2_LAX_VOLTAGE)
> +                       ocr_avail |= MMC_VDD_31_32 | MMC_VDD_34_35 |
> +                                    MMC_VDD_35_36;
>
>                 max_current_330 = ((max_current_caps &
>                                    SDHCI_MAX_CURRENT_330_MASK) >>
> @@ -2842,6 +2868,8 @@ int sdhci_add_host(struct sdhci_host *host)
>                 int max_current_300;
>
>                 ocr_avail |= MMC_VDD_29_30 | MMC_VDD_30_31;
> +               if (host->quirks2 & SDHCI_QUIRK2_LAX_VOLTAGE)
> +                       ocr_avail |= MMC_VDD_27_28 | MMC_VDD_28_29;
>
>                 max_current_300 = ((max_current_caps &
>                                    SDHCI_MAX_CURRENT_300_MASK) >>
> diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
> index c750f85..a03d613 100644
> --- a/include/linux/mmc/sdhci.h
> +++ b/include/linux/mmc/sdhci.h
> @@ -90,6 +90,9 @@ struct sdhci_host {
>
>         unsigned int quirks2;   /* More deviations from spec. */
>
> +/* Allow a wider range of voltages */
> +#define SDHCI_QUIRK2_LAX_VOLTAGE                       (1<<0)
> +
>         int irq;                /* Device IRQ */
>         void __iomem *ioaddr;   /* Mapped address */
>
>
>
>
>>>
>>> You will have to add a new quirk or callback e.g.
>> That's reason add new quirk. MMC_CAP2_BROKEN_VOLTAGE.
>>>
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index 8d66706..b8a04e3 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -2980,6 +2980,12 @@ int sdhci_add_host(struct sdhci_host *host)
>>>                host->vmmc = NULL;
>>>        }
>>>
>>> +       if (host->ops->fixup_host) {
>>> +               ret = host->ops->fixup_host(host);
>>> +               if (ret)
>>> +                       goto regput;
>>> +       }
>>> +
>>>        sdhci_init(host, 0);
>>>
>>>  #ifdef CONFIG_MMC_DEBUG
>>> @@ -3012,6 +3018,9 @@ int sdhci_add_host(struct sdhci_host *host)
>>>
>>>        return 0;
>>>
>>> +regput:
>>> +       if (host->vmmc)
>>> +               regulator_put(host->vmmc);
>>>  #ifdef SDHCI_USE_LEDS_CLASS
>>>  reset:
>>>        sdhci_reset(host, SDHCI_RESET_ALL);
>>>
>>>
>>> Then implement ->fixup_host() to make the change.
>>>
>>> Note that this is for I/O voltage or single voltage supply.  If there is
>>> a
>>> need to control 2 separate voltages (e.g. VDD, VDDF or VCCQ, VCC as JEDEC
>>> calls them) it is more challenging because mmc core does not support it.
>> New H/W has different voltage for eMMC v4.5. but it's covered by PMIC.
>> It can be switches on/off by gpio then PMIC turn on/off each LDOs for
>> eMMC properly.
>>
>> Thank you,
>> Kyungmin Park
>>>
>>>>
>>>> Thank you,
>>>> Kyungmin Park
>>>>>
>>>>>>>
>>>>>>>>>  - what is the contents of the OCR register?
>>>>>>>> In sdhci_set_power(). it returns SDHCI_POWER_180, SDHCI_POWER_300,
>>>>>>>> and
>>>>>>>> SDHCI_POWER_330.
>>>>>>>> In probe time, it return the SDHCI_POWER_330, and but fixed
>>>>>>>> regulator
>>>>>>>> returns the 2.8V so it calls regulator_set_voltage. and return
>>>>>>>> -EINVAL
>>>>>>>> since it's fixed regulator.
>>>>>>>>
>>>>>>>> Of course we can make workaround, set fixed regulator voltage as
>>>>>>>> 3.3V.
>>>>>>>> Even though actual voltage is 2.8V. so it's ambiguous.
>>>>>>>>
>>>>>>>> that's reason introduces the new quirk.
>>>>>>>>
>>>>>>>> Thank you,
>>>>>>>> Kyungmin Park
>>>>>>>>>
>>>>>>>>>> Galaxy S2 also uses the same voltage as ours.
>>>>>>>>>>
>>>>>>>>>> I also think the modify the regulator framework to support voltage
>>>>>>>>>> change at fixed regulator. but it's not good idea and doesn't
>>>>>>>>>> match
>>>>>>>>>> the regulator concept. so modify the sdhci codes to support our
>>>>>>>>>> boards.
>>>>>>>>>>
>>>>>>>>>> Thank you,
>>>>>>>>>> Kyungmin Park
>>>>>>>>>>>
>>>>>>>>>>>> try to change the voltage in core.c.
>>>>>>>>>>>> When change the voltage, maybe use the regulator_set_voltage().
>>>>>>>>>>>>
>>>>>>>>>>>> In regulator_set_voltage(), check the below condition.
>>>>>>>>>>>>
>>>>>>>>>>>>       /* sanity check */
>>>>>>>>>>>>       if (!rdev->desc->ops->set_voltage &&
>>>>>>>>>>>>           !rdev->desc->ops->set_voltage_sel) {
>>>>>>>>>>>>               ret = -EINVAL;
>>>>>>>>>>>>               goto out;
>>>>>>>>>>>>       }
>>>>>>>>>>>>
>>>>>>>>>>>> If Some-board should use the fixed-regulator, always return
>>>>>>>>>>>> -EINVAL.
>>>>>>>>>>>> Then, eMMC didn't initialize always.
>>>>>>>>>>>>
>>>>>>>>>>>> So if use the fixed-regulator or etc, we need to add the
>>>>>>>>>>>> MMC_CAP2_BROKEN_VOLTAGE.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
>>>>>>>>>>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>>>>>>>>>>>> ---
>>>>>>>>>>>>  drivers/mmc/core/core.c  |    4 ++++
>>>>>>>>>>>>  include/linux/mmc/host.h |    1 +
>>>>>>>>>>>>  2 files changed, 5 insertions(+), 0 deletions(-)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>>>>>>>>>>> index bec0bf2..6848789 100644
>>>>>>>>>>>> --- a/drivers/mmc/core/core.c
>>>>>>>>>>>> +++ b/drivers/mmc/core/core.c
>>>>>>>>>>>> @@ -1121,6 +1121,10 @@ int mmc_regulator_set_ocr(struct mmc_host
>>>>>>>>>>>> *mmc,
>>>>>>>>>>>>                * might not allow this operation
>>>>>>>>>>>>                */
>>>>>>>>>>>>               voltage = regulator_get_voltage(supply);
>>>>>>>>>>>> +
>>>>>>>>>>>> +             if (mmc->caps2 & MMC_CAP2_BROKEN_VOLTAGE)
>>>>>>>>>>>> +                     min_uV = max_uV = voltage;
>>>>>>>>>>>> +
>>>>>>>>>>>>               if (voltage < 0)
>>>>>>>>>>>>                       result = voltage;
>>>>>>>>>>>>               else if (voltage < min_uV || voltage > max_uV)
>>>>>>>>>>>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>>>>>>>>>>>> index dd13e05..5659aee 100644
>>>>>>>>>>>> --- a/include/linux/mmc/host.h
>>>>>>>>>>>> +++ b/include/linux/mmc/host.h
>>>>>>>>>>>> @@ -257,6 +257,7 @@ struct mmc_host {
>>>>>>>>>>>>  #define MMC_CAP2_HS200_1_2V_SDR      (1 << 6)        /* can
>>>>>>>>>>>> support */
>>>>>>>>>>>>  #define MMC_CAP2_HS200               (MMC_CAP2_HS200_1_8V_SDR |
>>>>>>>>>>>> \
>>>>>>>>>>>>                                MMC_CAP2_HS200_1_2V_SDR)
>>>>>>>>>>>> +#define MMC_CAP2_BROKEN_VOLTAGE      (1 << 7)        /* Use the
>>>>>>>>>>>> broken voltage
>>>>>>>>>>>> */
>>>>>>>>>>>>
>>>>>>>>>>>>       mmc_pm_flag_t           pm_caps;        /* supported pm
>>>>>>>>>>>> features */
>>>>>>>>>>>>       unsigned int        power_notify_type;
>>>>>>>>>>>> --
>>>>>>>>>>>> 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
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> 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
>>
>
> --
> 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
>