Re: [PATCH v8 7/8] dmaengine: sh: rz-dmac: Add device_tx_status() callback

[Frank Li]

On Mon, Mar 02, 2026 at 03:49:54PM +0200, Claudiu Beznea wrote:
> Hi, Frank,
>
> On 2/25/26 18:43, Frank Li wrote:
> > On Tue, Jan 20, 2026 at 03:33:29PM +0200, Claudiu wrote:
> > > From: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > >
> > > Add support for device_tx_status() callback as it is needed for
> > > RZ/G2L SCIFA driver.
> > >
> > > Based on a patch in the BSP similar to rcar-dmac by
> > > Long Luu <long.luu.ur@xxxxxxxxxxx>.
> >
> > If you want to give credit to Long Luu, any public link?
>
> No public link as far as I'm aware. Anyway, I'll add his SoB + Co-developed-by.
>
> >
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > > [claudiu.beznea:
> > >   - post-increment lmdesc in rz_dmac_get_next_lmdesc() to allow the next
> > >     pointer to advance
> > >   - use 'lmdesc->nxla != crla' comparison instead of
> > >     '!(lmdesc->nxla == crla)' in rz_dmac_calculate_residue_bytes_in_vd()
> > >   - in rz_dmac_calculate_residue_bytes_in_vd() use '++i >= DMAC_NR_LMDESC'
> > >     to verify if the full lmdesc list was checked
> > >   - drop rz_dmac_calculate_total_bytes_in_vd() and use desc->len instead
> > >   - re-arranged comments so they span fewer lines and are wrapped to ~80
> > >     characters
> > >   - use u32 for the residue value and the functions returning it
> > >   - use u32 for the variables storing register values
> > >   - fixed typos]
> >
> > Suppose needn't this section
>
> Just followed the process. I can drop it.
>
> >
> > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> > > ---
> > >
> > > Changes in v8:
> > > - populated engine->residue_granularity
> > >
> > > Changes in v7:
> > > - none
> > >
> > > Changes in v6:
> > > - s/byte/bytes in comment from rz_dmac_chan_get_residue()
> > >
> > > Changes in v5:
> > > - post-increment lmdesc in rz_dmac_get_next_lmdesc() to allow the next
> > >    pointer to advance
> > > - use 'lmdesc->nxla != crla' comparison instead of
> > >    '!(lmdesc->nxla == crla)' in rz_dmac_calculate_residue_bytes_in_vd()
> > > - in rz_dmac_calculate_residue_bytes_in_vd() use '++i >= DMAC_NR_LMDESC'
> > >    to verify if the full lmdesc list was checked
> > > - drop rz_dmac_calculate_total_bytes_in_vd() and use desc->len instead
> > > - re-arranged comments so they span fewer lines and are wrapped to ~80
> > >    characters
> > > - use u32 for the residue value and the functions returning it
> > > - use u32 for the variables storing register values
> > > - fixed typos
> > >
> > >   drivers/dma/sh/rz-dmac.c | 145 ++++++++++++++++++++++++++++++++++++++-
> > >   1 file changed, 144 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/dma/sh/rz-dmac.c b/drivers/dma/sh/rz-dmac.c
> > > index 4602f8b7408a..27c963083e29 100644
> > > --- a/drivers/dma/sh/rz-dmac.c
> > > +++ b/drivers/dma/sh/rz-dmac.c
> > > @@ -125,10 +125,12 @@ struct rz_dmac {
> > >    * Registers
> > >    */
> > >
> > > +#define CRTB				0x0020
> > >   #define CHSTAT				0x0024
> > >   #define CHCTRL				0x0028
> > >   #define CHCFG				0x002c
> > >   #define NXLA				0x0038
> > > +#define CRLA				0x003c
> > >
> > >   #define DCTRL				0x0000
> > >
> > > @@ -684,6 +686,146 @@ static void rz_dmac_device_synchronize(struct dma_chan *chan)
> > >   	rz_dmac_set_dma_req_no(dmac, channel->index, dmac->info->default_dma_req_no);
> > >   }
> > >
> > > +static struct rz_lmdesc *
> > > +rz_dmac_get_next_lmdesc(struct rz_lmdesc *base, struct rz_lmdesc *lmdesc)
> > > +{
> > > +	struct rz_lmdesc *next = ++lmdesc;
> > > +
> > > +	if (next >= base + DMAC_NR_LMDESC)
> > > +		next = base;
> > > +
> > > +	return next;
> > > +}
> > > +
> > > +static u32 rz_dmac_calculate_residue_bytes_in_vd(struct rz_dmac_chan *channel)
> > > +{
> > > +	struct rz_lmdesc *lmdesc = channel->lmdesc.head;
> > > +	struct dma_chan *chan = &channel->vc.chan;
> > > +	struct rz_dmac *dmac = to_rz_dmac(chan->device);
> > > +	u32 residue = 0, crla, i = 0;
> > > +
> > > +	crla = rz_dmac_ch_readl(channel, CRLA, 1);
> > > +	while (lmdesc->nxla != crla) {
> > > +		lmdesc = rz_dmac_get_next_lmdesc(channel->lmdesc.base, lmdesc);
> > > +		if (++i >= DMAC_NR_LMDESC)
> > > +			return 0;
> > > +	}
> > > +
> > > +	/* Calculate residue from next lmdesc to end of virtual desc */
> > > +	while (lmdesc->chcfg & CHCFG_DEM) {
> > > +		residue += lmdesc->tb;
> > > +		lmdesc = rz_dmac_get_next_lmdesc(channel->lmdesc.base, lmdesc);
> > > +	}
> >
> > can use one loop
> >
> > for (int i=0; i<DMAC_NR_LMDESC; i++) {
> > 	if (lmdesc->nxla == crla)
> > 		residue = 0; 	//reset to 0;
> >
> > 	if (lmdesc->chcfg & CHCFG_DEM)
> > 		residue += lmdesc->tb;
> >
> > 	lmdesc = rz_dmac_get_next_lmdesc(channel->lmdesc.base, lmdesc);
>
> I'm not sure this will work as the descriptors list is cyclic and resetting
> the residue to zero when lmdesc->nxla == crla and then start acumulating
> from there will not work if there are descriptors enqueued for the current
> transfers at the end and the beginning of the list, e.g:
>
> descriptors list:
>
> | d3 | d5 | d6 | ... | d0 | d1 | d2 |
>
> ^				    ^
> start			 	   end
> (index 0)			(index DMAC_NR_LMDESC-1)

Okay, you are right.

Frank
>
> > }
> >
> > return residue;
> >
> > > +
> > > +	dev_dbg(dmac->dev, "%s: VD residue is %u\n", __func__, residue);
> > > +
> > > +	return residue;
> > > +}
> > > +
> > > +static u32 rz_dmac_chan_get_residue(struct rz_dmac_chan *channel,
> > > +				    dma_cookie_t cookie)
> > > +{
> > > +	struct rz_dmac_desc *current_desc, *desc;
> > > +	enum dma_status status;
> > > +	u32 crla, crtb, i;
> > > +
> > > +	/* Get current processing virtual descriptor */
> > > +	current_desc = list_first_entry(&channel->ld_active,
> > > +					struct rz_dmac_desc, node);
> > > +	if (!current_desc)
> > > +		return 0;
> > > +
> > > +	/*
> > > +	 * If the cookie corresponds to a descriptor that has been completed
> > > +	 * there is no residue. The same check has already been performed by the
> > > +	 * caller but without holding the channel lock, so the descriptor could
> > > +	 * now be complete.
> > > +	 */
> > > +	status = dma_cookie_status(&channel->vc.chan, cookie, NULL);
> > > +	if (status == DMA_COMPLETE)
> > > +		return 0;
> > > +
> > > +	/*
> > > +	 * If the cookie doesn't correspond to the currently processing virtual
> > > +	 * descriptor then the descriptor hasn't been processed yet, and the
> > > +	 * residue is equal to the full descriptor size. Also, a client driver
> > > +	 * is possible to call this function before rz_dmac_irq_handler_thread()
> > > +	 * runs. In this case, the running descriptor will be the next
> > > +	 * descriptor, and will appear in the done list. So, if the argument
> > > +	 * cookie matches the done list's cookie, we can assume the residue is
> > > +	 * zero.
> > > +	 */
> > > +	if (cookie != current_desc->vd.tx.cookie) {
> > > +		list_for_each_entry(desc, &channel->ld_free, node) {
> > > +			if (cookie == desc->vd.tx.cookie)
> > > +				return 0;
> > > +		}
> > > +
> > > +		list_for_each_entry(desc, &channel->ld_queue, node) {
> > > +			if (cookie == desc->vd.tx.cookie)
> > > +				return desc->len;
> > > +		}
> > > +
> > > +		list_for_each_entry(desc, &channel->ld_active, node) {
> > > +			if (cookie == desc->vd.tx.cookie)
> > > +				return desc->len;
> > > +		}
> > > +
> > > +		/*
> > > +		 * No descriptor found for the cookie, there's thus no residue.
> > > +		 * This shouldn't happen if the calling driver passes a correct
> > > +		 * cookie value.
> > > +		 */
> > > +		WARN(1, "No descriptor for cookie!");
> > > +		return 0;
> > > +	}
> > > +
> > > +	/*
> > > +	 * We need to read two registers. Make sure the hardware does not move
> > > +	 * to next lmdesc while reading the current lmdesc. Trying it 3 times
> > > +	 * should be enough: initial read, retry, retry for the paranoid.
> > > +	 */
> > > +	for (i = 0; i < 3; i++) {
> > > +		crla = rz_dmac_ch_readl(channel, CRLA, 1);
> > > +		crtb = rz_dmac_ch_readl(channel, CRTB, 1);
> > > +		/* Still the same? */
> > > +		if (crla == rz_dmac_ch_readl(channel, CRLA, 1))
> > > +			break;
> > > +	}
> > > +
> > > +	WARN_ONCE(i >= 3, "residue might not be continuous!");
> > > +
> > > +	/*
> > > +	 * Calculate number of bytes transferred in processing virtual descriptor.
> > > +	 * One virtual descriptor can have many lmdesc.
> > > +	 */
> > > +	return crtb + rz_dmac_calculate_residue_bytes_in_vd(channel);
> >
> > you don't use varible 'ctra' here, so retry 3 become useless. suppose
> > rz_dmac_calculate_residue_bytes_in_vd(channel, ctra)
> >
> > and avoid rz_dmac_ch_readl(channel, CRLA, 1) in
> > rz_dmac_calculate_residue_bytes_in_vd() to keep ctra and ctrb reflect the
> > correct hardware state.
>
> Good point, I'll update it.
>
> Thank you for your review,
> Claudiu