[PATCH 0/2] mmc: sdhci-of-dwcmshc: Add command queue support for Rockchip SOCs

[PATCH 0/2] mmc: sdhci-of-dwcmshc: Add command queue support for Rockchip SOCs

[Sebastian Reichel]

Right now at least the recent Rockchip SoCs do not support system
suspend with the mainline kernel. I'm currently looking into
improving support for the RK3576 platform.

On the Sige5 one of the issues is the eMMC controller, which fails
to suspend when trying to disable CQE support. While investigating
I found a missing Rockchip quirk in the Rockchip kernel, which is
needed for CQE. Since the RK3576 DT has been upstreamed with the
'supports-cqe' property (RK3588 does not yet have it), we run into
this problem for that platform.

A simple workaround would be to drop the 'supports-cqe' property,
but DT is supposed to describe hardware and the hardware does
support CQE. Thus let's add proper support instead, which also
allows adding the flag for RK3588. IMHO the patch seems a bit
intrusive for backporting, so it might be sensible to drop
'supports-cqe' there instead. Thus I have not added any stable
tags.

Note, that there are more suspend related problems on the platform,
this is just fixing some parts :)

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
Sebastian Reichel (2):
      mmc: sdhci-of-dwcmshc: Add command queue support for rockchip SOCs
      arm64: dts: rockchip: add eMMC CQE support for rk3588

 arch/arm64/boot/dts/rockchip/rk3588-base.dtsi |  1 +
 drivers/mmc/host/sdhci-of-dwcmshc.c           | 85 ++++++++++++++++++++++++++-
 2 files changed, 83 insertions(+), 3 deletions(-)
---
base-commit: 3a8660878839faadb4f1a6dd72c3179c1df56787
change-id: 20251014-rockchip-emmc-cqe-support-370dbab21623

Best regards,
-- 
Sebastian Reichel <sebastian.reichel@collabora.com>

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

[Sebastian Reichel]

This adds CQE support for the Rockchip RK3588 and RK3576 platform. To
be functional, the eMMC device-tree node must have a 'supports-cqe;'
flag property.

As the RK3576 devicet-tree has been upstreamed with the 'supports-cqe;'
property set by default, the kernel already tried to use CQE, which
results in system hang during suspend. This fixes the issue.

Co-developed-by: Yifeng Zhao <yifeng.zhao@rock-chips.com>
Signed-off-by: Yifeng Zhao <yifeng.zhao@rock-chips.com>
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/mmc/host/sdhci-of-dwcmshc.c | 85 +++++++++++++++++++++++++++++++++++--
 1 file changed, 82 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
index eebd45389956..f533c98d5db1 100644
--- a/drivers/mmc/host/sdhci-of-dwcmshc.c
+++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
@@ -24,6 +24,7 @@
 
 #include "sdhci-pltfm.h"
 #include "cqhci.h"
+#include "sdhci-cqhci.h"
 
 #define SDHCI_DWCMSHC_ARG2_STUFF	GENMASK(31, 16)
 
@@ -82,6 +83,8 @@
 #define DWCMSHC_EMMC_DLL_TXCLK		0x808
 #define DWCMSHC_EMMC_DLL_STRBIN		0x80c
 #define DECMSHC_EMMC_DLL_CMDOUT		0x810
+#define DECMSHC_EMMC_MISC_CON		0x81C
+#define MISC_INTCLK_EN			BIT(1)
 #define DWCMSHC_EMMC_DLL_STATUS0	0x840
 #define DWCMSHC_EMMC_DLL_START		BIT(0)
 #define DWCMSHC_EMMC_DLL_LOCKED		BIT(8)
@@ -234,6 +237,7 @@ struct dwcmshc_priv {
 
 struct dwcmshc_pltfm_data {
 	const struct sdhci_pltfm_data pdata;
+	const struct cqhci_host_ops *cqhci_host_ops;
 	int (*init)(struct device *dev, struct sdhci_host *host, struct dwcmshc_priv *dwc_priv);
 	void (*postinit)(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv);
 };
@@ -561,6 +565,61 @@ static void dwcmshc_cqhci_dumpregs(struct mmc_host *mmc)
 	sdhci_dumpregs(mmc_priv(mmc));
 }
 
+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);
+}
+
+static void rk35xx_sdhci_cqe_disabled(struct mmc_host *mmc, bool recovery)
+{
+	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);
+	spin_lock_irqsave(&host->lock, flags);
+
+	/*
+	 * During CQE command transfers, command complete bit gets latched.
+	 * So s/w should clear command complete interrupt status when CQE is
+	 * either halted or disabled. Otherwise unexpected SDCHI legacy
+	 * interrupt gets triggered when CQE is halted/disabled.
+	 */
+	ctrl = sdhci_readl(host, SDHCI_INT_ENABLE);
+	ctrl |= SDHCI_INT_RESPONSE;
+	sdhci_writel(host,  ctrl, SDHCI_INT_ENABLE);
+	sdhci_writel(host, SDHCI_INT_RESPONSE, SDHCI_INT_STATUS);
+
+	spin_unlock_irqrestore(&host->lock, flags);
+
+	sdhci_cqe_disable(mmc, recovery);
+
+	ctrl = sdhci_readl(host, dwc_priv->vendor_specific_area2 + CQHCI_CFG);
+	ctrl &= ~CQHCI_ENABLE;
+	sdhci_writel(host, ctrl, dwc_priv->vendor_specific_area2 + CQHCI_CFG);
+}
+
 static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
@@ -679,6 +738,10 @@ static void rk35xx_sdhci_reset(struct sdhci_host *host, u8 mask)
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct dwcmshc_priv *dwc_priv = sdhci_pltfm_priv(pltfm_host);
 	struct rk35xx_priv *priv = dwc_priv->priv;
+	u32 extra = sdhci_readl(host, DECMSHC_EMMC_MISC_CON);
+
+	if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL))
+		cqhci_deactivate(host->mmc);
 
 	if (mask & SDHCI_RESET_ALL && priv->reset) {
 		reset_control_assert(priv->reset);
@@ -687,6 +750,9 @@ static void rk35xx_sdhci_reset(struct sdhci_host *host, u8 mask)
 	}
 
 	sdhci_reset(host, mask);
+
+	/* Enable INTERNAL CLOCK */
+	sdhci_writel(host, MISC_INTCLK_EN | extra, DECMSHC_EMMC_MISC_CON);
 }
 
 static int dwcmshc_rk35xx_init(struct device *dev, struct sdhci_host *host,
@@ -1188,6 +1254,13 @@ static const struct dwcmshc_pltfm_data sdhci_dwcmshc_bf3_pdata = {
 };
 #endif
 
+static const struct cqhci_host_ops rk35xx_cqhci_ops = {
+	.enable		= rk35xx_sdhci_cqe_enable,
+	.disable	= rk35xx_sdhci_cqe_disabled,
+	.dumpregs	= dwcmshc_cqhci_dumpregs,
+	.set_tran_desc	= dwcmshc_set_tran_desc,
+};
+
 static const struct dwcmshc_pltfm_data sdhci_dwcmshc_rk35xx_pdata = {
 	.pdata = {
 		.ops = &sdhci_dwcmshc_rk35xx_ops,
@@ -1196,6 +1269,7 @@ static const struct dwcmshc_pltfm_data sdhci_dwcmshc_rk35xx_pdata = {
 		.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
 			   SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN,
 	},
+	.cqhci_host_ops = &rk35xx_cqhci_ops,
 	.init = dwcmshc_rk35xx_init,
 	.postinit = dwcmshc_rk35xx_postinit,
 };
@@ -1245,7 +1319,9 @@ static const struct cqhci_host_ops dwcmshc_cqhci_ops = {
 	.set_tran_desc	= dwcmshc_set_tran_desc,
 };
 
-static void dwcmshc_cqhci_init(struct sdhci_host *host, struct platform_device *pdev)
+static void dwcmshc_cqhci_init(struct sdhci_host *host,
+			       struct platform_device *pdev,
+			       const struct dwcmshc_pltfm_data *pltfm_data)
 {
 	struct cqhci_host *cq_host;
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
@@ -1275,7 +1351,10 @@ static void dwcmshc_cqhci_init(struct sdhci_host *host, struct platform_device *
 	}
 
 	cq_host->mmio = host->ioaddr + priv->vendor_specific_area2;
-	cq_host->ops = &dwcmshc_cqhci_ops;
+	if (pltfm_data->cqhci_host_ops)
+		cq_host->ops = pltfm_data->cqhci_host_ops;
+	else
+		cq_host->ops = &dwcmshc_cqhci_ops;
 
 	/* Enable using of 128-bit task descriptors */
 	dma64 = host->flags & SDHCI_USE_64_BIT_DMA;
@@ -1443,7 +1522,7 @@ static int dwcmshc_probe(struct platform_device *pdev)
 		priv->vendor_specific_area2 =
 			sdhci_readw(host, DWCMSHC_P_VENDOR_AREA2);
 
-		dwcmshc_cqhci_init(host, pdev);
+		dwcmshc_cqhci_init(host, pdev, pltfm_data);
 	}
 
 	if (pltfm_data->postinit)

-- 
2.51.0

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

[Dragan Simic]

Hello Sebastian,

> This adds CQE support for the Rockchip RK3588 and RK3576 platform. To
> be functional, the eMMC device-tree node must have a 'supports-cqe;'
> flag property.

> As the RK3576 devicet-tree has been upstreamed with the 'supports-cqe;'
> property set by default, the kernel already tried to use CQE, which
> results in system hang during suspend. This fixes the issue.

Thanks for the patch, it's great to see this implemented!  Please,
see a small nitpick below.

> +static void rk35xx_sdhci_cqe_enable(struct mmc_host *mmc)
> +{
[snip]
> +}
> +
> +static void rk35xx_sdhci_cqe_disabled(struct mmc_host *mmc, bool recovery)
> +{
[snip]
> +}

The function rk35xx_sdhci_cqe_disabled() should obviously be called
rk35xx_sdhci_cqe_disable() instead, i.e. it should use the standard
imperative mood...

> +static const struct cqhci_host_ops rk35xx_cqhci_ops = {
> +	.enable		= rk35xx_sdhci_cqe_enable,
> +	.disable	= rk35xx_sdhci_cqe_disabled,
> +	.dumpregs	= dwcmshc_cqhci_dumpregs,
> +	.set_tran_desc	= dwcmshc_set_tran_desc,
> +};

... which corresponds with the struct member names.

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

[Adrian Hunter]

On 14/10/2025 18:41, Sebastian Reichel wrote:
> This adds CQE support for the Rockchip RK3588 and RK3576 platform. To
> be functional, the eMMC device-tree node must have a 'supports-cqe;'
> flag property.
> 
> As the RK3576 devicet-tree has been upstreamed with the 'supports-cqe;'

devicet-tree ?

> property set by default, the kernel already tried to use CQE, which
> results in system hang during suspend. This fixes the issue.
> 
> Co-developed-by: Yifeng Zhao <yifeng.zhao@rock-chips.com>
> Signed-off-by: Yifeng Zhao <yifeng.zhao@rock-chips.com>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  drivers/mmc/host/sdhci-of-dwcmshc.c | 85 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 82 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> index eebd45389956..f533c98d5db1 100644
> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> @@ -24,6 +24,7 @@
>  
>  #include "sdhci-pltfm.h"
>  #include "cqhci.h"
> +#include "sdhci-cqhci.h"
>  
>  #define SDHCI_DWCMSHC_ARG2_STUFF	GENMASK(31, 16)
>  
> @@ -82,6 +83,8 @@
>  #define DWCMSHC_EMMC_DLL_TXCLK		0x808
>  #define DWCMSHC_EMMC_DLL_STRBIN		0x80c
>  #define DECMSHC_EMMC_DLL_CMDOUT		0x810
> +#define DECMSHC_EMMC_MISC_CON		0x81C
> +#define MISC_INTCLK_EN			BIT(1)
>  #define DWCMSHC_EMMC_DLL_STATUS0	0x840
>  #define DWCMSHC_EMMC_DLL_START		BIT(0)
>  #define DWCMSHC_EMMC_DLL_LOCKED		BIT(8)
> @@ -234,6 +237,7 @@ struct dwcmshc_priv {
>  
>  struct dwcmshc_pltfm_data {
>  	const struct sdhci_pltfm_data pdata;
> +	const struct cqhci_host_ops *cqhci_host_ops;
>  	int (*init)(struct device *dev, struct sdhci_host *host, struct dwcmshc_priv *dwc_priv);
>  	void (*postinit)(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv);
>  };
> @@ -561,6 +565,61 @@ static void dwcmshc_cqhci_dumpregs(struct mmc_host *mmc)
>  	sdhci_dumpregs(mmc_priv(mmc));
>  }
>  
> +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

> +}
> +
> +static void rk35xx_sdhci_cqe_disabled(struct mmc_host *mmc, bool recovery)

As mentioned elsewhere "disabled" -> "disable"

> +{
> +	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.

> +	spin_lock_irqsave(&host->lock, flags);
> +
> +	/*
> +	 * During CQE command transfers, command complete bit gets latched.
> +	 * So s/w should clear command complete interrupt status when CQE is
> +	 * either halted or disabled. Otherwise unexpected SDCHI legacy
> +	 * interrupt gets triggered when CQE is halted/disabled.
> +	 */
> +	ctrl = sdhci_readl(host, SDHCI_INT_ENABLE);
> +	ctrl |= SDHCI_INT_RESPONSE;
> +	sdhci_writel(host,  ctrl, SDHCI_INT_ENABLE);
> +	sdhci_writel(host, SDHCI_INT_RESPONSE, SDHCI_INT_STATUS);
> +
> +	spin_unlock_irqrestore(&host->lock, flags);
> +
> +	sdhci_cqe_disable(mmc, recovery);
> +
> +	ctrl = sdhci_readl(host, dwc_priv->vendor_specific_area2 + CQHCI_CFG);
> +	ctrl &= ~CQHCI_ENABLE;
> +	sdhci_writel(host, ctrl, dwc_priv->vendor_specific_area2 + CQHCI_CFG);
> +}
> +
>  static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock)
>  {
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> @@ -679,6 +738,10 @@ static void rk35xx_sdhci_reset(struct sdhci_host *host, u8 mask)
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>  	struct dwcmshc_priv *dwc_priv = sdhci_pltfm_priv(pltfm_host);
>  	struct rk35xx_priv *priv = dwc_priv->priv;
> +	u32 extra = sdhci_readl(host, DECMSHC_EMMC_MISC_CON);
> +
> +	if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL))
> +		cqhci_deactivate(host->mmc);
>  
>  	if (mask & SDHCI_RESET_ALL && priv->reset) {
>  		reset_control_assert(priv->reset);
> @@ -687,6 +750,9 @@ static void rk35xx_sdhci_reset(struct sdhci_host *host, u8 mask)
>  	}
>  
>  	sdhci_reset(host, mask);
> +
> +	/* Enable INTERNAL CLOCK */
> +	sdhci_writel(host, MISC_INTCLK_EN | extra, DECMSHC_EMMC_MISC_CON);
>  }
>  
>  static int dwcmshc_rk35xx_init(struct device *dev, struct sdhci_host *host,
> @@ -1188,6 +1254,13 @@ static const struct dwcmshc_pltfm_data sdhci_dwcmshc_bf3_pdata = {
>  };
>  #endif
>  
> +static const struct cqhci_host_ops rk35xx_cqhci_ops = {
> +	.enable		= rk35xx_sdhci_cqe_enable,
> +	.disable	= rk35xx_sdhci_cqe_disabled,
> +	.dumpregs	= dwcmshc_cqhci_dumpregs,
> +	.set_tran_desc	= dwcmshc_set_tran_desc,
> +};
> +
>  static const struct dwcmshc_pltfm_data sdhci_dwcmshc_rk35xx_pdata = {
>  	.pdata = {
>  		.ops = &sdhci_dwcmshc_rk35xx_ops,
> @@ -1196,6 +1269,7 @@ static const struct dwcmshc_pltfm_data sdhci_dwcmshc_rk35xx_pdata = {
>  		.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
>  			   SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN,
>  	},
> +	.cqhci_host_ops = &rk35xx_cqhci_ops,
>  	.init = dwcmshc_rk35xx_init,
>  	.postinit = dwcmshc_rk35xx_postinit,
>  };
> @@ -1245,7 +1319,9 @@ static const struct cqhci_host_ops dwcmshc_cqhci_ops = {
>  	.set_tran_desc	= dwcmshc_set_tran_desc,
>  };
>  
> -static void dwcmshc_cqhci_init(struct sdhci_host *host, struct platform_device *pdev)
> +static void dwcmshc_cqhci_init(struct sdhci_host *host,
> +			       struct platform_device *pdev,
> +			       const struct dwcmshc_pltfm_data *pltfm_data)
>  {
>  	struct cqhci_host *cq_host;
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> @@ -1275,7 +1351,10 @@ static void dwcmshc_cqhci_init(struct sdhci_host *host, struct platform_device *
>  	}
>  
>  	cq_host->mmio = host->ioaddr + priv->vendor_specific_area2;
> -	cq_host->ops = &dwcmshc_cqhci_ops;
> +	if (pltfm_data->cqhci_host_ops)
> +		cq_host->ops = pltfm_data->cqhci_host_ops;
> +	else
> +		cq_host->ops = &dwcmshc_cqhci_ops;
>  
>  	/* Enable using of 128-bit task descriptors */
>  	dma64 = host->flags & SDHCI_USE_64_BIT_DMA;
> @@ -1443,7 +1522,7 @@ static int dwcmshc_probe(struct platform_device *pdev)
>  		priv->vendor_specific_area2 =
>  			sdhci_readw(host, DWCMSHC_P_VENDOR_AREA2);
>  
> -		dwcmshc_cqhci_init(host, pdev);
> +		dwcmshc_cqhci_init(host, pdev, pltfm_data);
>  	}
>  
>  	if (pltfm_data->postinit)
> 

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

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1975 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Adrian Hunter]

On 16/10/2025 20:09, Sebastian Reichel wrote:
> 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).

Leave out cqe_wait_for_idle() but make use of ->pre_enable() and
->post_disable(), refer for example msdc_cqe_pre_enable() /
msdc_cqe_post_disable() or sdhci_tegra_cqe_pre_enable() /
sdhci_tegra_cqe_post_disable()

[PATCH 2/2] arm64: dts: rockchip: add eMMC CQE support for rk3588

[Sebastian Reichel]

The RK3588 eMMC controller supports CQE, so add the missing
DT flag.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 arch/arm64/boot/dts/rockchip/rk3588-base.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
index e2500e31c434..2a7921793020 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
@@ -2181,6 +2181,7 @@ sdhci: mmc@fe2e0000 {
 			 <&cru SRST_A_EMMC>, <&cru SRST_B_EMMC>,
 			 <&cru SRST_T_EMMC>;
 		reset-names = "core", "bus", "axi", "block", "timer";
+		supports-cqe;
 		status = "disabled";
 	};
 

-- 
2.51.0