diff mbox series

[v1] mmc: sdhci_am654: Add sdhci_am654_start_signal_voltage_switch

Message ID 20240906175032.1580281-1-jm@ti.com (mailing list archive)
State New
Headers show
Series [v1] mmc: sdhci_am654: Add sdhci_am654_start_signal_voltage_switch | expand

Commit Message

Judith Mendez Sept. 6, 2024, 5:50 p.m. UTC
The sdhci_start_signal_voltage_switch function sets
V1P8_SIGNAL_ENA by default after switching to 1v8 signaling.
V1P8_SIGNAL_ENA determines whether to launch cmd/data on neg
edge or pos edge of clock.

Due to some eMMC and SD failures seen across am62x platform,
do not set V1P8_SIGNAL_ENA by default, only enable the bit
for devices that require this bit in order to switch to 1v8
voltage for uhs modes.

Signed-off-by: Judith Mendez <jm@ti.com>
---
 drivers/mmc/host/sdhci_am654.c | 86 ++++++++++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)


base-commit: cf6444ba528f043398b112ac36e041a4d8685cb1

Comments

kernel test robot Sept. 7, 2024, 4:14 p.m. UTC | #1
Hi Judith,

kernel test robot noticed the following build warnings:

[auto build test WARNING on cf6444ba528f043398b112ac36e041a4d8685cb1]

url:    https://github.com/intel-lab-lkp/linux/commits/Judith-Mendez/mmc-sdhci_am654-Add-sdhci_am654_start_signal_voltage_switch/20240907-015144
base:   cf6444ba528f043398b112ac36e041a4d8685cb1
patch link:    https://lore.kernel.org/r/20240906175032.1580281-1-jm%40ti.com
patch subject: [PATCH v1] mmc: sdhci_am654: Add sdhci_am654_start_signal_voltage_switch
config: openrisc-allyesconfig (https://download.01.org/0day-ci/archive/20240907/202409072350.685m24kB-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240907/202409072350.685m24kB-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409072350.685m24kB-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/mmc/host/sdhci_am654.c:360:5: warning: no previous prototype for 'sdhci_am654_start_signal_voltage_switch' [-Wmissing-prototypes]
     360 | int sdhci_am654_start_signal_voltage_switch(struct mmc_host *mmc,
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +/sdhci_am654_start_signal_voltage_switch +360 drivers/mmc/host/sdhci_am654.c

   359	
 > 360	int sdhci_am654_start_signal_voltage_switch(struct mmc_host *mmc,
   361						    struct mmc_ios *ios)
   362	{
   363		struct sdhci_host *host = mmc_priv(mmc);
   364		struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
   365		struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
   366		u16 ctrl;
   367		int ret;
   368	
   369		if (host->version < SDHCI_SPEC_300)
   370			return 0;
   371	
   372		switch (ios->signal_voltage) {
   373		case MMC_SIGNAL_VOLTAGE_330:
   374			if (!(host->flags & SDHCI_SIGNALING_330))
   375				return -EINVAL;
   376	
   377			ctrl &= ~SDHCI_CTRL_VDD_180;
   378			sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
   379	
   380			if (!IS_ERR(mmc->supply.vqmmc)) {
   381				ret = mmc_regulator_set_vqmmc(mmc, ios);
   382				if (ret < 0) {
   383					pr_warn("%s: Switching to 3.3V signalling voltage failed\n",
   384						mmc_hostname(mmc));
   385					return -EIO;
   386				}
   387			}
   388	
   389			usleep_range(5000, 5500);
   390	
   391			ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
   392			if (!(ctrl & SDHCI_CTRL_VDD_180))
   393				return 0;
   394	
   395			pr_warn("%s: 3.3V regulator output did not become stable\n",
   396				mmc_hostname(mmc));
   397	
   398			return -EAGAIN;
   399	
   400		case MMC_SIGNAL_VOLTAGE_180:
   401			if (!(host->flags & SDHCI_SIGNALING_180))
   402				return -EINVAL;
   403	
   404			if (!IS_ERR(mmc->supply.vqmmc)) {
   405				ret = mmc_regulator_set_vqmmc(mmc, ios);
   406				if (ret < 0) {
   407					pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
   408						mmc_hostname(mmc));
   409					return -EIO;
   410				}
   411			}
   412	
   413			if (sdhci_am654->quirks & SDHCI_AM654_QUIRK_SET_V1P8_ENA) {
   414				ctrl |= SDHCI_CTRL_VDD_180;
   415				sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
   416	
   417				ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
   418				if (ctrl & SDHCI_CTRL_VDD_180)
   419					return 0;
   420	
   421				pr_warn("%s: 1.8V regulator output did not become stable\n",
   422					mmc_hostname(mmc));
   423	
   424				return -EAGAIN;
   425			}
   426			return 0;
   427	
   428		default:
   429			return 0;
   430		}
   431	}
   432
kernel test robot Sept. 7, 2024, 4:55 p.m. UTC | #2
Hi Judith,

kernel test robot noticed the following build warnings:

[auto build test WARNING on cf6444ba528f043398b112ac36e041a4d8685cb1]

url:    https://github.com/intel-lab-lkp/linux/commits/Judith-Mendez/mmc-sdhci_am654-Add-sdhci_am654_start_signal_voltage_switch/20240907-015144
base:   cf6444ba528f043398b112ac36e041a4d8685cb1
patch link:    https://lore.kernel.org/r/20240906175032.1580281-1-jm%40ti.com
patch subject: [PATCH v1] mmc: sdhci_am654: Add sdhci_am654_start_signal_voltage_switch
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20240908/202409080031.zgsuKKXB-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240908/202409080031.zgsuKKXB-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409080031.zgsuKKXB-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/mmc/host/sdhci_am654.c:360:5: warning: no previous prototype for function 'sdhci_am654_start_signal_voltage_switch' [-Wmissing-prototypes]
     360 | int sdhci_am654_start_signal_voltage_switch(struct mmc_host *mmc,
         |     ^
   drivers/mmc/host/sdhci_am654.c:360:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
     360 | int sdhci_am654_start_signal_voltage_switch(struct mmc_host *mmc,
         | ^
         | static 
>> drivers/mmc/host/sdhci_am654.c:377:3: warning: variable 'ctrl' is uninitialized when used here [-Wuninitialized]
     377 |                 ctrl &= ~SDHCI_CTRL_VDD_180;
         |                 ^~~~
   drivers/mmc/host/sdhci_am654.c:366:10: note: initialize the variable 'ctrl' to silence this warning
     366 |         u16 ctrl;
         |                 ^
         |                  = 0
   2 warnings generated.


vim +/sdhci_am654_start_signal_voltage_switch +360 drivers/mmc/host/sdhci_am654.c

   359	
 > 360	int sdhci_am654_start_signal_voltage_switch(struct mmc_host *mmc,
   361						    struct mmc_ios *ios)
   362	{
   363		struct sdhci_host *host = mmc_priv(mmc);
   364		struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
   365		struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
   366		u16 ctrl;
   367		int ret;
   368	
   369		if (host->version < SDHCI_SPEC_300)
   370			return 0;
   371	
   372		switch (ios->signal_voltage) {
   373		case MMC_SIGNAL_VOLTAGE_330:
   374			if (!(host->flags & SDHCI_SIGNALING_330))
   375				return -EINVAL;
   376	
 > 377			ctrl &= ~SDHCI_CTRL_VDD_180;
   378			sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
   379	
   380			if (!IS_ERR(mmc->supply.vqmmc)) {
   381				ret = mmc_regulator_set_vqmmc(mmc, ios);
   382				if (ret < 0) {
   383					pr_warn("%s: Switching to 3.3V signalling voltage failed\n",
   384						mmc_hostname(mmc));
   385					return -EIO;
   386				}
   387			}
   388	
   389			usleep_range(5000, 5500);
   390	
   391			ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
   392			if (!(ctrl & SDHCI_CTRL_VDD_180))
   393				return 0;
   394	
   395			pr_warn("%s: 3.3V regulator output did not become stable\n",
   396				mmc_hostname(mmc));
   397	
   398			return -EAGAIN;
   399	
   400		case MMC_SIGNAL_VOLTAGE_180:
   401			if (!(host->flags & SDHCI_SIGNALING_180))
   402				return -EINVAL;
   403	
   404			if (!IS_ERR(mmc->supply.vqmmc)) {
   405				ret = mmc_regulator_set_vqmmc(mmc, ios);
   406				if (ret < 0) {
   407					pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
   408						mmc_hostname(mmc));
   409					return -EIO;
   410				}
   411			}
   412	
   413			if (sdhci_am654->quirks & SDHCI_AM654_QUIRK_SET_V1P8_ENA) {
   414				ctrl |= SDHCI_CTRL_VDD_180;
   415				sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
   416	
   417				ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
   418				if (ctrl & SDHCI_CTRL_VDD_180)
   419					return 0;
   420	
   421				pr_warn("%s: 1.8V regulator output did not become stable\n",
   422					mmc_hostname(mmc));
   423	
   424				return -EAGAIN;
   425			}
   426			return 0;
   427	
   428		default:
   429			return 0;
   430		}
   431	}
   432
Adrian Hunter Sept. 9, 2024, 6:26 a.m. UTC | #3
On 6/09/24 20:50, Judith Mendez wrote:
> The sdhci_start_signal_voltage_switch function sets
> V1P8_SIGNAL_ENA by default after switching to 1v8 signaling.
> V1P8_SIGNAL_ENA determines whether to launch cmd/data on neg
> edge or pos edge of clock.
> 
> Due to some eMMC and SD failures seen across am62x platform,
> do not set V1P8_SIGNAL_ENA by default, only enable the bit
> for devices that require this bit in order to switch to 1v8
> voltage for uhs modes.
> 
> Signed-off-by: Judith Mendez <jm@ti.com>
> ---
>  drivers/mmc/host/sdhci_am654.c | 86 ++++++++++++++++++++++++++++++++++
>  1 file changed, 86 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> index 0aa3c40ea6ed8..fb6232e56606b 100644
> --- a/drivers/mmc/host/sdhci_am654.c
> +++ b/drivers/mmc/host/sdhci_am654.c
> @@ -155,6 +155,7 @@ struct sdhci_am654_data {
>  	u32 tuning_loop;
>  
>  #define SDHCI_AM654_QUIRK_FORCE_CDTEST BIT(0)
> +#define SDHCI_AM654_QUIRK_SET_V1P8_ENA BIT(1)

It would be better for the quirk to represent the deviation
from normal i.e. 

#define SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA BIT(1)

>  };
>  
>  struct window {
> @@ -356,6 +357,79 @@ static void sdhci_j721e_4bit_set_clock(struct sdhci_host *host,
>  	sdhci_set_clock(host, clock);
>  }
>  
> +int sdhci_am654_start_signal_voltage_switch(struct mmc_host *mmc,
> +					    struct mmc_ios *ios)

Simpler to call sdhci_start_signal_voltage_switch() for the normal
case e.g.

static int sdhci_am654_start_signal_voltage_switch(struct mmc_host *mmc, struct mmc_ios *ios)
{
	struct sdhci_host *host = mmc_priv(mmc);
	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
	struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);


	if ((sdhci_am654->quirks & SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA) &&
	    ios->signal_voltage == MMC_SIGNAL_VOLTAGE_180) {
		ret = mmc_regulator_set_vqmmc(mmc, ios);
		if (ret < 0) {
			pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
				mmc_hostname(mmc));
			return -EIO;
		}
		return 0;
	}

	return sdhci_start_signal_voltage_switch(mmc, ios);
}

> +{
> +	struct sdhci_host *host = mmc_priv(mmc);
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
> +	u16 ctrl;
> +	int ret;
> +
> +	if (host->version < SDHCI_SPEC_300)
> +		return 0;
> +
> +	switch (ios->signal_voltage) {
> +	case MMC_SIGNAL_VOLTAGE_330:
> +		if (!(host->flags & SDHCI_SIGNALING_330))
> +			return -EINVAL;
> +
> +		ctrl &= ~SDHCI_CTRL_VDD_180;
> +		sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
> +
> +		if (!IS_ERR(mmc->supply.vqmmc)) {
> +			ret = mmc_regulator_set_vqmmc(mmc, ios);
> +			if (ret < 0) {
> +				pr_warn("%s: Switching to 3.3V signalling voltage failed\n",
> +					mmc_hostname(mmc));
> +				return -EIO;
> +			}
> +		}
> +
> +		usleep_range(5000, 5500);
> +
> +		ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> +		if (!(ctrl & SDHCI_CTRL_VDD_180))
> +			return 0;
> +
> +		pr_warn("%s: 3.3V regulator output did not become stable\n",
> +			mmc_hostname(mmc));
> +
> +		return -EAGAIN;
> +
> +	case MMC_SIGNAL_VOLTAGE_180:
> +		if (!(host->flags & SDHCI_SIGNALING_180))
> +			return -EINVAL;
> +
> +		if (!IS_ERR(mmc->supply.vqmmc)) {
> +			ret = mmc_regulator_set_vqmmc(mmc, ios);
> +			if (ret < 0) {
> +				pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
> +					mmc_hostname(mmc));
> +				return -EIO;
> +			}
> +		}
> +
> +		if (sdhci_am654->quirks & SDHCI_AM654_QUIRK_SET_V1P8_ENA) {
> +			ctrl |= SDHCI_CTRL_VDD_180;
> +			sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
> +
> +			ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> +			if (ctrl & SDHCI_CTRL_VDD_180)
> +				return 0;
> +
> +			pr_warn("%s: 1.8V regulator output did not become stable\n",
> +				mmc_hostname(mmc));
> +
> +			return -EAGAIN;
> +		}
> +		return 0;
> +
> +	default:
> +		return 0;
> +	}
> +}
> +
>  static u8 sdhci_am654_write_power_on(struct sdhci_host *host, u8 val, int reg)
>  {
>  	writeb(val, host->ioaddr + reg);
> @@ -801,6 +875,8 @@ static int sdhci_am654_get_of_property(struct platform_device *pdev,
>  					struct sdhci_am654_data *sdhci_am654)
>  {
>  	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct device_node *node;
>  	int drv_strength;
>  	int ret;
>  
> @@ -844,6 +920,15 @@ static int sdhci_am654_get_of_property(struct platform_device *pdev,
>  	if (device_property_read_bool(dev, "ti,fails-without-test-cd"))
>  		sdhci_am654->quirks |= SDHCI_AM654_QUIRK_FORCE_CDTEST;
>  
> +	node = of_parse_phandle(np, "vmmc-supply", 0);
> +
> +	if (node) {
> +		node = of_parse_phandle(np, "vqmmc-supply", 0);
> +
> +		if (!node)
> +			sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SET_V1P8_ENA;
> +	}

It would be simpler without 'np' and 'node'.  Not sure
what the rule is meant to be, but it could be something like:

	if (of_parse_phandle(dev->of_node, "vmmc-supply", 0) &&
	    of_parse_phandle(dev->of_node, "vqmmc-supply", 0)
		sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA;

> +
>  	sdhci_get_of_property(pdev);
>  
>  	return 0;
> @@ -940,6 +1025,7 @@ static int sdhci_am654_probe(struct platform_device *pdev)
>  		goto err_pltfm_free;
>  	}
>  
> +	host->mmc_host_ops.start_signal_voltage_switch = sdhci_am654_start_signal_voltage_switch;
>  	host->mmc_host_ops.execute_tuning = sdhci_am654_execute_tuning;
>  
>  	pm_runtime_get_noresume(dev);
> 
> base-commit: cf6444ba528f043398b112ac36e041a4d8685cb1
Judith Mendez Sept. 10, 2024, 2:30 p.m. UTC | #4
Hi Adrian,

On 9/9/24 1:26 AM, Adrian Hunter wrote:
> On 6/09/24 20:50, Judith Mendez wrote:
>> The sdhci_start_signal_voltage_switch function sets
>> V1P8_SIGNAL_ENA by default after switching to 1v8 signaling.
>> V1P8_SIGNAL_ENA determines whether to launch cmd/data on neg
>> edge or pos edge of clock.
>>
>> Due to some eMMC and SD failures seen across am62x platform,
>> do not set V1P8_SIGNAL_ENA by default, only enable the bit
>> for devices that require this bit in order to switch to 1v8
>> voltage for uhs modes.
>>
>> Signed-off-by: Judith Mendez <jm@ti.com>
>> ---
>>   drivers/mmc/host/sdhci_am654.c | 86 ++++++++++++++++++++++++++++++++++
>>   1 file changed, 86 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
>> index 0aa3c40ea6ed8..fb6232e56606b 100644
>> --- a/drivers/mmc/host/sdhci_am654.c
>> +++ b/drivers/mmc/host/sdhci_am654.c
>> @@ -155,6 +155,7 @@ struct sdhci_am654_data {
>>   	u32 tuning_loop;
>>   
>>   #define SDHCI_AM654_QUIRK_FORCE_CDTEST BIT(0)
>> +#define SDHCI_AM654_QUIRK_SET_V1P8_ENA BIT(1)
> 
> It would be better for the quirk to represent the deviation
> from normal i.e.
> 
> #define SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA BIT(1)
> 
>>   };
>>   
>>   struct window {
>> @@ -356,6 +357,79 @@ static void sdhci_j721e_4bit_set_clock(struct sdhci_host *host,
>>   	sdhci_set_clock(host, clock);
>>   }
>>   
>> +int sdhci_am654_start_signal_voltage_switch(struct mmc_host *mmc,
>> +					    struct mmc_ios *ios)
> 
> Simpler to call sdhci_start_signal_voltage_switch() for the normal
> case e.g.

This is simpler, so sure will use thanks.

> 
> static int sdhci_am654_start_signal_voltage_switch(struct mmc_host *mmc, struct mmc_ios *ios)
> {
> 	struct sdhci_host *host = mmc_priv(mmc);
> 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> 	struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
> 
> 
> 	if ((sdhci_am654->quirks & SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA) &&
> 	    ios->signal_voltage == MMC_SIGNAL_VOLTAGE_180) {
> 		ret = mmc_regulator_set_vqmmc(mmc, ios);
> 		if (ret < 0) {
> 			pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
> 				mmc_hostname(mmc));
> 			return -EIO;
> 		}
> 		return 0;
> 	}
> 
> 	return sdhci_start_signal_voltage_switch(mmc, ios);
> }
> 
>> +{
>> +	struct sdhci_host *host = mmc_priv(mmc);
>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +	struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
>> +	u16 ctrl;
>> +	int ret;
>> +
>> +	if (host->version < SDHCI_SPEC_300)
>> +		return 0;
>> +
>> +	switch (ios->signal_voltage) {
>> +	case MMC_SIGNAL_VOLTAGE_330:
>> +		if (!(host->flags & SDHCI_SIGNALING_330))
>> +			return -EINVAL;
>> +
>> +		ctrl &= ~SDHCI_CTRL_VDD_180;
>> +		sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>> +
>> +		if (!IS_ERR(mmc->supply.vqmmc)) {
>> +			ret = mmc_regulator_set_vqmmc(mmc, ios);
>> +			if (ret < 0) {
>> +				pr_warn("%s: Switching to 3.3V signalling voltage failed\n",
>> +					mmc_hostname(mmc));
>> +				return -EIO;
>> +			}
>> +		}
>> +
>> +		usleep_range(5000, 5500);
>> +
>> +		ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>> +		if (!(ctrl & SDHCI_CTRL_VDD_180))
>> +			return 0;
>> +
>> +		pr_warn("%s: 3.3V regulator output did not become stable\n",
>> +			mmc_hostname(mmc));
>> +
>> +		return -EAGAIN;
>> +
>> +	case MMC_SIGNAL_VOLTAGE_180:
>> +		if (!(host->flags & SDHCI_SIGNALING_180))
>> +			return -EINVAL;
>> +
>> +		if (!IS_ERR(mmc->supply.vqmmc)) {
>> +			ret = mmc_regulator_set_vqmmc(mmc, ios);
>> +			if (ret < 0) {
>> +				pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
>> +					mmc_hostname(mmc));
>> +				return -EIO;
>> +			}
>> +		}
>> +
>> +		if (sdhci_am654->quirks & SDHCI_AM654_QUIRK_SET_V1P8_ENA) {
>> +			ctrl |= SDHCI_CTRL_VDD_180;
>> +			sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>> +
>> +			ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>> +			if (ctrl & SDHCI_CTRL_VDD_180)
>> +				return 0;
>> +
>> +			pr_warn("%s: 1.8V regulator output did not become stable\n",
>> +				mmc_hostname(mmc));
>> +
>> +			return -EAGAIN;
>> +		}
>> +		return 0;
>> +
>> +	default:
>> +		return 0;
>> +	}
>> +}
>> +
>>   static u8 sdhci_am654_write_power_on(struct sdhci_host *host, u8 val, int reg)
>>   {
>>   	writeb(val, host->ioaddr + reg);
>> @@ -801,6 +875,8 @@ static int sdhci_am654_get_of_property(struct platform_device *pdev,
>>   					struct sdhci_am654_data *sdhci_am654)
>>   {
>>   	struct device *dev = &pdev->dev;
>> +	struct device_node *np = dev->of_node;
>> +	struct device_node *node;
>>   	int drv_strength;
>>   	int ret;
>>   
>> @@ -844,6 +920,15 @@ static int sdhci_am654_get_of_property(struct platform_device *pdev,
>>   	if (device_property_read_bool(dev, "ti,fails-without-test-cd"))
>>   		sdhci_am654->quirks |= SDHCI_AM654_QUIRK_FORCE_CDTEST;
>>   
>> +	node = of_parse_phandle(np, "vmmc-supply", 0);
>> +
>> +	if (node) {
>> +		node = of_parse_phandle(np, "vqmmc-supply", 0);
>> +
>> +		if (!node)
>> +			sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SET_V1P8_ENA;
>> +	}
> 
> It would be simpler without 'np' and 'node'.  Not sure
> what the rule is meant to be, but it could be something like:
> 
> 	if (of_parse_phandle(dev->of_node, "vmmc-supply", 0) &&
> 	    of_parse_phandle(dev->of_node, "vqmmc-supply", 0)
> 		sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA;

My issue with this is that I also need the quirk 
(SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA) for eMMC. DT node for eMMC does
not include vmmc and vqmmc supplies. That is why I had the quirk logic
inverted.

This patch fixes timing issues with both eMMC and SD. (:

~ Judith


> 
>> +
>>   	sdhci_get_of_property(pdev);
>>   
>>   	return 0;
>> @@ -940,6 +1025,7 @@ static int sdhci_am654_probe(struct platform_device *pdev)
>>   		goto err_pltfm_free;
>>   	}
>>   
>> +	host->mmc_host_ops.start_signal_voltage_switch = sdhci_am654_start_signal_voltage_switch;
>>   	host->mmc_host_ops.execute_tuning = sdhci_am654_execute_tuning;
>>   
>>   	pm_runtime_get_noresume(dev);
>>
>> base-commit: cf6444ba528f043398b112ac36e041a4d8685cb1
> 
>
Adrian Hunter Sept. 10, 2024, 5:10 p.m. UTC | #5
On 10/09/24 17:30, Judith Mendez wrote:
> Hi Adrian,
> 
> On 9/9/24 1:26 AM, Adrian Hunter wrote:
>> On 6/09/24 20:50, Judith Mendez wrote:
>>> The sdhci_start_signal_voltage_switch function sets
>>> V1P8_SIGNAL_ENA by default after switching to 1v8 signaling.
>>> V1P8_SIGNAL_ENA determines whether to launch cmd/data on neg
>>> edge or pos edge of clock.
>>>
>>> Due to some eMMC and SD failures seen across am62x platform,
>>> do not set V1P8_SIGNAL_ENA by default, only enable the bit
>>> for devices that require this bit in order to switch to 1v8
>>> voltage for uhs modes.
>>>
>>> Signed-off-by: Judith Mendez <jm@ti.com>
>>> ---
>>>   drivers/mmc/host/sdhci_am654.c | 86 ++++++++++++++++++++++++++++++++++
>>>   1 file changed, 86 insertions(+)
>>>
>>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
>>> index 0aa3c40ea6ed8..fb6232e56606b 100644
>>> --- a/drivers/mmc/host/sdhci_am654.c
>>> +++ b/drivers/mmc/host/sdhci_am654.c
>>> @@ -155,6 +155,7 @@ struct sdhci_am654_data {
>>>       u32 tuning_loop;
>>>     #define SDHCI_AM654_QUIRK_FORCE_CDTEST BIT(0)
>>> +#define SDHCI_AM654_QUIRK_SET_V1P8_ENA BIT(1)
>>
>> It would be better for the quirk to represent the deviation
>> from normal i.e.
>>
>> #define SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA BIT(1)
>>
>>>   };
>>>     struct window {
>>> @@ -356,6 +357,79 @@ static void sdhci_j721e_4bit_set_clock(struct sdhci_host *host,
>>>       sdhci_set_clock(host, clock);
>>>   }
>>>   +int sdhci_am654_start_signal_voltage_switch(struct mmc_host *mmc,
>>> +                        struct mmc_ios *ios)
>>
>> Simpler to call sdhci_start_signal_voltage_switch() for the normal
>> case e.g.
> 
> This is simpler, so sure will use thanks.
> 
>>
>> static int sdhci_am654_start_signal_voltage_switch(struct mmc_host *mmc, struct mmc_ios *ios)
>> {
>>     struct sdhci_host *host = mmc_priv(mmc);
>>     struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>     struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
>>
>>
>>     if ((sdhci_am654->quirks & SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA) &&
>>         ios->signal_voltage == MMC_SIGNAL_VOLTAGE_180) {
>>         ret = mmc_regulator_set_vqmmc(mmc, ios);
>>         if (ret < 0) {
>>             pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
>>                 mmc_hostname(mmc));
>>             return -EIO;
>>         }
>>         return 0;
>>     }
>>
>>     return sdhci_start_signal_voltage_switch(mmc, ios);
>> }
>>
>>> +{
>>> +    struct sdhci_host *host = mmc_priv(mmc);
>>> +    struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>> +    struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
>>> +    u16 ctrl;
>>> +    int ret;
>>> +
>>> +    if (host->version < SDHCI_SPEC_300)
>>> +        return 0;
>>> +
>>> +    switch (ios->signal_voltage) {
>>> +    case MMC_SIGNAL_VOLTAGE_330:
>>> +        if (!(host->flags & SDHCI_SIGNALING_330))
>>> +            return -EINVAL;
>>> +
>>> +        ctrl &= ~SDHCI_CTRL_VDD_180;
>>> +        sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>>> +
>>> +        if (!IS_ERR(mmc->supply.vqmmc)) {
>>> +            ret = mmc_regulator_set_vqmmc(mmc, ios);
>>> +            if (ret < 0) {
>>> +                pr_warn("%s: Switching to 3.3V signalling voltage failed\n",
>>> +                    mmc_hostname(mmc));
>>> +                return -EIO;
>>> +            }
>>> +        }
>>> +
>>> +        usleep_range(5000, 5500);
>>> +
>>> +        ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>>> +        if (!(ctrl & SDHCI_CTRL_VDD_180))
>>> +            return 0;
>>> +
>>> +        pr_warn("%s: 3.3V regulator output did not become stable\n",
>>> +            mmc_hostname(mmc));
>>> +
>>> +        return -EAGAIN;
>>> +
>>> +    case MMC_SIGNAL_VOLTAGE_180:
>>> +        if (!(host->flags & SDHCI_SIGNALING_180))
>>> +            return -EINVAL;
>>> +
>>> +        if (!IS_ERR(mmc->supply.vqmmc)) {
>>> +            ret = mmc_regulator_set_vqmmc(mmc, ios);
>>> +            if (ret < 0) {
>>> +                pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
>>> +                    mmc_hostname(mmc));
>>> +                return -EIO;
>>> +            }
>>> +        }
>>> +
>>> +        if (sdhci_am654->quirks & SDHCI_AM654_QUIRK_SET_V1P8_ENA) {
>>> +            ctrl |= SDHCI_CTRL_VDD_180;
>>> +            sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>>> +
>>> +            ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>>> +            if (ctrl & SDHCI_CTRL_VDD_180)
>>> +                return 0;
>>> +
>>> +            pr_warn("%s: 1.8V regulator output did not become stable\n",
>>> +                mmc_hostname(mmc));
>>> +
>>> +            return -EAGAIN;
>>> +        }
>>> +        return 0;
>>> +
>>> +    default:
>>> +        return 0;
>>> +    }
>>> +}
>>> +
>>>   static u8 sdhci_am654_write_power_on(struct sdhci_host *host, u8 val, int reg)
>>>   {
>>>       writeb(val, host->ioaddr + reg);
>>> @@ -801,6 +875,8 @@ static int sdhci_am654_get_of_property(struct platform_device *pdev,
>>>                       struct sdhci_am654_data *sdhci_am654)
>>>   {
>>>       struct device *dev = &pdev->dev;
>>> +    struct device_node *np = dev->of_node;
>>> +    struct device_node *node;
>>>       int drv_strength;
>>>       int ret;
>>>   @@ -844,6 +920,15 @@ static int sdhci_am654_get_of_property(struct platform_device *pdev,
>>>       if (device_property_read_bool(dev, "ti,fails-without-test-cd"))
>>>           sdhci_am654->quirks |= SDHCI_AM654_QUIRK_FORCE_CDTEST;
>>>   +    node = of_parse_phandle(np, "vmmc-supply", 0);
>>> +
>>> +    if (node) {
>>> +        node = of_parse_phandle(np, "vqmmc-supply", 0);
>>> +
>>> +        if (!node)
>>> +            sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SET_V1P8_ENA;
>>> +    }
>>
>> It would be simpler without 'np' and 'node'.  Not sure
>> what the rule is meant to be, but it could be something like:
>>
>>     if (of_parse_phandle(dev->of_node, "vmmc-supply", 0) &&
>>         of_parse_phandle(dev->of_node, "vqmmc-supply", 0)
>>         sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA;
> 
> My issue with this is that I also need the quirk (SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA) for eMMC. DT node for eMMC does
> not include vmmc and vqmmc supplies. That is why I had the quirk logic
> inverted.

Ideally there would be a more direct way to distinguish eMMC, but
otherwise, having both supplies or neither would be:

	if (!!of_parse_phandle(dev->of_node, "vmmc-supply", 0) ==
	    !!of_parse_phandle(dev->of_node, "vqmmc-supply", 0))
		sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA;


> 
> This patch fixes timing issues with both eMMC and SD. (:
> 
> ~ Judith
> 
> 
>>
>>> +
>>>       sdhci_get_of_property(pdev);
>>>         return 0;
>>> @@ -940,6 +1025,7 @@ static int sdhci_am654_probe(struct platform_device *pdev)
>>>           goto err_pltfm_free;
>>>       }
>>>   +    host->mmc_host_ops.start_signal_voltage_switch = sdhci_am654_start_signal_voltage_switch;
>>>       host->mmc_host_ops.execute_tuning = sdhci_am654_execute_tuning;
>>>         pm_runtime_get_noresume(dev);
>>>
>>> base-commit: cf6444ba528f043398b112ac36e041a4d8685cb1
>>
>>
>
Judith Mendez Sept. 10, 2024, 11:22 p.m. UTC | #6
Hi Adrian,

On 9/10/24 12:10 PM, Adrian Hunter wrote:
> On 10/09/24 17:30, Judith Mendez wrote:
>> Hi Adrian,
>>
>> On 9/9/24 1:26 AM, Adrian Hunter wrote:
>>> On 6/09/24 20:50, Judith Mendez wrote:
>>>> The sdhci_start_signal_voltage_switch function sets
>>>> V1P8_SIGNAL_ENA by default after switching to 1v8 signaling.
>>>> V1P8_SIGNAL_ENA determines whether to launch cmd/data on neg
>>>> edge or pos edge of clock.
>>>>
>>>> Due to some eMMC and SD failures seen across am62x platform,
>>>> do not set V1P8_SIGNAL_ENA by default, only enable the bit
>>>> for devices that require this bit in order to switch to 1v8
>>>> voltage for uhs modes.
>>>>
>>>> Signed-off-by: Judith Mendez <jm@ti.com>
>>>> ---
>>>>    drivers/mmc/host/sdhci_am654.c | 86 ++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 86 insertions(+)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
>>>> index 0aa3c40ea6ed8..fb6232e56606b 100644
>>>> --- a/drivers/mmc/host/sdhci_am654.c
>>>> +++ b/drivers/mmc/host/sdhci_am654.c
>>>> @@ -155,6 +155,7 @@ struct sdhci_am654_data {
>>>>        u32 tuning_loop;
>>>>      #define SDHCI_AM654_QUIRK_FORCE_CDTEST BIT(0)
>>>> +#define SDHCI_AM654_QUIRK_SET_V1P8_ENA BIT(1)
>>>
>>> It would be better for the quirk to represent the deviation
>>> from normal i.e.
>>>
>>> #define SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA BIT(1)
>>>
>>>>    };
>>>>      struct window {
>>>> @@ -356,6 +357,79 @@ static void sdhci_j721e_4bit_set_clock(struct sdhci_host *host,
>>>>        sdhci_set_clock(host, clock);
>>>>    }
>>>>    +int sdhci_am654_start_signal_voltage_switch(struct mmc_host *mmc,
>>>> +                        struct mmc_ios *ios)
>>>
>>> Simpler to call sdhci_start_signal_voltage_switch() for the normal
>>> case e.g.
>>
>> This is simpler, so sure will use thanks.
>>
>>>
>>> static int sdhci_am654_start_signal_voltage_switch(struct mmc_host *mmc, struct mmc_ios *ios)
>>> {
>>>      struct sdhci_host *host = mmc_priv(mmc);
>>>      struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>      struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
>>>
>>>
>>>      if ((sdhci_am654->quirks & SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA) &&
>>>          ios->signal_voltage == MMC_SIGNAL_VOLTAGE_180) {
>>>          ret = mmc_regulator_set_vqmmc(mmc, ios);
>>>          if (ret < 0) {
>>>              pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
>>>                  mmc_hostname(mmc));
>>>              return -EIO;
>>>          }
>>>          return 0;
>>>      }
>>>
>>>      return sdhci_start_signal_voltage_switch(mmc, ios);
>>> }
>>>
>>>> +{
>>>> +    struct sdhci_host *host = mmc_priv(mmc);
>>>> +    struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>> +    struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
>>>> +    u16 ctrl;
>>>> +    int ret;
>>>> +
>>>> +    if (host->version < SDHCI_SPEC_300)
>>>> +        return 0;
>>>> +
>>>> +    switch (ios->signal_voltage) {
>>>> +    case MMC_SIGNAL_VOLTAGE_330:
>>>> +        if (!(host->flags & SDHCI_SIGNALING_330))
>>>> +            return -EINVAL;
>>>> +
>>>> +        ctrl &= ~SDHCI_CTRL_VDD_180;
>>>> +        sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>>>> +
>>>> +        if (!IS_ERR(mmc->supply.vqmmc)) {
>>>> +            ret = mmc_regulator_set_vqmmc(mmc, ios);
>>>> +            if (ret < 0) {
>>>> +                pr_warn("%s: Switching to 3.3V signalling voltage failed\n",
>>>> +                    mmc_hostname(mmc));
>>>> +                return -EIO;
>>>> +            }
>>>> +        }
>>>> +
>>>> +        usleep_range(5000, 5500);
>>>> +
>>>> +        ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>>>> +        if (!(ctrl & SDHCI_CTRL_VDD_180))
>>>> +            return 0;
>>>> +
>>>> +        pr_warn("%s: 3.3V regulator output did not become stable\n",
>>>> +            mmc_hostname(mmc));
>>>> +
>>>> +        return -EAGAIN;
>>>> +
>>>> +    case MMC_SIGNAL_VOLTAGE_180:
>>>> +        if (!(host->flags & SDHCI_SIGNALING_180))
>>>> +            return -EINVAL;
>>>> +
>>>> +        if (!IS_ERR(mmc->supply.vqmmc)) {
>>>> +            ret = mmc_regulator_set_vqmmc(mmc, ios);
>>>> +            if (ret < 0) {
>>>> +                pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
>>>> +                    mmc_hostname(mmc));
>>>> +                return -EIO;
>>>> +            }
>>>> +        }
>>>> +
>>>> +        if (sdhci_am654->quirks & SDHCI_AM654_QUIRK_SET_V1P8_ENA) {
>>>> +            ctrl |= SDHCI_CTRL_VDD_180;
>>>> +            sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>>>> +
>>>> +            ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>>>> +            if (ctrl & SDHCI_CTRL_VDD_180)
>>>> +                return 0;
>>>> +
>>>> +            pr_warn("%s: 1.8V regulator output did not become stable\n",
>>>> +                mmc_hostname(mmc));
>>>> +
>>>> +            return -EAGAIN;
>>>> +        }
>>>> +        return 0;
>>>> +
>>>> +    default:
>>>> +        return 0;
>>>> +    }
>>>> +}
>>>> +
>>>>    static u8 sdhci_am654_write_power_on(struct sdhci_host *host, u8 val, int reg)
>>>>    {
>>>>        writeb(val, host->ioaddr + reg);
>>>> @@ -801,6 +875,8 @@ static int sdhci_am654_get_of_property(struct platform_device *pdev,
>>>>                        struct sdhci_am654_data *sdhci_am654)
>>>>    {
>>>>        struct device *dev = &pdev->dev;
>>>> +    struct device_node *np = dev->of_node;
>>>> +    struct device_node *node;
>>>>        int drv_strength;
>>>>        int ret;
>>>>    @@ -844,6 +920,15 @@ static int sdhci_am654_get_of_property(struct platform_device *pdev,
>>>>        if (device_property_read_bool(dev, "ti,fails-without-test-cd"))
>>>>            sdhci_am654->quirks |= SDHCI_AM654_QUIRK_FORCE_CDTEST;
>>>>    +    node = of_parse_phandle(np, "vmmc-supply", 0);
>>>> +
>>>> +    if (node) {
>>>> +        node = of_parse_phandle(np, "vqmmc-supply", 0);
>>>> +
>>>> +        if (!node)
>>>> +            sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SET_V1P8_ENA;
>>>> +    }
>>>
>>> It would be simpler without 'np' and 'node'.  Not sure
>>> what the rule is meant to be, but it could be something like:
>>>
>>>      if (of_parse_phandle(dev->of_node, "vmmc-supply", 0) &&
>>>          of_parse_phandle(dev->of_node, "vqmmc-supply", 0)
>>>          sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA;
>>
>> My issue with this is that I also need the quirk (SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA) for eMMC. DT node for eMMC does
>> not include vmmc and vqmmc supplies. That is why I had the quirk logic
>> inverted.
> 
> Ideally there would be a more direct way to distinguish eMMC, but
> otherwise, having both supplies or neither would be:
> 
> 	if (!!of_parse_phandle(dev->of_node, "vmmc-supply", 0) ==
> 	    !!of_parse_phandle(dev->of_node, "vqmmc-supply", 0))
> 		sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA;


Not sure I love the double NOT, but ok, I can use this, will fix for v2.

Thanks for your suggestion!

~ Judith

> 
> 
>>
>> This patch fixes timing issues with both eMMC and SD. (:
>>
>> ~ Judith
>>
>>
>>>
>>>> +
>>>>        sdhci_get_of_property(pdev);
>>>>          return 0;
>>>> @@ -940,6 +1025,7 @@ static int sdhci_am654_probe(struct platform_device *pdev)
>>>>            goto err_pltfm_free;
>>>>        }
>>>>    +    host->mmc_host_ops.start_signal_voltage_switch = sdhci_am654_start_signal_voltage_switch;
>>>>        host->mmc_host_ops.execute_tuning = sdhci_am654_execute_tuning;
>>>>          pm_runtime_get_noresume(dev);
>>>>
>>>> base-commit: cf6444ba528f043398b112ac36e041a4d8685cb1
>>>
>>>
>>
>
Adrian Hunter Sept. 11, 2024, 5:17 a.m. UTC | #7
On 11/09/24 02:22, Judith Mendez wrote:
> Hi Adrian,
> 
> On 9/10/24 12:10 PM, Adrian Hunter wrote:
>> On 10/09/24 17:30, Judith Mendez wrote:
>>> Hi Adrian,
>>>
>>> On 9/9/24 1:26 AM, Adrian Hunter wrote:
>>>> On 6/09/24 20:50, Judith Mendez wrote:
>>>>> The sdhci_start_signal_voltage_switch function sets
>>>>> V1P8_SIGNAL_ENA by default after switching to 1v8 signaling.
>>>>> V1P8_SIGNAL_ENA determines whether to launch cmd/data on neg
>>>>> edge or pos edge of clock.
>>>>>
>>>>> Due to some eMMC and SD failures seen across am62x platform,
>>>>> do not set V1P8_SIGNAL_ENA by default, only enable the bit
>>>>> for devices that require this bit in order to switch to 1v8
>>>>> voltage for uhs modes.
>>>>>
>>>>> Signed-off-by: Judith Mendez <jm@ti.com>
>>>>> ---
>>>>>    drivers/mmc/host/sdhci_am654.c | 86 ++++++++++++++++++++++++++++++++++
>>>>>    1 file changed, 86 insertions(+)
>>>>>
>>>>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
>>>>> index 0aa3c40ea6ed8..fb6232e56606b 100644
>>>>> --- a/drivers/mmc/host/sdhci_am654.c
>>>>> +++ b/drivers/mmc/host/sdhci_am654.c
>>>>> @@ -155,6 +155,7 @@ struct sdhci_am654_data {
>>>>>        u32 tuning_loop;
>>>>>      #define SDHCI_AM654_QUIRK_FORCE_CDTEST BIT(0)
>>>>> +#define SDHCI_AM654_QUIRK_SET_V1P8_ENA BIT(1)
>>>>
>>>> It would be better for the quirk to represent the deviation
>>>> from normal i.e.
>>>>
>>>> #define SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA BIT(1)
>>>>
>>>>>    };
>>>>>      struct window {
>>>>> @@ -356,6 +357,79 @@ static void sdhci_j721e_4bit_set_clock(struct sdhci_host *host,
>>>>>        sdhci_set_clock(host, clock);
>>>>>    }
>>>>>    +int sdhci_am654_start_signal_voltage_switch(struct mmc_host *mmc,
>>>>> +                        struct mmc_ios *ios)
>>>>
>>>> Simpler to call sdhci_start_signal_voltage_switch() for the normal
>>>> case e.g.
>>>
>>> This is simpler, so sure will use thanks.
>>>
>>>>
>>>> static int sdhci_am654_start_signal_voltage_switch(struct mmc_host *mmc, struct mmc_ios *ios)
>>>> {
>>>>      struct sdhci_host *host = mmc_priv(mmc);
>>>>      struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>>      struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
>>>>
>>>>
>>>>      if ((sdhci_am654->quirks & SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA) &&
>>>>          ios->signal_voltage == MMC_SIGNAL_VOLTAGE_180) {
>>>>          ret = mmc_regulator_set_vqmmc(mmc, ios);
>>>>          if (ret < 0) {
>>>>              pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
>>>>                  mmc_hostname(mmc));
>>>>              return -EIO;
>>>>          }
>>>>          return 0;
>>>>      }
>>>>
>>>>      return sdhci_start_signal_voltage_switch(mmc, ios);
>>>> }
>>>>
>>>>> +{
>>>>> +    struct sdhci_host *host = mmc_priv(mmc);
>>>>> +    struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>>> +    struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
>>>>> +    u16 ctrl;
>>>>> +    int ret;
>>>>> +
>>>>> +    if (host->version < SDHCI_SPEC_300)
>>>>> +        return 0;
>>>>> +
>>>>> +    switch (ios->signal_voltage) {
>>>>> +    case MMC_SIGNAL_VOLTAGE_330:
>>>>> +        if (!(host->flags & SDHCI_SIGNALING_330))
>>>>> +            return -EINVAL;
>>>>> +
>>>>> +        ctrl &= ~SDHCI_CTRL_VDD_180;
>>>>> +        sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>>>>> +
>>>>> +        if (!IS_ERR(mmc->supply.vqmmc)) {
>>>>> +            ret = mmc_regulator_set_vqmmc(mmc, ios);
>>>>> +            if (ret < 0) {
>>>>> +                pr_warn("%s: Switching to 3.3V signalling voltage failed\n",
>>>>> +                    mmc_hostname(mmc));
>>>>> +                return -EIO;
>>>>> +            }
>>>>> +        }
>>>>> +
>>>>> +        usleep_range(5000, 5500);
>>>>> +
>>>>> +        ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>>>>> +        if (!(ctrl & SDHCI_CTRL_VDD_180))
>>>>> +            return 0;
>>>>> +
>>>>> +        pr_warn("%s: 3.3V regulator output did not become stable\n",
>>>>> +            mmc_hostname(mmc));
>>>>> +
>>>>> +        return -EAGAIN;
>>>>> +
>>>>> +    case MMC_SIGNAL_VOLTAGE_180:
>>>>> +        if (!(host->flags & SDHCI_SIGNALING_180))
>>>>> +            return -EINVAL;
>>>>> +
>>>>> +        if (!IS_ERR(mmc->supply.vqmmc)) {
>>>>> +            ret = mmc_regulator_set_vqmmc(mmc, ios);
>>>>> +            if (ret < 0) {
>>>>> +                pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
>>>>> +                    mmc_hostname(mmc));
>>>>> +                return -EIO;
>>>>> +            }
>>>>> +        }
>>>>> +
>>>>> +        if (sdhci_am654->quirks & SDHCI_AM654_QUIRK_SET_V1P8_ENA) {
>>>>> +            ctrl |= SDHCI_CTRL_VDD_180;
>>>>> +            sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>>>>> +
>>>>> +            ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>>>>> +            if (ctrl & SDHCI_CTRL_VDD_180)
>>>>> +                return 0;
>>>>> +
>>>>> +            pr_warn("%s: 1.8V regulator output did not become stable\n",
>>>>> +                mmc_hostname(mmc));
>>>>> +
>>>>> +            return -EAGAIN;
>>>>> +        }
>>>>> +        return 0;
>>>>> +
>>>>> +    default:
>>>>> +        return 0;
>>>>> +    }
>>>>> +}
>>>>> +
>>>>>    static u8 sdhci_am654_write_power_on(struct sdhci_host *host, u8 val, int reg)
>>>>>    {
>>>>>        writeb(val, host->ioaddr + reg);
>>>>> @@ -801,6 +875,8 @@ static int sdhci_am654_get_of_property(struct platform_device *pdev,
>>>>>                        struct sdhci_am654_data *sdhci_am654)
>>>>>    {
>>>>>        struct device *dev = &pdev->dev;
>>>>> +    struct device_node *np = dev->of_node;
>>>>> +    struct device_node *node;
>>>>>        int drv_strength;
>>>>>        int ret;
>>>>>    @@ -844,6 +920,15 @@ static int sdhci_am654_get_of_property(struct platform_device *pdev,
>>>>>        if (device_property_read_bool(dev, "ti,fails-without-test-cd"))
>>>>>            sdhci_am654->quirks |= SDHCI_AM654_QUIRK_FORCE_CDTEST;
>>>>>    +    node = of_parse_phandle(np, "vmmc-supply", 0);
>>>>> +
>>>>> +    if (node) {
>>>>> +        node = of_parse_phandle(np, "vqmmc-supply", 0);
>>>>> +
>>>>> +        if (!node)
>>>>> +            sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SET_V1P8_ENA;
>>>>> +    }
>>>>
>>>> It would be simpler without 'np' and 'node'.  Not sure
>>>> what the rule is meant to be, but it could be something like:
>>>>
>>>>      if (of_parse_phandle(dev->of_node, "vmmc-supply", 0) &&
>>>>          of_parse_phandle(dev->of_node, "vqmmc-supply", 0)
>>>>          sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA;
>>>
>>> My issue with this is that I also need the quirk (SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA) for eMMC. DT node for eMMC does
>>> not include vmmc and vqmmc supplies. That is why I had the quirk logic
>>> inverted.
>>
>> Ideally there would be a more direct way to distinguish eMMC, but
>> otherwise, having both supplies or neither would be:
>>
>>     if (!!of_parse_phandle(dev->of_node, "vmmc-supply", 0) ==
>>         !!of_parse_phandle(dev->of_node, "vqmmc-supply", 0))
>>         sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA;
> 
> 
> Not sure I love the double NOT, but ok, I can use this, will fix for v2.

It could use a comment, including about the eMMC thing.

> 
> Thanks for your suggestion!
> 
> ~ Judith
> 
>>
>>
>>>
>>> This patch fixes timing issues with both eMMC and SD. (:
>>>
>>> ~ Judith
>>>
>>>
>>>>
>>>>> +
>>>>>        sdhci_get_of_property(pdev);
>>>>>          return 0;
>>>>> @@ -940,6 +1025,7 @@ static int sdhci_am654_probe(struct platform_device *pdev)
>>>>>            goto err_pltfm_free;
>>>>>        }
>>>>>    +    host->mmc_host_ops.start_signal_voltage_switch = sdhci_am654_start_signal_voltage_switch;
>>>>>        host->mmc_host_ops.execute_tuning = sdhci_am654_execute_tuning;
>>>>>          pm_runtime_get_noresume(dev);
>>>>>
>>>>> base-commit: cf6444ba528f043398b112ac36e041a4d8685cb1
>>>>
>>>>
>>>
>>
>
Judith Mendez Sept. 11, 2024, 2:34 p.m. UTC | #8
Hi Adrian,

On 9/11/24 12:17 AM, Adrian Hunter wrote:
> On 11/09/24 02:22, Judith Mendez wrote:
>> Hi Adrian,
>>
>> On 9/10/24 12:10 PM, Adrian Hunter wrote:
>>> On 10/09/24 17:30, Judith Mendez wrote:
>>>> Hi Adrian,
>>>>
>>>> On 9/9/24 1:26 AM, Adrian Hunter wrote:
>>>>> On 6/09/24 20:50, Judith Mendez wrote:
>>>>>> The sdhci_start_signal_voltage_switch function sets
>>>>>> V1P8_SIGNAL_ENA by default after switching to 1v8 signaling.
>>>>>> V1P8_SIGNAL_ENA determines whether to launch cmd/data on neg
>>>>>> edge or pos edge of clock.
>>>>>>
>>>>>> Due to some eMMC and SD failures seen across am62x platform,
>>>>>> do not set V1P8_SIGNAL_ENA by default, only enable the bit
>>>>>> for devices that require this bit in order to switch to 1v8
>>>>>> voltage for uhs modes.
>>>>>>
>>>>>> Signed-off-by: Judith Mendez <jm@ti.com>
>>>>>> ---
>>>>>>     drivers/mmc/host/sdhci_am654.c | 86 ++++++++++++++++++++++++++++++++++
>>>>>>     1 file changed, 86 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
>>>>>> index 0aa3c40ea6ed8..fb6232e56606b 100644
>>>>>> --- a/drivers/mmc/host/sdhci_am654.c
>>>>>> +++ b/drivers/mmc/host/sdhci_am654.c
>>>>>> @@ -155,6 +155,7 @@ struct sdhci_am654_data {
>>>>>>         u32 tuning_loop;
>>>>>>       #define SDHCI_AM654_QUIRK_FORCE_CDTEST BIT(0)
>>>>>> +#define SDHCI_AM654_QUIRK_SET_V1P8_ENA BIT(1)
>>>>>
>>>>> It would be better for the quirk to represent the deviation
>>>>> from normal i.e.
>>>>>
>>>>> #define SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA BIT(1)
>>>>>
>>>>>>     };
>>>>>>       struct window {
>>>>>> @@ -356,6 +357,79 @@ static void sdhci_j721e_4bit_set_clock(struct sdhci_host *host,
>>>>>>         sdhci_set_clock(host, clock);
>>>>>>     }
>>>>>>     +int sdhci_am654_start_signal_voltage_switch(struct mmc_host *mmc,
>>>>>> +                        struct mmc_ios *ios)
>>>>>
>>>>> Simpler to call sdhci_start_signal_voltage_switch() for the normal
>>>>> case e.g.
>>>>
>>>> This is simpler, so sure will use thanks.
>>>>
>>>>>
>>>>> static int sdhci_am654_start_signal_voltage_switch(struct mmc_host *mmc, struct mmc_ios *ios)
>>>>> {
>>>>>       struct sdhci_host *host = mmc_priv(mmc);
>>>>>       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>>>       struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
>>>>>
>>>>>
>>>>>       if ((sdhci_am654->quirks & SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA) &&
>>>>>           ios->signal_voltage == MMC_SIGNAL_VOLTAGE_180) {
>>>>>           ret = mmc_regulator_set_vqmmc(mmc, ios);
>>>>>           if (ret < 0) {
>>>>>               pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
>>>>>                   mmc_hostname(mmc));
>>>>>               return -EIO;
>>>>>           }
>>>>>           return 0;
>>>>>       }
>>>>>
>>>>>       return sdhci_start_signal_voltage_switch(mmc, ios);
>>>>> }
>>>>>
>>>>>> +{
>>>>>> +    struct sdhci_host *host = mmc_priv(mmc);
>>>>>> +    struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>>>> +    struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
>>>>>> +    u16 ctrl;
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    if (host->version < SDHCI_SPEC_300)
>>>>>> +        return 0;
>>>>>> +
>>>>>> +    switch (ios->signal_voltage) {
>>>>>> +    case MMC_SIGNAL_VOLTAGE_330:
>>>>>> +        if (!(host->flags & SDHCI_SIGNALING_330))
>>>>>> +            return -EINVAL;
>>>>>> +
>>>>>> +        ctrl &= ~SDHCI_CTRL_VDD_180;
>>>>>> +        sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>>>>>> +
>>>>>> +        if (!IS_ERR(mmc->supply.vqmmc)) {
>>>>>> +            ret = mmc_regulator_set_vqmmc(mmc, ios);
>>>>>> +            if (ret < 0) {
>>>>>> +                pr_warn("%s: Switching to 3.3V signalling voltage failed\n",
>>>>>> +                    mmc_hostname(mmc));
>>>>>> +                return -EIO;
>>>>>> +            }
>>>>>> +        }
>>>>>> +
>>>>>> +        usleep_range(5000, 5500);
>>>>>> +
>>>>>> +        ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>>>>>> +        if (!(ctrl & SDHCI_CTRL_VDD_180))
>>>>>> +            return 0;
>>>>>> +
>>>>>> +        pr_warn("%s: 3.3V regulator output did not become stable\n",
>>>>>> +            mmc_hostname(mmc));
>>>>>> +
>>>>>> +        return -EAGAIN;
>>>>>> +
>>>>>> +    case MMC_SIGNAL_VOLTAGE_180:
>>>>>> +        if (!(host->flags & SDHCI_SIGNALING_180))
>>>>>> +            return -EINVAL;
>>>>>> +
>>>>>> +        if (!IS_ERR(mmc->supply.vqmmc)) {
>>>>>> +            ret = mmc_regulator_set_vqmmc(mmc, ios);
>>>>>> +            if (ret < 0) {
>>>>>> +                pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
>>>>>> +                    mmc_hostname(mmc));
>>>>>> +                return -EIO;
>>>>>> +            }
>>>>>> +        }
>>>>>> +
>>>>>> +        if (sdhci_am654->quirks & SDHCI_AM654_QUIRK_SET_V1P8_ENA) {
>>>>>> +            ctrl |= SDHCI_CTRL_VDD_180;
>>>>>> +            sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>>>>>> +
>>>>>> +            ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>>>>>> +            if (ctrl & SDHCI_CTRL_VDD_180)
>>>>>> +                return 0;
>>>>>> +
>>>>>> +            pr_warn("%s: 1.8V regulator output did not become stable\n",
>>>>>> +                mmc_hostname(mmc));
>>>>>> +
>>>>>> +            return -EAGAIN;
>>>>>> +        }
>>>>>> +        return 0;
>>>>>> +
>>>>>> +    default:
>>>>>> +        return 0;
>>>>>> +    }
>>>>>> +}
>>>>>> +
>>>>>>     static u8 sdhci_am654_write_power_on(struct sdhci_host *host, u8 val, int reg)
>>>>>>     {
>>>>>>         writeb(val, host->ioaddr + reg);
>>>>>> @@ -801,6 +875,8 @@ static int sdhci_am654_get_of_property(struct platform_device *pdev,
>>>>>>                         struct sdhci_am654_data *sdhci_am654)
>>>>>>     {
>>>>>>         struct device *dev = &pdev->dev;
>>>>>> +    struct device_node *np = dev->of_node;
>>>>>> +    struct device_node *node;
>>>>>>         int drv_strength;
>>>>>>         int ret;
>>>>>>     @@ -844,6 +920,15 @@ static int sdhci_am654_get_of_property(struct platform_device *pdev,
>>>>>>         if (device_property_read_bool(dev, "ti,fails-without-test-cd"))
>>>>>>             sdhci_am654->quirks |= SDHCI_AM654_QUIRK_FORCE_CDTEST;
>>>>>>     +    node = of_parse_phandle(np, "vmmc-supply", 0);
>>>>>> +
>>>>>> +    if (node) {
>>>>>> +        node = of_parse_phandle(np, "vqmmc-supply", 0);
>>>>>> +
>>>>>> +        if (!node)
>>>>>> +            sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SET_V1P8_ENA;
>>>>>> +    }
>>>>>
>>>>> It would be simpler without 'np' and 'node'.  Not sure
>>>>> what the rule is meant to be, but it could be something like:
>>>>>
>>>>>       if (of_parse_phandle(dev->of_node, "vmmc-supply", 0) &&
>>>>>           of_parse_phandle(dev->of_node, "vqmmc-supply", 0)
>>>>>           sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA;
>>>>
>>>> My issue with this is that I also need the quirk (SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA) for eMMC. DT node for eMMC does
>>>> not include vmmc and vqmmc supplies. That is why I had the quirk logic
>>>> inverted.
>>>
>>> Ideally there would be a more direct way to distinguish eMMC, but
>>> otherwise, having both supplies or neither would be:
>>>
>>>      if (!!of_parse_phandle(dev->of_node, "vmmc-supply", 0) ==
>>>          !!of_parse_phandle(dev->of_node, "vqmmc-supply", 0))
>>>          sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA;
>>
>>
>> Not sure I love the double NOT, but ok, I can use this, will fix for v2.
> 
> It could use a comment, including about the eMMC thing.


Sure, will add. Thanks.

> 
>>
>> Thanks for your suggestion!
>>
>> ~ Judith
>>
>>>
>>>
>>>>
>>>> This patch fixes timing issues with both eMMC and SD. (:
>>>>
>>>> ~ Judith
>>>>
>>>>
>>>>>
>>>>>> +
>>>>>>         sdhci_get_of_property(pdev);
>>>>>>           return 0;
>>>>>> @@ -940,6 +1025,7 @@ static int sdhci_am654_probe(struct platform_device *pdev)
>>>>>>             goto err_pltfm_free;
>>>>>>         }
>>>>>>     +    host->mmc_host_ops.start_signal_voltage_switch = sdhci_am654_start_signal_voltage_switch;
>>>>>>         host->mmc_host_ops.execute_tuning = sdhci_am654_execute_tuning;
>>>>>>           pm_runtime_get_noresume(dev);
>>>>>>
>>>>>> base-commit: cf6444ba528f043398b112ac36e041a4d8685cb1
>>>>>
>>>>>
>>>>
>>>
>>
>
diff mbox series

Patch

diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
index 0aa3c40ea6ed8..fb6232e56606b 100644
--- a/drivers/mmc/host/sdhci_am654.c
+++ b/drivers/mmc/host/sdhci_am654.c
@@ -155,6 +155,7 @@  struct sdhci_am654_data {
 	u32 tuning_loop;
 
 #define SDHCI_AM654_QUIRK_FORCE_CDTEST BIT(0)
+#define SDHCI_AM654_QUIRK_SET_V1P8_ENA BIT(1)
 };
 
 struct window {
@@ -356,6 +357,79 @@  static void sdhci_j721e_4bit_set_clock(struct sdhci_host *host,
 	sdhci_set_clock(host, clock);
 }
 
+int sdhci_am654_start_signal_voltage_switch(struct mmc_host *mmc,
+					    struct mmc_ios *ios)
+{
+	struct sdhci_host *host = mmc_priv(mmc);
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
+	u16 ctrl;
+	int ret;
+
+	if (host->version < SDHCI_SPEC_300)
+		return 0;
+
+	switch (ios->signal_voltage) {
+	case MMC_SIGNAL_VOLTAGE_330:
+		if (!(host->flags & SDHCI_SIGNALING_330))
+			return -EINVAL;
+
+		ctrl &= ~SDHCI_CTRL_VDD_180;
+		sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
+
+		if (!IS_ERR(mmc->supply.vqmmc)) {
+			ret = mmc_regulator_set_vqmmc(mmc, ios);
+			if (ret < 0) {
+				pr_warn("%s: Switching to 3.3V signalling voltage failed\n",
+					mmc_hostname(mmc));
+				return -EIO;
+			}
+		}
+
+		usleep_range(5000, 5500);
+
+		ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
+		if (!(ctrl & SDHCI_CTRL_VDD_180))
+			return 0;
+
+		pr_warn("%s: 3.3V regulator output did not become stable\n",
+			mmc_hostname(mmc));
+
+		return -EAGAIN;
+
+	case MMC_SIGNAL_VOLTAGE_180:
+		if (!(host->flags & SDHCI_SIGNALING_180))
+			return -EINVAL;
+
+		if (!IS_ERR(mmc->supply.vqmmc)) {
+			ret = mmc_regulator_set_vqmmc(mmc, ios);
+			if (ret < 0) {
+				pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
+					mmc_hostname(mmc));
+				return -EIO;
+			}
+		}
+
+		if (sdhci_am654->quirks & SDHCI_AM654_QUIRK_SET_V1P8_ENA) {
+			ctrl |= SDHCI_CTRL_VDD_180;
+			sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
+
+			ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
+			if (ctrl & SDHCI_CTRL_VDD_180)
+				return 0;
+
+			pr_warn("%s: 1.8V regulator output did not become stable\n",
+				mmc_hostname(mmc));
+
+			return -EAGAIN;
+		}
+		return 0;
+
+	default:
+		return 0;
+	}
+}
+
 static u8 sdhci_am654_write_power_on(struct sdhci_host *host, u8 val, int reg)
 {
 	writeb(val, host->ioaddr + reg);
@@ -801,6 +875,8 @@  static int sdhci_am654_get_of_property(struct platform_device *pdev,
 					struct sdhci_am654_data *sdhci_am654)
 {
 	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct device_node *node;
 	int drv_strength;
 	int ret;
 
@@ -844,6 +920,15 @@  static int sdhci_am654_get_of_property(struct platform_device *pdev,
 	if (device_property_read_bool(dev, "ti,fails-without-test-cd"))
 		sdhci_am654->quirks |= SDHCI_AM654_QUIRK_FORCE_CDTEST;
 
+	node = of_parse_phandle(np, "vmmc-supply", 0);
+
+	if (node) {
+		node = of_parse_phandle(np, "vqmmc-supply", 0);
+
+		if (!node)
+			sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SET_V1P8_ENA;
+	}
+
 	sdhci_get_of_property(pdev);
 
 	return 0;
@@ -940,6 +1025,7 @@  static int sdhci_am654_probe(struct platform_device *pdev)
 		goto err_pltfm_free;
 	}
 
+	host->mmc_host_ops.start_signal_voltage_switch = sdhci_am654_start_signal_voltage_switch;
 	host->mmc_host_ops.execute_tuning = sdhci_am654_execute_tuning;
 
 	pm_runtime_get_noresume(dev);