Messages in this thread | | | From | Ulf Hansson <> | Date | Thu, 24 Aug 2023 12:11:12 +0200 | 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@unisoc.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@unisoc.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
| |