| Message ID | 20230516142116.29996-1-stefan.herbrechtsmeier-oss@weidmueller.com |
|---|---|
| State | RFC |
| Delegated to: | Michal Simek |
| Headers | show |
| Series | [RFC] mmc: zynq_sdhci: Dependable card detect | expand |
Hi Stefan,
We haven't seen any instability issue so far on ZynqMP boards and no one reported any such issue till now.
We are already taking care of card detection stability issue with below waiting logic.
https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/mmc/zynq_sdhci.c#L1161
Could you please answer below questions.
1. is this observed in SPL only or full u-boot ?
2. is this observed in dynamic config case only ? because your patch is in dynamic path.
3. "On our hardware we get a "Sdhci card detect state not stable" error in the SPL if no mmc card is present"
Are you seeing stability issue when card is not present ? can you please elaborate more about the issue.
4. It is not convincing to me that you are trying to select eMMC for some time and then switching back to SD.
Thanks,
Ashok
> -----Original Message-----
> From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Stefan
> Herbrechtsmeier
> Sent: Tuesday, May 16, 2023 7:51 PM
> To: u-boot@lists.denx.de
> Cc: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>;
> Jaehoon Chung <jh80.chung@samsung.com>; Simek, Michal
> <michal.simek@amd.com>; Peng Fan <peng.fan@nxp.com>
> Subject: [RFC PATCH] mmc: zynq_sdhci: Dependable card detect
>
> CAUTION: This message has originated from an External Source. Please use
> proper judgment and caution when opening attachments, clicking links, or
> responding to this email.
>
>
> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>
> The card detect logic needs a short card present signal to work dependable.
> Without a present card the SDHCI_CARD_STATE_STABLE signal is not set
> dependable after a reset. Use the internal fixed card present signal to initiate
> the card detect logic.
>
> Signed-off-by: Stefan Herbrechtsmeier
> <stefan.herbrechtsmeier@weidmueller.com>
>
> ---
> On our hardware we get a "Sdhci card detect state not stable" error in the SPL
> if no mmc card is present. It is unclear if this patch is the correct solution, but
> a short card inserts or a fixed card present signal leads to a
> SDHCI_CARD_STATE_STABLE signal with and without card.
>
> drivers/mmc/zynq_sdhci.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/drivers/mmc/zynq_sdhci.c b/drivers/mmc/zynq_sdhci.c index
> e44868aaec..a88feeb367 100644
> --- a/drivers/mmc/zynq_sdhci.c
> +++ b/drivers/mmc/zynq_sdhci.c
> @@ -1075,6 +1075,26 @@ static int
> sdhci_zynqmp_set_dynamic_config(struct arasan_sdhci_priv *priv,
> return ret;
> }
>
> + /* The card detect logic needs a short card present signal to work
> + * dependable. Without a present card the SDHCI_CARD_STATE_STABLE
> + * signal is not set dependable after a reset. Use the internal
> + * fixed card present signal to initiate the card detect logic.
> + */
> + if (!dev_read_bool(dev, "non-removable")) {
> + ret = zynqmp_pm_set_sd_config(priv->node_id,
> SD_CONFIG_EMMC_SEL,
> + 1);
> + if (ret) {
> + dev_err(dev, "SD_CONFIG_EMMC_SEL failed\n");
> + return ret;
> + }
> + ret = zynqmp_pm_set_sd_config(priv->node_id,
> SD_CONFIG_EMMC_SEL,
> + 0);
> + if (ret) {
> + dev_err(dev, "SD_CONFIG_EMMC_SEL failed\n");
> + return ret;
> + }
> + }
> +
> return 0;
> }
> #endif
> --
> 2.30.2
Hi Ashok, Am 18.05.2023 um 08:20 schrieb Soma, Ashok Reddy: > We haven't seen any instability issue so far on ZynqMP boards and no one reported any such issue till now. It isn't an instability issue. The high level at the card detect input is not marked as stable until a short low level is detected. > We are already taking care of card detection stability issue with below waiting logic. > https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/mmc/zynq_sdhci.c#L1161 This code makes problems for us. The SDHCI_CARD_STATE_STABLE signal isn't set without a card and we see a "Sdhci card detect state not stable" message. > Could you please answer below questions. > 1. is this observed in SPL only or full u-boot ? Both. We have enable environment support in SPL and see the "Sdhci card detect state not stable" message after "Loading Environment from MMC..." in SPL and "MMC: " in full U-Boot. > 2. is this observed in dynamic config case only ? because your patch is in dynamic path. No. I have override the return value of the zynqmp_pm_is_function_supported function and see the same error. Without dynamic case I don't see a possibility to set the SD_CONFIG_EMMC_SEL value. > 3. "On our hardware we get a "Sdhci card detect state not stable" error in the SPL if no mmc card is present" > Are you seeing stability issue when card is not present ? No, the SDHCI_CARD_STATE_STABLE signal is never set. > can you please elaborate more about the issue. I have change the code to monitor the SDHCI_PRESENT_STATE signal. Without a card at boot the SDHCI_CARD_STATE_STABLE signal is unset. After card insert the SDHCI_CARD_PRESENT signal and the SDHCI_CARD_STATE_STABLE signal is set after some time. After card remove the SDHCI_CARD_STATE_STABLE signal is set and the SDHCI_CARD_PRESENT signal is unset after some time. It doesn't matter if a physical card is insert or the SD_CONFIG_EMMC_SEL config is used to emulate a present card. Without a short card present signal on the card detect logic the SDHCI_CARD_STATE_STABLE signal isn't set. > 4. It is not convincing to me that you are trying to select eMMC for some time and then switching back to SD. I don't find a documentation what the consequence of this config is but this config is set based on the value of "non-removable" and setting this config with enabled clock leads to a functional SDHCI_CARD_STATE_STABLE signal even after unset of the config. Maybe the SD_CDn_CTRL register have the same result but this isn't available via PMU. Regards Stefan >> -----Original Message----- >> From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Stefan >> Herbrechtsmeier >> Sent: Tuesday, May 16, 2023 7:51 PM >> To: u-boot@lists.denx.de >> Cc: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>; >> Jaehoon Chung <jh80.chung@samsung.com>; Simek, Michal >> <michal.simek@amd.com>; Peng Fan <peng.fan@nxp.com> >> Subject: [RFC PATCH] mmc: zynq_sdhci: Dependable card detect >> >> CAUTION: This message has originated from an External Source. Please use >> proper judgment and caution when opening attachments, clicking links, or >> responding to this email. >> >> >> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> >> >> The card detect logic needs a short card present signal to work dependable. >> Without a present card the SDHCI_CARD_STATE_STABLE signal is not set >> dependable after a reset. Use the internal fixed card present signal to initiate >> the card detect logic. >> >> Signed-off-by: Stefan Herbrechtsmeier >> <stefan.herbrechtsmeier@weidmueller.com> >> >> --- >> On our hardware we get a "Sdhci card detect state not stable" error in the SPL >> if no mmc card is present. It is unclear if this patch is the correct solution, but >> a short card inserts or a fixed card present signal leads to a >> SDHCI_CARD_STATE_STABLE signal with and without card. >> >> drivers/mmc/zynq_sdhci.c | 20 ++++++++++++++++++++ >> 1 file changed, 20 insertions(+) >> >> diff --git a/drivers/mmc/zynq_sdhci.c b/drivers/mmc/zynq_sdhci.c index >> e44868aaec..a88feeb367 100644 >> --- a/drivers/mmc/zynq_sdhci.c >> +++ b/drivers/mmc/zynq_sdhci.c >> @@ -1075,6 +1075,26 @@ static int >> sdhci_zynqmp_set_dynamic_config(struct arasan_sdhci_priv *priv, >> return ret; >> } >> >> + /* The card detect logic needs a short card present signal to work >> + * dependable. Without a present card the SDHCI_CARD_STATE_STABLE >> + * signal is not set dependable after a reset. Use the internal >> + * fixed card present signal to initiate the card detect logic. >> + */ >> + if (!dev_read_bool(dev, "non-removable")) { >> + ret = zynqmp_pm_set_sd_config(priv->node_id, >> SD_CONFIG_EMMC_SEL, >> + 1); >> + if (ret) { >> + dev_err(dev, "SD_CONFIG_EMMC_SEL failed\n"); >> + return ret; >> + } >> + ret = zynqmp_pm_set_sd_config(priv->node_id, >> SD_CONFIG_EMMC_SEL, >> + 0); >> + if (ret) { >> + dev_err(dev, "SD_CONFIG_EMMC_SEL failed\n"); >> + return ret; >> + } >> + } >> + >> return 0; >> } >> #endif >> -- >> 2.30.2
Hi Stefan, > -----Original Message----- > From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier- > oss@weidmueller.com> > Sent: Monday, May 22, 2023 1:26 PM > To: Soma, Ashok Reddy <ashok.reddy.soma@amd.com>; u- > boot@lists.denx.de > Cc: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>; > Jaehoon Chung <jh80.chung@samsung.com>; Simek, Michal > <michal.simek@amd.com>; Peng Fan <peng.fan@nxp.com>; Potthuri, Sai > Krishna <sai.krishna.potthuri@amd.com> > Subject: Re: [RFC PATCH] mmc: zynq_sdhci: Dependable card detect > > Hi Ashok, > > Am 18.05.2023 um 08:20 schrieb Soma, Ashok Reddy: > > > We haven't seen any instability issue so far on ZynqMP boards and no one > reported any such issue till now. > > It isn't an instability issue. The high level at the card detect input is not > marked as stable until a short low level is detected. > > > We are already taking care of card detection stability issue with below > waiting logic. > > https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/mmc/zynq_sd > > hci.c#L1161 > > This code makes problems for us. The SDHCI_CARD_STATE_STABLE signal isn't > set without a card and we see a "Sdhci card detect state not stable" message. > > > Could you please answer below questions. > > 1. is this observed in SPL only or full u-boot ? > > Both. We have enable environment support in SPL and see the "Sdhci card > detect state not stable" message after "Loading Environment from MMC..." > in SPL and "MMC: " in full U-Boot. > > > 2. is this observed in dynamic config case only ? because your patch is in > dynamic path. > > No. I have override the return value of the > zynqmp_pm_is_function_supported function and see the same error. Without > dynamic case I don't see a possibility to set the SD_CONFIG_EMMC_SEL > value. > > > 3. "On our hardware we get a "Sdhci card detect state not stable" error in > the SPL if no mmc card is present" > > Are you seeing stability issue when card is not present ? > > No, the SDHCI_CARD_STATE_STABLE signal is never set. Surprised that card card state stable signal is never set. But we cannot go with the workaround in this way. > > > can you please elaborate more about the issue. > > I have change the code to monitor the SDHCI_PRESENT_STATE signal. > Without a card at boot the SDHCI_CARD_STATE_STABLE signal is unset. > After card insert the SDHCI_CARD_PRESENT signal and the > SDHCI_CARD_STATE_STABLE signal is set after some time. After card remove > the SDHCI_CARD_STATE_STABLE signal is set and the SDHCI_CARD_PRESENT > signal is unset after some time. It doesn't matter if a physical card is insert or > the SD_CONFIG_EMMC_SEL config is used to emulate a present card. > Without a short card present signal on the card detect logic the > SDHCI_CARD_STATE_STABLE signal isn't set. > > > 4. It is not convincing to me that you are trying to select eMMC for some > time and then switching back to SD. > > I don't find a documentation what the consequence of this config is but this > config is set based on the value of "non-removable" and setting this config > with enabled clock leads to a functional SDHCI_CARD_STATE_STABLE signal > even after unset of the config. > > Maybe the SD_CDn_CTRL register have the same result but this isn't available > via PMU. Please try any other method and lets see, but changing to eMMC mode and back to SDHC is NOT acceptable according to me. Thanks, Ashok > > Regards > Stefan > > >> -----Original Message----- > >> From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Stefan > >> Herbrechtsmeier > >> Sent: Tuesday, May 16, 2023 7:51 PM > >> To: u-boot@lists.denx.de > >> Cc: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>; > >> Jaehoon Chung <jh80.chung@samsung.com>; Simek, Michal > >> <michal.simek@amd.com>; Peng Fan <peng.fan@nxp.com> > >> Subject: [RFC PATCH] mmc: zynq_sdhci: Dependable card detect > >> > >> CAUTION: This message has originated from an External Source. Please > >> use proper judgment and caution when opening attachments, clicking > >> links, or responding to this email. > >> > >> > >> From: Stefan Herbrechtsmeier > <stefan.herbrechtsmeier@weidmueller.com> > >> > >> The card detect logic needs a short card present signal to work > dependable. > >> Without a present card the SDHCI_CARD_STATE_STABLE signal is not set > >> dependable after a reset. Use the internal fixed card present signal > >> to initiate the card detect logic. > >> > >> Signed-off-by: Stefan Herbrechtsmeier > >> <stefan.herbrechtsmeier@weidmueller.com> > >> > >> --- > >> On our hardware we get a "Sdhci card detect state not stable" error > >> in the SPL if no mmc card is present. It is unclear if this patch is > >> the correct solution, but a short card inserts or a fixed card > >> present signal leads to a SDHCI_CARD_STATE_STABLE signal with and > without card. > >> > >> drivers/mmc/zynq_sdhci.c | 20 ++++++++++++++++++++ > >> 1 file changed, 20 insertions(+) > >> > >> diff --git a/drivers/mmc/zynq_sdhci.c b/drivers/mmc/zynq_sdhci.c > >> index > >> e44868aaec..a88feeb367 100644 > >> --- a/drivers/mmc/zynq_sdhci.c > >> +++ b/drivers/mmc/zynq_sdhci.c > >> @@ -1075,6 +1075,26 @@ static int > >> sdhci_zynqmp_set_dynamic_config(struct arasan_sdhci_priv *priv, > >> return ret; > >> } > >> > >> + /* The card detect logic needs a short card present signal to work > >> + * dependable. Without a present card the > SDHCI_CARD_STATE_STABLE > >> + * signal is not set dependable after a reset. Use the internal > >> + * fixed card present signal to initiate the card detect logic. > >> + */ > >> + if (!dev_read_bool(dev, "non-removable")) { > >> + ret = zynqmp_pm_set_sd_config(priv->node_id, > >> SD_CONFIG_EMMC_SEL, > >> + 1); > >> + if (ret) { > >> + dev_err(dev, "SD_CONFIG_EMMC_SEL failed\n"); > >> + return ret; > >> + } > >> + ret = zynqmp_pm_set_sd_config(priv->node_id, > >> SD_CONFIG_EMMC_SEL, > >> + 0); > >> + if (ret) { > >> + dev_err(dev, "SD_CONFIG_EMMC_SEL failed\n"); > >> + return ret; > >> + } > >> + } > >> + > >> return 0; > >> } > >> #endif > >> -- > >> 2.30.2
Hi Ashok, Am 06.06.2023 um 10:20 schrieb Soma, Ashok Reddy: > Hi Stefan, > > >> -----Original Message----- >> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier- >> oss@weidmueller.com> >> Sent: Monday, May 22, 2023 1:26 PM >> To: Soma, Ashok Reddy <ashok.reddy.soma@amd.com>; u- >> boot@lists.denx.de >> Cc: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>; >> Jaehoon Chung <jh80.chung@samsung.com>; Simek, Michal >> <michal.simek@amd.com>; Peng Fan <peng.fan@nxp.com>; Potthuri, Sai >> Krishna <sai.krishna.potthuri@amd.com> >> Subject: Re: [RFC PATCH] mmc: zynq_sdhci: Dependable card detect >> >> Hi Ashok, >> >> Am 18.05.2023 um 08:20 schrieb Soma, Ashok Reddy: >> >>> We haven't seen any instability issue so far on ZynqMP boards and no one >> reported any such issue till now. >> >> It isn't an instability issue. The high level at the card detect input is not >> marked as stable until a short low level is detected. >> >>> We are already taking care of card detection stability issue with below >> waiting logic. >>> https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/mmc/zynq_sd >>> hci.c#L1161 >> This code makes problems for us. The SDHCI_CARD_STATE_STABLE signal isn't >> set without a card and we see a "Sdhci card detect state not stable" message. >> >>> Could you please answer below questions. >>> 1. is this observed in SPL only or full u-boot ? >> Both. We have enable environment support in SPL and see the "Sdhci card >> detect state not stable" message after "Loading Environment from MMC..." >> in SPL and "MMC: " in full U-Boot. >> >>> 2. is this observed in dynamic config case only ? because your patch is in >> dynamic path. >> >> No. I have override the return value of the >> zynqmp_pm_is_function_supported function and see the same error. Without >> dynamic case I don't see a possibility to set the SD_CONFIG_EMMC_SEL >> value. >> >>> 3. "On our hardware we get a "Sdhci card detect state not stable" error in >> the SPL if no mmc card is present" >>> Are you seeing stability issue when card is not present ? >> No, the SDHCI_CARD_STATE_STABLE signal is never set. > Surprised that card card state stable signal is never set. But we cannot go with the workaround in this way. It is impossible to debug it further without a knowledge about the card state stable signal logic. >>> can you please elaborate more about the issue. >> I have change the code to monitor the SDHCI_PRESENT_STATE signal. >> Without a card at boot the SDHCI_CARD_STATE_STABLE signal is unset. >> After card insert the SDHCI_CARD_PRESENT signal and the >> SDHCI_CARD_STATE_STABLE signal is set after some time. After card remove >> the SDHCI_CARD_STATE_STABLE signal is set and the SDHCI_CARD_PRESENT >> signal is unset after some time. It doesn't matter if a physical card is insert or >> the SD_CONFIG_EMMC_SEL config is used to emulate a present card. >> Without a short card present signal on the card detect logic the >> SDHCI_CARD_STATE_STABLE signal isn't set. >> >>> 4. It is not convincing to me that you are trying to select eMMC for some >> time and then switching back to SD. >> >> I don't find a documentation what the consequence of this config is but this >> config is set based on the value of "non-removable" and setting this config >> with enabled clock leads to a functional SDHCI_CARD_STATE_STABLE signal >> even after unset of the config. >> >> Maybe the SD_CDn_CTRL register have the same result but this isn't available >> via PMU. > Please try any other method and lets see, but changing to eMMC mode and back to SDHC is NOT acceptable according to me. At the moment the device tree property "non-removable" controls the value of the SD_CONFIG_EMMC_SEL config and regarding the device tree binding documentation this property marks an always present slot like eMMC. In this case the current implementation is wrong because it assume that a "non-removable" slot is always an eMMC. Would it be okay to set and unset the SD{0,1}_CDn_CTRL from the SPL? https://www.xilinx.com/htmldocs/registers/ug1087/ug1087-zynq-ultrascale-registers.html#iou_slcr___sd_cdn_ctrl.html Regard Stefan > > Thanks, > Ashok > >> Regards >> Stefan >> >>>> -----Original Message----- >>>> From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Stefan >>>> Herbrechtsmeier >>>> Sent: Tuesday, May 16, 2023 7:51 PM >>>> To: u-boot@lists.denx.de >>>> Cc: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>; >>>> Jaehoon Chung <jh80.chung@samsung.com>; Simek, Michal >>>> <michal.simek@amd.com>; Peng Fan <peng.fan@nxp.com> >>>> Subject: [RFC PATCH] mmc: zynq_sdhci: Dependable card detect >>>> >>>> CAUTION: This message has originated from an External Source. Please >>>> use proper judgment and caution when opening attachments, clicking >>>> links, or responding to this email. >>>> >>>> >>>> From: Stefan Herbrechtsmeier >> <stefan.herbrechtsmeier@weidmueller.com> >>>> The card detect logic needs a short card present signal to work >> dependable. >>>> Without a present card the SDHCI_CARD_STATE_STABLE signal is not set >>>> dependable after a reset. Use the internal fixed card present signal >>>> to initiate the card detect logic. >>>> >>>> Signed-off-by: Stefan Herbrechtsmeier >>>> <stefan.herbrechtsmeier@weidmueller.com> >>>> >>>> --- >>>> On our hardware we get a "Sdhci card detect state not stable" error >>>> in the SPL if no mmc card is present. It is unclear if this patch is >>>> the correct solution, but a short card inserts or a fixed card >>>> present signal leads to a SDHCI_CARD_STATE_STABLE signal with and >> without card. >>>> drivers/mmc/zynq_sdhci.c | 20 ++++++++++++++++++++ >>>> 1 file changed, 20 insertions(+) >>>> >>>> diff --git a/drivers/mmc/zynq_sdhci.c b/drivers/mmc/zynq_sdhci.c >>>> index >>>> e44868aaec..a88feeb367 100644 >>>> --- a/drivers/mmc/zynq_sdhci.c >>>> +++ b/drivers/mmc/zynq_sdhci.c >>>> @@ -1075,6 +1075,26 @@ static int >>>> sdhci_zynqmp_set_dynamic_config(struct arasan_sdhci_priv *priv, >>>> return ret; >>>> } >>>> >>>> + /* The card detect logic needs a short card present signal to work >>>> + * dependable. Without a present card the >> SDHCI_CARD_STATE_STABLE >>>> + * signal is not set dependable after a reset. Use the internal >>>> + * fixed card present signal to initiate the card detect logic. >>>> + */ >>>> + if (!dev_read_bool(dev, "non-removable")) { >>>> + ret = zynqmp_pm_set_sd_config(priv->node_id, >>>> SD_CONFIG_EMMC_SEL, >>>> + 1); >>>> + if (ret) { >>>> + dev_err(dev, "SD_CONFIG_EMMC_SEL failed\n"); >>>> + return ret; >>>> + } >>>> + ret = zynqmp_pm_set_sd_config(priv->node_id, >>>> SD_CONFIG_EMMC_SEL, >>>> + 0); >>>> + if (ret) { >>>> + dev_err(dev, "SD_CONFIG_EMMC_SEL failed\n"); >>>> + return ret; >>>> + } >>>> + } >>>> + >>>> return 0; >>>> } >>>> #endif >>>> -- >>>> 2.30.2
diff --git a/drivers/mmc/zynq_sdhci.c b/drivers/mmc/zynq_sdhci.c index e44868aaec..a88feeb367 100644 --- a/drivers/mmc/zynq_sdhci.c +++ b/drivers/mmc/zynq_sdhci.c @@ -1075,6 +1075,26 @@ static int sdhci_zynqmp_set_dynamic_config(struct arasan_sdhci_priv *priv, return ret; } + /* The card detect logic needs a short card present signal to work + * dependable. Without a present card the SDHCI_CARD_STATE_STABLE + * signal is not set dependable after a reset. Use the internal + * fixed card present signal to initiate the card detect logic. + */ + if (!dev_read_bool(dev, "non-removable")) { + ret = zynqmp_pm_set_sd_config(priv->node_id, SD_CONFIG_EMMC_SEL, + 1); + if (ret) { + dev_err(dev, "SD_CONFIG_EMMC_SEL failed\n"); + return ret; + } + ret = zynqmp_pm_set_sd_config(priv->node_id, SD_CONFIG_EMMC_SEL, + 0); + if (ret) { + dev_err(dev, "SD_CONFIG_EMMC_SEL failed\n"); + return ret; + } + } + return 0; } #endif