Re: [PATCH 4/4] mmc: sdhci-of-k1: Add TX tuning parameter

From: "Troy Mitchell" <troy.mitchell@linux.spacemit.com>
To: "Yixun Lan" <dlan@kernel.org>, "Ulf Hansson" <ulfh@kernel.org>,
	"Rob Herring" <robh@kernel.org>,
	"Krzysztof Kozlowski" <krzk+dt@kernel.org>,
	"Conor Dooley" <conor+dt@kernel.org>,
	"Adrian Hunter" <adrian.hunter@intel.com>,
	"Troy Mitchell" <troy.mitchell@linux.spacemit.com>
Cc: "Long Wan" <long.wan@linux.spacemit.com>,
	<linux-mmc@vger.kernel.org>, <devicetree@vger.kernel.org>,
	<linux-riscv@lists.infradead.org>, <spacemit@lists.linux.dev>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 4/4] mmc: sdhci-of-k1: Add TX tuning parameter
Date: Thu, 03 Sep 2026 11:27:14 +0800	[thread overview]
Message-ID: <DL5D12BKMIWN.QUCX18JF8GDU@linux.spacemit.com> (raw)
In-Reply-To: <20260902-07-k3-sdhci-dts-v1-4-4e7909cf4233@kernel.org>

On Wed Sep 2, 2026 at 3:03 PM +08, Yixun Lan wrote:
> From: Long Wan <long.wan@linux.spacemit.com>
>
> Instead of setting the TX delay code via hardcoded parameter, configuring
> it via device tree property, which make it able to adapt according to
> board specific tuning requirement.
>
> Signed-off-by: Long Wan <long.wan@linux.spacemit.com>
> Signed-off-by: Yixun Lan <dlan@kernel.org>
> ---
>  drivers/mmc/host/sdhci-of-k1.c | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-of-k1.c b/drivers/mmc/host/sdhci-of-k1.c
> index e6ab40c9a555..6cab06332747 100644
> --- a/drivers/mmc/host/sdhci-of-k1.c
> +++ b/drivers/mmc/host/sdhci-of-k1.c
> @@ -97,6 +97,7 @@ struct spacemit_sdhci_host {
>  	struct pinctrl *pinctrl;
>  	struct pinctrl_state *pinctrl_default;
>  	struct pinctrl_state *pinctrl_uhs;
> +	u8 tx_delaycode;
>  };
>  
>  /* All helper functions will update clr/set while preserve rest bits */
> @@ -263,6 +264,7 @@ static int spacemit_sdhci_execute_tuning(struct sdhci_host *host, u32 opcode)
>  	int max_pass_len = 0, max_pass_start = 0;
>  	struct mmc_host *mmc = host->mmc;
>  	struct mmc_ios ios = mmc->ios;
> +	struct spacemit_sdhci_host *sdhst = sdhci_pltfm_priv(sdhci_priv(host));
This is only a minor style suggestion, not a requirement. Would it be
clearer to make `sdhst` the first local declaration? That would make the
declaration block easier to scan.
>  	u8 final_delay;
>  	int ret = 0;
>  	int i;
> @@ -279,11 +281,11 @@ static int spacemit_sdhci_execute_tuning(struct sdhci_host *host, u32 opcode)
>  
>  	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_set_tx_delay(host, sdhst->tx_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_TX_TUNING_DLINE_REG, sdhst->tx_delaycode);
>  	}
>  
>  	spacemit_sdhci_prepare_tuning(host);
> @@ -496,6 +498,23 @@ static inline void spacemit_sdhci_get_pins(struct device *dev,
>  		sdhst->pinctrl_default, sdhst->pinctrl_uhs);
>  }
>  
> +static void spacemit_sdhci_get_tuning_params(struct device *dev,
> +					     struct sdhci_pltfm_host *pltfm_host)
> +{
> +	struct spacemit_sdhci_host *sdhst = sdhci_pltfm_priv(pltfm_host);
> +	u32 val;
> +
> +	sdhst->tx_delaycode = SPACEMIT_TX_TUNING_DELAYCODE;
> +
> +	/* Override from DT property */
> +	if (!device_property_read_u32(dev, "spacemit,tx-delay", &val)) {
> +		if (val <= U8_MAX)
> +			sdhst->tx_delaycode = val;
> +		else
> +			dev_warn(dev, "ignoring invalid tx delay: %u\n", val);
Should this warning also report the default delay code that will be used
instead?

                                        - Troy