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

[Jaehoon Chung <jh80.chung@samsung.com>]

Hi Aswath,

On 1/21/21 1:13 PM, Aswath Govindraju wrote:
> Hi Jaehoon,
> 
> On 21/01/21 4:26 am, Jaehoon Chung wrote:
>> Hi Aswath,
>>
>> On 1/19/21 9:35 PM, Aswath Govindraju wrote:
>>> 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@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.
>>
>> I understood what you said. :)
>> My mean is sdhci_set_control_reg() should be controlled a generic sdhci spec.
>> It doesn't need to assign to callback function. 
>>
>> The default implementation doesn't need to use callback function.
>> It will be used just in sdhci.c by default. callback function will be additionally used for board specific.
>> (s5p_sdhci.c has some specific bits at control register. So i had added the callback ops to control them in board specific driver.)
>>
> 
> [1]
> 
>> When i have checked its patchset again, am654_sdhci can be used the sdhci_set_control_reg() without callback assigned.
>>
>> So it's same sequence the below..
>>
>> sdhci_set_ios -> sdhci_set_contro_reg() in sdhci.c
>> sdhci_set_ios -> ops->set_control_reg() -> called set_control_reg() that is assigned to callback.
>>
>> doesn't it?
>>
> 
> Yes what you mentioned is correct assigning a callback is redundant in
> the case of am654_sdhci. The reason behind doing this is so that this
> patch would not break support for already existing drivers. If the
> implementation that you suggested below is followed then it would
> require follow up patches for all the existing drivers and some drivers
> might not be able to follow this implementation (reason is explained below).
> 
> This way assigning a callback can also be seen in linux kernel mmc host
> drivers,
> 
> For example,
> 
> drivers/mmc/host/sdhci-pxav2.c
> drivers/mmc/host/sdhci-xenon.c
> drivers/mmc/host/sdhci-omap.c
> 
> use function sdhci_pltfm_clk_get_max_clock() as a callback in their
> custom drivers which is a part of generic driver
> drivers/mmc/host/sdhci-pltfm.c.

But i think that sdhci_pltfm_clk_get_max_clock() is not good example.
sdhci-pltfm is provided the generic code of platform side. 

> 
> 
>> Maybe, it can be changed as likes belows
>>
>> index baa935e0d5b0..b0e3ecc6314d 100644
>> --- a/drivers/mmc/am654_sdhci.c
>> +++ b/drivers/mmc/am654_sdhci.c
>> @@ -295,7 +295,6 @@ static int am654_sdhci_deferred_probe(struct sdhci_host *host)
>>  const struct sdhci_ops am654_sdhci_ops = {
>>         .deferred_probe         = am654_sdhci_deferred_probe,
>>         .set_ios_post           = &am654_sdhci_set_ios_post,
>> -       .set_control_reg        = &am654_sdhci_set_control_reg,
>>  };
>>
>>  const struct am654_driver_data am654_drv_data = {
>> diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
>> index 06289343124e..9969a710755e 100644
>> --- a/drivers/mmc/sdhci.c
>> +++ b/drivers/mmc/sdhci.c
>> @@ -509,6 +509,12 @@ void sdhci_set_uhs_timing(struct sdhci_host *host)
>>         sdhci_writew(host, reg, SDHCI_HOST_CONTROL2);
>>  }
>>
>> +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)
>>  {
>> @@ -521,6 +527,9 @@ static int sdhci_set_ios(struct mmc *mmc)
>>         struct sdhci_host *host = mmc->priv;
>>         bool no_hispd_bit = false;
>>
>> +       sdhci_set_control_reg(host);
> 
> [2]
> 
>> +
>> +       /* If it needs to control the boards specific things, implement callback */
>>         if (host->ops && host->ops->set_control_reg)
>>                 host->ops->set_control_reg(host);
>>
>>
>> Then other sdhci host drivers can also used the generic sdhci_set_control_reg() according to SDHCI spec.
>>
> 
> If [2] is added, then all the drivers should use it and not "can use it"
> which would be a constraint while implementing.

Then it can be avoid with if-else?
At this point, you're right. I didn't know exactly, but it makes sense.

> 
>> Well, it's possible that i misunderstood something, if so that, let me know, plz.
>>
> 
> 
> As you have mentioned at [1] the s5p_sdhci.c driver does not implement
> set_control_reg() in the generic way so it should not call
> sdhci_set_control_reg(host) at [2] right?

Right. When i had added set_control_reg callback, u-boot didn't follow mush rather than now about linux kernel.
And it didn't introduce some features at that time.

> 
> 
> So, the main reason for going with the implementation in the patch
> rather than the one suggested by you is to not break support for the
> existing drivers.

Thanks for kindly explanation in more detail!

If possible to resend the patchset based-on latest, i will test with them.
(There are some conflicts.)


Best Regards,
Jaehoon Chung

> 
> 
> Thanks,
> Aswath
>