Re: [PATCH v5 12/17] dmaengine: sh: rz-dmac: Add cyclic DMA support

[Claudiu Beznea]

Hi, Frank,

On 5/13/26 01:00, Frank Li wrote:
> On Tue, May 12, 2026 at 03:12:13PM +0300, Claudiu Beznea wrote:
> > Add cyclic DMA support to the RZ DMAC driver. A per-channel status bit is
> > introduced to mark cyclic channels and is set during the DMA prepare
> > callback. The IRQ handler checks this status bit and calls
> > vchan_cyclic_callback() accordingly.
> >
> > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> > ---
> >
> > Changes in v5:
> > - none
> >
> > Changes in v4:
> > - drop the nxla update logic in rz_dmac_lmdesc_recycle() as this is
> >    not needed for any kind of transfers
> > - drop the update of channel->status = 0 from rz_dmac_free_chan_resources()
> >    and rz_dmac_terminate_all() as this was moved in patch 09/17
> >
> > Changes in v3:
> > - updated rz_dmac_lmdesc_recycle() to restore the lmdesc->nxla
> > - in rz_dmac_prepare_descs_for_cyclic() update directly the
> >    desc->start_lmdesc with the descriptor pointer insted of the
> >    descriptor address
> > - used rz_dmac_lmdesc_addr() to compute the descritor address
> > - set channel->status = 0 in rz_dmac_free_chan_resources()
> > - in rz_dmac_prep_dma_cyclic() check for invalid periods or buffer len
> >    and limit the critical area protected by spinlock
> > - set channel->status = 0 in rz_dmac_terminate_all()
> > - updated rz_dmac_calculate_residue_bytes_in_vd() to use
> >    rz_dmac_lmdesc_addr()
> > - dropped goto in rz_dmac_irq_handler_thread() as it is not needed
> >    anymore; dropped also the local variable desc
> >
> > Changes in v2:
> > - none
> >
> >   drivers/dma/sh/rz-dmac.c | 136 +++++++++++++++++++++++++++++++++++++--
> >   1 file changed, 130 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/dma/sh/rz-dmac.c b/drivers/dma/sh/rz-dmac.c
> > index 2de519b581b6..d6ad070be705 100644
> > --- a/drivers/dma/sh/rz-dmac.c
> > +++ b/drivers/dma/sh/rz-dmac.c
> > @@ -35,6 +35,7 @@
> >   enum  rz_dmac_prep_type {
> >   	RZ_DMAC_DESC_MEMCPY,
> >   	RZ_DMAC_DESC_SLAVE_SG,
> > +	RZ_DMAC_DESC_CYCLIC,
> >   };
> >
> >   struct rz_lmdesc {
> > @@ -67,9 +68,11 @@ struct rz_dmac_desc {
> >   /**
> >    * enum rz_dmac_chan_status: RZ DMAC channel status
> >    * @RZ_DMAC_CHAN_STATUS_PAUSED: Channel is paused though DMA engine callbacks
> > + * @RZ_DMAC_CHAN_STATUS_CYCLIC: Channel is cyclic
> >    */
> >   enum rz_dmac_chan_status {
> >   	RZ_DMAC_CHAN_STATUS_PAUSED,
> > +	RZ_DMAC_CHAN_STATUS_CYCLIC,
>
> suggest add new field bool iscycle in rz_dmac_chan.

I would prefer as it was proposed in this patch, if all good with everybody. In 
this way everything status related is packed in a single variable, struct 
rz_dmac_chan::status, and only a single cleanup operation is needed when the 
transactions are terminated.

> >   };
> >
> >   struct rz_dmac_chan {
> > @@ -191,6 +194,7 @@ struct rz_dmac {
> >
> >   /* LINK MODE DESCRIPTOR */
> >   #define HEADER_LV			BIT(0)
> > +#define HEADER_WBD			BIT(2)
> >
> >   #define RZ_DMAC_MAX_CHAN_DESCRIPTORS	16
> >   #define RZ_DMAC_MAX_CHANNELS		16
> > @@ -431,6 +435,57 @@ static void rz_dmac_prepare_descs_for_slave_sg(struct rz_dmac_chan *channel)
> >   	channel->chctrl = 0;
> >   }
> ...
> > +static struct dma_async_tx_descriptor *
> > +rz_dmac_prep_dma_cyclic(struct dma_chan *chan, dma_addr_t buf_addr,
> > +			size_t buf_len, size_t period_len,
> > +			enum dma_transfer_direction direction,
> > +			unsigned long flags)
> > +{
> > +	struct rz_dmac_chan *channel = to_rz_dmac_chan(chan);
> > +	struct rz_dmac_desc *desc;
> > +	size_t periods;
> > +
> > +	if (!is_slave_direction(direction))
> > +		return NULL;
> > +
> > +	if (!period_len || !buf_len)
> > +		return NULL;
> > +
> > +	periods = buf_len / period_len;
> > +	if (!periods || periods > DMAC_NR_LMDESC)
> > +		return NULL;
> > +
> > +	scoped_guard(spinlock_irqsave, &channel->vc.lock) {
> > +		if (channel->status & BIT(RZ_DMAC_CHAN_STATUS_CYCLIC))
> > +			return NULL;
> > +
> > +		desc = list_first_entry_or_null(&channel->ld_free, struct rz_dmac_desc, node);
>
> sugest use dma_pool manage desc, so ld_free can be removed.

Sure, but I would like to keep it aside from this set as it already big enough 
and I haven't noticed any potential issues with it.

--
Thank you,
Claudiu