Re: Possible regression with commit 52221610d

[Alexandre Courbot]

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@xxxxxxxxxx> 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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/