Message ID | 20230411134125.2064478-1-festevam@gmail.com |
---|---|
State | Rejected |
Delegated to: | Stefano Babic |
Headers | show |
Series | [v2] mmc: fsl_esdhc: Do not set UHS_CAPS based on CONFIG_MMC_UHS_SUPPORT | expand |
Hi Fabio, On 4/11/2023 9:41 PM, Fabio Estevam wrote: > From: Fabio Estevam <festevam@denx.de> > > 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: What kind card do you see the issue? Have you tried the other card? > > 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 > > Setting UHS_CAPS only based on the CONFIG_MMC_UHS_SUPPORT config > option is not correct because UHS_CAPS has the following definition: > > #define UHS_CAPS (MMC_CAP(UHS_SDR12) | MMC_CAP(UHS_SDR25) | \ > MMC_CAP(UHS_SDR50) | MMC_CAP(UHS_SDR104) | \ > MMC_CAP(UHS_DDR50)) > > and the SD card may not necessarily support all these modes. > > Remove the UHS_CAPS setting to fix the SD card regression. > > Fixes: 1a7904fdfa7d ("mmc: fsl_esdhc_imx: Use esdhc_soc_data flags to set host caps") > Signed-off-by: Fabio Estevam <festevam@denx.de> > --- > Changes since v1: > - Remove setting UHS_CAPS completely. > > drivers/mmc/fsl_esdhc_imx.c | 7 ------- > 1 file changed, 7 deletions(-) > > diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c > index 66caf683f7..5e7d9f41b6 100644 > --- 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(®s->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; I am afraid this just workaround the issue you met. Actually in drivers/mmc/mmc, if your card not support UHS_CAPS, the caps will remove the UHS_CAPS. 1762 /* Restrict card's capabilities by what the host can do */ 1763 caps = card_caps & mmc->host_caps; 1764 1765 if (!uhs_en) 1766 caps &= ~UHS_CAPS; So would you please dump the card_caps, then we find a proper fix? Thanks, Peng. > - > if (CONFIG_IS_ENABLED(MMC_HS200_SUPPORT)) { > if (priv->flags & ESDHC_FLAG_HS200) > cfg->host_caps |= MMC_CAP(MMC_HS_200);
Hi Fabio, > -----Original Message----- > From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Peng Fan > Sent: Wednesday, April 12, 2023 4:55 PM > To: Fabio Estevam <festevam@gmail.com>; Peng Fan <peng.fan@nxp.com> > Cc: jh80.chung@samsung.com; aford173@gmail.com; u-boot@lists.denx.de; > Fabio Estevam <festevam@denx.de>; Bough Chen <haibo.chen@nxp.com> > Subject: [EXT] Re: [PATCH v2] mmc: fsl_esdhc: Do not set UHS_CAPS based on > CONFIG_MMC_UHS_SUPPORT > > Caution: EXT Email > > Hi Fabio, > > On 4/11/2023 9:41 PM, Fabio Estevam wrote: > > From: Fabio Estevam <festevam@denx.de> > > > > 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: > > What kind card do you see the issue? Have you tried the other card? > > > > > 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 > > > > Setting UHS_CAPS only based on the CONFIG_MMC_UHS_SUPPORT config > > option is not correct because UHS_CAPS has the following definition: > > > > #define UHS_CAPS (MMC_CAP(UHS_SDR12) | MMC_CAP(UHS_SDR25) | \ > > MMC_CAP(UHS_SDR50) | MMC_CAP(UHS_SDR104) | \ > > MMC_CAP(UHS_DDR50)) > > > > and the SD card may not necessarily support all these modes. > > > > Remove the UHS_CAPS setting to fix the SD card regression. > > > > Fixes: 1a7904fdfa7d ("mmc: fsl_esdhc_imx: Use esdhc_soc_data flags to > > set host caps") > > Signed-off-by: Fabio Estevam <festevam@denx.de> Remove the UHS_CAPS is wrong, it will cause UHS-I mode not work. I just check the 7d sdb on upstream, it is due to imx7d-sdb.dts change. When UHS is enabled in defconfig, the usdhc1 node in imx7d-sdb.dts does not configure pad for VSELECT, also the data pad should be set to 100Mhz/200Mhz pin states. You can try below changes: --- a/arch/arm/dts/imx7d-sdb.dts +++ b/arch/arm/dts/imx7d-sdb.dts @@ -478,8 +478,10 @@ }; &usdhc1 { - pinctrl-names = "default"; - pinctrl-0 = <&pinctrl_usdhc1>; + pinctrl-names = "default", "state_100mhz", "state_200mhz"; + pinctrl-0 = <&pinctrl_usdhc1>, <&pinctrl_usdhc1_gpio>; + pinctrl-1 = <&pinctrl_usdhc1_100mhz>, <&pinctrl_usdhc1_gpio>; + pinctrl-2 = <&pinctrl_usdhc1_200mhz>, <&pinctrl_usdhc1_gpio>; cd-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>; wp-gpios = <&gpio5 1 GPIO_ACTIVE_HIGH>; wakeup-source; @@ -736,6 +738,15 @@ >; }; + pinctrl_usdhc1_gpio: usdhc1_gpiogrp { + fsl,pins = < + MX7D_PAD_SD1_CD_B__GPIO5_IO0 0x59 /* CD */ + MX7D_PAD_SD1_WP__GPIO5_IO1 0x59 /* WP */ + MX7D_PAD_SD1_RESET_B__GPIO5_IO2 0x59 /* vmmc */ + MX7D_PAD_GPIO1_IO08__SD1_VSELECT 0x59 /* VSELECT */ + >; + }; + pinctrl_usdhc1: usdhc1grp { fsl,pins = < MX7D_PAD_SD1_CMD__SD1_CMD 0x59 @@ -744,9 +755,28 @@ MX7D_PAD_SD1_DATA1__SD1_DATA1 0x59 MX7D_PAD_SD1_DATA2__SD1_DATA2 0x59 MX7D_PAD_SD1_DATA3__SD1_DATA3 0x59 - MX7D_PAD_SD1_CD_B__GPIO5_IO0 0x59 /* CD */ - MX7D_PAD_SD1_WP__GPIO5_IO1 0x59 /* WP */ - MX7D_PAD_SD1_RESET_B__GPIO5_IO2 0x59 /* vmmc */ + >; + }; + + pinctrl_usdhc1_100mhz: usdhc1grp_100mhz { + fsl,pins = < + MX7D_PAD_SD1_CMD__SD1_CMD 0x5a + MX7D_PAD_SD1_CLK__SD1_CLK 0x1a + MX7D_PAD_SD1_DATA0__SD1_DATA0 0x5a + MX7D_PAD_SD1_DATA1__SD1_DATA1 0x5a + MX7D_PAD_SD1_DATA2__SD1_DATA2 0x5a + MX7D_PAD_SD1_DATA3__SD1_DATA3 0x5a + >; + }; + + pinctrl_usdhc1_200mhz: usdhc1grp_200mhz { + fsl,pins = < + MX7D_PAD_SD1_CMD__SD1_CMD 0x5b + MX7D_PAD_SD1_CLK__SD1_CLK 0x1b + MX7D_PAD_SD1_DATA0__SD1_DATA0 0x5b + MX7D_PAD_SD1_DATA1__SD1_DATA1 0x5b + MX7D_PAD_SD1_DATA2__SD1_DATA2 0x5b + MX7D_PAD_SD1_DATA3__SD1_DATA3 0x5b >; }; Best regards, Ye Li > > --- > > Changes since v1: > > - Remove setting UHS_CAPS completely. > > > > drivers/mmc/fsl_esdhc_imx.c | 7 ------- > > 1 file changed, 7 deletions(-) > > > > diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c > > index 66caf683f7..5e7d9f41b6 100644 > > --- 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(®s->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; > > I am afraid this just workaround the issue you met. Actually in > drivers/mmc/mmc, if your card not support UHS_CAPS, the caps will remove the > UHS_CAPS. > > 1762 /* Restrict card's capabilities by what the host can do */ > 1763 caps = card_caps & mmc->host_caps; > 1764 > 1765 if (!uhs_en) > 1766 caps &= ~UHS_CAPS; > > So would you please dump the card_caps, then we find a proper fix? > > Thanks, > Peng. > > > - > > if (CONFIG_IS_ENABLED(MMC_HS200_SUPPORT)) { > > if (priv->flags & ESDHC_FLAG_HS200) > > cfg->host_caps |= > MMC_CAP(MMC_HS_200);
Hi Ye Li, On Wed, Apr 12, 2023 at 6:51 AM Ye Li <ye.li@nxp.com> wrote: > Remove the UHS_CAPS is wrong, it will cause UHS-I mode not work. > I just check the 7d sdb on upstream, it is due to imx7d-sdb.dts change. > When UHS is enabled in defconfig, the usdhc1 node in imx7d-sdb.dts does not configure pad for VSELECT, also > the data pad should be set to 100Mhz/200Mhz pin states. > You can try below changes: Thanks for your suggestion! I tried it and it works. I will submit a formal patch. Thanks
diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c index 66caf683f7..5e7d9f41b6 100644 --- 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(®s->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);