[PATCH v2 01/21] mmc: sdhci: Add helper functions for UHS modes
Jaehoon Chung
jh80.chung at samsung.com
Thu Jan 21 06:10:17 CET 2021
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 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.
>>
>> 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
>
More information about the U-Boot
mailing list