linux-kernel - Re: [PATCH 1/2] mmc: sdhci-of-dwcmshc: Add command queue support for rockchip SOCs

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] [day] [month] [year] [list]

Message-ID: <7bxs6nw5fnjq22p7gxrmjqjtw3g5nt6cacwpfjihxv5jk765si@ho3odkyxpi7m>
Date: Thu, 16 Oct 2025 19:09:37 +0200
From: Sebastian Reichel <sebastian.reichel@...labora.com>
To: Adrian Hunter <adrian.hunter@...el.com>
Cc: Ulf Hansson <ulf.hansson@...aro.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Heiko Stuebner <heiko@...ech.de>, linux-mmc@...r.kernel.org, linux-kernel@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-rockchip@...ts.infradead.org, kernel@...labora.com, 
	Yifeng Zhao <yifeng.zhao@...k-chips.com>
Subject: Re: [PATCH 1/2] mmc: sdhci-of-dwcmshc: Add command queue support for
 rockchip SOCs

Hi,

I will fix the typo in the commit message in PATCHv2.

On Thu, Oct 16, 2025 at 10:42:29AM +0300, Adrian Hunter wrote:
> > +static void rk35xx_sdhci_cqe_enable(struct mmc_host *mmc)
> > +{
> > +	struct sdhci_host *host = mmc_priv(mmc);
> > +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > +	struct dwcmshc_priv *dwc_priv = sdhci_pltfm_priv(pltfm_host);
> > +	u32 reg;
> > +
> > +	reg = sdhci_readl(host, dwc_priv->vendor_specific_area2 + CQHCI_CFG);
> > +	reg |= CQHCI_ENABLE;
> > +	sdhci_writel(host, reg, dwc_priv->vendor_specific_area2 + CQHCI_CFG);
> > +
> > +	reg = sdhci_readl(host, SDHCI_PRESENT_STATE);
> > +	while (reg & SDHCI_DATA_AVAILABLE) {
> > +		sdhci_readl(host, SDHCI_BUFFER);
> > +		reg = sdhci_readl(host, SDHCI_PRESENT_STATE);
> > +	}
> > +
> > +	sdhci_writew(host, DWCMSHC_SDHCI_CQE_TRNS_MODE, SDHCI_TRANSFER_MODE);
> > +
> > +	sdhci_cqe_enable(mmc);
> > +
> > +	sdhci_writew(host, DWCMSHC_SDHCI_CQE_TRNS_MODE, SDHCI_TRANSFER_MODE);
> 
> Transfer mode was set already 2 lines up

Indeed. I was not sure if this is an intentional quirk from Yifeng
Zhao and thus kept this dual write.

> > +}
> > +
> > +static void rk35xx_sdhci_cqe_disabled(struct mmc_host *mmc, bool recovery)
> 
> As mentioned elsewhere "disabled" -> "disable"

Ack, will fix in PATCHv2.

> > +{
> > +	struct sdhci_host *host = mmc_priv(mmc);
> > +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > +	struct dwcmshc_priv *dwc_priv = sdhci_pltfm_priv(pltfm_host);
> > +	unsigned long flags;
> > +	u32 ctrl;
> > +
> > +	mmc->cqe_ops->cqe_wait_for_idle(mmc);
> 
> Is this necessary?  If so, it seems more like something that should be done by
> cqhci itself.

The RK3588 TRM says that the software needs to verify that the eMMC
controller is in idle state without any ongoing commands or data
transfers before disabling the CQ_EN bit (CQHCI_ENABLE in the kernel).

Thanks for the review,

-- Sebastian

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)