Re: [PATCH 20/25] mmc: sdhci-of-dwcmshc: add MT2824 support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] mmc: sdhci-of-dwcmshc: missed reset in
  mt2824_set_uhs_signaling()
--

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

mmc: sdhci-of-dwcmshc: add MT2824 support

This commit adds support for the Synopsys DWC MSHC controller and PHY on
the Metanoia MT2824 SoC, including card type probing, voltage switching,
and workarounds for HOST_CONTROL2 writes and clock stabilization.

> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> index 09702022e7087..cb248fc572e21 100644
> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
[ ... ]
> @@ -2108,6 +2122,470 @@ static const struct sdhci_ops sdhci_dwcmshc_k230_ops = {
[ ... ]
> +/*
> + * A timing change must write the new bus speed mode to HOST_CONTROL2.
> + * Reset the CMD and DATA lines after the write, while the card clock is
> + * off, so that the next command gets a good response.
> + */
> +static void mt2824_set_uhs_signaling(struct sdhci_host *host,
> +				     unsigned int timing)
> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct dwcmshc_priv *dwc_priv = sdhci_pltfm_priv(pltfm_host);
> +	u32 reg = dwc_priv->vendor_specific_area1 + DWCMSHC_HOST_CTRL3;
> +	u16 ctrl2 = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> +	u8 ctrl;
> +
> +	ctrl = sdhci_readb(host, reg);
> +	ctrl &= ~MT2824_HOST_CTRL3_NEGEDGE_DATAOUT;
> +	if (timing == MMC_TIMING_MMC_DDR52 ||
> +	    timing == MMC_TIMING_UHS_DDR50)
> +		ctrl |= MT2824_HOST_CTRL3_NEGEDGE_DATAOUT;
> +	sdhci_writeb(host, ctrl, reg);
> +
> +	dwcmshc_set_uhs_signaling(host, timing);
> +	if (sdhci_readw(host, SDHCI_HOST_CONTROL2) != ctrl2)
> +		dwcmshc_reset(host, SDHCI_RESET_CMD | SDHCI_RESET_DATA);
> +}

[Severity: Medium]
Can a driver strength change write to the low byte of SDHCI_HOST_CONTROL2
without triggering the CMD and DATA line reset here?

In sdhci_set_ios(), the updated driver type bits (SDHCI_CTRL_DRV_TYPE_MASK,
in the low byte of SDHCI_HOST_CONTROL2) are written before
set_uhs_signaling() is called:

drivers/mmc/host/sdhci.c:sdhci_set_ios() {
    ...
			sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
			host->drv_type = ios->drv_type;
		}

		host->ops->set_uhs_signaling(host, ios->timing);
    ...
}

Because mt2824_set_uhs_signaling() samples ctrl2 after that write has
already happened, changing ios->drv_type via mmc_set_driver_type() while
ios->timing is unchanged leaves SDHCI_HOST_CONTROL2 equal to ctrl2 across
dwcmshc_set_uhs_signaling(). The reset is then skipped and the next
command (such as CMD6 during HS200 or UHS-I initialization) gets a broken
response.

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