| Message ID | 20190305055628.11826-2-dirk.behme@de.bosch.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/2] dmaengine: sh: rcar-dmac: With cyclic DMA residue 0 is valid | expand |
Hi Renesas SoC team, On 05.03.2019 06:56, Dirk Behme wrote: > From: Achim Dahlhoff <Achim.Dahlhoff@de.bosch.com> > > The tx_status poll in the rcar_dmac driver reads the status register > which indicates which chunk is busy (DMACHCRB). Afterwards the point > inside the chunk is read from DMATCRB. It is possible that the chunk > has changed between the two reads. The result is a non-monotonous > increase of the residue. Fix this by introducing a 'safe read' logic. > > Fixes: 73a47bd0da66 ("dmaengine: rcar-dmac: use TCRB instead of TCR for residue") > Signed-off-by: Achim Dahlhoff <Achim.Dahlhoff@de.bosch.com> > Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com> > Cc: <stable@vger.kernel.org> # v4.16+ > --- > Note: Patch done against mainline v5.0 > > Changes in v2: Switch goto/retry to for loop Any status update on this and the first fix [PATCH v2 1/2] dmaengine: sh: rcar-dmac: With cyclic DMA residue 0 is valid ? I still think they are required to fix the rcar-dmac driver. Best regards Dirk > drivers/dma/sh/rcar-dmac.c | 26 +++++++++++++++++++++++--- > 1 file changed, 23 insertions(+), 3 deletions(-) > > diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c > index 2ea59229d7f5..cceddc7099b0 100644 > --- a/drivers/dma/sh/rcar-dmac.c > +++ b/drivers/dma/sh/rcar-dmac.c > @@ -1282,6 +1282,9 @@ static unsigned int rcar_dmac_chan_get_residue(struct rcar_dmac_chan *chan, > enum dma_status status; > unsigned int residue = 0; > unsigned int dptr = 0; > + unsigned int chcrb; > + unsigned int tcrb; > + unsigned int i; > > if (!desc) > return 0; > @@ -1329,6 +1332,24 @@ static unsigned int rcar_dmac_chan_get_residue(struct rcar_dmac_chan *chan, > return 0; > } > > + /* > + * We need to read two registers. > + * Make sure the control register does not skip to next chunk > + * while reading the counter. > + * Trying it 3 times should be enough: Initial read, retry, retry > + * for the paranoid. > + */ > + for (i = 0; i < 3; i++) { > + chcrb = rcar_dmac_chan_read(chan, RCAR_DMACHCRB) & > + RCAR_DMACHCRB_DPTR_MASK; > + tcrb = rcar_dmac_chan_read(chan, RCAR_DMATCRB); > + /* Still the same? */ > + if (chcrb == (rcar_dmac_chan_read(chan, RCAR_DMACHCRB) & > + RCAR_DMACHCRB_DPTR_MASK)) > + break; > + } > + WARN_ONCE(i >= 3, "residue might be not continuous!"); > + > /* > * In descriptor mode the descriptor running pointer is not maintained > * by the interrupt handler, find the running descriptor from the > @@ -1336,8 +1357,7 @@ static unsigned int rcar_dmac_chan_get_residue(struct rcar_dmac_chan *chan, > * mode just use the running descriptor pointer. > */ > if (desc->hwdescs.use) { > - dptr = (rcar_dmac_chan_read(chan, RCAR_DMACHCRB) & > - RCAR_DMACHCRB_DPTR_MASK) >> RCAR_DMACHCRB_DPTR_SHIFT; > + dptr = chcrb >> RCAR_DMACHCRB_DPTR_SHIFT; > if (dptr == 0) > dptr = desc->nchunks; > dptr--; > @@ -1355,7 +1375,7 @@ static unsigned int rcar_dmac_chan_get_residue(struct rcar_dmac_chan *chan, > } > > /* Add the residue for the current chunk. */ > - residue += rcar_dmac_chan_read(chan, RCAR_DMATCRB) << desc->xfer_shift; > + residue += tcrb << desc->xfer_shift; > > return residue; > }
diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c index 2ea59229d7f5..cceddc7099b0 100644 --- a/drivers/dma/sh/rcar-dmac.c +++ b/drivers/dma/sh/rcar-dmac.c @@ -1282,6 +1282,9 @@ static unsigned int rcar_dmac_chan_get_residue(struct rcar_dmac_chan *chan, enum dma_status status; unsigned int residue = 0; unsigned int dptr = 0; + unsigned int chcrb; + unsigned int tcrb; + unsigned int i; if (!desc) return 0; @@ -1329,6 +1332,24 @@ static unsigned int rcar_dmac_chan_get_residue(struct rcar_dmac_chan *chan, return 0; } + /* + * We need to read two registers. + * Make sure the control register does not skip to next chunk + * while reading the counter. + * Trying it 3 times should be enough: Initial read, retry, retry + * for the paranoid. + */ + for (i = 0; i < 3; i++) { + chcrb = rcar_dmac_chan_read(chan, RCAR_DMACHCRB) & + RCAR_DMACHCRB_DPTR_MASK; + tcrb = rcar_dmac_chan_read(chan, RCAR_DMATCRB); + /* Still the same? */ + if (chcrb == (rcar_dmac_chan_read(chan, RCAR_DMACHCRB) & + RCAR_DMACHCRB_DPTR_MASK)) + break; + } + WARN_ONCE(i >= 3, "residue might be not continuous!"); + /* * In descriptor mode the descriptor running pointer is not maintained * by the interrupt handler, find the running descriptor from the @@ -1336,8 +1357,7 @@ static unsigned int rcar_dmac_chan_get_residue(struct rcar_dmac_chan *chan, * mode just use the running descriptor pointer. */ if (desc->hwdescs.use) { - dptr = (rcar_dmac_chan_read(chan, RCAR_DMACHCRB) & - RCAR_DMACHCRB_DPTR_MASK) >> RCAR_DMACHCRB_DPTR_SHIFT; + dptr = chcrb >> RCAR_DMACHCRB_DPTR_SHIFT; if (dptr == 0) dptr = desc->nchunks; dptr--; @@ -1355,7 +1375,7 @@ static unsigned int rcar_dmac_chan_get_residue(struct rcar_dmac_chan *chan, } /* Add the residue for the current chunk. */ - residue += rcar_dmac_chan_read(chan, RCAR_DMATCRB) << desc->xfer_shift; + residue += tcrb << desc->xfer_shift; return residue; }