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

[Claudiu Beznea <claudiu.beznea@kernel.org>]

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@bp.renesas.com>
>> ---
>>
>> 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