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

[Eric Biggers <ebiggers@kernel.org>]

On Wed, Oct 22, 2025 at 10:49:23AM +0530, Md Sadre Alam wrote:
> Hi,
> 
> On 10/17/2025 11:08 PM, Eric Biggers wrote:
> > 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?
> I tested this using fscrypt on a Phison eMMC device. However, since that
> particular eMMC does not support CMDQ, inline encryption (ICE) was bypassed
> during testing.

What do you mean by "inline encryption (ICE) was bypassed during
testing"?

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

Could you also remove the unused 'slot' parameter from this function?

> > > @@ -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.
> ok,I’ll move the ICE initialization earlier in the probe flow, ideally
> before __sdhci_add_host() is called.
> > 
> > > +#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?
> Thanks for raising this. You're right that some parts of the patch interact
> with CQE-related structures, such as cqhci_host, even on CQE-capable hosts.
> However, the intent is to reuse existing CQE infrastructure (like register
> access helpers and memory-mapped regions) to configure ICE for non-CMDQ
> requests.
> 
> Importantly, actual CQE functionality is only enabled if the eMMC device
> advertises CMDQ support. For devices without CMDQ, the CQE engine remains
> disabled, and the request path falls back to standard non-CMDQ flow. In this
> case, we're simply leveraging the CQE register space to program ICE
> parameters.
> 
> So while the code runs on CQE-capable hosts, there's no change in behavior
> for CMDQ-enabled devices — the patch does not interfere with CQE operation.
> It only enables ICE for non-CMDQ requests when supported by the platform.

So, we're dealing only with hosts that do support a command queue, but
support eMMC devices either with or without using it?

Could you explain why sdhci_msm_ice_enable() is called twice: once from
sdhci_msm_cqe_add_host() and once from sdhci_msm_cqe_enable()?

- Eric