Re: [PATCH 10/13] powerpc/5200: LocalPlus driver: fix problem caused by unpredictable IRQ order
On Tue, Dec 22, 2009 at 12:09 AM, Roman Fietze
roman.fie...@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.fie...@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;
Re: [PATCH 10/13] powerpc/5200: LocalPlus driver: fix problem caused by unpredictable IRQ order
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 -- Roman FietzeTelemotive AG Büro Mühlhausen Breitwiesen 73347 Mühlhausen Tel.: +49(0)7335/18493-45http://www.telemotive.de Amtsgericht UlmHRB 541321 Vorstand: Peter Kersten, Markus Fischer, Franz Diller, Markus Stolz signature.asc Description: This is a digitally signed message part. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 10/13] powerpc/5200: LocalPlus driver: fix problem caused by unpredictable IRQ order
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.fie...@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
