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

[Jaehoon Chung]

On 11/5/20 4:05 AM, Faiz Abbas wrote:
> 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://protect2.fireeye.com/v1/url?k=8b617b16-d4fa420c-8b60f059-0cc47a6cba04-71d56f4128587abb&q=1&e=02f975f3-5191-4501-a554-a58145bd6ac1&u=https%3A%2F%2Fgithub.com%2Ftorvalds%2Flinux%2Fblob%2Fmaster%2Fdrivers%2Fmmc%2Fhost%2Fsdhci.c%23L2547
> 
> Not sure if this a part of the spec or not though.

Thanks for sharing info. :)

> 
>>
>>
>>> +		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.

Yes..but when i have checked your code. It doesn't need to assign to callback for your driver.
If sdhci_set_control_reg() is common function about all sdhci-xx driver, then it can be just added in sdhci_set_ios().

callback is for sdhci-xx specific progress.

In my opinion,

static int sdhci_set_ios() 
{
...
	sdhci_set_control_reg();
	if (host->ops && host->ops->set_control_reg)
		host->ops->set_control_reg();

}

if sdhci_set_control_reg() is not generic sequence, it has to be located into am654_sdhci.c
It seems to add sdhci_set_ctonrol_reg() in sdhci.c for your am654_sdhci driver.

Best Regards,
Jaehoon Chung


> 
> Thanks,
> Faiz
>