Possible regression with commit 52221610d

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.

The following diff solves the issue for me, however I don't know whether it also reverts the intended purpose of the initial patch:

drivers/mmc/host/sdhci.c +0 -7

index ada1a3ea3a87..615701bb8ea3 100644
@@ -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:

drivers/mmc/host/sdhci.c +6 -0

@@ -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?

Quoted 11 lines · Alexandre Courbot

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.

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

Quoted 36 lines

The following diff solves the issue for me, however I don't know whether it also reverts the intended purpose of the initial patch:

drivers/mmc/host/sdhci.c +0 -7

index ada1a3ea3a87..615701bb8ea3 100644
@@ -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:

drivers/mmc/host/sdhci.c +6 -0

@@ -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);
+       }
  }
 

Quoted 2 lines

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.

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

Hi Tim, thanks for your reply!

Quoted 19 lines · Tim Kryger

Quoted 10 lines · Alexandre Courbot

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.

Quoted 4 lines

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.

Quoted 6 lines

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.

Quoted 3 lines

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.

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.

Quoted 2 lines

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?

Quoted 32 lines · Alexandre Courbot

Hi Tim, thanks for your reply!

Quoted 19 lines · Tim Kryger

Quoted 10 lines · Alexandre Courbot

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.

Quoted 4 lines

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.

Quoted 6 lines

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.

Quoted 9 lines

Quoted 3 lines

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.

I must have been looking at an old version of the file. Thanks for clearing this up.

Quoted 10 lines

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.

Quoted 2 lines

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:

Quoted 7 lines

[   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?

In a nutshell, the issue here is that the SDHCI spec demands that VMMC be supplied by the controller itself with the specific voltage configured using the SDHCI_POWER_CONTROL register but almost nobody does this. Many SoCs omit this capability from their controllers and instead rely upon external regulators. In such cases there isn't normally any need to update the voltage bits of the power control register. It sounds like you are saying this isn't true for the Tegra114.

Quoted 79 lines · Tim Kryger

Quoted 45 lines · Alexandre Courbot

Hi Tim, thanks for your reply!

Quoted 24 lines · Tim Kryger

Quoted 11 lines · Alexandre Courbot

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.

Quoted 6 lines

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.

Quoted 6 lines

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.

Quoted 3 lines

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

Quoted 5 lines

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.

I must have been looking at an old version of the file. Thanks for clearing this up.

Quoted 12 lines

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.

Quoted 2 lines

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:

Quoted 8 lines

[   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?

Quoted 8 lines

In a nutshell, the issue here is that the SDHCI spec demands that VMMC be supplied by the controller itself with the specific voltage configured using the SDHCI_POWER_CONTROL register but almost nobody does this. Many SoCs omit this capability from their controllers and instead rely upon external regulators. In such cases there isn't normally any need to update the voltage bits of the power control register. It sounds like you are saying this isn't true for the Tegra114.

Thanks for your explanation, it makes sense now.

Looking at other Tegra boards .dts I noticed that SHIELD is the only one using a vmmc-supply. Maybe this is the part that is wrong? I wrote this DTS and cannot exclude I misread the schematics. Maybe that regulator is used for some other (still sdmmc-related) purpose but the actual power provider is the controller itself.

If you can confirm that the driver is performing as it should, I will look in that direction and revise my DTS.

Thanks!
Alex.

Quoted 98 lines · Alexandre Courbot

Quoted 79 lines · Tim Kryger

Quoted 45 lines · Alexandre Courbot

Hi Tim, thanks for your reply!

Quoted 24 lines · Tim Kryger

Quoted 11 lines · Alexandre Courbot

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.

Quoted 6 lines

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.

Quoted 6 lines

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.

Quoted 3 lines

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

Quoted 5 lines

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.

I must have been looking at an old version of the file. Thanks for clearing this up.

Quoted 12 lines

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.

Quoted 2 lines

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:

Quoted 8 lines

[   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?

Quoted 8 lines

In a nutshell, the issue here is that the SDHCI spec demands that VMMC be supplied by the controller itself with the specific voltage configured using the SDHCI_POWER_CONTROL register but almost nobody does this. Many SoCs omit this capability from their controllers and instead rely upon external regulators. In such cases there isn't normally any need to update the voltage bits of the power control register. It sounds like you are saying this isn't true for the Tegra114.

Thanks for your explanation, it makes sense now.

Looking at other Tegra boards .dts I noticed that SHIELD is the only one using a vmmc-supply. Maybe this is the part that is wrong? I wrote this DTS and cannot exclude I misread the schematics. Maybe that regulator is used for some other (still sdmmc-related) purpose but the actual power provider is the controller itself.

Quoted 2 lines

If you can confirm that the driver is performing as it should, I will look in that direction and revise my DTS.

I believe it is working as intended. Is the schematic publicly available?

Quoted 5 lines · Alexandre Courbot

Hi Tim, thanks for your reply!

Quoted 2 lines · Tim Kryger

On Mon, Nov 3, 2014 at 7:05 PM, Alexandre Courbot <[email protected]>

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

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.

In a nutshell, the issue here is that the SDHCI spec demands that VMMC be supplied by the controller itself with the specific voltage configured using the SDHCI_POWER_CONTROL register but almost nobody does this. Many SoCs omit this capability from their controllers and instead rely upon external regulators. In such cases there isn't normally any need to update the voltage bits of the power control register. It sounds like you are saying this isn't true for the Tegra114.

Quoted 6 lines · Tim Kryger

Quoted 5 lines · Alexandre Courbot

Hi Tim, thanks for your reply!

Quoted 2 lines · Tim Kryger

On Mon, Nov 3, 2014 at 7:05 PM, Alexandre Courbot <[email protected]>

[..]

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

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.

After some hunting it seems like the recent Qualcomm platforms are suffering from this as well.

[..]

Quoted 8 lines

In a nutshell, the issue here is that the SDHCI spec demands that VMMC be supplied by the controller itself with the specific voltage configured using the SDHCI_POWER_CONTROL register but almost nobody does this. Many SoCs omit this capability from their controllers and instead rely upon external regulators. In such cases there isn't normally any need to update the voltage bits of the power control register. It sounds like you are saying this isn't true for the Tegra114.

Should one interpret your answer as that iff the SDHCI controller actually follows the specification (and provides the power control) then as of the introduction of 52221610dd vmmc should no longer be used for specifying the supply of power to the controller?

Or simply; what is vmmc (in the code) supposed to represent?

On Sat, Dec 13, 2014 at 11:22 PM, Bjorn Andersson <[email protected]> wrote:

Quoted 1 line

Or simply; what is vmmc (in the code) supposed to represent?

Hi Bjorn,

VMMC is the supply that delivers power out to the SD card itself (aka VDD).

It is not the internal power rail/power domain of the host controller within the SoC.

Quoted 1 line · Tim Kryger

On Sat, Dec 13, 2014 at 11:22 PM, Bjorn Andersson <[email protected]> wrote:

[..]

Quoted 9 lines

Quoted 1 line

Or simply; what is vmmc (in the code) supposed to represent?

Hi Bjorn,

VMMC is the supply that delivers power out to the SD card itself (aka VDD).

It is not the internal power rail/power domain of the host controller within the SoC.

Thanks for you answer Tim, I'll write up a patch for the Qualcomm driver that add the possibility of specifying an internal supply for the devices where that uses that.

My only concern is that for any standard compliant sdhci driver we're supposed to have a info printout that vmmc was not found (but vqmmc is there). But I guess that's a matter of proper documentation and hoping people don't pay too much attention to it?

On Sat, Dec 13, 2014 at 11:22 PM, Bjorn Andersson <[email protected]> wrote:

Quoted 1 line

Or simply; what is vmmc (in the code) supposed to represent?

Hi Bjorn,

VMMC is the supply that delivers power out to the SD card itself (aka VDD).

It is not the internal power rail/power domain of the host controller within the SoC.

We are routing the regulators straight to vdd of the memory and should hence use vmmc to specify this. However unless I actually program 0x29 in the Qualcomm sdhci block I get no responses from the card.

Which I believe is correct behavior as the SDHC specification [1] says the following about BIT(0) of 0x29:

"If this bit is cleared, the Host Controller shall immediately stop driving CMD and DAT[3:0] (tri-state) and drive SDCLK to low level".

So I think 52221610d is indeed incorrect.

[1] https://www.sdcard.org/downloads/pls/simplified_specs/archive/partA2_300.pdf

Quoted 13 lines · Bjorn Andersson

We are routing the regulators straight to vdd of the memory and should hence use vmmc to specify this. However unless I actually program 0x29 in the Qualcomm sdhci block I get no responses from the card.

Which I believe is correct behavior as the SDHC specification [1] says the following about BIT(0) of 0x29:

"If this bit is cleared, the Host Controller shall immediately stop driving CMD and DAT[3:0] (tri-state) and drive SDCLK to low level".

So I think 52221610d is indeed incorrect.

Quoted 1 line

[1] https://www.sdcard.org/downloads/pls/simplified_specs/archive/partA2_300.pdf

Agreed. Host controllers that fail to implement the required internal regulator configured via bits 1-3 of the Power Control Register may still follow the specification with regard to bit zero of that same register. The driver should be updated to configure bit zero appropriately even when an external regulator is used.

If you like, I can propose a patch or if you have one ready, I will be happy to review yours.

I'm somewhat puzzled to what benefit 52221610d brings after bringing back the write of BIT(0). Is it just that we don't hit the BUG() on non-standard voltages?

The full paragraph regarding BIT(0) reads:

Before setting this bit, the SD Host Driver shall set SD Bus Voltage Select. If the
Host Controller detects the No Card state, this bit shall be cleared. If this bit is cleared, the Host Controller shall immediately stop driving CMD and
DAT[3:0] (tri-state) and drive SDCLK to low level (Refer to Section 2.2.14).

So the Qualcomm HW engineers implemented the last "shall", but if someone else (what did nvidia do here?) also implemented the first "shall"s then we're back at needing a full revert of 52221610d.

Non-the-less, feel free to propose a patch and I will give it a test.

On Wed, Dec 17, 2014 at 11:57 AM, Bjorn Andersson <[email protected]> wrote:

Quoted 1 line

Non-the-less, feel free to propose a patch and I will give it a test.

Lets start with the simplest change first. Please give this a try and let me know what you think.

drivers/mmc/host/sdhci.c +6 -0

index ada1a3e..59a328a 100644
@@ -1239,6 +1239,12 @@ static void sdhci_set_power(struct sdhci_host
 *host, unsigned char mode,
                spin_unlock_irq(&host->lock);
                mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd);
                spin_lock_irq(&host->lock);
+
+               if (mode != MMC_POWER_OFF)
+                       sdhci_writeb(host, SDHCI_POWER_ON, SDHCI_POWER_CONTROL);
+               else
+                       sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
+
                return;
        }

Thanks again for your help in getting to the bottom of this.

Quoted 1 line · Tim Kryger

On Wed, Dec 17, 2014 at 11:57 AM, Bjorn Andersson <[email protected]> wrote:

[..]

Quoted 24 lines

Quoted 1 line

Non-the-less, feel free to propose a patch and I will give it a test.

Lets start with the simplest change first. Please give this a try and let me know what you think.

drivers/mmc/host/sdhci.c +6 -0

index ada1a3e..59a328a 100644
@@ -1239,6 +1239,12 @@ static void sdhci_set_power(struct sdhci_host
 *host, unsigned char mode,
                spin_unlock_irq(&host->lock);
                mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd);
                spin_lock_irq(&host->lock);
+
+               if (mode != MMC_POWER_OFF)
+                       sdhci_writeb(host, SDHCI_POWER_ON, SDHCI_POWER_CONTROL);
+               else
+                       sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
+
                return;
        }

Quoted 1 line

Thanks again for your help in getting to the bottom of this.

Sorry for the delay, but

Tested-by Bjorn Andersson <[email protected]>

Quoted 1 line · Tim Kryger

On Wed, Dec 17, 2014 at 11:57 AM, Bjorn Andersson <[email protected]> wrote:

[..]

Quoted 24 lines

Quoted 1 line

Non-the-less, feel free to propose a patch and I will give it a test.

Lets start with the simplest change first. Please give this a try and let me know what you think.

drivers/mmc/host/sdhci.c +6 -0

index ada1a3e..59a328a 100644
@@ -1239,6 +1239,12 @@ static void sdhci_set_power(struct sdhci_host
 *host, unsigned char mode,
                spin_unlock_irq(&host->lock);
                mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd);
                spin_lock_irq(&host->lock);
+
+               if (mode != MMC_POWER_OFF)
+                       sdhci_writeb(host, SDHCI_POWER_ON, SDHCI_POWER_CONTROL);
+               else
+                       sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
+
                return;
        }

Quoted 1 line

Thanks again for your help in getting to the bottom of this.

Sorry for the delay, but

Tested-by Bjorn Andersson <[email protected]>

Regards,
Bjorn

Quoted 31 lines · Bjorn Andersson

Quoted 1 line · Tim Kryger

On Wed, Dec 17, 2014 at 11:57 AM, Bjorn Andersson <[email protected]> wrote:

[..]

Quoted 24 lines

Quoted 1 line

Non-the-less, feel free to propose a patch and I will give it a test.

Lets start with the simplest change first. Please give this a try and let me know what you think.

drivers/mmc/host/sdhci.c +6 -0

index ada1a3e..59a328a 100644
@@ -1239,6 +1239,12 @@ static void sdhci_set_power(struct sdhci_host
 *host, unsigned char mode,
                spin_unlock_irq(&host->lock);
                mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd);
                spin_lock_irq(&host->lock);
+
+               if (mode != MMC_POWER_OFF)
+                       sdhci_writeb(host, SDHCI_POWER_ON, SDHCI_POWER_CONTROL);
+               else
+                       sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
+
                return;
        }

Quoted 1 line

Thanks again for your help in getting to the bottom of this.

Sorry for the delay, but

Quoted 2 lines

Tested-by Bjorn Andersson <[email protected]>

This looks an okay approach. Please send a proper patch for me to apply as a fix.

Kind regards
Uffe

Quoted 3 lines · Bjorn Andersson

I'm somewhat puzzled to what benefit 52221610d brings after bringing back the write of BIT(0). Is it just that we don't hit the BUG() on non-standard voltages?

It is to allow the use of external regulators that are capable of supplying a standard SD/MMC voltage but none of the standard SDHCI voltages.

Quoted 9 lines

The full paragraph regarding BIT(0) reads:

Before setting this bit, the SD Host Driver shall set SD Bus Voltage Select. If the
Host Controller detects the No Card state, this bit shall be cleared. If this bit is cleared, the Host Controller shall immediately stop driving CMD and
DAT[3:0] (tri-state) and drive SDCLK to low level (Refer to Section 2.2.14).

Quoted 3 lines

So the Qualcomm HW engineers implemented the last "shall", but if someone else (what did nvidia do here?) also implemented the first "shall"s then we're back at needing a full revert of 52221610d.

It is difficult to predict how non-compliant host controllers will behave in the area where they have chosen to deviate from the standard.

Do controllers that lack internal regulators claim to support 1.8, 3.0, or 3.3v in the host capabilities register? Or will they set none of these bits?

They lack the ability to influence the externally supplied VDD but will they place requirements on the values of the SD Bus Voltage Select field?

"If the Host Driver selects an unsupported voltage in the SD Bus Voltage Select field, the Host Controller may ignore writes to SD Bus Power and keep its value at zero."

I would hope that controllers that fail to implement the regulator would allow the SD Bus Voltage Select field to be set to any value.

We have established that it is okay to leave the Voltage Select as zero in the Broadcom, Qualcomm, and Samsung implementations.

It would be nice to get confirmation that this is also the case for other implementations that rely on an external regulator.