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_ab...@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.
Best Regards,
Jaehoon Chung
>
> Thanks,
> Faiz
>
