Re: [PATCH v4 05/17] dmaengine: sh: rz-dmac: Do not disable the channel on error

[Claudiu Beznea]

On 4/11/26 15:30, Biju Das wrote:
> > -----Original Message-----
> > From: Claudiu <claudiu.beznea@xxxxxxxxx>
> > Sent: 11 April 2026 12:43
> -soc@xxxxxxxxxxxxxxx; Claudiu Beznea
> > <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> > Subject: [PATCH v4 05/17] dmaengine: sh: rz-dmac: Do not disable the channel on error
> >
> > From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> >
> > Disabling the channel on error is pointless, as if other transfers are queued, the IRQ thread will be
> > woken up and will execute them anyway by calling rz_dmac_xfer_desc().
> >
> > rz_dmac_xfer_desc() re-enables the transfer. Before doing so, it sets CHCTRL.SWRST, which clears
> > CHSTAT.DER and CHSTAT.END anyway.
> >
> > Skip disabling the DMA channel and just log the error instead.
> >
> > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> > ---
> >
> > Changes in v4:
> > - none
> >
> > Changes in v3:
> > - none, this patch is new
> >
> >   drivers/dma/sh/rz-dmac.c | 4 ----
> >   1 file changed, 4 deletions(-)
> >
> > diff --git a/drivers/dma/sh/rz-dmac.c b/drivers/dma/sh/rz-dmac.c index 40ddf534c094..943c005f52bd
> > 100644
> > --- a/drivers/dma/sh/rz-dmac.c
> > +++ b/drivers/dma/sh/rz-dmac.c
> > @@ -871,10 +871,6 @@ static void rz_dmac_irq_handle_channel(struct rz_dmac_chan *channel)
> >   	if (chstat & CHSTAT_ER) {
> >   		dev_err(dmac->dev, "DMAC err CHSTAT_%d = %08X\n",
> >   			channel->index, chstat);
> > -
> > -		scoped_guard(spinlock_irqsave, &channel
> ->vc.lock)
> > -			rz_dmac_disable_hw(channel);
>
> On previous patch, rz_dmac_disable_hw() for initializing each register
>
> +	/* Initialize register for each channel */
> +	rz_dmac_disable_hw(channel);

This initializes a single register by clearing various bits.

> As per hardware manual,
>
> Once an error occurs, the data of the whole transfer cannot be guaranteed.
> Be sure to start the transaction again from the
> beginning by following the procedure below.
> 1. Set 1 in the SWRST bit of the CHCTRL_n/nS register.
> 2. Set each register again.

I wasn't aware of this sequence. Thank for pointing it. However, calling 
rz_dmac_disable_hw() as it previously was may be wrong from my point of view. 
According to the sequence you pointed, I think the code here should have only 
set the SWRST, if any, and let the rz_dmac_xfer_desc() "set each register 
again". According to "Figure 14.26 Setting Example 4", of RZ/G3S HW manual, rev 
1.20, the registers that need to be set when starting DMAC ch in Link mode are:

- DCTRL = 0x1
- NXLA = 0x1000
- CHCFG = 0x80000000
- CHCTRL = 0x8 // swreset
- CHCTRL = 0x5 // enable

So, I think these are the registers that need to be re-configured again (handled 
though the rz_dmac_xfer_desc()).

Anyway, I'll drop this patch from the next version, as it is not the subject of 
cyclic DMA.

Thank you,
Claudiu