Re: [PATCH 21/25] mmc: sdhci-of-dwcmshc: add MT2824 tuning and HS400 support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] mmc: sdhci-of-dwcmshc: improper HS400 PHY DLL handling in
  mt2824_set_clock()
- [Medium] mmc: sdhci-of-dwcmshc: incomplete error rollback in
  mt2824_execute_tuning()
- [Low] mmc: sdhci-of-dwcmshc: duplicate PHY DLL register macros for
  MT2824
--

commit 6e69732340199284e68817329ed4f3bf3f474149
Author: Benoît Monin <benoit.monin@bootlin.com>

mmc: sdhci-of-dwcmshc: add MT2824 tuning and HS400 support

This commit adds software tuning over the 128-tap sampling delay line, a
filter for stray data interrupts during tuning, and manual PHY DLL locking
for HS400 mode on the Metanoia MT2824 controller.

> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> index cb248fc572e21..324ae93295af0 100644
> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
[ ... ]
> @@ -98,6 +103,10 @@
>  #define MT2824_PHY_TXSLEW_P_STROBE		0x3
>  #define MT2824_PHY_SMPLDL_CNFG			0x0c
>  #define MT2824_PHY_ATDL_CNFG			0x0c
> +#define MT2824_PHY_DLL_CNFG1			0x20
> +#define MT2824_PHY_DLLDL_CNFG			0x60
> +#define MT2824_PHY_DLL_LOCKED			BIT(0)
> +#define MT2824_PHY_DLL_LOCK_ERR			BIT(1)

[Severity: Low]
This isn't a bug, but these macros duplicate the Synopsys DWC MSHC PHY DLL
definitions already present in this file (PHY_DLL_CNFG1_SLVDLY_MASK /
PHY_DLL_CNFG1_SLVDLY, PHY_DLLDL_CNFG_SLV_INPSEL_MASK /
PHY_DLLDL_CNFG_SLV_INPSEL, DLL_LOCK_STS, and DLL_ERROR_STS), while
mt2824_set_dll() already uses the shared PHY_DLL_*_R register offsets.

[ ... ]

> @@ -2271,6 +2281,38 @@ static int mt2824_phy_init(struct sdhci_host *host, bool v1v8)
>  	return 0;
>  }
>  
> +/* HS400 needs the PHY DLL locked on the card clock; lock it by hand */
> +static void mt2824_set_dll(struct sdhci_host *host)
> +{
> +	u16 clk;
> +	u8 status;
> +	int ret;
> +
> +	clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> +	clk &= ~SDHCI_CLOCK_CARD_EN;
> +	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
> +
> +	sdhci_writeb(host, MT2824_PHY_DLL_CNFG1, PHY_DLL_CNFG1_R);
> +	sdhci_writeb(host, MT2824_PHY_DLLDL_CNFG, PHY_DLLDL_CNFG_R);
> +
> +	sdhci_enable_clk(host, clk);
> +	usleep_range(10, 20);
> +
> +	sdhci_writeb(host, PHY_DLL_CTRL_ENABLE, PHY_DLL_CTRL_R);
> +	ret = read_poll_timeout(sdhci_readb, status,
> +				status & MT2824_PHY_DLL_LOCKED, 10, 150000,
> +				false, host, PHY_DLL_STATUS_R);
> +	if (ret) {
> +		dev_err(mmc_dev(host->mmc), "PHY DLL did not lock: 0x%02x\n",
> +			status);
> +		return;
> +	}
> +
> +	if (status & MT2824_PHY_DLL_LOCK_ERR)
> +		dev_err(mmc_dev(host->mmc), "PHY DLL locked with error: 0x%02x\n",
> +			status);
> +}
[ ... ]
> @@ -2332,6 +2374,11 @@ static void mt2824_set_clock(struct sdhci_host *host, unsigned int clock)
>  	if (clock)
>  		mt2824_enable_clk(host, sdhci_calc_clk(host, clock,
>  						       &mmc->actual_clock));
> +
> +	if (clock && host->timing == MMC_TIMING_MMC_HS400) {
> +		mt2824_set_dll(host);
> +		dwcmshc_reset(host, SDHCI_RESET_CMD | SDHCI_RESET_DATA);
> +	}
>  }

[Severity: High]
Does mt2824_set_clock() try to lock the HS400 PHY DLL at 52 MHz and then
fail to re-lock it when the clock is raised to 200 MHz?

During HS400 selection in mmc_select_hs400() (and mmc_select_hs400es()),
the MMC core calls mmc_set_timing(host, MMC_TIMING_MMC_HS400) while
host->clock is still 52 MHz, before calling mmc_set_bus_speed(card) to
switch to 200 MHz.  Because mt2824_set_clock() only checks for a non-zero
clock in MMC_TIMING_MMC_HS400, the first call enables PHY_DLL_CTRL_R at
52 MHz.  Neither mt2824_set_clock() nor mt2824_set_dll() ever writes
PHY_DLL_CTRL_DISABLE to PHY_DLL_CTRL_R, so when mmc_set_bus_speed() later
calls mt2824_set_clock() at 200 MHz, the clock is stopped and changed
while the DLL stays enabled, and writing PHY_DLL_CTRL_ENABLE again never
creates a 0-to-1 enable transition to lock onto the 200 MHz clock.
Leaving HS400 (such as via mmc_hs400_to_hs200()) also leaves the DLL
enabled at lower speeds.

Should PHY_DLL_CTRL_R be disabled before changing the clock, and only
configured and enabled when host->timing is MMC_TIMING_MMC_HS400 and the
clock is above 52 MHz?

[ ... ]

> @@ -2460,6 +2507,156 @@ static void mt2824_hw_reset(struct sdhci_host *host)
[ ... ]
> +	/*
> +	 * AT_CTRL_R must not be written while SAMPLE_CLK_SEL is set: enable
> +	 * software tuning first, then select the tuned sampling clock.
> +	 */
> +	at_ctrl = sdhci_readl(host, at_ctrl_reg);
> +	sdhci_writel(host, at_ctrl | AT_CTRL_SW_TUNE_EN, at_ctrl_reg);
> +	ctrl2 = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> +	sdhci_writew(host, ctrl2 | SDHCI_CTRL_TUNED_CLK, SDHCI_HOST_CONTROL2);
> +	/*
> +	 * That HOST_CONTROL2 write breaks the response of the next command:
> +	 * send one tuning command for nothing before the scan.
> +	 */
> +	mmc_send_tuning(host->mmc, opcode, NULL);
> +	host->ops->reset(host, SDHCI_RESET_CMD | SDHCI_RESET_DATA);
[ ... ]
> +fail:
> +	/* clear SAMPLE_CLK_SEL before AT_CTRL_R is written again */
> +	sdhci_reset_tuning(host);
> +	sdhci_writel(host, at_ctrl, at_ctrl_reg);
> +	host->ops->reset(host, SDHCI_RESET_CMD | SDHCI_RESET_DATA);
> +	sdhci_end_tuning(host);
> +	WRITE_ONCE(dwc_priv->flags, dwc_priv->flags & ~FLAG_SW_TUNING);
> +	return ret;
> +}

[Severity: Medium]
If a re-tuning pass fails after an earlier tuning pass succeeded, does the
fail path leave AT_CTRL_SW_TUNE_EN enabled with the delay line stuck at the
last scanned phase?

A successful mt2824_execute_tuning() call leaves AT_CTRL_SW_TUNE_EN set in
at_ctrl_reg, so on a subsequent tuning run at_ctrl is read with
AT_CTRL_SW_TUNE_EN already set.  If that scan fails, writing at_ctrl back
verbatim does not clear AT_CTRL_SW_TUNE_EN, leaving software tuning active
with DWCMSHC_AT_STAT set to phase 127.  In addition, sdhci_reset_tuning()
modifies the low byte of SDHCI_HOST_CONTROL2 while the card clock is
enabled without a dummy command or stopping the card clock around the
reset, which on this controller breaks the response of the next command.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-mt2824-v1-0-86087eaf5af4@bootlin.com?part=21