Hi Magnus,
Thank you for the patch.
On Thursday 14 January 2016 19:16:53 Magnus Damm wrote:
> From: Magnus Damm <damm+rene...@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+rene...@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
