linux-kernel - Re: [PATCH v4] mmc: core: Set HS clock speed before sending HS CMD13

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2235553.ElGaqSPkdT@g550jk>
Date:   Sat, 23 Apr 2022 16:41:23 +0200
From:   Luca Weiss <luca@...tu.xyz>
To:     Ulf Hansson <ulf.hansson@...aro.org>,
        Brian Norris <briannorris@...omium.org>
Cc:     Adrian Hunter <adrian.hunter@...el.com>,
        linux-kernel@...r.kernel.org,
        Heiner Kallweit <hkallweit1@...il.com>,
        Shawn Lin <shawn.lin@...k-chips.com>,
        linux-mmc@...r.kernel.org,
        Douglas Anderson <dianders@...omium.org>,
        Brian Norris <briannorris@...omium.org>
Subject: Re: [PATCH v4] mmc: core: Set HS clock speed before sending HS CMD13

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
> <shawn.lin@...k-chips.com>
> Link: https://lore.kernel.org/linux-mmc/11962455.O9o76ZdvQC@g550jk/
> Reported-by: Luca Weiss <luca@...tu.xyz>
> Signed-off-by: Brian Norris <briannorris@...omium.org>

Verified on qcom-apq8026-lg-lenok which also showed the same error with the 
last revision of this patch.

Tested-by: Luca Weiss <luca@...tu.xyz>

Regards
Luca

> ---
> 
> Changes in v4:
>  * Revert to hs_max_dtr for HS200, due to issues reported by Luca Weiss
>    <luca@...tu.xyz>; 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(-)
>