| 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 |
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
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
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
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 > >
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 >> >> >
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 >>> >>> >> >
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 >>>> >>>> >>> >> >
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 --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);
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