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

On 10/1/2026 1:50 AM, Krzysztof Kozlowski wrote:

On Wed, Sep 30, 2026 at 07:52:11AM -0700, Georgi Djakov wrote:

quoted

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 <redacted>
---
  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.

quoted

+++ 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.

quoted

+	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.
+

...

quoted

+	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

quoted

+{
+	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

quoted

+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.

quoted

+		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.

quoted

+		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.

quoted

+		goto power_off;
+	}
+
+	static_branch_enable(&qpace_drv_probed);
+
+	return ret;
+
+power_off:
+	qpace_power_off(dev);
+
+	return ret;
+}

Best regards,
Krzysztof