linux-kernel - Re: [PATCH v2] mmc: mtk-sd: Fix register settings for hs400(es) mode

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] [day] [month] [year] [list]
Message-ID: <CAPDyKFpXGoM+iYyMBtrnctPX_cJQv=tN52NunBRcfJecQzGy5A@mail.gmail.com>
Date: Mon, 3 Feb 2025 15:03:59 +0100
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Andy-ld Lu <andy-ld.lu@...iatek.com>
Cc: matthias.bgg@...il.com, angelogioacchino.delregno@...labora.com, 
	wenbin.mei@...iatek.com, linux-mmc@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, 
	linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH v2] mmc: mtk-sd: Fix register settings for hs400(es) mode

On Thu, 23 Jan 2025 at 10:26, Andy-ld Lu <andy-ld.lu@...iatek.com> wrote:
>
> For hs400(es) mode, the 'hs400-ds-delay' is typically configured in the
> dts. However, some projects may only define 'mediatek,hs400-ds-dly3',
> which can lead to initialization failures in hs400es mode. CMD13 reported
> response crc error in the mmc_switch_status() just after switching to
> hs400es mode.
>
> [    1.914038][   T82] mmc0: mmc_select_hs400es failed, error -84
> [    1.914954][   T82] mmc0: error -84 whilst initialising MMC card
>
> Currently, the hs400_ds_dly3 value is set within the tuning function. This
> means that the PAD_DS_DLY3 field is not configured before tuning process,
> which is the reason for the above-mentioned CMD13 response crc error.
>
> Move the PAD_DS_DLY3 field configuration into msdc_prepare_hs400_tuning(),
> and add a value check of hs400_ds_delay to prevent overwriting by zero when
> the 'hs400-ds-delay' is not set in the dts. In addition, since hs400(es)
> only tune the PAD_DS_DLY1, the PAD_DS_DLY2_SEL bit should be cleared to
> bypass it.
>
> Fixes: c4ac38c6539b ("mmc: mtk-sd: Add HS400 online tuning support")
> Signed-off-by: Andy-ld Lu <andy-ld.lu@...iatek.com>

Applied for fixes and by adding a stable-tags, thanks!

Kind regards
Uffe


> ---
> Changes in v2:
> - Change commit title from 'optimize' to 'Fix'
> - Add a Fixes tag
> - Change spaces back to tabs
>
> ---
>  drivers/mmc/host/mtk-sd.c | 31 ++++++++++++++++++++-----------
>  1 file changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> index 4b6e91372526..345ea91629e0 100644
> --- a/drivers/mmc/host/mtk-sd.c
> +++ b/drivers/mmc/host/mtk-sd.c
> @@ -273,6 +273,7 @@
>  #define MSDC_PAD_TUNE_CMD2_SEL   BIT(21)   /* RW */
>
>  #define PAD_DS_TUNE_DLY_SEL       BIT(0)         /* RW */
> +#define PAD_DS_TUNE_DLY2_SEL      BIT(1)         /* RW */
>  #define PAD_DS_TUNE_DLY1         GENMASK(6, 2)   /* RW */
>  #define PAD_DS_TUNE_DLY2         GENMASK(11, 7)  /* RW */
>  #define PAD_DS_TUNE_DLY3         GENMASK(16, 12) /* RW */
> @@ -318,6 +319,7 @@
>
>  /* EMMC50_PAD_DS_TUNE mask */
>  #define PAD_DS_DLY_SEL         BIT(16) /* RW */
> +#define PAD_DS_DLY2_SEL                BIT(15) /* RW */
>  #define PAD_DS_DLY1            GENMASK(14, 10) /* RW */
>  #define PAD_DS_DLY3            GENMASK(4, 0)   /* RW */
>
> @@ -2504,13 +2506,23 @@ static int msdc_execute_tuning(struct mmc_host *mmc, u32 opcode)
>  static int msdc_prepare_hs400_tuning(struct mmc_host *mmc, struct mmc_ios *ios)
>  {
>         struct msdc_host *host = mmc_priv(mmc);
> +
>         host->hs400_mode = true;
>
> -       if (host->top_base)
> -               writel(host->hs400_ds_delay,
> -                      host->top_base + EMMC50_PAD_DS_TUNE);
> -       else
> -               writel(host->hs400_ds_delay, host->base + PAD_DS_TUNE);
> +       if (host->top_base) {
> +               if (host->hs400_ds_dly3)
> +                       sdr_set_field(host->top_base + EMMC50_PAD_DS_TUNE,
> +                                     PAD_DS_DLY3, host->hs400_ds_dly3);
> +               if (host->hs400_ds_delay)
> +                       writel(host->hs400_ds_delay,
> +                              host->top_base + EMMC50_PAD_DS_TUNE);
> +       } else {
> +               if (host->hs400_ds_dly3)
> +                       sdr_set_field(host->base + PAD_DS_TUNE,
> +                                     PAD_DS_TUNE_DLY3, host->hs400_ds_dly3);
> +               if (host->hs400_ds_delay)
> +                       writel(host->hs400_ds_delay, host->base + PAD_DS_TUNE);
> +       }
>         /* hs400 mode must set it to 0 */
>         sdr_clr_bits(host->base + MSDC_PATCH_BIT2, MSDC_PATCH_BIT2_CFGCRCSTS);
>         /* to improve read performance, set outstanding to 2 */
> @@ -2530,14 +2542,11 @@ static int msdc_execute_hs400_tuning(struct mmc_host *mmc, struct mmc_card *card
>         if (host->top_base) {
>                 sdr_set_bits(host->top_base + EMMC50_PAD_DS_TUNE,
>                              PAD_DS_DLY_SEL);
> -               if (host->hs400_ds_dly3)
> -                       sdr_set_field(host->top_base + EMMC50_PAD_DS_TUNE,
> -                                     PAD_DS_DLY3, host->hs400_ds_dly3);
> +               sdr_clr_bits(host->top_base + EMMC50_PAD_DS_TUNE,
> +                            PAD_DS_DLY2_SEL);
>         } else {
>                 sdr_set_bits(host->base + PAD_DS_TUNE, PAD_DS_TUNE_DLY_SEL);
> -               if (host->hs400_ds_dly3)
> -                       sdr_set_field(host->base + PAD_DS_TUNE,
> -                                     PAD_DS_TUNE_DLY3, host->hs400_ds_dly3);
> +               sdr_clr_bits(host->base + PAD_DS_TUNE, PAD_DS_TUNE_DLY2_SEL);
>         }
>
>         host->hs400_tuning = true;
> --
> 2.46.0
>