Re: [PATCH v2 4/7] mmc: sdhci-of-k1: add comprehensive SDR tuning support

[Yao Zi <me@ziyao.cc>]

On Mon, Mar 09, 2026 at 12:40:14PM +0100, Iker Pedrosa wrote:
> Implement software tuning algorithm to enable UHS-I SDR modes for SD
> card operation. This adds both TX and RX delay line tuning based on the
> SpacemiT K1 controller capabilities.
> 
> Key features:
> - Conditional tuning: only tune when SD card is present and for
>   high-speed modes (≥100MHz)
> - TX tuning: configure transmit delay line with default values
>   (dline_reg=0, delaycode=127) to ensure optimal signal output timing
> - RX tuning: test full delay range (0-255) with window detection
>   algorithm to find optimal receive timing
> - Retry mechanism: multiple fallback delays within optimal window for
>   improved reliability
> - Complete register support: add delay line control and configuration
>   register definitions for fine-grained timing control
> 
> Signed-off-by: Iker Pedrosa <ikerpedrosam@gmail.com>

Sorry for raising this late: I think PATCH 3 and PATCH 4 (this one)
should also be squashed together to form a complete functionality, I
also doubt otherwise whether compilers complain about unused static
functions with only PATCH 3 applied.

> ---
>  drivers/mmc/host/sdhci-of-k1.c | 119 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 119 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-of-k1.c b/drivers/mmc/host/sdhci-of-k1.c
> index 79cb7c8d0b6d9c4206bf01721651c8efe8a173c9..d903851b9be0e1d21a2b30636f5e63a52cad0dc2 100644
> --- a/drivers/mmc/host/sdhci-of-k1.c
> +++ b/drivers/mmc/host/sdhci-of-k1.c
> @@ -84,6 +84,12 @@
>  #define  SDHC_TX_DLINE_REG_MASK         GENMASK(23, 16)
>  
>  #define SPACEMIT_RX_DLINE_REG		9
> +#define SPACEMIT_RX_TUNE_DELAY_MIN	0x0
> +#define SPACEMIT_RX_TUNE_DELAY_MAX	0xFF
> +#define SPACEMIT_RX_TUNE_DELAY_STEP	0x1

I think the STEP constant isn't very helpful, to me its purpose is
quite obvious in spacemit_sdhci_execute_tuning(), and you could simplify
the tuning function without adding confusing magic numbers -- since the
constant is literally one.

> +#define SPACEMIT_TX_TUNING_DLINE_REG	0x00
> +#define SPACEMIT_TX_TUNING_DELAYCODE	127

...

> +static int spacemit_sdhci_execute_tuning(struct sdhci_host *host, u32 opcode)
> +{
> +	int ret = 0;
> +	int i;
> +	bool pass_window[SPACEMIT_RX_TUNE_DELAY_MAX + 1] = {false};
> +	int pass_len = 0, pass_start = 0, max_pass_len = 0, max_pass_start = 0;
> +	u8 final_delay;
> +	struct mmc_host *mmc = host->mmc;
> +	struct mmc_ios ios = mmc->ios;
> +
> +	/*
> +	 * Tuning is required for SDR50/SDR104, HS200/HS400 cards and
> +	 * if clock frequency is greater than 100MHz in these modes.
> +	 */
> +	if (host->clock < 100 * 1000 * 1000 ||
> +	    !(ios.timing == MMC_TIMING_MMC_HS200 ||
> +	      ios.timing == MMC_TIMING_UHS_SDR50 ||
> +	      ios.timing == MMC_TIMING_UHS_SDR104))
> +		return 0;
> +
> +	if (!(mmc->caps2 & MMC_CAP2_NO_SD) && !mmc->ops->get_cd(mmc))
> +		return 0;

Is this check really necessary? Shouldn't this be handled in MMC core
instead?

> +	if (mmc->caps2 & MMC_CAP2_NO_MMC) {
> +		spacemit_sdhci_set_tx_dline_reg(host, SPACEMIT_TX_TUNING_DLINE_REG);
> +		spacemit_sdhci_set_tx_delay(host, SPACEMIT_TX_TUNING_DELAYCODE);
> +		spacemit_sdhci_tx_tuning_prepare(host);
> +
> +		dev_dbg(mmc_dev(host->mmc), "TX tuning: dline_reg=%d, delaycode=%d\n",
> +			SPACEMIT_TX_TUNING_DLINE_REG, SPACEMIT_TX_TUNING_DELAYCODE);
> +	}
> +
> +	spacemit_sdhci_prepare_tuning(host);
> +
> +	for (i = SPACEMIT_RX_TUNE_DELAY_MIN; i <= SPACEMIT_RX_TUNE_DELAY_MAX;
> +	     i += SPACEMIT_RX_TUNE_DELAY_STEP) {

The last expression could be simply i++ if you drop
SPACEMIT_RX_TUNE_DELAY_STEP, this is still quite readable. Same for the
loop below.

> +		spacemit_sdhci_set_rx_delay(host, i);
> +
> +		ret = mmc_send_tuning(host->mmc, opcode, NULL);
> +		pass_window[i] = (ret == 0);
> +
> +		dev_dbg(mmc_dev(host->mmc), "RX delay %d: %s\n",
> +			i, pass_window[i] ? "pass" : "fail");
> +	}
> +
> +	for (i = SPACEMIT_RX_TUNE_DELAY_MIN; i <= SPACEMIT_RX_TUNE_DELAY_MAX;
> +	     i += SPACEMIT_RX_TUNE_DELAY_STEP) {
> +		if (pass_window[i]) {
> +			if (pass_len == 0)
> +				pass_start = i;
> +			pass_len++;
> +		} else {
> +			if (pass_len > max_pass_len) {
> +				max_pass_len = pass_len;
> +				max_pass_start = pass_start;
> +			}
> +			pass_len = 0;
> +		}
> +	}

It seems pass_window is only used once in the later loop, why not
calculate and maintain the best window and its size on the fly? Then you
could get avoid of the pass_window buffer, and refactor a loop away.
Something like,

	int current_size, max_window, max_size;

	current_size = 0;
	max_size = 0;
	for (i = SPACEMIT_RX_TUNE_DELAY_MIN; i < SPACEMIT_RX_TUNE_DELAY_MAX;
	     i++) {
		ret = /* try tuning ... */

		if (ret) {
			if (max_size < current_size) {
				max_window = i - current_size;
				max_size = current_size;
			}

			current_size = 0;
		} else {
			current_size++;
		}
	}

	if (current_size > max_size) {
		max_window = i - current_size;
		max_size = current_size;
		current_size = 0;
	}

should work, too, and is simpler.

Regards,
Yao Zi