[PATCH] Fix mmiowb() for MIPS I

[PATCH] Fix mmiowb() for MIPS I

[Atsushi Nemoto]

The SYNC instruction is not available on MIPS I.  Use __sync() instead.

Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
---
diff --git a/include/asm-mips/io.h b/include/asm-mips/io.h
index 92ec261..855c304 100644
--- a/include/asm-mips/io.h
+++ b/include/asm-mips/io.h
@@ -502,8 +502,7 @@ BUILDSTRING(q, u64)
 #endif
 
 
-/* Depends on MIPS II instruction set */
-#define mmiowb() asm volatile ("sync" ::: "memory")
+#define mmiowb() __sync()
 
 static inline void memset_io(volatile void __iomem *addr, unsigned char val, int count)
 {

Re: [PATCH] Fix mmiowb() for MIPS I

[Maciej W. Rozycki]

On Thu, 22 Feb 2007, Atsushi Nemoto wrote:

> The SYNC instruction is not available on MIPS I.  Use __sync() instead.
> 
> Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
> ---
> diff --git a/include/asm-mips/io.h b/include/asm-mips/io.h
> index 92ec261..855c304 100644
> --- a/include/asm-mips/io.h
> +++ b/include/asm-mips/io.h
> @@ -502,8 +502,7 @@ BUILDSTRING(q, u64)
>  #endif
>  
>  
> -/* Depends on MIPS II instruction set */
> -#define mmiowb() asm volatile ("sync" ::: "memory")
> +#define mmiowb() __sync()
>  
>  static inline void memset_io(volatile void __iomem *addr, unsigned char val, int count)
>  {

 That's still not correct -- it should probably be defined like mb() 
currently is as the write-back buffer may defeat strong ordering (IIRC, 
the R2020 can do byte merging).  Also the semantics of mmiowb() does not 
seem to be well specified -- I gather a sequence of:

	writeb(mmioreg, val);
	mmiowb();
	readb(mmioreg);

should guarantee "val" has reached the register (mmiowb() replacing 
incorrect mb() used in many places like this), but with either definition 
of mmiowb() and a MIPS-I-style external write-back buffer it will not 
work.

  Maciej

Re: [PATCH] Fix mmiowb() for MIPS I

[Ralf Baechle]

On Wed, Feb 21, 2007 at 05:46:18PM +0000, Maciej W. Rozycki wrote:

> > diff --git a/include/asm-mips/io.h b/include/asm-mips/io.h
> > index 92ec261..855c304 100644
> > --- a/include/asm-mips/io.h
> > +++ b/include/asm-mips/io.h
> > @@ -502,8 +502,7 @@ BUILDSTRING(q, u64)
> >  #endif
> >  
> >  
> > -/* Depends on MIPS II instruction set */
> > -#define mmiowb() asm volatile ("sync" ::: "memory")
> > +#define mmiowb() __sync()
> >  
> >  static inline void memset_io(volatile void __iomem *addr, unsigned char val, int count)
> >  {
> 
>  That's still not correct -- it should probably be defined like mb() 
> currently is as the write-back buffer may defeat strong ordering (IIRC, 
> the R2020 can do byte merging).  Also the semantics of mmiowb() does not 
> seem to be well specified -- I gather a sequence of:
> 
> 	writeb(mmioreg, val);
> 	mmiowb();
> 	readb(mmioreg);
> 
> should guarantee "val" has reached the register (mmiowb() replacing 
> incorrect mb() used in many places like this), but with either definition 
> of mmiowb() and a MIPS-I-style external write-back buffer it will not 
> work.

Does a read from the same device suffice to provide the necessary flushing
the same way as it does on PCI?

I'm not opposed to allowing platform specific definitions for operations
like mmiowbb() but I think we really should get rid of the special MIPS
iob() operation.

  Ralf

Re: [PATCH] Fix mmiowb() for MIPS I

[Maciej W. Rozycki]

On Wed, 21 Feb 2007, Ralf Baechle wrote:

> > should guarantee "val" has reached the register (mmiowb() replacing 
> > incorrect mb() used in many places like this), but with either definition 
> > of mmiowb() and a MIPS-I-style external write-back buffer it will not 
> > work.
> 
> Does a read from the same device suffice to provide the necessary flushing
> the same way as it does on PCI?

 No.  Neither the R2020 nor the R3220 write-back buffer chips (used for 
the R2000 and R3000, respectively) as used in DECstation systems support 
snooping for MMIO, so reads from that space will bypass writes pending in 
the buffer, even for the very same address.  There is some logic in the 
memory controller to resolve such conflicts, but it is activated for main 
RAM accesses only.  Once the write-back buffer has been drained there is 
no need for a read-back though.  The CP0 condition can be used to check 
whether there are any pending writes in the buffer (it is false if there 
are).

 OTOH, DECstation systems that do not use either of the write-back buffers 
mentioned above do require a "sync" (if using the R4000 or R4400 CPU) 
followed by a read-back (any uncached address does) to drain the 
write-back buffer in the MB (memory buffer) ASIC.

> I'm not opposed to allowing platform specific definitions for operations
> like mmiowbb() but I think we really should get rid of the special MIPS
> iob() operation.

 Certainly, but before that happens there has to be a way to be able to 
specify the desired barrier in a generic driver like defxx.c without the 
need to know the underlying platform.

  Maciej