On Tue, 15 Aug 2023 at 03:41, Wenchao Chen <wenchao.chen@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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
