[PATCH v3 01/20] mmc: sdhci: Add helper functions for UHS modes

[Aswath Govindraju]

Hi Simon,

On 24/01/21 7:33 am, Simon Glass wrote:
> Hi Aswath,
> 
> On Thu, 21 Jan 2021 at 05:41, Aswath Govindraju <a-govindraju at ti.com> wrote:
>>
>> From: Faiz Abbas <faiz_abbas at ti.com>
>>
>> Add a set_voltage() function which handles the switch from 3.3V to 1.8V
>> for SD card UHS modes.
>>
>> Signed-off-by: Faiz Abbas <faiz_abbas at ti.com>
>> Signed-off-by: Aswath Govindraju <a-govindraju at ti.com>
>> ---
>>  drivers/mmc/sdhci.c | 80 +++++++++++++++++++++++++++++++++++++++++++++
>>  include/sdhci.h     |  1 +
>>  2 files changed, 81 insertions(+)
>>
>> diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
>> index 06289343124e..e7e3e22d1a7e 100644
>> --- a/drivers/mmc/sdhci.c
>> +++ b/drivers/mmc/sdhci.c
>> @@ -20,6 +20,7 @@
>>  #include <linux/delay.h>
>>  #include <linux/dma-mapping.h>
>>  #include <phys2bus.h>
>> +#include <power/regulator.h>
>>
>>  static void sdhci_reset(struct sdhci_host *host, u8 mask)
>>  {
>> @@ -509,6 +510,85 @@ void sdhci_set_uhs_timing(struct sdhci_host *host)
>>         sdhci_writew(host, reg, SDHCI_HOST_CONTROL2);
>>  }
>>
>> +#if CONFIG_IS_ENABLED(MMC_IO_VOLTAGE)

[1]

>> +static void sdhci_set_voltage(struct sdhci_host *host)
>> +{
>> +       struct mmc *mmc = (struct mmc *)host->mmc;
>> +       u32 ctrl;
>> +       int ret;
>> +
>> +       ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>> +
>> +       switch (mmc->signal_voltage) {
>> +       case MMC_SIGNAL_VOLTAGE_330:
>> +#if CONFIG_IS_ENABLED(DM_REGULATOR)
> 
> Can you use if(), please? You should see a patman warning about this.

Here if() can not be used as a pre-processor directive is necessary. In
include/mmc.h the field vqmmc_supply in the structure mmc is defined
inside #if CONFIG_IS_ENABLED(DM_REGULATOR). So, when DM_REGULATOR config
is not enabled then the field vqmmc_supply is not defined which would
result in a error.

The #if at [1] can be converted to if(IS_ENABLED()). I will convert this
in the respin.

> 
>> +               if (mmc->vqmmc_supply) {
>> +                       ret = regulator_set_enable(mmc->vqmmc_supply, false);
>> +                       if (ret) {
>> +                               pr_err("failed to disable vqmmc-supply: %d\n", ret);
>> +                               return;
>> +                       }
>> +
>> +                       ret = regulator_set_value(mmc->vqmmc_supply, 3300000);
>> +                       if (ret) {
>> +                               pr_err("failed to set vqmmc-voltage to 3.3V: %d\n", ret);
>> +                               return;
>> +                       }
>> +
>> +                       ret = regulator_set_enable(mmc->vqmmc_supply, true);
>> +                       if (ret) {
>> +                               pr_err("failed to enable vqmmc-supply: %d\n", ret);
>> +                               return;
>> +                       }
>> +               }
>> +#endif
>> +               mdelay(5);
>> +               if (IS_SD(mmc)) {
>> +                       ctrl &= ~SDHCI_CTRL_VDD_180;
>> +                       sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>> +               }
>> +               break;
>> +       case MMC_SIGNAL_VOLTAGE_180:
>> +#if CONFIG_IS_ENABLED(DM_REGULATOR)
>> +               if (mmc->vqmmc_supply) {
>> +                       regulator_set_enable(mmc->vqmmc_supply, false);
>> +                       if (ret) {
>> +                               pr_err("failed to disable vqmmc-supply: %d\n", ret);
>> +                               return;
>> +                       }
>> +
>> +                       regulator_set_value(mmc->vqmmc_supply, 1800000);
>> +                       if (ret) {
>> +                               pr_err("failed to set vqmmc-voltage to 1.8V: %d\n", ret);
>> +                               return;
>> +                       }
>> +
>> +                       regulator_set_enable(mmc->vqmmc_supply, true);
>> +                       if (ret) {
>> +                               pr_err("failed to enable vqmmc-supply: %d\n", ret);
>> +                               return;
>> +                       }
>> +               }
>> +#endif
>> +               if (IS_SD(mmc)) {
>> +                       ctrl |= SDHCI_CTRL_VDD_180;
>> +                       sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>> +               }
>> +               break;
>> +       default:
>> +               /* No signal voltage switch required */
>> +               return;
>> +       }
>> +}
>> +#else
>> +static void sdhci_set_voltage(struct sdhci_host *host) { }
>> +#endif
>> +void sdhci_set_control_reg(struct sdhci_host *host)
>> +{
>> +       sdhci_set_voltage(host);
>> +       sdhci_set_uhs_timing(host);
>> +}
>> +
>>  #ifdef CONFIG_DM_MMC
>>  static int sdhci_set_ios(struct udevice *dev)
>>  {
>> diff --git a/include/sdhci.h b/include/sdhci.h
>> index 3e5a64981857..0f820c6d2669 100644
>> --- a/include/sdhci.h
>> +++ b/include/sdhci.h
>> @@ -491,6 +491,7 @@ void sdhci_set_uhs_timing(struct sdhci_host *host);
>>  /* Export the operations to drivers */
>>  int sdhci_probe(struct udevice *dev);
>>  int sdhci_set_clock(struct mmc *mmc, unsigned int clock);
>> +void sdhci_set_control_reg(struct sdhci_host *host);
> 
> This needs a comment. What does it do?
> 

I will add a comment to describe this in the respin.

Thank you for the comments.

Regards,
Aswath

>>  extern const struct dm_mmc_ops sdhci_ops;
>>  #else
>>  #endif
>> --
>> 2.17.1
>>
> 
> Regards,
> Simon
>