[PATCH] ppc64: Fix __raw_* IO accessors

[PATCH] ppc64: Fix __raw_* IO accessors

[Benjamin Herrenschmidt]

Hi !

Linus, I don't know if you did that on purpose, but you removed the
"volatile" statement from the definition of the __raw_* IO accessors
on ppc64, which cause some real bad optisations to happen in some
fbdev's like matroxfb to happen (just imagine that matroxfb loops
reading an IO register waiting for a bit to change).

(Note: matroxfb has other potential problems due to the fact that
__raw_* do not do any barrier (Petr, we probably need to fix that
some way, unfortunatley, I don't think we have a good abstraction
for providing the missing barrier to a driver using __raw... can't
you just switch back to little endian registers and use normal
readX/writeX ?)

Anyway, here's the fix for asm-ppc64/io.h

Signed-off-by: Benjamin Herrenschmidt <benh@kenrel.crashing.org>

===== include/asm/io.h 1.21 vs edited =====
--- 1.21/include/asm-ppc64/io.h	2004-09-14 04:31:52 +10:00
+++ edited/include/asm/io.h	2004-09-21 19:14:10 +10:00
@@ -71,35 +71,35 @@
 
 static inline unsigned char __raw_readb(const volatile void __iomem *addr)
 {
-	return *(unsigned char __force *)addr;
+	return *(volatile unsigned char __force *)addr;
 }
 static inline unsigned short __raw_readw(const volatile void __iomem *addr)
 {
-	return *(unsigned short __force *)addr;
+	return *(volatile unsigned short __force *)addr;
 }
 static inline unsigned int __raw_readl(const volatile void __iomem *addr)
 {
-	return *(unsigned int __force *)addr;
+	return *(volatile unsigned int __force *)addr;
 }
 static inline unsigned long __raw_readq(const volatile void __iomem *addr)
 {
-	return *(unsigned long __force *)addr;
+	return *(volatile unsigned long __force *)addr;
 }
 static inline void __raw_writeb(unsigned char v, volatile void __iomem *addr)
 {
-	*(unsigned char __force *)addr = v;
+	*(volatile unsigned char __force *)addr = v;
 }
 static inline void __raw_writew(unsigned short v, volatile void __iomem *addr)
 {
-	*(unsigned short __force *)addr = v;
+	*(volatile unsigned short __force *)addr = v;
 }
 static inline void __raw_writel(unsigned int v, volatile void __iomem *addr)
 {
-	*(unsigned int __force *)addr = v;
+	*(volatile unsigned int __force *)addr = v;
 }
 static inline void __raw_writeq(unsigned long v, volatile void __iomem *addr)
 {
-	*(unsigned long __force *)addr = v;
+	*(volatile unsigned long __force *)addr = v;
 }
 #define readb(addr)		eeh_readb(addr)
 #define readw(addr)		eeh_readw(addr)

Re: [PATCH] ppc64: Fix __raw_* IO accessors

[Alan Cox]

On Maw, 2004-09-21 at 10:23, Benjamin Herrenschmidt wrote:
> Hi !
> 
> Linus, I don't know if you did that on purpose, but you removed the
> "volatile" statement from the definition of the __raw_* IO accessors
> on ppc64, which cause some real bad optisations to happen in some
> fbdev's like matroxfb to happen (just imagine that matroxfb loops
> reading an IO register waiting for a bit to change).

Why is it using __raw if it cares about ordering and not using barriers
? Way back when the original definition was that __raw didnt do
barriers. Thats why I2O for example uses __raw_ so that messages can be
generated as efficiently as possible.

Re: [PATCH] ppc64: Fix __raw_* IO accessors

[Benjamin Herrenschmidt]

On Tue, 2004-09-21 at 20:05, Alan Cox wrote:
> On Maw, 2004-09-21 at 10:23, Benjamin Herrenschmidt wrote:
> > Hi !
> > 
> > Linus, I don't know if you did that on purpose, but you removed the
> > "volatile" statement from the definition of the __raw_* IO accessors
> > on ppc64, which cause some real bad optisations to happen in some
> > fbdev's like matroxfb to happen (just imagine that matroxfb loops
> > reading an IO register waiting for a bit to change).
> 
> Why is it using __raw if it cares about ordering and not using barriers
> ? Way back when the original definition was that __raw didnt do
> barriers. Thats why I2O for example uses __raw_ so that messages can be
> generated as efficiently as possible.

It uses __raw for non-byteswap... The problem is that __raw does both
non-byteswap and non-barriers and there is no simple way to get one
and not the other...

Ben.

Re: [PATCH] ppc64: Fix __raw_* IO accessors

[Roland Dreier]

Is it possible to use __raw_*() in portable code?  I have some places
in my code where non-byte-swap IO functions would be useful, but on
ppc64, __raw_*() doesn't know about EEH.  Clearly I don't want to
teach portable code about IO_TOKEN_TO_ADDR etc. so it seems I'm out of
luck.  I end up doing the fairly insane:

	writel(swab32(val), addr);

instead of what I really mean, which is:

	__raw_writel(cpu_to_be32(val), addr);

I'm also a little worried that m68k, sh64 and s390 at least don't
define __raw_* functions.

Thanks,
  Roland

Re: [PATCH] ppc64: Fix __raw_* IO accessors

[Linus Torvalds]

On Tue, 21 Sep 2004, Roland Dreier wrote:
>
> Is it possible to use __raw_*() in portable code?  I have some places
> in my code where non-byte-swap IO functions would be useful, but on
> ppc64, __raw_*() doesn't know about EEH.

I don't think normal readb/writeb should know about EEH either. If you 
want error handling, there's a separate interface being worked on, so that 
normal accesses don't have to pay the cost..

> Clearly I don't want to
> teach portable code about IO_TOKEN_TO_ADDR etc. so it seems I'm out of
> luck.  I end up doing the fairly insane:
> 
> 	writel(swab32(val), addr);

Ok, so that _is_ insane. Mind telling what kind of insane hardware is BE 
in this day and age?

That said, I think

> instead of what I really mean, which is:
> 
> 	__raw_writel(cpu_to_be32(val), addr);

should work, and if you start using it, and the driver is relevant, I'm 
sure other architectures will implement the __raw_ interfaces too. In the 
meantime, please just make it conditional on the proper architectures.

		Linus

Re: [PATCH] ppc64: Fix __raw_* IO accessors

[Geert Uytterhoeven]

On Tue, 21 Sep 2004, Linus Torvalds wrote:
> On Tue, 21 Sep 2004, Roland Dreier wrote:
> > Is it possible to use __raw_*() in portable code?  I have some places

> > instead of what I really mean, which is:
> > 
> > 	__raw_writel(cpu_to_be32(val), addr);
> 
> should work, and if you start using it, and the driver is relevant, I'm 
> sure other architectures will implement the __raw_ interfaces too. In the 
> meantime, please just make it conditional on the proper architectures.

Yep, as soon as we _need_ them, m68k will get them...

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: [PATCH] ppc64: Fix __raw_* IO accessors

[Roland Dreier]

    Linus> I don't think normal readb/writeb should know about EEH
    Linus> either. If you want error handling, there's a separate
    Linus> interface being worked on, so that normal accesses don't
    Linus> have to pay the cost..

I guess I wasn't totally clear.  In <asm-ppc64/io.h>, compare:

	static inline void __raw_writel(unsigned int v, volatile void __iomem *addr)
	{
		*(volatile unsigned int __force *)addr = v;
	}

to:

	#define writel(data, addr)	eeh_writel((data), (addr))

where eeh_writel() is:

	static inline void eeh_writel(u32 val, volatile void __iomem *addr) {
		volatile u32 *vaddr = (volatile u32 *)IO_TOKEN_TO_ADDR(addr);
		out_le32(vaddr, val);
	}

That means using __raw_writel() is pretty much guaranteed to blow up
on IBM pSeries (and I do care about pSeries for my driver).

Maybe something like the patch below would make sense?  (Reordering of
code is to make sure IO_TOKEN_TO_ADDR() is defined before the
__raw_*() functions; eeh.h has to be included after the in_*() and
out_*() functions are defined)

By the way, I notice that <asm-ppc64/eeh.h> has a bunch of eeh_raw_*
functions that appear to be completely unused.  I didn't use them in
my patch because they add memory ordering (isync or sync) that Alan
says __raw_* functions shouldn't have.

    Linus> Ok, so that _is_ insane. Mind telling what kind of insane
    Linus> hardware is BE in this day and age?

:) Mellanox InfiniBand HCAs....

Thanks,
 Roland


Use IO_TOKEN_TO_ADDR() in ppc64 __raw_*() functions, so that drivers
don't need to know about EEH for __raw_*() to work on pSeries.

Signed-off-by: Roland Dreier <roland@topspin.com>

Index: linux-bk/include/asm-ppc64/io.h
===================================================================
--- linux-bk.orig/include/asm-ppc64/io.h	2004-09-21 14:09:30.000000000 -0700
+++ linux-bk/include/asm-ppc64/io.h	2004-09-21 14:29:17.000000000 -0700
@@ -67,40 +67,8 @@
  */
 #define insw_ns(port, buf, ns)	_insw_ns((u16 *)((port)+pci_io_base), (buf), (ns))
 #define insl_ns(port, buf, nl)	_insl_ns((u32 *)((port)+pci_io_base), (buf), (nl))
-#else
+#else /* CONFIG_PPC_ISERIES */
 
-static inline unsigned char __raw_readb(const volatile void __iomem *addr)
-{
-	return *(volatile unsigned char __force *)addr;
-}
-static inline unsigned short __raw_readw(const volatile void __iomem *addr)
-{
-	return *(volatile unsigned short __force *)addr;
-}
-static inline unsigned int __raw_readl(const volatile void __iomem *addr)
-{
-	return *(volatile unsigned int __force *)addr;
-}
-static inline unsigned long __raw_readq(const volatile void __iomem *addr)
-{
-	return *(volatile unsigned long __force *)addr;
-}
-static inline void __raw_writeb(unsigned char v, volatile void __iomem *addr)
-{
-	*(volatile unsigned char __force *)addr = v;
-}
-static inline void __raw_writew(unsigned short v, volatile void __iomem *addr)
-{
-	*(volatile unsigned short __force *)addr = v;
-}
-static inline void __raw_writel(unsigned int v, volatile void __iomem *addr)
-{
-	*(volatile unsigned int __force *)addr = v;
-}
-static inline void __raw_writeq(unsigned long v, volatile void __iomem *addr)
-{
-	*(volatile unsigned long __force *)addr = v;
-}
 #define readb(addr)		eeh_readb(addr)
 #define readw(addr)		eeh_readw(addr)
 #define readl(addr)		eeh_readl(addr)
@@ -134,7 +102,7 @@
 #define outsw(port, buf, ns)  _outsw_ns((u16 *)((port)+pci_io_base), (buf), (ns))
 #define outsl(port, buf, nl)  _outsl_ns((u32 *)((port)+pci_io_base), (buf), (nl))
 
-#endif
+#endif /* CONFIG_PPC_ISERIES */
 
 #define readb_relaxed(addr) readb(addr)
 #define readw_relaxed(addr) readw(addr)
@@ -390,9 +358,42 @@
 	__asm__ __volatile__("std%U0%X0 %1,%0; sync" : "=m" (*addr) : "r" (val));
 }
 
-#ifndef CONFIG_PPC_ISERIES 
+#ifndef CONFIG_PPC_ISERIES
 #include <asm/eeh.h>
-#endif
+
+static inline unsigned char __raw_readb(const volatile void __iomem *addr)
+{
+	return *(volatile unsigned char *) IO_TOKEN_TO_ADDR(addr);
+}
+static inline unsigned short __raw_readw(const volatile void __iomem *addr)
+{
+	return *(volatile unsigned short *) IO_TOKEN_TO_ADDR(addr);
+}
+static inline unsigned int __raw_readl(const volatile void __iomem *addr)
+{
+	return *(volatile unsigned int *) IO_TOKEN_TO_ADDR(addr);
+}
+static inline unsigned long __raw_readq(const volatile void __iomem *addr)
+{
+	return *(volatile unsigned long *) IO_TOKEN_TO_ADDR(addr);
+}
+static inline void __raw_writeb(unsigned char v, volatile void __iomem *addr)
+{
+	*(volatile unsigned char *) IO_TOKEN_TO_ADDR(addr) = v;
+}
+static inline void __raw_writew(unsigned short v, volatile void __iomem *addr)
+{
+	*(volatile unsigned short *) IO_TOKEN_TO_ADDR(addr) = v;
+}
+static inline void __raw_writel(unsigned int v, volatile void __iomem *addr)
+{
+	*(volatile unsigned int *) IO_TOKEN_TO_ADDR(addr) = v;
+}
+static inline void __raw_writeq(unsigned long v, volatile void __iomem *addr)
+{
+	*(volatile unsigned long *) IO_TOKEN_TO_ADDR(addr) = v;
+}
+#endif /* CONFIG_PPC_ISERIES */
 
 #ifdef __KERNEL__
 

Re: [PATCH] ppc64: Fix __raw_* IO accessors

[Linus Torvalds]

On Tue, 21 Sep 2004, Roland Dreier wrote:
> 
> That means using __raw_writel() is pretty much guaranteed to blow up
> on IBM pSeries (and I do care about pSeries for my driver).

Oh, that's true. And that's pretty clearly a bug, since it just means that 
__raw_writel() can't even work in general.

> Maybe something like the patch below would make sense?  (Reordering of
> code is to make sure IO_TOKEN_TO_ADDR() is defined before the
> __raw_*() functions; eeh.h has to be included after the in_*() and
> out_*() functions are defined)

I wonder if we could just remove the TOKEN/ADDR games. I think they were 
done entirely as a debugging aid (but I could be wrong). In particular, 
the compile-time type safefy should hopefully be better at finding these 
things in the long run, and in the short run the TOKEN games have 
obviously played their part.

I wasn't using pp64 back when, maybe there's some other reason for playing
games with the tokens? Who's the guity/knowledgeable party? Ben?

		Linus

Re: [PATCH] ppc64: Fix __raw_* IO accessors

[Benjamin Herrenschmidt]

On Wed, 2004-09-22 at 08:05, Roland Dreier wrote:

> That means using __raw_writel() is pretty much guaranteed to blow up
> on IBM pSeries (and I do care about pSeries for my driver).

Yah, this is junk, they should filter out the token at least even if
they don't do the actual checking. I don't know why somebody did that
token thing in the first place, I'll do some investigations, but I hate
it. Note that only devices for which eeh has been enabled will be
affected.

> Maybe something like the patch below would make sense?  (Reordering of
> code is to make sure IO_TOKEN_TO_ADDR() is defined before the
> __raw_*() functions; eeh.h has to be included after the in_*() and
> out_*() functions are defined)
> 
> By the way, I notice that <asm-ppc64/eeh.h> has a bunch of eeh_raw_*
> functions that appear to be completely unused.  I didn't use them in
> my patch because they add memory ordering (isync or sync) that Alan
> says __raw_* functions shouldn't have.
> 
>     Linus> Ok, so that _is_ insane. Mind telling what kind of insane
>     Linus> hardware is BE in this day and age?
> 
> :) Mellanox InfiniBand HCAs....

Note that I intend to clean up that mess sooner or later...

Your patch looks ok.

Ben.

Re: [PATCH] ppc64: Fix __raw_* IO accessors

[Benjamin Herrenschmidt]

On Wed, 2004-09-22 at 08:16, Linus Torvalds wrote:
> On Tue, 21 Sep 2004, Roland Dreier wrote:
> > 
> > That means using __raw_writel() is pretty much guaranteed to blow up
> > on IBM pSeries (and I do care about pSeries for my driver).
> 
> Oh, that's true. And that's pretty clearly a bug, since it just means that 
> __raw_writel() can't even work in general.

Only when eeh is enabled for a device, never happens on your g5 :)

> > Maybe something like the patch below would make sense?  (Reordering of
> > code is to make sure IO_TOKEN_TO_ADDR() is defined before the
> > __raw_*() functions; eeh.h has to be included after the in_*() and
> > out_*() functions are defined)
> 
> I wonder if we could just remove the TOKEN/ADDR games. I think they were 
> done entirely as a debugging aid (but I could be wrong). In particular, 
> the compile-time type safefy should hopefully be better at finding these 
> things in the long run, and in the short run the TOKEN games have 
> obviously played their part.
> 
> I wasn't using pp64 back when, maybe there's some other reason for playing
> games with the tokens? Who's the guity/knowledgeable party? Ben?

Well, g5 will never play token games on you. I need to investigate a bit
about what's up with pSeries, in the meantim, Roland patch looks fine.

There still is that issue with __raw_* doing both barrier-less and
endianswap-less accesses though. I think there is a fundamental problem
here with drivers like matroxfb using them to get endian-less access and
losing barriers at the same time.

I'd rather have matroxfb use writel with an explicit swap, or better, the
driver could maybe disable big endian register access and switch the card
to little endian, provided it can do that while keeping the frame buffer
itself set to BE (which is necessary most of the time).

Petr ?

Ben.

Re: [PATCH] ppc64: Fix __raw_* IO accessors

[Paul Mackerras]

Linus Torvalds writes:

> I wasn't using pp64 back when, maybe there's some other reason for playing
> games with the tokens? Who's the guity/knowledgeable party? Ben?

I believe this was done so that we could quickly work out the pci
bus/device/function inside read/write[bwl].  We needed that for coping
with the "Enhanced Error Handling" (EEH) stuff on pSeries machines.
I think we used to stuff the pci bus/dev/fn in the high bits and then
change the top 4 bits to make quite clear that it wasn't a valid
pointer.  These days we don't put the pci bus/dev/fn in the high bits
and we could certainly get rid of the IO_TOKEN_TO_ADDR games.

This stuff predates Ben's involvement in ppc64 by a long way, and was
put in before Anton or I had much influence.  We'll clean it up.

Paul.

Re: [PATCH] ppc64: Fix __raw_* IO accessors

[Roland Dreier]

    Paul> I believe this was done so that we could quickly work out
    Paul> the pci bus/device/function inside read/write[bwl].  We
    Paul> needed that for coping with the "Enhanced Error Handling"
    Paul> (EEH) stuff on pSeries machines.  I think we used to stuff
    Paul> the pci bus/dev/fn in the high bits and then change the top
    Paul> 4 bits to make quite clear that it wasn't a valid pointer.
    Paul> These days we don't put the pci bus/dev/fn in the high bits
    Paul> and we could certainly get rid of the IO_TOKEN_TO_ADDR
    Paul> games.

Here's a patch that gets rid of IO_TOKEN_TO_ADDR.  It's lightly tested
on my p630 (boots fine and my InfiniBand driver works well).

Thanks,
  Roland


Get rid of IO_TOKEN_TO_ADDR() and IO_ADDR_TO_TOKEN() for pSeries EEH;
the conversion to tokens is not needed now that we have __iomem
annotations to prevent drivers from dereferencing IO addresses.

Signed-off-by: Roland Dreier <roland@topspin.com>

Index: linux-bk/arch/ppc64/kernel/eeh.c
===================================================================
--- linux-bk.orig/arch/ppc64/kernel/eeh.c	2004-09-21 14:55:33.000000000 -0700
+++ linux-bk/arch/ppc64/kernel/eeh.c	2004-09-22 00:14:25.000000000 -0700
@@ -335,26 +335,19 @@
 
 /**
  * eeh_token_to_phys - convert EEH address token to phys address
- * @token i/o token, should be address in the form 0xA....
- *
- * Converts EEH address tokens into physical addresses.  Note that
- * ths routine does *not* convert I/O BAR addresses (which start
- * with 0xE...) to phys addresses!
+ * @token i/o token, should be address in the form 0xE....
  */
 static inline unsigned long eeh_token_to_phys(unsigned long token)
 {
 	pte_t *ptep;
-	unsigned long pa, vaddr;
+	unsigned long pa;
 
-	if (REGION_ID(token) == EEH_REGION_ID)
-		vaddr = IO_TOKEN_TO_ADDR(token);
-	else
+	ptep = find_linux_pte(ioremap_mm.pgd, token);
+	if (!ptep)
 		return token;
-
-	ptep = find_linux_pte(ioremap_mm.pgd, vaddr);
 	pa = pte_pfn(*ptep) << PAGE_SHIFT;
 
-	return pa | (vaddr & (PAGE_SIZE-1));
+	return pa | (token & (PAGE_SIZE-1));
 }
 
 /**
@@ -473,7 +466,7 @@
 	struct device_node *dn;
 
 	/* Finding the phys addr + pci device; this is pretty quick. */
-	addr = eeh_token_to_phys((unsigned long)token);
+	addr = eeh_token_to_phys((unsigned long __force) token);
 	dev = pci_get_device_by_addr(addr);
 	if (!dev)
 		return val;
@@ -750,15 +743,15 @@
  */
 void ioread8_rep(void __iomem *addr, void *dst, unsigned long count)
 {
-	_insb((void *) IO_TOKEN_TO_ADDR(addr), dst, count);
+	_insb((u8 __force *) addr, dst, count);
 }
 void ioread16_rep(void __iomem *addr, void *dst, unsigned long count)
 {
-	_insw_ns((void *) IO_TOKEN_TO_ADDR(addr), dst, count);
+	_insw_ns((u16 __force *) addr, dst, count);
 }
 void ioread32_rep(void __iomem *addr, void *dst, unsigned long count)
 {
-	_insl_ns((void *) IO_TOKEN_TO_ADDR(addr), dst, count);
+	_insl_ns((u32 __force *) addr, dst, count);
 }
 EXPORT_SYMBOL(ioread8_rep);
 EXPORT_SYMBOL(ioread16_rep);
@@ -766,15 +759,15 @@
 
 void iowrite8_rep(void __iomem *addr, const void *src, unsigned long count)
 {
-	_outsb((void *) IO_TOKEN_TO_ADDR(addr), src, count);
+	_outsb((u8 __force *) addr, src, count);
 }
 void iowrite16_rep(void __iomem *addr, const void *src, unsigned long count)
 {
-	_outsw_ns((void *) IO_TOKEN_TO_ADDR(addr), src, count);
+	_outsw_ns((u16 __force *) addr, src, count);
 }
 void iowrite32_rep(void __iomem *addr, const void *src, unsigned long count)
 {
-	_outsl_ns((void *) IO_TOKEN_TO_ADDR(addr), src, count);
+	_outsl_ns((u32 __force *) addr, src, count);
 }
 EXPORT_SYMBOL(iowrite8_rep);
 EXPORT_SYMBOL(iowrite16_rep);
@@ -784,7 +777,7 @@
 {
 	if (!_IO_IS_VALID(port))
 		return NULL;
-	return (void __iomem *) IO_ADDR_TO_TOKEN(port+pci_io_base);
+	return (void __iomem *) (port+pci_io_base);
 }
 
 void ioport_unmap(void __iomem *addr)
@@ -806,15 +799,8 @@
 		len = max;
 	if (flags & IORESOURCE_IO)
 		return ioport_map(start, len);
-	if (flags & IORESOURCE_MEM) {
-		void __iomem *vaddr = (void __iomem *) start;
-		if (dev  && eeh_subsystem_enabled) {
-			struct device_node *dn = pci_device_to_OF_node(dev);
-			if (dn && !(dn->eeh_mode & EEH_MODE_NOCHECK))
-				return (void __iomem *) IO_ADDR_TO_TOKEN(vaddr);
-		}
-		return vaddr;
-	}
+	if (flags & IORESOURCE_MEM)
+		return (void __iomem *) start;
 	/* What? */
 	return NULL;
 }
@@ -826,39 +812,6 @@
 EXPORT_SYMBOL(pci_iomap);
 EXPORT_SYMBOL(pci_iounmap);
 
-/*
- * If EEH is implemented, find the PCI device using given phys addr
- * and check to see if eeh failure checking is disabled.
- * Remap the addr (trivially) to the EEH region if EEH checking enabled.
- * For addresses not known to PCI the vaddr is simply returned unchanged.
- */
-void __iomem *eeh_ioremap(unsigned long addr, void __iomem *vaddr)
-{
-	struct pci_dev *dev;
-	struct device_node *dn;
-
-	if (!eeh_subsystem_enabled)
-		return vaddr;
-
-	dev = pci_get_device_by_addr(addr);
-	if (!dev)
-		return vaddr;
-
-	dn = pci_device_to_OF_node(dev);
-	if (!dn) {
-		pci_dev_put(dev);
-		return vaddr;
-	}
-
-	if (dn->eeh_mode & EEH_MODE_NOCHECK) {
-		pci_dev_put(dev);
-		return vaddr;
-	}
-
-	pci_dev_put(dev);
-	return (void __iomem *)IO_ADDR_TO_TOKEN(vaddr);
-}
-
 static int proc_eeh_show(struct seq_file *m, void *v)
 {
 	unsigned int cpu;
Index: linux-bk/arch/ppc64/mm/init.c
===================================================================
--- linux-bk.orig/arch/ppc64/mm/init.c	2004-09-21 14:55:02.000000000 -0700
+++ linux-bk/arch/ppc64/mm/init.c	2004-09-21 19:42:58.000000000 -0700
@@ -203,10 +203,7 @@
 void __iomem *
 ioremap(unsigned long addr, unsigned long size)
 {
-	void __iomem *ret = __ioremap(addr, size, _PAGE_NO_CACHE);
-	if(mem_init_done)
-		return eeh_ioremap(addr, ret);	/* may remap the addr */
-	return ret;
+	return __ioremap(addr, size, _PAGE_NO_CACHE);
 }
 
 void __iomem *
@@ -363,9 +360,7 @@
 		return;
 	}
 	
-	/* addr could be in EEH or IO region, map it to IO region regardless.
-	 */
-	addr = (void *) (IO_TOKEN_TO_ADDR(token) & PAGE_MASK);
+	addr = (void *) ((unsigned long __force) token & PAGE_MASK);
 	
 	if ((size = im_free(addr)) == 0) {
 		return;
@@ -415,9 +410,7 @@
 	unsigned long addr;
 	int rc;
 	
-	/* addr could be in EEH or IO region, map it to IO region regardless.
-	 */
-	addr = (IO_TOKEN_TO_ADDR(start) & PAGE_MASK);
+	addr = (unsigned long __force) start & PAGE_MASK;
 
 	/* Verify that the region either exists or is a subset of an existing
 	 * region.  In the latter case, split the parent region to create 
Index: linux-bk/include/asm-ppc64/eeh.h
===================================================================
--- linux-bk.orig/include/asm-ppc64/eeh.h	2004-09-21 14:57:25.000000000 -0700
+++ linux-bk/include/asm-ppc64/eeh.h	2004-09-21 19:39:19.000000000 -0700
@@ -26,18 +26,6 @@
 struct pci_dev;
 struct device_node;
 
-/* I/O addresses are converted to EEH "tokens" such that a driver will cause
- * a bad page fault if the address is used directly (i.e. these addresses are
- * never actually mapped.  Translation between IO <-> EEH region is 1 to 1.
- */
-#define IO_TOKEN_TO_ADDR(token) \
-	(((unsigned long __force)(token) & ~(0xfUL << REGION_SHIFT)) | \
-	(IO_REGION_ID << REGION_SHIFT))
-
-#define IO_ADDR_TO_TOKEN(addr) \
-	(((unsigned long)(addr) & ~(0xfUL << REGION_SHIFT)) | \
-	(EEH_REGION_ID << REGION_SHIFT))
-
 /* Values for eeh_mode bits in device_node */
 #define EEH_MODE_SUPPORTED	(1<<0)
 #define EEH_MODE_NOCHECK	(1<<1)
@@ -109,83 +97,83 @@
  * MMIO read/write operations with EEH support.
  */
 static inline u8 eeh_readb(const volatile void __iomem *addr) {
-	volatile u8 *vaddr = (volatile u8 *)IO_TOKEN_TO_ADDR(addr);
+	volatile u8 *vaddr = (volatile u8 __force *) addr;
 	u8 val = in_8(vaddr);
 	if (EEH_POSSIBLE_ERROR(addr, vaddr, val, u8))
 		return eeh_check_failure(addr, val);
 	return val;
 }
 static inline void eeh_writeb(u8 val, volatile void __iomem *addr) {
-	volatile u8 *vaddr = (volatile u8 *)IO_TOKEN_TO_ADDR(addr);
+	volatile u8 *vaddr = (volatile u8 __force *) addr;
 	out_8(vaddr, val);
 }
 
 static inline u16 eeh_readw(const volatile void __iomem *addr) {
-	volatile u16 *vaddr = (volatile u16 *)IO_TOKEN_TO_ADDR(addr);
+	volatile u16 *vaddr = (volatile u16 __force *) addr;
 	u16 val = in_le16(vaddr);
 	if (EEH_POSSIBLE_ERROR(addr, vaddr, val, u16))
 		return eeh_check_failure(addr, val);
 	return val;
 }
 static inline void eeh_writew(u16 val, volatile void __iomem *addr) {
-	volatile u16 *vaddr = (volatile u16 *)IO_TOKEN_TO_ADDR(addr);
+	volatile u16 *vaddr = (volatile u16 __force *) addr;
 	out_le16(vaddr, val);
 }
 static inline u16 eeh_raw_readw(const volatile void __iomem *addr) {
-	volatile u16 *vaddr = (volatile u16 *)IO_TOKEN_TO_ADDR(addr);
+	volatile u16 *vaddr = (volatile u16 __force *) addr;
 	u16 val = in_be16(vaddr);
 	if (EEH_POSSIBLE_ERROR(addr, vaddr, val, u16))
 		return eeh_check_failure(addr, val);
 	return val;
 }
 static inline void eeh_raw_writew(u16 val, volatile void __iomem *addr) {
-	volatile u16 *vaddr = (volatile u16 *)IO_TOKEN_TO_ADDR(addr);
+	volatile u16 *vaddr = (volatile u16 __force *) addr;
 	out_be16(vaddr, val);
 }
 
 static inline u32 eeh_readl(const volatile void __iomem *addr) {
-	volatile u32 *vaddr = (volatile u32 *)IO_TOKEN_TO_ADDR(addr);
+	volatile u32 *vaddr = (volatile u32 __force *) addr;
 	u32 val = in_le32(vaddr);
 	if (EEH_POSSIBLE_ERROR(addr, vaddr, val, u32))
 		return eeh_check_failure(addr, val);
 	return val;
 }
 static inline void eeh_writel(u32 val, volatile void __iomem *addr) {
-	volatile u32 *vaddr = (volatile u32 *)IO_TOKEN_TO_ADDR(addr);
+	volatile u32 *vaddr = (volatile u32 __force *) addr;
 	out_le32(vaddr, val);
 }
 static inline u32 eeh_raw_readl(const volatile void __iomem *addr) {
-	volatile u32 *vaddr = (volatile u32 *)IO_TOKEN_TO_ADDR(addr);
+	volatile u32 *vaddr = (volatile u32 __force *) addr;
 	u32 val = in_be32(vaddr);
 	if (EEH_POSSIBLE_ERROR(addr, vaddr, val, u32))
 		return eeh_check_failure(addr, val);
 	return val;
 }
 static inline void eeh_raw_writel(u32 val, volatile void __iomem *addr) {
-	volatile u32 *vaddr = (volatile u32 *)IO_TOKEN_TO_ADDR(addr);
+	volatile u32 *vaddr = (volatile u32 __force *) addr;
 	out_be32(vaddr, val);
 }
 
 static inline u64 eeh_readq(const volatile void __iomem *addr) {
-	volatile u64 *vaddr = (volatile u64 *)IO_TOKEN_TO_ADDR(addr);
+	volatile u64 *vaddr = (volatile u64 __force *) addr;
 	u64 val = in_le64(vaddr);
 	if (EEH_POSSIBLE_ERROR(addr, vaddr, val, u64))
 		return eeh_check_failure(addr, val);
 	return val;
 }
 static inline void eeh_writeq(u64 val, volatile void __iomem *addr) {
-	volatile u64 *vaddr = (volatile u64 *)IO_TOKEN_TO_ADDR(addr);
+	volatile u64 *vaddr = (volatile u64 __force *) addr;
 	out_le64(vaddr, val);
 }
 static inline u64 eeh_raw_readq(const volatile void __iomem *addr) {
-	volatile u64 *vaddr = (volatile u64 *)IO_TOKEN_TO_ADDR(addr);
+	volatile u64 *vaddr = (volatile u64 __force *) addr;
 	u64 val = in_be64(vaddr);
 	if (EEH_POSSIBLE_ERROR(addr, vaddr, val, u64))
 		return eeh_check_failure(addr, val);
 	return val;
 }
 static inline void eeh_raw_writeq(u64 val, volatile void __iomem *addr) {
-	volatile u64 *vaddr = (volatile u64 *)IO_TOKEN_TO_ADDR(addr);
+	volatile u64 *vaddr = (volatile u64 __force *) addr;
 	out_be64(vaddr, val);
 }
 
@@ -193,7 +181,7 @@
 	((((unsigned long)(v)) & ((a) - 1)) == 0)
 
 static inline void eeh_memset_io(volatile void __iomem *addr, int c, unsigned long n) {
-	void *vaddr = (void *)IO_TOKEN_TO_ADDR(addr);
+	void *vaddr = (void __force *) addr;
 	u32 lc = c;
 	lc |= lc << 8;
 	lc |= lc << 16;
@@ -216,7 +204,7 @@
 	__asm__ __volatile__ ("sync" : : : "memory");
 }
 static inline void eeh_memcpy_fromio(void *dest, const volatile void __iomem *src, unsigned long n) {
-	void *vsrc = (void *)IO_TOKEN_TO_ADDR(src);
+	void *vsrc = (void __force *) src;
 	void *vsrcsave = vsrc, *destsave = dest;
 	const volatile void __iomem *srcsave = src;
 	unsigned long nsave = n;
@@ -255,7 +243,7 @@
 }
 
 static inline void eeh_memcpy_toio(volatile void __iomem *dest, const void *src, unsigned long n) {
-	void *vdest = (void *)IO_TOKEN_TO_ADDR(dest);
+	void *vdest = (void __force *) dest;
 
 	while(n && (!EEH_CHECK_ALIGN(vdest, 4) || !EEH_CHECK_ALIGN(src, 4))) {
 		*((volatile u8 *)vdest) = *((u8 *)src);

Re: [PATCH] ppc64: Fix __raw_* IO accessors

[Petr Vandrovec]

On Wed, Sep 22, 2004 at 11:34:57AM +1000, Benjamin Herrenschmidt wrote:
> On Wed, 2004-09-22 at 08:16, Linus Torvalds wrote:
> 
> Well, g5 will never play token games on you. I need to investigate a bit
> about what's up with pSeries, in the meantim, Roland patch looks fine.
> 
> There still is that issue with __raw_* doing both barrier-less and
> endianswap-less accesses though. I think there is a fundamental problem
> here with drivers like matroxfb using them to get endian-less access and
> losing barriers at the same time.

Before I put __raw_* there, code was using direct *(u_int32_t*)(mmio +
reg) = value, and nobody complained... (and it worked on my PReP box).
It seems that PPC does not reorder concurrent writes targetting one
device.

> I'd rather have matroxfb use writel with an explicit swap, or better, the
> driver could maybe disable big endian register access and switch the card
> to little endian, provided it can do that while keeping the frame buffer
> itself set to BE (which is necessary most of the time).

It is due to compatibility with XFree (or at least I was told) - they want 
both framebuffer and accelerator in big-endian mode, so there is really no
choice (other than not supporting ppc...).

But of course, I can use writel(swab(...)) to get big-endian PCI
accesses if __raw_* does not work on your hardware...
							Petr Vandrovec

Re: [PATCH] ppc64: Fix __raw_* IO accessors

[Benjamin Herrenschmidt]

On Thu, 2004-09-23 at 04:58, Petr Vandrovec wrote:

> > There still is that issue with __raw_* doing both barrier-less and
> > endianswap-less accesses though. I think there is a fundamental problem
> > here with drivers like matroxfb using them to get endian-less access and
> > losing barriers at the same time.
> 
> Before I put __raw_* there, code was using direct *(u_int32_t*)(mmio +
> reg) = value, and nobody complained... (and it worked on my PReP box).
> It seems that PPC does not reorder concurrent writes targetting one
> device.

It may re-order write vs. reads, nobody complained because on most old
machines, the CPU would be too dumb to do really heavy re-ordering but
that is no longer the case. This is definitely a bug.

> > I'd rather have matroxfb use writel with an explicit swap, or better, the
> > driver could maybe disable big endian register access and switch the card
> > to little endian, provided it can do that while keeping the frame buffer
> > itself set to BE (which is necessary most of the time).
>
> It is due to compatibility with XFree (or at least I was told) - they want 
> both framebuffer and accelerator in big-endian mode, so there is really no
> choice (other than not supporting ppc...).

Hrm... having a quick look at mga driver in current Xorg tree, it uses
the MMIO_IN/OUT macros directly, those are not byteswapping ?

It also does this at one point (ugh !) :

#if X_BYTE_ORDER == X_BIG_ENDIAN
	/* Disable byte-swapping for big-endian architectures - the XFree
	   driver seems to like a little-endian framebuffer -ReneR */
	/* pReg->Option |= 0x80000000; */
	pReg->Option &= ~0x80000000;
#endif

Weird... I think the X driver just lacks any "knowledge" of what's going
on with endianness...

> But of course, I can use writel(swab(...)) to get big-endian PCI
> accesses if __raw_* does not work on your hardware...

It's not that "__raw_*" does not work for my hardware ... it's that __raw_*
is always wrong to use on MMIO register accesses (unless you know _exactly_ 
what you are doing, for example it may be acceptable for filling a fifo in
some cases provided the first & last writes are not __raw)

Ben.

Re: [PATCH] ppc64: Fix __raw_* IO accessors

[Petr Vandrovec]

On Thu, Sep 23, 2004 at 10:49:00AM +1000, Benjamin Herrenschmidt wrote:
> On Thu, 2004-09-23 at 04:58, Petr Vandrovec wrote:
> 
> > > I'd rather have matroxfb use writel with an explicit swap, or better, the
> > > driver could maybe disable big endian register access and switch the card
> > > to little endian, provided it can do that while keeping the frame buffer
> > > itself set to BE (which is necessary most of the time).
> >
> > It is due to compatibility with XFree (or at least I was told) - they want 
> > both framebuffer and accelerator in big-endian mode, so there is really no
> > choice (other than not supporting ppc...).
> 
> Hrm... having a quick look at mga driver in current Xorg tree, it uses
> the MMIO_IN/OUT macros directly, those are not byteswapping ?
> 
> It also does this at one point (ugh !) :
> 
> #if X_BYTE_ORDER == X_BIG_ENDIAN
> 	/* Disable byte-swapping for big-endian architectures - the XFree
> 	   driver seems to like a little-endian framebuffer -ReneR */
> 	/* pReg->Option |= 0x80000000; */
> 	pReg->Option &= ~0x80000000;
> #endif
> 
> Weird... I think the X driver just lacks any "knowledge" of what's going
> on with endianness...

Ok.  Can somebody tell me what byte order should be used for framebuffer
and for MMIO on PPC/PPC64 then?  From cfb* it seems that framebuffer
have to be in big-endian mode, and from Xorg code it seems that MMIO should 
be always in little-endian.  Yes?

							Petr Vandrovec

[PATCH] matroxfb big-endian update (was Re: [PATCH] ppc64: Fix __raw_* IO accessors)

[Petr Vandrovec]

Hi Andrew & Linus,

This change disconnects matroxfb accelerator endianess from processor endianess, plus
ports big-endian accessors from __raw_xxx to xxx + appropriate byte swaps.

Now you can select accelerator endianess during 'make config', while framebuffer endianess
depends on processor endianess (I hope that this is correct byte ordering for framebuffer).

Originally I tried to make CONFIG_FB_MATROX_BIG_ENDIAN_ACCEL depending on BE hardware (so
LE users could not screw their X servers), but unfortunately there does not seem to be generic
BIG_ENDIAN configuration option, and list of BE architectures seemed too long to me
(depends on FB_MATROX && (ARCH_S390 || ARM || H8300 || M32R || M69K || MIPS || PARISC || PPC ||
   SPARC32 || SPARC64 || SUPERH) ), so now also little-endian users can run accelerator
in BE mode.

If you prefer just dropping support for BE accel instead (and BE users will have to cope),
just tell me.

It also fixes small typo - M_C2CTL should be 0x3C10 and not 0x3E10.  Apparently Matrox notes
about need to program this register during initialization are not so important...

Patch was tested only on little-endian hardware, with both values for CONFIG_FB_MATROX_BIG_ENDIAN_ACCEL
(and I verified that hardware actually was in BE mode).  Patch applies to current -bk, and
with small offset in video/Kconfig also to 2.6.9-rc2-mm2.



Signed-off-by: Petr Vandrovec <vandrove@vc.cvut.cz>



diff -urdN linux/drivers/video/Kconfig linux/drivers/video/Kconfig
--- linux/drivers/video/Kconfig	2004-09-23 15:39:47.000000000 +0000
+++ linux/drivers/video/Kconfig	2004-09-23 18:44:50.000000000 +0000
@@ -654,6 +654,16 @@
 	  There is no need for enabling 'Matrox multihead support' if you have
 	  only one Matrox card in the box.
 
+config FB_MATROX_BIG_ENDIAN_ACCEL
+	bool "Drive accelerator in big-endian mode"
+	depends on FB_MATROX
+	---help---
+	  Say Y here if you are using applications which depend on Matrox
+	  device being in big-endian mode.  Note that it is not recommended
+	  to run device in big-endian mode on little-endian systems.
+
+	  If you do not know what this option talks about, say N.
+
 config FB_RADEON_OLD
 	tristate "ATI Radeon display support (Old driver)"
 	depends on FB && PCI
diff -urdN linux/drivers/video/matrox/matroxfb_accel.c linux/drivers/video/matrox/matroxfb_accel.c
--- linux/drivers/video/matrox/matroxfb_accel.c	2004-09-23 15:46:44.000000000 +0000
+++ linux/drivers/video/matrox/matroxfb_accel.c	2004-09-23 20:12:25.000000000 +0000
@@ -432,32 +432,24 @@
 	mga_writel(mmio, M_AR3, 0);
 	if (easy) {
 		mga_writel(mmio, M_YDSTLEN | M_EXEC, ydstlen);
-		mga_memcpy_toio(mmio, 0, chardata, xlen);
+		mga_memcpy_toio(mmio, chardata, xlen);
 	} else {
 		mga_writel(mmio, M_AR5, 0);
 		mga_writel(mmio, M_YDSTLEN | M_EXEC, ydstlen);
 		if ((step & 3) == 0) {
 			/* Great. Source has 32bit aligned lines, so we can feed them
 			   directly to the accelerator. */
-			mga_memcpy_toio(mmio, 0, chardata, charcell);
+			mga_memcpy_toio(mmio, chardata, charcell);
 		} else if (step == 1) {
 			/* Special case for 1..8bit widths */
 			while (height--) {
-#ifdef __LITTLE_ENDIAN
-				mga_writel(mmio, 0, *chardata);
-#else
-				mga_writel(mmio, 0, (*chardata) << 24);
-#endif
+				writel(*chardata, mmio.vaddr);
 				chardata++;
 			}
 		} else if (step == 2) {
 			/* Special case for 9..15bit widths */
 			while (height--) {
-#ifdef __LITTLE_ENDIAN
-				mga_writel(mmio, 0, *(u_int16_t*)chardata);
-#else
-				mga_writel(mmio, 0, (*(u_int16_t*)chardata) << 16);
-#endif
+				writel(*(u_int16_t*)chardata, mmio.vaddr);
 				chardata += 2;
 			}
 		} else {
@@ -467,7 +459,7 @@
 				
 				for (i = 0; i < step; i += 4) {
 					/* Hope that there are at least three readable bytes beyond the end of bitmap */
-					mga_writel(mmio, 0, get_unaligned((u_int32_t*)(chardata + i)));
+					writel(get_unaligned((u_int32_t*)(chardata + i)), mmio.vaddr);
 				}
 				chardata += step;
 			}
diff -urdN linux/drivers/video/matrox/matroxfb_base.h linux/drivers/video/matrox/matroxfb_base.h
--- linux/drivers/video/matrox/matroxfb_base.h	2004-09-23 15:42:26.000000000 +0000
+++ linux/drivers/video/matrox/matroxfb_base.h	2004-09-23 20:15:13.000000000 +0000
@@ -99,17 +99,6 @@
 #endif
 #endif
 
-#if defined(__alpha__) || defined(__mc68000__) || defined(__i386__) || defined(__x86_64__)
-#define READx_WORKS
-#define MEMCPYTOIO_WORKS
-#else
-/* ppc/ppc64 must use __raw_{read,write}[bwl] as we drive adapter 
-   in big-endian mode for compatibility with XFree mga driver, and
-   so we do not want little-endian {read,write}[bwl] */
-#define READx_FAILS
-#define MEMCPYTOIO_WRITEL
-#endif
-
 #if defined(__mc68000__)
 #define MAP_BUSTOVIRT
 #else
@@ -155,86 +144,78 @@
 #endif
 
 typedef struct {
-	u_int8_t __iomem*	vaddr;
+	void __iomem*	vaddr;
 } vaddr_t;
 
-#ifdef READx_WORKS
 static inline unsigned int mga_readb(vaddr_t va, unsigned int offs) {
 	return readb(va.vaddr + offs);
 }
 
-static inline unsigned int mga_readw(vaddr_t va, unsigned int offs) {
-	return readw(va.vaddr + offs);
-}
-
-static inline u_int32_t mga_readl(vaddr_t va, unsigned int offs) {
-	return readl(va.vaddr + offs);
-}
-
 static inline void mga_writeb(vaddr_t va, unsigned int offs, u_int8_t value) {
 	writeb(value, va.vaddr + offs);
 }
 
-static inline void mga_writew(vaddr_t va, unsigned int offs, u_int16_t value) {
+static inline void mga_writew_le(vaddr_t va, unsigned int offs, u_int16_t value) {
 	writew(value, va.vaddr + offs);
 }
 
-static inline void mga_writel(vaddr_t va, unsigned int offs, u_int32_t value) {
-	writel(value, va.vaddr + offs);
-}
-#else
-static inline unsigned int mga_readb(vaddr_t va, unsigned int offs) {
-	return __raw_readb(va.vaddr + offs);
-}
-
-static inline unsigned int mga_readw(vaddr_t va, unsigned int offs) {
-	return __raw_readw(va.vaddr + offs);
+#ifdef CONFIG_FB_MATROX_BIG_ENDIAN_ACCEL
+static inline void mga_writew(vaddr_t va, unsigned int offs, u_int16_t value) {
+	mga_writew_le(va, offs, swab16(value));
 }
 
 static inline u_int32_t mga_readl(vaddr_t va, unsigned int offs) {
-	return __raw_readl(va.vaddr + offs);
+	return swab32(readl(va.vaddr + offs));
 }
 
-static inline void mga_writeb(vaddr_t va, unsigned int offs, u_int8_t value) {
-	__raw_writeb(value, va.vaddr + offs);
+static inline void mga_writel(vaddr_t va, unsigned int offs, u_int32_t value) {
+	writel(swab32(value), va.vaddr + offs);
 }
-
+#else
+#if defined(__alpha__) || defined(__i386__) || defined(__x86_64__)
+#define MEMCPYTOIO_WORKS
+#endif
 static inline void mga_writew(vaddr_t va, unsigned int offs, u_int16_t value) {
-	__raw_writew(value, va.vaddr + offs);
+	mga_writew_le(va, offs, value);
+}
+
+static inline u_int32_t mga_readl(vaddr_t va, unsigned int offs) {
+	return readl(va.vaddr + offs);
 }
 
 static inline void mga_writel(vaddr_t va, unsigned int offs, u_int32_t value) {
-	__raw_writel(value, va.vaddr + offs);
+	writel(value, va.vaddr + offs);
 }
 #endif
 
-static inline void mga_memcpy_toio(vaddr_t va, unsigned int offs, const void* src, int len) {
+static inline void mga_memcpy_toio(vaddr_t va, const void* src, int len) {
 #ifdef MEMCPYTOIO_WORKS
-	memcpy_toio(va.vaddr + offs, src, len);
-#elif defined(MEMCPYTOIO_WRITEL)
-	if (offs & 3) {
+	/*
+	 * memcpy_toio works for us if:
+	 *  (1) Copies data as 32bit quantities, not byte after byte,
+	 *  (2) Performs LE ordered stores, and
+	 *  (3) It copes with unaligned source (destination is guaranteed to be page
+	 *      aligned and length is guaranteed to be multiple of 4).
+	 */
+	memcpy_toio(va.vaddr, src, len);
+#else
+        u_int32_t __iomem* addr = va.vaddr;
+
+	if ((unsigned long)src & 3) {
 		while (len >= 4) {
-			mga_writel(va, offs, get_unaligned((u32 *)src));
-			offs += 4;
+			writel(get_unaligned((u32 *)src), addr);
+			addr++;
 			len -= 4;
 			src += 4;
 		}
 	} else {
 		while (len >= 4) {
-			mga_writel(va, offs, *(u32 *)src);
-			offs += 4;
+			writel(*(u32 *)src, addr);
+			addr++;
 			len -= 4;
 			src += 4;
 		}
 	}
-	if (len) {
-		u_int32_t tmp;
-
-		memcpy(&tmp, src, len);
-		mga_writel(va, offs, tmp);
-	}
-#else
-#error "Sorry, do not know how to write block of data to device"
 #endif
 }
 
@@ -774,11 +755,15 @@
 #define DAC_XGENIOCTRL		0x2A
 #define DAC_XGENIODATA		0x2B
 
-#define M_C2CTL		0x3E10
+#define M_C2CTL		0x3C10
 
-#ifdef __LITTLE_ENDIAN
-#define MX_OPTION_BSWAP		0x00000000
+#ifdef CONFIG_FB_MATROX_BIG_ENDIAN_ACCEL
+#define MX_OPTION_BSWAP		0x80000000
+#else
+#define MX_OPTION_BSWAP         0x00000000
+#endif
 
+#ifdef __LITTLE_ENDIAN
 #define M_OPMODE_4BPP	(M_OPMODE_DMA_LE | M_OPMODE_DIR_LE | M_OPMODE_DMA_BLIT)
 #define M_OPMODE_8BPP	(M_OPMODE_DMA_LE | M_OPMODE_DIR_LE | M_OPMODE_DMA_BLIT)
 #define M_OPMODE_16BPP	(M_OPMODE_DMA_LE | M_OPMODE_DIR_LE | M_OPMODE_DMA_BLIT)
@@ -786,29 +771,28 @@
 #define M_OPMODE_32BPP	(M_OPMODE_DMA_LE | M_OPMODE_DIR_LE | M_OPMODE_DMA_BLIT)
 #else
 #ifdef __BIG_ENDIAN
-#define MX_OPTION_BSWAP		0x80000000
-
-#define M_OPMODE_4BPP	(M_OPMODE_DMA_LE | M_OPMODE_DIR_LE | M_OPMODE_DMA_BLIT)	/* TODO */
+#define M_OPMODE_4BPP	(M_OPMODE_DMA_LE       | M_OPMODE_DIR_LE       | M_OPMODE_DMA_BLIT)	/* TODO */
 #define M_OPMODE_8BPP	(M_OPMODE_DMA_BE_8BPP  | M_OPMODE_DIR_BE_8BPP  | M_OPMODE_DMA_BLIT)
 #define M_OPMODE_16BPP	(M_OPMODE_DMA_BE_16BPP | M_OPMODE_DIR_BE_16BPP | M_OPMODE_DMA_BLIT)
-#define M_OPMODE_24BPP	(M_OPMODE_DMA_BE_8BPP | M_OPMODE_DIR_BE_8BPP | M_OPMODE_DMA_BLIT)	/* TODO, ?32 */
+#define M_OPMODE_24BPP	(M_OPMODE_DMA_BE_8BPP  | M_OPMODE_DIR_BE_8BPP  | M_OPMODE_DMA_BLIT)	/* TODO, ?32 */
 #define M_OPMODE_32BPP	(M_OPMODE_DMA_BE_32BPP | M_OPMODE_DIR_BE_32BPP | M_OPMODE_DMA_BLIT)
 #else
 #error "Byte ordering have to be defined. Cannot continue."
 #endif
 #endif
 
-#define mga_inb(addr)	mga_readb(ACCESS_FBINFO(mmio.vbase), (addr))
-#define mga_inl(addr)	mga_readl(ACCESS_FBINFO(mmio.vbase), (addr))
-#define mga_outb(addr,val) mga_writeb(ACCESS_FBINFO(mmio.vbase), (addr), (val))
-#define mga_outw(addr,val) mga_writew(ACCESS_FBINFO(mmio.vbase), (addr), (val))
-#define mga_outl(addr,val) mga_writel(ACCESS_FBINFO(mmio.vbase), (addr), (val))
-#define mga_readr(port,idx) (mga_outb((port),(idx)), mga_inb((port)+1))
-#ifdef __LITTLE_ENDIAN
-#define mga_setr(addr,port,val) mga_outw(addr, ((val)<<8) | (port))
-#else
-#define mga_setr(addr,port,val) do { mga_outb(addr, port); mga_outb((addr)+1, val); } while (0)
-#endif
+#define mga_inb(addr)		mga_readb(ACCESS_FBINFO(mmio.vbase), (addr))
+#define mga_inl(addr)		mga_readl(ACCESS_FBINFO(mmio.vbase), (addr))
+#define mga_outb(addr,val)	mga_writeb(ACCESS_FBINFO(mmio.vbase), (addr), (val))
+#define mga_outw(addr,val)	mga_writew(ACCESS_FBINFO(mmio.vbase), (addr), (val))
+#define mga_outw_le(addr,val)	mga_writew_le(ACCESS_FBINFO(mmio.vbase), (addr), (val))
+#define mga_outl(addr,val)	mga_writel(ACCESS_FBINFO(mmio.vbase), (addr), (val))
+#define mga_readr(port,idx)	(mga_outb((port),(idx)), mga_inb((port)+1))
+/*
+ * 1F00-1FFF and 3C00-3C0F ranges are not byteswapped (doc says 3C00-3CFF, but it
+ * is incorrect, 3C10-3CFF are swapped)
+ */
+#define mga_setr(addr,port,val)	mga_outw_le(addr, ((val)<<8) | (port))
 
 #define mga_fifo(n)	do {} while ((mga_inl(M_FIFOSTATUS) & 0xFF) < (n))
 

Re: [PATCH] ppc64: Fix __raw_* IO accessors

[Benjamin Herrenschmidt]

On Fri, 2004-09-24 at 01:25, Petr Vandrovec wrote:

> 
> Ok.  Can somebody tell me what byte order should be used for framebuffer
> and for MMIO on PPC/PPC64 then?  From cfb* it seems that framebuffer
> have to be in big-endian mode, and from Xorg code it seems that MMIO should 
> be always in little-endian.  Yes?

I don't know exactly what X.org does ...

In general, on a PCI bus, we expect MMIO to be little endian. Some cards
try to be "smart" and have an endian swap facility for MMIO (like nvidia,
and I think matrox) but I tend to consider that useless since it force us
to have different IO accessors or to add an "un-byteswap" macro. The PPC
is very good at doing byteswapped accesses with one instruction so it
isn't really a waste to do all MMIOs littel endian anyway.

For the framebuffer, it's common practice to have it in big endian format
(and using the proper byteswap register for the access bit depth)

Ben.
 

Re: [PATCH] matroxfb big-endian update (was Re: [PATCH] ppc64: Fix __raw_* IO accessors)

[Benjamin Herrenschmidt]

On Fri, 2004-09-24 at 06:26, Petr Vandrovec wrote:
> Hi Andrew & Linus,
> 
> This change disconnects matroxfb accelerator endianess from processor endianess, plus
> ports big-endian accessors from __raw_xxx to xxx + appropriate byte swaps.

Applied to current bk, make oldconfig (FB_MATROX_BIG_ENDIAN_ACCEL is not set),
works like a charm on the ppc POWER3 I have here, haven't had a chance to
test X though.

Ben.

Re: [PATCH] matroxfb big-endian update (was Re: [PATCH] ppc64: Fix __raw_* IO accessors)

[Petr Vandrovec]

On Fri, Sep 24, 2004 at 04:25:37PM +1000, Benjamin Herrenschmidt wrote:
> On Fri, 2004-09-24 at 06:26, Petr Vandrovec wrote:
> > Hi Andrew & Linus,
> > 
> > This change disconnects matroxfb accelerator endianess from processor endianess, plus
> > ports big-endian accessors from __raw_xxx to xxx + appropriate byte swaps.
> 
> Applied to current bk, make oldconfig (FB_MATROX_BIG_ENDIAN_ACCEL is not set),
> works like a charm on the ppc POWER3 I have here, haven't had a chance to
> test X though.

Thanks. 

XFree 3.x did not touch BE mode bit and accessed MMIO directly with pointer 
dereference (expecting firmware to put card into BE mode?), while XFree 4.x (if
I understand code properly) does not touch BE bit on primary device
(while it clears it on secondary devices) while expecting hardware to be
in LE mode...

So I'm either confused, or XF3 needs BE_ACCEL set while XF4 needs BE_ACCEL
disabled.  Does anybody actually use matroxfb with XFree server on PPC (or any
other BE machine) at all?
							Petr Vandrovec

P.S.: Trimmed down CC list a bit. 

Re: [PATCH] matroxfb big-endian update (was Re: [PATCH] ppc64: Fix __raw_* IO accessors)

[Kostas Georgiou]

On Fri, 24 Sep 2004, Petr Vandrovec wrote:

> XFree 3.x did not touch BE mode bit and accessed MMIO directly with pointer 
> dereference (expecting firmware to put card into BE mode?), while XFree 4.x (if
> I understand code properly) does not touch BE bit on primary device
> (while it clears it on secondary devices) while expecting hardware to be
> in LE mode...
> 
> So I'm either confused, or XF3 needs BE_ACCEL set while XF4 needs BE_ACCEL
> disabled.  Does anybody actually use matroxfb with XFree server on PPC (or any
> other BE machine) at all?

We had almost the same discussion 4 years ago :) 
Have a look at: http://lists.debian.org/debian-powerpc/2001/01/msg00443.html

The driver in XFree 3.x never worked under ppc so don't worry about it.

Kostas

Re: [PATCH] matroxfb big-endian update (was Re: [PATCH] ppc64: Fix __raw_* IO accessors)

[Benjamin Herrenschmidt]

On Fri, 2004-09-24 at 19:53, Petr Vandrovec wrote:

> XFree 3.x did not touch BE mode bit and accessed MMIO directly with pointer 
> dereference (expecting firmware to put card into BE mode?), while XFree 4.x (if
> I understand code properly) does not touch BE bit on primary device
> (while it clears it on secondary devices) while expecting hardware to be
> in LE mode...
> 
> So I'm either confused, or XF3 needs BE_ACCEL set while XF4 needs BE_ACCEL
> disabled.  Does anybody actually use matroxfb with XFree server on PPC (or any
> other BE machine) at all?

People here tend to put a radeon card in their pSeries and forget about
the matrox one it seems :) I'll have a look next week.

Ben.