[PATCH V2 0/2] mmc: sdhci-sprd: Add SD HS mode online tuning

[PATCH V2 0/2] mmc: sdhci-sprd: Add SD HS mode online tuning

[Wenchao Chen]

Change in v2:
- add mmc_sd_switch() and mmc_send_status() to the header file
- split up core changes from host driver changes
- Use pr_debug instead of dev_info and dev_dbg
- Optimize the best sampled value algorithm

Wenchao Chen (2):
  mmc: core: Add host specific tuning support for SD HS mode
  mmc: sdhci-sprd: Add SD HS mode online tuning

 drivers/mmc/core/sd.c         |  12 +++
 drivers/mmc/core/sd_ops.c     |   1 +
 drivers/mmc/host/sdhci-sprd.c | 152 ++++++++++++++++++++++++++++++++++
 include/linux/mmc/host.h      |   8 ++
 4 files changed, 173 insertions(+)

-- 
2.17.1

[PATCH V2 1/2] mmc: core: Add host specific tuning support for SD HS mode

[Wenchao Chen]

Added .prepare_hs_tuning and .execute_hs_tuning host callbacks to
support host-specific tuning for SD high speed mode.

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) {
+			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) {
+			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 */
+	int	(*prepare_hs_tuning)(struct mmc_host *host, struct mmc_card *card);
+
+	/* Execute HS tuning depending host driver */
+	int	(*execute_hs_tuning)(struct mmc_host *host, struct mmc_card *card);
+
 	/* Prepare switch to DDR during the HS400 init sequence */
 	int	(*hs400_prepare_ddr)(struct mmc_host *host);
 
-- 
2.17.1

Re: [PATCH V2 1/2] mmc: core: Add host specific tuning support for SD HS mode

[Ulf Hansson]

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

[PATCH V2 2/2] mmc: sdhci-sprd: Add SD HS mode online tuning

[Wenchao Chen]

First of all, Unisoc's IC provides cmd delay and read delay to ensure
that the host can get the correct data. However, according to SD Spec,
there is no need to do tuning in high speed mode, but with the
development of chip processes, it is more and more difficult to find
a suitable delay to cover all the chips. Therefore, we need SD high
speed mode online tuning.

In addition, we added mmc_sd_switch() and mmc_send_status() to the
header file to allow it to be usable by the drive

Signed-off-by: Wenchao Chen <wenchao.chen@unisoc.com>
---
 drivers/mmc/core/sd_ops.c     |   1 +
 drivers/mmc/host/sdhci-sprd.c | 152 ++++++++++++++++++++++++++++++++++
 include/linux/mmc/host.h      |   2 +
 3 files changed, 155 insertions(+)

diff --git a/drivers/mmc/core/sd_ops.c b/drivers/mmc/core/sd_ops.c
index ef8d1dce5af1..a59cd592f06e 100644
--- a/drivers/mmc/core/sd_ops.c
+++ b/drivers/mmc/core/sd_ops.c
@@ -323,6 +323,7 @@ int mmc_sd_switch(struct mmc_card *card, int mode, int group,
 	return mmc_send_adtc_data(card, card->host, SD_SWITCH, cmd_args, resp,
 				  64);
 }
+EXPORT_SYMBOL_GPL(mmc_sd_switch);
 
 int mmc_app_sd_status(struct mmc_card *card, void *ssr)
 {
diff --git a/drivers/mmc/host/sdhci-sprd.c b/drivers/mmc/host/sdhci-sprd.c
index 7f4ee2e12735..96c59e010d4c 100644
--- a/drivers/mmc/host/sdhci-sprd.c
+++ b/drivers/mmc/host/sdhci-sprd.c
@@ -9,6 +9,8 @@
 #include <linux/dma-mapping.h>
 #include <linux/highmem.h>
 #include <linux/iopoll.h>
+#include <linux/mmc/mmc.h>
+#include <linux/mmc/host.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
@@ -73,6 +75,11 @@
 #define SDHCI_SPRD_CLK_DEF_RATE		26000000
 #define SDHCI_SPRD_PHY_DLL_CLK		52000000
 
+#define SDHCI_SPRD_MAX_RANGE		0xff
+#define SDHCI_SPRD_CMD_DLY_MASK		GENMASK(15, 8)
+#define SDHCI_SPRD_POSRD_DLY_MASK	GENMASK(23, 16)
+#define SDHCI_SPRD_CPST_EN		GENMASK(27, 24)
+
 struct sdhci_sprd_host {
 	u32 version;
 	struct clk *clk_sdio;
@@ -86,6 +93,11 @@ struct sdhci_sprd_host {
 	u32 phy_delay[MMC_TIMING_MMC_HS400 + 2];
 };
 
+enum sdhci_sprd_tuning_type {
+	SDHCI_SPRD_TUNING_SD_HS_CMD,
+	SDHCI_SPRD_TUNING_SD_HS_DATA,
+};
+
 struct sdhci_sprd_phy_cfg {
 	const char *property;
 	u8 timing;
@@ -533,6 +545,141 @@ static void sdhci_sprd_hs400_enhanced_strobe(struct mmc_host *mmc,
 		     SDHCI_SPRD_REG_32_DLL_DLY);
 }
 
+static int mmc_send_tuning_cmd(struct mmc_card *card)
+{
+	return mmc_send_status(card, NULL);
+}
+
+static int mmc_send_tuning_data(struct mmc_card *card)
+{
+	u8 *status;
+	int ret;
+
+	status = kmalloc(64, GFP_KERNEL);
+	if (!status)
+		return -ENOMEM;
+
+	ret = mmc_sd_switch(card, 0, 0, 0, status);
+
+	kfree(status);
+
+	return ret;
+}
+
+static int sdhci_sprd_get_best_clk_sample(struct mmc_host *mmc, u8 *value)
+{
+	int range_end = SDHCI_SPRD_MAX_RANGE;
+	int range_length = 0;
+	int middle_range = 0;
+	int count = 0;
+	int i;
+
+	for (i = 0; i <= SDHCI_SPRD_MAX_RANGE; i++) {
+		if (value[i]) {
+			pr_debug("%s: tuning ok: %d\n", mmc_hostname(mmc), i);
+			count++;
+		} else {
+			pr_debug("%s: tuning fail: %d\n", mmc_hostname(mmc), i);
+			if (range_length < count) {
+				range_length = count;
+				range_end = i - 1;
+				count = 0;
+			}
+		}
+	}
+
+	if (!count) {
+		middle_range = 0;
+		return -EIO;
+	}
+
+	if (count > range_length) {
+		range_length = count;
+		range_end = i - 1;
+	}
+
+	middle_range = range_end - (range_length - 1) / 2;
+
+	return middle_range;
+}
+
+
+static int sdhci_sprd_tuning(struct mmc_host *mmc, struct mmc_card *card,
+			enum sdhci_sprd_tuning_type type)
+{
+	struct sdhci_host *host = mmc_priv(mmc);
+	struct sdhci_sprd_host *sprd_host = TO_SPRD_HOST(host);
+	u32 *p = sprd_host->phy_delay;
+	u32 dll_cfg, dll_dly;
+	int best_clk_sample;
+	int err = 0;
+	u8 *value;
+	int i;
+
+	value = kmalloc(SDHCI_SPRD_MAX_RANGE + 1, GFP_KERNEL);
+	if (!value)
+		return -ENOMEM;
+
+	sdhci_reset(host, SDHCI_RESET_CMD | SDHCI_RESET_DATA);
+
+	dll_cfg = sdhci_readl(host, SDHCI_SPRD_REG_32_DLL_CFG);
+	dll_cfg &= ~SDHCI_SPRD_CPST_EN;
+	sdhci_writel(host, dll_cfg, SDHCI_SPRD_REG_32_DLL_CFG);
+
+	dll_dly = p[mmc->ios.timing];
+
+	for (i = 0; i <= SDHCI_SPRD_MAX_RANGE; i++) {
+		if (type == SDHCI_SPRD_TUNING_SD_HS_CMD) {
+			dll_dly &= ~SDHCI_SPRD_CMD_DLY_MASK;
+			dll_dly |= ((i << 8) & SDHCI_SPRD_CMD_DLY_MASK);
+		} else {
+			dll_dly &= ~SDHCI_SPRD_POSRD_DLY_MASK;
+			dll_dly |= ((i << 16) & SDHCI_SPRD_POSRD_DLY_MASK);
+		}
+
+		sdhci_writel(host, dll_dly, SDHCI_SPRD_REG_32_DLL_DLY);
+
+		if (type == SDHCI_SPRD_TUNING_SD_HS_CMD)
+			value[i] = !mmc_send_tuning_cmd(card);
+		else
+			value[i] = !mmc_send_tuning_data(card);
+	}
+
+	best_clk_sample = sdhci_sprd_get_best_clk_sample(mmc, value);
+	if (best_clk_sample < 0) {
+		dev_err(mmc_dev(host->mmc), "all tuning phase fail!\n");
+		goto out;
+	}
+
+	if (type == SDHCI_SPRD_TUNING_SD_HS_CMD) {
+		p[mmc->ios.timing] &= ~SDHCI_SPRD_CMD_DLY_MASK;
+		p[mmc->ios.timing] |= ((best_clk_sample << 8) & SDHCI_SPRD_CMD_DLY_MASK);
+	} else {
+		p[mmc->ios.timing] &= ~(SDHCI_SPRD_POSRD_DLY_MASK);
+		p[mmc->ios.timing] |= ((best_clk_sample << 16) & SDHCI_SPRD_POSRD_DLY_MASK);
+	}
+
+	pr_debug("%s: the best clk sample %d, delay value 0x%08x\n",
+			mmc_hostname(host->mmc), best_clk_sample, p[mmc->ios.timing]);
+
+out:
+	sdhci_writel(host, p[mmc->ios.timing], SDHCI_SPRD_REG_32_DLL_DLY);
+
+	kfree(value);
+
+	return err;
+}
+
+static int sdhci_sprd_prepare_hs_cmd_tuning(struct mmc_host *mmc, struct mmc_card *card)
+{
+	return sdhci_sprd_tuning(mmc, card, SDHCI_SPRD_TUNING_SD_HS_CMD);
+}
+
+static int sdhci_sprd_execute_hs_data_tuning(struct mmc_host *mmc, struct mmc_card *card)
+{
+	return sdhci_sprd_tuning(mmc, card, SDHCI_SPRD_TUNING_SD_HS_DATA);
+}
+
 static void sdhci_sprd_phy_param_parse(struct sdhci_sprd_host *sprd_host,
 				       struct device_node *np)
 {
@@ -577,6 +724,11 @@ static int sdhci_sprd_probe(struct platform_device *pdev)
 	host->mmc_host_ops.request = sdhci_sprd_request;
 	host->mmc_host_ops.hs400_enhanced_strobe =
 		sdhci_sprd_hs400_enhanced_strobe;
+	host->mmc_host_ops.prepare_hs_tuning =
+		sdhci_sprd_prepare_hs_cmd_tuning;
+	host->mmc_host_ops.execute_hs_tuning =
+		sdhci_sprd_execute_hs_data_tuning;
+
 	/*
 	 * We can not use the standard ops to change and detect the voltage
 	 * signal for Spreadtrum SD host controller, since our voltage regulator
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 13cf894b9e3c..5d069510dc4b 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -671,6 +671,8 @@ static inline void mmc_debugfs_err_stats_inc(struct mmc_host *host,
 	host->err_stats[stat] += 1;
 }
 
+int mmc_sd_switch(struct mmc_card *card, int mode, int group, u8 value, u8 *resp);
+int mmc_send_status(struct mmc_card *card, u32 *status);
 int mmc_send_tuning(struct mmc_host *host, u32 opcode, int *cmd_error);
 int mmc_send_abort_tuning(struct mmc_host *host, u32 opcode);
 int mmc_get_ext_csd(struct mmc_card *card, u8 **new_ext_csd);
-- 
2.17.1

Re: [PATCH V2 2/2] mmc: sdhci-sprd: Add SD HS mode online tuning

[Ulf Hansson]

On Tue, 15 Aug 2023 at 03:41, Wenchao Chen <wenchao.chen@unisoc.com> wrote:
>
> First of all, Unisoc's IC provides cmd delay and read delay to ensure
> that the host can get the correct data. However, according to SD Spec,
> there is no need to do tuning in high speed mode, but with the
> development of chip processes, it is more and more difficult to find
> a suitable delay to cover all the chips. Therefore, we need SD high
> speed mode online tuning.
>
> In addition, we added mmc_sd_switch() and mmc_send_status() to the
> header file to allow it to be usable by the drive
>
> Signed-off-by: Wenchao Chen <wenchao.chen@unisoc.com>
> ---
>  drivers/mmc/core/sd_ops.c     |   1 +
>  drivers/mmc/host/sdhci-sprd.c | 152 ++++++++++++++++++++++++++++++++++
>  include/linux/mmc/host.h      |   2 +
>  3 files changed, 155 insertions(+)
>
> diff --git a/drivers/mmc/core/sd_ops.c b/drivers/mmc/core/sd_ops.c
> index ef8d1dce5af1..a59cd592f06e 100644
> --- a/drivers/mmc/core/sd_ops.c
> +++ b/drivers/mmc/core/sd_ops.c
> @@ -323,6 +323,7 @@ int mmc_sd_switch(struct mmc_card *card, int mode, int group,
>         return mmc_send_adtc_data(card, card->host, SD_SWITCH, cmd_args, resp,
>                                   64);
>  }
> +EXPORT_SYMBOL_GPL(mmc_sd_switch);

Please move changes in include/linux/mmc/host.h and
drivers/mmc/core/sd_ops.c into patch1. When doing that, please update
the commit messages too.

[...]

Other than the above, this looks okay to me!

Kind regards
Uffe

Re: [PATCH V2 0/2] mmc: sdhci-sprd: Add SD HS mode online tuning

[Adrian Hunter]

On 15/08/23 04:40, Wenchao Chen wrote:
> Change in v2:
> - add mmc_sd_switch() and mmc_send_status() to the header file
> - split up core changes from host driver changes
> - Use pr_debug instead of dev_info and dev_dbg
> - Optimize the best sampled value algorithm

What about hooking ->set_ios() as Ulf suggested?

> 
> Wenchao Chen (2):
>   mmc: core: Add host specific tuning support for SD HS mode
>   mmc: sdhci-sprd: Add SD HS mode online tuning
> 
>  drivers/mmc/core/sd.c         |  12 +++
>  drivers/mmc/core/sd_ops.c     |   1 +
>  drivers/mmc/host/sdhci-sprd.c | 152 ++++++++++++++++++++++++++++++++++
>  include/linux/mmc/host.h      |   8 ++
>  4 files changed, 173 insertions(+)
> 

Re: [PATCH V2 0/2] mmc: sdhci-sprd: Add SD HS mode online tuning

[Wenchao Chen]

On Tue, Aug 15, 2023 at 2:21 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 15/08/23 04:40, Wenchao Chen wrote:
> > Change in v2:
> > - add mmc_sd_switch() and mmc_send_status() to the header file
> > - split up core changes from host driver changes
> > - Use pr_debug instead of dev_info and dev_dbg
> > - Optimize the best sampled value algorithm
>
> What about hooking ->set_ios() as Ulf suggested?
>

I've tried that, but it's not a good way to do it.
We found that sdhci_runtime_resume_host() calls ->set_ios, but we
don't want to do that.
We just need SD HS mode tuning at mmc_sd_init_card().

> >
> > Wenchao Chen (2):
> >   mmc: core: Add host specific tuning support for SD HS mode
> >   mmc: sdhci-sprd: Add SD HS mode online tuning
> >
> >  drivers/mmc/core/sd.c         |  12 +++
> >  drivers/mmc/core/sd_ops.c     |   1 +
> >  drivers/mmc/host/sdhci-sprd.c | 152 ++++++++++++++++++++++++++++++++++
> >  include/linux/mmc/host.h      |   8 ++
> >  4 files changed, 173 insertions(+)
> >
>

Re: [PATCH V2 0/2] mmc: sdhci-sprd: Add SD HS mode online tuning

[Adrian Hunter]

On 15/08/23 13:29, Wenchao Chen wrote:
> On Tue, Aug 15, 2023 at 2:21 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>
>> On 15/08/23 04:40, Wenchao Chen wrote:
>>> Change in v2:
>>> - add mmc_sd_switch() and mmc_send_status() to the header file
>>> - split up core changes from host driver changes
>>> - Use pr_debug instead of dev_info and dev_dbg
>>> - Optimize the best sampled value algorithm
>>
>> What about hooking ->set_ios() as Ulf suggested?
>>
> 
> I've tried that, but it's not a good way to do it.
> We found that sdhci_runtime_resume_host() calls ->set_ios, but we
> don't want to do that.

Given that sdhci_sprd_runtime_resume() calls sdhci_runtime_resume_host(),
it should be possible to determine when to tune, right?

> We just need SD HS mode tuning at mmc_sd_init_card().
> 
>>>
>>> Wenchao Chen (2):
>>>   mmc: core: Add host specific tuning support for SD HS mode
>>>   mmc: sdhci-sprd: Add SD HS mode online tuning
>>>
>>>  drivers/mmc/core/sd.c         |  12 +++
>>>  drivers/mmc/core/sd_ops.c     |   1 +
>>>  drivers/mmc/host/sdhci-sprd.c | 152 ++++++++++++++++++++++++++++++++++
>>>  include/linux/mmc/host.h      |   8 ++
>>>  4 files changed, 173 insertions(+)
>>>
>>

Re: [PATCH V2 0/2] mmc: sdhci-sprd: Add SD HS mode online tuning

[Wenchao Chen]

On Tue, Aug 15, 2023 at 6:37 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 15/08/23 13:29, Wenchao Chen wrote:
> > On Tue, Aug 15, 2023 at 2:21 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>
> >> On 15/08/23 04:40, Wenchao Chen wrote:
> >>> Change in v2:
> >>> - add mmc_sd_switch() and mmc_send_status() to the header file
> >>> - split up core changes from host driver changes
> >>> - Use pr_debug instead of dev_info and dev_dbg
> >>> - Optimize the best sampled value algorithm
> >>
> >> What about hooking ->set_ios() as Ulf suggested?
> >>
> >
> > I've tried that, but it's not a good way to do it.
> > We found that sdhci_runtime_resume_host() calls ->set_ios, but we
> > don't want to do that.
>
> Given that sdhci_sprd_runtime_resume() calls sdhci_runtime_resume_host(),
> it should be possible to determine when to tune, right?
>

You mean like this? For example:
static int sdhci_sprd_runtime_resume(struct device *dev)
{
...
sprd_host->need_hs_tuning = false;
sdhci_runtime_resume_host(host, 1);
sprd_host->need_hs_tuning = true;
...
}

> > We just need SD HS mode tuning at mmc_sd_init_card().
> >
> >>>
> >>> Wenchao Chen (2):
> >>>   mmc: core: Add host specific tuning support for SD HS mode
> >>>   mmc: sdhci-sprd: Add SD HS mode online tuning
> >>>
> >>>  drivers/mmc/core/sd.c         |  12 +++
> >>>  drivers/mmc/core/sd_ops.c     |   1 +
> >>>  drivers/mmc/host/sdhci-sprd.c | 152 ++++++++++++++++++++++++++++++++++
> >>>  include/linux/mmc/host.h      |   8 ++
> >>>  4 files changed, 173 insertions(+)
> >>>
> >>
>

Re: [PATCH V2 0/2] mmc: sdhci-sprd: Add SD HS mode online tuning

[Adrian Hunter]

On 15/08/23 13:55, Wenchao Chen wrote:
> On Tue, Aug 15, 2023 at 6:37 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>
>> On 15/08/23 13:29, Wenchao Chen wrote:
>>> On Tue, Aug 15, 2023 at 2:21 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>
>>>> On 15/08/23 04:40, Wenchao Chen wrote:
>>>>> Change in v2:
>>>>> - add mmc_sd_switch() and mmc_send_status() to the header file
>>>>> - split up core changes from host driver changes
>>>>> - Use pr_debug instead of dev_info and dev_dbg
>>>>> - Optimize the best sampled value algorithm
>>>>
>>>> What about hooking ->set_ios() as Ulf suggested?
>>>>
>>>
>>> I've tried that, but it's not a good way to do it.
>>> We found that sdhci_runtime_resume_host() calls ->set_ios, but we
>>> don't want to do that.
>>
>> Given that sdhci_sprd_runtime_resume() calls sdhci_runtime_resume_host(),
>> it should be possible to determine when to tune, right?
>>
> 
> You mean like this? For example:
> static int sdhci_sprd_runtime_resume(struct device *dev)
> {
> ...
> sprd_host->need_hs_tuning = false;
> sdhci_runtime_resume_host(host, 1);
> sprd_host->need_hs_tuning = true;

Yes

> ...
> }
> 
>>> We just need SD HS mode tuning at mmc_sd_init_card().
>>>
>>>>>
>>>>> Wenchao Chen (2):
>>>>>   mmc: core: Add host specific tuning support for SD HS mode
>>>>>   mmc: sdhci-sprd: Add SD HS mode online tuning
>>>>>
>>>>>  drivers/mmc/core/sd.c         |  12 +++
>>>>>  drivers/mmc/core/sd_ops.c     |   1 +
>>>>>  drivers/mmc/host/sdhci-sprd.c | 152 ++++++++++++++++++++++++++++++++++
>>>>>  include/linux/mmc/host.h      |   8 ++
>>>>>  4 files changed, 173 insertions(+)
>>>>>
>>>>
>>

Re: [PATCH V2 0/2] mmc: sdhci-sprd: Add SD HS mode online tuning

[Wenchao Chen]

On Tue, Aug 15, 2023 at 7:19 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 15/08/23 13:55, Wenchao Chen wrote:
> > On Tue, Aug 15, 2023 at 6:37 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>
> >> On 15/08/23 13:29, Wenchao Chen wrote:
> >>> On Tue, Aug 15, 2023 at 2:21 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>>>
> >>>> On 15/08/23 04:40, Wenchao Chen wrote:
> >>>>> Change in v2:
> >>>>> - add mmc_sd_switch() and mmc_send_status() to the header file
> >>>>> - split up core changes from host driver changes
> >>>>> - Use pr_debug instead of dev_info and dev_dbg
> >>>>> - Optimize the best sampled value algorithm
> >>>>
> >>>> What about hooking ->set_ios() as Ulf suggested?
> >>>>
> >>>
> >>> I've tried that, but it's not a good way to do it.
> >>> We found that sdhci_runtime_resume_host() calls ->set_ios, but we
> >>> don't want to do that.
> >>
> >> Given that sdhci_sprd_runtime_resume() calls sdhci_runtime_resume_host(),
> >> it should be possible to determine when to tune, right?
> >>
> >
> > You mean like this? For example:
> > static int sdhci_sprd_runtime_resume(struct device *dev)
> > {
> > ...
> > sprd_host->need_hs_tuning = false;
> > sdhci_runtime_resume_host(host, 1);
> > sprd_host->need_hs_tuning = true;
>
> Yes
>

This works, but we found a problem: if sd hs tuning fails, ->set_ios
is of type void with no return value.

> > ...
> > }
> >
> >>> We just need SD HS mode tuning at mmc_sd_init_card().
> >>>
> >>>>>
> >>>>> Wenchao Chen (2):
> >>>>>   mmc: core: Add host specific tuning support for SD HS mode
> >>>>>   mmc: sdhci-sprd: Add SD HS mode online tuning
> >>>>>
> >>>>>  drivers/mmc/core/sd.c         |  12 +++
> >>>>>  drivers/mmc/core/sd_ops.c     |   1 +
> >>>>>  drivers/mmc/host/sdhci-sprd.c | 152 ++++++++++++++++++++++++++++++++++
> >>>>>  include/linux/mmc/host.h      |   8 ++
> >>>>>  4 files changed, 173 insertions(+)
> >>>>>
> >>>>
> >>
>