linux-kernel - Re: [PATCH V2 1/2] mmc: core: Add host specific tuning support for SD HS 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: <CAPDyKFphAPOSj5L6Q8tdmD0S3EkxTW8EfMyXKsoUU2jqLydx=g@mail.gmail.com>
Date:   Thu, 24 Aug 2023 12:11:12 +0200
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     Wenchao Chen <wenchao.chen@...soc.com>
Cc:     adrian.hunter@...el.com, linux-mmc@...r.kernel.org,
        linux-kernel@...r.kernel.org, wenchao.chen666@...il.com,
        zhenxiong.lai@...soc.com, chunyan.zhang@...soc.com,
        yuelin.tang@...soc.com
Subject: Re: [PATCH V2 1/2] mmc: core: Add host specific tuning support for SD
 HS mode

On Tue, 15 Aug 2023 at 03:41, Wenchao Chen <wenchao.chen@...soc.com> wrote:
>
> Added .prepare_hs_tuning and .execute_hs_tuning host callbacks to
> support host-specific tuning for SD high speed mode.

Please clarify this is entirely optional, host specific - and that
there is nothing in the SD spec that mentions this.

>
> Signed-off-by: Wenchao Chen <wenchao.chen@...soc.com>
> ---
>  drivers/mmc/core/sd.c    | 12 ++++++++++++
>  include/linux/mmc/host.h |  6 ++++++
>  2 files changed, 18 insertions(+)
>
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index 246ce027ae0a..ac2da8f2fbce 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -1518,6 +1518,12 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
>                  */
>                 mmc_set_clock(host, mmc_sd_get_max_clock(card));
>
> +               if (host->ops->prepare_hs_tuning) {

Shouldn't we check if we actually succeeded to enable MMC_TIMING_SD_HS
before invoking this callback?

> +                       err = host->ops->prepare_hs_tuning(host, card);
> +                       if (err)
> +                               goto free_card;
> +               }
> +
>                 /*
>                  * Switch to wider bus (if supported).
>                  */
> @@ -1529,6 +1535,12 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
>
>                         mmc_set_bus_width(host, MMC_BUS_WIDTH_4);
>                 }
> +
> +               if (host->ops->execute_hs_tuning) {

Ditto.

> +                       err = host->ops->execute_hs_tuning(host, card);
> +                       if (err)
> +                               goto free_card;
> +               }
>         }
>  cont:
>         if (!oldcard) {
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 461d1543893b..13cf894b9e3c 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -184,6 +184,12 @@ struct mmc_host_ops {
>         /* Execute HS400 tuning depending host driver */
>         int     (*execute_hs400_tuning)(struct mmc_host *host, struct mmc_card *card);
>
> +       /* Prepare HS tuning depending host driver */

How about rephrasing this into something along the lines of "Optional
callback to prepare for SD high-speed tuning"

> +       int     (*prepare_hs_tuning)(struct mmc_host *host, struct mmc_card *card);

To make it more clear this if for SD high-speed, maybe we should
rename the callback into:

"prepare_sd_hs_tuning"

> +
> +       /* Execute HS tuning depending host driver */

How about rephrasing this to something along the lines of "Optional
callback to execute SD high-speed tuning"

> +       int     (*execute_hs_tuning)(struct mmc_host *host, struct mmc_card *card);

Maybe execute_sd_hs_tuning instead?

> +
>         /* Prepare switch to DDR during the HS400 init sequence */
>         int     (*hs400_prepare_ddr)(struct mmc_host *host);
>

Kind regards
Uffe