Re: [PATCH v24 06/14] dmaengine: qcom: bam_dma: add support for BAM locking

[Stephan Gerhold <stephan.gerhold@linaro.org>]

On Thu, Jul 23, 2026 at 07:09:12PM +0200, Bartosz Golaszewski wrote:
> Add support for BAM pipe locking. To that end: when starting DMA on a TX
> channel (DMA_MEM_TO_DEV) - 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 slave
> config.
> 
> Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
> [Stephan: came up with the solution to write lock/unlock descriptors
> directly into the FIFO]
> Co-developed-by: Stephan Gerhold <stephan.gerhold@linaro.org>
> Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
> ---
>  drivers/dma/qcom/bam_dma.c       | 146 ++++++++++++++++++++++++++++++++++-----
>  include/linux/dma/qcom_bam_dma.h |  13 ++++
>  2 files changed, 142 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index f3e713a5259c2c7c24cfdcec094814eb1202971a..373569c36a94e149d54ef041974b11018e634669 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c
> [...]
> @@ -676,10 +697,51 @@ static void bam_free_chan(struct dma_chan *chan)
>  static int bam_slave_config(struct dma_chan *chan,
>  			    struct dma_slave_config *cfg)
>  {
> +	struct bam_config *peripheral_cfg = cfg->peripheral_config;
>  	struct bam_chan *bchan = to_bam_chan(chan);
> +	const struct bam_device_data *bdata = bchan->bdev->dev_data;
> +
> +	if (peripheral_cfg && cfg->peripheral_size != sizeof(*peripheral_cfg))
> +		return -EINVAL;
>  
>  	guard(spinlock_irqsave)(&bchan->vc.lock);
>  
> +	/*
> +	 * This is required to setup the pipe locking and must be done even
> +	 * before the first call to bam_start_dma().
> +	 */
> +	if (bdata->pipe_lock_supported && peripheral_cfg) {
> +		if (cfg->direction != DMA_MEM_TO_DEV)
> +			return -EINVAL;
> +
> +		if (bchan->bam_locked)
> +			return -EBUSY;
> +
> +		if (!bchan->lock_ce) {
> +			bchan->lock_ce = kmalloc_obj(*bchan->lock_ce, GFP_ATOMIC);
> +			if (!bchan->lock_ce)
> +				return -ENOMEM;
> +
> +			bchan->lock_ce_phys = dma_map_single(bchan->bdev->dev, bchan->lock_ce,
> +							     sizeof(*bchan->lock_ce),
> +							     DMA_TO_DEVICE);
> +			if (dma_mapping_error(bchan->bdev->dev, bchan->lock_ce_phys)) {
> +				kfree(bchan->lock_ce);
> +				bchan->lock_ce = NULL;
> +				return -ENOMEM;
> +			}
> +		}
> +
> +		bam_prep_ce_le32(bchan->lock_ce, peripheral_cfg->lock_scratchpad_addr,
> +				 BAM_WRITE_COMMAND, 0);
> +		dma_sync_single_for_device(bchan->bdev->dev, bchan->lock_ce_phys,
> +					   sizeof(*bchan->lock_ce), DMA_TO_DEVICE);

Nitpick: The sync is redundant if you just called dma_map_single()
above. It doesn't hurt to sync again for the one-time setup though.

> +		bchan->locking_enabled = true;
> +	} else {
> +		/* Don't touch lock_ce here, it might still be used by issued descriptors */
> +		bchan->locking_enabled = false;
> +	}
> +
>  	memcpy(&bchan->slave, cfg, sizeof(*cfg));
>  	bchan->reconfigure = 1;
>  
> @@ -802,6 +864,7 @@ static int bam_dma_terminate_all(struct dma_chan *chan)
>  		}
>  
>  		vchan_get_all_descriptors(&bchan->vc, &head);
> +		bchan->bam_locked = false;
>  	}

I think this needs some more thought. I put this assignment into
bam_reset_channel(), because presumably that's the only operation that
would really reset the lock status without processing an unlock
descriptor.

bam_dma_terminate_all() does not always reset the BAM channel. It does
that only if (!list_empty(&bchan->desc_list)) {
	bam_chan_init_hw(bchan, async_desc->dir);
}

I _think_ it is possible that we have no in-flight descriptors
(bchan->desc_list), but the unlock is still pending (or not even written
to the FIFO yet). In that case, bchan->bam_locked should not be reset
here.

I would put the bchan->bam_locked = false; into bam_reset_channel() like
in my diff.

Then, you could either leave this as-is (and assume the unlock
descriptor will still be finished/written even after cancelling the
descriptors).

Or, you add some more checks to this if statement to reset the BAM. This
could work I think:

	if (!list_empty(&bchan->desc_list)) {
		async_desc = list_first_entry(&bchan->desc_list,
					      struct bam_async_desc, desc_node);
		bam_chan_init_hw(bchan, async_desc->dir);
	} else if (bchan->bam_locked || bchan->head != bchan->tail) {
		/* BAM still locked or unlock still pending */
		bam_chan_init_hw(bchan, DMA_MEM_TO_DEV);
	}

Unfortunately, bchan->bam_locked alone doesn't tell us if the BAM is
locked, only if we haven't written the UNLOCK descriptor yet. It may be
pending in the FIFO. head != tail should tell us that the FIFO is not
empty (which should be the UNLOCK descriptor if desc_list is empty)...

>  
>  	vchan_dma_desc_free_list(&bchan->vc, &head);
> @@ -869,7 +932,7 @@ static int bam_resume(struct dma_chan *chan)
>  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_async_desc *async_desc;
>  
>  	srcs = readl_relaxed(bam_addr(bdev, 0, BAM_IRQ_SRCS_EE));
>  
> [...]
> @@ -919,13 +991,12 @@ 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.
>  			 */
> -			if (!async_desc->num_desc) {
> +			list_del(&async_desc->desc_node);
> +			if (!async_desc->num_desc)
>  				vchan_cookie_complete(&async_desc->vd);
> -			} else {
> +			else
>  				list_add(&async_desc->vd.node,
>  					 &bchan->vc.desc_issued);
> -			}
> -			list_del(&async_desc->desc_node);
>  		}

I think this is an unneeded/unrelated formatting change now. Drop?

>  	}
>  
> [...]
>  /**
>   * bam_start_dma - start next transaction
>   * @bchan: bam dma channel
>   */
>  static void bam_start_dma(struct bam_chan *bchan)
>  {
> -	struct virt_dma_desc *vd = vchan_next_desc(&bchan->vc);
> +	struct virt_dma_desc *vd;
>  	struct bam_device *bdev = bchan->bdev;
>  	struct bam_async_desc *async_desc = NULL;
>  	struct bam_desc_hw *desc;
> @@ -1064,22 +1145,34 @@ static void bam_start_dma(struct bam_chan *bchan)
>  
>  	lockdep_assert_held(&bchan->vc.lock);
>  
> -	if (!vd)
> +	vd = vchan_next_desc(&bchan->vc);
> +	if (IS_BUSY(bchan) || (!vd && !bchan->bam_locked))
>  		return;
>  
>  	ret = pm_runtime_get_sync(bdev->dev);
>  	if (ret < 0)
>  		return;
>  
> +	if (!bchan->initialized)
> +		bam_chan_init_hw(bchan, container_of(vd, struct bam_async_desc, vd)->dir);

Okay yeah, that was my bug. Thanks for moving it :-)

> +
> +	if (bchan->locking_enabled && !bchan->bam_locked) {
> +		/* Defer locking until we also have space for a data descriptor */
> +		avail = CIRC_SPACE(bchan->tail, bchan->head, MAX_DESCRIPTORS + 1);
> +		if (avail < 2) {
> +			queue_work(system_bh_highpri_wq, &bdev->work);
> +			goto out;
> +		}
> +
> +		bam_fifo_write_lock(bchan, DESC_FLAG_LOCK);
> +		bchan->bam_locked = true;
> +	}
> +
>  	while (vd && !IS_BUSY(bchan)) {
>  		list_del(&vd->node);
>  
>  		async_desc = container_of(vd, struct bam_async_desc, vd);
>  
> -		/* on first use, initialize the channel hardware */
> -		if (!bchan->initialized)
> -			bam_chan_init_hw(bchan, async_desc->dir);
> -
>  		/* apply new slave config changes, if necessary */
>  		if (bchan->reconfigure)
>  			bam_apply_new_config(bchan, async_desc->dir);
> @@ -1135,11 +1228,24 @@ static void bam_start_dma(struct bam_chan *bchan)
>  		list_add_tail(&async_desc->desc_node, &bchan->desc_list);
>  	}
>  
> +	/*
> +	 * Close the bracket once there is no more client work queued. The
> +	 * UNLOCK is flagged for an interrupt so process_channel_irqs() is
> +	 * guaranteed to observe its completion and retire it from the FIFO
> +	 * promptly, instead of leaving bchan->head to lag until a later
> +	 * bracket's skip-loop catches up with it.
> +	 */

Fair enough, I was a bit lazy here and omitted the DESC_FLAG_INT. It
should help with the head!=tail check I mentioned for
bam_dma_terminate_all() though.

> +	if (bchan->bam_locked && !vd && !IS_BUSY(bchan)) {
> +		bam_fifo_write_lock(bchan, DESC_FLAG_UNLOCK | DESC_FLAG_INT);
> +		bchan->bam_locked = false;
> +	}
> +
>  	/* ensure descriptor writes and dma start not reordered */
>  	wmb();
>  	writel_relaxed(bchan->tail * sizeof(struct bam_desc_hw),
>  			bam_addr(bdev, bchan->id, BAM_P_EVNT_REG));
>  
> +out:
>  	pm_runtime_mark_last_busy(bdev->dev);
>  	pm_runtime_put_autosuspend(bdev->dev);
>  }
> @@ -1162,7 +1268,13 @@ static void bam_dma_work(struct work_struct *work)
>  
>  		guard(spinlock_irqsave)(&bchan->vc.lock);
>  
> -		if (!list_empty(&bchan->vc.desc_issued) && !IS_BUSY(bchan))
> +		/*
> +		 * A channel also needs kicking if a bracket is still open
> +		 * (bam_locked) with no further client work queued: closing
> +		 * the UNLOCK requires a fresh call into bam_start_dma().
> +		 */
> +		if ((!list_empty(&bchan->vc.desc_issued) || bchan->bam_locked) &&
> +		    !IS_BUSY(bchan))
>  			bam_start_dma(bchan);

The if statement is redundant, bam_start_dma() now checks all of that
internally. 

	if (IS_BUSY(bchan) || (!vd && !bchan->bam_locked))
		return;

list_empty(desc_issed) == !vd

Same for !IS_BUSY() in bam_issue_pending().

Thanks,
Stephan