[PATCH v9 00/11] crypto/dmaengine: qce: introduce BAM locking and use DMA for register I/O

[PATCH v9 00/11] crypto/dmaengine: qce: introduce BAM locking and use DMA for register I/O

[Bartosz Golaszewski]

Currently the QCE crypto driver accesses the crypto engine registers
directly via CPU. Trust Zone may perform crypto operations simultaneously
resulting in a race condition. To remedy that, let's introduce support
for BAM locking/unlocking using DMA descriptor metadata as medium for
passing the relevant information from the QCE engine driver to the BAM
driver.

In the specific case of the BAM DMA this translates to sending command
descriptors performing dummy writes with the relevant flags set. The BAM
will then lock all other pipes not related to the current pipe group, and
keep handling the current pipe only until it sees the the unlock bit.

In order for the locking to work correctly, we also need to switch to
using DMA for all register I/O.

On top of this, the series contains some additional tweaks and
refactoring.

The goal of this is not to improve the performance but to prepare the
driver for supporting decryption into secure buffers in the future.

Tested with tcrypt.ko, kcapi and cryptsetup.

Shout out to Daniel and Udit from Qualcomm for helping me out with some
DMA issues we encountered.

Merging strategy: either an Ack from Vinod or an immutable branch with
the DMA changes for the crypto subsystem will work.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
Changes in v9:
- Drop the global, generic LOCK/UNLOCK flags and instead use DMA
  descriptor metadata ops to pass BAM-specific information from the QCE
  to the DMA engine
- Link to v8: https://lore.kernel.org/r/20251106-qcom-qce-cmd-descr-v8-0-ecddca23ca26@linaro.org

Changes in v8:
- Rework the command descriptor logic and drop a lot of unneeded code
- Use the physical address for BAM command descriptor access, not the
  mapped DMA address
- Fix the problems with iommu faults on newer platforms
- Generalize the LOCK/UNLOCK flags in dmaengine and reword the docs and
  commit messages
- Make the BAM locking logic stricter in the DMA engine driver
- Add some additional minor QCE driver refactoring changes to the series
- Lots of small reworks and tweaks to rebase on current mainline and fix
  previous issues
- Link to v7: https://lore.kernel.org/all/20250311-qce-cmd-descr-v7-0-db613f5d9c9f@linaro.org/

Changes in v7:
- remove unused code: writing to multiple registers was not used in v6,
  neither were the functions for reading registers over BAM DMA-
- remove
- don't read the SW_VERSION register needlessly in the BAM driver,
  instead: encode the information on whether the IP supports BAM locking
  in device match data
- shrink code where possible with logic modifications (for instance:
  change the implementation of qce_write() instead of replacing it
  everywhere with a new symbol)
- remove duplicated error messages
- rework commit messages
- a lot of shuffling code around for easier review and a more
  streamlined series
- Link to v6: https://lore.kernel.org/all/20250115103004.3350561-1-quic_mdalam@quicinc.com/

Changes in v6:
- change "BAM" to "DMA"
- Ensured this series is compilable with the current Linux-next tip of
  the tree (TOT).

Changes in v5:
- Added DMA_PREP_LOCK and DMA_PREP_UNLOCK flag support in separate patch
- Removed DMA_PREP_LOCK & DMA_PREP_UNLOCK flag
- Added FIELD_GET and GENMASK macro to extract major and minor version

Changes in v4:
- Added feature description and test hardware
  with test command
- Fixed patch version numbering
- Dropped dt-binding patch
- Dropped device tree changes
- Added BAM_SW_VERSION register read
- Handled the error path for the api dma_map_resource()
  in probe
- updated the commit messages for batter redability
- Squash the change where qce_bam_acquire_lock() and
  qce_bam_release_lock() api got introduce to the change where
  the lock/unlock flag get introced
- changed cover letter subject heading to
  "dmaengine: qcom: bam_dma: add cmd descriptor support"
- Added the very initial post for BAM lock/unlock patch link
  as v1 to track this feature

Changes in v3:
- https://lore.kernel.org/lkml/183d4f5e-e00a-8ef6-a589-f5704bc83d4a@quicinc.com/
- Addressed all the comments from v2
- Added the dt-binding
- Fix alignment issue
- Removed type casting from qce_write_reg_dma()
  and qce_read_reg_dma()
- Removed qce_bam_txn = dma->qce_bam_txn; line from
  qce_alloc_bam_txn() api and directly returning
  dma->qce_bam_txn

Changes in v2:
- https://lore.kernel.org/lkml/20231214114239.2635325-1-quic_mdalam@quicinc.com/
- Initial set of patches for cmd descriptor support
- Add client driver to use BAM lock/unlock feature
- Added register read/write via BAM in QCE Crypto driver
  to use BAM lock/unlock feature

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

---
Bartosz Golaszewski (11):
      dmaengine: qcom: bam_dma: Extend the driver's device match data
      dmaengine: qcom: bam_dma: Add bam_pipe_lock flag support
      dmaengine: qcom: bam_dma: implement support for BAM locking
      crypto: qce - Include algapi.h in the core.h header
      crypto: qce - Remove unused ignore_buf
      crypto: qce - Simplify arguments of devm_qce_dma_request()
      crypto: qce - Use existing devres APIs in devm_qce_dma_request()
      crypto: qce - Map crypto memory for DMA
      crypto: qce - Add BAM DMA support for crypto register I/O
      crypto: qce - Add support for BAM locking
      crypto: qce - Switch to using BAM DMA for crypto I/O

 drivers/crypto/qce/aead.c        |  10 +++
 drivers/crypto/qce/common.c      |  39 ++++++---
 drivers/crypto/qce/core.c        |  28 ++++++-
 drivers/crypto/qce/core.h        |  11 +++
 drivers/crypto/qce/dma.c         | 176 ++++++++++++++++++++++++++++++++-------
 drivers/crypto/qce/dma.h         |  15 +++-
 drivers/crypto/qce/sha.c         |   8 ++
 drivers/crypto/qce/skcipher.c    |   7 ++
 drivers/dma/qcom/bam_dma.c       |  92 ++++++++++++++++++--
 include/linux/dma/qcom_bam_dma.h |  12 +++
 10 files changed, 345 insertions(+), 53 deletions(-)
---
base-commit: 60ce1919ad707c77df15449238bcb665020ffc93
change-id: 20251103-qcom-qce-cmd-descr-c5e9b11fe609

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

[PATCH v9 01/11] dmaengine: qcom: bam_dma: Extend the driver's device match data

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

In preparation for supporting the pipe locking feature flag, extend the
amount of information we can carry in device match data: create a
separate structure and make the register information one of its fields.
This way, in subsequent patches, it will be just a matter of adding a
new field to the device data.

Reviewed-by: Dmitry Baryshkov <lumag@kernel.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/dma/qcom/bam_dma.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
index 2cf060174795fe326abaf053a7a7a10022455586..8861245314b1d13c1abb78f474fd0749fea52f06 100644
--- a/drivers/dma/qcom/bam_dma.c
+++ b/drivers/dma/qcom/bam_dma.c
@@ -111,6 +111,10 @@ struct reg_offset_data {
 	unsigned int pipe_mult, evnt_mult, ee_mult;
 };
 
+struct bam_device_data {
+	const struct reg_offset_data *reg_info;
+};
+
 static const struct reg_offset_data bam_v1_3_reg_info[] = {
 	[BAM_CTRL]		= { 0x0F80, 0x00, 0x00, 0x00 },
 	[BAM_REVISION]		= { 0x0F84, 0x00, 0x00, 0x00 },
@@ -140,6 +144,10 @@ static const struct reg_offset_data bam_v1_3_reg_info[] = {
 	[BAM_P_FIFO_SIZES]	= { 0x1020, 0x00, 0x40, 0x00 },
 };
 
+static const struct bam_device_data bam_v1_3_data = {
+	.reg_info = bam_v1_3_reg_info,
+};
+
 static const struct reg_offset_data bam_v1_4_reg_info[] = {
 	[BAM_CTRL]		= { 0x0000, 0x00, 0x00, 0x00 },
 	[BAM_REVISION]		= { 0x0004, 0x00, 0x00, 0x00 },
@@ -169,6 +177,10 @@ static const struct reg_offset_data bam_v1_4_reg_info[] = {
 	[BAM_P_FIFO_SIZES]	= { 0x1820, 0x00, 0x1000, 0x00 },
 };
 
+static const struct bam_device_data bam_v1_4_data = {
+	.reg_info = bam_v1_4_reg_info,
+};
+
 static const struct reg_offset_data bam_v1_7_reg_info[] = {
 	[BAM_CTRL]		= { 0x00000, 0x00, 0x00, 0x00 },
 	[BAM_REVISION]		= { 0x01000, 0x00, 0x00, 0x00 },
@@ -198,6 +210,10 @@ static const struct reg_offset_data bam_v1_7_reg_info[] = {
 	[BAM_P_FIFO_SIZES]	= { 0x13820, 0x00, 0x1000, 0x00 },
 };
 
+static const struct bam_device_data bam_v1_7_data = {
+	.reg_info = bam_v1_7_reg_info,
+};
+
 /* BAM CTRL */
 #define BAM_SW_RST			BIT(0)
 #define BAM_EN				BIT(1)
@@ -391,7 +407,7 @@ struct bam_device {
 	bool powered_remotely;
 	u32 active_channels;
 
-	const struct reg_offset_data *layout;
+	const struct bam_device_data *dev_data;
 
 	struct clk *bamclk;
 	int irq;
@@ -409,7 +425,7 @@ struct bam_device {
 static inline void __iomem *bam_addr(struct bam_device *bdev, u32 pipe,
 		enum bam_reg reg)
 {
-	const struct reg_offset_data r = bdev->layout[reg];
+	const struct reg_offset_data r = bdev->dev_data->reg_info[reg];
 
 	return bdev->regs + r.base_offset +
 		r.pipe_mult * pipe +
@@ -1225,9 +1241,9 @@ static void bam_channel_init(struct bam_device *bdev, struct bam_chan *bchan,
 }
 
 static const struct of_device_id bam_of_match[] = {
-	{ .compatible = "qcom,bam-v1.3.0", .data = &bam_v1_3_reg_info },
-	{ .compatible = "qcom,bam-v1.4.0", .data = &bam_v1_4_reg_info },
-	{ .compatible = "qcom,bam-v1.7.0", .data = &bam_v1_7_reg_info },
+	{ .compatible = "qcom,bam-v1.3.0", .data = &bam_v1_3_data },
+	{ .compatible = "qcom,bam-v1.4.0", .data = &bam_v1_4_data },
+	{ .compatible = "qcom,bam-v1.7.0", .data = &bam_v1_7_data },
 	{}
 };
 
@@ -1251,7 +1267,7 @@ static int bam_dma_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	bdev->layout = match->data;
+	bdev->dev_data = match->data;
 
 	bdev->regs = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(bdev->regs))

-- 
2.51.0

[PATCH v9 02/11] dmaengine: qcom: bam_dma: Add bam_pipe_lock flag support

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Extend the device match data with a flag indicating whether the IP
supports the BAM lock/unlock feature. Set it to true on BAM IP versions
1.4.0 and above.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/dma/qcom/bam_dma.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
index 8861245314b1d13c1abb78f474fd0749fea52f06..c9ae1fffe44d79c5eb59b8bbf7f147a8fa3aa0bd 100644
--- a/drivers/dma/qcom/bam_dma.c
+++ b/drivers/dma/qcom/bam_dma.c
@@ -58,6 +58,8 @@ struct bam_desc_hw {
 #define DESC_FLAG_EOB BIT(13)
 #define DESC_FLAG_NWD BIT(12)
 #define DESC_FLAG_CMD BIT(11)
+#define DESC_FLAG_LOCK BIT(10)
+#define DESC_FLAG_UNLOCK BIT(9)
 
 struct bam_async_desc {
 	struct virt_dma_desc vd;
@@ -113,6 +115,7 @@ struct reg_offset_data {
 
 struct bam_device_data {
 	const struct reg_offset_data *reg_info;
+	bool bam_pipe_lock;
 };
 
 static const struct reg_offset_data bam_v1_3_reg_info[] = {
@@ -179,6 +182,7 @@ static const struct reg_offset_data bam_v1_4_reg_info[] = {
 
 static const struct bam_device_data bam_v1_4_data = {
 	.reg_info = bam_v1_4_reg_info,
+	.bam_pipe_lock = true,
 };
 
 static const struct reg_offset_data bam_v1_7_reg_info[] = {
@@ -212,6 +216,7 @@ static const struct reg_offset_data bam_v1_7_reg_info[] = {
 
 static const struct bam_device_data bam_v1_7_data = {
 	.reg_info = bam_v1_7_reg_info,
+	.bam_pipe_lock = true,
 };
 
 /* BAM CTRL */

-- 
2.51.0

[PATCH v9 03/11] dmaengine: qcom: bam_dma: implement support for BAM locking

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Use metadata operations in DMA descriptors to allow BAM users to pass
additional information to the engine. To that end: define a new
structure - struct bam_desc_metadata - as a medium and define two new
commands: for locking and unlocking the BAM respectively. Handle the
locking in the .attach() callback.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/dma/qcom/bam_dma.c       | 59 +++++++++++++++++++++++++++++++++++++++-
 include/linux/dma/qcom_bam_dma.h | 12 ++++++++
 2 files changed, 70 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
index c9ae1fffe44d79c5eb59b8bbf7f147a8fa3aa0bd..d1dc80b29818897b333cd223ec7306a169cc51fd 100644
--- a/drivers/dma/qcom/bam_dma.c
+++ b/drivers/dma/qcom/bam_dma.c
@@ -30,6 +30,7 @@
 #include <linux/module.h>
 #include <linux/interrupt.h>
 #include <linux/dma-mapping.h>
+#include <linux/dma/qcom_bam_dma.h>
 #include <linux/scatterlist.h>
 #include <linux/device.h>
 #include <linux/platform_device.h>
@@ -391,6 +392,8 @@ struct bam_chan {
 	struct list_head desc_list;
 
 	struct list_head node;
+
+	bool bam_locked;
 };
 
 static inline struct bam_chan *to_bam_chan(struct dma_chan *common)
@@ -655,6 +658,53 @@ static int bam_slave_config(struct dma_chan *chan,
 	return 0;
 }
 
+static int bam_metadata_attach(struct dma_async_tx_descriptor *desc, void *data, size_t len)
+{
+	struct virt_dma_desc *vd = container_of(desc, struct virt_dma_desc, tx);
+	struct bam_async_desc *async_desc = container_of(vd, struct bam_async_desc,  vd);
+	struct bam_desc_hw *hw_desc = async_desc->desc;
+	struct bam_desc_metadata *metadata = data;
+	struct bam_chan *bchan = to_bam_chan(metadata->chan);
+	struct bam_device *bdev = bchan->bdev;
+
+	if (!data)
+		return -EINVAL;
+
+	if (metadata->op == BAM_META_CMD_LOCK || metadata->op == BAM_META_CMD_UNLOCK) {
+		if (!bdev->dev_data->bam_pipe_lock)
+			return -EOPNOTSUPP;
+
+		/* Expecting a dummy write when locking, only one descriptor allowed. */
+		if (async_desc->num_desc != 1)
+			return -EINVAL;
+	}
+
+	switch (metadata->op) {
+	case BAM_META_CMD_LOCK:
+		if (bchan->bam_locked)
+			return -EBUSY;
+
+		hw_desc->flags |= DESC_FLAG_LOCK;
+		bchan->bam_locked = true;
+		break;
+	case BAM_META_CMD_UNLOCK:
+		if (!bchan->bam_locked)
+			return -EPERM;
+
+		hw_desc->flags |= DESC_FLAG_UNLOCK;
+		bchan->bam_locked = false;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+static struct dma_descriptor_metadata_ops bam_metadata_ops = {
+	.attach = bam_metadata_attach,
+};
+
 /**
  * bam_prep_slave_sg - Prep slave sg transaction
  *
@@ -671,6 +721,7 @@ static struct dma_async_tx_descriptor *bam_prep_slave_sg(struct dma_chan *chan,
 	void *context)
 {
 	struct bam_chan *bchan = to_bam_chan(chan);
+	struct dma_async_tx_descriptor *tx_desc;
 	struct bam_device *bdev = bchan->bdev;
 	struct bam_async_desc *async_desc;
 	struct scatterlist *sg;
@@ -732,7 +783,12 @@ static struct dma_async_tx_descriptor *bam_prep_slave_sg(struct dma_chan *chan,
 		} while (remainder > 0);
 	}
 
-	return vchan_tx_prep(&bchan->vc, &async_desc->vd, flags);
+	tx_desc = vchan_tx_prep(&bchan->vc, &async_desc->vd, flags);
+	if (!tx_desc)
+		return NULL;
+
+	tx_desc->metadata_ops = &bam_metadata_ops;
+	return tx_desc;
 }
 
 /**
@@ -1372,6 +1428,7 @@ static int bam_dma_probe(struct platform_device *pdev)
 	bdev->common.device_terminate_all = bam_dma_terminate_all;
 	bdev->common.device_issue_pending = bam_issue_pending;
 	bdev->common.device_tx_status = bam_tx_status;
+	bdev->common.desc_metadata_modes = DESC_METADATA_CLIENT;
 	bdev->common.dev = bdev->dev;
 
 	ret = dma_async_device_register(&bdev->common);
diff --git a/include/linux/dma/qcom_bam_dma.h b/include/linux/dma/qcom_bam_dma.h
index 68fc0e643b1b97fe4520d5878daa322b81f4f559..dd30bb9c520fac7bd98c5a47e56a5a286331461a 100644
--- a/include/linux/dma/qcom_bam_dma.h
+++ b/include/linux/dma/qcom_bam_dma.h
@@ -8,6 +8,8 @@
 
 #include <asm/byteorder.h>
 
+struct dma_chan;
+
 /*
  * This data type corresponds to the native Command Element
  * supported by BAM DMA Engine.
@@ -34,6 +36,16 @@ enum bam_command_type {
 	BAM_READ_COMMAND,
 };
 
+enum bam_desc_metadata_op {
+	BAM_META_CMD_LOCK = 1,
+	BAM_META_CMD_UNLOCK,
+};
+
+struct bam_desc_metadata {
+	enum bam_desc_metadata_op op;
+	struct dma_chan *chan;
+};
+
 /*
  * prep_bam_ce_le32 - Wrapper function to prepare a single BAM command
  * element with the data already in le32 format.

-- 
2.51.0

[PATCH v9 04/11] crypto: qce - Include algapi.h in the core.h header

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

The header defines a struct embedding struct crypto_queue whose size
needs to be known and which is defined in crypto/algapi.h. Move the
inclusion from core.c to core.h.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/crypto/qce/core.c | 1 -
 drivers/crypto/qce/core.h | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
index b966f3365b7de8d2a8f6707397a34aa4facdc4ac..65205100c3df961ffaa4b7bc9e217e8d3e08ed57 100644
--- a/drivers/crypto/qce/core.c
+++ b/drivers/crypto/qce/core.c
@@ -13,7 +13,6 @@
 #include <linux/mod_devicetable.h>
 #include <linux/platform_device.h>
 #include <linux/types.h>
-#include <crypto/algapi.h>
 #include <crypto/internal/hash.h>
 
 #include "core.h"
diff --git a/drivers/crypto/qce/core.h b/drivers/crypto/qce/core.h
index eb6fa7a8b64a81daf9ad5304a3ae4e5e597a70b8..f092ce2d3b04a936a37805c20ac5ba78d8fdd2df 100644
--- a/drivers/crypto/qce/core.h
+++ b/drivers/crypto/qce/core.h
@@ -8,6 +8,7 @@
 
 #include <linux/mutex.h>
 #include <linux/workqueue.h>
+#include <crypto/algapi.h>
 
 #include "dma.h"
 

-- 
2.51.0

[PATCH v9 05/11] crypto: qce - Remove unused ignore_buf

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

It's unclear what the purpose of this field is. It has been here since
the initial commit but without any explanation. The driver works fine
without it. We still keep allocating more space in the result buffer, we
just don't need to store its address. While at it: move the
QCE_IGNORE_BUF_SZ definition into dma.c as it's not used outside of this
compilation unit.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/crypto/qce/dma.c | 4 ++--
 drivers/crypto/qce/dma.h | 2 --
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/crypto/qce/dma.c b/drivers/crypto/qce/dma.c
index 68cafd4741ad3d91906d39e817fc7873b028d498..08bf3e8ec12433c1a8ee17003f3487e41b7329e4 100644
--- a/drivers/crypto/qce/dma.c
+++ b/drivers/crypto/qce/dma.c
@@ -9,6 +9,8 @@
 
 #include "dma.h"
 
+#define QCE_IGNORE_BUF_SZ		(2 * QCE_BAM_BURST_SIZE)
+
 static void qce_dma_release(void *data)
 {
 	struct qce_dma_data *dma = data;
@@ -41,8 +43,6 @@ int devm_qce_dma_request(struct device *dev, struct qce_dma_data *dma)
 		goto error_nomem;
 	}
 
-	dma->ignore_buf = dma->result_buf + QCE_RESULT_BUF_SZ;
-
 	return devm_add_action_or_reset(dev, qce_dma_release, dma);
 
 error_nomem:
diff --git a/drivers/crypto/qce/dma.h b/drivers/crypto/qce/dma.h
index 31629185000e12242fa07c2cc08b95fcbd5d4b8c..fc337c435cd14917bdfb99febcf9119275afdeba 100644
--- a/drivers/crypto/qce/dma.h
+++ b/drivers/crypto/qce/dma.h
@@ -23,7 +23,6 @@ struct qce_result_dump {
 	u32 status2;
 };
 
-#define QCE_IGNORE_BUF_SZ	(2 * QCE_BAM_BURST_SIZE)
 #define QCE_RESULT_BUF_SZ	\
 		ALIGN(sizeof(struct qce_result_dump), QCE_BAM_BURST_SIZE)
 
@@ -31,7 +30,6 @@ struct qce_dma_data {
 	struct dma_chan *txchan;
 	struct dma_chan *rxchan;
 	struct qce_result_dump *result_buf;
-	void *ignore_buf;
 };
 
 int devm_qce_dma_request(struct device *dev, struct qce_dma_data *dma);

-- 
2.51.0

[PATCH v9 06/11] crypto: qce - Simplify arguments of devm_qce_dma_request()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

This function can extract all the information it needs from struct
qce_device alone so simplify its arguments. This is done in preparation
for adding support for register I/O over DMA which will require
accessing even more fields from struct qce_device.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/crypto/qce/core.c | 2 +-
 drivers/crypto/qce/dma.c  | 5 ++++-
 drivers/crypto/qce/dma.h  | 4 +++-
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
index 65205100c3df961ffaa4b7bc9e217e8d3e08ed57..8b7bcd0c420c45caf8b29e5455e0f384fd5c5616 100644
--- a/drivers/crypto/qce/core.c
+++ b/drivers/crypto/qce/core.c
@@ -226,7 +226,7 @@ static int qce_crypto_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	ret = devm_qce_dma_request(qce->dev, &qce->dma);
+	ret = devm_qce_dma_request(qce);
 	if (ret)
 		return ret;
 
diff --git a/drivers/crypto/qce/dma.c b/drivers/crypto/qce/dma.c
index 08bf3e8ec12433c1a8ee17003f3487e41b7329e4..c29b0abe9445381a019e0447d30acfd7319d5c1f 100644
--- a/drivers/crypto/qce/dma.c
+++ b/drivers/crypto/qce/dma.c
@@ -7,6 +7,7 @@
 #include <linux/dmaengine.h>
 #include <crypto/scatterwalk.h>
 
+#include "core.h"
 #include "dma.h"
 
 #define QCE_IGNORE_BUF_SZ		(2 * QCE_BAM_BURST_SIZE)
@@ -20,8 +21,10 @@ static void qce_dma_release(void *data)
 	kfree(dma->result_buf);
 }
 
-int devm_qce_dma_request(struct device *dev, struct qce_dma_data *dma)
+int devm_qce_dma_request(struct qce_device *qce)
 {
+	struct qce_dma_data *dma = &qce->dma;
+	struct device *dev = qce->dev;
 	int ret;
 
 	dma->txchan = dma_request_chan(dev, "tx");
diff --git a/drivers/crypto/qce/dma.h b/drivers/crypto/qce/dma.h
index fc337c435cd14917bdfb99febcf9119275afdeba..483789d9fa98e79d1283de8297bf2fc2a773f3a7 100644
--- a/drivers/crypto/qce/dma.h
+++ b/drivers/crypto/qce/dma.h
@@ -8,6 +8,8 @@
 
 #include <linux/dmaengine.h>
 
+struct qce_device;
+
 /* maximum data transfer block size between BAM and CE */
 #define QCE_BAM_BURST_SIZE		64
 
@@ -32,7 +34,7 @@ struct qce_dma_data {
 	struct qce_result_dump *result_buf;
 };
 
-int devm_qce_dma_request(struct device *dev, struct qce_dma_data *dma);
+int devm_qce_dma_request(struct qce_device *qce);
 int qce_dma_prep_sgs(struct qce_dma_data *dma, struct scatterlist *sg_in,
 		     int in_ents, struct scatterlist *sg_out, int out_ents,
 		     dma_async_tx_callback cb, void *cb_param);

-- 
2.51.0

[PATCH v9 07/11] crypto: qce - Use existing devres APIs in devm_qce_dma_request()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Switch to devm_kmalloc() and devm_dma_alloc_chan() in
devm_qce_dma_request(). This allows us to drop two labels and shrink the
function.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/crypto/qce/dma.c | 39 +++++++++------------------------------
 1 file changed, 9 insertions(+), 30 deletions(-)

diff --git a/drivers/crypto/qce/dma.c b/drivers/crypto/qce/dma.c
index c29b0abe9445381a019e0447d30acfd7319d5c1f..a46264735bb895b6199969e83391383ccbbacc5f 100644
--- a/drivers/crypto/qce/dma.c
+++ b/drivers/crypto/qce/dma.c
@@ -12,47 +12,26 @@
 
 #define QCE_IGNORE_BUF_SZ		(2 * QCE_BAM_BURST_SIZE)
 
-static void qce_dma_release(void *data)
-{
-	struct qce_dma_data *dma = data;
-
-	dma_release_channel(dma->txchan);
-	dma_release_channel(dma->rxchan);
-	kfree(dma->result_buf);
-}
-
 int devm_qce_dma_request(struct qce_device *qce)
 {
 	struct qce_dma_data *dma = &qce->dma;
 	struct device *dev = qce->dev;
-	int ret;
 
-	dma->txchan = dma_request_chan(dev, "tx");
+	dma->txchan = devm_dma_request_chan(dev, "tx");
 	if (IS_ERR(dma->txchan))
 		return dev_err_probe(dev, PTR_ERR(dma->txchan),
 				     "Failed to get TX DMA channel\n");
 
-	dma->rxchan = dma_request_chan(dev, "rx");
-	if (IS_ERR(dma->rxchan)) {
-		ret = dev_err_probe(dev, PTR_ERR(dma->rxchan),
-				    "Failed to get RX DMA channel\n");
-		goto error_rx;
-	}
+	dma->rxchan = devm_dma_request_chan(dev, "rx");
+	if (IS_ERR(dma->rxchan))
+		return dev_err_probe(dev, PTR_ERR(dma->rxchan),
+				     "Failed to get RX DMA channel\n");
 
-	dma->result_buf = kmalloc(QCE_RESULT_BUF_SZ + QCE_IGNORE_BUF_SZ,
-				  GFP_KERNEL);
-	if (!dma->result_buf) {
-		ret = -ENOMEM;
-		goto error_nomem;
-	}
+	dma->result_buf = devm_kmalloc(dev, QCE_RESULT_BUF_SZ + QCE_IGNORE_BUF_SZ, GFP_KERNEL);
+	if (!dma->result_buf)
+		return -ENOMEM;
 
-	return devm_add_action_or_reset(dev, qce_dma_release, dma);
-
-error_nomem:
-	dma_release_channel(dma->rxchan);
-error_rx:
-	dma_release_channel(dma->txchan);
-	return ret;
+	return 0;
 }
 
 struct scatterlist *

-- 
2.51.0

[PATCH v9 08/11] crypto: qce - Map crypto memory for DMA

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

As the first step in converting the driver to using DMA for register
I/O, let's map the crypto memory range.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/crypto/qce/core.c | 25 +++++++++++++++++++++++--
 drivers/crypto/qce/core.h |  6 ++++++
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
index 8b7bcd0c420c45caf8b29e5455e0f384fd5c5616..2667fcd67fee826a44080da8f88a3e2abbb9b2cf 100644
--- a/drivers/crypto/qce/core.c
+++ b/drivers/crypto/qce/core.c
@@ -185,10 +185,19 @@ static int qce_check_version(struct qce_device *qce)
 	return 0;
 }
 
+static void qce_crypto_unmap_dma(void *data)
+{
+	struct qce_device *qce = data;
+
+	dma_unmap_resource(qce->dev, qce->base_dma, qce->dma_size,
+			   DMA_BIDIRECTIONAL, 0);
+}
+
 static int qce_crypto_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct qce_device *qce;
+	struct resource *res;
 	int ret;
 
 	qce = devm_kzalloc(dev, sizeof(*qce), GFP_KERNEL);
@@ -198,7 +207,7 @@ static int qce_crypto_probe(struct platform_device *pdev)
 	qce->dev = dev;
 	platform_set_drvdata(pdev, qce);
 
-	qce->base = devm_platform_ioremap_resource(pdev, 0);
+	qce->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
 	if (IS_ERR(qce->base))
 		return PTR_ERR(qce->base);
 
@@ -244,7 +253,19 @@ static int qce_crypto_probe(struct platform_device *pdev)
 	qce->async_req_enqueue = qce_async_request_enqueue;
 	qce->async_req_done = qce_async_request_done;
 
-	return devm_qce_register_algs(qce);
+	ret = devm_qce_register_algs(qce);
+	if (ret)
+		return ret;
+
+	qce->dma_size = resource_size(res);
+	qce->base_dma = dma_map_resource(dev, res->start, qce->dma_size,
+					 DMA_BIDIRECTIONAL, 0);
+	qce->base_phys = res->start;
+	ret = dma_mapping_error(dev, qce->base_dma);
+	if (ret)
+		return ret;
+
+	return devm_add_action_or_reset(qce->dev, qce_crypto_unmap_dma, qce);
 }
 
 static const struct of_device_id qce_crypto_of_match[] = {
diff --git a/drivers/crypto/qce/core.h b/drivers/crypto/qce/core.h
index f092ce2d3b04a936a37805c20ac5ba78d8fdd2df..a80e12eac6c87e5321cce16c56a4bf5003474ef0 100644
--- a/drivers/crypto/qce/core.h
+++ b/drivers/crypto/qce/core.h
@@ -27,6 +27,9 @@
  * @dma: pointer to dma data
  * @burst_size: the crypto burst size
  * @pipe_pair_id: which pipe pair id the device using
+ * @base_dma: base DMA address
+ * @base_phys: base physical address
+ * @dma_size: size of memory mapped for DMA
  * @async_req_enqueue: invoked by every algorithm to enqueue a request
  * @async_req_done: invoked by every algorithm to finish its request
  */
@@ -43,6 +46,9 @@ struct qce_device {
 	struct qce_dma_data dma;
 	int burst_size;
 	unsigned int pipe_pair_id;
+	dma_addr_t base_dma;
+	phys_addr_t base_phys;
+	size_t dma_size;
 	int (*async_req_enqueue)(struct qce_device *qce,
 				 struct crypto_async_request *req);
 	void (*async_req_done)(struct qce_device *qce, int ret);

-- 
2.51.0

[PATCH v9 09/11] crypto: qce - Add BAM DMA support for crypto register I/O

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Implement the infrastructure for performing register I/O over BAM DMA,
not CPU. No functional change yet.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/crypto/qce/core.h |   4 ++
 drivers/crypto/qce/dma.c  | 109 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/crypto/qce/dma.h  |   5 +++
 3 files changed, 118 insertions(+)

diff --git a/drivers/crypto/qce/core.h b/drivers/crypto/qce/core.h
index a80e12eac6c87e5321cce16c56a4bf5003474ef0..d238097f834e4605f3825f23d0316d4196439116 100644
--- a/drivers/crypto/qce/core.h
+++ b/drivers/crypto/qce/core.h
@@ -30,6 +30,8 @@
  * @base_dma: base DMA address
  * @base_phys: base physical address
  * @dma_size: size of memory mapped for DMA
+ * @read_buf: Buffer for DMA to write back to
+ * @read_buf_dma: Mapped address of the read buffer
  * @async_req_enqueue: invoked by every algorithm to enqueue a request
  * @async_req_done: invoked by every algorithm to finish its request
  */
@@ -49,6 +51,8 @@ struct qce_device {
 	dma_addr_t base_dma;
 	phys_addr_t base_phys;
 	size_t dma_size;
+	__le32 *read_buf;
+	dma_addr_t read_buf_dma;
 	int (*async_req_enqueue)(struct qce_device *qce,
 				 struct crypto_async_request *req);
 	void (*async_req_done)(struct qce_device *qce, int ret);
diff --git a/drivers/crypto/qce/dma.c b/drivers/crypto/qce/dma.c
index a46264735bb895b6199969e83391383ccbbacc5f..ba7a52fd4c6349d59c075c346f75741defeb6034 100644
--- a/drivers/crypto/qce/dma.c
+++ b/drivers/crypto/qce/dma.c
@@ -4,6 +4,8 @@
  */
 
 #include <linux/device.h>
+#include <linux/dma/qcom_bam_dma.h>
+#include <linux/dma-mapping.h>
 #include <linux/dmaengine.h>
 #include <crypto/scatterwalk.h>
 
@@ -11,6 +13,98 @@
 #include "dma.h"
 
 #define QCE_IGNORE_BUF_SZ		(2 * QCE_BAM_BURST_SIZE)
+#define QCE_BAM_CMD_SGL_SIZE		128
+#define QCE_BAM_CMD_ELEMENT_SIZE	128
+#define QCE_MAX_REG_READ		8
+
+struct qce_desc_info {
+	struct dma_async_tx_descriptor *dma_desc;
+	enum dma_data_direction dir;
+};
+
+struct qce_bam_transaction {
+	struct bam_cmd_element bam_ce[QCE_BAM_CMD_ELEMENT_SIZE];
+	struct scatterlist wr_sgl[QCE_BAM_CMD_SGL_SIZE];
+	struct qce_desc_info *desc;
+	u32 bam_ce_idx;
+	u32 pre_bam_ce_idx;
+	u32 wr_sgl_cnt;
+};
+
+void qce_clear_bam_transaction(struct qce_device *qce)
+{
+	struct qce_bam_transaction *bam_txn = qce->dma.bam_txn;
+
+	bam_txn->bam_ce_idx = 0;
+	bam_txn->wr_sgl_cnt = 0;
+	bam_txn->bam_ce_idx = 0;
+	bam_txn->pre_bam_ce_idx = 0;
+}
+
+int qce_submit_cmd_desc(struct qce_device *qce)
+{
+	struct qce_desc_info *qce_desc = qce->dma.bam_txn->desc;
+	struct qce_bam_transaction *bam_txn = qce->dma.bam_txn;
+	struct dma_async_tx_descriptor *dma_desc;
+	struct dma_chan *chan = qce->dma.rxchan;
+	unsigned long attrs = DMA_PREP_CMD;
+	dma_cookie_t cookie;
+	unsigned int mapped;
+	int ret;
+
+	mapped = dma_map_sg_attrs(qce->dev, bam_txn->wr_sgl, bam_txn->wr_sgl_cnt,
+				  DMA_TO_DEVICE, attrs);
+	if (!mapped)
+		return -ENOMEM;
+
+	dma_desc = dmaengine_prep_slave_sg(chan, bam_txn->wr_sgl, bam_txn->wr_sgl_cnt,
+					   DMA_MEM_TO_DEV, attrs);
+	if (!dma_desc) {
+		dma_unmap_sg(qce->dev, bam_txn->wr_sgl, bam_txn->wr_sgl_cnt, DMA_TO_DEVICE);
+		return -ENOMEM;
+	}
+
+	qce_desc->dma_desc = dma_desc;
+	cookie = dmaengine_submit(qce_desc->dma_desc);
+
+	ret = dma_submit_error(cookie);
+	if (ret)
+		return ret;
+
+	qce_dma_issue_pending(&qce->dma);
+
+	return 0;
+}
+
+static void qce_prep_dma_cmd_desc(struct qce_device *qce, struct qce_dma_data *dma,
+				  unsigned int addr, void *buf)
+{
+	struct qce_bam_transaction *bam_txn = dma->bam_txn;
+	struct bam_cmd_element *bam_ce_buf;
+	int bam_ce_size, cnt, idx;
+
+	idx = bam_txn->bam_ce_idx;
+	bam_ce_buf = &bam_txn->bam_ce[idx];
+	bam_prep_ce_le32(bam_ce_buf, addr, BAM_WRITE_COMMAND, *((__le32 *)buf));
+
+	bam_ce_buf = &bam_txn->bam_ce[bam_txn->pre_bam_ce_idx];
+	bam_txn->bam_ce_idx++;
+	bam_ce_size = (bam_txn->bam_ce_idx - bam_txn->pre_bam_ce_idx) * sizeof(*bam_ce_buf);
+
+	cnt = bam_txn->wr_sgl_cnt;
+
+	sg_set_buf(&bam_txn->wr_sgl[cnt], bam_ce_buf, bam_ce_size);
+
+	++bam_txn->wr_sgl_cnt;
+	bam_txn->pre_bam_ce_idx = bam_txn->bam_ce_idx;
+}
+
+void qce_write_dma(struct qce_device *qce, unsigned int offset, u32 val)
+{
+	unsigned int reg_addr = ((unsigned int)(qce->base_phys) + offset);
+
+	qce_prep_dma_cmd_desc(qce, &qce->dma, reg_addr, &val);
+}
 
 int devm_qce_dma_request(struct qce_device *qce)
 {
@@ -31,6 +125,21 @@ int devm_qce_dma_request(struct qce_device *qce)
 	if (!dma->result_buf)
 		return -ENOMEM;
 
+	dma->bam_txn = devm_kzalloc(dev, sizeof(*dma->bam_txn), GFP_KERNEL);
+	if (!dma->bam_txn)
+		return -ENOMEM;
+
+	dma->bam_txn->desc = devm_kzalloc(dev, sizeof(*dma->bam_txn->desc), GFP_KERNEL);
+	if (!dma->bam_txn->desc)
+		return -ENOMEM;
+
+	sg_init_table(dma->bam_txn->wr_sgl, QCE_BAM_CMD_SGL_SIZE);
+
+	qce->read_buf = dmam_alloc_coherent(qce->dev, QCE_MAX_REG_READ * sizeof(*qce->read_buf),
+					    &qce->read_buf_dma, GFP_KERNEL);
+	if (!qce->read_buf)
+		return -ENOMEM;
+
 	return 0;
 }
 
diff --git a/drivers/crypto/qce/dma.h b/drivers/crypto/qce/dma.h
index 483789d9fa98e79d1283de8297bf2fc2a773f3a7..f05dfa9e6b25bd60e32f45079a8bc7e6a4cf81f9 100644
--- a/drivers/crypto/qce/dma.h
+++ b/drivers/crypto/qce/dma.h
@@ -8,6 +8,7 @@
 
 #include <linux/dmaengine.h>
 
+struct qce_bam_transaction;
 struct qce_device;
 
 /* maximum data transfer block size between BAM and CE */
@@ -32,6 +33,7 @@ struct qce_dma_data {
 	struct dma_chan *txchan;
 	struct dma_chan *rxchan;
 	struct qce_result_dump *result_buf;
+	struct qce_bam_transaction *bam_txn;
 };
 
 int devm_qce_dma_request(struct qce_device *qce);
@@ -43,5 +45,8 @@ int qce_dma_terminate_all(struct qce_dma_data *dma);
 struct scatterlist *
 qce_sgtable_add(struct sg_table *sgt, struct scatterlist *sg_add,
 		unsigned int max_len);
+void qce_write_dma(struct qce_device *qce, unsigned int offset, u32 val);
+int qce_submit_cmd_desc(struct qce_device *qce);
+void qce_clear_bam_transaction(struct qce_device *qce);
 
 #endif /* _DMA_H_ */

-- 
2.51.0

[PATCH v9 10/11] crypto: qce - Add support for BAM locking

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Implement the infrastructure for using the new DMA controller lock/unlock
feature of the BAM driver. No functional change for now.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/crypto/qce/common.c | 18 ++++++++++++++++++
 drivers/crypto/qce/dma.c    | 39 ++++++++++++++++++++++++++++++++++-----
 drivers/crypto/qce/dma.h    |  4 ++++
 3 files changed, 56 insertions(+), 5 deletions(-)

diff --git a/drivers/crypto/qce/common.c b/drivers/crypto/qce/common.c
index 04253a8d33409a2a51db527435d09ae85a7880af..74756c222fed6d0298eb6c957ed15b8b7083b72f 100644
--- a/drivers/crypto/qce/common.c
+++ b/drivers/crypto/qce/common.c
@@ -593,3 +593,21 @@ void qce_get_version(struct qce_device *qce, u32 *major, u32 *minor, u32 *step)
 	*minor = (val & CORE_MINOR_REV_MASK) >> CORE_MINOR_REV_SHIFT;
 	*step = (val & CORE_STEP_REV_MASK) >> CORE_STEP_REV_SHIFT;
 }
+
+int qce_bam_lock(struct qce_device *qce)
+{
+	qce_clear_bam_transaction(qce);
+	/* Dummy write to acquire the lock on the BAM pipe. */
+	qce_write(qce, REG_AUTH_SEG_CFG, 0);
+
+	return qce_submit_cmd_desc_lock(qce);
+}
+
+int qce_bam_unlock(struct qce_device *qce)
+{
+	qce_clear_bam_transaction(qce);
+	/* Dummy write to release the lock on the BAM pipe. */
+	qce_write(qce, REG_AUTH_SEG_CFG, 0);
+
+	return qce_submit_cmd_desc_unlock(qce);
+}
diff --git a/drivers/crypto/qce/dma.c b/drivers/crypto/qce/dma.c
index ba7a52fd4c6349d59c075c346f75741defeb6034..885053955ac3dc95efefef541907f57844b60a3d 100644
--- a/drivers/crypto/qce/dma.c
+++ b/drivers/crypto/qce/dma.c
@@ -41,7 +41,7 @@ void qce_clear_bam_transaction(struct qce_device *qce)
 	bam_txn->pre_bam_ce_idx = 0;
 }
 
-int qce_submit_cmd_desc(struct qce_device *qce)
+static int qce_do_submit_cmd_desc(struct qce_device *qce, struct bam_desc_metadata *meta)
 {
 	struct qce_desc_info *qce_desc = qce->dma.bam_txn->desc;
 	struct qce_bam_transaction *bam_txn = qce->dma.bam_txn;
@@ -50,7 +50,7 @@ int qce_submit_cmd_desc(struct qce_device *qce)
 	unsigned long attrs = DMA_PREP_CMD;
 	dma_cookie_t cookie;
 	unsigned int mapped;
-	int ret;
+	int ret = -ENOMEM;
 
 	mapped = dma_map_sg_attrs(qce->dev, bam_txn->wr_sgl, bam_txn->wr_sgl_cnt,
 				  DMA_TO_DEVICE, attrs);
@@ -59,9 +59,15 @@ int qce_submit_cmd_desc(struct qce_device *qce)
 
 	dma_desc = dmaengine_prep_slave_sg(chan, bam_txn->wr_sgl, bam_txn->wr_sgl_cnt,
 					   DMA_MEM_TO_DEV, attrs);
-	if (!dma_desc) {
-		dma_unmap_sg(qce->dev, bam_txn->wr_sgl, bam_txn->wr_sgl_cnt, DMA_TO_DEVICE);
-		return -ENOMEM;
+	if (!dma_desc)
+		goto err_out;
+
+	if (meta) {
+		meta->chan = chan;
+
+		ret = dmaengine_desc_attach_metadata(dma_desc, meta, 0);
+		if (ret)
+			goto err_out;
 	}
 
 	qce_desc->dma_desc = dma_desc;
@@ -74,6 +80,29 @@ int qce_submit_cmd_desc(struct qce_device *qce)
 	qce_dma_issue_pending(&qce->dma);
 
 	return 0;
+
+err_out:
+	dma_unmap_sg(qce->dev, bam_txn->wr_sgl, bam_txn->wr_sgl_cnt, DMA_TO_DEVICE);
+	return ret;
+}
+
+int qce_submit_cmd_desc(struct qce_device *qce)
+{
+	return qce_do_submit_cmd_desc(qce, NULL);
+}
+
+int qce_submit_cmd_desc_lock(struct qce_device *qce)
+{
+	struct bam_desc_metadata meta = { .op = BAM_META_CMD_LOCK, };
+
+	return qce_do_submit_cmd_desc(qce, &meta);
+}
+
+int qce_submit_cmd_desc_unlock(struct qce_device *qce)
+{
+	struct bam_desc_metadata meta = { .op = BAM_META_CMD_UNLOCK };
+
+	return qce_do_submit_cmd_desc(qce, &meta);
 }
 
 static void qce_prep_dma_cmd_desc(struct qce_device *qce, struct qce_dma_data *dma,
diff --git a/drivers/crypto/qce/dma.h b/drivers/crypto/qce/dma.h
index f05dfa9e6b25bd60e32f45079a8bc7e6a4cf81f9..4b3ee17db72e29b9f417994477ad8a0ec2294db1 100644
--- a/drivers/crypto/qce/dma.h
+++ b/drivers/crypto/qce/dma.h
@@ -47,6 +47,10 @@ qce_sgtable_add(struct sg_table *sgt, struct scatterlist *sg_add,
 		unsigned int max_len);
 void qce_write_dma(struct qce_device *qce, unsigned int offset, u32 val);
 int qce_submit_cmd_desc(struct qce_device *qce);
+int qce_submit_cmd_desc_lock(struct qce_device *qce);
+int qce_submit_cmd_desc_unlock(struct qce_device *qce);
 void qce_clear_bam_transaction(struct qce_device *qce);
+int qce_bam_lock(struct qce_device *qce);
+int qce_bam_unlock(struct qce_device *qce);
 
 #endif /* _DMA_H_ */

-- 
2.51.0

[PATCH v9 11/11] crypto: qce - Switch to using BAM DMA for crypto I/O

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

With everything else in place, we can now switch to actually using the
BAM DMA for register I/O with DMA engine locking.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/crypto/qce/aead.c     | 10 ++++++++++
 drivers/crypto/qce/common.c   | 21 ++++++++++-----------
 drivers/crypto/qce/sha.c      |  8 ++++++++
 drivers/crypto/qce/skcipher.c |  7 +++++++
 4 files changed, 35 insertions(+), 11 deletions(-)

diff --git a/drivers/crypto/qce/aead.c b/drivers/crypto/qce/aead.c
index 11cec08544c912e562bf4b33d9a409f0e69a0ada..0fc69b019929342e14d3af8e24d7629ab171bc60 100644
--- a/drivers/crypto/qce/aead.c
+++ b/drivers/crypto/qce/aead.c
@@ -63,6 +63,10 @@ static void qce_aead_done(void *data)
 		sg_free_table(&rctx->dst_tbl);
 	}
 
+	error = qce_bam_unlock(qce);
+	if (error)
+		dev_err(qce->dev, "aead: failed to unlock the BAM\n");
+
 	error = qce_check_status(qce, &status);
 	if (error < 0 && (error != -EBADMSG))
 		dev_err(qce->dev, "aead operation error (%x)\n", status);
@@ -188,6 +192,8 @@ qce_aead_ccm_prepare_buf_assoclen(struct aead_request *req)
 	struct crypto_aead *tfm = crypto_aead_reqtfm(req);
 	struct qce_aead_reqctx *rctx = aead_request_ctx_dma(req);
 	struct qce_aead_ctx *ctx = crypto_aead_ctx(tfm);
+	struct qce_alg_template *tmpl = to_aead_tmpl(crypto_aead_reqtfm(req));
+	struct qce_device *qce = tmpl->qce;
 	unsigned int assoclen = rctx->assoclen;
 	unsigned int adata_header_len, cryptlen, totallen;
 	gfp_t gfp;
@@ -200,6 +206,10 @@ qce_aead_ccm_prepare_buf_assoclen(struct aead_request *req)
 		cryptlen = rctx->cryptlen;
 	totallen = cryptlen + req->assoclen;
 
+	ret = qce_bam_lock(qce);
+	if (ret)
+		return ret;
+
 	/* Get the msg */
 	msg_sg = scatterwalk_ffwd(__sg, req->src, req->assoclen);
 
diff --git a/drivers/crypto/qce/common.c b/drivers/crypto/qce/common.c
index 74756c222fed6d0298eb6c957ed15b8b7083b72f..930006aaba4accb51576ecfb84aa9cf20849a72f 100644
--- a/drivers/crypto/qce/common.c
+++ b/drivers/crypto/qce/common.c
@@ -14,6 +14,7 @@
 #include "cipher.h"
 #include "common.h"
 #include "core.h"
+#include "dma.h"
 #include "regs-v5.h"
 #include "sha.h"
 #include "aead.h"
@@ -25,7 +26,7 @@ static inline u32 qce_read(struct qce_device *qce, u32 offset)
 
 static inline void qce_write(struct qce_device *qce, u32 offset, u32 val)
 {
-	writel(val, qce->base + offset);
+	qce_write_dma(qce, offset, val);
 }
 
 static inline void qce_write_array(struct qce_device *qce, u32 offset,
@@ -82,6 +83,8 @@ static void qce_setup_config(struct qce_device *qce)
 {
 	u32 config;
 
+	qce_clear_bam_transaction(qce);
+
 	/* get big endianness */
 	config = qce_config_reg(qce, 0);
 
@@ -90,12 +93,14 @@ static void qce_setup_config(struct qce_device *qce)
 	qce_write(qce, REG_CONFIG, config);
 }
 
-static inline void qce_crypto_go(struct qce_device *qce, bool result_dump)
+static int qce_crypto_go(struct qce_device *qce, bool result_dump)
 {
 	if (result_dump)
 		qce_write(qce, REG_GOPROC, BIT(GO_SHIFT) | BIT(RESULTS_DUMP_SHIFT));
 	else
 		qce_write(qce, REG_GOPROC, BIT(GO_SHIFT));
+
+	return qce_submit_cmd_desc(qce);
 }
 
 #if defined(CONFIG_CRYPTO_DEV_QCE_SHA) || defined(CONFIG_CRYPTO_DEV_QCE_AEAD)
@@ -223,9 +228,7 @@ static int qce_setup_regs_ahash(struct crypto_async_request *async_req)
 	config = qce_config_reg(qce, 1);
 	qce_write(qce, REG_CONFIG, config);
 
-	qce_crypto_go(qce, true);
-
-	return 0;
+	return qce_crypto_go(qce, true);
 }
 #endif
 
@@ -386,9 +389,7 @@ static int qce_setup_regs_skcipher(struct crypto_async_request *async_req)
 	config = qce_config_reg(qce, 1);
 	qce_write(qce, REG_CONFIG, config);
 
-	qce_crypto_go(qce, true);
-
-	return 0;
+	return qce_crypto_go(qce, true);
 }
 #endif
 
@@ -535,9 +536,7 @@ static int qce_setup_regs_aead(struct crypto_async_request *async_req)
 	qce_write(qce, REG_CONFIG, config);
 
 	/* Start the process */
-	qce_crypto_go(qce, !IS_CCM(flags));
-
-	return 0;
+	return qce_crypto_go(qce, !IS_CCM(flags));
 }
 #endif
 
diff --git a/drivers/crypto/qce/sha.c b/drivers/crypto/qce/sha.c
index 0c7aab711b7b8434d5f89ab4565ef4123fc5322e..286477a3001248e745d79b209aebb6ed6bf11f62 100644
--- a/drivers/crypto/qce/sha.c
+++ b/drivers/crypto/qce/sha.c
@@ -60,6 +60,10 @@ static void qce_ahash_done(void *data)
 	rctx->byte_count[0] = cpu_to_be32(result->auth_byte_count[0]);
 	rctx->byte_count[1] = cpu_to_be32(result->auth_byte_count[1]);
 
+	error = qce_bam_unlock(qce);
+	if (error)
+		dev_err(qce->dev, "ahash: failed to unlock the BAM\n");
+
 	error = qce_check_status(qce, &status);
 	if (error < 0)
 		dev_dbg(qce->dev, "ahash operation error (%x)\n", status);
@@ -90,6 +94,10 @@ static int qce_ahash_async_req_handle(struct crypto_async_request *async_req)
 		rctx->authklen = AES_KEYSIZE_128;
 	}
 
+	ret = qce_bam_lock(qce);
+	if (ret)
+		return ret;
+
 	rctx->src_nents = sg_nents_for_len(req->src, req->nbytes);
 	if (rctx->src_nents < 0) {
 		dev_err(qce->dev, "Invalid numbers of src SG.\n");
diff --git a/drivers/crypto/qce/skcipher.c b/drivers/crypto/qce/skcipher.c
index cab796cd7e43c548a49df468b2dde84942c5bd87..8317c79fb9c2b209884187d65655d04c580e9cde 100644
--- a/drivers/crypto/qce/skcipher.c
+++ b/drivers/crypto/qce/skcipher.c
@@ -51,6 +51,9 @@ static void qce_skcipher_done(void *data)
 	dma_unmap_sg(qce->dev, rctx->dst_sg, rctx->dst_nents, dir_dst);
 
 	sg_free_table(&rctx->dst_tbl);
+	error = qce_bam_unlock(qce);
+	if (error)
+		dev_err(qce->dev, "skcipher: failed to unlock the BAM\n");
 
 	error = qce_check_status(qce, &status);
 	if (error < 0)
@@ -78,6 +81,10 @@ qce_skcipher_async_req_handle(struct crypto_async_request *async_req)
 	rctx->ivsize = crypto_skcipher_ivsize(skcipher);
 	rctx->cryptlen = req->cryptlen;
 
+	ret = qce_bam_lock(qce);
+	if (ret)
+		return ret;
+
 	diff_dst = (req->src != req->dst) ? true : false;
 	dir_src = diff_dst ? DMA_TO_DEVICE : DMA_BIDIRECTIONAL;
 	dir_dst = diff_dst ? DMA_FROM_DEVICE : DMA_BIDIRECTIONAL;

-- 
2.51.0

Re: [PATCH v9 05/11] crypto: qce - Remove unused ignore_buf

[Konrad Dybcio]

On 11/28/25 12:44 PM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> It's unclear what the purpose of this field is. It has been here since
> the initial commit but without any explanation. The driver works fine
> without it. We still keep allocating more space in the result buffer, we
> just don't need to store its address. While at it: move the
> QCE_IGNORE_BUF_SZ definition into dma.c as it's not used outside of this
> compilation unit.

It's apparently used downstream, at a glance to work around some oddities

https://github.com/cupid-development/android_kernel_xiaomi_sm8450/blob/lineage-22.2/drivers/crypto/msm/qce50.c

Konrad

Re: [PATCH v9 07/11] crypto: qce - Use existing devres APIs in devm_qce_dma_request()

[Konrad Dybcio]

On 11/28/25 12:44 PM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Switch to devm_kmalloc() and devm_dma_alloc_chan() in
> devm_qce_dma_request(). This allows us to drop two labels and shrink the
> function.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad

Re: [PATCH v9 05/11] crypto: qce - Remove unused ignore_buf

[Bartosz Golaszewski]

On Fri, Nov 28, 2025 at 1:02 PM Konrad Dybcio
<konrad.dybcio@oss.qualcomm.com> wrote:
>
> On 11/28/25 12:44 PM, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > It's unclear what the purpose of this field is. It has been here since
> > the initial commit but without any explanation. The driver works fine
> > without it. We still keep allocating more space in the result buffer, we
> > just don't need to store its address. While at it: move the
> > QCE_IGNORE_BUF_SZ definition into dma.c as it's not used outside of this
> > compilation unit.
>
> It's apparently used downstream, at a glance to work around some oddities
>
> https://github.com/cupid-development/android_kernel_xiaomi_sm8450/blob/lineage-22.2/drivers/crypto/msm/qce50.c
>

Thanks. This driver is very far from upstream. :)

I think it's still safe to remove it.

Bart

Re: [PATCH v9 11/11] crypto: qce - Switch to using BAM DMA for crypto I/O

[Konrad Dybcio]

On 11/28/25 12:44 PM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> With everything else in place, we can now switch to actually using the
> BAM DMA for register I/O with DMA engine locking.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---

[...]

> @@ -25,7 +26,7 @@ static inline u32 qce_read(struct qce_device *qce, u32 offset)
>  
>  static inline void qce_write(struct qce_device *qce, u32 offset, u32 val)
>  {
> -	writel(val, qce->base + offset);
> +	qce_write_dma(qce, offset, val);
>  }

qce_write() seems no longer useful now

Konrad

Re: [PATCH v9 05/11] crypto: qce - Remove unused ignore_buf

[Konrad Dybcio]

On 11/28/25 1:05 PM, Bartosz Golaszewski wrote:
> On Fri, Nov 28, 2025 at 1:02 PM Konrad Dybcio
> <konrad.dybcio@oss.qualcomm.com> wrote:
>>
>> On 11/28/25 12:44 PM, Bartosz Golaszewski wrote:
>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>
>>> It's unclear what the purpose of this field is. It has been here since
>>> the initial commit but without any explanation. The driver works fine
>>> without it. We still keep allocating more space in the result buffer, we
>>> just don't need to store its address. While at it: move the
>>> QCE_IGNORE_BUF_SZ definition into dma.c as it's not used outside of this
>>> compilation unit.
>>
>> It's apparently used downstream, at a glance to work around some oddities
>>
>> https://github.com/cupid-development/android_kernel_xiaomi_sm8450/blob/lineage-22.2/drivers/crypto/msm/qce50.c
>>
> 
> Thanks. This driver is very far from upstream. :)
> 
> I think it's still safe to remove it.

Seems so!

Konrad

Re: [PATCH v9 11/11] crypto: qce - Switch to using BAM DMA for crypto I/O

[Bartosz Golaszewski]

On Fri, Nov 28, 2025 at 1:08 PM Konrad Dybcio
<konrad.dybcio@oss.qualcomm.com> wrote:
>
> On 11/28/25 12:44 PM, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > With everything else in place, we can now switch to actually using the
> > BAM DMA for register I/O with DMA engine locking.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > ---
>
> [...]
>
> > @@ -25,7 +26,7 @@ static inline u32 qce_read(struct qce_device *qce, u32 offset)
> >
> >  static inline void qce_write(struct qce_device *qce, u32 offset, u32 val)
> >  {
> > -     writel(val, qce->base + offset);
> > +     qce_write_dma(qce, offset, val);
> >  }
>
> qce_write() seems no longer useful now
>

I prefer to leave it like this if there are no strong objections. It
reduces the size of the final patch and also - if for any reason in
the future - we need to go back to supporting both DMA and CPU, we
could handle it here.

Bart

Re: [PATCH v9 11/11] crypto: qce - Switch to using BAM DMA for crypto I/O

[Konrad Dybcio]

On 11/28/25 1:11 PM, Bartosz Golaszewski wrote:
> On Fri, Nov 28, 2025 at 1:08 PM Konrad Dybcio
> <konrad.dybcio@oss.qualcomm.com> wrote:
>>
>> On 11/28/25 12:44 PM, Bartosz Golaszewski wrote:
>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>
>>> With everything else in place, we can now switch to actually using the
>>> BAM DMA for register I/O with DMA engine locking.
>>>
>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>> ---
>>
>> [...]
>>
>>> @@ -25,7 +26,7 @@ static inline u32 qce_read(struct qce_device *qce, u32 offset)
>>>
>>>  static inline void qce_write(struct qce_device *qce, u32 offset, u32 val)
>>>  {
>>> -     writel(val, qce->base + offset);
>>> +     qce_write_dma(qce, offset, val);
>>>  }
>>
>> qce_write() seems no longer useful now
>>
> 
> I prefer to leave it like this if there are no strong objections. It
> reduces the size of the final patch and also - if for any reason in
> the future - we need to go back to supporting both DMA and CPU, we
> could handle it here.

alright

Konrad

Re: [PATCH v9 10/11] crypto: qce - Add support for BAM locking

[Konrad Dybcio]

On 11/28/25 12:44 PM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Implement the infrastructure for using the new DMA controller lock/unlock
> feature of the BAM driver. No functional change for now.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  drivers/crypto/qce/common.c | 18 ++++++++++++++++++
>  drivers/crypto/qce/dma.c    | 39 ++++++++++++++++++++++++++++++++++-----
>  drivers/crypto/qce/dma.h    |  4 ++++
>  3 files changed, 56 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/crypto/qce/common.c b/drivers/crypto/qce/common.c
> index 04253a8d33409a2a51db527435d09ae85a7880af..74756c222fed6d0298eb6c957ed15b8b7083b72f 100644
> --- a/drivers/crypto/qce/common.c
> +++ b/drivers/crypto/qce/common.c
> @@ -593,3 +593,21 @@ void qce_get_version(struct qce_device *qce, u32 *major, u32 *minor, u32 *step)
>  	*minor = (val & CORE_MINOR_REV_MASK) >> CORE_MINOR_REV_SHIFT;
>  	*step = (val & CORE_STEP_REV_MASK) >> CORE_STEP_REV_SHIFT;
>  }
> +
> +int qce_bam_lock(struct qce_device *qce)
> +{
> +	qce_clear_bam_transaction(qce);
> +	/* Dummy write to acquire the lock on the BAM pipe. */
> +	qce_write(qce, REG_AUTH_SEG_CFG, 0);

This works because qce_bam_lock() isn't used in a place where the state
of this register matters which isn't obvious.. but I'm not sure there's
a much better one to use in its place

Wonder if we could use the VERSION one (base+0x0) - although it's supposed
to be read-only, but at the same time I don't think that matters much for
the BAM engine

Konrad

Re: [PATCH v9 02/11] dmaengine: qcom: bam_dma: Add bam_pipe_lock flag support

[Dmitry Baryshkov]

On Fri, Nov 28, 2025 at 12:44:00PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Extend the device match data with a flag indicating whether the IP
> supports the BAM lock/unlock feature. Set it to true on BAM IP versions
> 1.4.0 and above.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  drivers/dma/qcom/bam_dma.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index 8861245314b1d13c1abb78f474fd0749fea52f06..c9ae1fffe44d79c5eb59b8bbf7f147a8fa3aa0bd 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c
> @@ -58,6 +58,8 @@ struct bam_desc_hw {
>  #define DESC_FLAG_EOB BIT(13)
>  #define DESC_FLAG_NWD BIT(12)
>  #define DESC_FLAG_CMD BIT(11)
> +#define DESC_FLAG_LOCK BIT(10)
> +#define DESC_FLAG_UNLOCK BIT(9)

If this patch gets resend, please move these two definitions to the next
patch. Otherwise:

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>


>  
>  struct bam_async_desc {
>  	struct virt_dma_desc vd;
> @@ -113,6 +115,7 @@ struct reg_offset_data {
>  
>  struct bam_device_data {
>  	const struct reg_offset_data *reg_info;
> +	bool bam_pipe_lock;
>  };
>  
>  static const struct reg_offset_data bam_v1_3_reg_info[] = {
> @@ -179,6 +182,7 @@ static const struct reg_offset_data bam_v1_4_reg_info[] = {
>  
>  static const struct bam_device_data bam_v1_4_data = {
>  	.reg_info = bam_v1_4_reg_info,
> +	.bam_pipe_lock = true,
>  };
>  
>  static const struct reg_offset_data bam_v1_7_reg_info[] = {
> @@ -212,6 +216,7 @@ static const struct reg_offset_data bam_v1_7_reg_info[] = {
>  
>  static const struct bam_device_data bam_v1_7_data = {
>  	.reg_info = bam_v1_7_reg_info,
> +	.bam_pipe_lock = true,
>  };
>  
>  /* BAM CTRL */
> 
> -- 
> 2.51.0
> 

-- 
With best wishes
Dmitry

Re: [PATCH v9 03/11] dmaengine: qcom: bam_dma: implement support for BAM locking

[Dmitry Baryshkov]

On Fri, Nov 28, 2025 at 12:44:01PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Use metadata operations in DMA descriptors to allow BAM users to pass
> additional information to the engine. To that end: define a new
> structure - struct bam_desc_metadata - as a medium and define two new
> commands: for locking and unlocking the BAM respectively. Handle the
> locking in the .attach() callback.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  drivers/dma/qcom/bam_dma.c       | 59 +++++++++++++++++++++++++++++++++++++++-
>  include/linux/dma/qcom_bam_dma.h | 12 ++++++++
>  2 files changed, 70 insertions(+), 1 deletion(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>


-- 
With best wishes
Dmitry

Re: [PATCH v9 11/11] crypto: qce - Switch to using BAM DMA for crypto I/O

[Dmitry Baryshkov]

On Fri, Nov 28, 2025 at 12:44:09PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> With everything else in place, we can now switch to actually using the
> BAM DMA for register I/O with DMA engine locking.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  drivers/crypto/qce/aead.c     | 10 ++++++++++
>  drivers/crypto/qce/common.c   | 21 ++++++++++-----------
>  drivers/crypto/qce/sha.c      |  8 ++++++++
>  drivers/crypto/qce/skcipher.c |  7 +++++++
>  4 files changed, 35 insertions(+), 11 deletions(-)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>


-- 
With best wishes
Dmitry

Re: [PATCH v9 03/11] dmaengine: qcom: bam_dma: implement support for BAM locking

[Vinod Koul]

On 28-11-25, 12:44, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Use metadata operations in DMA descriptors to allow BAM users to pass
> additional information to the engine. To that end: define a new
> structure - struct bam_desc_metadata - as a medium and define two new
> commands: for locking and unlocking the BAM respectively. Handle the
> locking in the .attach() callback.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  drivers/dma/qcom/bam_dma.c       | 59 +++++++++++++++++++++++++++++++++++++++-
>  include/linux/dma/qcom_bam_dma.h | 12 ++++++++
>  2 files changed, 70 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index c9ae1fffe44d79c5eb59b8bbf7f147a8fa3aa0bd..d1dc80b29818897b333cd223ec7306a169cc51fd 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c
> @@ -30,6 +30,7 @@
>  #include <linux/module.h>
>  #include <linux/interrupt.h>
>  #include <linux/dma-mapping.h>
> +#include <linux/dma/qcom_bam_dma.h>
>  #include <linux/scatterlist.h>
>  #include <linux/device.h>
>  #include <linux/platform_device.h>
> @@ -391,6 +392,8 @@ struct bam_chan {
>  	struct list_head desc_list;
>  
>  	struct list_head node;
> +
> +	bool bam_locked;
>  };
>  
>  static inline struct bam_chan *to_bam_chan(struct dma_chan *common)
> @@ -655,6 +658,53 @@ static int bam_slave_config(struct dma_chan *chan,
>  	return 0;
>  }
>  
> +static int bam_metadata_attach(struct dma_async_tx_descriptor *desc, void *data, size_t len)
> +{
> +	struct virt_dma_desc *vd = container_of(desc, struct virt_dma_desc, tx);
> +	struct bam_async_desc *async_desc = container_of(vd, struct bam_async_desc,  vd);
> +	struct bam_desc_hw *hw_desc = async_desc->desc;
> +	struct bam_desc_metadata *metadata = data;
> +	struct bam_chan *bchan = to_bam_chan(metadata->chan);
> +	struct bam_device *bdev = bchan->bdev;
> +
> +	if (!data)
> +		return -EINVAL;
> +
> +	if (metadata->op == BAM_META_CMD_LOCK || metadata->op == BAM_META_CMD_UNLOCK) {
> +		if (!bdev->dev_data->bam_pipe_lock)
> +			return -EOPNOTSUPP;
> +
> +		/* Expecting a dummy write when locking, only one descriptor allowed. */
> +		if (async_desc->num_desc != 1)
> +			return -EINVAL;
> +	}
> +
> +	switch (metadata->op) {
> +	case BAM_META_CMD_LOCK:
> +		if (bchan->bam_locked)
> +			return -EBUSY;
> +
> +		hw_desc->flags |= DESC_FLAG_LOCK;

Why does this flag imply for the hardware.

I do not like the interface designed here. This is overloading. Can we
look at doing something better here.

-- 
~Vinod

Re: [PATCH v9 03/11] dmaengine: qcom: bam_dma: implement support for BAM locking

[Bartosz Golaszewski]

On Tue, Dec 16, 2025 at 2:00 PM Vinod Koul <vkoul@kernel.org> wrote:
>
> On 28-11-25, 12:44, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Use metadata operations in DMA descriptors to allow BAM users to pass
> > additional information to the engine. To that end: define a new
> > structure - struct bam_desc_metadata - as a medium and define two new
> > commands: for locking and unlocking the BAM respectively. Handle the
> > locking in the .attach() callback.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > ---
> >  drivers/dma/qcom/bam_dma.c       | 59 +++++++++++++++++++++++++++++++++++++++-
> >  include/linux/dma/qcom_bam_dma.h | 12 ++++++++
> >  2 files changed, 70 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> > index c9ae1fffe44d79c5eb59b8bbf7f147a8fa3aa0bd..d1dc80b29818897b333cd223ec7306a169cc51fd 100644
> > --- a/drivers/dma/qcom/bam_dma.c
> > +++ b/drivers/dma/qcom/bam_dma.c
> > @@ -30,6 +30,7 @@
> >  #include <linux/module.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/dma-mapping.h>
> > +#include <linux/dma/qcom_bam_dma.h>
> >  #include <linux/scatterlist.h>
> >  #include <linux/device.h>
> >  #include <linux/platform_device.h>
> > @@ -391,6 +392,8 @@ struct bam_chan {
> >       struct list_head desc_list;
> >
> >       struct list_head node;
> > +
> > +     bool bam_locked;
> >  };
> >
> >  static inline struct bam_chan *to_bam_chan(struct dma_chan *common)
> > @@ -655,6 +658,53 @@ static int bam_slave_config(struct dma_chan *chan,
> >       return 0;
> >  }
> >
> > +static int bam_metadata_attach(struct dma_async_tx_descriptor *desc, void *data, size_t len)
> > +{
> > +     struct virt_dma_desc *vd = container_of(desc, struct virt_dma_desc, tx);
> > +     struct bam_async_desc *async_desc = container_of(vd, struct bam_async_desc,  vd);
> > +     struct bam_desc_hw *hw_desc = async_desc->desc;
> > +     struct bam_desc_metadata *metadata = data;
> > +     struct bam_chan *bchan = to_bam_chan(metadata->chan);
> > +     struct bam_device *bdev = bchan->bdev;
> > +
> > +     if (!data)
> > +             return -EINVAL;
> > +
> > +     if (metadata->op == BAM_META_CMD_LOCK || metadata->op == BAM_META_CMD_UNLOCK) {
> > +             if (!bdev->dev_data->bam_pipe_lock)
> > +                     return -EOPNOTSUPP;
> > +
> > +             /* Expecting a dummy write when locking, only one descriptor allowed. */
> > +             if (async_desc->num_desc != 1)
> > +                     return -EINVAL;
> > +     }
> > +
> > +     switch (metadata->op) {
> > +     case BAM_META_CMD_LOCK:
> > +             if (bchan->bam_locked)
> > +                     return -EBUSY;
> > +
> > +             hw_desc->flags |= DESC_FLAG_LOCK;
>
> Why does this flag imply for the hardware.

Please rephrase, I don't get what you mean.

>
> I do not like the interface designed here. This is overloading. Can we
> look at doing something better here.
>

It used to be a generic flag in dmaengine visible for all users.
Dmitry argued that it's too Qualcomm-specific for a generic flag and
suggested using the metadata to hide the communication between the QCE
and BAM drivers. I'm open to other suggestions but it has to be a bit
more specific than "do something better". :)

Bartosz

Re: [PATCH v9 03/11] dmaengine: qcom: bam_dma: implement support for BAM locking

[Vinod Koul]

On 16-12-25, 16:00, Bartosz Golaszewski wrote:
> On Tue, Dec 16, 2025 at 2:00 PM Vinod Koul <vkoul@kernel.org> wrote:
> >
> > On 28-11-25, 12:44, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > Use metadata operations in DMA descriptors to allow BAM users to pass
> > > additional information to the engine. To that end: define a new
> > > structure - struct bam_desc_metadata - as a medium and define two new
> > > commands: for locking and unlocking the BAM respectively. Handle the
> > > locking in the .attach() callback.
> > >
> > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > ---
> > >  drivers/dma/qcom/bam_dma.c       | 59 +++++++++++++++++++++++++++++++++++++++-
> > >  include/linux/dma/qcom_bam_dma.h | 12 ++++++++
> > >  2 files changed, 70 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> > > index c9ae1fffe44d79c5eb59b8bbf7f147a8fa3aa0bd..d1dc80b29818897b333cd223ec7306a169cc51fd 100644
> > > --- a/drivers/dma/qcom/bam_dma.c
> > > +++ b/drivers/dma/qcom/bam_dma.c
> > > @@ -30,6 +30,7 @@
> > >  #include <linux/module.h>
> > >  #include <linux/interrupt.h>
> > >  #include <linux/dma-mapping.h>
> > > +#include <linux/dma/qcom_bam_dma.h>
> > >  #include <linux/scatterlist.h>
> > >  #include <linux/device.h>
> > >  #include <linux/platform_device.h>
> > > @@ -391,6 +392,8 @@ struct bam_chan {
> > >       struct list_head desc_list;
> > >
> > >       struct list_head node;
> > > +
> > > +     bool bam_locked;
> > >  };
> > >
> > >  static inline struct bam_chan *to_bam_chan(struct dma_chan *common)
> > > @@ -655,6 +658,53 @@ static int bam_slave_config(struct dma_chan *chan,
> > >       return 0;
> > >  }
> > >
> > > +static int bam_metadata_attach(struct dma_async_tx_descriptor *desc, void *data, size_t len)
> > > +{
> > > +     struct virt_dma_desc *vd = container_of(desc, struct virt_dma_desc, tx);
> > > +     struct bam_async_desc *async_desc = container_of(vd, struct bam_async_desc,  vd);
> > > +     struct bam_desc_hw *hw_desc = async_desc->desc;
> > > +     struct bam_desc_metadata *metadata = data;
> > > +     struct bam_chan *bchan = to_bam_chan(metadata->chan);
> > > +     struct bam_device *bdev = bchan->bdev;
> > > +
> > > +     if (!data)
> > > +             return -EINVAL;
> > > +
> > > +     if (metadata->op == BAM_META_CMD_LOCK || metadata->op == BAM_META_CMD_UNLOCK) {
> > > +             if (!bdev->dev_data->bam_pipe_lock)
> > > +                     return -EOPNOTSUPP;
> > > +
> > > +             /* Expecting a dummy write when locking, only one descriptor allowed. */
> > > +             if (async_desc->num_desc != 1)
> > > +                     return -EINVAL;
> > > +     }
> > > +
> > > +     switch (metadata->op) {
> > > +     case BAM_META_CMD_LOCK:
> > > +             if (bchan->bam_locked)
> > > +                     return -EBUSY;
> > > +
> > > +             hw_desc->flags |= DESC_FLAG_LOCK;
> >
> > Why does this flag imply for the hardware.

s/Why/What !
> 
> Please rephrase, I don't get what you mean.

I am trying to understand what the flag refers to and why do you need
this.. What is the problem that lock tries to solve

> >
> > I do not like the interface designed here. This is overloading. Can we
> > look at doing something better here.
> >
> 
> It used to be a generic flag in dmaengine visible for all users.
> Dmitry argued that it's too Qualcomm-specific for a generic flag and
> suggested using the metadata to hide the communication between the QCE
> and BAM drivers. I'm open to other suggestions but it has to be a bit
> more specific than "do something better". :)
> 
> Bartosz

-- 
~Vinod

Re: [PATCH v9 03/11] dmaengine: qcom: bam_dma: implement support for BAM locking

[Bartosz Golaszewski]

On Tue, Dec 16, 2025 at 4:11 PM Vinod Koul <vkoul@kernel.org> wrote:
>
> > > > +
> > > > +     switch (metadata->op) {
> > > > +     case BAM_META_CMD_LOCK:
> > > > +             if (bchan->bam_locked)
> > > > +                     return -EBUSY;
> > > > +
> > > > +             hw_desc->flags |= DESC_FLAG_LOCK;
> > >
> > > Why does this flag imply for the hardware.
>
> s/Why/What !
> >
> > Please rephrase, I don't get what you mean.
>
> I am trying to understand what the flag refers to and why do you need
> this.. What is the problem that lock tries to solve
>

In the DRM use-case the TA will use the QCE simultaneously with linux.
It will perform register I/O with DMA using the BAM locking mechanism
for synchronization. Currently linux doesn't use BAM locking and is
using CPU for register I/O so trying to access locked registers will
result in external abort. I'm trying to make the QCE driver use DMA
for register I/O AND use BAM locking. To that end: we need to pass
information about wanting the command descriptor to contain the
LOCK/UNLOCK flag (this is what we set here in the hardware descriptor)
from the QCE driver to the BAM driver. I initially used a global flag.
Dmitry said it's too Qualcomm-specific and to use metadata instead.
This is what I did in this version.

As I said: I'm open to other suggestions but I'm not sure if we have
any other existing options.

What exactly is the problem with using the attach callback?

Bart

Re: [PATCH v9 10/11] crypto: qce - Add support for BAM locking

[Bartosz Golaszewski]

On Mon, Dec 1, 2025 at 2:03 PM Konrad Dybcio
<konrad.dybcio@oss.qualcomm.com> wrote:
>
> On 11/28/25 12:44 PM, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Implement the infrastructure for using the new DMA controller lock/unlock
> > feature of the BAM driver. No functional change for now.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > ---
> >  drivers/crypto/qce/common.c | 18 ++++++++++++++++++
> >  drivers/crypto/qce/dma.c    | 39 ++++++++++++++++++++++++++++++++++-----
> >  drivers/crypto/qce/dma.h    |  4 ++++
> >  3 files changed, 56 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/crypto/qce/common.c b/drivers/crypto/qce/common.c
> > index 04253a8d33409a2a51db527435d09ae85a7880af..74756c222fed6d0298eb6c957ed15b8b7083b72f 100644
> > --- a/drivers/crypto/qce/common.c
> > +++ b/drivers/crypto/qce/common.c
> > @@ -593,3 +593,21 @@ void qce_get_version(struct qce_device *qce, u32 *major, u32 *minor, u32 *step)
> >       *minor = (val & CORE_MINOR_REV_MASK) >> CORE_MINOR_REV_SHIFT;
> >       *step = (val & CORE_STEP_REV_MASK) >> CORE_STEP_REV_SHIFT;
> >  }
> > +
> > +int qce_bam_lock(struct qce_device *qce)
> > +{
> > +     qce_clear_bam_transaction(qce);
> > +     /* Dummy write to acquire the lock on the BAM pipe. */
> > +     qce_write(qce, REG_AUTH_SEG_CFG, 0);
>
> This works because qce_bam_lock() isn't used in a place where the state
> of this register matters which isn't obvious.. but I'm not sure there's
> a much better one to use in its place
>
> Wonder if we could use the VERSION one (base+0x0) - although it's supposed
> to be read-only, but at the same time I don't think that matters much for
> the BAM engine
>

It seems that we can use VERSION as well, so I'll change that in v10.

Bart

Re: [PATCH v9 03/11] dmaengine: qcom: bam_dma: implement support for BAM locking

[Vinod Koul]

On 17-12-25, 15:31, Bartosz Golaszewski wrote:
> On Tue, Dec 16, 2025 at 4:11 PM Vinod Koul <vkoul@kernel.org> wrote:

> >
> > I am trying to understand what the flag refers to and why do you need
> > this.. What is the problem that lock tries to solve
> >
> 
> In the DRM use-case the TA will use the QCE simultaneously with linux.

TA..?

> It will perform register I/O with DMA using the BAM locking mechanism
> for synchronization. Currently linux doesn't use BAM locking and is
> using CPU for register I/O so trying to access locked registers will
> result in external abort. I'm trying to make the QCE driver use DMA
> for register I/O AND use BAM locking. To that end: we need to pass
> information about wanting the command descriptor to contain the
> LOCK/UNLOCK flag (this is what we set here in the hardware descriptor)
> from the QCE driver to the BAM driver. I initially used a global flag.
> Dmitry said it's too Qualcomm-specific and to use metadata instead.
> This is what I did in this version.

Okay, how will client figure out should it set the lock or not? What are
the conditions where the lock is set or not set by client..?

-- 
~Vinod

Re: [PATCH v9 03/11] dmaengine: qcom: bam_dma: implement support for BAM locking

[Bartosz Golaszewski]

On Tue, Dec 23, 2025 at 11:45 AM Vinod Koul <vkoul@kernel.org> wrote:
>
> On 17-12-25, 15:31, Bartosz Golaszewski wrote:
> > On Tue, Dec 16, 2025 at 4:11 PM Vinod Koul <vkoul@kernel.org> wrote:
>
> > >
> > > I am trying to understand what the flag refers to and why do you need
> > > this.. What is the problem that lock tries to solve
> > >
> >
> > In the DRM use-case the TA will use the QCE simultaneously with linux.
>
> TA..?

Trusted Application, the one to which we offload the decryption of the
stream. That's not really relevant though.

>
> > It will perform register I/O with DMA using the BAM locking mechanism
> > for synchronization. Currently linux doesn't use BAM locking and is
> > using CPU for register I/O so trying to access locked registers will
> > result in external abort. I'm trying to make the QCE driver use DMA
> > for register I/O AND use BAM locking. To that end: we need to pass
> > information about wanting the command descriptor to contain the
> > LOCK/UNLOCK flag (this is what we set here in the hardware descriptor)
> > from the QCE driver to the BAM driver. I initially used a global flag.
> > Dmitry said it's too Qualcomm-specific and to use metadata instead.
> > This is what I did in this version.
>
> Okay, how will client figure out should it set the lock or not? What are
> the conditions where the lock is set or not set by client..?
>

I'm not sure what you refer to as "client". The user of the BAM engine
- the crypto driver? If so - we convert it to always lock/unlock
assuming the TA *may* use it and it's better to be safe. Other users
are not affected.

Bartosz

Re: [PATCH v9 03/11] dmaengine: qcom: bam_dma: implement support for BAM locking

[Dmitry Baryshkov]

On Tue, Dec 23, 2025 at 01:35:30PM +0100, Bartosz Golaszewski wrote:
> On Tue, Dec 23, 2025 at 11:45 AM Vinod Koul <vkoul@kernel.org> wrote:
> >
> > On 17-12-25, 15:31, Bartosz Golaszewski wrote:
> > > On Tue, Dec 16, 2025 at 4:11 PM Vinod Koul <vkoul@kernel.org> wrote:
> >
> > > >
> > > > I am trying to understand what the flag refers to and why do you need
> > > > this.. What is the problem that lock tries to solve
> > > >
> > >
> > > In the DRM use-case the TA will use the QCE simultaneously with linux.
> >
> > TA..?
> 
> Trusted Application, the one to which we offload the decryption of the
> stream. That's not really relevant though.
> 
> >
> > > It will perform register I/O with DMA using the BAM locking mechanism
> > > for synchronization. Currently linux doesn't use BAM locking and is
> > > using CPU for register I/O so trying to access locked registers will
> > > result in external abort. I'm trying to make the QCE driver use DMA
> > > for register I/O AND use BAM locking. To that end: we need to pass
> > > information about wanting the command descriptor to contain the
> > > LOCK/UNLOCK flag (this is what we set here in the hardware descriptor)
> > > from the QCE driver to the BAM driver. I initially used a global flag.
> > > Dmitry said it's too Qualcomm-specific and to use metadata instead.
> > > This is what I did in this version.
> >
> > Okay, how will client figure out should it set the lock or not? What are
> > the conditions where the lock is set or not set by client..?
> >
> 
> I'm not sure what you refer to as "client". The user of the BAM engine
> - the crypto driver? If so - we convert it to always lock/unlock
> assuming the TA *may* use it and it's better to be safe. Other users
> are not affected.

Just to confirm, we support QCE since IPQ4019 and MSM8996. Is lock
semantics supported on those platforms?

-- 
With best wishes
Dmitry

Re: [PATCH v9 03/11] dmaengine: qcom: bam_dma: implement support for BAM locking

[Bartosz Golaszewski]

On Tue, Dec 23, 2025 at 9:19 PM Dmitry Baryshkov
<dmitry.baryshkov@oss.qualcomm.com> wrote:
>
> On Tue, Dec 23, 2025 at 01:35:30PM +0100, Bartosz Golaszewski wrote:
> > On Tue, Dec 23, 2025 at 11:45 AM Vinod Koul <vkoul@kernel.org> wrote:
> > >
> > > On 17-12-25, 15:31, Bartosz Golaszewski wrote:
> > > > On Tue, Dec 16, 2025 at 4:11 PM Vinod Koul <vkoul@kernel.org> wrote:
> > >
> > > > >
> > > > > I am trying to understand what the flag refers to and why do you need
> > > > > this.. What is the problem that lock tries to solve
> > > > >
> > > >
> > > > In the DRM use-case the TA will use the QCE simultaneously with linux.
> > >
> > > TA..?
> >
> > Trusted Application, the one to which we offload the decryption of the
> > stream. That's not really relevant though.
> >
> > >
> > > > It will perform register I/O with DMA using the BAM locking mechanism
> > > > for synchronization. Currently linux doesn't use BAM locking and is
> > > > using CPU for register I/O so trying to access locked registers will
> > > > result in external abort. I'm trying to make the QCE driver use DMA
> > > > for register I/O AND use BAM locking. To that end: we need to pass
> > > > information about wanting the command descriptor to contain the
> > > > LOCK/UNLOCK flag (this is what we set here in the hardware descriptor)
> > > > from the QCE driver to the BAM driver. I initially used a global flag.
> > > > Dmitry said it's too Qualcomm-specific and to use metadata instead.
> > > > This is what I did in this version.
> > >
> > > Okay, how will client figure out should it set the lock or not? What are
> > > the conditions where the lock is set or not set by client..?
> > >
> >
> > I'm not sure what you refer to as "client". The user of the BAM engine
> > - the crypto driver? If so - we convert it to always lock/unlock
> > assuming the TA *may* use it and it's better to be safe. Other users
> > are not affected.
>
> Just to confirm, we support QCE since IPQ4019 and MSM8996. Is lock
> semantics supported on those platforms?
>

Yes, locking is supported on BAM since version 1.4. The only user of
this feature right now is the crypto engine and even on IPQ4019 and
MSM8996 the crypto BAM is version 1.7.

Bartosz

Re: [PATCH v9 03/11] dmaengine: qcom: bam_dma: implement support for BAM locking

[Dmitry Baryshkov]

On Wed, Dec 24, 2025 at 09:58:05AM +0100, Bartosz Golaszewski wrote:
> On Tue, Dec 23, 2025 at 9:19 PM Dmitry Baryshkov
> <dmitry.baryshkov@oss.qualcomm.com> wrote:
> >
> > On Tue, Dec 23, 2025 at 01:35:30PM +0100, Bartosz Golaszewski wrote:
> > > On Tue, Dec 23, 2025 at 11:45 AM Vinod Koul <vkoul@kernel.org> wrote:
> > > >
> > > > On 17-12-25, 15:31, Bartosz Golaszewski wrote:
> > > > > On Tue, Dec 16, 2025 at 4:11 PM Vinod Koul <vkoul@kernel.org> wrote:
> > > >
> > > > > >
> > > > > > I am trying to understand what the flag refers to and why do you need
> > > > > > this.. What is the problem that lock tries to solve
> > > > > >
> > > > >
> > > > > In the DRM use-case the TA will use the QCE simultaneously with linux.
> > > >
> > > > TA..?
> > >
> > > Trusted Application, the one to which we offload the decryption of the
> > > stream. That's not really relevant though.
> > >
> > > >
> > > > > It will perform register I/O with DMA using the BAM locking mechanism
> > > > > for synchronization. Currently linux doesn't use BAM locking and is
> > > > > using CPU for register I/O so trying to access locked registers will
> > > > > result in external abort. I'm trying to make the QCE driver use DMA
> > > > > for register I/O AND use BAM locking. To that end: we need to pass
> > > > > information about wanting the command descriptor to contain the
> > > > > LOCK/UNLOCK flag (this is what we set here in the hardware descriptor)
> > > > > from the QCE driver to the BAM driver. I initially used a global flag.
> > > > > Dmitry said it's too Qualcomm-specific and to use metadata instead.
> > > > > This is what I did in this version.
> > > >
> > > > Okay, how will client figure out should it set the lock or not? What are
> > > > the conditions where the lock is set or not set by client..?
> > > >
> > >
> > > I'm not sure what you refer to as "client". The user of the BAM engine
> > > - the crypto driver? If so - we convert it to always lock/unlock
> > > assuming the TA *may* use it and it's better to be safe. Other users
> > > are not affected.
> >
> > Just to confirm, we support QCE since IPQ4019 and MSM8996. Is lock
> > semantics supported on those platforms?
> >
> 
> Yes, locking is supported on BAM since version 1.4. The only user of
> this feature right now is the crypto engine and even on IPQ4019 and
> MSM8996 the crypto BAM is version 1.7.

Ack, thanks for the confirmation.

For the record, BAM 1.3 is APQ8064, MSM8960 and IPQ8064.

> 
> Bartosz

-- 
With best wishes
Dmitry

Re: [PATCH v9 03/11] dmaengine: qcom: bam_dma: implement support for BAM locking

[Vinod Koul]

On 23-12-25, 13:35, Bartosz Golaszewski wrote:
> On Tue, Dec 23, 2025 at 11:45 AM Vinod Koul <vkoul@kernel.org> wrote:
> >
> > On 17-12-25, 15:31, Bartosz Golaszewski wrote:
> > > On Tue, Dec 16, 2025 at 4:11 PM Vinod Koul <vkoul@kernel.org> wrote:
> >
> > > >
> > > > I am trying to understand what the flag refers to and why do you need
> > > > this.. What is the problem that lock tries to solve
> > > >
> > >
> > > In the DRM use-case the TA will use the QCE simultaneously with linux.
> >
> > TA..?
> 
> Trusted Application, the one to which we offload the decryption of the
> stream. That's not really relevant though.
> 
> >
> > > It will perform register I/O with DMA using the BAM locking mechanism
> > > for synchronization. Currently linux doesn't use BAM locking and is
> > > using CPU for register I/O so trying to access locked registers will
> > > result in external abort. I'm trying to make the QCE driver use DMA
> > > for register I/O AND use BAM locking. To that end: we need to pass
> > > information about wanting the command descriptor to contain the
> > > LOCK/UNLOCK flag (this is what we set here in the hardware descriptor)
> > > from the QCE driver to the BAM driver. I initially used a global flag.
> > > Dmitry said it's too Qualcomm-specific and to use metadata instead.
> > > This is what I did in this version.
> >
> > Okay, how will client figure out should it set the lock or not? What are
> > the conditions where the lock is set or not set by client..?
> >
> 
> I'm not sure what you refer to as "client". The user of the BAM engine
> - the crypto driver? If so - we convert it to always lock/unlock
> assuming the TA *may* use it and it's better to be safe. Other users
> are not affected.

Client are users of dmaengine. So how does the crypto driver figure out
when to lock/unlock. Why not do this always...?

-- 
~Vinod

Re: [PATCH v9 03/11] dmaengine: qcom: bam_dma: implement support for BAM locking

[Bartosz Golaszewski]

On Thu, Jan 1, 2026 at 1:00 PM Vinod Koul <vkoul@kernel.org> wrote:
>
> > >
> > > > It will perform register I/O with DMA using the BAM locking mechanism
> > > > for synchronization. Currently linux doesn't use BAM locking and is
> > > > using CPU for register I/O so trying to access locked registers will
> > > > result in external abort. I'm trying to make the QCE driver use DMA
> > > > for register I/O AND use BAM locking. To that end: we need to pass
> > > > information about wanting the command descriptor to contain the
> > > > LOCK/UNLOCK flag (this is what we set here in the hardware descriptor)
> > > > from the QCE driver to the BAM driver. I initially used a global flag.
> > > > Dmitry said it's too Qualcomm-specific and to use metadata instead.
> > > > This is what I did in this version.
> > >
> > > Okay, how will client figure out should it set the lock or not? What are
> > > the conditions where the lock is set or not set by client..?
> > >
> >
> > I'm not sure what you refer to as "client". The user of the BAM engine
> > - the crypto driver? If so - we convert it to always lock/unlock
> > assuming the TA *may* use it and it's better to be safe. Other users
> > are not affected.
>
> Client are users of dmaengine. So how does the crypto driver figure out
> when to lock/unlock. Why not do this always...?
>

It *does* do it always. We assume the TA may be doing it so the crypto
driver is converted to *always* perform register I/O with DMA *and* to
always lock the BAM for each transaction later in the series. This is
why Dmitry inquired whether all the HW with upstream support actually
supports the lock semantics.

Bart

Re: [PATCH v9 03/11] dmaengine: qcom: bam_dma: implement support for BAM locking

[Vinod Koul]

On 02-01-26, 10:26, Bartosz Golaszewski wrote:
> On Thu, Jan 1, 2026 at 1:00 PM Vinod Koul <vkoul@kernel.org> wrote:
> >
> > > >
> > > > > It will perform register I/O with DMA using the BAM locking mechanism
> > > > > for synchronization. Currently linux doesn't use BAM locking and is
> > > > > using CPU for register I/O so trying to access locked registers will
> > > > > result in external abort. I'm trying to make the QCE driver use DMA
> > > > > for register I/O AND use BAM locking. To that end: we need to pass
> > > > > information about wanting the command descriptor to contain the
> > > > > LOCK/UNLOCK flag (this is what we set here in the hardware descriptor)
> > > > > from the QCE driver to the BAM driver. I initially used a global flag.
> > > > > Dmitry said it's too Qualcomm-specific and to use metadata instead.
> > > > > This is what I did in this version.
> > > >
> > > > Okay, how will client figure out should it set the lock or not? What are
> > > > the conditions where the lock is set or not set by client..?
> > > >
> > >
> > > I'm not sure what you refer to as "client". The user of the BAM engine
> > > - the crypto driver? If so - we convert it to always lock/unlock
> > > assuming the TA *may* use it and it's better to be safe. Other users
> > > are not affected.
> >
> > Client are users of dmaengine. So how does the crypto driver figure out
> > when to lock/unlock. Why not do this always...?
> >
> 
> It *does* do it always. We assume the TA may be doing it so the crypto
> driver is converted to *always* perform register I/O with DMA *and* to
> always lock the BAM for each transaction later in the series. This is
> why Dmitry inquired whether all the HW with upstream support actually
> supports the lock semantics.

Okay then why do we need an API?

Just lock it always and set the bits in the dma driver

-- 
~Vinod

Re: [PATCH v9 03/11] dmaengine: qcom: bam_dma: implement support for BAM locking

[Bartosz Golaszewski]

On Fri, Jan 2, 2026 at 5:59 PM Vinod Koul <vkoul@kernel.org> wrote:
>
> On 02-01-26, 10:26, Bartosz Golaszewski wrote:
> > On Thu, Jan 1, 2026 at 1:00 PM Vinod Koul <vkoul@kernel.org> wrote:
> > >
> > > > >
> > > > > > It will perform register I/O with DMA using the BAM locking mechanism
> > > > > > for synchronization. Currently linux doesn't use BAM locking and is
> > > > > > using CPU for register I/O so trying to access locked registers will
> > > > > > result in external abort. I'm trying to make the QCE driver use DMA
> > > > > > for register I/O AND use BAM locking. To that end: we need to pass
> > > > > > information about wanting the command descriptor to contain the
> > > > > > LOCK/UNLOCK flag (this is what we set here in the hardware descriptor)
> > > > > > from the QCE driver to the BAM driver. I initially used a global flag.
> > > > > > Dmitry said it's too Qualcomm-specific and to use metadata instead.
> > > > > > This is what I did in this version.
> > > > >
> > > > > Okay, how will client figure out should it set the lock or not? What are
> > > > > the conditions where the lock is set or not set by client..?
> > > > >
> > > >
> > > > I'm not sure what you refer to as "client". The user of the BAM engine
> > > > - the crypto driver? If so - we convert it to always lock/unlock
> > > > assuming the TA *may* use it and it's better to be safe. Other users
> > > > are not affected.
> > >
> > > Client are users of dmaengine. So how does the crypto driver figure out
> > > when to lock/unlock. Why not do this always...?
> > >
> >
> > It *does* do it always. We assume the TA may be doing it so the crypto
> > driver is converted to *always* perform register I/O with DMA *and* to
> > always lock the BAM for each transaction later in the series. This is
> > why Dmitry inquired whether all the HW with upstream support actually
> > supports the lock semantics.
>
> Okay then why do we need an API?
>
> Just lock it always and set the bits in the dma driver
>

We need an API because we send a locking descriptor, then a regular
descriptor (or descriptors) for the actual transaction(s) and then an
unlocking descriptor. It's a thing the user of the DMA engine needs to
decide on, not the DMA engine itself.

Also: only the crypto engine needs it for now, not all the other users
of the BAM engine.

Bartosz

Re: [PATCH v9 03/11] dmaengine: qcom: bam_dma: implement support for BAM locking

[Bartosz Golaszewski]

On Fri, Jan 2, 2026 at 6:14 PM Bartosz Golaszewski <brgl@kernel.org> wrote:
>
> On Fri, Jan 2, 2026 at 5:59 PM Vinod Koul <vkoul@kernel.org> wrote:
> >
> > On 02-01-26, 10:26, Bartosz Golaszewski wrote:
> > > On Thu, Jan 1, 2026 at 1:00 PM Vinod Koul <vkoul@kernel.org> wrote:
> > > >
> > > > > >
> > > > > > > It will perform register I/O with DMA using the BAM locking mechanism
> > > > > > > for synchronization. Currently linux doesn't use BAM locking and is
> > > > > > > using CPU for register I/O so trying to access locked registers will
> > > > > > > result in external abort. I'm trying to make the QCE driver use DMA
> > > > > > > for register I/O AND use BAM locking. To that end: we need to pass
> > > > > > > information about wanting the command descriptor to contain the
> > > > > > > LOCK/UNLOCK flag (this is what we set here in the hardware descriptor)
> > > > > > > from the QCE driver to the BAM driver. I initially used a global flag.
> > > > > > > Dmitry said it's too Qualcomm-specific and to use metadata instead.
> > > > > > > This is what I did in this version.
> > > > > >
> > > > > > Okay, how will client figure out should it set the lock or not? What are
> > > > > > the conditions where the lock is set or not set by client..?
> > > > > >
> > > > >
> > > > > I'm not sure what you refer to as "client". The user of the BAM engine
> > > > > - the crypto driver? If so - we convert it to always lock/unlock
> > > > > assuming the TA *may* use it and it's better to be safe. Other users
> > > > > are not affected.
> > > >
> > > > Client are users of dmaengine. So how does the crypto driver figure out
> > > > when to lock/unlock. Why not do this always...?
> > > >
> > >
> > > It *does* do it always. We assume the TA may be doing it so the crypto
> > > driver is converted to *always* perform register I/O with DMA *and* to
> > > always lock the BAM for each transaction later in the series. This is
> > > why Dmitry inquired whether all the HW with upstream support actually
> > > supports the lock semantics.
> >
> > Okay then why do we need an API?
> >
> > Just lock it always and set the bits in the dma driver
> >
>
> We need an API because we send a locking descriptor, then a regular
> descriptor (or descriptors) for the actual transaction(s) and then an
> unlocking descriptor. It's a thing the user of the DMA engine needs to
> decide on, not the DMA engine itself.
>
> Also: only the crypto engine needs it for now, not all the other users
> of the BAM engine.
>

Hi Vinod, is there anything else I can do or more information I can
provide in order to move this forward?

Bartosz

Re: [PATCH v9 03/11] dmaengine: qcom: bam_dma: implement support for BAM locking

[Vinod Koul]

On 02-01-26, 18:14, Bartosz Golaszewski wrote:
> On Fri, Jan 2, 2026 at 5:59 PM Vinod Koul <vkoul@kernel.org> wrote:
> >
> > On 02-01-26, 10:26, Bartosz Golaszewski wrote:
> > > On Thu, Jan 1, 2026 at 1:00 PM Vinod Koul <vkoul@kernel.org> wrote:
> > > >
> > > > > >
> > > > > > > It will perform register I/O with DMA using the BAM locking mechanism
> > > > > > > for synchronization. Currently linux doesn't use BAM locking and is
> > > > > > > using CPU for register I/O so trying to access locked registers will
> > > > > > > result in external abort. I'm trying to make the QCE driver use DMA
> > > > > > > for register I/O AND use BAM locking. To that end: we need to pass
> > > > > > > information about wanting the command descriptor to contain the
> > > > > > > LOCK/UNLOCK flag (this is what we set here in the hardware descriptor)
> > > > > > > from the QCE driver to the BAM driver. I initially used a global flag.
> > > > > > > Dmitry said it's too Qualcomm-specific and to use metadata instead.
> > > > > > > This is what I did in this version.
> > > > > >
> > > > > > Okay, how will client figure out should it set the lock or not? What are
> > > > > > the conditions where the lock is set or not set by client..?
> > > > > >
> > > > >
> > > > > I'm not sure what you refer to as "client". The user of the BAM engine
> > > > > - the crypto driver? If so - we convert it to always lock/unlock
> > > > > assuming the TA *may* use it and it's better to be safe. Other users
> > > > > are not affected.
> > > >
> > > > Client are users of dmaengine. So how does the crypto driver figure out
> > > > when to lock/unlock. Why not do this always...?
> > > >
> > >
> > > It *does* do it always. We assume the TA may be doing it so the crypto
> > > driver is converted to *always* perform register I/O with DMA *and* to
> > > always lock the BAM for each transaction later in the series. This is
> > > why Dmitry inquired whether all the HW with upstream support actually
> > > supports the lock semantics.
> >
> > Okay then why do we need an API?
> >
> > Just lock it always and set the bits in the dma driver
> >
> 
> We need an API because we send a locking descriptor, then a regular
> descriptor (or descriptors) for the actual transaction(s) and then an
> unlocking descriptor. It's a thing the user of the DMA engine needs to
> decide on, not the DMA engine itself.

I think downstream sends lock descriptor always. What is the harm in
doing that every time if we go down that path?
Reg Dmitry question above, this is dma hw capability, how will client
know if it has to lock on older rev of hardware or not...?

> Also: only the crypto engine needs it for now, not all the other users
> of the BAM engine.

But they might eventually right?

-- 
~Vinod

Re: [PATCH v9 03/11] dmaengine: qcom: bam_dma: implement support for BAM locking

[Bartosz Golaszewski]

On Fri, Jan 9, 2026 at 3:27 AM Vinod Koul <vkoul@kernel.org> wrote:
>
> >
> > We need an API because we send a locking descriptor, then a regular
> > descriptor (or descriptors) for the actual transaction(s) and then an
> > unlocking descriptor. It's a thing the user of the DMA engine needs to
> > decide on, not the DMA engine itself.
>
> I think downstream sends lock descriptor always. What is the harm in
> doing that every time if we go down that path?

No, in downstream it too depends on the user setting the right bits.
Currently the only user of the BAM locking downstream is the NAND
driver. I don't think the code where the crypto driver uses it is
public yet.

And yes, there is harm - it slightly impacts performance. For QCE it
doesn't really matter as any users wanting to offload skcipher or SHA
are better off using the Arm Crypto Extensions anyway as they are
faster by an order of magnitude (!). It's also the default upstream,
where the priorities are set such that the ARM CEs are preferred over
the QCE. QCE however, is able to coordinate with the TrustZone and
will be used to support the DRM use-cases.

I prefer to avoid impacting any other users of BAM DMA.

> Reg Dmitry question above, this is dma hw capability, how will client
> know if it has to lock on older rev of hardware or not...?
>
> > Also: only the crypto engine needs it for now, not all the other users
> > of the BAM engine.
>

Trying to set the lock/unlock bits will make
dmaengine_desc_attach_metadata() fail if HW does not support it.

> But they might eventually right?
>

Yes, and they will already have the interface to do it - in the form
of descriptor metadata.

Thanks,
Bartosz

Re: [PATCH v9 03/11] dmaengine: qcom: bam_dma: implement support for BAM locking

[Bartosz Golaszewski]

On Fri, Jan 9, 2026 at 3:15 PM Bartosz Golaszewski <brgl@kernel.org> wrote:
>
> On Fri, Jan 9, 2026 at 3:27 AM Vinod Koul <vkoul@kernel.org> wrote:
> >
> > >
> > > We need an API because we send a locking descriptor, then a regular
> > > descriptor (or descriptors) for the actual transaction(s) and then an
> > > unlocking descriptor. It's a thing the user of the DMA engine needs to
> > > decide on, not the DMA engine itself.
> >
> > I think downstream sends lock descriptor always. What is the harm in
> > doing that every time if we go down that path?
>
> No, in downstream it too depends on the user setting the right bits.
> Currently the only user of the BAM locking downstream is the NAND
> driver. I don't think the code where the crypto driver uses it is
> public yet.
>
> And yes, there is harm - it slightly impacts performance. For QCE it
> doesn't really matter as any users wanting to offload skcipher or SHA
> are better off using the Arm Crypto Extensions anyway as they are
> faster by an order of magnitude (!). It's also the default upstream,
> where the priorities are set such that the ARM CEs are preferred over
> the QCE. QCE however, is able to coordinate with the TrustZone and
> will be used to support the DRM use-cases.
>
> I prefer to avoid impacting any other users of BAM DMA.
>
> > Reg Dmitry question above, this is dma hw capability, how will client
> > know if it has to lock on older rev of hardware or not...?
> >
> > > Also: only the crypto engine needs it for now, not all the other users
> > > of the BAM engine.
> >
>
> Trying to set the lock/unlock bits will make
> dmaengine_desc_attach_metadata() fail if HW does not support it.
>
> > But they might eventually right?
> >
>
> Yes, and they will already have the interface to do it - in the form
> of descriptor metadata.
>

Hi! Have I answered all your questions? Can we proceed with this?

Bartosz

Re: [PATCH v9 03/11] dmaengine: qcom: bam_dma: implement support for BAM locking

[Dmitry Baryshkov]

On Fri, Jan 09, 2026 at 07:57:00AM +0530, Vinod Koul wrote:
> On 02-01-26, 18:14, Bartosz Golaszewski wrote:
> > On Fri, Jan 2, 2026 at 5:59 PM Vinod Koul <vkoul@kernel.org> wrote:
> > >
> > > On 02-01-26, 10:26, Bartosz Golaszewski wrote:
> > > > On Thu, Jan 1, 2026 at 1:00 PM Vinod Koul <vkoul@kernel.org> wrote:
> > > > >
> > > > > > >
> > > > > > > > It will perform register I/O with DMA using the BAM locking mechanism
> > > > > > > > for synchronization. Currently linux doesn't use BAM locking and is
> > > > > > > > using CPU for register I/O so trying to access locked registers will
> > > > > > > > result in external abort. I'm trying to make the QCE driver use DMA
> > > > > > > > for register I/O AND use BAM locking. To that end: we need to pass
> > > > > > > > information about wanting the command descriptor to contain the
> > > > > > > > LOCK/UNLOCK flag (this is what we set here in the hardware descriptor)
> > > > > > > > from the QCE driver to the BAM driver. I initially used a global flag.
> > > > > > > > Dmitry said it's too Qualcomm-specific and to use metadata instead.
> > > > > > > > This is what I did in this version.
> > > > > > >
> > > > > > > Okay, how will client figure out should it set the lock or not? What are
> > > > > > > the conditions where the lock is set or not set by client..?
> > > > > > >
> > > > > >
> > > > > > I'm not sure what you refer to as "client". The user of the BAM engine
> > > > > > - the crypto driver? If so - we convert it to always lock/unlock
> > > > > > assuming the TA *may* use it and it's better to be safe. Other users
> > > > > > are not affected.
> > > > >
> > > > > Client are users of dmaengine. So how does the crypto driver figure out
> > > > > when to lock/unlock. Why not do this always...?
> > > > >
> > > >
> > > > It *does* do it always. We assume the TA may be doing it so the crypto
> > > > driver is converted to *always* perform register I/O with DMA *and* to
> > > > always lock the BAM for each transaction later in the series. This is
> > > > why Dmitry inquired whether all the HW with upstream support actually
> > > > supports the lock semantics.
> > >
> > > Okay then why do we need an API?
> > >
> > > Just lock it always and set the bits in the dma driver
> > >
> > 
> > We need an API because we send a locking descriptor, then a regular
> > descriptor (or descriptors) for the actual transaction(s) and then an
> > unlocking descriptor. It's a thing the user of the DMA engine needs to
> > decide on, not the DMA engine itself.
> 
> I think downstream sends lock descriptor always. What is the harm in
> doing that every time if we go down that path?
> Reg Dmitry question above, this is dma hw capability, how will client
> know if it has to lock on older rev of hardware or not...?

We can identify that on the calling side, but I doubt we'd need it: The
lock semantics was absent on APQ8064 / MSM8960 / IPQ8064 and it seems to
be present for all devices afterwards.

Frankly speaking, I think this is the best API we can get. It is
definitely much better than the original proposal.

> 
> > Also: only the crypto engine needs it for now, not all the other users
> > of the BAM engine.
> 
> But they might eventually right?

-- 
With best wishes
Dmitry

Re: [PATCH v9 03/11] dmaengine: qcom: bam_dma: implement support for BAM locking

[Bartosz Golaszewski]

On Wed, Jan 14, 2026 at 4:37 PM Bartosz Golaszewski <brgl@kernel.org> wrote:
>
> >
> > > But they might eventually right?
> > >
> >
> > Yes, and they will already have the interface to do it - in the form
> > of descriptor metadata.
> >
>
> Hi! Have I answered all your questions? Can we proceed with this?
>
> Bartosz

Ping.

Bart

Re: [PATCH v9 03/11] dmaengine: qcom: bam_dma: implement support for BAM locking

[Bartosz Golaszewski]

On Thu, Jan 22, 2026 at 10:33 AM Bartosz Golaszewski <brgl@kernel.org> wrote:
>
> On Wed, Jan 14, 2026 at 4:37 PM Bartosz Golaszewski <brgl@kernel.org> wrote:
> >
> > >
> > > > But they might eventually right?
> > > >
> > >
> > > Yes, and they will already have the interface to do it - in the form
> > > of descriptor metadata.
> > >
> >
> > Hi! Have I answered all your questions? Can we proceed with this?
> >
> > Bartosz
>
> Ping.
>
> Bart

Before I repost it after v7.0-rc1 - can you please let me know if all
your questions have been answered and if we can proceed with this
approach?

Bartosz

Re: [PATCH v9 03/11] dmaengine: qcom: bam_dma: implement support for BAM locking

[Manivannan Sadhasivam]

On Fri, Jan 09, 2026 at 03:15:38PM +0100, Bartosz Golaszewski wrote:
> On Fri, Jan 9, 2026 at 3:27 AM Vinod Koul <vkoul@kernel.org> wrote:
> >
> > >
> > > We need an API because we send a locking descriptor, then a regular
> > > descriptor (or descriptors) for the actual transaction(s) and then an
> > > unlocking descriptor. It's a thing the user of the DMA engine needs to
> > > decide on, not the DMA engine itself.
> >
> > I think downstream sends lock descriptor always. What is the harm in
> > doing that every time if we go down that path?
> 
> No, in downstream it too depends on the user setting the right bits.
> Currently the only user of the BAM locking downstream is the NAND
> driver. I don't think the code where the crypto driver uses it is
> public yet.
> 
> And yes, there is harm - it slightly impacts performance. For QCE it
> doesn't really matter as any users wanting to offload skcipher or SHA
> are better off using the Arm Crypto Extensions anyway as they are
> faster by an order of magnitude (!). It's also the default upstream,
> where the priorities are set such that the ARM CEs are preferred over
> the QCE. QCE however, is able to coordinate with the TrustZone and
> will be used to support the DRM use-cases.
> 
> I prefer to avoid impacting any other users of BAM DMA.
> 

Sorry for jumping late. But I disagree with the argument that the client drivers
have to set the LOCK/UNLOCK bit. These bits are specific to BAM DMA IP for
serializing the command descriptors from multiple entities. So DMA clients like
Crypto/NAND have no business in setting this flag. It is the job of the BAM
dmaengine driver to set/unset it at the start and end of the descriptor chain.

> > Reg Dmitry question above, this is dma hw capability, how will client
> > know if it has to lock on older rev of hardware or not...?
> >
> > > Also: only the crypto engine needs it for now, not all the other users
> > > of the BAM engine.
> >
> 
> Trying to set the lock/unlock bits will make
> dmaengine_desc_attach_metadata() fail if HW does not support it.
> 

The BAM dmaengine driver *must* know based on the IP version whether it supports
the LOCK/UNLOCK bits or not, not the client drivers. How can the client drivers
know about the BAM DMA IP capability?

For all these reasons, BAM driver should handle the locking mechanism internaly.
This will allow the client drivers to work without any modifications.

FWIW, NAND driver too is impacted by this missing feature in the BAM driver as
both Modem and Linux tries to driver BAM and currently Linux BAM driver doesn't
set these bits leading to crashes.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v9 03/11] dmaengine: qcom: bam_dma: implement support for BAM locking

[Dmitry Baryshkov]

On Thu, 19 Feb 2026 at 14:12, Manivannan Sadhasivam <mani@kernel.org> wrote:
>
> On Fri, Jan 09, 2026 at 03:15:38PM +0100, Bartosz Golaszewski wrote:
> > On Fri, Jan 9, 2026 at 3:27 AM Vinod Koul <vkoul@kernel.org> wrote:
> > >
> > > >
> > > > We need an API because we send a locking descriptor, then a regular
> > > > descriptor (or descriptors) for the actual transaction(s) and then an
> > > > unlocking descriptor. It's a thing the user of the DMA engine needs to
> > > > decide on, not the DMA engine itself.
> > >
> > > I think downstream sends lock descriptor always. What is the harm in
> > > doing that every time if we go down that path?
> >
> > No, in downstream it too depends on the user setting the right bits.
> > Currently the only user of the BAM locking downstream is the NAND
> > driver. I don't think the code where the crypto driver uses it is
> > public yet.
> >
> > And yes, there is harm - it slightly impacts performance. For QCE it
> > doesn't really matter as any users wanting to offload skcipher or SHA
> > are better off using the Arm Crypto Extensions anyway as they are
> > faster by an order of magnitude (!). It's also the default upstream,
> > where the priorities are set such that the ARM CEs are preferred over
> > the QCE. QCE however, is able to coordinate with the TrustZone and
> > will be used to support the DRM use-cases.
> >
> > I prefer to avoid impacting any other users of BAM DMA.
> >
>
> Sorry for jumping late. But I disagree with the argument that the client drivers
> have to set the LOCK/UNLOCK bit. These bits are specific to BAM DMA IP for
> serializing the command descriptors from multiple entities. So DMA clients like
> Crypto/NAND have no business in setting this flag. It is the job of the BAM
> dmaengine driver to set/unset it at the start and end of the descriptor chain.

Is it really granular to the single DMA chain or can it be required
for the BAM to be locked for more than single DMA chain?

>
> > > Reg Dmitry question above, this is dma hw capability, how will client
> > > know if it has to lock on older rev of hardware or not...?
> > >
> > > > Also: only the crypto engine needs it for now, not all the other users
> > > > of the BAM engine.
> > >
> >
> > Trying to set the lock/unlock bits will make
> > dmaengine_desc_attach_metadata() fail if HW does not support it.
> >
>
> The BAM dmaengine driver *must* know based on the IP version whether it supports
> the LOCK/UNLOCK bits or not, not the client drivers. How can the client drivers
> know about the BAM DMA IP capability?

How do blocks know about capabilities of other blocks? If there is no
API for getting those we can encode platform peculiarities into the OF
match data.

>
> For all these reasons, BAM driver should handle the locking mechanism internaly.
> This will allow the client drivers to work without any modifications.
>
> FWIW, NAND driver too is impacted by this missing feature in the BAM driver as
> both Modem and Linux tries to driver BAM and currently Linux BAM driver doesn't
> set these bits leading to crashes.
>
> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்



-- 
With best wishes
Dmitry

Re: [PATCH v9 03/11] dmaengine: qcom: bam_dma: implement support for BAM locking

[Bjorn Andersson]

On Thu, Feb 19, 2026 at 05:42:09PM +0530, Manivannan Sadhasivam wrote:
> On Fri, Jan 09, 2026 at 03:15:38PM +0100, Bartosz Golaszewski wrote:
> > On Fri, Jan 9, 2026 at 3:27 AM Vinod Koul <vkoul@kernel.org> wrote:
> > >
> > > >
> > > > We need an API because we send a locking descriptor, then a regular
> > > > descriptor (or descriptors) for the actual transaction(s) and then an
> > > > unlocking descriptor. It's a thing the user of the DMA engine needs to
> > > > decide on, not the DMA engine itself.
> > >
> > > I think downstream sends lock descriptor always. What is the harm in
> > > doing that every time if we go down that path?
> > 
> > No, in downstream it too depends on the user setting the right bits.
> > Currently the only user of the BAM locking downstream is the NAND
> > driver. I don't think the code where the crypto driver uses it is
> > public yet.
> > 
> > And yes, there is harm - it slightly impacts performance. For QCE it
> > doesn't really matter as any users wanting to offload skcipher or SHA
> > are better off using the Arm Crypto Extensions anyway as they are
> > faster by an order of magnitude (!). It's also the default upstream,
> > where the priorities are set such that the ARM CEs are preferred over
> > the QCE. QCE however, is able to coordinate with the TrustZone and
> > will be used to support the DRM use-cases.
> > 
> > I prefer to avoid impacting any other users of BAM DMA.
> > 
> 
> Sorry for jumping late. But I disagree with the argument that the client drivers
> have to set the LOCK/UNLOCK bit. These bits are specific to BAM DMA IP for
> serializing the command descriptors from multiple entities. So DMA clients like
> Crypto/NAND have no business in setting this flag. It is the job of the BAM
> dmaengine driver to set/unset it at the start and end of the descriptor chain.
> 

Thanks for pointing this out, Mani.

I agree, pushing this responsibility to the clients breaks the
abstraction between the components and leads to nasty debugging
scenarios.

The question worth answering is if there are any legitimate cases for
holding the lock beyond what can reasonably be expressed as a descriptor
chain.

Regards,
Bjorn

> > > Reg Dmitry question above, this is dma hw capability, how will client
> > > know if it has to lock on older rev of hardware or not...?
> > >
> > > > Also: only the crypto engine needs it for now, not all the other users
> > > > of the BAM engine.
> > >
> > 
> > Trying to set the lock/unlock bits will make
> > dmaengine_desc_attach_metadata() fail if HW does not support it.
> > 
> 
> The BAM dmaengine driver *must* know based on the IP version whether it supports
> the LOCK/UNLOCK bits or not, not the client drivers. How can the client drivers
> know about the BAM DMA IP capability?
> 
> For all these reasons, BAM driver should handle the locking mechanism internaly.
> This will allow the client drivers to work without any modifications.
> 
> FWIW, NAND driver too is impacted by this missing feature in the BAM driver as
> both Modem and Linux tries to driver BAM and currently Linux BAM driver doesn't
> set these bits leading to crashes.
> 
> - Mani
> 
> -- 
> மணிவண்ணன் சதாசிவம்
> 

Re: [PATCH v9 03/11] dmaengine: qcom: bam_dma: implement support for BAM locking

[Bartosz Golaszewski]

On Thu, 19 Feb 2026 13:12:09 +0100, Manivannan Sadhasivam
<mani@kernel.org> said:
> On Fri, Jan 09, 2026 at 03:15:38PM +0100, Bartosz Golaszewski wrote:
>> On Fri, Jan 9, 2026 at 3:27 AM Vinod Koul <vkoul@kernel.org> wrote:
>> >
>> > >
>> > > We need an API because we send a locking descriptor, then a regular
>> > > descriptor (or descriptors) for the actual transaction(s) and then an
>> > > unlocking descriptor. It's a thing the user of the DMA engine needs to
>> > > decide on, not the DMA engine itself.
>> >
>> > I think downstream sends lock descriptor always. What is the harm in
>> > doing that every time if we go down that path?
>>
>> No, in downstream it too depends on the user setting the right bits.
>> Currently the only user of the BAM locking downstream is the NAND
>> driver. I don't think the code where the crypto driver uses it is
>> public yet.
>>
>> And yes, there is harm - it slightly impacts performance. For QCE it
>> doesn't really matter as any users wanting to offload skcipher or SHA
>> are better off using the Arm Crypto Extensions anyway as they are
>> faster by an order of magnitude (!). It's also the default upstream,
>> where the priorities are set such that the ARM CEs are preferred over
>> the QCE. QCE however, is able to coordinate with the TrustZone and
>> will be used to support the DRM use-cases.
>>
>> I prefer to avoid impacting any other users of BAM DMA.
>>
>
> Sorry for jumping late. But I disagree with the argument that the client drivers
> have to set the LOCK/UNLOCK bit. These bits are specific to BAM DMA IP for
> serializing the command descriptors from multiple entities. So DMA clients like
> Crypto/NAND have no business in setting this flag. It is the job of the BAM
> dmaengine driver to set/unset it at the start and end of the descriptor chain.
>

But what if a given client does not need locking? We don't want to enable it
for everyone - as I explained before.

>> > Reg Dmitry question above, this is dma hw capability, how will client
>> > know if it has to lock on older rev of hardware or not...?
>> >
>> > > Also: only the crypto engine needs it for now, not all the other users
>> > > of the BAM engine.
>> >
>>
>> Trying to set the lock/unlock bits will make
>> dmaengine_desc_attach_metadata() fail if HW does not support it.
>>
>
> The BAM dmaengine driver *must* know based on the IP version whether it supports
> the LOCK/UNLOCK bits or not, not the client drivers. How can the client drivers
> know about the BAM DMA IP capability?
>

FYI: the current version of this is v10[1].

In it (and in this one too but let's discuss the current one) the BAM driver
*does* know *based on IP version* whether is supports locking or not. The client
requests a lock but this will fail if the BAM does not support it. The
client does
not check the BAM IP revision. So yes: it's the BAM driver that's in charge.

> For all these reasons, BAM driver should handle the locking mechanism internaly.
> This will allow the client drivers to work without any modifications.
>

Ok, I'm open to alternatives but please help me figure out the "hows": How do
you tell the BAM driver that the client needs (or does not) locking? How do
you handle the case where we need to lock the BAM, send an arbitrary number
of descriptors from the client and then unlock it? How can the BAM know *when*
to lock/unlock?

> FWIW, NAND driver too is impacted by this missing feature in the BAM driver as
> both Modem and Linux tries to driver BAM and currently Linux BAM driver doesn't
> set these bits leading to crashes.
>

Yes, downstream handles that in a very dirty way (same as downstream QCE).

Bart

[1] https://lore.kernel.org/all/20251219-qcom-qce-cmd-descr-v10-0-ff7e4bf7dad4@oss.qualcomm.com/

Re: [PATCH v9 03/11] dmaengine: qcom: bam_dma: implement support for BAM locking

[Manivannan Sadhasivam]

On Thu, Feb 19, 2026 at 07:30:04AM -0600, Bartosz Golaszewski wrote:
> On Thu, 19 Feb 2026 13:12:09 +0100, Manivannan Sadhasivam
> <mani@kernel.org> said:
> > On Fri, Jan 09, 2026 at 03:15:38PM +0100, Bartosz Golaszewski wrote:
> >> On Fri, Jan 9, 2026 at 3:27 AM Vinod Koul <vkoul@kernel.org> wrote:
> >> >
> >> > >
> >> > > We need an API because we send a locking descriptor, then a regular
> >> > > descriptor (or descriptors) for the actual transaction(s) and then an
> >> > > unlocking descriptor. It's a thing the user of the DMA engine needs to
> >> > > decide on, not the DMA engine itself.
> >> >
> >> > I think downstream sends lock descriptor always. What is the harm in
> >> > doing that every time if we go down that path?
> >>
> >> No, in downstream it too depends on the user setting the right bits.
> >> Currently the only user of the BAM locking downstream is the NAND
> >> driver. I don't think the code where the crypto driver uses it is
> >> public yet.
> >>
> >> And yes, there is harm - it slightly impacts performance. For QCE it
> >> doesn't really matter as any users wanting to offload skcipher or SHA
> >> are better off using the Arm Crypto Extensions anyway as they are
> >> faster by an order of magnitude (!). It's also the default upstream,
> >> where the priorities are set such that the ARM CEs are preferred over
> >> the QCE. QCE however, is able to coordinate with the TrustZone and
> >> will be used to support the DRM use-cases.
> >>
> >> I prefer to avoid impacting any other users of BAM DMA.
> >>
> >
> > Sorry for jumping late. But I disagree with the argument that the client drivers
> > have to set the LOCK/UNLOCK bit. These bits are specific to BAM DMA IP for
> > serializing the command descriptors from multiple entities. So DMA clients like
> > Crypto/NAND have no business in setting this flag. It is the job of the BAM
> > dmaengine driver to set/unset it at the start and end of the descriptor chain.
> >
> 
> But what if a given client does not need locking? We don't want to enable it
> for everyone - as I explained before.
> 

That's not going to hurt. AFAIK, enabling locking wouldn't cause any notable
performance overhead.

> >> > Reg Dmitry question above, this is dma hw capability, how will client
> >> > know if it has to lock on older rev of hardware or not...?
> >> >
> >> > > Also: only the crypto engine needs it for now, not all the other users
> >> > > of the BAM engine.
> >> >
> >>
> >> Trying to set the lock/unlock bits will make
> >> dmaengine_desc_attach_metadata() fail if HW does not support it.
> >>
> >
> > The BAM dmaengine driver *must* know based on the IP version whether it supports
> > the LOCK/UNLOCK bits or not, not the client drivers. How can the client drivers
> > know about the BAM DMA IP capability?
> >
> 
> FYI: the current version of this is v10[1].
> 
> In it (and in this one too but let's discuss the current one) the BAM driver
> *does* know *based on IP version* whether is supports locking or not. The client
> requests a lock but this will fail if the BAM does not support it. The
> client does
> not check the BAM IP revision. So yes: it's the BAM driver that's in charge.
> 

This design looks flawed. The client *doesn't* know whether it needs locking or
not. If the BAM supports locking, it should enable it for all descriptors.

> > For all these reasons, BAM driver should handle the locking mechanism internaly.
> > This will allow the client drivers to work without any modifications.
> >
> 
> Ok, I'm open to alternatives but please help me figure out the "hows": How do
> you tell the BAM driver that the client needs (or does not) locking?

As said above, BAM doesn't need to know. Locking is the hardware capability of
the BAM, not clients.

> How do
> you handle the case where we need to lock the BAM, send an arbitrary number
> of descriptors from the client and then unlock it? How can the BAM know *when*
> to lock/unlock?
> 

BAM driver has to perform lock during issue_pending() and unlock while reporting
the completion using vchan_cookie_complete().

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v9 03/11] dmaengine: qcom: bam_dma: implement support for BAM locking

[Vinod Koul]

On 19-02-26, 20:10, Manivannan Sadhasivam wrote:
> On Thu, Feb 19, 2026 at 07:30:04AM -0600, Bartosz Golaszewski wrote:
> > On Thu, 19 Feb 2026 13:12:09 +0100, Manivannan Sadhasivam
> > <mani@kernel.org> said:
> > > On Fri, Jan 09, 2026 at 03:15:38PM +0100, Bartosz Golaszewski wrote:
> > >> On Fri, Jan 9, 2026 at 3:27 AM Vinod Koul <vkoul@kernel.org> wrote:
> > >> >
> > >> > >
> > >> > > We need an API because we send a locking descriptor, then a regular
> > >> > > descriptor (or descriptors) for the actual transaction(s) and then an
> > >> > > unlocking descriptor. It's a thing the user of the DMA engine needs to
> > >> > > decide on, not the DMA engine itself.
> > >> >
> > >> > I think downstream sends lock descriptor always. What is the harm in
> > >> > doing that every time if we go down that path?
> > >>
> > >> No, in downstream it too depends on the user setting the right bits.
> > >> Currently the only user of the BAM locking downstream is the NAND
> > >> driver. I don't think the code where the crypto driver uses it is
> > >> public yet.
> > >>
> > >> And yes, there is harm - it slightly impacts performance. For QCE it
> > >> doesn't really matter as any users wanting to offload skcipher or SHA
> > >> are better off using the Arm Crypto Extensions anyway as they are
> > >> faster by an order of magnitude (!). It's also the default upstream,
> > >> where the priorities are set such that the ARM CEs are preferred over
> > >> the QCE. QCE however, is able to coordinate with the TrustZone and
> > >> will be used to support the DRM use-cases.
> > >>
> > >> I prefer to avoid impacting any other users of BAM DMA.
> > >>
> > >
> > > Sorry for jumping late. But I disagree with the argument that the client drivers
> > > have to set the LOCK/UNLOCK bit. These bits are specific to BAM DMA IP for
> > > serializing the command descriptors from multiple entities. So DMA clients like
> > > Crypto/NAND have no business in setting this flag. It is the job of the BAM
> > > dmaengine driver to set/unset it at the start and end of the descriptor chain.
> > >
> > 
> > But what if a given client does not need locking? We don't want to enable it
> > for everyone - as I explained before.
> > 
> 
> That's not going to hurt. AFAIK, enabling locking wouldn't cause any notable
> performance overhead.

I was always skeptical on this one. I had never seen why locking should
be pushed to clients. As Bjorn said it leads to more mess than worth it.
Thanks Mnai

> 
> > >> > Reg Dmitry question above, this is dma hw capability, how will client
> > >> > know if it has to lock on older rev of hardware or not...?
> > >> >
> > >> > > Also: only the crypto engine needs it for now, not all the other users
> > >> > > of the BAM engine.
> > >> >
> > >>
> > >> Trying to set the lock/unlock bits will make
> > >> dmaengine_desc_attach_metadata() fail if HW does not support it.
> > >>
> > >
> > > The BAM dmaengine driver *must* know based on the IP version whether it supports
> > > the LOCK/UNLOCK bits or not, not the client drivers. How can the client drivers
> > > know about the BAM DMA IP capability?

Lock bits are on the BAM DMA IP or client? Can we not add this
capability to BAM driver and lock for IPs that support

> > >
> > 
> > FYI: the current version of this is v10[1].
> > 
> > In it (and in this one too but let's discuss the current one) the BAM driver
> > *does* know *based on IP version* whether is supports locking or not. The client
> > requests a lock but this will fail if the BAM does not support it. The
> > client does
> > not check the BAM IP revision. So yes: it's the BAM driver that's in charge.
> > 
> 
> This design looks flawed. The client *doesn't* know whether it needs locking or
> not. If the BAM supports locking, it should enable it for all descriptors.

Ack

> 
> > > For all these reasons, BAM driver should handle the locking mechanism internaly.
> > > This will allow the client drivers to work without any modifications.
> > >
> > 
> > Ok, I'm open to alternatives but please help me figure out the "hows": How do
> > you tell the BAM driver that the client needs (or does not) locking?
> 
> As said above, BAM doesn't need to know. Locking is the hardware capability of
> the BAM, not clients.
> 
> > How do
> > you handle the case where we need to lock the BAM, send an arbitrary number
> > of descriptors from the client and then unlock it? How can the BAM know *when*
> > to lock/unlock?
> > 
> 
> BAM driver has to perform lock during issue_pending() and unlock while reporting
> the completion using vchan_cookie_complete().

Sounds good to me, thanks Mani

-- 
~Vinod

Re: [PATCH v9 03/11] dmaengine: qcom: bam_dma: implement support for BAM locking

[Bartosz Golaszewski]

On Wed, Feb 25, 2026 at 10:52 AM Vinod Koul <vkoul@kernel.org> wrote:
>
> > > >
> > > > Sorry for jumping late. But I disagree with the argument that the client drivers
> > > > have to set the LOCK/UNLOCK bit. These bits are specific to BAM DMA IP for
> > > > serializing the command descriptors from multiple entities. So DMA clients like
> > > > Crypto/NAND have no business in setting this flag. It is the job of the BAM
> > > > dmaengine driver to set/unset it at the start and end of the descriptor chain.
> > > >
> > >
> > > But what if a given client does not need locking? We don't want to enable it
> > > for everyone - as I explained before.
> > >
> >
> > That's not going to hurt. AFAIK, enabling locking wouldn't cause any notable
> > performance overhead.
>
> I was always skeptical on this one. I had never seen why locking should
> be pushed to clients. As Bjorn said it leads to more mess than worth it.
> Thanks Mnai
>

[snip]

> >
> > > >> > Reg Dmitry question above, this is dma hw capability, how will client
> > > >> > know if it has to lock on older rev of hardware or not...?
> > > >> >
> > > >> > > Also: only the crypto engine needs it for now, not all the other users
> > > >> > > of the BAM engine.
> > > >> >
> > > >>
> > > >> Trying to set the lock/unlock bits will make
> > > >> dmaengine_desc_attach_metadata() fail if HW does not support it.
> > > >>
> > > >
> > > > The BAM dmaengine driver *must* know based on the IP version whether it supports
> > > > the LOCK/UNLOCK bits or not, not the client drivers. How can the client drivers
> > > > know about the BAM DMA IP capability?
>
> Lock bits are on the BAM DMA IP or client? Can we not add this
> capability to BAM driver and lock for IPs that support
>

Lock bits are in the BAM command descriptors. We do in fact already
expose the layout of BAM command elements to consumers.

> >
> > > How do
> > > you handle the case where we need to lock the BAM, send an arbitrary number
> > > of descriptors from the client and then unlock it? How can the BAM know *when*
> > > to lock/unlock?
> > >
> >
> > BAM driver has to perform lock during issue_pending() and unlock while reporting
> > the completion using vchan_cookie_complete().
>
> Sounds good to me, thanks Mani
>

This sounds great in theory but submitting new descriptors while
you're starting the engines proves to be quite tricky. :(

Over the course of this week, I tried really hard to make this happen.
The fact that we have two descriptor chains - one with data and one
with command descriptors - prepared with two separate calls to
dmaengine_prep_slave_sg(), but which we want to wrap with a
lock/unlock means we'll most likely really need to find a way to
insert the dummy command descriptors the moment we want to flush the
queue. Though that also means that the consumers need to adjust - for
instance: they need to submit both the data and command descriptors
*before* calling dmaengine_issue_pending(). So there goes not
involving the clients in the locking logic.

I will give it another try on Monday, but there's also another
problem. The lock/unlock bits need to be attached to *real* command
descriptors. So we need an actual "dummy" register write. The HPG
evens says this is the way to do it and I verified that passing 0 in
addr and size fields of a command descriptor will result in an smmu
fault. In the current approach, we do a harmles write into the VERSION
register of the QCE. But the address of that register is not known to
the BAM driver. How do I tell the BAM driver which address to write
to? There's struct dma_slave_config that has dst_addr and src_addr
fields that could be reused but that's probably not really their
function.

Bart