diff mbox

[10/13] powerpc/5200: LocalPlus driver: fix problem caused by unpredictable IRQ order

Message ID 200912220809.09578.roman.fietze@telemotive.de (mailing list archive)
State Changes Requested
Delegated to: Grant Likely
Headers show

Commit Message

Roman Fietze Dec. 22, 2009, 7:09 a.m. UTC
The order of the raised interrupts of SCLPC and BCOM cannot be
predicted, because it depends on the individual BCOM and CPU loads. So
in DMA mode we just wait for both until we finish the transaction.

Signed-off-by: Roman Fietze <roman.fietze@telemotive.de>
---
 arch/powerpc/platforms/52xx/mpc52xx_lpbfifo.c |   94 +++++++++++++++++--------
 1 files changed, 64 insertions(+), 30 deletions(-)

Comments

Grant Likely Jan. 11, 2010, 8:19 p.m. UTC | #1
On Tue, Dec 22, 2009 at 12:09 AM, Roman Fietze
<roman.fietze@telemotive.de> wrote:
>
> The order of the raised interrupts of SCLPC and BCOM cannot be
> predicted, because it depends on the individual BCOM and CPU loads. So
> in DMA mode we just wait for both until we finish the transaction.

I'm really not convinced.  It is true that the IRQ ordering may be
different, but by definition the BCOM *must* be finished before the
FIFO finishes on the TX path, and the FIFO definitely completes before
the BCOM completes on the RX path, regardless of the order IRQs are
actually processed.

g.

>
> Signed-off-by: Roman Fietze <roman.fietze@telemotive.de>
> ---
>  arch/powerpc/platforms/52xx/mpc52xx_lpbfifo.c |   94 +++++++++++++++++--------
>  1 files changed, 64 insertions(+), 30 deletions(-)
>
> diff --git a/arch/powerpc/platforms/52xx/mpc52xx_lpbfifo.c b/arch/powerpc/platforms/52xx/mpc52xx_lpbfifo.c
> index 21b2a40..cd8dc69 100644
> --- a/arch/powerpc/platforms/52xx/mpc52xx_lpbfifo.c
> +++ b/arch/powerpc/platforms/52xx/mpc52xx_lpbfifo.c
> @@ -32,7 +32,7 @@ struct mpc52xx_lpbfifo {
>        struct device *dev;
>        phys_addr_t regs_phys;
>        struct mpc52xx_sclpc __iomem *regs;
> -       int irq;
> +       int sclpc_irq;
>        spinlock_t lock;
>
>        struct bcom_task *bcom_tx_task;
> @@ -41,6 +41,7 @@ struct mpc52xx_lpbfifo {
>
>        /* Current state data */
>        struct mpc52xx_lpbfifo_request *req;
> +       unsigned short irqs_pending;
>        int dma_irqs_enabled;
>  };
>
> @@ -48,6 +49,14 @@ struct mpc52xx_lpbfifo {
>  static struct mpc52xx_lpbfifo lpbfifo;
>
>
> +/* The order of the raised interrupts of SCLPC and BCOM cann not be
> + * predicted, because it depends on the individual BCOM and CPU
> + * loads. So in DMA mode we just wait for both until we finish the
> + * transaction. */
> +#define MPC52XX_LPBFIFO_PENDING_SCLPC  BIT(0)
> +#define MPC52XX_LPBFIFO_PENDING_BCOM   BIT(1)
> +
> +
>  /**
>  * mpc52xx_lpbfifo_is_write - return true if it's a WRITE request
>  */
> @@ -127,6 +136,8 @@ static void mpc52xx_lpbfifo_kick(struct mpc52xx_lpbfifo_request *req)
>                                out_be32(reg, *data++);
>                }
>
> +               lpbfifo.irqs_pending = MPC52XX_LPBFIFO_PENDING_SCLPC;
> +
>                /* Unmask both error and completion irqs */
>                out_be32(&lpbfifo.regs->enable, (MPC52xx_SCLPC_ENABLE_AIE |
>                                                 MPC52xx_SCLPC_ENABLE_NIE |
> @@ -172,6 +183,8 @@ static void mpc52xx_lpbfifo_kick(struct mpc52xx_lpbfifo_request *req)
>                /* error irq & master enabled bit */
>                out_be32(&lpbfifo.regs->enable, MPC52xx_SCLPC_ENABLE_AIE | MPC52xx_SCLPC_ENABLE_NIE | MPC52xx_SCLPC_ENABLE_ME);
>
> +               lpbfifo.irqs_pending = MPC52XX_LPBFIFO_PENDING_BCOM | MPC52XX_LPBFIFO_PENDING_SCLPC;
> +
>                bd = bcom_prepare_next_buffer(lpbfifo.bcom_cur_task);
>                bd->status = tc;
>                bd->data[0] = req->data_dma + req->pos;
> @@ -188,6 +201,7 @@ static void mpc52xx_lpbfifo_kick(struct mpc52xx_lpbfifo_request *req)
>                bcom_enable(lpbfifo.bcom_cur_task);
>  }
>
> +
>  /**
>  * mpc52xx_lpbfifo_sclpc_irq - IRQ handler for LPB FIFO
>  *
> @@ -232,8 +246,9 @@ static void mpc52xx_lpbfifo_kick(struct mpc52xx_lpbfifo_request *req)
>  */
>  static irqreturn_t mpc52xx_lpbfifo_sclpc_irq(int irq, void *dev_id)
>  {
> +       struct mpc52xx_lpbfifo *lpbfifo = dev_id;
>        struct mpc52xx_lpbfifo_request *req;
> -       u32 status_count = in_be32(&lpbfifo.regs->bytes_done_status.bytes_done);
> +       u32 status_count = in_be32(&lpbfifo->regs->bytes_done_status.bytes_done);
>        void __iomem *reg;
>        u32 *data;
>        size_t i;
> @@ -242,18 +257,20 @@ static irqreturn_t mpc52xx_lpbfifo_sclpc_irq(int irq, void *dev_id)
>        unsigned long flags;
>        int rflags;
>
> -       spin_lock_irqsave(&lpbfifo.lock, flags);
> +       spin_lock_irqsave(&lpbfifo->lock, flags);
>        ts = get_tbl();
>
> -       req = lpbfifo.req;
> +       req = lpbfifo->req;
>        if (!req) {
> -               spin_unlock_irqrestore(&lpbfifo.lock, flags);
> +               spin_unlock_irqrestore(&lpbfifo->lock, flags);
>                pr_err("bogus SCLPC IRQ\n");
>                return IRQ_HANDLED;
>        }
>
> +       lpbfifo->irqs_pending &= ~MPC52XX_LPBFIFO_PENDING_SCLPC;
> +
>        rflags = req->flags;
> -       status_count = in_be32(&lpbfifo.regs->bytes_done_status.bytes_done);
> +       status_count = in_be32(&lpbfifo->regs->bytes_done_status.bytes_done);
>
>        /* Check normal termination bit */
>        if (!(status_count & MPC52xx_SCLPC_STATUS_NT))
> @@ -261,19 +278,23 @@ static irqreturn_t mpc52xx_lpbfifo_sclpc_irq(int irq, void *dev_id)
>
>        /* Check abort bit */
>        if (status_count & MPC52xx_SCLPC_STATUS_AT) {
> -               out_be32(&lpbfifo.regs->enable, MPC52xx_SCLPC_ENABLE_RC | MPC52xx_SCLPC_ENABLE_RF);
> +               out_be32(&lpbfifo->regs->enable, MPC52xx_SCLPC_ENABLE_RC | MPC52xx_SCLPC_ENABLE_RF);
>                do_callback = 1;
>                goto out;
>        }
>
> -       if (!mpc52xx_lpbfifo_is_dma(rflags)) {
> +       if (mpc52xx_lpbfifo_is_dma(rflags)) {
> +               if (!lpbfifo->irqs_pending)
> +                       do_callback = 1;
> +       }
> +       else {
>
>                /* Bytes done */
>                status_count &= MPC52xx_SCLPC_STATUS_BYTES_DONE_MASK;
>
>                if (!mpc52xx_lpbfifo_is_write(rflags)) {
>                        /* copy the data out of the FIFO */
> -                       reg = &lpbfifo.regs->fifo_data;
> +                       reg = &lpbfifo->regs->fifo_data;
>                        data = req->data + req->pos;
>                        for (i = 0; i < status_count; i += sizeof(u32))
>                                *data++ = in_be32(reg);
> @@ -288,13 +309,10 @@ static irqreturn_t mpc52xx_lpbfifo_sclpc_irq(int irq, void *dev_id)
>                else
>                        do_callback = 1;
>        }
> -       else {
> -               do_callback = 1;
> -       }
>
>  out:
>        /* Clear the IRQ */
> -       out_8(&lpbfifo.regs->bytes_done_status.status, BIT(0));
> +       out_8(&lpbfifo->regs->bytes_done_status.status, BIT(0));
>
>        req->last_byte = ((u8 *)req->data)[req->size - 1];
>
> @@ -304,11 +322,11 @@ out:
>        /* When the do_callback flag is set; it means the transfer is finished
>         * so set the FIFO as idle */
>        if (do_callback) {
> -               lpbfifo.req = NULL;
> -               out_be32(&lpbfifo.regs->enable, MPC52xx_SCLPC_ENABLE_RC | MPC52xx_SCLPC_ENABLE_RF);
> +               lpbfifo->req = NULL;
> +               out_be32(&lpbfifo->regs->enable, MPC52xx_SCLPC_ENABLE_RC | MPC52xx_SCLPC_ENABLE_RF);
>
>                req->irq_ticks += get_tbl() - ts;
> -               spin_unlock_irqrestore(&lpbfifo.lock, flags);
> +               spin_unlock_irqrestore(&lpbfifo->lock, flags);
>
>                /* Spinlock is released; it is now safe to call the callback */
>                if (req->callback)
> @@ -318,7 +336,7 @@ out:
>        }
>        else {
>                req->irq_ticks += get_tbl() - ts;
> -               spin_unlock_irqrestore(&lpbfifo.lock, flags);
> +               spin_unlock_irqrestore(&lpbfifo->lock, flags);
>
>                return IRQ_HANDLED;
>        }
> @@ -354,14 +372,30 @@ static irqreturn_t mpc52xx_lpbfifo_bcom_irq(int irq, void *dev_id)
>        bcom_retrieve_buffer(lpbfifo->bcom_cur_task, NULL, NULL);
>        // req->irq_ticks += get_tbl() - ts;
>
> -       if (lpbfifo->req) {
> -               if (mpc52xx_lpbfifo_is_write(lpbfifo->req->flags))
> -                       dma_unmap_single(lpbfifo->dev, lpbfifo->req->data_dma, lpbfifo->req->size, DMA_TO_DEVICE);
> -               else
> -                       dma_unmap_single(lpbfifo->dev, lpbfifo->req->data_dma, lpbfifo->req->size, DMA_FROM_DEVICE);
> -       } else
> -       {
> -               dev_err(lpbfifo->dev, "request is NULL\n");
> +       lpbfifo->irqs_pending &= ~MPC52XX_LPBFIFO_PENDING_BCOM;
> +       if (!lpbfifo->irqs_pending) {
> +               struct mpc52xx_lpbfifo_request *req = lpbfifo->req;
> +
> +               if (req) {
> +                       if (mpc52xx_lpbfifo_is_write(lpbfifo->req->flags))
> +                               dma_unmap_single(lpbfifo->dev, lpbfifo->req->data_dma, lpbfifo->req->size, DMA_TO_DEVICE);
> +                       else
> +                               dma_unmap_single(lpbfifo->dev, lpbfifo->req->data_dma, lpbfifo->req->size, DMA_FROM_DEVICE);
> +
> +                       lpbfifo->req = NULL;
> +                       out_be32(&lpbfifo->regs->enable, MPC52xx_SCLPC_ENABLE_RC | MPC52xx_SCLPC_ENABLE_RF);
> +
> +                       spin_unlock_irqrestore(&lpbfifo->lock, flags);
> +
> +                       /* Spinlock is released; it is now safe to call the callback */
> +                       if (req->callback)
> +                               req->callback(req);
> +
> +                       return IRQ_HANDLED;
> +               }
> +               else {
> +                       dev_err(lpbfifo->dev, "bogus BCOM IRQ\n");
> +               }
>        }
>
>        spin_unlock_irqrestore(&lpbfifo->lock, flags);
> @@ -451,8 +485,8 @@ mpc52xx_lpbfifo_probe(struct of_device *op, const struct of_device_id *match)
>        if (lpbfifo.dev != NULL)
>                return -ENOSPC;
>
> -       lpbfifo.irq = irq_of_parse_and_map(op->node, 0);
> -       if (!lpbfifo.irq)
> +       lpbfifo.sclpc_irq = irq_of_parse_and_map(op->node, 0);
> +       if (!lpbfifo.sclpc_irq)
>                return -ENODEV;
>
>        if (of_address_to_resource(op->node, 0, &res))
> @@ -468,7 +502,7 @@ mpc52xx_lpbfifo_probe(struct of_device *op, const struct of_device_id *match)
>        out_be32(&lpbfifo.regs->enable, MPC52xx_SCLPC_ENABLE_RC | MPC52xx_SCLPC_ENABLE_RF);
>
>        /* register the interrupt handler */
> -       rc = request_irq(lpbfifo.irq, mpc52xx_lpbfifo_sclpc_irq, 0,
> +       rc = request_irq(lpbfifo.sclpc_irq, mpc52xx_lpbfifo_sclpc_irq, 0,
>                         "mpc52xx-lpbfifo", &lpbfifo);
>        if (rc)
>                goto err_irq;
> @@ -539,7 +573,7 @@ static int __devexit mpc52xx_lpbfifo_remove(struct of_device *op)
>        free_irq(bcom_get_task_irq(lpbfifo.bcom_rx_task), &lpbfifo);
>        bcom_gen_bd_rx_release(lpbfifo.bcom_rx_task);
>
> -       free_irq(lpbfifo.irq, &lpbfifo);
> +       free_irq(lpbfifo.sclpc_irq, &lpbfifo);
>        iounmap(lpbfifo.regs);
>        lpbfifo.regs = NULL;
>        lpbfifo.dev = NULL;
> @@ -547,7 +581,7 @@ static int __devexit mpc52xx_lpbfifo_remove(struct of_device *op)
>        return 0;
>  }
>
> -static struct of_device_id mpc52xx_lpbfifo_match[] __devinitconst = {
> +static const struct of_device_id mpc52xx_lpbfifo_match[] __devinitconst = {
>        { .compatible = "fsl,mpc5200-lpbfifo", },
>        {},
>  };
> --
> 1.6.5.5
>
>
>
> --
> Roman Fietze                Telemotive AG Büro Mühlhausen
> Breitwiesen                              73347 Mühlhausen
> Tel.: +49(0)7335/18493-45        http://www.telemotive.de
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
>
Roman Fietze Jan. 12, 2010, 7:43 a.m. UTC | #2
Hello Grant,

On Monday 11 January 2010 21:19:14 Grant Likely wrote:

> I'm really not convinced.

At least you made me think about that some more. And of course, you
are right, the order must be fixed, one way using TX, the other way
using RX.

I think delayed BCOM IRQs in TX direction, caused by a high FEC or ATA
BCOM load, could cause starting the next job with a not yet cleand up
BD ring.

Please give me another change to come up with a hopefully much cleaner
design, esp. after some tests under low and high load with alternating
transfer directions. Probably we can stay with the original design,
and only take some extra effort when the transfer directions changes.


Roman
diff mbox

Patch

diff --git a/arch/powerpc/platforms/52xx/mpc52xx_lpbfifo.c b/arch/powerpc/platforms/52xx/mpc52xx_lpbfifo.c
index 21b2a40..cd8dc69 100644
--- a/arch/powerpc/platforms/52xx/mpc52xx_lpbfifo.c
+++ b/arch/powerpc/platforms/52xx/mpc52xx_lpbfifo.c
@@ -32,7 +32,7 @@  struct mpc52xx_lpbfifo {
 	struct device *dev;
 	phys_addr_t regs_phys;
 	struct mpc52xx_sclpc __iomem *regs;
-	int irq;
+	int sclpc_irq;
 	spinlock_t lock;
 
 	struct bcom_task *bcom_tx_task;
@@ -41,6 +41,7 @@  struct mpc52xx_lpbfifo {
 
 	/* Current state data */
 	struct mpc52xx_lpbfifo_request *req;
+	unsigned short irqs_pending;
 	int dma_irqs_enabled;
 };
 
@@ -48,6 +49,14 @@  struct mpc52xx_lpbfifo {
 static struct mpc52xx_lpbfifo lpbfifo;
 
 
+/* The order of the raised interrupts of SCLPC and BCOM cann not be
+ * predicted, because it depends on the individual BCOM and CPU
+ * loads. So in DMA mode we just wait for both until we finish the
+ * transaction. */
+#define MPC52XX_LPBFIFO_PENDING_SCLPC	BIT(0)
+#define MPC52XX_LPBFIFO_PENDING_BCOM	BIT(1)
+
+
 /**
  * mpc52xx_lpbfifo_is_write - return true if it's a WRITE request
  */
@@ -127,6 +136,8 @@  static void mpc52xx_lpbfifo_kick(struct mpc52xx_lpbfifo_request *req)
 				out_be32(reg, *data++);
 		}
 
+		lpbfifo.irqs_pending = MPC52XX_LPBFIFO_PENDING_SCLPC;
+
 		/* Unmask both error and completion irqs */
 		out_be32(&lpbfifo.regs->enable, (MPC52xx_SCLPC_ENABLE_AIE |
 						 MPC52xx_SCLPC_ENABLE_NIE |
@@ -172,6 +183,8 @@  static void mpc52xx_lpbfifo_kick(struct mpc52xx_lpbfifo_request *req)
 		/* error irq & master enabled bit */
 		out_be32(&lpbfifo.regs->enable, MPC52xx_SCLPC_ENABLE_AIE | MPC52xx_SCLPC_ENABLE_NIE | MPC52xx_SCLPC_ENABLE_ME);
 
+		lpbfifo.irqs_pending = MPC52XX_LPBFIFO_PENDING_BCOM | MPC52XX_LPBFIFO_PENDING_SCLPC;
+
 		bd = bcom_prepare_next_buffer(lpbfifo.bcom_cur_task);
 		bd->status = tc;
 		bd->data[0] = req->data_dma + req->pos;
@@ -188,6 +201,7 @@  static void mpc52xx_lpbfifo_kick(struct mpc52xx_lpbfifo_request *req)
 		bcom_enable(lpbfifo.bcom_cur_task);
 }
 
+
 /**
  * mpc52xx_lpbfifo_sclpc_irq - IRQ handler for LPB FIFO
  *
@@ -232,8 +246,9 @@  static void mpc52xx_lpbfifo_kick(struct mpc52xx_lpbfifo_request *req)
  */
 static irqreturn_t mpc52xx_lpbfifo_sclpc_irq(int irq, void *dev_id)
 {
+	struct mpc52xx_lpbfifo *lpbfifo = dev_id;
 	struct mpc52xx_lpbfifo_request *req;
-	u32 status_count = in_be32(&lpbfifo.regs->bytes_done_status.bytes_done);
+	u32 status_count = in_be32(&lpbfifo->regs->bytes_done_status.bytes_done);
 	void __iomem *reg;
 	u32 *data;
 	size_t i;
@@ -242,18 +257,20 @@  static irqreturn_t mpc52xx_lpbfifo_sclpc_irq(int irq, void *dev_id)
 	unsigned long flags;
 	int rflags;
 
-	spin_lock_irqsave(&lpbfifo.lock, flags);
+	spin_lock_irqsave(&lpbfifo->lock, flags);
 	ts = get_tbl();
 
-	req = lpbfifo.req;
+	req = lpbfifo->req;
 	if (!req) {
-		spin_unlock_irqrestore(&lpbfifo.lock, flags);
+		spin_unlock_irqrestore(&lpbfifo->lock, flags);
 		pr_err("bogus SCLPC IRQ\n");
 		return IRQ_HANDLED;
 	}
 
+	lpbfifo->irqs_pending &= ~MPC52XX_LPBFIFO_PENDING_SCLPC;
+
 	rflags = req->flags;
-	status_count = in_be32(&lpbfifo.regs->bytes_done_status.bytes_done);
+	status_count = in_be32(&lpbfifo->regs->bytes_done_status.bytes_done);
 
 	/* Check normal termination bit */
 	if (!(status_count & MPC52xx_SCLPC_STATUS_NT))
@@ -261,19 +278,23 @@  static irqreturn_t mpc52xx_lpbfifo_sclpc_irq(int irq, void *dev_id)
 
 	/* Check abort bit */
 	if (status_count & MPC52xx_SCLPC_STATUS_AT) {
-		out_be32(&lpbfifo.regs->enable, MPC52xx_SCLPC_ENABLE_RC | MPC52xx_SCLPC_ENABLE_RF);
+		out_be32(&lpbfifo->regs->enable, MPC52xx_SCLPC_ENABLE_RC | MPC52xx_SCLPC_ENABLE_RF);
 		do_callback = 1;
 		goto out;
 	}
 
-	if (!mpc52xx_lpbfifo_is_dma(rflags)) {
+	if (mpc52xx_lpbfifo_is_dma(rflags)) {
+		if (!lpbfifo->irqs_pending)
+			do_callback = 1;
+	}
+	else {
 
 		/* Bytes done */
 		status_count &= MPC52xx_SCLPC_STATUS_BYTES_DONE_MASK;
 
 		if (!mpc52xx_lpbfifo_is_write(rflags)) {
 			/* copy the data out of the FIFO */
-			reg = &lpbfifo.regs->fifo_data;
+			reg = &lpbfifo->regs->fifo_data;
 			data = req->data + req->pos;
 			for (i = 0; i < status_count; i += sizeof(u32))
 				*data++ = in_be32(reg);
@@ -288,13 +309,10 @@  static irqreturn_t mpc52xx_lpbfifo_sclpc_irq(int irq, void *dev_id)
 		else
 			do_callback = 1;
 	}
-	else {
-		do_callback = 1;
-	}
 
 out:
 	/* Clear the IRQ */
-	out_8(&lpbfifo.regs->bytes_done_status.status, BIT(0));
+	out_8(&lpbfifo->regs->bytes_done_status.status, BIT(0));
 
 	req->last_byte = ((u8 *)req->data)[req->size - 1];
 
@@ -304,11 +322,11 @@  out:
 	/* When the do_callback flag is set; it means the transfer is finished
 	 * so set the FIFO as idle */
 	if (do_callback) {
-		lpbfifo.req = NULL;
-		out_be32(&lpbfifo.regs->enable, MPC52xx_SCLPC_ENABLE_RC | MPC52xx_SCLPC_ENABLE_RF);
+		lpbfifo->req = NULL;
+		out_be32(&lpbfifo->regs->enable, MPC52xx_SCLPC_ENABLE_RC | MPC52xx_SCLPC_ENABLE_RF);
 
 		req->irq_ticks += get_tbl() - ts;
-		spin_unlock_irqrestore(&lpbfifo.lock, flags);
+		spin_unlock_irqrestore(&lpbfifo->lock, flags);
 
 		/* Spinlock is released; it is now safe to call the callback */
 		if (req->callback)
@@ -318,7 +336,7 @@  out:
 	}
 	else {
 		req->irq_ticks += get_tbl() - ts;
-		spin_unlock_irqrestore(&lpbfifo.lock, flags);
+		spin_unlock_irqrestore(&lpbfifo->lock, flags);
 
 		return IRQ_HANDLED;
 	}
@@ -354,14 +372,30 @@  static irqreturn_t mpc52xx_lpbfifo_bcom_irq(int irq, void *dev_id)
 	bcom_retrieve_buffer(lpbfifo->bcom_cur_task, NULL, NULL);
 	// req->irq_ticks += get_tbl() - ts;
 
-	if (lpbfifo->req) {
-		if (mpc52xx_lpbfifo_is_write(lpbfifo->req->flags))
-			dma_unmap_single(lpbfifo->dev, lpbfifo->req->data_dma, lpbfifo->req->size, DMA_TO_DEVICE);
-		else
-			dma_unmap_single(lpbfifo->dev, lpbfifo->req->data_dma, lpbfifo->req->size, DMA_FROM_DEVICE);
-	} else
-	{
-		dev_err(lpbfifo->dev, "request is NULL\n");
+	lpbfifo->irqs_pending &= ~MPC52XX_LPBFIFO_PENDING_BCOM;
+	if (!lpbfifo->irqs_pending) {
+		struct mpc52xx_lpbfifo_request *req = lpbfifo->req;
+
+		if (req) {
+			if (mpc52xx_lpbfifo_is_write(lpbfifo->req->flags))
+				dma_unmap_single(lpbfifo->dev, lpbfifo->req->data_dma, lpbfifo->req->size, DMA_TO_DEVICE);
+			else
+				dma_unmap_single(lpbfifo->dev, lpbfifo->req->data_dma, lpbfifo->req->size, DMA_FROM_DEVICE);
+
+			lpbfifo->req = NULL;
+			out_be32(&lpbfifo->regs->enable, MPC52xx_SCLPC_ENABLE_RC | MPC52xx_SCLPC_ENABLE_RF);
+
+			spin_unlock_irqrestore(&lpbfifo->lock, flags);
+
+			/* Spinlock is released; it is now safe to call the callback */
+			if (req->callback)
+				req->callback(req);
+
+			return IRQ_HANDLED;
+		}
+		else {
+			dev_err(lpbfifo->dev, "bogus BCOM IRQ\n");
+		}
 	}
 
 	spin_unlock_irqrestore(&lpbfifo->lock, flags);
@@ -451,8 +485,8 @@  mpc52xx_lpbfifo_probe(struct of_device *op, const struct of_device_id *match)
 	if (lpbfifo.dev != NULL)
 		return -ENOSPC;
 
-	lpbfifo.irq = irq_of_parse_and_map(op->node, 0);
-	if (!lpbfifo.irq)
+	lpbfifo.sclpc_irq = irq_of_parse_and_map(op->node, 0);
+	if (!lpbfifo.sclpc_irq)
 		return -ENODEV;
 
 	if (of_address_to_resource(op->node, 0, &res))
@@ -468,7 +502,7 @@  mpc52xx_lpbfifo_probe(struct of_device *op, const struct of_device_id *match)
 	out_be32(&lpbfifo.regs->enable, MPC52xx_SCLPC_ENABLE_RC | MPC52xx_SCLPC_ENABLE_RF);
 
 	/* register the interrupt handler */
-	rc = request_irq(lpbfifo.irq, mpc52xx_lpbfifo_sclpc_irq, 0,
+	rc = request_irq(lpbfifo.sclpc_irq, mpc52xx_lpbfifo_sclpc_irq, 0,
 			 "mpc52xx-lpbfifo", &lpbfifo);
 	if (rc)
 		goto err_irq;
@@ -539,7 +573,7 @@  static int __devexit mpc52xx_lpbfifo_remove(struct of_device *op)
 	free_irq(bcom_get_task_irq(lpbfifo.bcom_rx_task), &lpbfifo);
 	bcom_gen_bd_rx_release(lpbfifo.bcom_rx_task);
 
-	free_irq(lpbfifo.irq, &lpbfifo);
+	free_irq(lpbfifo.sclpc_irq, &lpbfifo);
 	iounmap(lpbfifo.regs);
 	lpbfifo.regs = NULL;
 	lpbfifo.dev = NULL;
@@ -547,7 +581,7 @@  static int __devexit mpc52xx_lpbfifo_remove(struct of_device *op)
 	return 0;
 }
 
-static struct of_device_id mpc52xx_lpbfifo_match[] __devinitconst = {
+static const struct of_device_id mpc52xx_lpbfifo_match[] __devinitconst = {
 	{ .compatible = "fsl,mpc5200-lpbfifo", },
 	{},
 };