[PATCH V2 1/2] mmc: sdhci: Request regulators before reading capabilities

[PATCH V2 1/2] mmc: sdhci: Request regulators before reading capabilities

[Jon Hunter]

The capabilities of the SDHCI host controller are read early during the
SDHCI host initialisation in sdhci_setup_host() and before any
regulators for the host have been requested. This means that if the host
supports some high-speed modes (according to its capabilities register),
but the board cannot because the appropriate voltage regulator is not
available, then the host cannot easily override the capabilities that
are supported.

To allow a SDHCI host controller to determine if it can support high
speed modes via the presense of the MMC regulators, request the
regulators before reading the capabilites of the host controller. This
will allow the SDHCI host to use the 'reset' callback to take the
appropriate action (set flags, configure registers, etc) before the
capabilities register(s) are read.

Please note that some SDHCI hosts, such as the Tegra SDHCI host, has
the ability to mask bits in the capabilities register to prevent
certain capabilities from being advertised.

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

Although this is described as a "V2" this patch has been added since
the original patch was posted.

If the below is deemed not appropriate, then another solution I was
thinking of is to allow the SDHCI host to call 'mmc_regulator_get_supply'
before calling sdhci_setup_host() and then in sdhci_setup_host() check
if any regulators are already present before calling
mmc_regulator_get_supply().

 drivers/mmc/host/sdhci.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 2ee8bfa77116..628c4b3558c0 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -3021,6 +3021,17 @@ int sdhci_setup_host(struct sdhci_host *host)
 
 	mmc = host->mmc;
 
+	/*
+	 * If there are external regulators, get them. Note
+	 * this must be done early before resetting the host
+	 * and reading the capabilities so that the host can
+	 * take the appropriate action if regulators are not
+	 * available.
+	 */
+	ret = mmc_regulator_get_supply(mmc);
+	if (ret == -EPROBE_DEFER)
+		return ret;
+
 	sdhci_read_caps(host);
 
 	override_timeout_clk = host->timeout_clk;
@@ -3253,11 +3264,6 @@ int sdhci_setup_host(struct sdhci_host *host)
 	    mmc_gpio_get_cd(host->mmc) < 0)
 		mmc->caps |= MMC_CAP_NEEDS_POLL;
 
-	/* If there are external regulators, get them */
-	ret = mmc_regulator_get_supply(mmc);
-	if (ret == -EPROBE_DEFER)
-		goto undma;
-
 	/* If vqmmc regulator and no 1.8V signalling, then there's no UHS */
 	if (!IS_ERR(mmc->supply.vqmmc)) {
 		ret = regulator_enable(mmc->supply.vqmmc);
-- 
2.1.4

[PATCH V2 2/2] 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. Even if the regulator is not
present but the host supports the UHS modes and the 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.

It has been found that with some SD cards, that once it has been switch
to operate at a high-speed mode, all subsequent commands issues to the
card will fail and so it will not be possible to switch back to a non
high-speed mode and so 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. Hence, if there is no IO
voltage regulator available for the Tegra SDHCI host, then don't
advertise the UHS modes.

Note that if the regulator is not available, we also don't advertise that
the SDHCI is compatible with v3.0 of the SDHCI specification because
this will 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>
---
 drivers/mmc/host/sdhci-tegra.c | 49 +++++++++++++++++++++++++-----------------
 1 file changed, 29 insertions(+), 20 deletions(-)

diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
index bcc0de47fe7e..bd1199825f9f 100644
--- a/drivers/mmc/host/sdhci-tegra.c
+++ b/drivers/mmc/host/sdhci-tegra.c
@@ -148,28 +148,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 (!IS_ERR(host->mmc->supply.vqmmc)) {
+		/* 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 V2 2/2] mmc: tegra: Only advertise UHS modes if IO regulator is present

[Jon Hunter]

On 12/07/16 14:53, Jon Hunter wrote:
> To support UHS modes for Tegra an external regulator must be present
> to adjust the IO voltage accordingly. Even if the regulator is not
> present but the host supports the UHS modes and the 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.
> 
> It has been found that with some SD cards, that once it has been switch
> to operate at a high-speed mode, all subsequent commands issues to the
> card will fail and so it will not be possible to switch back to a non
> high-speed mode and so 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. Hence, if there is no IO
> voltage regulator available for the Tegra SDHCI host, then don't
> advertise the UHS modes.
> 
> Note that if the regulator is not available, we also don't advertise that
> the SDHCI is compatible with v3.0 of the SDHCI specification because
> this will 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>
> ---

Should have listed V2 changes here which are:

- Updated patch to enable UHS modes if the IO voltage regulator has
  been successfully requested rather than just checking the DT for the
  presence of the regulator (based on Alex C's feedback).

>  drivers/mmc/host/sdhci-tegra.c | 49 +++++++++++++++++++++++++-----------------
>  1 file changed, 29 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
> index bcc0de47fe7e..bd1199825f9f 100644
> --- a/drivers/mmc/host/sdhci-tegra.c
> +++ b/drivers/mmc/host/sdhci-tegra.c
> @@ -148,28 +148,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 (!IS_ERR(host->mmc->supply.vqmmc)) {
> +		/* 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)
> 

-- 
nvpublic

Re: [PATCH V2 1/2] mmc: sdhci: Request regulators before reading capabilities

[Adrian Hunter]

On 12/07/16 16:53, Jon Hunter wrote:
> The capabilities of the SDHCI host controller are read early during the
> SDHCI host initialisation in sdhci_setup_host() and before any
> regulators for the host have been requested. This means that if the host
> supports some high-speed modes (according to its capabilities register),
> but the board cannot because the appropriate voltage regulator is not
> available, then the host cannot easily override the capabilities that
> are supported.
> 
> To allow a SDHCI host controller to determine if it can support high
> speed modes via the presense of the MMC regulators, request the

Try not to confuse High Speed mode with UHS modes.

presense -> presence

> regulators before reading the capabilites of the host controller. This

capabilites -> capabilities

> will allow the SDHCI host to use the 'reset' callback to take the
> appropriate action (set flags, configure registers, etc) before the
> capabilities register(s) are read.
> 
> Please note that some SDHCI hosts, such as the Tegra SDHCI host, has
> the ability to mask bits in the capabilities register to prevent
> certain capabilities from being advertised.
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
> 
> Although this is described as a "V2" this patch has been added since
> the original patch was posted.
> 
> If the below is deemed not appropriate, then another solution I was
> thinking of is to allow the SDHCI host to call 'mmc_regulator_get_supply'
> before calling sdhci_setup_host() and then in sdhci_setup_host() check
> if any regulators are already present before calling
> mmc_regulator_get_supply().

And we can still do that later if we need to.

> 
>  drivers/mmc/host/sdhci.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 2ee8bfa77116..628c4b3558c0 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -3021,6 +3021,17 @@ int sdhci_setup_host(struct sdhci_host *host)
>  
>  	mmc = host->mmc;
>  
> +	/*
> +	 * If there are external regulators, get them. Note
> +	 * this must be done early before resetting the host
> +	 * and reading the capabilities so that the host can
> +	 * take the appropriate action if regulators are not
> +	 * available.
> +	 */
> +	ret = mmc_regulator_get_supply(mmc);
> +	if (ret == -EPROBE_DEFER)
> +		return ret;
> +
>  	sdhci_read_caps(host);
>  
>  	override_timeout_clk = host->timeout_clk;
> @@ -3253,11 +3264,6 @@ int sdhci_setup_host(struct sdhci_host *host)
>  	    mmc_gpio_get_cd(host->mmc) < 0)
>  		mmc->caps |= MMC_CAP_NEEDS_POLL;
>  
> -	/* If there are external regulators, get them */
> -	ret = mmc_regulator_get_supply(mmc);
> -	if (ret == -EPROBE_DEFER)
> -		goto undma;

All goto's before this now need to goto unreg.

> -
>  	/* If vqmmc regulator and no 1.8V signalling, then there's no UHS */
>  	if (!IS_ERR(mmc->supply.vqmmc)) {
>  		ret = regulator_enable(mmc->supply.vqmmc);
> 

Re: [PATCH V2 1/2] mmc: sdhci: Request regulators before reading capabilities

[Adrian Hunter]

On 20/07/16 16:02, Adrian Hunter wrote:
> On 12/07/16 16:53, Jon Hunter wrote:
>> The capabilities of the SDHCI host controller are read early during the
>> SDHCI host initialisation in sdhci_setup_host() and before any
>> regulators for the host have been requested. This means that if the host
>> supports some high-speed modes (according to its capabilities register),
>> but the board cannot because the appropriate voltage regulator is not
>> available, then the host cannot easily override the capabilities that
>> are supported.
>>
>> To allow a SDHCI host controller to determine if it can support high
>> speed modes via the presense of the MMC regulators, request the
> 
> Try not to confuse High Speed mode with UHS modes.
> 
> presense -> presence
> 
>> regulators before reading the capabilites of the host controller. This
> 
> capabilites -> capabilities
> 
>> will allow the SDHCI host to use the 'reset' callback to take the
>> appropriate action (set flags, configure registers, etc) before the
>> capabilities register(s) are read.
>>
>> Please note that some SDHCI hosts, such as the Tegra SDHCI host, has
>> the ability to mask bits in the capabilities register to prevent
>> certain capabilities from being advertised.
>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>> ---
>>
>> Although this is described as a "V2" this patch has been added since
>> the original patch was posted.
>>
>> If the below is deemed not appropriate, then another solution I was
>> thinking of is to allow the SDHCI host to call 'mmc_regulator_get_supply'
>> before calling sdhci_setup_host() and then in sdhci_setup_host() check
>> if any regulators are already present before calling
>> mmc_regulator_get_supply().
> 
> And we can still do that later if we need to.
> 
>>
>>  drivers/mmc/host/sdhci.c | 16 +++++++++++-----
>>  1 file changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 2ee8bfa77116..628c4b3558c0 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -3021,6 +3021,17 @@ int sdhci_setup_host(struct sdhci_host *host)
>>  
>>  	mmc = host->mmc;
>>  
>> +	/*
>> +	 * If there are external regulators, get them. Note
>> +	 * this must be done early before resetting the host
>> +	 * and reading the capabilities so that the host can
>> +	 * take the appropriate action if regulators are not
>> +	 * available.

Also you could spread this comment out to 80 columns.

>> +	 */
>> +	ret = mmc_regulator_get_supply(mmc);
>> +	if (ret == -EPROBE_DEFER)
>> +		return ret;
>> +
>>  	sdhci_read_caps(host);
>>  
>>  	override_timeout_clk = host->timeout_clk;
>> @@ -3253,11 +3264,6 @@ int sdhci_setup_host(struct sdhci_host *host)
>>  	    mmc_gpio_get_cd(host->mmc) < 0)
>>  		mmc->caps |= MMC_CAP_NEEDS_POLL;
>>  
>> -	/* If there are external regulators, get them */
>> -	ret = mmc_regulator_get_supply(mmc);
>> -	if (ret == -EPROBE_DEFER)
>> -		goto undma;
> 
> All goto's before this now need to goto unreg.
> 
>> -
>>  	/* If vqmmc regulator and no 1.8V signalling, then there's no UHS */
>>  	if (!IS_ERR(mmc->supply.vqmmc)) {
>>  		ret = regulator_enable(mmc->supply.vqmmc);
>>
> 
> 

Re: [PATCH V2 1/2] mmc: sdhci: Request regulators before reading capabilities

[Adrian Hunter]

On 20/07/16 16:04, Adrian Hunter wrote:
> On 20/07/16 16:02, Adrian Hunter wrote:
>> On 12/07/16 16:53, Jon Hunter wrote:
>>> The capabilities of the SDHCI host controller are read early during the
>>> SDHCI host initialisation in sdhci_setup_host() and before any
>>> regulators for the host have been requested. This means that if the host
>>> supports some high-speed modes (according to its capabilities register),
>>> but the board cannot because the appropriate voltage regulator is not
>>> available, then the host cannot easily override the capabilities that
>>> are supported.
>>>
>>> To allow a SDHCI host controller to determine if it can support high
>>> speed modes via the presense of the MMC regulators, request the
>>
>> Try not to confuse High Speed mode with UHS modes.
>>
>> presense -> presence
>>
>>> regulators before reading the capabilites of the host controller. This
>>
>> capabilites -> capabilities
>>
>>> will allow the SDHCI host to use the 'reset' callback to take the
>>> appropriate action (set flags, configure registers, etc) before the
>>> capabilities register(s) are read.
>>>
>>> Please note that some SDHCI hosts, such as the Tegra SDHCI host, has
>>> the ability to mask bits in the capabilities register to prevent
>>> certain capabilities from being advertised.
>>>
>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>>> ---
>>>
>>> Although this is described as a "V2" this patch has been added since
>>> the original patch was posted.
>>>
>>> If the below is deemed not appropriate, then another solution I was
>>> thinking of is to allow the SDHCI host to call 'mmc_regulator_get_supply'
>>> before calling sdhci_setup_host() and then in sdhci_setup_host() check
>>> if any regulators are already present before calling
>>> mmc_regulator_get_supply().
>>
>> And we can still do that later if we need to.
>>
>>>
>>>  drivers/mmc/host/sdhci.c | 16 +++++++++++-----
>>>  1 file changed, 11 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index 2ee8bfa77116..628c4b3558c0 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -3021,6 +3021,17 @@ int sdhci_setup_host(struct sdhci_host *host)
>>>  
>>>  	mmc = host->mmc;
>>>  
>>> +	/*
>>> +	 * If there are external regulators, get them. Note
>>> +	 * this must be done early before resetting the host
>>> +	 * and reading the capabilities so that the host can
>>> +	 * take the appropriate action if regulators are not
>>> +	 * available.
> 
> Also you could spread this comment out to 80 columns.
> 
>>> +	 */
>>> +	ret = mmc_regulator_get_supply(mmc);
>>> +	if (ret == -EPROBE_DEFER)
>>> +		return ret;
>>> +
>>>  	sdhci_read_caps(host);
>>>  
>>>  	override_timeout_clk = host->timeout_clk;
>>> @@ -3253,11 +3264,6 @@ int sdhci_setup_host(struct sdhci_host *host)
>>>  	    mmc_gpio_get_cd(host->mmc) < 0)
>>>  		mmc->caps |= MMC_CAP_NEEDS_POLL;
>>>  
>>> -	/* If there are external regulators, get them */
>>> -	ret = mmc_regulator_get_supply(mmc);
>>> -	if (ret == -EPROBE_DEFER)
>>> -		goto undma;
>>
>> All goto's before this now need to goto unreg.

Er, actually they don't!

So apart from the cosmetics above:

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

>>
>>> -
>>>  	/* If vqmmc regulator and no 1.8V signalling, then there's no UHS */
>>>  	if (!IS_ERR(mmc->supply.vqmmc)) {
>>>  		ret = regulator_enable(mmc->supply.vqmmc);
>>>
>>
>>
> 
> 

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

[Adrian Hunter]

On 12/07/16 16:53, Jon Hunter wrote:
> To support UHS modes for Tegra an external regulator must be present
> to adjust the IO voltage accordingly. Even if the regulator is not
> present but the host supports the UHS modes and the 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.
> 
> It has been found that with some SD cards, that once it has been switch
> to operate at a high-speed mode, all subsequent commands issues to the
> card will fail and so it will not be possible to switch back to a non
> high-speed mode and so 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. Hence, if there is no IO
> voltage regulator available for the Tegra SDHCI host, then don't
> advertise the UHS modes.
> 
> Note that if the regulator is not available, we also don't advertise that
> the SDHCI is compatible with v3.0 of the SDHCI specification because
> this will 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>


Acked-by: Adrian Hunter <adrian.hunter@intel.com>


> ---
>  drivers/mmc/host/sdhci-tegra.c | 49 +++++++++++++++++++++++++-----------------
>  1 file changed, 29 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
> index bcc0de47fe7e..bd1199825f9f 100644
> --- a/drivers/mmc/host/sdhci-tegra.c
> +++ b/drivers/mmc/host/sdhci-tegra.c
> @@ -148,28 +148,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 (!IS_ERR(host->mmc->supply.vqmmc)) {
> +		/* 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)
> 

Re: [PATCH V2 1/2] mmc: sdhci: Request regulators before reading capabilities

[Ulf Hansson]

On 12 July 2016 at 15:53, Jon Hunter <jonathanh@nvidia.com> wrote:
> The capabilities of the SDHCI host controller are read early during the
> SDHCI host initialisation in sdhci_setup_host() and before any
> regulators for the host have been requested. This means that if the host
> supports some high-speed modes (according to its capabilities register),
> but the board cannot because the appropriate voltage regulator is not
> available, then the host cannot easily override the capabilities that
> are supported.
>
> To allow a SDHCI host controller to determine if it can support high
> speed modes via the presense of the MMC regulators, request the
> regulators before reading the capabilites of the host controller. This
> will allow the SDHCI host to use the 'reset' callback to take the
> appropriate action (set flags, configure registers, etc) before the
> capabilities register(s) are read.
>
> Please note that some SDHCI hosts, such as the Tegra SDHCI host, has
> the ability to mask bits in the capabilities register to prevent
> certain capabilities from being advertised.
>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>

Thanks, applied for next! I changed the minor things pointed out by Adrian.

Kind regards
Uffe


> ---
>
> Although this is described as a "V2" this patch has been added since
> the original patch was posted.
>
> If the below is deemed not appropriate, then another solution I was
> thinking of is to allow the SDHCI host to call 'mmc_regulator_get_supply'
> before calling sdhci_setup_host() and then in sdhci_setup_host() check
> if any regulators are already present before calling
> mmc_regulator_get_supply().
>
>  drivers/mmc/host/sdhci.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 2ee8bfa77116..628c4b3558c0 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -3021,6 +3021,17 @@ int sdhci_setup_host(struct sdhci_host *host)
>
>         mmc = host->mmc;
>
> +       /*
> +        * If there are external regulators, get them. Note
> +        * this must be done early before resetting the host
> +        * and reading the capabilities so that the host can
> +        * take the appropriate action if regulators are not
> +        * available.
> +        */
> +       ret = mmc_regulator_get_supply(mmc);
> +       if (ret == -EPROBE_DEFER)
> +               return ret;
> +
>         sdhci_read_caps(host);
>
>         override_timeout_clk = host->timeout_clk;
> @@ -3253,11 +3264,6 @@ int sdhci_setup_host(struct sdhci_host *host)
>             mmc_gpio_get_cd(host->mmc) < 0)
>                 mmc->caps |= MMC_CAP_NEEDS_POLL;
>
> -       /* If there are external regulators, get them */
> -       ret = mmc_regulator_get_supply(mmc);
> -       if (ret == -EPROBE_DEFER)
> -               goto undma;
> -
>         /* If vqmmc regulator and no 1.8V signalling, then there's no UHS */
>         if (!IS_ERR(mmc->supply.vqmmc)) {
>                 ret = regulator_enable(mmc->supply.vqmmc);
> --
> 2.1.4
>

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

[Ulf Hansson]

On 12 July 2016 at 15:53, Jon Hunter <jonathanh@nvidia.com> wrote:
> To support UHS modes for Tegra an external regulator must be present
> to adjust the IO voltage accordingly. Even if the regulator is not
> present but the host supports the UHS modes and the 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.
>
> It has been found that with some SD cards, that once it has been switch
> to operate at a high-speed mode, all subsequent commands issues to the
> card will fail and so it will not be possible to switch back to a non
> high-speed mode and so 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. Hence, if there is no IO
> voltage regulator available for the Tegra SDHCI host, then don't
> advertise the UHS modes.
>
> Note that if the regulator is not available, we also don't advertise that
> the SDHCI is compatible with v3.0 of the SDHCI specification because
> this will 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>

Thanks, applied for next!

Kind regards
Uffe


> ---
>  drivers/mmc/host/sdhci-tegra.c | 49 +++++++++++++++++++++++++-----------------
>  1 file changed, 29 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
> index bcc0de47fe7e..bd1199825f9f 100644
> --- a/drivers/mmc/host/sdhci-tegra.c
> +++ b/drivers/mmc/host/sdhci-tegra.c
> @@ -148,28 +148,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 (!IS_ERR(host->mmc->supply.vqmmc)) {
> +               /* 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
>