[PATCH] ARM: tegra: beaver: allow SD card voltage to be changed

[PATCH] ARM: tegra: beaver: allow SD card voltage to be changed

[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-8ppwABl0HbeELgA04lAiVw@public.gmane.org>
---
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";
-					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>;
-- 
2.5.0

[PATCH] ARM: tegra: beaver: allow SD card voltage to be changed

[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.
---
 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 at 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>;
-- 
2.5.0

Re: [PATCH] ARM: tegra: beaver: allow SD card voltage to be changed

[Lucas Stach]

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-8ppwABl0HbeELgA04lAiVw@public.gmane.org>
> ---
> 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>;

[PATCH] ARM: tegra: beaver: allow SD card voltage to be changed

[Lucas Stach]

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 at 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>;

Re: [PATCH] ARM: tegra: beaver: allow SD card voltage to be changed

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 951 bytes --]

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-8ppwABl0HbeELgA04lAiVw@public.gmane.org>
> > ---
> > 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH] ARM: tegra: beaver: allow SD card voltage to be changed

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

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160513/eda0eaad/attachment.sig>

Re: [PATCH] ARM: tegra: beaver: allow SD card voltage to be changed

[Lucas Stach]

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-8ppwABl0HbeELgA04lAiVw@public.gmane.org>
> > > ---
> > > 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

[PATCH] ARM: tegra: beaver: allow SD card voltage to be changed

[Lucas Stach]

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

Re: [PATCH] ARM: tegra: beaver: allow SD card voltage to be changed

[Jon Hunter]

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-8ppwABl0HbeELgA04lAiVw@public.gmane.org>
>>> ---
>>> 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

-- 
nvpublic

[PATCH] ARM: tegra: beaver: allow SD card voltage to be changed

[Jon Hunter]

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

-- 
nvpublic

Re: [PATCH] ARM: tegra: beaver: allow SD card voltage to be changed

[Jon Hunter]

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-8ppwABl0HbeELgA04lAiVw@public.gmane.org>
> ---
> 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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Cheers
Jon

-- 
nvpublic

[PATCH] ARM: tegra: beaver: allow SD card voltage to be changed

[Jon Hunter]

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

-- 
nvpublic

Re: [PATCH] ARM: tegra: beaver: allow SD card voltage to be changed

[Stephen Warren]

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-8ppwABl0HbeELgA04lAiVw@public.gmane.org>
>> ---
>> 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.

[PATCH] ARM: tegra: beaver: allow SD card voltage to be changed

[Stephen Warren]

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.

Re: [PATCH] ARM: tegra: beaver: allow SD card voltage to be changed

[Jon Hunter]

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-8ppwABl0HbeELgA04lAiVw@public.gmane.org>
>>>> ---
>>>> 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

-- 
nvpublic

[PATCH] ARM: tegra: beaver: allow SD card voltage to be changed

[Jon Hunter]

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

-- 
nvpublic

Re: [PATCH] ARM: tegra: beaver: allow SD card voltage to be changed

[Adrian Hunter]

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-8ppwABl0HbeELgA04lAiVw@public.gmane.org>
>>>>> ---
>>>>> 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.

[PATCH] ARM: tegra: beaver: allow SD card voltage to be changed

[Adrian Hunter]

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.

Re: [PATCH] ARM: tegra: beaver: allow SD card voltage to be changed

[Jon Hunter]

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-8ppwABl0HbeELgA04lAiVw@public.gmane.org>
>>>>>> ---
>>>>>> 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

-- 
nvpublic

[PATCH] ARM: tegra: beaver: allow SD card voltage to be changed

[Jon Hunter]

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

-- 
nvpublic

Re: [PATCH] ARM: tegra: beaver: allow SD card voltage to be changed

[Adrian Hunter]

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-8ppwABl0HbeELgA04lAiVw@public.gmane.org>
>>>>>>> ---
>>>>>>> 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

[PATCH] ARM: tegra: beaver: allow SD card voltage to be changed

[Adrian Hunter]

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

Re: [PATCH] ARM: tegra: beaver: allow SD card voltage to be changed

[Jon Hunter]

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-8ppwABl0HbeELgA04lAiVw@public.gmane.org>
>>>>>>>> ---
>>>>>>>> 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

-- 
nvpublic

[PATCH] ARM: tegra: beaver: allow SD card voltage to be changed

[Jon Hunter]

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

-- 
nvpublic

Re: [PATCH] ARM: tegra: beaver: allow SD card voltage to be changed

[Jon Hunter]

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

-- 
nvpublic

[PATCH] ARM: tegra: beaver: allow SD card voltage to be changed

[Jon Hunter]

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

-- 
nvpublic

Re: [PATCH] ARM: tegra: beaver: allow SD card voltage to be changed

[Adrian Hunter]

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

	

[PATCH] ARM: tegra: beaver: allow SD card voltage to be changed

[Adrian Hunter]

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

	

Re: [PATCH] ARM: tegra: beaver: Allow SD card voltage to be changed

[Olof Johansson]

On Thu, Jun 30, 2016 at 05:32:08PM +0200, Thierry Reding wrote:
> From: Lucas Stach <dev-8ppwABl0HbeELgA04lAiVw@public.gmane.org>
> 
> This allows to switch the card signal voltage level to 1.8 V, which is
> needed for any ultra high speed modes to work.
> 
> Signed-off-by: Lucas Stach <dev-8ppwABl0HbeELgA04lAiVw@public.gmane.org>
> Acked-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> Hi ARM-SoC maintainers,
> 
> Can you please apply this for v4.7? It fixes a regression with UHS modes
> that I had hoped could be fixed in the MMC core, but it turns out that a
> fairly invasive change will be required and it's not going to make v4.7.
> 
> I should note that this is not a workaround. It is the proper fix to
> enable UHS modes, but it exposes a weakness in the SDHCI driver which
> will happily switch to UHS modes whether or not the power supply can be
> configured to output the required voltage. Ideally the SDHCI driver will
> be taught to reject UHS modes if the available supply voltages aren't
> suitable. There is work underway to do that, but it won't make v4.7.

Applied to fixes.


-Olof

[PATCH] ARM: tegra: beaver: Allow SD card voltage to be changed

[Thierry Reding]

From: Lucas Stach <dev-8ppwABl0HbeELgA04lAiVw@public.gmane.org>

This allows to switch the card signal voltage level to 1.8 V, which is
needed for any ultra high speed modes to work.

Signed-off-by: Lucas Stach <dev-8ppwABl0HbeELgA04lAiVw@public.gmane.org>
Acked-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
Hi ARM-SoC maintainers,

Can you please apply this for v4.7? It fixes a regression with UHS modes
that I had hoped could be fixed in the MMC core, but it turns out that a
fairly invasive change will be required and it's not going to make v4.7.

I should note that this is not a workaround. It is the proper fix to
enable UHS modes, but it exposes a weakness in the SDHCI driver which
will happily switch to UHS modes whether or not the power supply can be
configured to output the required voltage. Ideally the SDHCI driver will
be taught to reject UHS modes if the available supply voltages aren't
suitable. There is work underway to do that, but it won't make v4.7.

Thanks,
Thierry

 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 1eca3b28ac64..b6da15d823a6 100644
--- a/arch/arm/boot/dts/tegra30-beaver.dts
+++ b/arch/arm/boot/dts/tegra30-beaver.dts
@@ -1843,7 +1843,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;
 				};
@@ -1914,6 +1914,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>;
-- 
2.8.3