[PATCH 10/13] powerpc/5200: LocalPlus driver: fix problem caused by unpredictable IRQ order
Grant Likely
grant.likely at secretlab.ca
Tue Jan 12 07:19:14 EST 2010
On Tue, Dec 22, 2009 at 12:09 AM, Roman Fietze
<roman.fietze at 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 at 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 at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
>
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
More information about the Linuxppc-dev
mailing list