Re: MMC error on Exynos4210 board

[Ulf Hansson <ulf.hansson@linaro.org>]

On 23 juni 2014 06:30:08 CEST, Sachin Kamat <spk.linux@gmail.com> wrote:
>Hi Tim,
>
>On Sat, Jun 21, 2014 at 2:31 AM, Tim Kryger <tim.kryger@gmail.com>
>wrote:
>> On Thu, Jun 19, 2014 at 8:33 PM, Sachin Kamat <spk.linux@gmail.com>
>wrote:
>>> On Thu, Jun 19, 2014 at 6:11 PM, Jaehoon Chung
><jh80.chung@samsung.com> wrote:
>>>> On 06/19/2014 07:40 PM, Sachin Kamat wrote:
>>>>> On Thu, Jun 19, 2014 at 2:40 PM, Tim Kryger <tim.kryger@gmail.com>
>wrote:
>>>>>> On Thu, Jun 19, 2014 at 1:49 AM, Sachin Kamat
><spk.linux@gmail.com> wrote:
>>>>>>> On Thu, Jun 19, 2014 at 2:12 PM, Tim Kryger
><tim.kryger@gmail.com> wrote:
>>>>>>>> On Wed, Jun 18, 2014 at 8:54 PM, Sachin Kamat
><spk.linux@gmail.com> wrote:
>>>>>>>>>> On Wed, Jun 18, 2014 at 4:33 AM, Sachin Kamat
><spk.linux@gmail.com> wrote:
>>>>>>>>
>>>>>>>>>>> I see the below error on Exynos4210 based Origen board with
>linux-next
>>>>>>>>>>> (20140618).
>>>>>>>>>>> Reverting the below commit works fine.
>>>>>>>>>>>
>>>>>>>>>>> Commit: 8d02e775a6 "mmc: sdhci: Use mmc core regulator
>infrastucture"
>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> -- [    2.068992] sdhci: Secure Digital Host Controller
>Interface driver
>>>>>>>>>>> [    2.075059] sdhci: Copyright(c) Pierre Ossman
>>>>>>>>>>> [    2.079762] of_get_named_gpiod_flags: can't parse gpios
>property of
>>>>>>>>>>> node '/sdhci@12510000[0]'
>>>>>>>>>>> [    2.088021] s3c-sdhci 12510000.sdhci: clock source 2:
>mmc_busclk.2
>>>>>>>>>>> (50000000 Hz)
>>>>>>>>>>> [    2.095322] of_get_named_gpiod_flags: can't parse gpios
>property of
>>>>>>>>>>> node '/sdhci@12510000[0]'
>>>>>>>>>>> [    2.103794] of_get_named_gpiod_flags: can't parse gpios
>property of
>>>>>>>>>>> node '/sdhci@12510000[0]'
>>>>>>>>>>> [    2.112478] s3c-sdhci 12510000.sdhci: No vqmmc regulator
>found
>>>>>>>>>>> [    2.118117] mmc0: Hardware doesn't report any support
>voltages.
>>>>>>>>>>> [    2.124004] s3c-sdhci 12510000.sdhci: sdhci_add_host()
>failed
>>>>>>>>>>> [    2.130080] of_get_named_gpiod_flags: can't parse gpios
>property of
>>>>>>>>>>> node '/sdhci@12530000[0]'
>>>>>>>>>>> [    2.138352] s3c-sdhci 12530000.sdhci: clock source 2:
>mmc_busclk.2
>>>>>>>>>>> (16666667 Hz)
>>>>>>>>>>> [    2.145661] of_get_named_gpiod_flags: can't parse gpios
>property of
>>>>>>>>>>> node '/sdhci@12530000[0]'
>>>>>>>>>>> [    2.154139] of_get_named_gpiod_flags: can't parse gpios
>property of
>>>>>>>>>>> node '/sdhci@12530000[0]'
>>>>>>>>>>> [    2.162834] s3c-sdhci 12530000.sdhci: No vqmmc regulator
>found
>>>>>>>>>>> [    2.168464] mmc0: Hardware doesn't report any support
>voltages.
>>>>>>>>>>> [    2.174349] s3c-sdhci 12530000.sdhci: sdhci_add_host()
>failed
>>>>>>>>
>>>>>>>>>>> [    2.336148] Waiting for root device /dev/mmcblk0p1...
>>>>>>>>
>>>>>>>>> FYI, the board has a 2.8V fixed regulator supply connected to
>the MMC.
>>>>>>>>> You may refer to arch/arm/boot/dts/exynos4210-origen.dts for
>more details.
>>>>>>>>
>>>>>>>> A 2.8v regulator results in mmc->ocr_avail being set to
>MMC_VDD_27_28
>>>>>>>> | MMC_VDD_28_29.
>>>>>>>>
>>>>>>>> The SDHCI capabilities register only indicates support of three
>voltage levels
>>>>>>>>   - 1.8v: SDHCI_CAN_VDD_180 => MMC_VDD_165_195
>>>>>>>>   - 3.0v: SDHCI_CAN_VDD_300 => MMC_VDD_29_30 | MMC_VDD_30_31
>>>>>>>>   - 3.3v: SDHCI_CAN_VDD_330 => MMC_VDD_32_33 | MMC_VDD_33_34
>>>>
>>>> Right. sdhci capabilities only indicated them.
>>>> But I think SoC can be support the specific VDD range.
>>>>
>>>>>>>>
>>>>>>>> Even if all capability bits of the host controller were set,
>there
>>>>>>>> still wouldn't be any overlap.  Thus you see a "Hardware
>doesn't
>>>>>>>> report any support voltages" message.
>>>>>>>>
>>>>>>>> Previously, this issue was being swept under the rug by cec2e21
>mmc:
>>>>>>>> sdhci: Use regulator min/max voltage range according to spec. 
>That
>>>>>>>> change hacked up the voltage range checks such that with your
>2.8v
>>>>>>>> fixed regulator, the driver would believe the host could
>support
>>>>>>>> MMC_VDD_29_30 | MMC_VDD_30_31 | MMC_VDD_32_33 | MMC_VDD_33_34. 
>The
>>>>>>>> driver would start down the path of commanding 3.3v-3.4v (the
>highest
>>>>>>>> voltage range believed to be supported).  At the last second,
>the
>>>>>>>> driver would see the regulator was fixed and blindly skip over
>the set
>>>>>>>> voltage operation, saving it from failure.
>>>>>>>>
>>>>>>>> Since my patch eliminates the bogus voltage range checks, your
>board
>>>>>>>> is now getting caught playing too loose with the SDHCI
>regulator
>>>>>>>> voltages.
>>>>>>>>
>>>>>>>> Furthermore, the fixed regulator special-case logic that helped
>hide
>>>>>>>> your issue should also be considered for removal given that
>fixed
>>>>>>>> regulators now behave properly thanks to c00dc35 regulator:
>core:
>>>>>>>> Allow regulator_set_voltage for fixed regulators.
>>>>>>>
>>>>>>> Thanks for the detailed explanation. What do you propose to get
>this fixed
>>
>>>>>> It would be nice if the driver could be extended
>>>>>> to handle the peculiarities of your board in a deliberate manner
>but
>>>>>> limiting the common sdhci driver to supporting only the three
>voltages
>>>>>> from the spec also seems sensible.
>>>>>
>>>>> Until such time that the driver gets fixed to handle 2.8V fixed
>supply your
>>>>> current patch leaves several of Exynos boards broken for now.
>>>>
>>>> the all of exynos used the fixed-regulator(2.8v) should be broken.
>>>> (Maybe exynos4 series??)
>>>
>>> Probably. I haven't verified for the other boards. Hence would be
>good to handle
>>> this case in the driver itself.
>>
>> The current external VDD regulator support in the SDHCI driver feels
>a
>> bit tacked on.
>>
>> External regulators could reasonably support other voltage ranges,
>> like MMC_VDD_27_28 | MMC_VDD_28_29, but those never appear in the
>> final ocr_avail for the host. The driver only looks for the
>> intersection of the ranges implied by the capabilities register and
>> those of the external regulator.
>>
>> Later, when it comes time to set a voltage, the driver will BUG if it
>> can't translate the requested voltage into one of the three voltage
>> levels supported by the SDHCI Power Control register.
>>
>> I wonder if it is really necessary to constrain the driver this way.
>> It seems like if we are using an external regulator, the capabilities
>> of the host controller itself are irrelevant.  Also, must we touch
>the
>> Power Control register if we are using an external regulator?  I
>would
>> think not.
>
>You argument above seems reasonable.
>
>>
>> Can you give the following patch a try and see if it resolves the
>> issue you observed?
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index c23a872..07a2426 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -1226,6 +1226,13 @@ static void sdhci_set_power(struct sdhci_host
>> *host, unsigned char mode,
>>   struct mmc_host *mmc = host->mmc;
>>   u8 pwr = 0;
>>
>> + if (!IS_ERR(mmc->supply.vmmc)) {
>> + spin_unlock_irq(&host->lock);
>> + mmc_regulator_set_ocr(host->mmc, mmc->supply.vmmc, vdd);
>> + spin_lock_irq(&host->lock);
>> + return;
>> + }
>> +
>>   if (mode != MMC_POWER_OFF) {
>>   switch (1 << vdd) {
>>   case MMC_VDD_165_195:
>> @@ -1284,12 +1291,6 @@ static void sdhci_set_power(struct sdhci_host
>> *host, unsigned char mode,
>>   if (host->quirks & SDHCI_QUIRK_DELAY_AFTER_POWER)
>>   mdelay(10);
>>   }
>> -
>> - if (!IS_ERR(mmc->supply.vmmc)) {
>> - spin_unlock_irq(&host->lock);
>> - mmc_regulator_set_ocr(host->mmc, mmc->supply.vmmc, vdd);
>> - spin_lock_irq(&host->lock);
>> - }
>>  }
>>
>> 
>/*****************************************************************************\
>> @@ -3092,8 +3093,9 @@ int sdhci_add_host(struct sdhci_host *host)
>>     SDHCI_MAX_CURRENT_MULTIPLIER;
>>   }
>>
>> + /* If OCR set by external regulators, use it instead */
>>   if (mmc->ocr_avail)
>> - ocr_avail &= mmc->ocr_avail;
>> + ocr_avail = mmc->ocr_avail;
>>
>>   if (host->ocr_mask)
>>   ocr_avail &= host->ocr_mask;
>
>I can confirm that the above patch fixes the reported issue on
>Exynos4210 and 4412
>based origen boards that I have. Thanks for the fix.

Hi Tim/Sachin,

Great that you seemed to have work out issues. Could you please resend a the patch in proper format, thus I can apply it as a fix for 3.16.

Kind regards
Uffe