Re: [PATCH v5] mmc: sdhci-msm: Enable ICE for CQE-capable controllers with non-CQE cards

[Eric Biggers <ebiggers@kernel.org>]

On Wed, Nov 19, 2025 at 05:16:53PM +0530, Md Sadre Alam wrote:
>  struct sdhci_msm_offset {
>  	u32 core_hc_mode;
>  	u32 core_mci_data_cnt;
> @@ -300,6 +312,7 @@ struct sdhci_msm_host {
>  	u32 dll_config;
>  	u32 ddr_config;
>  	bool vqmmc_enabled;
> +	bool ice_init_done;

Rename to non_cqe_ice_init_done

> +static void sdhci_msm_non_cqe_ice_init(struct sdhci_host *host)
> +{
> +	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;
> +	u32 config;
> +	u32 ice_cap;
> +
> +	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);
> +	}
> +}

Why would ICE_HCI_SUPPORT not be set here?  When this is called, the
driver is already in the middle of processing an mmc_request with an
encryption context, due to the driver advertising that it supports
inline crypto earlier.  If the hardware doesn't actually support inline
crypto, that has to be detected earlier.  But I thought it already does.
So it's unclear what checking ICE_HCI_SUPPORT here is meant to achieve.

> +static int sdhci_msm_ice_cfg(struct sdhci_host *host, struct mmc_request *mrq)

This should return void.

> +	if (mrq->crypto_ctx) {
> +		if (!msm_host->ice_init_done) {
> +			sdhci_msm_non_cqe_ice_init(host);
> +			msm_host->ice_init_done = true;
> +		}
> +
> +		crypto_enable = true;
> +		dun = mrq->crypto_ctx->bc_dun[0];
> +		key_index = mrq->crypto_key_slot;
> +		crypto_params = FIELD_PREP(ICE_HCI_PARAM_CE, crypto_enable) |
> +				FIELD_PREP(ICE_HCI_PARAM_CCI, key_index);
> +
> +		cqhci_writel(cq_host, crypto_params, NONCQ_CRYPTO_PARM);
> +		cqhci_writel(cq_host, lower_32_bits(dun), NONCQ_CRYPTO_DUN);

No need for the crypto_enable variable.  Just use:

	FIELD_PREP(ICE_HCI_PARAM_CE, 1).

Also no need for the dun variable.  Just use: 

	cqhci_writel(cq_host, lower_32_bits(mrq->crypto_ctx->bc_dun[0]),
		     NONCQ_CRYPTO_DUN);

> + * For CQE requests, crypto is handled in cqhci_request() in
> + * drivers/mmc/host/cqhci-core.c using the existing CQE crypto infrastructure.

It's recommended to not reference file paths like this in kernel code,
since files are sometimes moved around.

- Eric