Messages in this thread | | | Date | Tue, 4 Nov 2014 18:00:34 +0900 | From | Alexandre Courbot <> | Subject | Re: Possible regression with commit 52221610d |
| |
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.
| |