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

[Aswath Govindraju]

Hi Jaehoon,

On 05/11/20 4:03 am, Jaehoon Chung wrote:
> 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.
> 


As Faiz has mentioned sdhci_set_control_reg() added here is the default
implementation according to the spec. One other driver apart from
drivers/mmc/am654_sdhci.c that implements set_control_reg() similar to
the above implementation is drivers/mmc/zynq_sdhci.c and there are some
which implement it differently for example drivers/mmc/s5p_sdhci.c and
drivers/mmc/sdhci-cadence.c.

So, if a host driver wants use the default implementation then it can
use the one implemented in the sdhci.c or it can implement its own
implementation the way in which all the drivers have done so far.


Thanks,
Aswath