Re: Possible regression with commit 52221610d

[Tim Kryger <tim.kryger@gmail.com>]

On Wed, Dec 17, 2014 at 11:57 AM, Bjorn Andersson <bjorn@kryo.se> wrote:

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

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

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.

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

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index ada1a3e..59a328a 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -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.

-Tim