Possible regression with commit 52221610d

Possible regression with commit 52221610d

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

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.

Possible regression with commit 52221610d

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

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.

Re: Possible regression with commit 52221610d

[Tim Kryger]

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

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

Re: Possible regression with commit 52221610d

[Tim Kryger]

On Tue, Nov 4, 2014 at 1:00 AM, Alexandre Courbot <acourbot@nvidia.com> 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 <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.
>

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.

Re: Possible regression with commit 52221610d

[Alexandre Courbot]

On 11/05/2014 12:31 AM, Tim Kryger wrote:
> On Tue, Nov 4, 2014 at 1:00 AM, Alexandre Courbot <acourbot@nvidia.com> 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 <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.
>>
>
> 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.

Re: Possible regression with commit 52221610d

[Tim Kryger]

On Wed, Nov 5, 2014 at 12:10 AM, Alexandre Courbot <acourbot@nvidia.com> wrote:
> On 11/05/2014 12:31 AM, Tim Kryger wrote:
>>
>> On Tue, Nov 4, 2014 at 1:00 AM, Alexandre Courbot <acourbot@nvidia.com>
>> 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 <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.
>>>
>>
>> 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.
>
>

Re: Possible regression with commit 52221610d

[Alexandre Courbot]

On 11/06/2014 12:27 AM, Tim Kryger wrote:
> On Wed, Nov 5, 2014 at 12:10 AM, Alexandre Courbot <acourbot@nvidia.com> wrote:
>> On 11/05/2014 12:31 AM, Tim Kryger wrote:
>>>
>>> On Tue, Nov 4, 2014 at 1:00 AM, Alexandre Courbot <acourbot@nvidia.com>
>>> 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 <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.
>>>>
>>>
>>> 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!

Re: Possible regression with commit 52221610d

[Bjorn Andersson]

On Tue, Nov 4, 2014 at 7:31 AM, Tim Kryger <tim.kryger@gmail.com> wrote:
> On Tue, Nov 4, 2014 at 1:00 AM, Alexandre Courbot <acourbot@nvidia.com> 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 <acourbot@nvidia.com>
[..]
>>>> 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

Re: Possible regression with commit 52221610d

[Tim Kryger]

On Sat, Dec 13, 2014 at 11:22 PM, Bjorn Andersson <bjorn@kryo.se> wrote:
> On Tue, Nov 4, 2014 at 7:31 AM, Tim Kryger <tim.kryger@gmail.com> wrote:
>> On Tue, Nov 4, 2014 at 1:00 AM, Alexandre Courbot <acourbot@nvidia.com> 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 <acourbot@nvidia.com>
> [..]
>>>>> 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

Re: Possible regression with commit 52221610d

[Bjorn Andersson]

On Sun, Dec 14, 2014 at 8:48 PM, Tim Kryger <tim.kryger@gmail.com> wrote:
> On Sat, Dec 13, 2014 at 11:22 PM, Bjorn Andersson <bjorn@kryo.se> 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

Re: Possible regression with commit 52221610d

[Bjorn Andersson]

On Mon, Dec 15, 2014 at 10:27 PM, Bjorn Andersson <bjorn@kryo.se> wrote:
> On Sun, Dec 14, 2014 at 8:48 PM, Tim Kryger <tim.kryger@gmail.com> wrote:
>> On Sat, Dec 13, 2014 at 11:22 PM, Bjorn Andersson <bjorn@kryo.se> 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

Re: Possible regression with commit 52221610d

[Stephen Warren]

On 12/14/2014 09:48 PM, Tim Kryger wrote:
> On Sat, Dec 13, 2014 at 11:22 PM, Bjorn Andersson <bjorn@kryo.se> 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.

Re: Possible regression with commit 52221610d

[Tim Kryger]

On Tue, Dec 16, 2014 at 10:18 AM, Bjorn Andersson <bjorn@kryo.se> 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

Re: Possible regression with commit 52221610d

[Bjorn Andersson]

On Tue, Dec 16, 2014 at 10:20 PM, Tim Kryger <tim.kryger@gmail.com> wrote:
> On Tue, Dec 16, 2014 at 10:18 AM, Bjorn Andersson <bjorn@kryo.se> 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

Re: Possible regression with commit 52221610d

[Tim Kryger]

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

Re: Possible regression with commit 52221610d

[Bjorn Andersson]

On Sun, Dec 21, 2014 at 7:01 PM, Tim Kryger <tim.kryger@gmail.com> wrote:
> On Wed, Dec 17, 2014 at 11:57 AM, Bjorn Andersson <bjorn@kryo.se> wrote:
[..]
>> 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.

Sorry for the delay, but

Tested-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>

Regards,
Bjorn

Re: Possible regression with commit 52221610d

[Ulf Hansson]

On 5 January 2015 at 20:52, Bjorn Andersson <bjorn@kryo.se> wrote:
> On Sun, Dec 21, 2014 at 7:01 PM, Tim Kryger <tim.kryger@gmail.com> wrote:
>> On Wed, Dec 17, 2014 at 11:57 AM, Bjorn Andersson <bjorn@kryo.se> wrote:
> [..]
>>> 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.
>
> Sorry for the delay, but
>
> Tested-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
>

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

Kind regards
Uffe

Re: Possible regression with commit 52221610d

[Tim Kryger]

On Mon, Jan 5, 2015 at 11:52 AM, Bjorn Andersson <bjorn@kryo.se> wrote:
> On Sun, Dec 21, 2014 at 7:01 PM, Tim Kryger <tim.kryger@gmail.com> wrote:
>> On Wed, Dec 17, 2014 at 11:57 AM, Bjorn Andersson <bjorn@kryo.se> wrote:
> [..]
>>> 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.
>
> Sorry for the delay, but
>
> Tested-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
>
> Regards,
> Bjorn

Thanks.  I really appreciate your help.

-Tim

Re: Possible regression with commit 52221610d

[Tim Kryger]

On Mon, Jan 12, 2015 at 2:31 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 5 January 2015 at 20:52, Bjorn Andersson <bjorn@kryo.se> wrote:
>> On Sun, Dec 21, 2014 at 7:01 PM, Tim Kryger <tim.kryger@gmail.com> wrote:
>>> On Wed, Dec 17, 2014 at 11:57 AM, Bjorn Andersson <bjorn@kryo.se> wrote:
>> [..]
>>>> 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.
>>
>> Sorry for the delay, but
>>
>> Tested-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
>>
>
> This looks an okay approach. Please send a proper patch for me to
> apply as a fix.
>
> Kind regards
> Uffe

Okay.  I'll try to send something out tonight.

-Tim

Re: Possible regression with commit 52221610d

[Tim Kryger]

On Sun, Dec 21, 2014 at 7:01 PM, Tim Kryger <tim.kryger@gmail.com> wrote:
> 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.

I took a look at Nvidia's host controller in the Tegra124 SoC on a
Jetson TK1 board.

Fortunately, it behaves like the Qualcomm chip and only requires the LSB be set.