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@xxxxxxxxxx>
> ---

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