[PATCH] mmc: tegra: Only advertise UHS modes if IO regulator is present

[PATCH] mmc: tegra: Only advertise UHS modes if IO regulator is present

[Jon Hunter]

To support UHS modes for Tegra an external regulator must be present
to adjust the IO voltage accordingly. If the regulator is not present
but the host supports the UHS modes and the SD/MMC device supports the
UHS modes, then we will attempt to switch to a high-speed mode.

Without an external regulator, Tegra will fail to switch to the
high-speed mode. Although this is expected, it has been found that with
some SD cards, once it has been switched to operate at a high-speed
mode and hence a lower IO voltage, if the IO voltage is not changed then
all subsequent commands issues to the card will fail. Furthermore, it
will not be possible to switch back to a non high-speed mode and so
eventually the SD card initialisation will fail.

The SDHCI core does not require that the host have an external regulator
when switching to UHS modes and therefore, the Tegra SDHCI host
controller should only advertise the UHS modes as being supported if the
regulator for the IO voltage is present. Fortunately, Tegra has a vendor
specific register which can be used to control which modes are
advertised via the SDHCI_CAPABILITIES register and if the controller is
compatible with v3.0 of the SDHCI specification. Hence, if there is no
IO voltage regulator present in device-tree for the Tegra SDHCI host, then
don't advertise the UHS modes or that the controller is SDHCI v3.0
compatible.

Note that if the SDHCI is compatible with v3.0 of the SDHCI specification
then this will cause the SDHCI core to read the SDHCI_CAPABILITIES_1
register which will enable other UHS modes.

This fixes commit 7ad2ed1dfcbe ("mmc: tegra: enable UHS-I modes") which
enables UHS mode without checking if the board can support them.

Fixes: 7ad2ed1dfcbe ("mmc: tegra: enable UHS-I modes")

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---

Please note that I have been reviewing the series from Adrian [0] to
make it easier for drivers to set the capabilities. However, given that
we can control the capabilities that are advertised by the CAPS registers
it seems easier to use this feature.

[0] http://marc.info/?l=linux-mmc&m=146712062816835&w=2

 drivers/mmc/host/sdhci-tegra.c | 50 +++++++++++++++++++++++++-----------------
 1 file changed, 30 insertions(+), 20 deletions(-)

diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
index bcc0de47fe7e..c8b51cd71c56 100644
--- a/drivers/mmc/host/sdhci-tegra.c
+++ b/drivers/mmc/host/sdhci-tegra.c
@@ -140,6 +140,7 @@ static void tegra_sdhci_reset(struct sdhci_host *host, u8 mask)
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct sdhci_tegra *tegra_host = sdhci_pltfm_priv(pltfm_host);
 	const struct sdhci_tegra_soc_data *soc_data = tegra_host->soc_data;
+	struct device *dev = mmc_dev(host->mmc);
 	u32 misc_ctrl, clk_ctrl;
 
 	sdhci_reset(host, mask);
@@ -148,28 +149,37 @@ static void tegra_sdhci_reset(struct sdhci_host *host, u8 mask)
 		return;
 
 	misc_ctrl = sdhci_readl(host, SDHCI_TEGRA_VENDOR_MISC_CTRL);
-	/* Erratum: Enable SDHCI spec v3.00 support */
-	if (soc_data->nvquirks & NVQUIRK_ENABLE_SDHCI_SPEC_300)
-		misc_ctrl |= SDHCI_MISC_CTRL_ENABLE_SDHCI_SPEC_300;
-	/* Advertise UHS modes as supported by host */
-	if (soc_data->nvquirks & NVQUIRK_ENABLE_SDR50)
-		misc_ctrl |= SDHCI_MISC_CTRL_ENABLE_SDR50;
-	else
-		misc_ctrl &= ~SDHCI_MISC_CTRL_ENABLE_SDR50;
-	if (soc_data->nvquirks & NVQUIRK_ENABLE_DDR50)
-		misc_ctrl |= SDHCI_MISC_CTRL_ENABLE_DDR50;
-	else
-		misc_ctrl &= ~SDHCI_MISC_CTRL_ENABLE_DDR50;
-	if (soc_data->nvquirks & NVQUIRK_ENABLE_SDR104)
-		misc_ctrl |= SDHCI_MISC_CTRL_ENABLE_SDR104;
-	else
-		misc_ctrl &= ~SDHCI_MISC_CTRL_ENABLE_SDR104;
-	sdhci_writel(host, misc_ctrl, SDHCI_TEGRA_VENDOR_MISC_CTRL);
-
 	clk_ctrl = sdhci_readl(host, SDHCI_TEGRA_VENDOR_CLOCK_CTRL);
+
+	misc_ctrl &= ~(SDHCI_MISC_CTRL_ENABLE_SDHCI_SPEC_300 |
+		       SDHCI_MISC_CTRL_ENABLE_SDR50 |
+		       SDHCI_MISC_CTRL_ENABLE_DDR50 |
+		       SDHCI_MISC_CTRL_ENABLE_SDR104);
+
 	clk_ctrl &= ~SDHCI_CLOCK_CTRL_SPI_MODE_CLKEN_OVERRIDE;
-	if (soc_data->nvquirks & SDHCI_MISC_CTRL_ENABLE_SDR50)
-		clk_ctrl |= SDHCI_CLOCK_CTRL_SDR50_TUNING_OVERRIDE;
+
+	/*
+	 * If the board does not define a regulator for the SDHCI
+	 * IO voltage, then don't advertise support for UHS modes
+	 * even if the device supports it because the IO voltage
+	 * cannot be configured.
+	 */
+	if (of_property_read_bool(dev->of_node, "vqmmc-supply")) {
+		/* Erratum: Enable SDHCI spec v3.00 support */
+		if (soc_data->nvquirks & NVQUIRK_ENABLE_SDHCI_SPEC_300)
+			misc_ctrl |= SDHCI_MISC_CTRL_ENABLE_SDHCI_SPEC_300;
+		/* Advertise UHS modes as supported by host */
+		if (soc_data->nvquirks & NVQUIRK_ENABLE_SDR50)
+			misc_ctrl |= SDHCI_MISC_CTRL_ENABLE_SDR50;
+		if (soc_data->nvquirks & NVQUIRK_ENABLE_DDR50)
+			misc_ctrl |= SDHCI_MISC_CTRL_ENABLE_DDR50;
+		if (soc_data->nvquirks & NVQUIRK_ENABLE_SDR104)
+			misc_ctrl |= SDHCI_MISC_CTRL_ENABLE_SDR104;
+		if (soc_data->nvquirks & SDHCI_MISC_CTRL_ENABLE_SDR50)
+			clk_ctrl |= SDHCI_CLOCK_CTRL_SDR50_TUNING_OVERRIDE;
+	}
+
+	sdhci_writel(host, misc_ctrl, SDHCI_TEGRA_VENDOR_MISC_CTRL);
 	sdhci_writel(host, clk_ctrl, SDHCI_TEGRA_VENDOR_CLOCK_CTRL);
 
 	if (soc_data->nvquirks & NVQUIRK_HAS_PADCALIB)
-- 
2.1.4

Re: [PATCH] mmc: tegra: Only advertise UHS modes if IO regulator is present

[Alexandre Courbot]

On Tue, Jul 5, 2016 at 11:07 PM, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> To support UHS modes for Tegra an external regulator must be present
> to adjust the IO voltage accordingly. If the regulator is not present
> but the host supports the UHS modes and the SD/MMC device supports the
> UHS modes, then we will attempt to switch to a high-speed mode.
>
> Without an external regulator, Tegra will fail to switch to the
> high-speed mode. Although this is expected, it has been found that with
> some SD cards, once it has been switched to operate at a high-speed
> mode and hence a lower IO voltage, if the IO voltage is not changed then
> all subsequent commands issues to the card will fail. Furthermore, it
> will not be possible to switch back to a non high-speed mode and so
> eventually the SD card initialisation will fail.
>
> The SDHCI core does not require that the host have an external regulator
> when switching to UHS modes and therefore, the Tegra SDHCI host
> controller should only advertise the UHS modes as being supported if the
> regulator for the IO voltage is present. Fortunately, Tegra has a vendor
> specific register which can be used to control which modes are
> advertised via the SDHCI_CAPABILITIES register and if the controller is
> compatible with v3.0 of the SDHCI specification. Hence, if there is no
> IO voltage regulator present in device-tree for the Tegra SDHCI host, then
> don't advertise the UHS modes or that the controller is SDHCI v3.0
> compatible.
>
> Note that if the SDHCI is compatible with v3.0 of the SDHCI specification
> then this will cause the SDHCI core to read the SDHCI_CAPABILITIES_1
> register which will enable other UHS modes.
>
> This fixes commit 7ad2ed1dfcbe ("mmc: tegra: enable UHS-I modes") which
> enables UHS mode without checking if the board can support them.
>
> Fixes: 7ad2ed1dfcbe ("mmc: tegra: enable UHS-I modes")
>
> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>
> Please note that I have been reviewing the series from Adrian [0] to
> make it easier for drivers to set the capabilities. However, given that
> we can control the capabilities that are advertised by the CAPS registers
> it seems easier to use this feature.
>
> [0] http://marc.info/?l=linux-mmc&m=146712062816835&w=2
>
>  drivers/mmc/host/sdhci-tegra.c | 50 +++++++++++++++++++++++++-----------------
>  1 file changed, 30 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
> index bcc0de47fe7e..c8b51cd71c56 100644
> --- a/drivers/mmc/host/sdhci-tegra.c
> +++ b/drivers/mmc/host/sdhci-tegra.c
> @@ -140,6 +140,7 @@ static void tegra_sdhci_reset(struct sdhci_host *host, u8 mask)
>         struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>         struct sdhci_tegra *tegra_host = sdhci_pltfm_priv(pltfm_host);
>         const struct sdhci_tegra_soc_data *soc_data = tegra_host->soc_data;
> +       struct device *dev = mmc_dev(host->mmc);
>         u32 misc_ctrl, clk_ctrl;
>
>         sdhci_reset(host, mask);
> @@ -148,28 +149,37 @@ static void tegra_sdhci_reset(struct sdhci_host *host, u8 mask)
>                 return;
>
>         misc_ctrl = sdhci_readl(host, SDHCI_TEGRA_VENDOR_MISC_CTRL);
> -       /* Erratum: Enable SDHCI spec v3.00 support */
> -       if (soc_data->nvquirks & NVQUIRK_ENABLE_SDHCI_SPEC_300)
> -               misc_ctrl |= SDHCI_MISC_CTRL_ENABLE_SDHCI_SPEC_300;
> -       /* Advertise UHS modes as supported by host */
> -       if (soc_data->nvquirks & NVQUIRK_ENABLE_SDR50)
> -               misc_ctrl |= SDHCI_MISC_CTRL_ENABLE_SDR50;
> -       else
> -               misc_ctrl &= ~SDHCI_MISC_CTRL_ENABLE_SDR50;
> -       if (soc_data->nvquirks & NVQUIRK_ENABLE_DDR50)
> -               misc_ctrl |= SDHCI_MISC_CTRL_ENABLE_DDR50;
> -       else
> -               misc_ctrl &= ~SDHCI_MISC_CTRL_ENABLE_DDR50;
> -       if (soc_data->nvquirks & NVQUIRK_ENABLE_SDR104)
> -               misc_ctrl |= SDHCI_MISC_CTRL_ENABLE_SDR104;
> -       else
> -               misc_ctrl &= ~SDHCI_MISC_CTRL_ENABLE_SDR104;
> -       sdhci_writel(host, misc_ctrl, SDHCI_TEGRA_VENDOR_MISC_CTRL);
> -
>         clk_ctrl = sdhci_readl(host, SDHCI_TEGRA_VENDOR_CLOCK_CTRL);
> +
> +       misc_ctrl &= ~(SDHCI_MISC_CTRL_ENABLE_SDHCI_SPEC_300 |
> +                      SDHCI_MISC_CTRL_ENABLE_SDR50 |
> +                      SDHCI_MISC_CTRL_ENABLE_DDR50 |
> +                      SDHCI_MISC_CTRL_ENABLE_SDR104);
> +
>         clk_ctrl &= ~SDHCI_CLOCK_CTRL_SPI_MODE_CLKEN_OVERRIDE;
> -       if (soc_data->nvquirks & SDHCI_MISC_CTRL_ENABLE_SDR50)
> -               clk_ctrl |= SDHCI_CLOCK_CTRL_SDR50_TUNING_OVERRIDE;
> +
> +       /*
> +        * If the board does not define a regulator for the SDHCI
> +        * IO voltage, then don't advertise support for UHS modes
> +        * even if the device supports it because the IO voltage
> +        * cannot be configured.
> +        */
> +       if (of_property_read_bool(dev->of_node, "vqmmc-supply")) {

Can't you check whether (!IS_SERR(host->mmc->supply.vqmmc)) instead?
The property being present doesn't necessarily guarantee that the
regulator has been obtained successfully.

Re: [PATCH] mmc: tegra: Only advertise UHS modes if IO regulator is present

[Jon Hunter]

On 06/07/16 03:14, Alexandre Courbot wrote:
> On Tue, Jul 5, 2016 at 11:07 PM, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>> To support UHS modes for Tegra an external regulator must be present
>> to adjust the IO voltage accordingly. If the regulator is not present
>> but the host supports the UHS modes and the SD/MMC device supports the
>> UHS modes, then we will attempt to switch to a high-speed mode.
>>
>> Without an external regulator, Tegra will fail to switch to the
>> high-speed mode. Although this is expected, it has been found that with
>> some SD cards, once it has been switched to operate at a high-speed
>> mode and hence a lower IO voltage, if the IO voltage is not changed then
>> all subsequent commands issues to the card will fail. Furthermore, it
>> will not be possible to switch back to a non high-speed mode and so
>> eventually the SD card initialisation will fail.
>>
>> The SDHCI core does not require that the host have an external regulator
>> when switching to UHS modes and therefore, the Tegra SDHCI host
>> controller should only advertise the UHS modes as being supported if the
>> regulator for the IO voltage is present. Fortunately, Tegra has a vendor
>> specific register which can be used to control which modes are
>> advertised via the SDHCI_CAPABILITIES register and if the controller is
>> compatible with v3.0 of the SDHCI specification. Hence, if there is no
>> IO voltage regulator present in device-tree for the Tegra SDHCI host, then
>> don't advertise the UHS modes or that the controller is SDHCI v3.0
>> compatible.
>>
>> Note that if the SDHCI is compatible with v3.0 of the SDHCI specification
>> then this will cause the SDHCI core to read the SDHCI_CAPABILITIES_1
>> register which will enable other UHS modes.
>>
>> This fixes commit 7ad2ed1dfcbe ("mmc: tegra: enable UHS-I modes") which
>> enables UHS mode without checking if the board can support them.
>>
>> Fixes: 7ad2ed1dfcbe ("mmc: tegra: enable UHS-I modes")
>>
>> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>
>> Please note that I have been reviewing the series from Adrian [0] to
>> make it easier for drivers to set the capabilities. However, given that
>> we can control the capabilities that are advertised by the CAPS registers
>> it seems easier to use this feature.
>>
>> [0] http://marc.info/?l=linux-mmc&m=146712062816835&w=2
>>
>>  drivers/mmc/host/sdhci-tegra.c | 50 +++++++++++++++++++++++++-----------------
>>  1 file changed, 30 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
>> index bcc0de47fe7e..c8b51cd71c56 100644
>> --- a/drivers/mmc/host/sdhci-tegra.c
>> +++ b/drivers/mmc/host/sdhci-tegra.c
>> @@ -140,6 +140,7 @@ static void tegra_sdhci_reset(struct sdhci_host *host, u8 mask)
>>         struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>         struct sdhci_tegra *tegra_host = sdhci_pltfm_priv(pltfm_host);
>>         const struct sdhci_tegra_soc_data *soc_data = tegra_host->soc_data;
>> +       struct device *dev = mmc_dev(host->mmc);
>>         u32 misc_ctrl, clk_ctrl;
>>
>>         sdhci_reset(host, mask);
>> @@ -148,28 +149,37 @@ static void tegra_sdhci_reset(struct sdhci_host *host, u8 mask)
>>                 return;
>>
>>         misc_ctrl = sdhci_readl(host, SDHCI_TEGRA_VENDOR_MISC_CTRL);
>> -       /* Erratum: Enable SDHCI spec v3.00 support */
>> -       if (soc_data->nvquirks & NVQUIRK_ENABLE_SDHCI_SPEC_300)
>> -               misc_ctrl |= SDHCI_MISC_CTRL_ENABLE_SDHCI_SPEC_300;
>> -       /* Advertise UHS modes as supported by host */
>> -       if (soc_data->nvquirks & NVQUIRK_ENABLE_SDR50)
>> -               misc_ctrl |= SDHCI_MISC_CTRL_ENABLE_SDR50;
>> -       else
>> -               misc_ctrl &= ~SDHCI_MISC_CTRL_ENABLE_SDR50;
>> -       if (soc_data->nvquirks & NVQUIRK_ENABLE_DDR50)
>> -               misc_ctrl |= SDHCI_MISC_CTRL_ENABLE_DDR50;
>> -       else
>> -               misc_ctrl &= ~SDHCI_MISC_CTRL_ENABLE_DDR50;
>> -       if (soc_data->nvquirks & NVQUIRK_ENABLE_SDR104)
>> -               misc_ctrl |= SDHCI_MISC_CTRL_ENABLE_SDR104;
>> -       else
>> -               misc_ctrl &= ~SDHCI_MISC_CTRL_ENABLE_SDR104;
>> -       sdhci_writel(host, misc_ctrl, SDHCI_TEGRA_VENDOR_MISC_CTRL);
>> -
>>         clk_ctrl = sdhci_readl(host, SDHCI_TEGRA_VENDOR_CLOCK_CTRL);
>> +
>> +       misc_ctrl &= ~(SDHCI_MISC_CTRL_ENABLE_SDHCI_SPEC_300 |
>> +                      SDHCI_MISC_CTRL_ENABLE_SDR50 |
>> +                      SDHCI_MISC_CTRL_ENABLE_DDR50 |
>> +                      SDHCI_MISC_CTRL_ENABLE_SDR104);
>> +
>>         clk_ctrl &= ~SDHCI_CLOCK_CTRL_SPI_MODE_CLKEN_OVERRIDE;
>> -       if (soc_data->nvquirks & SDHCI_MISC_CTRL_ENABLE_SDR50)
>> -               clk_ctrl |= SDHCI_CLOCK_CTRL_SDR50_TUNING_OVERRIDE;
>> +
>> +       /*
>> +        * If the board does not define a regulator for the SDHCI
>> +        * IO voltage, then don't advertise support for UHS modes
>> +        * even if the device supports it because the IO voltage
>> +        * cannot be configured.
>> +        */
>> +       if (of_property_read_bool(dev->of_node, "vqmmc-supply")) {
> 
> Can't you check whether (!IS_SERR(host->mmc->supply.vqmmc)) instead?
> The property being present doesn't necessarily guarantee that the
> regulator has been obtained successfully.

The problem is that the regulator is obtained after the reset occurs and
so unfortunately that won't work either :-(

I wonder if we should request the regulator during the Tegra SDHCI probe
before we call sdhci_add_host(). This would allow us to defer the probe
if it is missing/not present.

Cheers
Jon

-- 
nvpublic

Re: [PATCH] mmc: tegra: Only advertise UHS modes if IO regulator is present

[Alexandre Courbot]

On Wed, Jul 6, 2016 at 4:46 PM, Jon Hunter <jonathanh@nvidia.com> wrote:
>
> On 06/07/16 03:14, Alexandre Courbot wrote:
>> On Tue, Jul 5, 2016 at 11:07 PM, Jon Hunter <jonathanh@nvidia.com> wrote:
>>> To support UHS modes for Tegra an external regulator must be present
>>> to adjust the IO voltage accordingly. If the regulator is not present
>>> but the host supports the UHS modes and the SD/MMC device supports the
>>> UHS modes, then we will attempt to switch to a high-speed mode.
>>>
>>> Without an external regulator, Tegra will fail to switch to the
>>> high-speed mode. Although this is expected, it has been found that with
>>> some SD cards, once it has been switched to operate at a high-speed
>>> mode and hence a lower IO voltage, if the IO voltage is not changed then
>>> all subsequent commands issues to the card will fail. Furthermore, it
>>> will not be possible to switch back to a non high-speed mode and so
>>> eventually the SD card initialisation will fail.
>>>
>>> The SDHCI core does not require that the host have an external regulator
>>> when switching to UHS modes and therefore, the Tegra SDHCI host
>>> controller should only advertise the UHS modes as being supported if the
>>> regulator for the IO voltage is present. Fortunately, Tegra has a vendor
>>> specific register which can be used to control which modes are
>>> advertised via the SDHCI_CAPABILITIES register and if the controller is
>>> compatible with v3.0 of the SDHCI specification. Hence, if there is no
>>> IO voltage regulator present in device-tree for the Tegra SDHCI host, then
>>> don't advertise the UHS modes or that the controller is SDHCI v3.0
>>> compatible.
>>>
>>> Note that if the SDHCI is compatible with v3.0 of the SDHCI specification
>>> then this will cause the SDHCI core to read the SDHCI_CAPABILITIES_1
>>> register which will enable other UHS modes.
>>>
>>> This fixes commit 7ad2ed1dfcbe ("mmc: tegra: enable UHS-I modes") which
>>> enables UHS mode without checking if the board can support them.
>>>
>>> Fixes: 7ad2ed1dfcbe ("mmc: tegra: enable UHS-I modes")
>>>
>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>>> ---
>>>
>>> Please note that I have been reviewing the series from Adrian [0] to
>>> make it easier for drivers to set the capabilities. However, given that
>>> we can control the capabilities that are advertised by the CAPS registers
>>> it seems easier to use this feature.
>>>
>>> [0] http://marc.info/?l=linux-mmc&m=146712062816835&w=2
>>>
>>>  drivers/mmc/host/sdhci-tegra.c | 50 +++++++++++++++++++++++++-----------------
>>>  1 file changed, 30 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
>>> index bcc0de47fe7e..c8b51cd71c56 100644
>>> --- a/drivers/mmc/host/sdhci-tegra.c
>>> +++ b/drivers/mmc/host/sdhci-tegra.c
>>> @@ -140,6 +140,7 @@ static void tegra_sdhci_reset(struct sdhci_host *host, u8 mask)
>>>         struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>         struct sdhci_tegra *tegra_host = sdhci_pltfm_priv(pltfm_host);
>>>         const struct sdhci_tegra_soc_data *soc_data = tegra_host->soc_data;
>>> +       struct device *dev = mmc_dev(host->mmc);
>>>         u32 misc_ctrl, clk_ctrl;
>>>
>>>         sdhci_reset(host, mask);
>>> @@ -148,28 +149,37 @@ static void tegra_sdhci_reset(struct sdhci_host *host, u8 mask)
>>>                 return;
>>>
>>>         misc_ctrl = sdhci_readl(host, SDHCI_TEGRA_VENDOR_MISC_CTRL);
>>> -       /* Erratum: Enable SDHCI spec v3.00 support */
>>> -       if (soc_data->nvquirks & NVQUIRK_ENABLE_SDHCI_SPEC_300)
>>> -               misc_ctrl |= SDHCI_MISC_CTRL_ENABLE_SDHCI_SPEC_300;
>>> -       /* Advertise UHS modes as supported by host */
>>> -       if (soc_data->nvquirks & NVQUIRK_ENABLE_SDR50)
>>> -               misc_ctrl |= SDHCI_MISC_CTRL_ENABLE_SDR50;
>>> -       else
>>> -               misc_ctrl &= ~SDHCI_MISC_CTRL_ENABLE_SDR50;
>>> -       if (soc_data->nvquirks & NVQUIRK_ENABLE_DDR50)
>>> -               misc_ctrl |= SDHCI_MISC_CTRL_ENABLE_DDR50;
>>> -       else
>>> -               misc_ctrl &= ~SDHCI_MISC_CTRL_ENABLE_DDR50;
>>> -       if (soc_data->nvquirks & NVQUIRK_ENABLE_SDR104)
>>> -               misc_ctrl |= SDHCI_MISC_CTRL_ENABLE_SDR104;
>>> -       else
>>> -               misc_ctrl &= ~SDHCI_MISC_CTRL_ENABLE_SDR104;
>>> -       sdhci_writel(host, misc_ctrl, SDHCI_TEGRA_VENDOR_MISC_CTRL);
>>> -
>>>         clk_ctrl = sdhci_readl(host, SDHCI_TEGRA_VENDOR_CLOCK_CTRL);
>>> +
>>> +       misc_ctrl &= ~(SDHCI_MISC_CTRL_ENABLE_SDHCI_SPEC_300 |
>>> +                      SDHCI_MISC_CTRL_ENABLE_SDR50 |
>>> +                      SDHCI_MISC_CTRL_ENABLE_DDR50 |
>>> +                      SDHCI_MISC_CTRL_ENABLE_SDR104);
>>> +
>>>         clk_ctrl &= ~SDHCI_CLOCK_CTRL_SPI_MODE_CLKEN_OVERRIDE;
>>> -       if (soc_data->nvquirks & SDHCI_MISC_CTRL_ENABLE_SDR50)
>>> -               clk_ctrl |= SDHCI_CLOCK_CTRL_SDR50_TUNING_OVERRIDE;
>>> +
>>> +       /*
>>> +        * If the board does not define a regulator for the SDHCI
>>> +        * IO voltage, then don't advertise support for UHS modes
>>> +        * even if the device supports it because the IO voltage
>>> +        * cannot be configured.
>>> +        */
>>> +       if (of_property_read_bool(dev->of_node, "vqmmc-supply")) {
>>
>> Can't you check whether (!IS_SERR(host->mmc->supply.vqmmc)) instead?
>> The property being present doesn't necessarily guarantee that the
>> regulator has been obtained successfully.
>
> The problem is that the regulator is obtained after the reset occurs and
> so unfortunately that won't work either :-(
>
> I wonder if we should request the regulator during the Tegra SDHCI probe
> before we call sdhci_add_host(). This would allow us to defer the probe
> if it is missing/not present.

That or move the call to regulator_get_supply() before the reset
happens in sdhci_add_host()?

Re: [PATCH] mmc: tegra: Only advertise UHS modes if IO regulator is present

[Stephen Warren]

On 07/06/2016 01:46 AM, Jon Hunter wrote:
>
> On 06/07/16 03:14, Alexandre Courbot wrote:
>> On Tue, Jul 5, 2016 at 11:07 PM, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>>> To support UHS modes for Tegra an external regulator must be present
>>> to adjust the IO voltage accordingly. If the regulator is not present
>>> but the host supports the UHS modes and the SD/MMC device supports the
>>> UHS modes, then we will attempt to switch to a high-speed mode.
>>>
>>> Without an external regulator, Tegra will fail to switch to the
>>> high-speed mode. Although this is expected, it has been found that with
>>> some SD cards, once it has been switched to operate at a high-speed
>>> mode and hence a lower IO voltage, if the IO voltage is not changed then
>>> all subsequent commands issues to the card will fail. Furthermore, it
>>> will not be possible to switch back to a non high-speed mode and so
>>> eventually the SD card initialisation will fail.
>>>
>>> The SDHCI core does not require that the host have an external regulator
>>> when switching to UHS modes and therefore, the Tegra SDHCI host
>>> controller should only advertise the UHS modes as being supported if the
>>> regulator for the IO voltage is present. Fortunately, Tegra has a vendor
>>> specific register which can be used to control which modes are
>>> advertised via the SDHCI_CAPABILITIES register and if the controller is
>>> compatible with v3.0 of the SDHCI specification. Hence, if there is no
>>> IO voltage regulator present in device-tree for the Tegra SDHCI host, then
>>> don't advertise the UHS modes or that the controller is SDHCI v3.0
>>> compatible.
>>>
>>> Note that if the SDHCI is compatible with v3.0 of the SDHCI specification
>>> then this will cause the SDHCI core to read the SDHCI_CAPABILITIES_1
>>> register which will enable other UHS modes.
>>>
>>> This fixes commit 7ad2ed1dfcbe ("mmc: tegra: enable UHS-I modes") which
>>> enables UHS mode without checking if the board can support them.
>>>
>>> Fixes: 7ad2ed1dfcbe ("mmc: tegra: enable UHS-I modes")
>>>
>>> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>> ---
>>>
>>> Please note that I have been reviewing the series from Adrian [0] to
>>> make it easier for drivers to set the capabilities. However, given that
>>> we can control the capabilities that are advertised by the CAPS registers
>>> it seems easier to use this feature.
>>>
>>> [0] http://marc.info/?l=linux-mmc&m=146712062816835&w=2

>>> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c

>>> +
>>> +       /*
>>> +        * If the board does not define a regulator for the SDHCI
>>> +        * IO voltage, then don't advertise support for UHS modes
>>> +        * even if the device supports it because the IO voltage
>>> +        * cannot be configured.
>>> +        */
>>> +       if (of_property_read_bool(dev->of_node, "vqmmc-supply")) {
>>
>> Can't you check whether (!IS_SERR(host->mmc->supply.vqmmc)) instead?
>> The property being present doesn't necessarily guarantee that the
>> regulator has been obtained successfully.
>
> The problem is that the regulator is obtained after the reset occurs and
> so unfortunately that won't work either :-(
>
> I wonder if we should request the regulator during the Tegra SDHCI probe
> before we call sdhci_add_host(). This would allow us to defer the probe
> if it is missing/not present.

Yes, any resource acquisition must happen in probe(). There's no way 
around that.