Re: [PATCH 2/6] soc: qcom: qpace: Add Qualcomm Page Compression Engine driver

[Oreoluwa Babatunde <oreoluwa.babatunde@oss.qualcomm.com>]

On 10/1/2026 1:50 AM, Krzysztof Kozlowski wrote:
> On Wed, Sep 30, 2026 at 07:52:11AM -0700, Georgi Djakov wrote:
>> Add a platform driver for the Qualcomm Page Compression Engine (QPaCE), a
>> hardware block that accelerates compression and decompression of memory
>> pages.
>>
>> Provide the urgent command path for synchronous single-page compression and
>> decompression. This exposes the low-latency operations needed by
>> compressed-memory users such as zram, especially for page decompression on
>> the read path.
>>
>> Signed-off-by: Georgi Djakov <georgi.djakov@oss.qualcomm.com>
>> ---
>>   drivers/soc/qcom/Kconfig          |  14 +
>>   drivers/soc/qcom/Makefile         |   1 +
>>   drivers/soc/qcom/qpace.c          | 764 ++++++++++++++++++++++++++++++
>>   drivers/soc/qcom/qpace_internal.h |  84 ++++
>>   include/linux/soc/qcom/qpace.h    | 154 ++++++
>>   5 files changed, 1017 insertions(+)
>>   create mode 100644 drivers/soc/qcom/qpace.c
>>   create mode 100644 drivers/soc/qcom/qpace_internal.h
>>   create mode 100644 include/linux/soc/qcom/qpace.h
>>
>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>> index 535c8619197b..6bcb86dcd726 100644
>> --- a/drivers/soc/qcom/Kconfig
> 
> Sorry, but no. Soc is not a dumping ground. This has clear function of
> compression offload, so it should have some dedicated maintainers like
> other offload engines.

The reason for putting this in soc/qcom is because this is a qcom HW 
block driver. As per your comments below we will check and see if we can 
make use of existing crypto framework and respond back on this.

>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -288,6 +288,20 @@ config QCOM_PBS
>>   	  This module provides the APIs to the client drivers that wants to send the
>>   	  PBS trigger event to the PBS RAM.
>>   
>> +config QCOM_PAGE_COMPRESSION_ENGINE
>> +	tristate "Qualcomm Page Compression Engine (QPaCE)"
>> +	depends on ARM64
> 
> Why this can't be built on other archs? This is really odd and I do not
> see any asm headers included.
ACK. We will remove this so that it can be built on other architectures.

> 
>> +	depends on ARCH_QCOM || COMPILE_TEST
>> +	depends on OF
>> +	depends on INTERCONNECT
>> +	help
>> +	  Enable support for the Qualcomm Page Compression Engine (QPaCE),
>> +	  a hardware accelerator that provides high-throughput page compression,
>> +	  decompression, and DMA copy operations.
>> +
>> +	  The engine is used as a hardware backend for compressed-memory
>> +	  subsystems such as zram. If unsure, say N.
>> +
> 
> ...
> 
> 
>> +	ret = FIELD_GET(URG_CMD_0_ED_STAT_SIZE, stat_reg);
>> +out:
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(qpace_urgent_compress);
>> +
>> +int qpace_urgent_decompress(dma_addr_t input_addr,
>> +			    dma_addr_t output_addr,
>> +			    size_t input_size,
>> +			    struct qpace_algorithm *algo)
> 
> You need kerneldoc for every export.

ACK

>> +{
>> +	int urg_reg_num;
>> +	int stat_reg;
>> +	u32 stat_reg_val;
>> +	int ret;
>> +
>> +	ret = qpace_get();
>> +	if (ret)
>> +		goto out;
>> +
>> +	urg_reg_num = get_cpu() % NUM_TRS_ERS_URG_CMD_REGS;
>> +	qpace_write_urg_cmd_ctx(qpace_priv, QPACE_URG_CMD_0_CFG_CNTXT_SIZE_n_OFFSET,
>> +				urg_reg_num, algo->urg_decomp_cntxt,
>> +				FIELD_PREP(URG_CMD_0_CFG_CNTXT_SIZE_SIZE, input_size));
>> +	stat_reg = qpace_urgent_command_trigger(input_addr, output_addr, urg_reg_num,
>> +						algo->urg_decomp_cntxt);
>> +	put_cpu();
>> +
>> +	qpace_put();
>> +
>> +	if (stat_reg < 0) {
>> +		ret = stat_reg;
>> +		goto out;
>> +	}
>> +
>> +	stat_reg_val = FIELD_GET(URG_CMD_0_ED_STAT_COMP_CODE, stat_reg);
>> +	if (stat_reg_val != OP_OK) {
>> +		pr_err("%s: register %d failed with %u\n",
>> +		       __func__, urg_reg_num, stat_reg_val);
>> +		ret = -EINVAL;
>> +		goto out;
>> +	}
>> +
>> +	ret = FIELD_GET(URG_CMD_0_ED_STAT_SIZE, stat_reg);
>> +out:
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(qpace_urgent_decompress);
>> +
> 
> So singleton? For what reason exactly? Random drivers will be getting
> the reference to compress something? If so, aren't you duplicating
> existing infrastructure/API for in-kernel hardware offloaded
> compression (e.g. drivers/crypto/)?
> 

We will check and see if we can use existing crypto framework and 
respond back on this.

> You miss proper comments (see checkpatch --strict) explaining lock
> usage.

ACK

> 
> 
>> +static DEFINE_MUTEX(qpace_ref_lock);
>> +
>> +static void _get_qpace(void)
>> +{
>> +	lockdep_assert_held(&qpace_ref_lock);
>> +	if (!qpace_priv->active_rings) {
>> +		reinit_completion(&qpace_priv->no_active_refs);
>> +		pm_stay_awake(qpace_priv->dev);
>> +		cpu_latency_qos_update_request(&qpace_priv->qos_req, 300);
>> +		program_urg_command_contexts_v2();
>> +		program_decomp_core_cfg();
>> +	}
>> +	qpace_priv->active_rings++;
>> +}
>> +
>> +static void _put_qpace(void)
>> +{
>> +	lockdep_assert_held(&qpace_ref_lock);
>> +	if (!--qpace_priv->active_rings) {
>> +		cpu_latency_qos_update_request(&qpace_priv->qos_req, PM_QOS_DEFAULT_VALUE);
>> +		pm_relax(qpace_priv->dev);
>> +		complete(&qpace_priv->no_active_refs);
>> +	}
>> +}
>> +
>> +int qpace_get(void)
>> +{
>> +	int ret = 0;
>> +
>> +	mutex_lock(&qpace_ref_lock);
>> +	if (qpace_priv->suspended || READ_ONCE(qpace_priv->broken))
>> +		ret = -EBUSY;
>> +	else
>> +		_get_qpace();
>> +	mutex_unlock(&qpace_ref_lock);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(qpace_get);
>> +
>> +void qpace_put(void)
>> +{
>> +	mutex_lock(&qpace_ref_lock);
>> +	_put_qpace();
>> +	mutex_unlock(&qpace_ref_lock);
>> +}
>> +EXPORT_SYMBOL_GPL(qpace_put);
>> +
>> +static irqreturn_t urgent_interrupt_handler(int irq, void *unused)
>> +{
>> +	pr_debug("Urgent interrupt handled\n");
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int qpace_hw_init(void)
>> +{
>> +	u32 reg_val;
>> +
>> +	/* Select CPU SCID for our system cache slice. */
>> +	reg_val = qpace_read_gen(qpace_priv, QPACE_CORE_QNS4_CFG_OFFSET);
>> +	reg_val = u32_replace_bits(reg_val, 0x1, CORE_QNS4_CFG_CACHEINDEX);
>> +	qpace_write_gen(qpace_priv, QPACE_CORE_QNS4_CFG_OFFSET, reg_val);
>> +
>> +	/* QMB2 register configurations. */
>> +	reg_val = qpace_read_gen(qpace_priv, QPACE_CORE_GEN_CFG_OFFSET);
>> +	reg_val = u32_replace_bits(reg_val, 0x48, CORE_GEN_CFG_QMB2_MAX_RD_OUTST_LIMIT);
>> +	reg_val = u32_replace_bits(reg_val, 0x48, CORE_GEN_CFG_QMB2_MAX_WR_OUTST_LIMIT);
>> +	qpace_write_gen(qpace_priv, QPACE_CORE_GEN_CFG_OFFSET, reg_val);
>> +
>> +	/* DECOMP_CORE_CFG init steps. */
>> +	program_decomp_core_cfg();
>> +
>> +	/* Below settings help save power since all decomp cores are set to sync. */
>> +	reg_val = qpace_read_gen_core(qpace_priv, QPACE_CORE_OPER_CFG_OFFSET);
>> +	reg_val |= CORE_OPER_CFG_COMP_MEM_PWR_DWN_1;
>> +	qpace_write_gen_core(qpace_priv, QPACE_CORE_OPER_CFG_OFFSET, reg_val);
>> +
>> +	reg_val = qpace_read_comp_core(qpace_priv, QPACE_COMP_CORE_CFG_OFFSET);
>> +	reg_val = u32_replace_bits(reg_val, 0x8, COMP_CORE_CFG_DMA_RD_MAX_OT);
>> +	reg_val = u32_replace_bits(reg_val, 0x8, COMP_CORE_CFG_DMA_WR_MAX_OT);
>> +	qpace_write_comp_core(qpace_priv, QPACE_COMP_CORE_CFG_OFFSET, reg_val);
>> +
>> +	/* Set all COMP engines to bulk mode. */
>> +	reg_val = qpace_read_comp_core(qpace_priv, QPACE_COMP_CORE_BULK_MODE_OFFSET);
>> +	reg_val |= COMP_CORE_BULK_MODE_ALL_CORES;
>> +	qpace_write_comp_core(qpace_priv, QPACE_COMP_CORE_BULK_MODE_OFFSET, reg_val);
>> +
>> +	/* URG CMD register configurations. */
>> +	program_urg_command_contexts_v2();
>> +
>> +	return 0;
>> +}
>> +
>> +enum qpace_interrupts {
>> +	QPACE_IRQ_URGENT
>> +};
>> +
>> +static int qpace_register_interrupts(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	int irq, ret;
>> +
>> +	irq = platform_get_irq(pdev, QPACE_IRQ_URGENT);
>> +	if (irq < 0)
>> +		return irq;
>> +
>> +	ret = devm_request_irq(dev, irq, urgent_interrupt_handler,
>> +			       0, "qpace-urgent-irq", NULL);
>> +	if (ret)
>> +		dev_err(dev, "failed to request urgent interrupt\n");
>> +
>> +	return ret;
>> +}
>> +
>> +static inline bool _qpace_power_on(void)
>> +{
>> +	u32 ready_status;
>> +
>> +	qpace_write_gen_core(qpace_priv, QPACE_CORE_OPER_CORE_RUN_STOP_OFFSET, QPACE_RUN);
>> +
>> +	if (readl_poll_timeout(qpace_priv->gen_core_regs +
>> +			       QPACE_CORE_OPER_CORE_READY_OFFSET,
>> +			       ready_status, ready_status,
>> +			       1000, 5 * QPACE_STATE_CHANGE_TIMEOUT_US)) {
>> +		pr_err("Timeout in waiting for QPaCE to turn on\n");
>> +		return false;
>> +	}
>> +
>> +	return true;
>> +}
>> +
>> +static int qpace_power_on(struct device *dev)
>> +{
>> +	int ret, ret2;
>> +
>> +	qpace_priv->interconnect = devm_of_icc_get(dev, "qpace-mem");
>> +	if (IS_ERR_OR_NULL(qpace_priv->interconnect)) {
>> +		ret = PTR_ERR_OR_ZERO(qpace_priv->interconnect);
>> +		pr_err("%s: devm_of_icc_get() failed with %d\n", __func__, ret);
> 
> use dev_err, not pr_err

ACK.

> 
>> +		return qpace_priv->interconnect ? ret : -EINVAL;
>> +	}
>> +
>> +	ret = device_init_wakeup(dev, true);
>> +	if (ret) {
>> +		pr_err("%s: device_init_wakeup() failed with %d\n", __func__, ret);
>> +		return ret;
>> +	}
>> +
>> +	cpu_latency_qos_add_request(&qpace_priv->qos_req, PM_QOS_DEFAULT_VALUE);
>> +
>> +	icc_set_tag(qpace_priv->interconnect, QCOM_ICC_TAG_ACTIVE_ONLY);
>> +
>> +	ret = icc_set_bw(qpace_priv->interconnect, 0, 1);
>> +	if (ret) {
>> +		pr_err("Failed to turn on QPaCE VCD: %d\n", ret);
>> +		goto rm_qos;
>> +	}
>> +
>> +	if (!_qpace_power_on()) {
>> +		pr_err("Failed to start QPaCE\n");
>> +		ret = -EINVAL;
>> +		goto rm_bw;
>> +	}
>> +
>> +	return 0;
>> +
>> +rm_bw:
>> +	ret2 = icc_set_bw(qpace_priv->interconnect, 0, 0);
>> +	if (ret2)
>> +		pr_err("Failed to remove QPaCE VCD vote: %d\n", ret2);
>> +rm_qos:
>> +	cpu_latency_qos_remove_request(&qpace_priv->qos_req);
>> +	device_init_wakeup(dev, false);
>> +
>> +	return ret;
>> +}
>> +
>> +static inline bool _qpace_power_off(void)
>> +{
>> +	u32 ready_status;
>> +
>> +	qpace_write_gen_core(qpace_priv, QPACE_CORE_OPER_CORE_RUN_STOP_OFFSET, QPACE_STOP);
>> +
>> +	if (readl_poll_timeout(qpace_priv->gen_core_regs +
>> +			       QPACE_CORE_OPER_CORE_READY_OFFSET,
>> +			       ready_status, !ready_status,
>> +			       1000, 5 * QPACE_STATE_CHANGE_TIMEOUT_US)) {
>> +		pr_err("Timeout in waiting for QPaCE to turn off\n");
>> +		return false;
>> +	}
>> +
>> +	return true;
>> +}
>> +
>> +static void qpace_power_off(struct device *dev)
>> +{
>> +	int ret;
>> +
>> +	/* If this fails we can still remove our vote for the VCD to turn QPaCE off */
>> +	if (!_qpace_power_off())
>> +		pr_err("Failed to stop QPaCE\n");
>> +
>> +	ret = icc_set_bw(qpace_priv->interconnect, 0, 0);
>> +	if (ret)
>> +		pr_err("Failed to turn off QPaCE VCD: %d\n", ret);
>> +
>> +	cpu_latency_qos_remove_request(&qpace_priv->qos_req);
>> +
>> +	device_init_wakeup(dev, false);
>> +}
>> +
>> +static inline int qpace_register_ioremap(struct platform_device *pdev)
>> +{
>> +	qpace_priv->gen_regs = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(qpace_priv->gen_regs))
>> +		return PTR_ERR(qpace_priv->gen_regs);
>> +
>> +	qpace_priv->gen_core_regs = qpace_priv->gen_regs + QPACE_GEN_CORE_REGS_OFFSET;
>> +	qpace_priv->comp_core_regs = qpace_priv->gen_regs + QPACE_COMP_CORE_REGS_OFFSET;
>> +	qpace_priv->decomp_core_regs = qpace_priv->gen_regs + QPACE_DECOMP_CORE_REGS_OFFSET;
>> +	qpace_priv->urg_regs = qpace_priv->gen_regs + QPACE_URG_REGS_OFFSET;
>> +
>> +	return 0;
>> +}
>> +
>> +bool qpace_is_dev_available(void)
>> +{
>> +	return static_branch_likely(&qpace_drv_probed) &&
>> +	       !READ_ONCE(qpace_priv->broken);
>> +}
>> +EXPORT_SYMBOL_GPL(qpace_is_dev_available);
>> +
>> +struct device *qpace_get_dma_dev(void)
>> +{
>> +	return qpace_priv->dev;
>> +}
>> +EXPORT_SYMBOL_GPL(qpace_get_dma_dev);
>> +
>> +static int qpace_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct qpace_priv *priv;
>> +	int ret;
>> +
>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	priv->dev = dev;
>> +	/* Starts already complete since active_rings == 0 at init. */
>> +	init_completion(&priv->no_active_refs);
>> +	complete(&priv->no_active_refs);
>> +	INIT_WORK(&priv->disable_work, qpace_disable_work_fn);
>> +	qpace_priv = priv;
>> +	platform_set_drvdata(pdev, priv);
>> +
>> +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "failed to set DMA mask\n");
>> +
>> +	ret = qpace_register_ioremap(pdev);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "failed to map QPaCE registers\n");
>> +
>> +	ret = qpace_power_on(dev);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "failed to power on QPaCE\n");
>> +
>> +	/* Get QPaCE HW version. */
>> +	qpace_priv->hw_version = qpace_read_gen(qpace_priv, QPACE_CORE_HW_VERSION_OFFSET);
>> +	if (qpace_priv->hw_version != QPACE_HW_VERSION_V2) {
>> +		dev_err(dev, "Unsupported QPaCE HW version returned: 0x%x\n",
>> +			qpace_priv->hw_version);
>> +		ret = -EINVAL;
>> +		goto power_off;
>> +	}
>> +
>> +	ret = qpace_hw_init();
>> +	if (ret) {
> 
> How is this possible?

ACK. This can be removed.

> 
>> +		dev_err(dev, "init failed: (%d)\n", ret);
>> +		goto power_off;
>> +	}
>> +
>> +	ret = qpace_register_interrupts(pdev);
>> +	if (ret) {
>> +		dev_err(dev, "failed to register interrupts\n");
> 
> Do not print same error multiple times.

ACK.

> 
>> +		goto power_off;
>> +	}
>> +
>> +	static_branch_enable(&qpace_drv_probed);
>> +
>> +	return ret;
>> +
>> +power_off:
>> +	qpace_power_off(dev);
>> +
>> +	return ret;
>> +}
> 
> Best regards,
> Krzysztof