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

[Yixun Lan]

Hi Troy,

On 11:27 Thu 03 Sep     , Troy Mitchell wrote:
> On Wed Sep 2, 2026 at 3:03 PM +08, Yixun Lan wrote:
> > From: Long Wan <long.wan@xxxxxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Yixun Lan <dlan@xxxxxxxxxx>
> > ---
> >  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.

somehow I hate to bring extra unnecessary changes..
but if we want to follow reverse christmas tree style and put struct declaration first,
then we should also move "int max_pass_len = 0, max_pass_start = 0;" down below

I can adjust this if maintainer/others don't mind..

> >  	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?
> 
yes, can improve a bit

-- 
Yixun Lan (dlan)