[PATCH v21 06/14] dmaengine: qcom: bam_dma: add support for BAM locking
Bartosz Golaszewski
brgl at kernel.org
Wed Jul 15 07:43:21 PDT 2026
On Tue, 14 Jul 2026 11:49:14 +0200, Stephan Gerhold
<stephan.gerhold at linaro.org> said:
> On Mon, Jul 13, 2026 at 03:01:07PM +0200, 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.
>>
>> The dummy writes must be issued into a scratchpad register of the client
>> so provide a mechanism to communicate the right address via descriptor
>> metadata.
>>
>> Reviewed-by: Manivannan Sadhasivam <mani at kernel.org>
>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski at oss.qualcomm.com>
>
> Thanks for the fixes. The lock/unlock sequence looks good to me now,
> I commented on a couple of minor things below that would be good to fix
> (some of them are also reported by Sashiko).
>
Thanks.
>> ---
>> drivers/dma/qcom/bam_dma.c | 191 +++++++++++++++++++++++++++++++++++++--
>> include/linux/dma/qcom_bam_dma.h | 14 +++
>> 2 files changed, 198 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
>> index f3e713a5259c2c7c24cfdcec094814eb1202971a..f08549ee3872eece85884606d6ee9e540ee688ca 100644
>> --- a/drivers/dma/qcom/bam_dma.c
>> +++ b/drivers/dma/qcom/bam_dma.c
>> [...]
>> @@ -686,6 +702,35 @@ 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 bam_chan *bchan = to_bam_chan(desc->chan);
>> + const struct bam_device_data *bdata = bchan->bdev->dev_data;
>> + struct bam_desc_metadata *metadata = data;
>> +
>> + if (!data)
>
> Doesn't really matter much, but since the parameter exists you might as
> well add
>
> && len == sizeof(*metadata)
>
> here to be sure.
>
Ok.
>> + return -EINVAL;
>> +
>> + if (!bdata->pipe_lock_supported)
>> + /*
>> + * The client wants to use locking but this BAM version doesn't
>> + * support it. Don't return an error here as this will stop the
>> + * client from using DMA at all for no reason.
>> + */
>> + return 0;
>> +
>> + guard(spinlock_irqsave)(&bchan->vc.lock);
>> +
>> + bchan->scratchpad_addr = metadata->scratchpad_addr;
>> + bchan->direction = metadata->direction;
>> +
>> + return 0;
>> +}
>> +
>> +static const struct dma_descriptor_metadata_ops bam_metadata_ops = {
>> + .attach = bam_metadata_attach,
>> +};
>
> I'm not sure if we have discussed this before, but could we avoid
> re-programming the scratchpad_addr all the time by placing it into
> struct dma_slave_config -> peripheral_config? It still feels awkward to
> me to place a global constant configuration value into per-descriptor
> metadata.
>
Yes, this was discussed several versions ago. I went with using
dmaengine_slave_config() until v11. I think Vinod or Dmitry requested we change
it to the metadata.
>> +
>> /**
>> * bam_prep_slave_sg - Prep slave sg transaction
>> *
>> [...]
>> @@ -802,6 +851,7 @@ static int bam_dma_terminate_all(struct dma_chan *chan)
>> }
>>
>> vchan_get_all_descriptors(&bchan->vc, &head);
>> + bchan->bam_locked = false;
>
> I wonder about the implications of this. If the LOCK descriptor was
> already processed, will we cause a deadlock if we never submit the
> UNLOCK descriptor? Or I guess bam_reset_channel() might reset the lock
> as well?
>
I'll look into it.
>> }
>>
>> vchan_dma_desc_free_list(&bchan->vc, &head);
>> [...]
>> @@ -870,6 +929,7 @@ static u32 process_channel_irqs(struct bam_device *bdev)
>> {
>> u32 i, srcs, pipe_stts, offset, avail;
>> struct bam_async_desc *async_desc, *tmp;
>> + struct bam_desc_hw *hdesc;
>>
>> srcs = readl_relaxed(bam_addr(bdev, 0, BAM_IRQ_SRCS_EE));
>>
>> @@ -919,13 +979,20 @@ static u32 process_channel_irqs(struct bam_device *bdev)
>> * push back to front of desc_issued so that
>> * it gets restarted by the work queue.
>> */
>> +
>> + list_del(&async_desc->desc_node);
>> if (!async_desc->num_desc) {
>> - vchan_cookie_complete(&async_desc->vd);
>> + hdesc = async_desc->desc;
>> + u16 flags = le16_to_cpu(hdesc->flags);
>
> Is this unused? Also a bit odd to have hdesc declared outside of the
> loop and flags declared inside.
>
>> +
>> + if (async_desc->is_lock_desc)
>> + bam_dma_free_lock_desc(&async_desc->vd);
>> + else
>> + vchan_cookie_complete(&async_desc->vd);
>> } else {
>> list_add(&async_desc->vd.node,
>> &bchan->vc.desc_issued);
>> }
>> - list_del(&async_desc->desc_node);
>> }
>> }
>>
>> @@ -1046,13 +1113,102 @@ static void bam_apply_new_config(struct bam_chan *bchan,
>> bchan->reconfigure = 0;
>> }
>>
>> +static struct bam_async_desc *
>> +bam_make_lock_desc(struct bam_chan *bchan, unsigned long flag)
>> +{
>> + struct dma_chan *chan = &bchan->vc.chan;
>> + struct bam_async_desc *async_desc;
>> + struct bam_desc_hw *desc;
>> + struct virt_dma_desc *vd;
>> + struct virt_dma_chan *vc;
>> + unsigned int mapped;
>> +
>> + async_desc = kzalloc_flex(*async_desc, desc, 1, GFP_NOWAIT);
>> + if (!async_desc) {
>> + dev_err(bchan->bdev->dev, "failed to allocate the BAM lock descriptor\n");
>> + return ERR_PTR(-ENOMEM);
>> + }
>> +
>> + sg_init_table(&async_desc->lock_sg, 1);
>> +
>> + async_desc->num_desc = 1;
>> + async_desc->curr_desc = async_desc->desc;
>> + async_desc->dir = DMA_MEM_TO_DEV;
>> + async_desc->is_lock_desc = true;
>> +
>> + desc = async_desc->desc;
>> +
>> + bam_prep_ce_le32(&async_desc->lock_ce, bchan->scratchpad_addr, BAM_WRITE_COMMAND, 0);
>> + sg_set_buf(&async_desc->lock_sg, &async_desc->lock_ce, sizeof(async_desc->lock_ce));
>> +
>> + mapped = dma_map_sg(chan->slave, &async_desc->lock_sg, 1, DMA_TO_DEVICE);
>
> I agree with Sashiko that mapping using the BAM device
> (bchan->bdev->dev) would be more precise, since the BAM will be reading
> the descriptor. (It doesn't matter in practice since both BAM and
> consumer usually have the same IOMMUs defined.)
>
Ok
>> + if (!mapped) {
>> + kfree(async_desc);
>> + return ERR_PTR(-ENOMEM);
>> + }
>> +
>> + desc->flags |= cpu_to_le16(DESC_FLAG_CMD | flag);
>> + desc->addr = sg_dma_address(&async_desc->lock_sg);
>
> cpu_to_le32()
>
Right.
>> + desc->size = cpu_to_le16(sizeof(struct bam_cmd_element));
>> +
>> + vc = &bchan->vc;
>> + vd = &async_desc->vd;
>> +
>> + dma_async_tx_descriptor_init(&vd->tx, &vc->chan);
>> + vd->tx.flags = DMA_PREP_CMD;
>> + vd->tx_result.result = DMA_TRANS_NOERROR;
>> + vd->tx_result.residue = 0;
>> +
>> + return async_desc;
>> +}
>> +
>> [...]
>> @@ -1072,6 +1229,18 @@ static void bam_start_dma(struct bam_chan *bchan)
>> return;
>>
>> while (vd && !IS_BUSY(bchan)) {
>> + /*
>> + * Open a LOCK/UNLOCK bracket around each fresh sequence.
>> + * Sentinels inserted by bam_setup_pipe_lock() are skipped: they
>> + * already have bam_locked set and must not trigger a second pair.
>> + */
>> + if (!bchan->bam_locked &&
>> + !container_of(vd, struct bam_async_desc, vd)->is_lock_desc) {
>
> Do we need the ->is_lock_desc check here? Looks redundant to me.
>
I don't remember anymore but I think I did it on purpose for some reason. I'll
double check.
>> + ret = bam_setup_pipe_lock(bchan);
>> + if (ret == 0 && bchan->bam_locked)
>> + vd = vchan_next_desc(&bchan->vc);
>
> Do we want some error handling here? If there is an error, this will
> silently continue queuing everything without any locking.
>
Ok, makes sense.
Bart
More information about the linux-arm-kernel
mailing list