[PATCH v2 01/21] mmc: sdhci: Add helper functions for UHS modes
Aswath Govindraju
a-govindraju at ti.com
Thu Jan 21 05:13:44 CET 2021
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.
> 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.
> 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?
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,
Aswath
More information about the U-Boot
mailing list