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

[Vinod Koul <vkoul@kernel.org>]

On 15-07-26, 14:43, Bartosz Golaszewski wrote:
> On Tue, 14 Jul 2026 11:49:14 +0200, Stephan Gerhold
> <stephan.gerhold@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@kernel.org>
> >> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@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.

The concept is that a dmaengine can handle multiple peripherals, so the
peripheral is always per desciptor

> 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.

metadata seems better fit for this.

> >>  /**
> >>   * 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?

Good point

-- 
~Vinod