Re: [PATCH v4 2/2] mmc: sdhci-cadence: Add CQE support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Hardcoded cqhci_host_ops breaks SoC-specific write wrappers (e.g., Elba SoC byte-lane enables), causing potential hardware state corruption.
--

--- Patch [2]: [PATCH v4 2/2] mmc: sdhci-cadence: Add CQE support ---
commit dbab4b1c448ae45285ec3f2e6b5c3e939ca07cc3
Author: Rohan Joshi <rohan1sj@cadence.com>

mmc: sdhci-cadence: Add CQE support
    
Cadence host controller has optional CQE (Command Queue Engine).
The HW presence of CQE is determined by an additional host capability
register. Look up for "cqhci" register space from the DT and when
present, initialize CQHCI engine.

> diff --git a/drivers/mmc/host/sdhci-cadence.c b/drivers/mmc/host/sdhci-cadence.c
> index 435603c8c00b2..aa4431ef2b394 100644
> --- a/drivers/mmc/host/sdhci-cadence.c
> +++ b/drivers/mmc/host/sdhci-cadence.c
[ ... ]
> @@ -385,6 +392,68 @@ static void sdhci_cdns_set_uhs_signaling(struct sdhci_host *host,
>  		sdhci_set_uhs_signaling(host, timing);
>  }
>  
> +static u32 sdhci_cdns_cqhci_irq(struct sdhci_host *host, u32 intmask)
> +{
> +	int cmd_err = 0;
> +	int data_err = 0;
> +
> +	/* return original intmask to be handled by other handlers if it's not a CQE interrupt */
> +	if (!sdhci_cqe_irq(host, intmask, &cmd_err, &data_err))
> +		return intmask;
> +
> +	cqhci_irq(host->mmc, cmd_err, data_err);
> +
> +	return 0;
> +}
> +
> +static const struct cqhci_host_ops sdhci_cdns_cqhci_ops = {
> +	.enable		= sdhci_cqe_enable,
> +	.disable	= sdhci_cqe_disable,
> +};
> +
> +static int sdhci_cdns_cqe_add_host(struct sdhci_host *host, struct platform_device *pdev)
> +{
> +	struct cqhci_host *cq_host;
> +	bool dma64;
> +	int ret;
[ ... ]
> +	dma64 = host->flags & SDHCI_USE_64_BIT_DMA;
> +	if (dma64)
> +		cq_host->caps |= CQHCI_TASK_DESC_SZ_128;
> +
> +	cq_host->ops = &sdhci_cdns_cqhci_ops;

[Severity: Medium]
By hardcoding cq_host->ops to sdhci_cdns_cqhci_ops here, does this bypass
the SoC-specific write wrappers needed by platforms like the Pensando Elba
SoC?

The sdhci_cdns_cqhci_ops struct lacks a custom .write_l callback. Elba
requires a custom byte-lane enable configuration protected by wrlock
(elba_priv_writel). If a raw writel() from the CQE core interleaves with
an elba_write_b(), the 32-bit CQHCI write could be truncated by the hardware
to 1 byte, potentially corrupting the CQE state.

While Elba's current device tree might not define the cqhci resource, it
appears CQE support is architecturally intended for it since CQE hooks are
added to sdhci_elba_ops further below in this patch. Is there a way to allow
sdhci_elba_drv_data to supply its own cqhci_host_ops, or dynamically inherit
the write_l wrapper from the host operations?

> +
> +	host->mmc->caps2 |= MMC_CAP2_CQE | MMC_CAP2_CQE_DCMD;
> +
> +	ret = cqhci_init(cq_host, host->mmc, dma64);
> +	if (ret) {
> +		dev_err_probe(&pdev->dev, ret, "cqhci init failed\n");
> +		goto cleanup;
> +	}
> +
> +	/* add host to MMC subsystem */
> +	ret = __sdhci_add_host(host);
> +	if (ret)
> +		goto cleanup;
> +
> +	return 0;
> +
> +cleanup:
> +	sdhci_cleanup_host(host);
> +	return ret;
> +}
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825-cdns_sdhci_cqe-support-v4-0-e9f72e338373@cadence.com?part=2