Way back in commit 4f25580fb84d ("mmc: core: changes frequency to
hs_max_dtr when selecting hs400es"), Rockchip engineers noticed that
some eMMC don't respond to SEND_STATUS commands very reliably if they're
still running at a low initial frequency. As mentioned in that commit,
JESD84-B51 P49 suggests a sequence in which the host:
1. sets HS_TIMING
2. bumps the clock ("<= 52 MHz")
3. sends further commands
It doesn't exactly require that we don't use a lower-than-52MHz
frequency, but in practice, these eMMC don't like it.
The aforementioned commit tried to get that right for HS400ES, although
it's unclear whether this ever truly worked as committed into mainline,
as other changes/refactoring adjusted the sequence in conflicting ways:
08573eaf1a70 ("mmc: mmc: do not use CMD13 to get status after speed mode
switch")
53e60650f74e ("mmc: core: Allow CMD13 polling when switching to HS mode
for mmc")
In any case, today we do step 3 before step 2. Let's fix that, and also
apply the same logic to HS200/400, where this eMMC has problems too.
Resolves errors like this seen when booting some RK3399 Gru/Scarlet
systems:
[ 2.058881] mmc1: CQHCI version 5.10
[ 2.097545] mmc1: SDHCI controller on fe330000.mmc [fe330000.mmc] using ADMA
[ 2.209804] mmc1: mmc_select_hs400es failed, error -84
[ 2.215597] mmc1: error -84 whilst initialising MMC card
[ 2.417514] mmc1: mmc_select_hs400es failed, error -110
[ 2.423373] mmc1: error -110 whilst initialising MMC card
[ 2.605052] mmc1: mmc_select_hs400es failed, error -110
[ 2.617944] mmc1: error -110 whilst initialising MMC card
[ 2.835884] mmc1: mmc_select_hs400es failed, error -110
[ 2.841751] mmc1: error -110 whilst initialising MMC card
Ealier versions of this patch bumped to 200MHz/HS200 speeds too early,
which caused issues on, e.g., qcom-msm8974-fairphone-fp2. (Thanks for
the report Luca!) After a second look, it appears that aligns with
JESD84 / page 45 / table 28, so we need to keep to lower (HS / 52 MHz)
rates first.
Fixes: 08573eaf1a70 ("mmc: mmc: do not use CMD13 to get status after speed mode switch")
Fixes: 53e60650f74e ("mmc: core: Allow CMD13 polling when switching to HS mode for mmc")
Fixes: 4f25580fb84d ("mmc: core: changes frequency to hs_max_dtr when selecting hs400es")
Cc: Shawn Lin <[email protected]>
Link: https://lore.kernel.org/linux-mmc/11962455.O9o76ZdvQC@g550jk/
Reported-by: Luca Weiss <[email protected]>
Signed-off-by: Brian Norris <[email protected]>
---
Changes in v4:
* Revert to hs_max_dtr for HS200, due to issues reported by Luca Weiss
<[email protected]>; Luca, feel free to provide a "Tested-by: ..." reply
if you'd like that included
* Drop the "redundant clock rate" changes, as they aren't needed any
more
Changes in v3:
* Use mmc_set_bus_speed() to help choose the right clock rate
* Avoid redundant clock rate changes
* Restore clock rate on failed HS200 switch
Changes in v2:
* Use ext_csd.hs200_max_dtr for HS200
* Retest on top of 3b6c472822f8 ("mmc: core: Improve fallback to speed
modes if eMMC HS200 fails")
drivers/mmc/core/mmc.c | 23 +++++++++++++++++++----
1 file changed, 19 insertions(+), 4 deletions(-)
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 5d8d9f72476f..82ca62c8669c 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1384,13 +1384,17 @@ static int mmc_select_hs400es(struct mmc_card *card)
goto out_err;
}
+ /*
+ * Bump to HS timing and frequency. Some cards don't handle
+ * SEND_STATUS reliably at the initial frequency.
+ */
mmc_set_timing(host, MMC_TIMING_MMC_HS);
+ mmc_set_bus_speed(card);
+
err = mmc_switch_status(card, true);
if (err)
goto out_err;
- mmc_set_clock(host, card->ext_csd.hs_max_dtr);
-
/* Switch card to DDR with strobe bit */
val = EXT_CSD_DDR_BUS_WIDTH_8 | EXT_CSD_BUS_WIDTH_STROBE;
err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
@@ -1448,7 +1452,7 @@ static int mmc_select_hs400es(struct mmc_card *card)
static int mmc_select_hs200(struct mmc_card *card)
{
struct mmc_host *host = card->host;
- unsigned int old_timing, old_signal_voltage;
+ unsigned int old_timing, old_signal_voltage, old_clock;
int err = -EINVAL;
u8 val;
@@ -1479,8 +1483,17 @@ static int mmc_select_hs200(struct mmc_card *card)
false, true, MMC_CMD_RETRIES);
if (err)
goto err;
+
+ /*
+ * Bump to HS timing and frequency. Some cards don't handle
+ * SEND_STATUS reliably at the initial frequency.
+ * NB: We can't move to full (HS200) speeds until after we've
+ * successfully switched over.
+ */
old_timing = host->ios.timing;
+ old_clock = host->ios.clock;
mmc_set_timing(host, MMC_TIMING_MMC_HS200);
+ mmc_set_clock(card->host, card->ext_csd.hs_max_dtr);
/*
* For HS200, CRC errors are not a reliable way to know the
@@ -1493,8 +1506,10 @@ static int mmc_select_hs200(struct mmc_card *card)
* mmc_select_timing() assumes timing has not changed if
* it is a switch error.
*/
- if (err == -EBADMSG)
+ if (err == -EBADMSG) {
+ mmc_set_clock(host, old_clock);
mmc_set_timing(host, old_timing);
+ }
}
err:
if (err) {
--
2.36.0.rc2.479.g8af0fa9b8e-goog
Hi Brian,
On Freitag, 22. April 2022 19:08:53 CEST Brian Norris wrote:
> Way back in commit 4f25580fb84d ("mmc: core: changes frequency to
> hs_max_dtr when selecting hs400es"), Rockchip engineers noticed that
> some eMMC don't respond to SEND_STATUS commands very reliably if they're
> still running at a low initial frequency. As mentioned in that commit,
> JESD84-B51 P49 suggests a sequence in which the host:
> 1. sets HS_TIMING
> 2. bumps the clock ("<= 52 MHz")
> 3. sends further commands
>
> It doesn't exactly require that we don't use a lower-than-52MHz
> frequency, but in practice, these eMMC don't like it.
>
> The aforementioned commit tried to get that right for HS400ES, although
> it's unclear whether this ever truly worked as committed into mainline,
> as other changes/refactoring adjusted the sequence in conflicting ways:
>
> 08573eaf1a70 ("mmc: mmc: do not use CMD13 to get status after speed mode
> switch")
>
> 53e60650f74e ("mmc: core: Allow CMD13 polling when switching to HS mode
> for mmc")
>
> In any case, today we do step 3 before step 2. Let's fix that, and also
> apply the same logic to HS200/400, where this eMMC has problems too.
>
> Resolves errors like this seen when booting some RK3399 Gru/Scarlet
> systems:
>
> [ 2.058881] mmc1: CQHCI version 5.10
> [ 2.097545] mmc1: SDHCI controller on fe330000.mmc [fe330000.mmc] using
> ADMA [ 2.209804] mmc1: mmc_select_hs400es failed, error -84
> [ 2.215597] mmc1: error -84 whilst initialising MMC card
> [ 2.417514] mmc1: mmc_select_hs400es failed, error -110
> [ 2.423373] mmc1: error -110 whilst initialising MMC card
> [ 2.605052] mmc1: mmc_select_hs400es failed, error -110
> [ 2.617944] mmc1: error -110 whilst initialising MMC card
> [ 2.835884] mmc1: mmc_select_hs400es failed, error -110
> [ 2.841751] mmc1: error -110 whilst initialising MMC card
>
> Ealier versions of this patch bumped to 200MHz/HS200 speeds too early,
> which caused issues on, e.g., qcom-msm8974-fairphone-fp2. (Thanks for
> the report Luca!) After a second look, it appears that aligns with
> JESD84 / page 45 / table 28, so we need to keep to lower (HS / 52 MHz)
> rates first.
>
> Fixes: 08573eaf1a70 ("mmc: mmc: do not use CMD13 to get status after speed
> mode switch") Fixes: 53e60650f74e ("mmc: core: Allow CMD13 polling when
> switching to HS mode for mmc") Fixes: 4f25580fb84d ("mmc: core: changes
> frequency to hs_max_dtr when selecting hs400es") Cc: Shawn Lin
> <[email protected]>
> Link: https://lore.kernel.org/linux-mmc/11962455.O9o76ZdvQC@g550jk/
> Reported-by: Luca Weiss <[email protected]>
> Signed-off-by: Brian Norris <[email protected]>
Verified on qcom-apq8026-lg-lenok which also showed the same error with the
last revision of this patch.
Tested-by: Luca Weiss <[email protected]>
Regards
Luca
> ---
>
> Changes in v4:
> * Revert to hs_max_dtr for HS200, due to issues reported by Luca Weiss
> <[email protected]>; Luca, feel free to provide a "Tested-by: ..." reply
> if you'd like that included
> * Drop the "redundant clock rate" changes, as they aren't needed any
> more
>
> Changes in v3:
> * Use mmc_set_bus_speed() to help choose the right clock rate
> * Avoid redundant clock rate changes
> * Restore clock rate on failed HS200 switch
>
> Changes in v2:
> * Use ext_csd.hs200_max_dtr for HS200
> * Retest on top of 3b6c472822f8 ("mmc: core: Improve fallback to speed
> modes if eMMC HS200 fails")
>
> drivers/mmc/core/mmc.c | 23 +++++++++++++++++++----
> 1 file changed, 19 insertions(+), 4 deletions(-)
>
On Fri, 22 Apr 2022 at 19:09, Brian Norris <[email protected]> wrote:
>
> Way back in commit 4f25580fb84d ("mmc: core: changes frequency to
> hs_max_dtr when selecting hs400es"), Rockchip engineers noticed that
> some eMMC don't respond to SEND_STATUS commands very reliably if they're
> still running at a low initial frequency. As mentioned in that commit,
> JESD84-B51 P49 suggests a sequence in which the host:
> 1. sets HS_TIMING
> 2. bumps the clock ("<= 52 MHz")
> 3. sends further commands
>
> It doesn't exactly require that we don't use a lower-than-52MHz
> frequency, but in practice, these eMMC don't like it.
>
> The aforementioned commit tried to get that right for HS400ES, although
> it's unclear whether this ever truly worked as committed into mainline,
> as other changes/refactoring adjusted the sequence in conflicting ways:
>
> 08573eaf1a70 ("mmc: mmc: do not use CMD13 to get status after speed mode
> switch")
>
> 53e60650f74e ("mmc: core: Allow CMD13 polling when switching to HS mode
> for mmc")
>
> In any case, today we do step 3 before step 2. Let's fix that, and also
> apply the same logic to HS200/400, where this eMMC has problems too.
>
> Resolves errors like this seen when booting some RK3399 Gru/Scarlet
> systems:
>
> [ 2.058881] mmc1: CQHCI version 5.10
> [ 2.097545] mmc1: SDHCI controller on fe330000.mmc [fe330000.mmc] using ADMA
> [ 2.209804] mmc1: mmc_select_hs400es failed, error -84
> [ 2.215597] mmc1: error -84 whilst initialising MMC card
> [ 2.417514] mmc1: mmc_select_hs400es failed, error -110
> [ 2.423373] mmc1: error -110 whilst initialising MMC card
> [ 2.605052] mmc1: mmc_select_hs400es failed, error -110
> [ 2.617944] mmc1: error -110 whilst initialising MMC card
> [ 2.835884] mmc1: mmc_select_hs400es failed, error -110
> [ 2.841751] mmc1: error -110 whilst initialising MMC card
>
> Ealier versions of this patch bumped to 200MHz/HS200 speeds too early,
> which caused issues on, e.g., qcom-msm8974-fairphone-fp2. (Thanks for
> the report Luca!) After a second look, it appears that aligns with
> JESD84 / page 45 / table 28, so we need to keep to lower (HS / 52 MHz)
> rates first.
>
> Fixes: 08573eaf1a70 ("mmc: mmc: do not use CMD13 to get status after speed mode switch")
> Fixes: 53e60650f74e ("mmc: core: Allow CMD13 polling when switching to HS mode for mmc")
> Fixes: 4f25580fb84d ("mmc: core: changes frequency to hs_max_dtr when selecting hs400es")
> Cc: Shawn Lin <[email protected]>
> Link: https://lore.kernel.org/linux-mmc/11962455.O9o76ZdvQC@g550jk/
> Reported-by: Luca Weiss <[email protected]>
> Signed-off-by: Brian Norris <[email protected]>
Applied for fixes and by adding a stable tag, thanks!
Kind regards
Uffe
> ---
>
> Changes in v4:
> * Revert to hs_max_dtr for HS200, due to issues reported by Luca Weiss
> <[email protected]>; Luca, feel free to provide a "Tested-by: ..." reply
> if you'd like that included
> * Drop the "redundant clock rate" changes, as they aren't needed any
> more
>
> Changes in v3:
> * Use mmc_set_bus_speed() to help choose the right clock rate
> * Avoid redundant clock rate changes
> * Restore clock rate on failed HS200 switch
>
> Changes in v2:
> * Use ext_csd.hs200_max_dtr for HS200
> * Retest on top of 3b6c472822f8 ("mmc: core: Improve fallback to speed
> modes if eMMC HS200 fails")
>
> drivers/mmc/core/mmc.c | 23 +++++++++++++++++++----
> 1 file changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 5d8d9f72476f..82ca62c8669c 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1384,13 +1384,17 @@ static int mmc_select_hs400es(struct mmc_card *card)
> goto out_err;
> }
>
> + /*
> + * Bump to HS timing and frequency. Some cards don't handle
> + * SEND_STATUS reliably at the initial frequency.
> + */
> mmc_set_timing(host, MMC_TIMING_MMC_HS);
> + mmc_set_bus_speed(card);
> +
> err = mmc_switch_status(card, true);
> if (err)
> goto out_err;
>
> - mmc_set_clock(host, card->ext_csd.hs_max_dtr);
> -
> /* Switch card to DDR with strobe bit */
> val = EXT_CSD_DDR_BUS_WIDTH_8 | EXT_CSD_BUS_WIDTH_STROBE;
> err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> @@ -1448,7 +1452,7 @@ static int mmc_select_hs400es(struct mmc_card *card)
> static int mmc_select_hs200(struct mmc_card *card)
> {
> struct mmc_host *host = card->host;
> - unsigned int old_timing, old_signal_voltage;
> + unsigned int old_timing, old_signal_voltage, old_clock;
> int err = -EINVAL;
> u8 val;
>
> @@ -1479,8 +1483,17 @@ static int mmc_select_hs200(struct mmc_card *card)
> false, true, MMC_CMD_RETRIES);
> if (err)
> goto err;
> +
> + /*
> + * Bump to HS timing and frequency. Some cards don't handle
> + * SEND_STATUS reliably at the initial frequency.
> + * NB: We can't move to full (HS200) speeds until after we've
> + * successfully switched over.
> + */
> old_timing = host->ios.timing;
> + old_clock = host->ios.clock;
> mmc_set_timing(host, MMC_TIMING_MMC_HS200);
> + mmc_set_clock(card->host, card->ext_csd.hs_max_dtr);
>
> /*
> * For HS200, CRC errors are not a reliable way to know the
> @@ -1493,8 +1506,10 @@ static int mmc_select_hs200(struct mmc_card *card)
> * mmc_select_timing() assumes timing has not changed if
> * it is a switch error.
> */
> - if (err == -EBADMSG)
> + if (err == -EBADMSG) {
> + mmc_set_clock(host, old_clock);
> mmc_set_timing(host, old_timing);
> + }
> }
> err:
> if (err) {
> --
> 2.36.0.rc2.479.g8af0fa9b8e-goog
>