Sure thing. I'll try to get it sent out later today.
-Tim
On Tue, Jun 24, 2014 at 4:11 AM, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
>
>
> On 23 juni 2014 06:30:08 CEST, Sachin Kamat <spk.linux@xxxxxxxxx> wrote:
>>Hi Tim,
>>
>>On Sat, Jun 21, 2014 at 2:31 AM, Tim Kryger <tim.kryger@xxxxxxxxx>
>>wrote:
>>> On Thu, Jun 19, 2014 at 8:33 PM, Sachin Kamat <spk.linux@xxxxxxxxx>
>>wrote:
>>>> On Thu, Jun 19, 2014 at 6:11 PM, Jaehoon Chung
>><jh80.chung@xxxxxxxxxxx> wrote:
>>>>> On 06/19/2014 07:40 PM, Sachin Kamat wrote:
>>>>>> On Thu, Jun 19, 2014 at 2:40 PM, Tim Kryger <tim.kryger@xxxxxxxxx>
>>wrote:
>>>>>>> On Thu, Jun 19, 2014 at 1:49 AM, Sachin Kamat
>><spk.linux@xxxxxxxxx> wrote:
>>>>>>>> On Thu, Jun 19, 2014 at 2:12 PM, Tim Kryger
>><tim.kryger@xxxxxxxxx> wrote:
>>>>>>>>> On Wed, Jun 18, 2014 at 8:54 PM, Sachin Kamat
>><spk.linux@xxxxxxxxx> wrote:
>>>>>>>>>>> On Wed, Jun 18, 2014 at 4:33 AM, Sachin Kamat
>><spk.linux@xxxxxxxxx> 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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
