[PATCH v2 01/21] mmc: sdhci: Add helper functions for UHS modes

[Jaehoon Chung]

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?

> +			regulator_set_enable(mmc->vqmmc_supply, true);
> +		}
> +#endif
> +		mdelay(5);

For what purpose about mdelay(5)?


> +		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.

Best Regards,
Jaehoon Chung

> +{
> +	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 94fc3ed56a..d3f8741042 100644
> --- a/include/sdhci.h
> +++ b/include/sdhci.h
> @@ -492,6 +492,7 @@ 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);
> +void sdhci_set_control_reg(struct sdhci_host *host);
>  extern const struct dm_mmc_ops sdhci_ops;
>  #else
>  #endif
>