Re: Possible regression with commit 52221610d

[Alexandre Courbot <acourbot@nvidia.com>]

Hi Tim, thanks for your reply!

On 11/04/2014 02:28 PM, Tim Kryger wrote:
> On Mon, Nov 3, 2014 at 7:05 PM, Alexandre Courbot <acourbot@nvidia.com> wrote:
>> Hi guys,
>>
>> On the NVIDIA shield (tegra114-roth) platform, I have noticed that MMC
>> stopped working completely on recent kernels. MMC devices will not show up
>> and the message "mmc1: Controller never released inhibit bit(s)." shows up
>> repeatedly in the console.
>>
>> After bisecting I tracked commit 52221610dd84dc3e9196554f0292ca9e8ab3541d
>> ("mmc: sdhci: Improve external VDD regulator support") as the one that
>> introduced this issue, which seems somehow surprising to me since it has
>> been around for a while and nobody else complained about this AFAICT.
>
> I'm not too familiar with the Nvidia Shield so can you please confirm
> the following?
>
> The controller in the Tegra114 is SDHCI compliant and as such
> sdhci_tegra_probe calls sdhci_add_host.  External regulators are
> sought in sdhci_add_host with a call to mmc_regulator_get_supply.

This is correct.

> Since no external regulators are specified in tegra114.dtsi or
> tegra114-roth.dts, mmc->supply.vmmc and mmc->supply.vqmmc are set to
> -ENODEV.

Actually 2 of the MMC nodes in tegra114-roth.dts (for SD card and eMMC) 
have a vmmc-supply property, so for two of them at least 
mmc->supply.vmmc is a valid pointer.

>
>> The following diff solves the issue for me, however I don't know whether it
>> also reverts the intended purpose of the initial patch:
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index ada1a3ea3a87..615701bb8ea3 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -1235,13 +1235,6 @@ 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(mmc, mmc->supply.vmmc, vdd);
>> -               spin_lock_irq(&host->lock);
>> -               return;
>> -       }
>> -
>>          if (mode != MMC_POWER_OFF) {
>>                  switch (1 << vdd) {
>>                  case MMC_VDD_165_195:
>> @@ -1300,6 +1293,12 @@ 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(mmc, mmc->supply.vmmc, vdd);
>> +               spin_lock_irq(&host->lock);
>> +       }
>>   }
>>
>> Does this look like the right approach? If not, would you have any
>> suggestion as to how to solve this problem?
>
> The patch you proposed would break Exynos4210 so I don't think it is
> appropriate.
>
> Do you understand why this code block is executed on your hardware?  I
> wouldn't expect it.

As explained above, vmmc is a valid pointer for 2 instances of the MMC 
controller. Interestingly, if I just remove the "return" line in the 
IS_ERR() block (without moving it around), the issue also seems to be fixed.

>
> Can you provide the relevant parts of the log before the problem occurs?

There is not much unfortunately ; the only relevant log I have is this:

[   12.246022] mmc2: Timeout waiting for hardware interrupt.
[   12.264990] mmc2: Controller never released inhibit bit(s).

Some hardware interrupt timed out. I don't know much about the MMC 
subsystem. but could it be because initially the controller is not in a 
powered-on state, and that return statement causes the function to leave 
it unpowered?

Thanks,
Alex.