Re: [PATCH 18/30] mci: imx-esdhc: add support for delay/tuning properties in DT

[Sascha Hauer <s.hauer@pengutronix.de>]

On Mon, May 05, 2025 at 02:06:21PM +0200, Ahmad Fatoum wrote:
> With higher data rates, we will need to fine tune delay settings, which
> can be supplied via the device tree. Add support for the binding to
> barebox.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  drivers/mci/imx-esdhc.c | 70 ++++++++++++++++++++++++++++++++++++++++-
>  drivers/mci/imx-esdhc.h | 14 +++++++++
>  2 files changed, 83 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mci/imx-esdhc.c b/drivers/mci/imx-esdhc.c
> index 86dc6daba845..9d82c886cdcf 100644
> --- a/drivers/mci/imx-esdhc.c
> +++ b/drivers/mci/imx-esdhc.c
> @@ -145,6 +145,15 @@ static void usdhc_set_timing(struct fsl_esdhc_host *host, enum mci_timing timing
>  	case MMC_TIMING_MMC_DDR52:
>  		mixctrl |= MIX_CTRL_DDREN;
>  		sdhci_write32(&host->sdhci, IMX_SDHCI_MIXCTRL, mixctrl);
> +		if (host->boarddata.delay_line) {
> +			u32 v;
> +			v = host->boarddata.delay_line <<
> +				IMX_SDHCI_DLL_CTRL_OVERRIDE_VAL_SHIFT |
> +				(1 << IMX_SDHCI_DLL_CTRL_OVERRIDE_EN_SHIFT);
> +			if (cpu_is_mx53())
> +				v <<= 1;
> +			sdhci_write32(&host->sdhci, IMX_SDHCI_DLL_CTRL, v);
> +		}
>  		break;
>  	default:
>  		sdhci_write32(&host->sdhci, IMX_SDHCI_MIXCTRL, mixctrl);
> @@ -290,7 +299,49 @@ static int esdhc_init(struct mci_host *mci, struct device *dev)
>  	esdhc_clrsetbits32(host, SDHCI_CLOCK_CONTROL__TIMEOUT_CONTROL__SOFTWARE_RESET,
>  			SYSCTL_TIMEOUT_MASK, 14 << 16);
>  
> -	return ret;
> +	if (IS_ENABLED(CONFIG_MCI_TUNING) && esdhc_is_usdhc(host) &&
> +	    (host->socdata->flags & ESDHC_FLAG_STD_TUNING)) {
> +		u32 tmp;
> +
> +		/* disable DLL_CTRL delay line settings */
> +		sdhci_write32(&host->sdhci, ESDHC_DLL_CTRL, 0x0);
> +
> +		tmp = sdhci_read32(&host->sdhci, ESDHC_TUNING_CTRL);
> +		tmp |= ESDHC_STD_TUNING_EN;
> +
> +		/*
> +		 * ROM code or bootloader may config the start tap
> +		 * and step, unmask them first.
> +		 */

This comment is rather pointless. It's just good style to mask out
the current values before setting them.

> +		tmp &= ~(ESDHC_TUNING_START_TAP_MASK | ESDHC_TUNING_STEP_MASK);
> +		if (host->boarddata.tuning_start_tap)
> +			tmp |= host->boarddata.tuning_start_tap;
> +		else
> +			tmp |= ESDHC_TUNING_START_TAP_DEFAULT;

You could initialize boarddata.tuning_start_tap with ESDHC_TUNING_START_TAP_DEFAULT
in fsl_esdhc_probe_dt().

> +
> +		if (host->boarddata.tuning_step) {

Same here.

Sascha

> +			tmp |= host->boarddata.tuning_step
> +				<< ESDHC_TUNING_STEP_SHIFT;
> +		} else {
> +			tmp |= ESDHC_TUNING_STEP_DEFAULT
> +				<< ESDHC_TUNING_STEP_SHIFT;
> +		}
> +
> +		/* Disable the CMD CRC check for tuning, if not, need to
> +		 * add some delay after every tuning command, because
> +		 * hardware standard tuning logic will directly go to next
> +		 * step once it detect the CMD CRC error, will not wait for
> +		 * the card side to finally send out the tuning data, trigger
> +		 * the buffer read ready interrupt immediately. If usdhc send
> +		 * the next tuning command some eMMC card will stuck, can't
> +		 * response, block the tuning procedure or the first command
> +		 * after the whole tuning procedure always can't get any response.
> +		 */
> +		tmp |= ESDHC_TUNING_CMD_CRC_CHECK_DISABLE;
> +		sdhci_write32(&host->sdhci, ESDHC_TUNING_CTRL, tmp);
> +	}
> +
> +	return 0;
>  }
>  
>  static const struct mci_ops fsl_esdhc_ops = {
> @@ -300,6 +351,21 @@ static const struct mci_ops fsl_esdhc_ops = {
>  	.card_present = esdhc_card_present,
>  };
>  
> +static void fsl_esdhc_probe_dt(struct device *dev, struct fsl_esdhc_host *host)
> +{
> +	struct device_node *np = dev->of_node;
> +	struct esdhc_platform_data *boarddata = &host->boarddata;
> +
> +	if (!IS_ENABLED(CONFIG_MCI_TUNING))
> +		return;
> +
> +	of_property_read_u32(np, "fsl,tuning-step", &boarddata->tuning_step);
> +	of_property_read_u32(np, "fsl,tuning-start-tap",
> +			     &boarddata->tuning_start_tap);
> +	if (of_property_read_u32(np, "fsl,delay-line", &boarddata->delay_line))
> +		boarddata->delay_line = 0;
> +}
> +

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |