[PATCH 26/30] loongson: flush irq write operation

[PATCH 26/30] loongson: flush irq write operation

[Wu Zhangjin]

>From d0431e4d83ec7f852d631f80aaca4b66586b4db2 Mon Sep 17 00:00:00 2001
From: Wu Zhangjin <wuzhangjin@gmail.com>
Date: Sat, 16 May 2009 04:54:27 +0800
Subject: [PATCH 26/30] loongson: flush irq write operation

read back after write, otherwise, there will be many spurious irqs from
it
---
 arch/mips/kernel/i8259.c               |    5 +++++
 arch/mips/loongson/common/bonito-irq.c |    4 ++++
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/arch/mips/kernel/i8259.c b/arch/mips/kernel/i8259.c
index 413bd1d..f7c3a2b 100644
--- a/arch/mips/kernel/i8259.c
+++ b/arch/mips/kernel/i8259.c
@@ -175,12 +175,17 @@ handle_real_irq:
 	if (irq & 8) {
 		inb(PIC_SLAVE_IMR);	/* DUMMY - (do we need this?) */
 		outb(cached_slave_mask, PIC_SLAVE_IMR);
+		inb(PIC_SLAVE_IMR);
 		outb(0x60+(irq&7), PIC_SLAVE_CMD);/* 'Specific EOI' to slave */
+		inb(PIC_SLAVE_CMD);
 		outb(0x60+PIC_CASCADE_IR, PIC_MASTER_CMD); /* 'Specific EOI' to
master-IRQ2 */
+		inb(PIC_MASTER_CMD);
 	} else {
 		inb(PIC_MASTER_IMR);	/* DUMMY - (do we need this?) */
 		outb(cached_master_mask, PIC_MASTER_IMR);
+		inb(PIC_SLAVE_IMR);
 		outb(0x60+irq, PIC_MASTER_CMD);	/* 'Specific EOI to master */
+		inb(PIC_MASTER_CMD);
 	}
 	smtc_im_ack_irq(irq);
 	spin_unlock_irqrestore(&i8259A_lock, flags);
diff --git a/arch/mips/loongson/common/bonito-irq.c
b/arch/mips/loongson/common/bonito-irq.c
index d5a5ae8..cfbeaf5 100644
--- a/arch/mips/loongson/common/bonito-irq.c
+++ b/arch/mips/loongson/common/bonito-irq.c
@@ -36,12 +36,16 @@
 static inline void bonito_irq_enable(unsigned int irq)
 {
 	LOONGSON_INTENSET = (1 << (irq - LOONGSON_IRQ_BASE));
+	/* flush the write operation via a following read operation */
+	(void)LOONGSON_INTENSET;
 	mmiowb();
 }
 
 static inline void bonito_irq_disable(unsigned int irq)
 {
 	LOONGSON_INTENCLR = (1 << (irq - LOONGSON_IRQ_BASE));
+	/* flush the write operation via a following read operation */
+	(void)LOONGSON_INTENCLR;
 	mmiowb();
 }
 
-- 
1.6.2.1

[PATCH 26/30] loongson: flush irq write operation

[Wu Zhangjin]

From d0431e4d83ec7f852d631f80aaca4b66586b4db2 Mon Sep 17 00:00:00 2001
From: Wu Zhangjin <wuzhangjin@gmail.com>
Date: Sat, 16 May 2009 04:54:27 +0800
Subject: [PATCH 26/30] loongson: flush irq write operation

read back after write, otherwise, there will be many spurious irqs from
it
---
 arch/mips/kernel/i8259.c               |    5 +++++
 arch/mips/loongson/common/bonito-irq.c |    4 ++++
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/arch/mips/kernel/i8259.c b/arch/mips/kernel/i8259.c
index 413bd1d..f7c3a2b 100644
--- a/arch/mips/kernel/i8259.c
+++ b/arch/mips/kernel/i8259.c
@@ -175,12 +175,17 @@ handle_real_irq:
 	if (irq & 8) {
 		inb(PIC_SLAVE_IMR);	/* DUMMY - (do we need this?) */
 		outb(cached_slave_mask, PIC_SLAVE_IMR);
+		inb(PIC_SLAVE_IMR);
 		outb(0x60+(irq&7), PIC_SLAVE_CMD);/* 'Specific EOI' to slave */
+		inb(PIC_SLAVE_CMD);
 		outb(0x60+PIC_CASCADE_IR, PIC_MASTER_CMD); /* 'Specific EOI' to
master-IRQ2 */
+		inb(PIC_MASTER_CMD);
 	} else {
 		inb(PIC_MASTER_IMR);	/* DUMMY - (do we need this?) */
 		outb(cached_master_mask, PIC_MASTER_IMR);
+		inb(PIC_SLAVE_IMR);
 		outb(0x60+irq, PIC_MASTER_CMD);	/* 'Specific EOI to master */
+		inb(PIC_MASTER_CMD);
 	}
 	smtc_im_ack_irq(irq);
 	spin_unlock_irqrestore(&i8259A_lock, flags);
diff --git a/arch/mips/loongson/common/bonito-irq.c
b/arch/mips/loongson/common/bonito-irq.c
index d5a5ae8..cfbeaf5 100644
--- a/arch/mips/loongson/common/bonito-irq.c
+++ b/arch/mips/loongson/common/bonito-irq.c
@@ -36,12 +36,16 @@
 static inline void bonito_irq_enable(unsigned int irq)
 {
 	LOONGSON_INTENSET = (1 << (irq - LOONGSON_IRQ_BASE));
+	/* flush the write operation via a following read operation */
+	(void)LOONGSON_INTENSET;
 	mmiowb();
 }
 
 static inline void bonito_irq_disable(unsigned int irq)
 {
 	LOONGSON_INTENCLR = (1 << (irq - LOONGSON_IRQ_BASE));
+	/* flush the write operation via a following read operation */
+	(void)LOONGSON_INTENCLR;
 	mmiowb();
 }
 
-- 
1.6.2.1

Re: [PATCH 26/30] loongson: flush irq write operation

[Ralf Baechle]

On Sat, May 16, 2009 at 06:28:47AM +0800, Wu Zhangjin wrote:

> read back after write, otherwise, there will be many spurious irqs from
> it
> ---
>  arch/mips/kernel/i8259.c               |    5 +++++
>  arch/mips/loongson/common/bonito-irq.c |    4 ++++
>  2 files changed, 9 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/mips/kernel/i8259.c b/arch/mips/kernel/i8259.c
> index 413bd1d..f7c3a2b 100644
> --- a/arch/mips/kernel/i8259.c
> +++ b/arch/mips/kernel/i8259.c
> @@ -175,12 +175,17 @@ handle_real_irq:
>  	if (irq & 8) {
>  		inb(PIC_SLAVE_IMR);	/* DUMMY - (do we need this?) */
>  		outb(cached_slave_mask, PIC_SLAVE_IMR);
> +		inb(PIC_SLAVE_IMR);
>  		outb(0x60+(irq&7), PIC_SLAVE_CMD);/* 'Specific EOI' to slave */
> +		inb(PIC_SLAVE_CMD);
>  		outb(0x60+PIC_CASCADE_IR, PIC_MASTER_CMD); /* 'Specific EOI' to
> master-IRQ2 */
> +		inb(PIC_MASTER_CMD);
>  	} else {
>  		inb(PIC_MASTER_IMR);	/* DUMMY - (do we need this?) */
>  		outb(cached_master_mask, PIC_MASTER_IMR);
> +		inb(PIC_SLAVE_IMR);
>  		outb(0x60+irq, PIC_MASTER_CMD);	/* 'Specific EOI to master */
> +		inb(PIC_MASTER_CMD);
>  	}
>  	smtc_im_ack_irq(irq);
>  	spin_unlock_irqrestore(&i8259A_lock, flags);

The semantic of inX() / outX() is defined by the x86 architecture which
forbids posting I/O port writes.  In short I think this one is papering
over a bug in the outX() implementation.

I'm ok with the bonito part of this patch.

  Ralf

Re: [PATCH 26/30] loongson: flush irq write operation

[yanh]

在 2009-05-18一的 17:36 +0100，Ralf Baechle写道：
> On Sat, May 16, 2009 at 06:28:47AM +0800, Wu Zhangjin wrote:
> 
> > read back after write, otherwise, there will be many spurious irqs from
> > it
> > ---
> >  arch/mips/kernel/i8259.c               |    5 +++++
> >  arch/mips/loongson/common/bonito-irq.c |    4 ++++
> >  2 files changed, 9 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/mips/kernel/i8259.c b/arch/mips/kernel/i8259.c
> > index 413bd1d..f7c3a2b 100644
> > --- a/arch/mips/kernel/i8259.c
> > +++ b/arch/mips/kernel/i8259.c
> > @@ -175,12 +175,17 @@ handle_real_irq:
> >  	if (irq & 8) {
> >  		inb(PIC_SLAVE_IMR);	/* DUMMY - (do we need this?) */
> >  		outb(cached_slave_mask, PIC_SLAVE_IMR);
> > +		inb(PIC_SLAVE_IMR);
> >  		outb(0x60+(irq&7), PIC_SLAVE_CMD);/* 'Specific EOI' to slave */
> > +		inb(PIC_SLAVE_CMD);
> >  		outb(0x60+PIC_CASCADE_IR, PIC_MASTER_CMD); /* 'Specific EOI' to
> > master-IRQ2 */
> > +		inb(PIC_MASTER_CMD);
> >  	} else {
> >  		inb(PIC_MASTER_IMR);	/* DUMMY - (do we need this?) */
> >  		outb(cached_master_mask, PIC_MASTER_IMR);
> > +		inb(PIC_SLAVE_IMR);
> >  		outb(0x60+irq, PIC_MASTER_CMD);	/* 'Specific EOI to master */
> > +		inb(PIC_MASTER_CMD);
> >  	}
> >  	smtc_im_ack_irq(irq);
> >  	spin_unlock_irqrestore(&i8259A_lock, flags);
> 
> The semantic of inX() / outX() is defined by the x86 architecture which
> forbids posting I/O port writes.  In short I think this one is papering
> over a bug in the outX() implementation.
Yes, the outX should do a delayed write, however it does not. 
So our solution is making a read to flush the write.
> 
> I'm ok with the bonito part of this patch.
> 
>   Ralf

Re: [PATCH 26/30] loongson: flush irq write operation

[Ralf Baechle]

On Tue, May 19, 2009 at 10:37:17AM +0800, yanh wrote:

> > The semantic of inX() / outX() is defined by the x86 architecture which
> > forbids posting I/O port writes.  In short I think this one is papering
> > over a bug in the outX() implementation.
> Yes, the outX should do a delayed write, however it does not. 
> So our solution is making a read to flush the write.

Do you actually need all the inb() you added to get things to work or is

diff --git a/arch/mips/kernel/i8259.c b/arch/mips/kernel/i8259.c
index 01c0885..42d75d7 100644
--- a/arch/mips/kernel/i8259.c
+++ b/arch/mips/kernel/i8259.c
@@ -177,10 +177,12 @@ handle_real_irq:
 		outb(cached_slave_mask, PIC_SLAVE_IMR);
 		outb(0x60+(irq&7), PIC_SLAVE_CMD);/* 'Specific EOI' to slave */
 		outb(0x60+PIC_CASCADE_IR, PIC_MASTER_CMD); /* 'Specific EOI' to master-IRQ2 */
+		inb(PIC_MASTER_CMD);
 	} else {
 		inb(PIC_MASTER_IMR);	/* DUMMY - (do we need this?) */
 		outb(cached_master_mask, PIC_MASTER_IMR);
 		outb(0x60+irq, PIC_MASTER_CMD);	/* 'Specific EOI to master */
+		inb(PIC_MASTER_CMD);
 	}
 	smtc_im_ack_irq(irq);
 	spin_unlock_irqrestore(&i8259A_lock, flags);

sufficient to solve the problem?

  Ralf

Re: [PATCH 26/30] loongson: flush irq write operation

[yanh]

在 2009-05-19二的 17:01 +0100，Ralf Baechle写道：
> On Tue, May 19, 2009 at 10:37:17AM +0800, yanh wrote:
> 
> > > The semantic of inX() / outX() is defined by the x86 architecture which
> > > forbids posting I/O port writes.  In short I think this one is papering
> > > over a bug in the outX() implementation.
> > Yes, the outX should do a delayed write, however it does not. 
> > So our solution is making a read to flush the write.
> 
> Do you actually need all the inb() you added to get things to work or is

Thanks for your reply.
As my test, if there is no the read, there will be many spurious irqs. 
> 
> diff --git a/arch/mips/kernel/i8259.c b/arch/mips/kernel/i8259.c
> index 01c0885..42d75d7 100644
> --- a/arch/mips/kernel/i8259.c
> +++ b/arch/mips/kernel/i8259.c
> @@ -177,10 +177,12 @@ handle_real_irq:
>  		outb(cached_slave_mask, PIC_SLAVE_IMR);
>  		outb(0x60+(irq&7), PIC_SLAVE_CMD);/* 'Specific EOI' to slave */
>  		outb(0x60+PIC_CASCADE_IR, PIC_MASTER_CMD); /* 'Specific EOI' to master-IRQ2 */
> +		inb(PIC_MASTER_CMD);
>  	} else {
>  		inb(PIC_MASTER_IMR);	/* DUMMY - (do we need this?) */
>  		outb(cached_master_mask, PIC_MASTER_IMR);
>  		outb(0x60+irq, PIC_MASTER_CMD);	/* 'Specific EOI to master */
> +		inb(PIC_MASTER_CMD);
>  	}
>  	smtc_im_ack_irq(irq);
>  	spin_unlock_irqrestore(&i8259A_lock, flags);
> 
> sufficient to solve the problem?
I have test this patch just now. It works well on yeeloong. 
I have one question what's the difference between the two patch? 
> 
>   Ralf

Re: [PATCH 26/30] loongson: flush irq write operation

[yanh]

在 2009-05-20三的 10:28 +0800，yanh写道：
> 在 2009-05-19二的 17:01 +0100，Ralf Baechle写道：
> > On Tue, May 19, 2009 at 10:37:17AM +0800, yanh wrote:
> > 
> > > > The semantic of inX() / outX() is defined by the x86 architecture which
> > > > forbids posting I/O port writes.  In short I think this one is papering
> > > > over a bug in the outX() implementation.
> > > Yes, the outX should do a delayed write, however it does not. 
> > > So our solution is making a read to flush the write.
> > 
> > Do you actually need all the inb() you added to get things to work or is
> 
> Thanks for your reply.
> As my test, if there is no the read, there will be many spurious irqs. 
> > 
> > diff --git a/arch/mips/kernel/i8259.c b/arch/mips/kernel/i8259.c
> > index 01c0885..42d75d7 100644
> > --- a/arch/mips/kernel/i8259.c
> > +++ b/arch/mips/kernel/i8259.c
> > @@ -177,10 +177,12 @@ handle_real_irq:
> >  		outb(cached_slave_mask, PIC_SLAVE_IMR);
> >  		outb(0x60+(irq&7), PIC_SLAVE_CMD);/* 'Specific EOI' to slave */
> >  		outb(0x60+PIC_CASCADE_IR, PIC_MASTER_CMD); /* 'Specific EOI' to master-IRQ2 */
> > +		inb(PIC_MASTER_CMD);
> >  	} else {
> >  		inb(PIC_MASTER_IMR);	/* DUMMY - (do we need this?) */
> >  		outb(cached_master_mask, PIC_MASTER_IMR);
> >  		outb(0x60+irq, PIC_MASTER_CMD);	/* 'Specific EOI to master */
> > +		inb(PIC_MASTER_CMD);
> >  	}
> >  	smtc_im_ack_irq(irq);
> >  	spin_unlock_irqrestore(&i8259A_lock, flags);
> > 
> > sufficient to solve the problem?
> I have test this patch just now. It works well on yeeloong. 
> I have one question what's the difference between the two patch? 
My original patch only flush imr write. Really only one read is suffient. So no question about it now.
> > 
> >   Ralf
> 
> 

Re: [PATCH 26/30] loongson: flush irq write operation

[Ralf Baechle]

On Wed, May 20, 2009 at 10:28:14AM +0800, yanh wrote:

> I have test this patch just now. It works well on yeeloong. 

Thanks for testing.

> I have one question what's the difference between the two patch? 

Uncached writes can't be re-ordered.  By adding a read after the last
write my patch forces not only completion of the preceding write but due
to this ordering constraint also completion of all preceding writes is
enforced.

I/O space writes are slow.  I mean slower than slugs in space.  So my
patch is an optimization but that was not the point; I really only meant
to verify that we understood what's going on and we seem to.  Now let's
fix the real issue and make outX() non-posted.

  Ralf

Re: [PATCH 26/30] loongson: flush irq write operation

[yanh]

在 2009-05-20三的 08:07 +0100，Ralf Baechle写道：
> On Wed, May 20, 2009 at 10:28:14AM +0800, yanh wrote:
> 
> > I have test this patch just now. It works well on yeeloong. 
> 
> Thanks for testing.
> 
> > I have one question what's the difference between the two patch? 
> 
> Uncached writes can't be re-ordered.  By adding a read after the last
> write my patch forces not only completion of the preceding write but due
> to this ordering constraint also completion of all preceding writes is
> enforced.
> 
> I/O space writes are slow.  I mean slower than slugs in space.  So my
> patch is an optimization but that was not the point; I really only meant
> to verify that we understood what's going on and we seem to.  Now let's
> fix the real issue and make outX() non-posted.
Thanks for your explanation. 
> 
>   Ralf
>