Re: [PATCH v12 05/12] dmaengine: qcom: bam_dma: add support for BAM locking

Stephan Gerhold
2026-03-11T10:00:54Z

On Tue, Mar 10, 2026 at 04:44:19PM +0100, Bartosz Golaszewski wrote:
> Add support for BAM pipe locking. To that end: when starting DMA on an RX
> channel - prepend the existing queue of issued descriptors with an
> additional "dummy" command descriptor with the LOCK bit set. Once the
> transaction is done (no more issued descriptors), issue one more dummy
> descriptor with the UNLOCK bit.
>
> We *must* wait until the transaction is signalled as done because we
> must not perform any writes into config registers while the engine is
> busy.
>
> [...]
> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index 83491e7c2f17d8c9d12a1a055baea7e3a0a75a53..627c85a2df4dcdbac247d831a4aef047c2189456 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c
> [...]
> +static int bam_do_setup_pipe_lock(struct bam_chan *bchan, bool lock)
> +{
> + struct bam_device *bdev = bchan->bdev;
> + const struct bam_device_data *bdata = bdev->dev_data;
> + struct bam_async_desc *lock_desc;
> + struct bam_cmd_element *ce;
> + struct scatterlist *sgl;
> + unsigned long flag;
> +
> + lockdep_assert_held(&bchan->vc.lock);
> +
> + if (!bdata->pipe_lock_supported || !bchan->scratchpad_addr ||
> + bchan->slave.direction != DMA_MEM_TO_DEV)
> + return 0;
> +
> + if (lock) {
> + sgl = &bchan->lock_sg;
> + ce = &bchan->lock_ce;
> + flag = DESC_FLAG_LOCK;
> + } else {
> + sgl = &bchan->unlock_sg;
> + ce = &bchan->unlock_ce;
> + flag = DESC_FLAG_UNLOCK;
> + }
> +
> + lock_desc = bam_make_lock_desc(bchan, sgl, ce, flag);
> + if (!lock_desc)
> + return -ENOMEM;
> +
> + if (lock)
> + list_add(&lock_desc->vd.node, &bchan->vc.desc_issued);
> + else
> + list_add_tail(&lock_desc->vd.node, &bchan->vc.desc_issued);
> +
> + bchan->locked = lock;
> +
> + return 0;
> +}
> +
> +static int bam_setup_pipe_lock(struct bam_chan *bchan)
> +{
> + return bam_do_setup_pipe_lock(bchan, true);
> +}
> +
> +static int bam_setup_pipe_unlock(struct bam_chan *bchan)
> +{
> + return bam_do_setup_pipe_lock(bchan, false);
> +}
> +
> /**
> * bam_start_dma - start next transaction
> * @bchan: bam dma channel
> @@ -1121,6 +1266,7 @@ static void bam_dma_work(struct work_struct *work)
> struct bam_device *bdev = from_work(bdev, work, work);
> struct bam_chan *bchan;
> unsigned int i;
> + int ret;
>
> /* go through the channels and kick off transactions */
> for (i = 0; i < bdev->num_channels; i++) {
> @@ -1128,6 +1274,13 @@ static void bam_dma_work(struct work_struct *work)
>
> guard(spinlock_irqsave)(&bchan->vc.lock);
>
> + if (list_empty(&bchan->vc.desc_issued) && bchan->locked) {
> + ret = bam_setup_pipe_unlock(bchan);
> + if (ret)
> + dev_err(bchan->vc.chan.slave,
> + "Failed to set up the pipe unlock descriptor\n");
> + }
> +
> if (!list_empty(&bchan->vc.desc_issued) && !IS_BUSY(bchan))
> bam_start_dma(bchan);
> }

I'm not entirely sure if this actually guarantees waiting with the
unlock until the transaction is "done", for two reasons:

1. &bchan->vc.desc_issued looks like a "TODO" list for descriptors we
haven't fully managed to squeeze into the BAM FIFO yet. It doesn't
tell you which descriptors have been consumed and finished
processing inside the FIFO.

Consider e.g. the following case: The client has issued a number of
descriptors, they all fit into the FIFO. The first descriptor has a
callback assigned, so we ask the BAM to send us an interrupt when it
has been consumed. We get the interrupt for the first descriptor and
process_channel_irqs() marks it as completed, the rest of the
descriptors are still pending. &bchan->vc.desc_issued is empty, so
you queue the unlock command before the rest of the descriptors have
finished.

2. From reading the BAM chapter in the APQ8016E TRM I get the
impression that by default an interrupt for a descriptor just tells
you that the descriptor was consumed by the BAM (and forwarded to
the peripheral). If you want to guarantee that the transaction is
actually done on the peripheral side before allowing writes into
config registers, you would need to set the NWD (Notify When Done)
bit (aka DMA_PREP_FENCE) on the last descriptor before the unlock
command.

NWD seems to stall descriptor processing until the peripheral
signals completion, so this might allow you to immediately queue the
unlock command like in v11. The downside is that you would need to
make assumptions about the set of commands submitted by the client
again... The downstream driver seems to set NWD on the data
descriptor immediately before the UNLOCK command [1].

The chapter in the APQ8016E TRM kind of contradicts itself
sometimes, but there is this sentence for example: "On the data
descriptor preceding command descriptor, NWD bit must be asserted in
order to assure that all the data has been transferred and the
peripheral is ready to be re-configured."

Thanks,
Stephan

[1]: https://git.codelinaro.org/clo/la/platform/vendor/qcom/opensource/securemsm-kernel/-/blob/fa55a96773d3fbfcd96beb2965efcaaae5697816/crypto-qti/qce50.c#L5361-5362