Re: [PATCH/RFC] dmaengine: rcar-dmac: Use CAE/CAIR instead of error IRQ

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Magnus,

Thank you for the patch.

On Thursday 14 January 2016 19:16:53 Magnus Damm wrote:
> From: Magnus Damm <damm+renesas@opensource.se>
> 
> While using SYS-DMAC together with the IPMMU it became evident
> that the shared error interrupt hooked up by rcar-dmac.c never
> got invoked but instead the per-channel CAE bit got set which
> in turn may generate a per-channel interrupt if CAIE is set.
> 
> This patch removes the shared interrupt handler and instead
> simply enables CAIE and makes sure CAE gets cleared by the
> interrupt handler. Without enabling CAIE and clearing CAE the
> DMA transfer blocks forever in case of error.
> 
> During normal operation it is hard to trigger error interrupts,
> but I have managed to trigger the SYS-DMAC error by using local
> IPMMU debug code that tracks active page table entries using
> the AF bit. This debug code results in rather high latencies
> which in turn makes the SYS-DMAC generate error interrupts.
> 
> Tested on r8a7795 Salvator-X. Not for upstream merge - needs
> more discussion and testing on other SoCs.
> 
> Not-Yet-Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> ---
> 
>  drivers/dma/sh/rcar-dmac.c |   60 ++---------------------------------------
>  1 file changed, 3 insertions(+), 57 deletions(-)
> 
> --- 0001/drivers/dma/sh/rcar-dmac.c
> +++ work/drivers/dma/sh/rcar-dmac.c	2016-01-14 18:16:23.040513000 +0900
> @@ -808,23 +808,6 @@ static void rcar_dmac_stop(struct rcar_d
>  	rcar_dmac_write(dmac, RCAR_DMAOR, 0);
>  }
> 
> -static void rcar_dmac_abort(struct rcar_dmac *dmac)
> -{
> -	unsigned int i;
> -
> -	/* Stop all channels. */
> -	for (i = 0; i < dmac->n_channels; ++i) {
> -		struct rcar_dmac_chan *chan = &dmac->channels[i];
> -
> -		/* Stop and reinitialize the channel. */
> -		spin_lock(&chan->lock);
> -		rcar_dmac_chan_halt(chan);
> -		spin_unlock(&chan->lock);
> -
> -		rcar_dmac_chan_reinit(chan);
> -	}
> -}
> -
>  /*
> ---------------------------------------------------------------------------
> -- * Descriptors preparation
>   */
> @@ -864,7 +847,7 @@ static void rcar_dmac_chan_configure_des
>  	}
> 
>  	desc->xfer_shift = ilog2(xfer_size);
> -	desc->chcr = chcr | chcr_ts[desc->xfer_shift];
> +	desc->chcr = chcr | chcr_ts[desc->xfer_shift] | RCAR_DMACHCR_CAIE;

I'd rather let desc->chcr store the channel-specific configuration as 
documented in the rcar_dmac_desc structure kerneldoc and set the CAIE flag at 
the start of the rcar_dmac_chan_start_xfer() function with

        u32 chcr = desc->chcr | RCAR_DMACHCR_CAIE;

>  }
> 
>  /*
> @@ -1427,6 +1410,8 @@ static irqreturn_t rcar_dmac_isr_channel
>  	spin_lock(&chan->lock);
> 
>  	chcr = rcar_dmac_chan_read(chan, RCAR_DMACHCR);
> +	if (chcr & RCAR_DMACHCR_CAE)
> +		mask |= RCAR_DMACHCR_CAE;

How about setting the CAE flag unconditionally at the beginning of the 
function with

        u32 mask = RCAR_DMACHCR_CAE | RCAR_DMACHCR_DSE | RCAR_DMACHCR_TE;

>  	if (chcr & RCAR_DMACHCR_TE)
>  		mask |= RCAR_DMACHCR_DE;
>  	rcar_dmac_chan_write(chan, RCAR_DMACHCR, chcr & ~mask);

Don't you also need to act on the CAE flag being set ? Otherwise the transfer 
will just hang.

> @@ -1497,24 +1482,6 @@ static irqreturn_t rcar_dmac_isr_channel
>  	return IRQ_HANDLED;
>  }
> 
> -static irqreturn_t rcar_dmac_isr_error(int irq, void *data)
> -{
> -	struct rcar_dmac *dmac = data;
> -
> -	if (!(rcar_dmac_read(dmac, RCAR_DMAOR) & RCAR_DMAOR_AE))
> -		return IRQ_NONE;
> -
> -	/*
> -	 * An unrecoverable error occurred on an unknown channel. Halt the DMAC,
> -	 * abort transfers on all channels, and reinitialize the DMAC.
> -	 */
> -	rcar_dmac_stop(dmac);
> -	rcar_dmac_abort(dmac);
> -	rcar_dmac_init(dmac);
> -
> -	return IRQ_HANDLED;
> -}
> -
>  /*
> ---------------------------------------------------------------------------
> -- * OF xlate and channel filter
>   */
> @@ -1693,8 +1660,6 @@ static int rcar_dmac_probe(struct platfo
>  	struct rcar_dmac *dmac;
>  	struct resource *mem;
>  	unsigned int i;
> -	char *irqname;
> -	int irq;
>  	int ret;
> 
>  	dmac = devm_kzalloc(&pdev->dev, sizeof(*dmac), GFP_KERNEL);
> @@ -1732,25 +1697,6 @@ static int rcar_dmac_probe(struct platfo
>  	if (IS_ERR(dmac->iomem))
>  		return PTR_ERR(dmac->iomem);
> 
> -	irq = platform_get_irq_byname(pdev, "error");
> -	if (irq < 0) {
> -		dev_err(&pdev->dev, "no error IRQ specified\n");
> -		return -ENODEV;
> -	}
> -
> -	irqname = devm_kasprintf(dmac->dev, GFP_KERNEL, "%s:error",
> -				 dev_name(dmac->dev));
> -	if (!irqname)
> -		return -ENOMEM;
> -
> -	ret = devm_request_irq(&pdev->dev, irq, rcar_dmac_isr_error, 0,
> -			       irqname, dmac);
> -	if (ret) {
> -		dev_err(&pdev->dev, "failed to request IRQ %u (%d)\n",
> -			irq, ret);
> -		return ret;
> -	}
> -
>  	/* Enable runtime PM and initialize the device. */
>  	pm_runtime_enable(&pdev->dev);
>  	ret = pm_runtime_get_sync(&pdev->dev);

-- 
Regards,

Laurent Pinchart