Re: Possible regression with commit 52221610d
On Sun, Dec 21, 2014 at 7:01 PM, Tim Kryger wrote: > On Wed, Dec 17, 2014 at 11:57 AM, Bjorn Andersson 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Possible regression with commit 52221610d
On Mon, Jan 12, 2015 at 2:31 AM, Ulf Hansson wrote: > On 5 January 2015 at 20:52, Bjorn Andersson wrote: >> On Sun, Dec 21, 2014 at 7:01 PM, Tim Kryger wrote: >>> On Wed, Dec 17, 2014 at 11:57 AM, Bjorn Andersson 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 >> > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Possible regression with commit 52221610d
On Mon, Jan 5, 2015 at 11:52 AM, Bjorn Andersson wrote: > On Sun, Dec 21, 2014 at 7:01 PM, Tim Kryger wrote: >> On Wed, Dec 17, 2014 at 11:57 AM, Bjorn Andersson 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 > > Regards, > Bjorn Thanks. I really appreciate your help. -Tim -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Possible regression with commit 52221610d
On 5 January 2015 at 20:52, Bjorn Andersson wrote: > On Sun, Dec 21, 2014 at 7:01 PM, Tim Kryger wrote: >> On Wed, Dec 17, 2014 at 11:57 AM, Bjorn Andersson 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 > This looks an okay approach. Please send a proper patch for me to apply as a fix. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Possible regression with commit 52221610d
On Sun, Dec 21, 2014 at 7:01 PM, Tim Kryger wrote: > On Wed, Dec 17, 2014 at 11:57 AM, Bjorn Andersson 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 Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Possible regression with commit 52221610d
On Wed, Dec 17, 2014 at 11:57 AM, Bjorn Andersson 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Possible regression with commit 52221610d
On Tue, Dec 16, 2014 at 10:20 PM, Tim Kryger wrote: > On Tue, Dec 16, 2014 at 10:18 AM, Bjorn Andersson 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Possible regression with commit 52221610d
On Tue, Dec 16, 2014 at 10:18 AM, Bjorn Andersson 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Possible regression with commit 52221610d
On 12/14/2014 09:48 PM, Tim Kryger wrote: On Sat, Dec 13, 2014 at 11:22 PM, Bjorn Andersson 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Possible regression with commit 52221610d
On Mon, Dec 15, 2014 at 10:27 PM, Bjorn Andersson wrote: > On Sun, Dec 14, 2014 at 8:48 PM, Tim Kryger wrote: >> On Sat, Dec 13, 2014 at 11:22 PM, Bjorn Andersson 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Possible regression with commit 52221610d
On Sun, Dec 14, 2014 at 8:48 PM, Tim Kryger wrote: > On Sat, Dec 13, 2014 at 11:22 PM, Bjorn Andersson 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Possible regression with commit 52221610d
On Sat, Dec 13, 2014 at 11:22 PM, Bjorn Andersson wrote: > On Tue, Nov 4, 2014 at 7:31 AM, Tim Kryger wrote: >> On Tue, Nov 4, 2014 at 1:00 AM, Alexandre Courbot >> 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 > [..] > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Possible regression with commit 52221610d
On Tue, Nov 4, 2014 at 7:31 AM, Tim Kryger wrote: > On Tue, Nov 4, 2014 at 1:00 AM, Alexandre Courbot 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 [..] 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Possible regression with commit 52221610d
On 11/06/2014 12:27 AM, Tim Kryger wrote: On Wed, Nov 5, 2014 at 12:10 AM, Alexandre Courbot wrote: On 11/05/2014 12:31 AM, Tim Kryger wrote: On Tue, Nov 4, 2014 at 1:00 AM, Alexandre Courbot 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 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! -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Possible regression with commit 52221610d
On Wed, Nov 5, 2014 at 12:10 AM, Alexandre Courbot wrote: > On 11/05/2014 12:31 AM, Tim Kryger wrote: >> >> On Tue, Nov 4, 2014 at 1:00 AM, Alexandre Courbot >> 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 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. > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Possible regression with commit 52221610d
On 11/05/2014 12:31 AM, Tim Kryger wrote: On Tue, Nov 4, 2014 at 1:00 AM, Alexandre Courbot 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 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Possible regression with commit 52221610d
On Tue, Nov 4, 2014 at 1:00 AM, Alexandre Courbot 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 >> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Possible regression with commit 52221610d
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 wrote: Hi guys, On the NVIDIA shield (tegra114-roth) platform, I have noticed that MMC stopped working completely on recent kernels. MMC devices will not show up and the message "mmc1: Controller never released inhibit bit(s)." shows up repeatedly in the console. After bisecting I tracked commit 52221610dd84dc3e9196554f0292ca9e8ab3541d ("mmc: sdhci: Improve external VDD regulator support") as the one that introduced this issue, which seems somehow surprising to me since it has been around for a while and nobody else complained about this AFAICT. I'm not too familiar with the Nvidia Shield so can you please confirm the following? The controller in the Tegra114 is SDHCI compliant and as such sdhci_tegra_probe calls sdhci_add_host. External regulators are sought in sdhci_add_host with a call to mmc_regulator_get_supply. This is correct. Since no external regulators are specified in tegra114.dtsi or tegra114-roth.dts, mmc->supply.vmmc and mmc->supply.vqmmc are set to -ENODEV. Actually 2 of the MMC nodes in tegra114-roth.dts (for SD card and eMMC) have a vmmc-supply property, so for two of them at least mmc->supply.vmmc is a valid pointer. The following diff solves the issue for me, however I don't know whether it also reverts the intended purpose of the initial patch: diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index ada1a3ea3a87..615701bb8ea3 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -1235,13 +1235,6 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned char mode, struct mmc_host *mmc = host->mmc; u8 pwr = 0; - if (!IS_ERR(mmc->supply.vmmc)) { - spin_unlock_irq(&host->lock); - mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd); - spin_lock_irq(&host->lock); - return; - } - if (mode != MMC_POWER_OFF) { switch (1 << vdd) { case MMC_VDD_165_195: @@ -1300,6 +1293,12 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned char mode, if (host->quirks & SDHCI_QUIRK_DELAY_AFTER_POWER) mdelay(10); } + + if (!IS_ERR(mmc->supply.vmmc)) { + spin_unlock_irq(&host->lock); + mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd); + spin_lock_irq(&host->lock); + } } Does this look like the right approach? If not, would you have any suggestion as to how to solve this problem? The patch you proposed would break Exynos4210 so I don't think it is appropriate. Do you understand why this code block is executed on your hardware? I wouldn't expect it. As explained above, vmmc is a valid pointer for 2 instances of the MMC controller. Interestingly, if I just remove the "return" line in the IS_ERR() block (without moving it around), the issue also seems to be fixed. Can you provide the relevant parts of the log before the problem occurs? There is not much unfortunately ; the only relevant log I have is this: [ 12.246022] mmc2: Timeout waiting for hardware interrupt. [ 12.264990] mmc2: Controller never released inhibit bit(s). Some hardware interrupt timed out. I don't know much about the MMC subsystem. but could it be because initially the controller is not in a powered-on state, and that return statement causes the function to leave it unpowered? Thanks, Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Possible regression with commit 52221610d
On Mon, Nov 3, 2014 at 7:05 PM, Alexandre Courbot 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/