[PATCH v2 01/21] mmc: sdhci: Add helper functions for UHS modes
Aswath Govindraju
a-govindraju at ti.com
Thu Jan 21 13:46:47 CET 2021
Hi Jaehoon,
On 21/01/21 10:40 am, Jaehoon Chung wrote:
> 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.)
>
I have sent a respin(v3) of this series.
Thanks,
Aswath
More information about the U-Boot
mailing list