[PATCH v2 01/21] mmc: sdhci: Add helper functions for UHS modes

[Faiz Abbas]

Jaehoon,

On 21/10/20 5:08 pm, Jaehoon Chung wrote:
> Hi Faiz,
> 
> On 10/16/20 8:08 PM, Faiz Abbas wrote:
>> 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>
>> ---
>>  drivers/mmc/sdhci.c | 51 +++++++++++++++++++++++++++++++++++++++++++++
>>  include/sdhci.h     |  1 +
>>  2 files changed, 52 insertions(+)
>>
>> diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
>> index 7673219fb3..a69f058191 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)
>>  {
>> @@ -556,6 +557,56 @@ void sdhci_set_uhs_timing(struct sdhci_host *host)
>>  	sdhci_writew(host, reg, SDHCI_HOST_CONTROL2);
>>  }
>>  
>> +#if CONFIG_IS_ENABLED(MMC_IO_VOLTAGE)
>> +static void sdhci_set_voltage(struct sdhci_host *host)
>> +{
>> +	struct mmc *mmc = (struct mmc *)host->mmc;
>> +	u32 ctrl;
>> +
>> +	ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>> +
>> +	switch (mmc->signal_voltage) {
>> +	case MMC_SIGNAL_VOLTAGE_330:
>> +#if CONFIG_IS_ENABLED(DM_REGULATOR)
>> +		if (mmc->vqmmc_supply) {
>> +			regulator_set_enable(mmc->vqmmc_supply, false);
>> +			regulator_set_value(mmc->vqmmc_supply, 3300000);
> 
> Doesn't need to consider about fail to set its value?

You're right. I'll handle the failure case in v3.

> 
>> +			regulator_set_enable(mmc->vqmmc_supply, true);
>> +		}
>> +#endif
>> +		mdelay(5);
> 
> For what purpose about mdelay(5)?

I'm following this from the kernel implementation here:
https://github.com/torvalds/linux/blob/master/drivers/mmc/host/sdhci.c#L2547

Not sure if this a part of the spec or not though.

> 
> 
>> +		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);
>> +			regulator_set_value(mmc->vqmmc_supply, 1800000);
>> +			regulator_set_enable(mmc->vqmmc_supply, true);
>> +		}
>> +#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)
> 
> this function is called as callback function in sdhci_set_ios(). 
> it's strange... set_control_reg callback is for host specific control register.
> 
> I think that it doesn't need to assign to callback.

This is the default set_control_reg() implementation which is defined
in the sdhci spec. Any host that that wants default implementation
case assign this as their callback.

Hosts that have custom implementations can add in their own drivers.

Thanks,
Faiz