linux-kernel - Re: [PATCH] mmc: sdhci-of-dwcmshc: Promote the th1520 reset handling to ip level

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aRSHZeurjbC266cd@xhacker>
Date: Wed, 12 Nov 2025 21:11:01 +0800
From: Jisheng Zhang <jszhang@...nel.org>
To: Shawn Lin <shawn.lin@...k-chips.com>
Cc: linux-mmc@...r.kernel.org, linux-kernel@...r.kernel.org,
	Adrian Hunter <adrian.hunter@...el.com>,
	Ulf Hansson <ulf.hansson@...aro.org>
Subject: Re: [PATCH] mmc: sdhci-of-dwcmshc: Promote the th1520 reset handling
 to ip level

On Wed, Nov 12, 2025 at 03:01:43PM +0800, Shawn Lin wrote:
> Hi Jisheng,

Hi

> 
> 在 2025/11/12 星期三 8:14, Jisheng Zhang 写道:
> > Commit 27e8fe0da3b7 ("mmc: sdhci-of-dwcmshc: Prevent stale command
> > interrupt handling") clears pending interrupts when resetting
> > host->pending_reset to ensure no pending stale interrupts after
> > sdhci_threaded_irq restores interrupts. But this fix is only added for
> > th1520 platforms, in fact per my test, this issue exists on all
> > dwcmshc users, such as cv1800b, sg2002, and synaptics platforms.
> > 
> > So promote the above reset handling from th1520 to ip level.
> > > Fixes: 017199c2849c ("mmc: sdhci-of-dwcmshc: Add support for Sophgo
> CV1800B and SG2002")
> > Signed-off-by: Jisheng Zhang <jszhang@...nel.org>
> > ---
> >   drivers/mmc/host/sdhci-of-dwcmshc.c | 35 ++++++++++++++++-------------
> >   1 file changed, 20 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> > index eebd45389956..c17168edc9fd 100644
> > --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> > +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> > @@ -289,6 +289,19 @@ static void dwcmshc_adma_write_desc(struct sdhci_host *host, void **desc,
> >   	sdhci_adma_write_desc(host, desc, addr, len, cmd);
> >   }
> > +static void dwcmshc_reset(struct sdhci_host *host, u8 mask)
> > +{
> > +	sdhci_reset(host, mask);
> > +
> > +	/* The dwcmshc does not comply with the SDHCI specification
> > +	 * regarding the "Software Reset for CMD line should clear 'Command
> > +	 * Complete' in the Normal Interrupt Status Register." Clear the bit
> > +	 * here to compensate for this quirk.
> > +	 */
> > +	if (mask & SDHCI_RESET_CMD)
> > +		sdhci_writel(host, SDHCI_INT_RESPONSE, SDHCI_INT_STATUS);
> > +}
> > +
> >   static unsigned int dwcmshc_get_max_clock(struct sdhci_host *host)
> >   {
> >   	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > @@ -686,7 +699,7 @@ static void rk35xx_sdhci_reset(struct sdhci_host *host, u8 mask)
> >   		reset_control_deassert(priv->reset);
> >   	}
> > -	sdhci_reset(host, mask);
> > +	dwcmshc_reset(host, mask);
> 
> Thanks for the patch. However, I'm afraid it's overkill for the IP used
> by Rockchip at least. From the databook v1.70a released by synopsys, it
> clearly says:
> 
> "The following registers and bits are cleared by this bit:
> - Present State register - Command Inhibit (CMD) bit
> - Normal Interrupt Status register - Command Complete bit
> - Error Interrupt Status - Response error statuses related
> to Command Inhibit (CMD) bit "

The fact seems different. I have both 1.70a and 1.80a platforms, and
the issue exists on both plaforms.

> 
> To be more rigorous, I askeed my IC team performed a simulation this
> morning. The results confirmed that the IP's behavior complies with the
> description in the databook.

See above. NOTE: this isn't 100% reproduced. And per my testing, doesn't
exists with EMMC case. It's easier to be reproduced when using the host
for sdio card, e.g sdio wifi.

> 
> Is this a problem with an early version of the dwc IP? If so, I think it

See above, this exists on 1.80a too. And I belive the ip version on th1520,
cv1800b and sg2002 is not earlier than 1.70a. 

>From another side, clearing SDHCI_INT_RESPONSE does no harm.

> should cc all users of this driver to have a check if making it a IP
> level change.
> 
> >   }
> >   static int dwcmshc_rk35xx_init(struct device *dev, struct sdhci_host *host,
> > @@ -832,15 +845,7 @@ static void th1520_sdhci_reset(struct sdhci_host *host, u8 mask)
> >   	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> >   	u16 ctrl_2;
> > -	sdhci_reset(host, mask);
> > -
> > -	/* The T-Head 1520 SoC does not comply with the SDHCI specification
> > -	 * regarding the "Software Reset for CMD line should clear 'Command
> > -	 * Complete' in the Normal Interrupt Status Register." Clear the bit
> > -	 * here to compensate for this quirk.
> > -	 */
> > -	if (mask & SDHCI_RESET_CMD)
> > -		sdhci_writel(host, SDHCI_INT_RESPONSE, SDHCI_INT_STATUS);
> > +	dwcmshc_reset(host, mask);
> >   	if (priv->flags & FLAG_IO_FIXED_1V8) {
> >   		ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> > @@ -886,7 +891,7 @@ static void cv18xx_sdhci_reset(struct sdhci_host *host, u8 mask)
> >   	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> >   	u32 val, emmc_caps = MMC_CAP2_NO_SD | MMC_CAP2_NO_SDIO;
> > -	sdhci_reset(host, mask);
> > +	dwcmshc_reset(host, mask);
> >   	if ((host->mmc->caps2 & emmc_caps) == emmc_caps) {
> >   		val = sdhci_readl(host, priv->vendor_specific_area1 + CV18XX_SDHCI_MSHC_CTRL);
> > @@ -958,7 +963,7 @@ static void cv18xx_sdhci_post_tuning(struct sdhci_host *host)
> >   	val |= SDHCI_INT_DATA_AVAIL;
> >   	sdhci_writel(host, val, SDHCI_INT_STATUS);
> > -	sdhci_reset(host, SDHCI_RESET_CMD | SDHCI_RESET_DATA);
> > +	dwcmshc_reset(host, SDHCI_RESET_CMD | SDHCI_RESET_DATA);
> >   }
> >   static int cv18xx_sdhci_execute_tuning(struct sdhci_host *host, u32 opcode)
> > @@ -1080,7 +1085,7 @@ static inline void sg2042_sdhci_phy_init(struct sdhci_host *host)
> >   static void sg2042_sdhci_reset(struct sdhci_host *host, u8 mask)
> >   {
> > -	sdhci_reset(host, mask);
> > +	dwcmshc_reset(host, mask);
> >   	if (mask & SDHCI_RESET_ALL)
> >   		sg2042_sdhci_phy_init(host);
> > @@ -1100,7 +1105,7 @@ static const struct sdhci_ops sdhci_dwcmshc_ops = {
> >   	.set_bus_width		= sdhci_set_bus_width,
> >   	.set_uhs_signaling	= dwcmshc_set_uhs_signaling,
> >   	.get_max_clock		= dwcmshc_get_max_clock,
> > -	.reset			= sdhci_reset,
> > +	.reset			= dwcmshc_reset,
> >   	.adma_write_desc	= dwcmshc_adma_write_desc,
> >   	.irq			= dwcmshc_cqe_irq_handler,
> >   };
> > @@ -1121,7 +1126,7 @@ static const struct sdhci_ops sdhci_dwcmshc_bf3_ops = {
> >   	.set_bus_width		= sdhci_set_bus_width,
> >   	.set_uhs_signaling	= dwcmshc_set_uhs_signaling,
> >   	.get_max_clock		= dwcmshc_get_max_clock,
> > -	.reset			= sdhci_reset,
> > +	.reset			= dwcmshc_reset,
> >   	.adma_write_desc	= dwcmshc_adma_write_desc,
> >   	.irq			= dwcmshc_cqe_irq_handler,
> >   	.hw_reset		= dwcmshc_bf3_hw_reset,
>