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

[Md Sadre Alam]

Hi,

On 11/12/2025 2:22 AM, Eric Biggers wrote:
> On Tue, Nov 11, 2025 at 04:16:04PM +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
> >
> > 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.
>
> This really should explain that this patch actually applies only to host
> controllers that *do* support CQE.  Just they are using a card that
> doesn't support CQE or CQE was explicitly disabled.  Right?
Yes, you are absolutely correct. Thank you for pointing this out - the 
commit message should be clearer about this important detail.

This patch applies specifically to CQE-capable host controllers 
(sdhci-msm controllers that support CQHCI) when they are operating in 
non-CQE mode.
This can happen in two scenarios:

1. CQE-capable controller + non-CQE card: The host controller supports 
CQE, but the eMMC card doesn't support Command Queue Engine
2. CQE explicitly disabled: The host controller and card both support 
CQE, but CQE has been explicitly disabled (e.g., via device tree)

For the 2nd case I will post another path which will handle host side
CQE enable/disable.
> > +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);
> > +	}
> > +	sdhci_msm_ice_enable(msm_host);
> > +}
> > +
> > +static int sdhci_msm_ice_cfg(struct sdhci_host *host, struct mmc_request *mrq)
> > +{
> > +	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;
> > +	bool crypto_enable;
> > +	u64 dun = 0;
> > +
> > +	if (mrq->crypto_ctx) {
> > +		if (!msm_host->ice_init_done) {
> > +			sdhci_msm_non_cqe_ice_init(host);
> > +			msm_host->ice_init_done = true;
> > +		}
>
> This means sdhci_msm_ice_enable() is called only once per host
> controller.  It looks like the existing call to sdhci_msm_ice_enable()
> happens each time after the host controller is resumed.  So there seems
> to be an inconsistency there.  Which way is correct?
Thank you for highlighting this. After revisiting the code paths, I 
believe the behavior is consistent across both CQE and non-CQE modes.
ICE is re-enabled on every resume via the common 
sdhci_msm_runtime_resume() → sdhci_msm_ice_resume() → qcom_ice_resume() 
→ sdhci_msm_ice_enable() path.
The ice_init_done flag only governs one-time initialization in 
sdhci_msm_ice_cfg() and doesn’t interfere with the resume logic.

In summary:
CQE mode: ICE enabled during sdhci_msm_cqe_enable() + every resume
Non-CQE mode: ICE enabled on first crypto request + every resume
> > +	} else {
> > +		crypto_enable = false;
> > +		key_index = 0;
> > +		cqhci_writel(cq_host, crypto_params, NONCQ_CRYPTO_PARM);
>
> The values assigned to 'crypto_enable' and 'key_index' are never used.
Ok, will remove in next revision.
> > +static void sdhci_msm_request(struct mmc_host *mmc, struct mmc_request *mrq)
> > +{
>
> Could you leave a comment here that notes this is used only for non-CQE
> requests and that crypto on CQE requests is handled elsewhere?
Thank you very much — that’s a valuable suggestion. Adding a comment 
will make the code much clearer. Will add in next revision.

Thanks,
Alam.