Hi Laurent,
Thank you for the review. Please see my comments below. Also please
let me know if this code needs to be fixed on somewhat urgent basis
since I am a little occupied with few other tasks. I'd try to
prioritize it then
Best regards,
Hamza
On Thu, Oct 15, 2015 at 5:58 PM, Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
> Hello Muhammad,
>
> Thank you for the patch.
>
> On Tuesday 29 September 2015 22:44:43 hamzahfrq.sub@xxxxxxxxx wrote:
>> From: Muhammad Hamza Farooq <mfarooq@xxxxxxxxxxx>
>>
>> DMA engine does not stop instantaneously when transaction is going on
>> (see datasheet). Wait has been added
>>
>> Signed-off-by: Muhammad Hamza Farooq <mfarooq@xxxxxxxxxxx>
>> ---
>> drivers/dma/sh/rcar-dmac.c | 28 ++++++++++++++++++++++++++--
>> 1 file changed, 26 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
>> index 7820d07..2b28291 100644
>> --- a/drivers/dma/sh/rcar-dmac.c
>> +++ b/drivers/dma/sh/rcar-dmac.c
>> @@ -716,14 +716,38 @@ static int rcar_dmac_fill_hwdesc(struct rcar_dmac_chan
>> *chan, /*
>> ---------------------------------------------------------------------------
>> * Stop and reset
>> */
>> +#define NR_READS_TO_WAIT 5 /* number of times to check if DE = 0 */
>> +static inline int rcar_dmac_wait_stop(struct rcar_dmac_chan *chan)
>> +{
>> + unsigned int i = 0;
>> +
>> + do {
>> + u32 chcr = rcar_dmac_chan_read(chan, RCAR_DMACHCR);
>> +
>> + if (!(chcr & RCAR_DMACHCR_DE))
>> + return 0;
>> + cpu_relax();
>> + dev_dbg(chan->chan.device->dev, "DMA xfer couldn't be stopped");
>
> Would it make sense to move the message out of the loop and use dev_err() ? It
> seems like a pretty serious error.
It happens quite often so dev_err might be too noisy. After couple
attempts (1-3) it stops so I think it is better to not make it dev_err
>
>> + } while (++i < NR_READS_TO_WAIT);
>
> How long does the DMA engine typically need to stop ? Is there a safe upper
> bound ?
I have tested it many times. The highest number of retries was 3 which
occurred not very frequently. Normally it is 1-2
>
>> -static void rcar_dmac_chan_halt(struct rcar_dmac_chan *chan)
>> + return -EBUSY;
>> +}
>> +
>> +/* Called with chan lock held */
>> +static int rcar_dmac_chan_halt(struct rcar_dmac_chan *chan)
>> {
>> - u32 chcr = rcar_dmac_chan_read(chan, RCAR_DMACHCR);
>> + u32 chcr;
>> + int ret;
>>
>> + chcr = rcar_dmac_chan_read(chan, RCAR_DMACHCR);
>> chcr &= ~(RCAR_DMACHCR_DSE | RCAR_DMACHCR_DSIE | RCAR_DMACHCR_IE |
>> RCAR_DMACHCR_TE | RCAR_DMACHCR_DE);
>> rcar_dmac_chan_write(chan, RCAR_DMACHCR, chcr);
>> + ret = rcar_dmac_wait_stop(chan);
>
> As the rcar_dmac_wait_stop() function is used here only I'd inline the code
> directly.
It is used in DMA_PAUSE operation as well
>
>> +
>> + WARN_ON(ret < 0);
>
> If you use dev_err() instead of dev_dbg() above you could remove the WARN_on.
see comment about dev_err above
>
>> +
>> + return ret;
>> }
>>
>> static void rcar_dmac_chan_reinit(struct rcar_dmac_chan *chan)
>
> --
> Regards,
>
> Laurent Pinchart
>
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
