Re: [PATCH] mmc: fsl_esdhc: Fix enablement of UHS mode

2023-04-07 Thread Fabio Estevam
Hi Peng,

On Fri, Apr 7, 2023 at 12:21 AM Peng Fan  wrote:

> The supports_uhs is as below, so this condition check will never have
> UHS_CAPS set even MMC_UHS_SUPPORT selected. This seems not correct fix.

Thanks for your review.

Should we fix it like this instead?

--- a/drivers/mmc/fsl_esdhc_imx.c
+++ b/drivers/mmc/fsl_esdhc_imx.c
@@ -1258,13 +1258,6 @@ static int fsl_esdhc_init(struct fsl_esdhc_priv *priv,
esdhc_write32(>tuning_ctrl, val);
}

-   /*
-* UHS doesn't have explicit ESDHC flags, so if it's
-* not supported, disable it in config.
-*/
-   if (CONFIG_IS_ENABLED(MMC_UHS_SUPPORT))
-   cfg->host_caps |= UHS_CAPS;
-
if (CONFIG_IS_ENABLED(MMC_HS200_SUPPORT)) {
if (priv->flags & ESDHC_FLAG_HS200)
cfg->host_caps |= MMC_CAP(MMC_HS_200);


Setting UHS_CAPS based on CONFIG_MMC_UHS_SUPPORT=y does not look correct.

#define UHS_CAPS (MMC_CAP(UHS_SDR12) | MMC_CAP(UHS_SDR25) | \
  MMC_CAP(UHS_SDR50) | MMC_CAP(UHS_SDR104) | \
  MMC_CAP(UHS_DDR50))

The SD card may not support all these modes.

What do you think?


Re: [PATCH] mmc: fsl_esdhc: Fix enablement of UHS mode

2023-04-06 Thread Peng Fan




On 4/6/2023 8:14 PM, Fabio Estevam wrote:

From: Fabio Estevam 

Since commit 1a7904fdfa7d ("mmc: fsl_esdhc_imx: Use esdhc_soc_data flags
to set host caps") the following SD card error is observed on an imx7d-sdb
board:

U-Boot 2023.04-00652-g487e42f7bc5e (Apr 05 2023 - 22:14:21 -0300)

CPU:   Freescale i.MX7D rev1.0 1000 MHz (running at 792 MHz)
CPU:   Commercial temperature grade (0C to 95C) at 35C
Reset cause: POR
Model: Freescale i.MX7 SabreSD Board
Board: i.MX7D SABRESD in non-secure mode
DRAM:  1 GiB
Core:  100 devices, 19 uclasses, devicetree: separate
PMIC: PFUZE3000 DEV_ID=0x30 REV_ID=0x10
MMC:   FSL_SDHC: 0, FSL_SDHC: 1, FSL_SDHC: 2
Loading Environment from MMC... Card did not respond to voltage select! : -110
*** Warning - No block device, using default environment

Fix the problem by only asserting the UHS_CAPS when supports_uhs() is true
instead of doing it unconditionally, only based on the defconfig option.

Signed-off-by: Fabio Estevam 
---
  drivers/mmc/fsl_esdhc_imx.c | 6 +-
  1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c
index 66caf683f741..bfc94d2a5326 100644
--- a/drivers/mmc/fsl_esdhc_imx.c
+++ b/drivers/mmc/fsl_esdhc_imx.c
@@ -1258,11 +1258,7 @@ static int fsl_esdhc_init(struct fsl_esdhc_priv *priv,
esdhc_write32(>tuning_ctrl, val);
}
  
-		/*

-* UHS doesn't have explicit ESDHC flags, so if it's
-* not supported, disable it in config.
-*/
-   if (CONFIG_IS_ENABLED(MMC_UHS_SUPPORT))
+   if (CONFIG_IS_ENABLED(MMC_UHS_SUPPORT) && 
supports_uhs(cfg->host_caps))
cfg->host_caps |= UHS_CAPS;


The supports_uhs is as below, so this condition check will never have 
UHS_CAPS set even MMC_UHS_SUPPORT selected. This seems not correct fix.


static inline bool supports_uhs(uint caps)
{
#if CONFIG_IS_ENABLED(MMC_UHS_SUPPORT)
return (caps & UHS_CAPS) ? true : false;
#else
return false;
#endif
}

Regards,
Peng.

  
  		if (CONFIG_IS_ENABLED(MMC_HS200_SUPPORT)) {


[PATCH] mmc: fsl_esdhc: Fix enablement of UHS mode

2023-04-06 Thread Fabio Estevam
From: Fabio Estevam 

Since commit 1a7904fdfa7d ("mmc: fsl_esdhc_imx: Use esdhc_soc_data flags
to set host caps") the following SD card error is observed on an imx7d-sdb
board:

U-Boot 2023.04-00652-g487e42f7bc5e (Apr 05 2023 - 22:14:21 -0300)

CPU:   Freescale i.MX7D rev1.0 1000 MHz (running at 792 MHz)
CPU:   Commercial temperature grade (0C to 95C) at 35C
Reset cause: POR
Model: Freescale i.MX7 SabreSD Board
Board: i.MX7D SABRESD in non-secure mode
DRAM:  1 GiB
Core:  100 devices, 19 uclasses, devicetree: separate
PMIC: PFUZE3000 DEV_ID=0x30 REV_ID=0x10
MMC:   FSL_SDHC: 0, FSL_SDHC: 1, FSL_SDHC: 2
Loading Environment from MMC... Card did not respond to voltage select! : -110
*** Warning - No block device, using default environment

Fix the problem by only asserting the UHS_CAPS when supports_uhs() is true
instead of doing it unconditionally, only based on the defconfig option.

Signed-off-by: Fabio Estevam 
---
 drivers/mmc/fsl_esdhc_imx.c | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c
index 66caf683f741..bfc94d2a5326 100644
--- a/drivers/mmc/fsl_esdhc_imx.c
+++ b/drivers/mmc/fsl_esdhc_imx.c
@@ -1258,11 +1258,7 @@ static int fsl_esdhc_init(struct fsl_esdhc_priv *priv,
esdhc_write32(>tuning_ctrl, val);
}
 
-   /*
-* UHS doesn't have explicit ESDHC flags, so if it's
-* not supported, disable it in config.
-*/
-   if (CONFIG_IS_ENABLED(MMC_UHS_SUPPORT))
+   if (CONFIG_IS_ENABLED(MMC_UHS_SUPPORT) && 
supports_uhs(cfg->host_caps))
cfg->host_caps |= UHS_CAPS;
 
if (CONFIG_IS_ENABLED(MMC_HS200_SUPPORT)) {
-- 
2.34.1