| Message ID | 1456779678-20173-1-git-send-email-dev@lynxeye.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am Montag, den 29.02.2016, 22:01 +0100 schrieb Lucas Stach: > This allows to switch the card signal voltage level to 1.8V, > which is needed for any ultra high speed modes to work. > > Signed-off-by: Lucas Stach <dev@lynxeye.de> > --- > This needs the SDMMC memcomp pad calibration patches I just > sent out to be applied, otherwise the card voltage change will > fail with a message in the kernel log and a fall back to > high speed operation. The patches this one depends on have been applied for some time now. Please pick up this patch. Thanks, Lucas > --- > arch/arm/boot/dts/tegra30-beaver.dts | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/boot/dts/tegra30-beaver.dts > b/arch/arm/boot/dts/tegra30-beaver.dts > index 1daed40..6b237f2 100644 > --- a/arch/arm/boot/dts/tegra30-beaver.dts > +++ b/arch/arm/boot/dts/tegra30-beaver.dts > @@ -1848,7 +1848,7 @@ > > ldo5_reg: ldo5 { > regulator-name = > "vddio_sdmmc,avdd_vdac"; > - regulator-min-microvolt = > <3300000>; > + regulator-min-microvolt = > <1800000>; > regulator-max-microvolt = > <3300000>; > regulator-always-on; > }; > @@ -1919,6 +1919,7 @@ > > sdhci@78000000 { > status = "okay"; > + vqmmc-supply = <&ldo5_reg>; > cd-gpios = <&gpio TEGRA_GPIO(I, 5) GPIO_ACTIVE_LOW>; > wp-gpios = <&gpio TEGRA_GPIO(T, 3) > GPIO_ACTIVE_HIGH>; > power-gpios = <&gpio TEGRA_GPIO(D, 7) > GPIO_ACTIVE_HIGH>;
On Fri, May 13, 2016 at 09:25:31AM +0200, Lucas Stach wrote: > Am Montag, den 29.02.2016, 22:01 +0100 schrieb Lucas Stach: > > This allows to switch the card signal voltage level to 1.8V, > > which is needed for any ultra high speed modes to work. > > > > Signed-off-by: Lucas Stach <dev@lynxeye.de> > > --- > > This needs the SDMMC memcomp pad calibration patches I just > > sent out to be applied, otherwise the card voltage change will > > fail with a message in the kernel log and a fall back to > > high speed operation. > > The patches this one depends on have been applied for some time now. > Please pick up this patch. My understanding is that UHS modes currently cause problems on Beaver. What I don't understand about that is how it will even try those modes if the voltage regulator can't be set to 1.8 V? Shouldn't that actively prevent those modes from even being attempted? Thierry
Am Freitag, den 13.05.2016, 19:27 +0200 schrieb Thierry Reding: > On Fri, May 13, 2016 at 09:25:31AM +0200, Lucas Stach wrote: > > > > Am Montag, den 29.02.2016, 22:01 +0100 schrieb Lucas Stach: > > > > > > This allows to switch the card signal voltage level to 1.8V, > > > which is needed for any ultra high speed modes to work. > > > > > > Signed-off-by: Lucas Stach <dev@lynxeye.de> > > > --- > > > This needs the SDMMC memcomp pad calibration patches I just > > > sent out to be applied, otherwise the card voltage change will > > > fail with a message in the kernel log and a fall back to > > > high speed operation. > > The patches this one depends on have been applied for some time > > now. > > Please pick up this patch. > My understanding is that UHS modes currently cause problems on > Beaver. > What I don't understand about that is how it will even try those > modes > if the voltage regulator can't be set to 1.8 V? Shouldn't that > actively > prevent those modes from even being attempted? > AFAIK this is right. I haven't re-tested with v4.6 yet, but on v4.5 without this change the high-speed modes using 1.8V signaling would not be used on the SD card. As eMMC doesn't need the voltage switch, this one will work in high-speed mode on Beaver. Regards, Lucas
On 13/05/16 18:27, Thierry Reding wrote: > * PGP Signed by an unknown key > > On Fri, May 13, 2016 at 09:25:31AM +0200, Lucas Stach wrote: >> Am Montag, den 29.02.2016, 22:01 +0100 schrieb Lucas Stach: >>> This allows to switch the card signal voltage level to 1.8V, >>> which is needed for any ultra high speed modes to work. >>> >>> Signed-off-by: Lucas Stach <dev@lynxeye.de> >>> --- >>> This needs the SDMMC memcomp pad calibration patches I just >>> sent out to be applied, otherwise the card voltage change will >>> fail with a message in the kernel log and a fall back to >>> high speed operation. >> >> The patches this one depends on have been applied for some time now. >> Please pick up this patch. > > My understanding is that UHS modes currently cause problems on Beaver. > What I don't understand about that is how it will even try those modes > if the voltage regulator can't be set to 1.8 V? Shouldn't that actively > prevent those modes from even being attempted? Looking at the sdhci code, if the regulator is missing then we still attempt to place the controller is 1.8V mode ... static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc, struct mmc_ios *ios) { ... case MMC_SIGNAL_VOLTAGE_180: if (!IS_ERR(mmc->supply.vqmmc)) { ret = regulator_set_voltage(mmc->supply.vqmmc, 1700000, 1950000); if (ret) { pr_warn("%s: Switching to 1.8V signalling voltage failed\n", mmc_hostname(mmc)); return -EIO; } } /* * Enable 1.8V Signal Enable in the Host Control2 * register */ ctrl |= SDHCI_CTRL_VDD_180; sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2); /* Some controller need to do more when switching */ if (host->ops->voltage_switch) host->ops->voltage_switch(host); /* 1.8V regulator output should be stable within 5 ms */ ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2); if (ctrl & SDHCI_CTRL_VDD_180) return 0; pr_warn("%s: 1.8V regulator output did not became stable\n", mmc_hostname(mmc)); return -EAGAIN; Ideally, the above *should* fail if the regulator is missing. However, what I have found, is that in my case, even though the regulator is missing, the above succeeds and the host thinks we are operating at 1.8V even though we are still at 3.3V! It seems that this does not happen with all SD cards that support UHS. This patch resolves the problems I am seeing on beaver with SD card initialisation failing. I am surprised this is not causing problems for others? That said, looking at the schematics for the board, this patch looks correct to me and we should apply this and mark for stable as this appears to be broken for sometime. Note that even before Lucas' changes in the SD card init we will try to switch to 1.8V if the cards supports it and the controller is greater or equal to sdhci v3. Cheers Jon
On 29/02/16 21:01, Lucas Stach wrote: > This allows to switch the card signal voltage level to 1.8V, > which is needed for any ultra high speed modes to work. > > Signed-off-by: Lucas Stach <dev@lynxeye.de> > --- > This needs the SDMMC memcomp pad calibration patches I just > sent out to be applied, otherwise the card voltage change will > fail with a message in the kernel log and a fall back to > high speed operation. > --- > arch/arm/boot/dts/tegra30-beaver.dts | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/boot/dts/tegra30-beaver.dts b/arch/arm/boot/dts/tegra30-beaver.dts > index 1daed40..6b237f2 100644 > --- a/arch/arm/boot/dts/tegra30-beaver.dts > +++ b/arch/arm/boot/dts/tegra30-beaver.dts > @@ -1848,7 +1848,7 @@ > > ldo5_reg: ldo5 { > regulator-name = "vddio_sdmmc,avdd_vdac"; Stephen, looking at the schematics, I don't see that this is connected to the avdd_vdac (which appears to be unconnected) and so I am wondering if this name should be just "vddio_sdmmc"? > - regulator-min-microvolt = <3300000>; > + regulator-min-microvolt = <1800000>; > regulator-max-microvolt = <3300000>; > regulator-always-on; > }; > @@ -1919,6 +1919,7 @@ > > sdhci@78000000 { > status = "okay"; > + vqmmc-supply = <&ldo5_reg>; > cd-gpios = <&gpio TEGRA_GPIO(I, 5) GPIO_ACTIVE_LOW>; > wp-gpios = <&gpio TEGRA_GPIO(T, 3) GPIO_ACTIVE_HIGH>; > power-gpios = <&gpio TEGRA_GPIO(D, 7) GPIO_ACTIVE_HIGH>; > Otherwise ... Acked-by: Jon Hunter <jonathanh@nvidia.com> Cheers Jon
On 05/19/2016 08:31 AM, Jon Hunter wrote: > > On 29/02/16 21:01, Lucas Stach wrote: >> This allows to switch the card signal voltage level to 1.8V, >> which is needed for any ultra high speed modes to work. >> >> Signed-off-by: Lucas Stach <dev@lynxeye.de> >> --- >> This needs the SDMMC memcomp pad calibration patches I just >> sent out to be applied, otherwise the card voltage change will >> fail with a message in the kernel log and a fall back to >> high speed operation. >> --- >> arch/arm/boot/dts/tegra30-beaver.dts | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm/boot/dts/tegra30-beaver.dts b/arch/arm/boot/dts/tegra30-beaver.dts >> index 1daed40..6b237f2 100644 >> --- a/arch/arm/boot/dts/tegra30-beaver.dts >> +++ b/arch/arm/boot/dts/tegra30-beaver.dts >> @@ -1848,7 +1848,7 @@ >> >> ldo5_reg: ldo5 { >> regulator-name = "vddio_sdmmc,avdd_vdac"; > > Stephen, looking at the schematics, I don't see that this is connected > to the avdd_vdac (which appears to be unconnected) and so I am wondering > if this name should be just "vddio_sdmmc"? Yes, I don't know where avdd_vdac came from; it doesn't appear connected on any Beaver (or even Cardhu, from which it was probably derived) schematic that I have. CC += Bryan in case he has any recollection since he wrote this DT.
Adding Adrian and Ulf ... On 19/05/16 15:29, Jon Hunter wrote: > > On 13/05/16 18:27, Thierry Reding wrote: >> * PGP Signed by an unknown key >> >> On Fri, May 13, 2016 at 09:25:31AM +0200, Lucas Stach wrote: >>> Am Montag, den 29.02.2016, 22:01 +0100 schrieb Lucas Stach: >>>> This allows to switch the card signal voltage level to 1.8V, >>>> which is needed for any ultra high speed modes to work. >>>> >>>> Signed-off-by: Lucas Stach <dev@lynxeye.de> >>>> --- >>>> This needs the SDMMC memcomp pad calibration patches I just >>>> sent out to be applied, otherwise the card voltage change will >>>> fail with a message in the kernel log and a fall back to >>>> high speed operation. >>> >>> The patches this one depends on have been applied for some time now. >>> Please pick up this patch. >> >> My understanding is that UHS modes currently cause problems on Beaver. >> What I don't understand about that is how it will even try those modes >> if the voltage regulator can't be set to 1.8 V? Shouldn't that actively >> prevent those modes from even being attempted? > > Looking at the sdhci code, if the regulator is missing then we still > attempt to place the controller is 1.8V mode ... > > static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc, > struct mmc_ios *ios) > { > > ... > > case MMC_SIGNAL_VOLTAGE_180: > if (!IS_ERR(mmc->supply.vqmmc)) { > ret = regulator_set_voltage(mmc->supply.vqmmc, > 1700000, 1950000); > if (ret) { > pr_warn("%s: Switching to 1.8V signalling voltage failed\n", > mmc_hostname(mmc)); > return -EIO; > } > } > > /* > * Enable 1.8V Signal Enable in the Host Control2 > * register > */ > ctrl |= SDHCI_CTRL_VDD_180; > sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2); > > /* Some controller need to do more when switching */ > if (host->ops->voltage_switch) > host->ops->voltage_switch(host); > > /* 1.8V regulator output should be stable within 5 ms */ > ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2); > if (ctrl & SDHCI_CTRL_VDD_180) > return 0; > > pr_warn("%s: 1.8V regulator output did not became stable\n", > mmc_hostname(mmc)); > > return -EAGAIN; > > Ideally, the above *should* fail if the regulator is missing. However, what > I have found, is that in my case, even though the regulator is missing, the > above succeeds and the host thinks we are operating at 1.8V even though we > are still at 3.3V! It seems that this does not happen with all SD cards that > support UHS. > > This patch resolves the problems I am seeing on beaver with SD card > initialisation failing. I am surprised this is not causing problems for > others? Adrian, Ulf, per the above, I have found that on a Tegra30 beaver board, if we enable UHS-I modes for Tegra30 but the device-tree for the board is missing the regulator to select 1.8V mode operation, then the above code sequence may still return success (ie. SDHCI_CTRL_VDD_180 bit is set in SDHCI_HOST_CONTROL2) even though we have not changed the voltage. This leads to other problems later on during SD initialisation. Would you expect that an SDHCI controller should fail to set the SDHCI_CTRL_VDD_180 bit in the SDHCI_HOST_CONTROL2 register if we did not change the voltage? We want to ensure that Tegra devices do not attempt to switch the UHS-I modes if the regulator is not present and it is not clear to me if there is a problem with the Tegra SDHCI controller or how this should be handled. Let me know if this makes sense. Cheers Jon
On 13/06/16 13:22, Jon Hunter wrote: > Adding Adrian and Ulf ... > > On 19/05/16 15:29, Jon Hunter wrote: >> >> On 13/05/16 18:27, Thierry Reding wrote: >>> * PGP Signed by an unknown key >>> >>> On Fri, May 13, 2016 at 09:25:31AM +0200, Lucas Stach wrote: >>>> Am Montag, den 29.02.2016, 22:01 +0100 schrieb Lucas Stach: >>>>> This allows to switch the card signal voltage level to 1.8V, >>>>> which is needed for any ultra high speed modes to work. >>>>> >>>>> Signed-off-by: Lucas Stach <dev@lynxeye.de> >>>>> --- >>>>> This needs the SDMMC memcomp pad calibration patches I just >>>>> sent out to be applied, otherwise the card voltage change will >>>>> fail with a message in the kernel log and a fall back to >>>>> high speed operation. >>>> >>>> The patches this one depends on have been applied for some time now. >>>> Please pick up this patch. >>> >>> My understanding is that UHS modes currently cause problems on Beaver. >>> What I don't understand about that is how it will even try those modes >>> if the voltage regulator can't be set to 1.8 V? Shouldn't that actively >>> prevent those modes from even being attempted? >> >> Looking at the sdhci code, if the regulator is missing then we still >> attempt to place the controller is 1.8V mode ... >> >> static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc, >> struct mmc_ios *ios) >> { >> >> ... >> >> case MMC_SIGNAL_VOLTAGE_180: >> if (!IS_ERR(mmc->supply.vqmmc)) { >> ret = regulator_set_voltage(mmc->supply.vqmmc, >> 1700000, 1950000); >> if (ret) { >> pr_warn("%s: Switching to 1.8V signalling voltage failed\n", >> mmc_hostname(mmc)); >> return -EIO; >> } >> } >> >> /* >> * Enable 1.8V Signal Enable in the Host Control2 >> * register >> */ >> ctrl |= SDHCI_CTRL_VDD_180; >> sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2); >> >> /* Some controller need to do more when switching */ >> if (host->ops->voltage_switch) >> host->ops->voltage_switch(host); >> >> /* 1.8V regulator output should be stable within 5 ms */ >> ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2); >> if (ctrl & SDHCI_CTRL_VDD_180) >> return 0; >> >> pr_warn("%s: 1.8V regulator output did not became stable\n", >> mmc_hostname(mmc)); >> >> return -EAGAIN; >> >> Ideally, the above *should* fail if the regulator is missing. However, what >> I have found, is that in my case, even though the regulator is missing, the >> above succeeds and the host thinks we are operating at 1.8V even though we >> are still at 3.3V! It seems that this does not happen with all SD cards that >> support UHS. >> >> This patch resolves the problems I am seeing on beaver with SD card >> initialisation failing. I am surprised this is not causing problems for >> others? > > Adrian, Ulf, per the above, I have found that on a Tegra30 beaver board, > if we enable UHS-I modes for Tegra30 but the device-tree for the board > is missing the regulator to select 1.8V mode operation, then the above > code sequence may still return success (ie. SDHCI_CTRL_VDD_180 bit is > set in SDHCI_HOST_CONTROL2) even though we have not changed the voltage. > This leads to other problems later on during SD initialisation. > > Would you expect that an SDHCI controller should fail to set the > SDHCI_CTRL_VDD_180 bit in the SDHCI_HOST_CONTROL2 register if we did not > change the voltage? What is meant to happen is that sdhci should wait 5ms and then check SDHCI_CTRL_VDD_180 - which it used to do but then someone took the 5ms wait away. In any case, if you are using a regulator there is no knowing what sdhci is meant to do. > > We want to ensure that Tegra devices do not attempt to switch the UHS-I > modes if the regulator is not present and it is not clear to me if there > is a problem with the Tegra SDHCI controller or how this should be handled. If the driver doesn't support UHS-I modes then it must remove the cap flags.
On 14/06/16 07:20, Adrian Hunter wrote: > On 13/06/16 13:22, Jon Hunter wrote: >> Adding Adrian and Ulf ... >> >> On 19/05/16 15:29, Jon Hunter wrote: >>> >>> On 13/05/16 18:27, Thierry Reding wrote: >>>> * PGP Signed by an unknown key >>>> >>>> On Fri, May 13, 2016 at 09:25:31AM +0200, Lucas Stach wrote: >>>>> Am Montag, den 29.02.2016, 22:01 +0100 schrieb Lucas Stach: >>>>>> This allows to switch the card signal voltage level to 1.8V, >>>>>> which is needed for any ultra high speed modes to work. >>>>>> >>>>>> Signed-off-by: Lucas Stach <dev@lynxeye.de> >>>>>> --- >>>>>> This needs the SDMMC memcomp pad calibration patches I just >>>>>> sent out to be applied, otherwise the card voltage change will >>>>>> fail with a message in the kernel log and a fall back to >>>>>> high speed operation. >>>>> >>>>> The patches this one depends on have been applied for some time now. >>>>> Please pick up this patch. >>>> >>>> My understanding is that UHS modes currently cause problems on Beaver. >>>> What I don't understand about that is how it will even try those modes >>>> if the voltage regulator can't be set to 1.8 V? Shouldn't that actively >>>> prevent those modes from even being attempted? >>> >>> Looking at the sdhci code, if the regulator is missing then we still >>> attempt to place the controller is 1.8V mode ... >>> >>> static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc, >>> struct mmc_ios *ios) >>> { >>> >>> ... >>> >>> case MMC_SIGNAL_VOLTAGE_180: >>> if (!IS_ERR(mmc->supply.vqmmc)) { >>> ret = regulator_set_voltage(mmc->supply.vqmmc, >>> 1700000, 1950000); >>> if (ret) { >>> pr_warn("%s: Switching to 1.8V signalling voltage failed\n", >>> mmc_hostname(mmc)); >>> return -EIO; >>> } >>> } >>> >>> /* >>> * Enable 1.8V Signal Enable in the Host Control2 >>> * register >>> */ >>> ctrl |= SDHCI_CTRL_VDD_180; >>> sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2); >>> >>> /* Some controller need to do more when switching */ >>> if (host->ops->voltage_switch) >>> host->ops->voltage_switch(host); >>> >>> /* 1.8V regulator output should be stable within 5 ms */ >>> ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2); >>> if (ctrl & SDHCI_CTRL_VDD_180) >>> return 0; >>> >>> pr_warn("%s: 1.8V regulator output did not became stable\n", >>> mmc_hostname(mmc)); >>> >>> return -EAGAIN; >>> >>> Ideally, the above *should* fail if the regulator is missing. However, what >>> I have found, is that in my case, even though the regulator is missing, the >>> above succeeds and the host thinks we are operating at 1.8V even though we >>> are still at 3.3V! It seems that this does not happen with all SD cards that >>> support UHS. >>> >>> This patch resolves the problems I am seeing on beaver with SD card >>> initialisation failing. I am surprised this is not causing problems for >>> others? >> >> Adrian, Ulf, per the above, I have found that on a Tegra30 beaver board, >> if we enable UHS-I modes for Tegra30 but the device-tree for the board >> is missing the regulator to select 1.8V mode operation, then the above >> code sequence may still return success (ie. SDHCI_CTRL_VDD_180 bit is >> set in SDHCI_HOST_CONTROL2) even though we have not changed the voltage. >> This leads to other problems later on during SD initialisation. >> >> Would you expect that an SDHCI controller should fail to set the >> SDHCI_CTRL_VDD_180 bit in the SDHCI_HOST_CONTROL2 register if we did not >> change the voltage? > > What is meant to happen is that sdhci should wait 5ms and then check > SDHCI_CTRL_VDD_180 - which it used to do but then someone took the 5ms wait > away. Do you plan to add the 5ms delay again? > In any case, if you are using a regulator there is no knowing what sdhci is > meant to do. Ok, seems fragile. >> >> We want to ensure that Tegra devices do not attempt to switch the UHS-I >> modes if the regulator is not present and it is not clear to me if there >> is a problem with the Tegra SDHCI controller or how this should be handled. > > If the driver doesn't support UHS-I modes then it must remove the cap flags. So the controller itself supports UHS-I modes, but a given board may not have the regulator to support them. We need a way to determine if the board can support the UHS-I modes. Now we could check to see if the regulator is present in the Tegra SDHCI driver and if not remove the cap flags. However, I was not sure if this is applicable to other sdhci controllers and so there should be a generic solution for this? Cheers Jon
On 14/06/16 11:23, Jon Hunter wrote: > > On 14/06/16 07:20, Adrian Hunter wrote: >> On 13/06/16 13:22, Jon Hunter wrote: >>> Adding Adrian and Ulf ... >>> >>> On 19/05/16 15:29, Jon Hunter wrote: >>>> >>>> On 13/05/16 18:27, Thierry Reding wrote: >>>>> * PGP Signed by an unknown key >>>>> >>>>> On Fri, May 13, 2016 at 09:25:31AM +0200, Lucas Stach wrote: >>>>>> Am Montag, den 29.02.2016, 22:01 +0100 schrieb Lucas Stach: >>>>>>> This allows to switch the card signal voltage level to 1.8V, >>>>>>> which is needed for any ultra high speed modes to work. >>>>>>> >>>>>>> Signed-off-by: Lucas Stach <dev@lynxeye.de> >>>>>>> --- >>>>>>> This needs the SDMMC memcomp pad calibration patches I just >>>>>>> sent out to be applied, otherwise the card voltage change will >>>>>>> fail with a message in the kernel log and a fall back to >>>>>>> high speed operation. >>>>>> >>>>>> The patches this one depends on have been applied for some time now. >>>>>> Please pick up this patch. >>>>> >>>>> My understanding is that UHS modes currently cause problems on Beaver. >>>>> What I don't understand about that is how it will even try those modes >>>>> if the voltage regulator can't be set to 1.8 V? Shouldn't that actively >>>>> prevent those modes from even being attempted? >>>> >>>> Looking at the sdhci code, if the regulator is missing then we still >>>> attempt to place the controller is 1.8V mode ... >>>> >>>> static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc, >>>> struct mmc_ios *ios) >>>> { >>>> >>>> ... >>>> >>>> case MMC_SIGNAL_VOLTAGE_180: >>>> if (!IS_ERR(mmc->supply.vqmmc)) { >>>> ret = regulator_set_voltage(mmc->supply.vqmmc, >>>> 1700000, 1950000); >>>> if (ret) { >>>> pr_warn("%s: Switching to 1.8V signalling voltage failed\n", >>>> mmc_hostname(mmc)); >>>> return -EIO; >>>> } >>>> } >>>> >>>> /* >>>> * Enable 1.8V Signal Enable in the Host Control2 >>>> * register >>>> */ >>>> ctrl |= SDHCI_CTRL_VDD_180; >>>> sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2); >>>> >>>> /* Some controller need to do more when switching */ >>>> if (host->ops->voltage_switch) >>>> host->ops->voltage_switch(host); >>>> >>>> /* 1.8V regulator output should be stable within 5 ms */ >>>> ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2); >>>> if (ctrl & SDHCI_CTRL_VDD_180) >>>> return 0; >>>> >>>> pr_warn("%s: 1.8V regulator output did not became stable\n", >>>> mmc_hostname(mmc)); >>>> >>>> return -EAGAIN; >>>> >>>> Ideally, the above *should* fail if the regulator is missing. However, what >>>> I have found, is that in my case, even though the regulator is missing, the >>>> above succeeds and the host thinks we are operating at 1.8V even though we >>>> are still at 3.3V! It seems that this does not happen with all SD cards that >>>> support UHS. >>>> >>>> This patch resolves the problems I am seeing on beaver with SD card >>>> initialisation failing. I am surprised this is not causing problems for >>>> others? >>> >>> Adrian, Ulf, per the above, I have found that on a Tegra30 beaver board, >>> if we enable UHS-I modes for Tegra30 but the device-tree for the board >>> is missing the regulator to select 1.8V mode operation, then the above >>> code sequence may still return success (ie. SDHCI_CTRL_VDD_180 bit is >>> set in SDHCI_HOST_CONTROL2) even though we have not changed the voltage. >>> This leads to other problems later on during SD initialisation. >>> >>> Would you expect that an SDHCI controller should fail to set the >>> SDHCI_CTRL_VDD_180 bit in the SDHCI_HOST_CONTROL2 register if we did not >>> change the voltage? >> >> What is meant to happen is that sdhci should wait 5ms and then check >> SDHCI_CTRL_VDD_180 - which it used to do but then someone took the 5ms wait >> away. > > Do you plan to add the 5ms delay again? I guess the assumption is the card will fail to switch voltage, so the check is unnecessary. > >> In any case, if you are using a regulator there is no knowing what sdhci is >> meant to do. > > Ok, seems fragile. In what way. > >>> >>> We want to ensure that Tegra devices do not attempt to switch the UHS-I >>> modes if the regulator is not present and it is not clear to me if there >>> is a problem with the Tegra SDHCI controller or how this should be handled. >> >> If the driver doesn't support UHS-I modes then it must remove the cap flags. > > So the controller itself supports UHS-I modes, but a given board may not > have the regulator to support them. We need a way to determine if the > board can support the UHS-I modes. Now we could check to see if the > regulator is present in the Tegra SDHCI driver and if not remove the cap > flags. However, I was not sure if this is applicable to other sdhci > controllers and so there should be a generic solution for this? There is SDHCI_QUIRK2_NO_1_8_V but it doesn't cover the eMMC 1.8V DDR52 case at present. Dong Aisheng wanted to plug that gap but I wanted to get rid of SDHCI_QUIRK2_NO_1_8_V: http://marc.info/?l=linux-mmc&m=146132847206423&w=2
On 14/06/16 11:05, Adrian Hunter wrote: > On 14/06/16 11:23, Jon Hunter wrote: >> >> On 14/06/16 07:20, Adrian Hunter wrote: >>> On 13/06/16 13:22, Jon Hunter wrote: >>>> Adding Adrian and Ulf ... >>>> >>>> On 19/05/16 15:29, Jon Hunter wrote: >>>>> >>>>> On 13/05/16 18:27, Thierry Reding wrote: >>>>>> * PGP Signed by an unknown key >>>>>> >>>>>> On Fri, May 13, 2016 at 09:25:31AM +0200, Lucas Stach wrote: >>>>>>> Am Montag, den 29.02.2016, 22:01 +0100 schrieb Lucas Stach: >>>>>>>> This allows to switch the card signal voltage level to 1.8V, >>>>>>>> which is needed for any ultra high speed modes to work. >>>>>>>> >>>>>>>> Signed-off-by: Lucas Stach <dev@lynxeye.de> >>>>>>>> --- >>>>>>>> This needs the SDMMC memcomp pad calibration patches I just >>>>>>>> sent out to be applied, otherwise the card voltage change will >>>>>>>> fail with a message in the kernel log and a fall back to >>>>>>>> high speed operation. >>>>>>> >>>>>>> The patches this one depends on have been applied for some time now. >>>>>>> Please pick up this patch. >>>>>> >>>>>> My understanding is that UHS modes currently cause problems on Beaver. >>>>>> What I don't understand about that is how it will even try those modes >>>>>> if the voltage regulator can't be set to 1.8 V? Shouldn't that actively >>>>>> prevent those modes from even being attempted? >>>>> >>>>> Looking at the sdhci code, if the regulator is missing then we still >>>>> attempt to place the controller is 1.8V mode ... >>>>> >>>>> static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc, >>>>> struct mmc_ios *ios) >>>>> { >>>>> >>>>> ... >>>>> >>>>> case MMC_SIGNAL_VOLTAGE_180: >>>>> if (!IS_ERR(mmc->supply.vqmmc)) { >>>>> ret = regulator_set_voltage(mmc->supply.vqmmc, >>>>> 1700000, 1950000); >>>>> if (ret) { >>>>> pr_warn("%s: Switching to 1.8V signalling voltage failed\n", >>>>> mmc_hostname(mmc)); >>>>> return -EIO; >>>>> } >>>>> } >>>>> >>>>> /* >>>>> * Enable 1.8V Signal Enable in the Host Control2 >>>>> * register >>>>> */ >>>>> ctrl |= SDHCI_CTRL_VDD_180; >>>>> sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2); >>>>> >>>>> /* Some controller need to do more when switching */ >>>>> if (host->ops->voltage_switch) >>>>> host->ops->voltage_switch(host); >>>>> >>>>> /* 1.8V regulator output should be stable within 5 ms */ >>>>> ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2); >>>>> if (ctrl & SDHCI_CTRL_VDD_180) >>>>> return 0; >>>>> >>>>> pr_warn("%s: 1.8V regulator output did not became stable\n", >>>>> mmc_hostname(mmc)); >>>>> >>>>> return -EAGAIN; >>>>> >>>>> Ideally, the above *should* fail if the regulator is missing. However, what >>>>> I have found, is that in my case, even though the regulator is missing, the >>>>> above succeeds and the host thinks we are operating at 1.8V even though we >>>>> are still at 3.3V! It seems that this does not happen with all SD cards that >>>>> support UHS. >>>>> >>>>> This patch resolves the problems I am seeing on beaver with SD card >>>>> initialisation failing. I am surprised this is not causing problems for >>>>> others? >>>> >>>> Adrian, Ulf, per the above, I have found that on a Tegra30 beaver board, >>>> if we enable UHS-I modes for Tegra30 but the device-tree for the board >>>> is missing the regulator to select 1.8V mode operation, then the above >>>> code sequence may still return success (ie. SDHCI_CTRL_VDD_180 bit is >>>> set in SDHCI_HOST_CONTROL2) even though we have not changed the voltage. >>>> This leads to other problems later on during SD initialisation. >>>> >>>> Would you expect that an SDHCI controller should fail to set the >>>> SDHCI_CTRL_VDD_180 bit in the SDHCI_HOST_CONTROL2 register if we did not >>>> change the voltage? >>> >>> What is meant to happen is that sdhci should wait 5ms and then check >>> SDHCI_CTRL_VDD_180 - which it used to do but then someone took the 5ms wait >>> away. >> >> Do you plan to add the 5ms delay again? > > I guess the assumption is the card will fail to switch voltage, so the check > is unnecessary. > >> >>> In any case, if you are using a regulator there is no knowing what sdhci is >>> meant to do. >> >> Ok, seems fragile. > > In what way. In the way, that the sdhci core is unable to determine if the regulator for 1.8V is mandatory or not for a given device and if the board has the required regulator. For Tegra we really want ... case MMC_SIGNAL_VOLTAGE_180: if (IS_ERR(mmc->supply.vqmmc)) return -EINVAL; ... But this probably would not work for all I am guessing? >>>> We want to ensure that Tegra devices do not attempt to switch the UHS-I >>>> modes if the regulator is not present and it is not clear to me if there >>>> is a problem with the Tegra SDHCI controller or how this should be handled. >>> >>> If the driver doesn't support UHS-I modes then it must remove the cap flags. >> >> So the controller itself supports UHS-I modes, but a given board may not >> have the regulator to support them. We need a way to determine if the >> board can support the UHS-I modes. Now we could check to see if the >> regulator is present in the Tegra SDHCI driver and if not remove the cap >> flags. However, I was not sure if this is applicable to other sdhci >> controllers and so there should be a generic solution for this? > > There is SDHCI_QUIRK2_NO_1_8_V but it doesn't cover the eMMC 1.8V DDR52 case > at present. Dong Aisheng wanted to plug that gap but I wanted to get rid of > SDHCI_QUIRK2_NO_1_8_V: > > http://marc.info/?l=linux-mmc&m=146132847206423&w=2 Ok, that would require the tegra sdhci driver to set this quirk for a board, which is do-able, I guess. However, given the above I am not sure what path you are suggesting we take to resolve this? Does not sound like we should be looking at using SDHCI_QUIRK2_NO_1_8_V anyway. Cheers Jon
Hi Adrian, On 14/06/16 15:19, Jon Hunter wrote: ... >>> So the controller itself supports UHS-I modes, but a given board may not >>> have the regulator to support them. We need a way to determine if the >>> board can support the UHS-I modes. Now we could check to see if the >>> regulator is present in the Tegra SDHCI driver and if not remove the cap >>> flags. However, I was not sure if this is applicable to other sdhci >>> controllers and so there should be a generic solution for this? >> >> There is SDHCI_QUIRK2_NO_1_8_V but it doesn't cover the eMMC 1.8V DDR52 case >> at present. Dong Aisheng wanted to plug that gap but I wanted to get rid of >> SDHCI_QUIRK2_NO_1_8_V: >> >> http://marc.info/?l=linux-mmc&m=146132847206423&w=2 > > Ok, that would require the tegra sdhci driver to set this quirk for a > board, which is do-able, I guess. However, given the above I am not sure > what path you are suggesting we take to resolve this? Does not sound > like we should be looking at using SDHCI_QUIRK2_NO_1_8_V anyway. Any feedback here? Are you still planning to get rid of SDHCI_QUIRK2_NO_1_8_V or should we use this? Cheers Jon
On 24/06/16 12:14, Jon Hunter wrote: > Hi Adrian, > > On 14/06/16 15:19, Jon Hunter wrote: > > ... > >>>> So the controller itself supports UHS-I modes, but a given board may not >>>> have the regulator to support them. We need a way to determine if the >>>> board can support the UHS-I modes. Now we could check to see if the >>>> regulator is present in the Tegra SDHCI driver and if not remove the cap >>>> flags. However, I was not sure if this is applicable to other sdhci >>>> controllers and so there should be a generic solution for this? >>> >>> There is SDHCI_QUIRK2_NO_1_8_V but it doesn't cover the eMMC 1.8V DDR52 case >>> at present. Dong Aisheng wanted to plug that gap but I wanted to get rid of >>> SDHCI_QUIRK2_NO_1_8_V: >>> >>> http://marc.info/?l=linux-mmc&m=146132847206423&w=2 >> >> Ok, that would require the tegra sdhci driver to set this quirk for a >> board, which is do-able, I guess. However, given the above I am not sure >> what path you are suggesting we take to resolve this? Does not sound >> like we should be looking at using SDHCI_QUIRK2_NO_1_8_V anyway. > > Any feedback here? Are you still planning to get rid of > SDHCI_QUIRK2_NO_1_8_V or should we use this? Don't use SDHCI_QUIRK2_NO_1_8_V. I sent some patches that make it easier for drivers to do whatever they want: http://marc.info/?l=linux-mmc&m=146712062816835
diff --git a/arch/arm/boot/dts/tegra30-beaver.dts b/arch/arm/boot/dts/tegra30-beaver.dts index 1daed40..6b237f2 100644 --- a/arch/arm/boot/dts/tegra30-beaver.dts +++ b/arch/arm/boot/dts/tegra30-beaver.dts @@ -1848,7 +1848,7 @@ ldo5_reg: ldo5 { regulator-name = "vddio_sdmmc,avdd_vdac"; - regulator-min-microvolt = <3300000>; + regulator-min-microvolt = <1800000>; regulator-max-microvolt = <3300000>; regulator-always-on; }; @@ -1919,6 +1919,7 @@ sdhci@78000000 { status = "okay"; + vqmmc-supply = <&ldo5_reg>; cd-gpios = <&gpio TEGRA_GPIO(I, 5) GPIO_ACTIVE_LOW>; wp-gpios = <&gpio TEGRA_GPIO(T, 3) GPIO_ACTIVE_HIGH>; power-gpios = <&gpio TEGRA_GPIO(D, 7) GPIO_ACTIVE_HIGH>;
This allows to switch the card signal voltage level to 1.8V, which is needed for any ultra high speed modes to work. Signed-off-by: Lucas Stach <dev@lynxeye.de> --- This needs the SDMMC memcomp pad calibration patches I just sent out to be applied, otherwise the card voltage change will fail with a message in the kernel log and a fall back to high speed operation. --- arch/arm/boot/dts/tegra30-beaver.dts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)