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

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

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