Hi Aswath,
On 2/4/21 2:09 PM, Aswath Govindraju wrote:
> Hi Jaehoon,
>
> On 04/02/21 4:23 am, Jaehoon Chung wrote:
>> Hi Aswath,
>>
>> On 2/3/21 3:06 PM, Aswath Govindraju wrote:
>>> Hi Jaehoon,
>>>
>>> On 02/02/21 3:40 am, Jaehoon Chung wrote:
>>>> Hi Aswath,
>>>>
>>>> On 1/29/21 11:47 PM, Aswath Govindraju wrote:
>>>>> From: Faiz Abbas <faiz_ab...@ti.com>
>>>>>
>>>>> 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>
>>>>> Signed-off-by: Aswath Govindraju <a-govindr...@ti.com>
>>>>> ---
>>>>> drivers/mmc/sdhci.c | 73 +++++++++++++++++++++++++++++++++++++++++++++
>>>>> include/sdhci.h | 10 +++++++
>>>>> 2 files changed, 83 insertions(+)
>>>>>
>>>>> diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
>>>>> index 06289343124e..276b6a08e571 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)
>>>>> {
>>>>> @@ -509,6 +510,78 @@ void sdhci_set_uhs_timing(struct sdhci_host *host)
>>>>> sdhci_writew(host, reg, SDHCI_HOST_CONTROL2);
>>>>> }
>>>>>
>>>>> +static void sdhci_set_voltage(struct sdhci_host *host)
>>>>> +{
>>>>> + if (IS_ENABLED(CONFIG_MMC_IO_VOLTAGE)) {
>>>>> + 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) {
>>>>> + if
>>>>> (regulator_set_enable_if_allowed(mmc->vqmmc_supply, false)) {
>>>>> + pr_err("failed to disable
>>>>> vqmmc-supply\n");
>>>>> + return;
>>>>> + }
>>>>> +
>>>>> + if (regulator_set_value(mmc->vqmmc_supply,
>>>>> 3300000)) {
>>>>> + pr_err("failed to set vqmmc-voltage to
>>>>> 3.3V\n");
>>>>> + return;
>>>>> + }
>>>>> +
>>>>> + if
>>>>> (regulator_set_enable_if_allowed(mmc->vqmmc_supply, true)) {
>>>>> + pr_err("failed to enable
>>>>> vqmmc-supply\n");
>>>>> + return;
>>>>> + }
>>>>> + }
>>>>> +#endif
>>>>> + /* 3.3V regulator output should be stable within 5 ms */
>>>>> + mdelay(5);
>>>>> + if (IS_SD(mmc)) {
>>>>> + ctrl &= ~SDHCI_CTRL_VDD_180;
>>>>> + sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>>>>> + }
>>>>
>>>> According to Specification, after Signal bit was changed to 0 from 1, it
>>>> needs to wait for stable within 5ms.
>>>> Isn't it right that mdelay(5) locates at here?
>>>>
>>>
>>> I searched the spec for this and found the following line in it,
>>>
>>> ```
>>> If this status is not supported, Host Driver should take more than 5ms for
>>> stable time of host voltage regulator from changing 1.8V Signaling
>>> Enable. Specific Host Driver may use a specific time, which is provided by
>>> Host System, instead of using 5ms.
>>>
>>> ```
>>>
>>> This is from the row of "Host Regulator Voltage Stable" in table
>>> describing "Present state register", "PartA2_SD
>>> Host_Controller_Simplified_Specification_Ver4.20" document, page 66.
>>>
>>> The status mentioned is about host regulator voltage stable. Yes I can
>>> see that it has been mentioned that host driver has to wait for 5ms
>>> after changing the 1.8V signal enable bit. In that case I suppose the
>>> same has to be done at [1] as well right ?
>>
>> Right, i think that it needs to be stable within 5ms.
>>
>> One more, does it needs to check whether card is SD or not?
>> I think that it doesn't need to check it. Because eMMC needs to switch 1.8V
>> when its mode is upper than DDR, doesn't it?
>>
>
> Voltage switching is not allowed for eMMC and to ensure it, the above
> mentioned check is done. eMMC operates at the same voltage from the start.
I don't remember correctly. :) So i need to check more after time.
I don't have any objection about this patch. If you re-send patch v6, you can
add my reviewed-tag, plz.
Reviewed-by: Jaehoon Chung <jh80.ch...@samsung.com>
Best Regards,
Jaehoon Chung
>
> Thanks,
> Aswath
>
>>>
>>>>> + break;
>>>>> + case MMC_SIGNAL_VOLTAGE_180:
>>>>> +#if CONFIG_IS_ENABLED(DM_REGULATOR)
>>>>> + if (mmc->vqmmc_supply) {
>>>>> + if
>>>>> (regulator_set_enable_if_allowed(mmc->vqmmc_supply, false)) {
>>>>> + pr_err("failed to disable
>>>>> vqmmc-supply\n");
>>>>> + return;
>>>>> + }
>>>>> +
>>>>> + if (regulator_set_value(mmc->vqmmc_supply,
>>>>> 1800000)) {
>>>>> + pr_err("failed to set vqmmc-voltage to
>>>>> 1.8V\n");
>>>>> + return;
>>>>> + }
>>>>> +
>>>>> + if
>>>>> (regulator_set_enable_if_allowed(mmc->vqmmc_supply, true)) {
>>>>> + pr_err("failed to enable
>>>>> vqmmc-supply\n");
>>>>> + return;
>>>>> + }
>>>>> + }
>>>>> +#endif
>>>>> + if (IS_SD(mmc)) {
>>>>> + ctrl |= SDHCI_CTRL_VDD_180;
>>>>> + sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>>>>> + }
>>>
>>> [1]
>>>
>>> For reference, I have also attached the page from where the above line
>>> was taken.
>>>
>>> Thank you for pointing it out. I will post a re-spin fixing it.
>>>
>>> Thanks,
>>> Aswath
>>>
>>>
>>>>> + break;
>>>>> + default:
>>>>> + /* No signal voltage switch required */
>>>>> + return;
>>>>> + }
>>>>> + }
>>>>> +}
>>>>> +
>>>>> +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)
>>>>> {
>>>>> diff --git a/include/sdhci.h b/include/sdhci.h
>>>>> index 3e5a64981857..0ae9471ad749 100644
>>>>> --- a/include/sdhci.h
>>>>> +++ b/include/sdhci.h
>>>>> @@ -491,6 +491,16 @@ void sdhci_set_uhs_timing(struct sdhci_host *host);
>>>>> /* Export the operations to drivers */
>>>>> int sdhci_probe(struct udevice *dev);
>>>>> int sdhci_set_clock(struct mmc *mmc, unsigned int clock);
>>>>> +
>>>>> +/**
>>>>> + * sdhci_set_control_reg - Set control registers
>>>>> + *
>>>>> + * This is used set up control registers for voltage level and UHS speed
>>>>> + * mode.
>>>>> + *
>>>>> + * @host: SDHCI host structure
>>>>> + */
>>>>> +void sdhci_set_control_reg(struct sdhci_host *host);
>>>>> extern const struct dm_mmc_ops sdhci_ops;
>>>>> #else
>>>>> #endif
>>>>>
>>>>
>>>
>>
>
>
