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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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