Re: [PATCH v2] mmc: sdhci-msm: Enable ICE support for non-cmdq eMMC devices

[Eric Biggers <ebiggers@kernel.org>]

On Tue, Oct 14, 2025 at 03:05:03PM +0530, Md Sadre Alam wrote:
> Enable Inline Crypto Engine (ICE) support for eMMC devices that operate
> without Command Queue Engine (CQE).This allows hardware-accelerated
> encryption and decryption for standard (non-CMDQ) requests.
> 
> This patch:
> - Adds ICE register definitions for non-CMDQ crypto configuration
> - Implements a per-request crypto setup via sdhci_msm_ice_cfg()
> - Hooks into the request path via mmc_host_ops.request
> - Initializes ICE hardware during CQE setup for compatible platforms
> 
> With this, non-CMDQ eMMC devices can benefit from inline encryption,
> improving performance for encrypted I/O while maintaining compatibility
> with existing CQE crypto support.
> 
> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>

How was this tested?

>  #ifdef CONFIG_MMC_CRYPTO
>  
> +static int sdhci_msm_ice_cfg(struct sdhci_host *host, struct mmc_request *mrq,
> +			     u32 slot)
> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +	struct mmc_host *mmc = msm_host->mmc;
> +	struct cqhci_host *cq_host = mmc->cqe_private;
> +	unsigned int crypto_params = 0;
> +	int key_index = 0;
> +	bool bypass = true;
> +	u64 dun = 0;
> +
> +	if (mrq->crypto_ctx) {
> +		dun = mrq->crypto_ctx->bc_dun[0];
> +		bypass = false;
> +		key_index = mrq->crypto_key_slot;
> +	}
> +
> +	crypto_params = FIELD_PREP(ICE_HCI_PARAM_CE, !bypass) |
> +			FIELD_PREP(ICE_HCI_PARAM_CCI, key_index);
> +
> +	cqhci_writel(cq_host, crypto_params, NONCQ_CRYPTO_PARM);
> +
> +	if (mrq->crypto_ctx)
> +		cqhci_writel(cq_host, lower_32_bits(dun), NONCQ_CRYPTO_DUN);
> +
> +	/* Ensure crypto configuration is written before proceeding */
> +	wmb();
> +
> +	return 0;
> +}

This would probably be easier to read with separate code paths for
crypto_ctx != NULL and crypto_ctx == NULL.  Also 'bypass' should be
inverted and renamed to 'crypto_enable' to match the bitfield.  Or just
prepare the bitfield directly, without an intermediate variable.

> @@ -2131,6 +2185,8 @@ static int sdhci_msm_cqe_add_host(struct sdhci_host *host,
>  	struct cqhci_host *cq_host;
>  	bool dma64;
>  	u32 cqcfg;
> +	u32 config;
> +	u32 ice_cap;
>  	int ret;
>  
>  	/*
> @@ -2185,6 +2241,18 @@ static int sdhci_msm_cqe_add_host(struct sdhci_host *host,
>  	if (ret)
>  		goto cleanup;
>  
> +	/* Initialize ICE for non-CMDQ eMMC devices */
> +	config = sdhci_readl(host, HC_VENDOR_SPECIFIC_FUNC4);
> +	config &= ~DISABLE_CRYPTO;
> +	sdhci_writel(host, config, HC_VENDOR_SPECIFIC_FUNC4);
> +	ice_cap = cqhci_readl(cq_host, CQHCI_CAP);
> +	if (ice_cap & ICE_HCI_SUPPORT) {
> +		config = cqhci_readl(cq_host, CQHCI_CFG);
> +		config |= CRYPTO_GENERAL_ENABLE;
> +		cqhci_writel(cq_host, config, CQHCI_CFG);
> +	}
> +	sdhci_msm_ice_enable(msm_host);

This is after __sdhci_add_host() was called, which is probably too late.

> +#ifdef CONFIG_MMC_CRYPTO
> +	host->mmc_host_ops.request = sdhci_msm_request;
> +#endif
>  	/* Set the timeout value to max possible */
>  	host->max_timeout_count = 0xF;

A lot of the code in this patch also seems to actually run on
CQE-capable hosts.  Can you explain?  Why is it needed?  Is there any
change in behavior on them?

- Eric