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:
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?
Thanks,
Alex.
On Mon, Nov 3, 2014 at 7:05 PM, Alexandre Courbot <[email protected]> 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.
Since no external regulators are specified in tegra114.dtsi or
tegra114-roth.dts, mmc->supply.vmmc and mmc->supply.vqmmc are set to
-ENODEV.
> 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.
Can you provide the relevant parts of the log before the problem occurs?
Thanks,
Tim Kryger
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 <[email protected]> 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.
On Tue, Nov 4, 2014 at 1:00 AM, Alexandre Courbot <[email protected]> wrote:
> 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 <[email protected]>
>> 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.
>
I must have been looking at an old version of the file. Thanks for
clearing this up.
> 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?
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.
On 11/05/2014 12:31 AM, Tim Kryger wrote:
> On Tue, Nov 4, 2014 at 1:00 AM, Alexandre Courbot <[email protected]> wrote:
>> 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 <[email protected]>
>>> 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.
>>
>
> I must have been looking at an old version of the file. Thanks for
> clearing this up.
>
>> 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?
>
> 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.
On Wed, Nov 5, 2014 at 12:10 AM, Alexandre Courbot <[email protected]> wrote:
> On 11/05/2014 12:31 AM, Tim Kryger wrote:
>>
>> On Tue, Nov 4, 2014 at 1:00 AM, Alexandre Courbot <[email protected]>
>> wrote:
>>>
>>> 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 <[email protected]>
>>>> 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.
>>>
>>
>> I must have been looking at an old version of the file. Thanks for
>> clearing this up.
>>
>>> 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?
>>
>>
>> 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.
I believe it is working as intended. Is the schematic publicly available?
>
> Thanks!
> Alex.
>
>
On 11/06/2014 12:27 AM, Tim Kryger wrote:
> On Wed, Nov 5, 2014 at 12:10 AM, Alexandre Courbot <[email protected]> wrote:
>> On 11/05/2014 12:31 AM, Tim Kryger wrote:
>>>
>>> On Tue, Nov 4, 2014 at 1:00 AM, Alexandre Courbot <[email protected]>
>>> wrote:
>>>>
>>>> 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 <[email protected]>
>>>>> 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.
>>>>
>>>
>>> I must have been looking at an old version of the file. Thanks for
>>> clearing this up.
>>>
>>>> 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?
>>>
>>>
>>> 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.
>
> I believe it is working as intended. Is the schematic publicly available?
Sadly, no. But it is built similarly to other T114 boards, so I have to
assume the error is mine here. I will come back to this thread if it
turns out it is not.
Thanks for your help!
On Tue, Nov 4, 2014 at 7:31 AM, Tim Kryger <[email protected]> wrote:
> On Tue, Nov 4, 2014 at 1:00 AM, Alexandre Courbot <[email protected]> wrote:
>> 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 <[email protected]>
[..]
>>>> 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.
[..]
> 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?
Regards,
Bjorn
On Sat, Dec 13, 2014 at 11:22 PM, Bjorn Andersson <[email protected]> wrote:
> On Tue, Nov 4, 2014 at 7:31 AM, Tim Kryger <[email protected]> wrote:
>> On Tue, Nov 4, 2014 at 1:00 AM, Alexandre Courbot <[email protected]> wrote:
>>> 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 <[email protected]>
> [..]
>>>>> 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.
>
> [..]
>> 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?
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.
-Tim
On Sun, Dec 14, 2014 at 8:48 PM, Tim Kryger <[email protected]> wrote:
> On Sat, Dec 13, 2014 at 11:22 PM, Bjorn Andersson <[email protected]> wrote:
[..]
>> 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?
Regards,
Bjorn
On Mon, Dec 15, 2014 at 10:27 PM, Bjorn Andersson <[email protected]> wrote:
> On Sun, Dec 14, 2014 at 8:48 PM, Tim Kryger <[email protected]> wrote:
>> On Sat, Dec 13, 2014 at 11:22 PM, Bjorn Andersson <[email protected]> wrote:
> [..]
>>> 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?
>
Sorry, this is wrong.
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
Regards,
Bjorn
On 12/14/2014 09:48 PM, Tim Kryger wrote:
> On Sat, Dec 13, 2014 at 11:22 PM, Bjorn Andersson <[email protected]> wrote:
...
>> 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.
I've seen this question come up quite a few times.
Should Documentation/devicetree/bindings/mmc/mmc.txt document the
vmmc/vqmmc regulators? I assume they're considered shared across all
MMC/SDHCI controller bindings?
Since that only covers DT, it might be nice to document vmmc-vs-vqmmc
somewhere else too, such as right by the devm_regulator_get_optional()
calls in mmc_regulator_get_supply() in drivers/mmc/core/core.c.
On Tue, Dec 16, 2014 at 10:18 AM, Bjorn Andersson <[email protected]> wrote:
> 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
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.
Thanks,
Tim Kryger
On Tue, Dec 16, 2014 at 10:20 PM, Tim Kryger <[email protected]> wrote:
> On Tue, Dec 16, 2014 at 10:18 AM, Bjorn Andersson <[email protected]> wrote:
>
>> 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
>
> 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.
>
I gave it a spin on one of our Qualcomm 8974 based devices and writing
BIT(0) only seems to be enough.
> 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.
Regards,
Bjorn
On Wed, Dec 17, 2014 at 11:57 AM, Bjorn Andersson <[email protected]> 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