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

[Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]

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