> diff -Nru a/drivers/video/riva/riva_hw.h b/drivers/video/riva/riva_hw.h
> --- a/drivers/video/riva/riva_hw.h 2004-11-07 21:21:47 -08:00
> +++ b/drivers/video/riva/riva_hw.h 2004-11-07 21:21:47 -08:00
> @@ -78,13 +78,13 @@
> #define NV_WR08(p,i,d) out_8(p+i, d)
> #define NV_RD08(p,i) in_8(p+i)
> #else
> -#define NV_WR08(p,i,d) (((U008 *)(p))[i]=(d))
> -#define NV_RD08(p,i) (((U008 *)(p))[i])
> +#define NV_WR08(p,i,d) (writeb((d), (u8 __iomem *)(p) + (i)))
> +#define NV_RD08(p,i) (readb((u8 __iomem *)(p) + (i)))
> #endif
> -#define NV_WR16(p,i,d) (((U016 *)(p))[(i)/2]=(d))
> -#define NV_RD16(p,i) (((U016 *)(p))[(i)/2])
> -#define NV_WR32(p,i,d) (((U032 *)(p))[(i)/4]=(d))
> -#define NV_RD32(p,i) (((U032 *)(p))[(i)/4])
> +#define NV_WR16(p,i,d) (writew((d), (u16 __iomem *)(p) + (i)/2))
> +#define NV_RD16(p,i) (readw((u16 __iomem *)(p) + (i)/2))
> +#define NV_WR32(p,i,d) (writel((d), (u32 __iomem *)(p) + (i)/4))
> +#define NV_RD32(p,i) (readl((u32 __iomem *)(p) + (i)/4))
> #define VGA_WR08(p,i,d) NV_WR08(p,i,d)
> #define VGA_RD08(p,i) NV_RD08(p,i)
You probably broke ppc versions here. The driver enables "big endian"
register access, but readw/writew/l functions do byteswap, which will
lead to incorrect results.
The fix would be to probably just remove the code that puts the chip
registers into big endian mode, this isn't necessary nor a very good
idea actually.
I don't have an nVidia card on ppc to test any more unfortunately.
Ben.
On Monday 08 November 2004 13:57, Benjamin Herrenschmidt wrote:
> > diff -Nru a/drivers/video/riva/riva_hw.h b/drivers/video/riva/riva_hw.h
> > --- a/drivers/video/riva/riva_hw.h 2004-11-07 21:21:47 -08:00
> > +++ b/drivers/video/riva/riva_hw.h 2004-11-07 21:21:47 -08:00
> > @@ -78,13 +78,13 @@
> > #define NV_WR08(p,i,d) out_8(p+i, d)
> > #define NV_RD08(p,i) in_8(p+i)
> > #else
> > -#define NV_WR08(p,i,d) (((U008 *)(p))[i]=(d))
> > -#define NV_RD08(p,i) (((U008 *)(p))[i])
> > +#define NV_WR08(p,i,d) (writeb((d), (u8 __iomem *)(p) + (i)))
> > +#define NV_RD08(p,i) (readb((u8 __iomem *)(p) + (i)))
> > #endif
> > -#define NV_WR16(p,i,d) (((U016 *)(p))[(i)/2]=(d))
> > -#define NV_RD16(p,i) (((U016 *)(p))[(i)/2])
> > -#define NV_WR32(p,i,d) (((U032 *)(p))[(i)/4]=(d))
> > -#define NV_RD32(p,i) (((U032 *)(p))[(i)/4])
> > +#define NV_WR16(p,i,d) (writew((d), (u16 __iomem *)(p) + (i)/2))
> > +#define NV_RD16(p,i) (readw((u16 __iomem *)(p) + (i)/2))
> > +#define NV_WR32(p,i,d) (writel((d), (u32 __iomem *)(p) + (i)/4))
> > +#define NV_RD32(p,i) (readl((u32 __iomem *)(p) + (i)/4))
> > #define VGA_WR08(p,i,d) NV_WR08(p,i,d)
> > #define VGA_RD08(p,i) NV_RD08(p,i)
>
> You probably broke ppc versions here. The driver enables "big endian"
> register access, but readw/writew/l functions do byteswap, which will
> lead to incorrect results.
>
> The fix would be to probably just remove the code that puts the chip
> registers into big endian mode, this isn't necessary nor a very good
> idea actually.
>
> I don't have an nVidia card on ppc to test any more unfortunately.
Hmm, I'll ask Guido Guenther if he can test the changes. I think a different
set of access macros for PPC might be a more preferrable solution.
Tony
On Monday 08 November 2004 13:57, Benjamin Herrenschmidt wrote:
> > diff -Nru a/drivers/video/riva/riva_hw.h b/drivers/video/riva/riva_hw.h
> > --- a/drivers/video/riva/riva_hw.h 2004-11-07 21:21:47 -08:00
> > +++ b/drivers/video/riva/riva_hw.h 2004-11-07 21:21:47 -08:00
> > @@ -78,13 +78,13 @@
> > #define NV_WR08(p,i,d) out_8(p+i, d)
> > #define NV_RD08(p,i) in_8(p+i)
> > #else
> > -#define NV_WR08(p,i,d) (((U008 *)(p))[i]=(d))
> > -#define NV_RD08(p,i) (((U008 *)(p))[i])
> > +#define NV_WR08(p,i,d) (writeb((d), (u8 __iomem *)(p) + (i)))
> > +#define NV_RD08(p,i) (readb((u8 __iomem *)(p) + (i)))
> > #endif
> > -#define NV_WR16(p,i,d) (((U016 *)(p))[(i)/2]=(d))
> > -#define NV_RD16(p,i) (((U016 *)(p))[(i)/2])
> > -#define NV_WR32(p,i,d) (((U032 *)(p))[(i)/4]=(d))
> > -#define NV_RD32(p,i) (((U032 *)(p))[(i)/4])
> > +#define NV_WR16(p,i,d) (writew((d), (u16 __iomem *)(p) + (i)/2))
> > +#define NV_RD16(p,i) (readw((u16 __iomem *)(p) + (i)/2))
> > +#define NV_WR32(p,i,d) (writel((d), (u32 __iomem *)(p) + (i)/4))
> > +#define NV_RD32(p,i) (readl((u32 __iomem *)(p) + (i)/4))
> > #define VGA_WR08(p,i,d) NV_WR08(p,i,d)
> > #define VGA_RD08(p,i) NV_RD08(p,i)
>
> You probably broke ppc versions here. The driver enables "big endian"
> register access, but readw/writew/l functions do byteswap, which will
> lead to incorrect results.
>
> The fix would be to probably just remove the code that puts the chip
> registers into big endian mode, this isn't necessary nor a very good
> idea actually.
>
How about this patch? This is almost the original macro in riva_hw.h,
with the __force annotation.
Signed-off-by: Antonino Daplas <[email protected]>
---
diff -Nru a/drivers/video/riva/riva_hw.h b/drivers/video/riva/riva_hw.h
--- a/drivers/video/riva/riva_hw.h 2004-11-06 12:42:19 +08:00
+++ b/drivers/video/riva/riva_hw.h 2004-11-08 16:59:34 +08:00
@@ -77,14 +77,18 @@
#include <asm/io.h>
#define NV_WR08(p,i,d) out_8(p+i, d)
#define NV_RD08(p,i) in_8(p+i)
+#define NV_WR16(p,i,d) (((volatile u16 __force *)(p))[(i)/2]=(d))
+#define NV_RD16(p,i) (((volatile u16 __force *)(p))[(i)/2])
+#define NV_WR32(p,i,d) (((volatile u32 __force *)(p))[(i)/4]=(d))
+#define NV_RD32(p,i) (((volatile u32 __force *)(p))[(i)/4])
#else
#define NV_WR08(p,i,d) (writeb((d), (u8 __iomem *)(p) + (i)))
#define NV_RD08(p,i) (readb((u8 __iomem *)(p) + (i)))
-#endif
#define NV_WR16(p,i,d) (writew((d), (u16 __iomem *)(p) + (i)/2))
#define NV_RD16(p,i) (readw((u16 __iomem *)(p) + (i)/2))
#define NV_WR32(p,i,d) (writel((d), (u32 __iomem *)(p) + (i)/4))
#define NV_RD32(p,i) (readl((u32 __iomem *)(p) + (i)/4))
+#endif
#define VGA_WR08(p,i,d) NV_WR08(p,i,d)
#define VGA_RD08(p,i) NV_RD08(p,i)
On Mon, 8 Nov 2004, Antonino A. Daplas wrote:
> On Monday 08 November 2004 13:57, Benjamin Herrenschmidt wrote:
> > You probably broke ppc versions here. The driver enables "big endian"
> > register access, but readw/writew/l functions do byteswap, which will
> > lead to incorrect results.
> >
> > The fix would be to probably just remove the code that puts the chip
> > registers into big endian mode, this isn't necessary nor a very good
> > idea actually.
> >
>
> How about this patch? This is almost the original macro in riva_hw.h,
> with the __force annotation.
>
> Signed-off-by: Antonino Daplas <[email protected]>
> ---
>
> diff -Nru a/drivers/video/riva/riva_hw.h b/drivers/video/riva/riva_hw.h
> --- a/drivers/video/riva/riva_hw.h 2004-11-06 12:42:19 +08:00
> +++ b/drivers/video/riva/riva_hw.h 2004-11-08 16:59:34 +08:00
> @@ -77,14 +77,18 @@
> #include <asm/io.h>
> #define NV_WR08(p,i,d) out_8(p+i, d)
> #define NV_RD08(p,i) in_8(p+i)
> +#define NV_WR16(p,i,d) (((volatile u16 __force *)(p))[(i)/2]=(d))
> +#define NV_RD16(p,i) (((volatile u16 __force *)(p))[(i)/2])
> +#define NV_WR32(p,i,d) (((volatile u32 __force *)(p))[(i)/4]=(d))
> +#define NV_RD32(p,i) (((volatile u32 __force *)(p))[(i)/4])
> #else
> #define NV_WR08(p,i,d) (writeb((d), (u8 __iomem *)(p) + (i)))
> #define NV_RD08(p,i) (readb((u8 __iomem *)(p) + (i)))
> -#endif
> #define NV_WR16(p,i,d) (writew((d), (u16 __iomem *)(p) + (i)/2))
> #define NV_RD16(p,i) (readw((u16 __iomem *)(p) + (i)/2))
> #define NV_WR32(p,i,d) (writel((d), (u32 __iomem *)(p) + (i)/4))
> #define NV_RD32(p,i) (readl((u32 __iomem *)(p) + (i)/4))
> +#endif
> #define VGA_WR08(p,i,d) NV_WR08(p,i,d)
> #define VGA_RD08(p,i) NV_RD08(p,i)
PPC also has nice {out,in}_be{16,32}() operations.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
On Mon, 8 Nov 2004, Antonino A. Daplas wrote:
>
> How about this patch? This is almost the original macro in riva_hw.h,
> with the __force annotation.
Why not just use __raw_readl/__raw_writel?
That's what they exist for, and they still do any IO accesses correctly,
which a direct store does not do (it would seriously break on older
alphas, for example).
Of course, clearly the thing has never worked on things like alphas
anyway, but the point is that accessing IO memory with direct loads and
stores really _is_ fundamentally wrong, even if it happens to work on many
architectures. The keyword is "happens".
Linus
On Monday 08 November 2004 23:21, Linus Torvalds wrote:
> On Mon, 8 Nov 2004, Antonino A. Daplas wrote:
> > How about this patch? This is almost the original macro in riva_hw.h,
> > with the __force annotation.
>
> Why not just use __raw_readl/__raw_writel?
>
Ok.
Tony
In big endian machines, the read*/write* accessors do a byteswap for an
inherently little endian PCI bus. However, rivafb puts the hardwire in big
endian register access, thus the byteswap is not needed. So, instead of
read*/write*, use __raw_read*/__raw_write*.
Signed-off-by: Antonino Daplas <[email protected]>
---
diff -Nru a/drivers/video/riva/riva_hw.h b/drivers/video/riva/riva_hw.h
--- a/drivers/video/riva/riva_hw.h 2004-11-09 03:45:55 +08:00
+++ b/drivers/video/riva/riva_hw.h 2004-11-09 03:50:32 +08:00
@@ -77,14 +77,18 @@
#include <asm/io.h>
#define NV_WR08(p,i,d) out_8(p+i, d)
#define NV_RD08(p,i) in_8(p+i)
+#define NV_WR16(p,i,d) (__raw_writew((d), (u16 __iomem *)(p) + (i)/2))
+#define NV_RD16(p,i) (__raw_readw((u16 __iomem *)(p) + (i)/2))
+#define NV_WR32(p,i,d) (__raw_writel((d), (u32 __iomem *)(p) + (i)/4))
+#define NV_RD32(p,i) (__raw_readl((u32 __iomem *)(p) + (i)/4))
#else
#define NV_WR08(p,i,d) (writeb((d), (u8 __iomem *)(p) + (i)))
#define NV_RD08(p,i) (readb((u8 __iomem *)(p) + (i)))
-#endif
#define NV_WR16(p,i,d) (writew((d), (u16 __iomem *)(p) + (i)/2))
#define NV_RD16(p,i) (readw((u16 __iomem *)(p) + (i)/2))
#define NV_WR32(p,i,d) (writel((d), (u32 __iomem *)(p) + (i)/4))
#define NV_RD32(p,i) (readl((u32 __iomem *)(p) + (i)/4))
+#endif
#define VGA_WR08(p,i,d) NV_WR08(p,i,d)
#define VGA_RD08(p,i) NV_RD08(p,i)
On Tue, 9 Nov 2004, Antonino A. Daplas wrote:
>
> In big endian machines, the read*/write* accessors do a byteswap for an
> inherently little endian PCI bus. However, rivafb puts the hardwire in big
> endian register access, thus the byteswap is not needed. So, instead of
> read*/write*, use __raw_read*/__raw_write*.
This fix should make the #ifdef CONFIG_PCC entirely superfluous afaik.
The thing is, once riva does its HW accesses right, the special cases just
go away. There's a reason we have abstractions..
Does anybody have the hardware to test with?
Linus
On Mon, 2004-11-08 at 16:33 +0800, Antonino A. Daplas wrote:
> Hmm, I'll ask Guido Guenther if he can test the changes. I think a different
> set of access macros for PPC might be a more preferrable solution.
Well, I'd rather leave the registers Little Endian, but then, it will
clash with X which does put them into Big Endian mode, so that would
have to be restored all the time.
Ben.
On Mon, 2004-11-08 at 17:06 +0800, Antonino A. Daplas wrote:
>
> How about this patch? This is almost the original macro in riva_hw.h,
> with the __force annotation.
I don't like it neither. It lacks barriers. the rivafb driver
notoriously lacks barriers, except in a few places where it was so bad
that it actually broke all the time, where we added some. This
originates from the X "nv" driver written by Mark Vojkovich who didn't
want to hear about barriers for perfs reasons I think.
Ben.
On Tuesday 09 November 2004 04:13, Linus Torvalds wrote:
> On Tue, 9 Nov 2004, Antonino A. Daplas wrote:
> > In big endian machines, the read*/write* accessors do a byteswap for an
> > inherently little endian PCI bus. However, rivafb puts the hardwire in
> > big endian register access, thus the byteswap is not needed. So, instead
> > of read*/write*, use __raw_read*/__raw_write*.
>
> This fix should make the #ifdef CONFIG_PCC entirely superfluous afaik.
Ah, of course.
>
> The thing is, once riva does its HW accesses right, the special cases just
> go away. There's a reason we have abstractions..
>
> Does anybody have the hardware to test with?
>
I've asked Guido Guenther to test, might take a couple of days. I also
asked him if he can use the in/out_be* which is probably safer for ppc.
And the mb()'s scattered all over the driver may be removed.
Tony
In big endian machines, the read*/write* accessors do a byteswap for an
inherently little endian PCI bus. However, rivafb puts the hardwire in big
endian register access, thus the byteswap is not needed. So for 16- and
32-bit access, instead of read*/write*, use __raw_read*/__raw_write* for all
archs.
Signed-off-by: Antonino Daplas <[email protected]>
---
diff -Nru a/drivers/video/riva/riva_hw.h b/drivers/video/riva/riva_hw.h
--- a/drivers/video/riva/riva_hw.h 2004-11-09 05:39:48 +08:00
+++ b/drivers/video/riva/riva_hw.h 2004-11-09 06:00:09 +08:00
@@ -73,18 +73,13 @@
/*
* HW access macros.
*/
-#if defined(__powerpc__)
#include <asm/io.h>
-#define NV_WR08(p,i,d) out_8(p+i, d)
-#define NV_RD08(p,i) in_8(p+i)
-#else
#define NV_WR08(p,i,d) (writeb((d), (u8 __iomem *)(p) + (i)))
#define NV_RD08(p,i) (readb((u8 __iomem *)(p) + (i)))
-#endif
-#define NV_WR16(p,i,d) (writew((d), (u16 __iomem *)(p) + (i)/2))
-#define NV_RD16(p,i) (readw((u16 __iomem *)(p) + (i)/2))
-#define NV_WR32(p,i,d) (writel((d), (u32 __iomem *)(p) + (i)/4))
-#define NV_RD32(p,i) (readl((u32 __iomem *)(p) + (i)/4))
+#define NV_WR16(p,i,d) (__raw_writew((d), (u16 __iomem *)(p) + (i)/2))
+#define NV_RD16(p,i) (__raw_readw((u16 __iomem *)(p) + (i)/2))
+#define NV_WR32(p,i,d) (__raw_writel((d), (u32 __iomem *)(p) + (i)/4))
+#define NV_RD32(p,i) (__raw_readl((u32 __iomem *)(p) + (i)/4))
#define VGA_WR08(p,i,d) NV_WR08(p,i,d)
#define VGA_RD08(p,i) NV_RD08(p,i)
On Tuesday 09 November 2004 05:50, Benjamin Herrenschmidt wrote:
> On Mon, 2004-11-08 at 16:33 +0800, Antonino A. Daplas wrote:
> > Hmm, I'll ask Guido Guenther if he can test the changes. I think a
> > different set of access macros for PPC might be a more preferrable
> > solution.
>
> Well, I'd rather leave the registers Little Endian, but then, it will
> clash with X which does put them into Big Endian mode, so that would
> have to be restored all the time.
>
I also asked Guido if he can test the removal of the code that puts the
hardware in big endian, although I told him that I prefer a new set of
access macros. But I'll leave this decision to you people.
Tony
On Tue, 9 Nov 2004, Antonino A. Daplas wrote:
>
> In big endian machines, the read*/write* accessors do a byteswap for an
> inherently little endian PCI bus. However, rivafb puts the hardwire in big
> endian register access, thus the byteswap is not needed. So for 16- and
> 32-bit access, instead of read*/write*, use __raw_read*/__raw_write* for all
> archs.
Ok, applied with some further simplifications (use "void __iomem *" and
suddenly those /2 and /4 just go away - also use __raw_xxxx for the
single-byte versions to be consistent).
Linus
On Mon, Nov 08, 2004 at 02:25:04PM -0800, Linus Torvalds wrote:
>
>
> On Tue, 9 Nov 2004, Antonino A. Daplas wrote:
> >
> > In big endian machines, the read*/write* accessors do a byteswap for an
> > inherently little endian PCI bus. However, rivafb puts the hardwire in big
> > endian register access, thus the byteswap is not needed. So for 16- and
> > 32-bit access, instead of read*/write*, use __raw_read*/__raw_write* for all
> > archs.
>
> Ok, applied with some further simplifications (use "void __iomem *" and
> suddenly those /2 and /4 just go away - also use __raw_xxxx for the
> single-byte versions to be consistent).
Doesn't work for me. This one works:
diff -u -u linux-2.6.10-rc1-mm5.orig/drivers/video/riva/riva_hw.h linux-2.6.10-rc1-mm5/drivers/video/riva/riva_hw.h
--- linux-2.6.10-rc1-mm5.orig/drivers/video/riva/riva_hw.h 2004-11-12 13:42:54.000000000 +0100
+++ linux-2.6.10-rc1-mm5/drivers/video/riva/riva_hw.h 2004-11-12 13:47:29.400807920 +0100
@@ -75,12 +75,12 @@
*/
#include <asm/io.h>
-#define NV_WR08(p,i,d) (__raw_writeb((d), (void __iomem *)(p) + (i)))
-#define NV_RD08(p,i) (__raw_readb((void __iomem *)(p) + (i)))
-#define NV_WR16(p,i,d) (__raw_writew((d), (void __iomem *)(p) + (i)))
-#define NV_RD16(p,i) (__raw_readw((void __iomem *)(p) + (i)))
-#define NV_WR32(p,i,d) (__raw_writel((d), (void __iomem *)(p) + (i)))
-#define NV_RD32(p,i) (__raw_readl((void __iomem *)(p) + (i)))
+#define NV_WR08(p,i,d) (writeb((d), (u8 __iomem *)(p) + (i)))
+#define NV_RD08(p,i) (readb((u8 __iomem *)(p) + (i)))
+#define NV_WR16(p,i,d) (__raw_writew((d), (u16 __iomem *)(p) + (i)/2))
+#define NV_RD16(p,i) (__raw_readw((u16 __iomem *)(p) + (i)/2))
+#define NV_WR32(p,i,d) (__raw_writel((d), (u32 __iomem *)(p) + (i)/4))
+#define NV_RD32(p,i) (__raw_readl((u32 __iomem *)(p) + (i)/4))
#define VGA_WR08(p,i,d) NV_WR08(p,i,d)
#define VGA_RD08(p,i) NV_RD08(p,i)
Interesting enough this one doesn't (only differenc in NV_{WR,RW}08:
diff -u -u linux-2.6.10-rc1-mm5.orig/drivers/video/riva/riva_hw.h linux-2.6.10-rc1-mm5/drivers/video/riva/riva_hw.h
--- linux-2.6.10-rc1-mm5.orig/drivers/video/riva/riva_hw.h 2004-11-12 13:42:54.000000000 +0100
+++ linux-2.6.10-rc1-mm5/drivers/video/riva/riva_hw.h 2004-11-12 13:47:29.400807920 +0100
@@ -75,12 +75,12 @@
*/
#include <asm/io.h>
-#define NV_WR08(p,i,d) (__raw_writeb((d), (void __iomem *)(p) + (i)))
-#define NV_RD08(p,i) (__raw_readb((void __iomem *)(p) + (i)))
-#define NV_WR16(p,i,d) (__raw_writew((d), (void __iomem *)(p) + (i)))
-#define NV_RD16(p,i) (__raw_readw((void __iomem *)(p) + (i)))
-#define NV_WR32(p,i,d) (__raw_writel((d), (void __iomem *)(p) + (i)))
-#define NV_RD32(p,i) (__raw_readl((void __iomem *)(p) + (i)))
+#define NV_WR08(p,i,d) (writeb((d), (u8 __iomem *)(p) + (i)))
+#define NV_RD08(p,i) (readb((u8 __iomem *)(p) + (i)))
+#define NV_WR16(p,i,d) (__raw_writew((d), (u16 __iomem *)(p) + (i)/2))
+#define NV_RD16(p,i) (__raw_readw((u16 __iomem *)(p) + (i)/2))
+#define NV_WR32(p,i,d) (__raw_writel((d), (u32 __iomem *)(p) + (i)/4))
+#define NV_RD32(p,i) (__raw_readl((u32 __iomem *)(p) + (i)/4))
#define VGA_WR08(p,i,d) NV_WR08(p,i,d)
#define VGA_RD08(p,i) NV_RD08(p,i)
Cheers,
-- Guido
On Fri, 12 Nov 2004, Guido Guenther wrote:
>
> Doesn't work for me. This one works:
>
> diff -u -u linux-2.6.10-rc1-mm5.orig/drivers/video/riva/riva_hw.h linux-2.6.10-rc1-mm5/drivers/video/riva/riva_hw.h
> --- linux-2.6.10-rc1-mm5.orig/drivers/video/riva/riva_hw.h 2004-11-12 13:42:54.000000000 +0100
> +++ linux-2.6.10-rc1-mm5/drivers/video/riva/riva_hw.h 2004-11-12 13:47:29.400807920 +0100
> @@ -75,12 +75,12 @@
> */
> #include <asm/io.h>
>
> -#define NV_WR08(p,i,d) (__raw_writeb((d), (void __iomem *)(p) + (i)))
> -#define NV_RD08(p,i) (__raw_readb((void __iomem *)(p) + (i)))
> -#define NV_WR16(p,i,d) (__raw_writew((d), (void __iomem *)(p) + (i)))
> -#define NV_RD16(p,i) (__raw_readw((void __iomem *)(p) + (i)))
> -#define NV_WR32(p,i,d) (__raw_writel((d), (void __iomem *)(p) + (i)))
> -#define NV_RD32(p,i) (__raw_readl((void __iomem *)(p) + (i)))
> +#define NV_WR08(p,i,d) (writeb((d), (u8 __iomem *)(p) + (i)))
> +#define NV_RD08(p,i) (readb((u8 __iomem *)(p) + (i)))
> +#define NV_WR16(p,i,d) (__raw_writew((d), (u16 __iomem *)(p) + (i)/2))
> +#define NV_RD16(p,i) (__raw_readw((u16 __iomem *)(p) + (i)/2))
> +#define NV_WR32(p,i,d) (__raw_writel((d), (u32 __iomem *)(p) + (i)/4))
> +#define NV_RD32(p,i) (__raw_readl((u32 __iomem *)(p) + (i)/4))
Can you please try without the broken "u16" and "(i/2)" things (and u32
and i/4)? They really should not be needed, unless something is
_seriously_ broken. The only thing they do is the automatically align the
reference - and quite frankly, it looks like anybody passing an unaligned
register offset is _quite_ buggy.
IOW, it would seem that the real difference is the "__raw_writeb" vs
"writeb". Can you test just that change?
> #define VGA_WR08(p,i,d) NV_WR08(p,i,d)
> #define VGA_RD08(p,i) NV_RD08(p,i)
>
> Interesting enough this one doesn't (only differenc in NV_{WR,RW}08:
>
> diff -u -u linux-2.6.10-rc1-mm5.orig/drivers/video/riva/riva_hw.h linux-2.6.10-rc1-mm5/drivers/video/riva/riva_hw.h
> --- linux-2.6.10-rc1-mm5.orig/drivers/video/riva/riva_hw.h 2004-11-12 13:42:54.000000000 +0100
> +++ linux-2.6.10-rc1-mm5/drivers/video/riva/riva_hw.h 2004-11-12 13:47:29.400807920 +0100
> @@ -75,12 +75,12 @@
> */
> #include <asm/io.h>
>
> -#define NV_WR08(p,i,d) (__raw_writeb((d), (void __iomem *)(p) + (i)))
> -#define NV_RD08(p,i) (__raw_readb((void __iomem *)(p) + (i)))
> -#define NV_WR16(p,i,d) (__raw_writew((d), (void __iomem *)(p) + (i)))
> -#define NV_RD16(p,i) (__raw_readw((void __iomem *)(p) + (i)))
> -#define NV_WR32(p,i,d) (__raw_writel((d), (void __iomem *)(p) + (i)))
> -#define NV_RD32(p,i) (__raw_readl((void __iomem *)(p) + (i)))
> +#define NV_WR08(p,i,d) (writeb((d), (u8 __iomem *)(p) + (i)))
> +#define NV_RD08(p,i) (readb((u8 __iomem *)(p) + (i)))
> +#define NV_WR16(p,i,d) (__raw_writew((d), (u16 __iomem *)(p) + (i)/2))
> +#define NV_RD16(p,i) (__raw_readw((u16 __iomem *)(p) + (i)/2))
> +#define NV_WR32(p,i,d) (__raw_writel((d), (u32 __iomem *)(p) + (i)/4))
> +#define NV_RD32(p,i) (__raw_readl((u32 __iomem *)(p) + (i)/4))
I don't see the difference to your other patch. Did you put in the wrong
patch?
Linus
On Fri, Nov 12, 2004 at 08:01:00AM -0800, Linus Torvalds wrote:
> On Fri, 12 Nov 2004, Guido Guenther wrote:
> >
> > Doesn't work for me. This one works:
> >
> > diff -u -u linux-2.6.10-rc1-mm5.orig/drivers/video/riva/riva_hw.h linux-2.6.10-rc1-mm5/drivers/video/riva/riva_hw.h
> > --- linux-2.6.10-rc1-mm5.orig/drivers/video/riva/riva_hw.h 2004-11-12 13:42:54.000000000 +0100
> > +++ linux-2.6.10-rc1-mm5/drivers/video/riva/riva_hw.h 2004-11-12 13:47:29.400807920 +0100
> > @@ -75,12 +75,12 @@
> > */
> > #include <asm/io.h>
> >
> > -#define NV_WR08(p,i,d) (__raw_writeb((d), (void __iomem *)(p) + (i)))
> > -#define NV_RD08(p,i) (__raw_readb((void __iomem *)(p) + (i)))
> > -#define NV_WR16(p,i,d) (__raw_writew((d), (void __iomem *)(p) + (i)))
> > -#define NV_RD16(p,i) (__raw_readw((void __iomem *)(p) + (i)))
> > -#define NV_WR32(p,i,d) (__raw_writel((d), (void __iomem *)(p) + (i)))
> > -#define NV_RD32(p,i) (__raw_readl((void __iomem *)(p) + (i)))
> > +#define NV_WR08(p,i,d) (writeb((d), (u8 __iomem *)(p) + (i)))
> > +#define NV_RD08(p,i) (readb((u8 __iomem *)(p) + (i)))
> > +#define NV_WR16(p,i,d) (__raw_writew((d), (u16 __iomem *)(p) + (i)/2))
> > +#define NV_RD16(p,i) (__raw_readw((u16 __iomem *)(p) + (i)/2))
> > +#define NV_WR32(p,i,d) (__raw_writel((d), (u32 __iomem *)(p) + (i)/4))
> > +#define NV_RD32(p,i) (__raw_readl((u32 __iomem *)(p) + (i)/4))
>
> Can you please try without the broken "u16" and "(i/2)" things (and u32
> and i/4)? They really should not be needed, unless something is
> _seriously_ broken. The only thing they do is the automatically align the
> reference - and quite frankly, it looks like anybody passing an unaligned
> register offset is _quite_ buggy.
O.k., it was the __raw_{write,read}b which broke things, not the
"alignment". This one works:
diff -u -u linux-2.6.10-rc1-mm5.orig/drivers/video/riva/riva_hw.h linux-2.6.10-rc1-mm5/drivers/video/riva/riva_hw.h
--- linux-2.6.10-rc1-mm5.orig/drivers/video/riva/riva_hw.h 2004-11-12 13:42:54.000000000 +0100
+++ linux-2.6.10-rc1-mm5/drivers/video/riva/riva_hw.h 2004-11-12 17:39:22.000000000 +0100
@@ -75,8 +75,8 @@
*/
#include <asm/io.h>
-#define NV_WR08(p,i,d) (__raw_writeb((d), (void __iomem *)(p) + (i)))
-#define NV_RD08(p,i) (__raw_readb((void __iomem *)(p) + (i)))
+#define NV_WR08(p,i,d) (writeb((d), (void __iomem *)(p) + (i)))
+#define NV_RD08(p,i) (readb((void __iomem *)(p) + (i)))
#define NV_WR16(p,i,d) (__raw_writew((d), (void __iomem *)(p) + (i)))
#define NV_RD16(p,i) (__raw_readw((void __iomem *)(p) + (i)))
#define NV_WR32(p,i,d) (__raw_writel((d), (void __iomem *)(p) + (i)))
Signed-off-by: Guido Guenther <[email protected]>
> IOW, it would seem that the real difference is the "__raw_writeb" vs
> "writeb". Can you test just that change?
>
> > #define VGA_WR08(p,i,d) NV_WR08(p,i,d)
> > #define VGA_RD08(p,i) NV_RD08(p,i)
> >
> > Interesting enough this one doesn't (only differenc in NV_{WR,RW}08:
> >
>
> I don't see the difference to your other patch. Did you put in the wrong
> patch?
There aren't any, I actually attached the wrong patch. The non-working
version has __raw_{read,write}b8 instead of {read,write}b8. In 2.6.9
riva_hw.h used {in,out}_8 for NV_{RD,WR}08 so using the "non-raw"
writeb/readb now looks correct since these map to {in,out}_8 now.
Cheers,
-- Guido
On Fri, 12 Nov 2004, Guido Guenther wrote:
>
> O.k., it was the __raw_{write,read}b which broke things, not the
> "alignment". This one works:
All right, that's as expected. However, it does seem to point out that the
riva driver depends on _different_ memory ordering guarantees for the
8-bit accesses as opposed to the other ones. Whee. Can you say "UGGLEE"?
Anyway, patch applied. Thanks,
Linus
On Fri, 2004-11-12 at 20:18 +0100, Guido Guenther wrote:
> O.k., it was the __raw_{write,read}b which broke things, not the
> "alignment". This one works:
>
> diff -u -u linux-2.6.10-rc1-mm5.orig/drivers/video/riva/riva_hw.h linux-2.6.10-rc1-mm5/drivers/video/riva/riva_hw.h
> --- linux-2.6.10-rc1-mm5.orig/drivers/video/riva/riva_hw.h 2004-11-12 13:42:54.000000000 +0100
> +++ linux-2.6.10-rc1-mm5/drivers/video/riva/riva_hw.h 2004-11-12 17:39:22.000000000 +0100
> @@ -75,8 +75,8 @@
> */
> #include <asm/io.h>
>
> -#define NV_WR08(p,i,d) (__raw_writeb((d), (void __iomem *)(p) + (i)))
> -#define NV_RD08(p,i) (__raw_readb((void __iomem *)(p) + (i)))
> +#define NV_WR08(p,i,d) (writeb((d), (void __iomem *)(p) + (i)))
> +#define NV_RD08(p,i) (readb((void __iomem *)(p) + (i)))
Interesting. The only difference here should be barriers. I hate the
lack of barriers in that driver ... I'm not sure the driver may not have
other bugs related to the lack of them in the 16 and 32 bits accessors.
It does use non-barrier version on purpose in some accel ops though,
when filling the fifo with pixels, but that's pretty much the only case
where it makes sense.
> There aren't any, I actually attached the wrong patch. The non-working
> version has __raw_{read,write}b8 instead of {read,write}b8. In 2.6.9
> riva_hw.h used {in,out}_8 for NV_{RD,WR}08 so using the "non-raw"
> writeb/readb now looks correct since these map to {in,out}_8 now.
{in,out}_8 are ppc-specific things that are identical to readb/writeb
indeed, with barriers.
Ben.
On Sat, Nov 13, 2004 at 12:39:32PM +1100, Benjamin Herrenschmidt wrote:
> On Fri, 2004-11-12 at 20:18 +0100, Guido Guenther wrote:
>
> > O.k., it was the __raw_{write,read}b which broke things, not the
> > "alignment". This one works:
> >
> > diff -u -u linux-2.6.10-rc1-mm5.orig/drivers/video/riva/riva_hw.h linux-2.6.10-rc1-mm5/drivers/video/riva/riva_hw.h
> > --- linux-2.6.10-rc1-mm5.orig/drivers/video/riva/riva_hw.h 2004-11-12 13:42:54.000000000 +0100
> > +++ linux-2.6.10-rc1-mm5/drivers/video/riva/riva_hw.h 2004-11-12 17:39:22.000000000 +0100
> > @@ -75,8 +75,8 @@
> > */
> > #include <asm/io.h>
> >
> > -#define NV_WR08(p,i,d) (__raw_writeb((d), (void __iomem *)(p) + (i)))
> > -#define NV_RD08(p,i) (__raw_readb((void __iomem *)(p) + (i)))
> > +#define NV_WR08(p,i,d) (writeb((d), (void __iomem *)(p) + (i)))
> > +#define NV_RD08(p,i) (readb((void __iomem *)(p) + (i)))
>
> Interesting. The only difference here should be barriers. I hate the
> lack of barriers in that driver ... I'm not sure the driver may not have
Yes, it's a real pain. Missing barriers in RIVA_FIFO_FREE caused lots of
lockups, until I found them in your 2.4 tree.
> other bugs related to the lack of them in the 16 and 32 bits accessors.
> It does use non-barrier version on purpose in some accel ops though,
> when filling the fifo with pixels, but that's pretty much the only case
> where it makes sense.
>
> > There aren't any, I actually attached the wrong patch. The non-working
> > version has __raw_{read,write}b8 instead of {read,write}b8. In 2.6.9
> > riva_hw.h used {in,out}_8 for NV_{RD,WR}08 so using the "non-raw"
> > writeb/readb now looks correct since these map to {in,out}_8 now.
>
> {in,out}_8 are ppc-specific things that are identical to readb/writeb
> indeed, with barriers.
In 2.6.10-rc1-mm5 {in,out}_8 and read/writeb are exactly identical, only
__raw_{read,write}b is different. So you mean __raw_{read,write}b in the
above? (no nitpicking, just want to be sure I understand this
correctly).
Cheers,
-- Guido
On Saturday 13 November 2004 19:22, Guido Guenther wrote:
> On Sat, Nov 13, 2004 at 12:39:32PM +1100, Benjamin Herrenschmidt wrote:
> > On Fri, 2004-11-12 at 20:18 +0100, Guido Guenther wrote:
> In 2.6.10-rc1-mm5 {in,out}_8 and read/writeb are exactly identical, only
> __raw_{read,write}b is different. So you mean __raw_{read,write}b in the
> above? (no nitpicking, just want to be sure I understand this
> correctly).
Why not use in_be* and out_be* for __raw_read and raw_write? If I
understand correctly, they also have barriers. Or would that hurt
performance?
Tony
Hi Linus,
At Mon, 8 Nov 2004 07:21:53 -0800 (PST),
Linus Torvalds wrote:
>
> On Mon, 8 Nov 2004, Antonino A. Daplas wrote:
> >
> > How about this patch? This is almost the original macro in riva_hw.h,
> > with the __force annotation.
>
> Why not just use __raw_readl/__raw_writel?
>
> That's what they exist for, and they still do any IO accesses correctly,
> which a direct store does not do (it would seriously break on older
> alphas, for example).
sorry for a dumb question but should readl/writel on big endian system
swap like ppc does?
hmm... checking source code, it seems like most arch indeed does that,
except m68knommu, h8300 and microblaze. is it nommu thing?
--
yashi
On Fri, Nov 12, 2004 at 11:32:07AM -0800, Linus Torvalds wrote:
>
>
> On Fri, 12 Nov 2004, Guido Guenther wrote:
> >
> > O.k., it was the __raw_{write,read}b which broke things, not the
> > "alignment". This one works:
>
> All right, that's as expected. However, it does seem to point out that the
> riva driver depends on _different_ memory ordering guarantees for the
> 8-bit accesses as opposed to the other ones. Whee. Can you say "UGGLEE"?
Aglie. This scared me too, so I had another look. It seems P{V,C}IO
areas are only accessed using VGA_{RD,WR}8 macros. NV_{RW,WR}08 are
never actually used directly. So this patch makes at least usage
consistent. VGA_{RD,WR}8 to access "I/O areas" in an ordered way. NV_*
for the rest. Please apply.
Cheers,
-- Guido
--- linux-2.6.10-rc1-mm5/drivers/video/riva/riva_hw.orig.2 2004-11-13 12:24:48.000000000 +0100
+++ linux-2.6.10-rc1-mm5/drivers/video/riva/riva_hw.h 2004-11-13 12:24:56.000000000 +0100
@@ -75,15 +75,15 @@
*/
#include <asm/io.h>
-#define NV_WR08(p,i,d) (writeb((d), (void __iomem *)(p) + (i)))
-#define NV_RD08(p,i) (readb((void __iomem *)(p) + (i)))
+#define NV_WR08(p,i,d) (__raw_writeb((d), (void __iomem *)(p) + (i)))
+#define NV_RD08(p,i) (__raw_readb((void __iomem *)(p) + (i)))
#define NV_WR16(p,i,d) (__raw_writew((d), (void __iomem *)(p) + (i)))
#define NV_RD16(p,i) (__raw_readw((void __iomem *)(p) + (i)))
#define NV_WR32(p,i,d) (__raw_writel((d), (void __iomem *)(p) + (i)))
#define NV_RD32(p,i) (__raw_readl((void __iomem *)(p) + (i)))
-#define VGA_WR08(p,i,d) NV_WR08(p,i,d)
-#define VGA_RD08(p,i) NV_RD08(p,i)
+#define VGA_WR08(p,i,d) (writeb((d), (void __iomem *)(p) + (i)))
+#define VGA_RD08(p,i) (readb((void __iomem *)(p) + (i)))
/*
* Define different architectures.
Signed-Off-By: Guido Guenther <[email protected]>
On Sat, Nov 13, 2004 at 08:00:30PM +0800, Antonino A. Daplas wrote:
> On Saturday 13 November 2004 19:22, Guido Guenther wrote:
> > On Sat, Nov 13, 2004 at 12:39:32PM +1100, Benjamin Herrenschmidt wrote:
> > > On Fri, 2004-11-12 at 20:18 +0100, Guido Guenther wrote:
> > In 2.6.10-rc1-mm5 {in,out}_8 and read/writeb are exactly identical, only
> > __raw_{read,write}b is different. So you mean __raw_{read,write}b in the
> > above? (no nitpicking, just want to be sure I understand this
> > correctly).
>
> Why not use in_be* and out_be* for __raw_read and raw_write? If I
> understand correctly, they also have barriers. Or would that hurt
> performance?
I think it would. XFree86 comes along without these barriers nicely (and
this this driver was written with documentation ;), so rivafb should be
o.k. too.
-- Guido
On Sat, 13 Nov 2004, Antonino A. Daplas wrote:
>
> Why not use in_be* and out_be* for __raw_read and raw_write?
Please don't start using some stupid magic ppc-specific macros for a
driver that has no reason to be PPC-specific. It then only causes bugs
that show on one platform and not another.
Linus
On Sunday 14 November 2004 02:00, Linus Torvalds wrote:
> On Sat, 13 Nov 2004, Antonino A. Daplas wrote:
> > Why not use in_be* and out_be* for __raw_read and raw_write?
>
> Please don't start using some stupid magic ppc-specific macros for a
> driver that has no reason to be PPC-specific. It then only causes bugs
> that show on one platform and not another.
I'm not. I'm just wondering that if the other approach was taken (keep the
hardware in little endian mode), then the write/read* macros, which are just
wrappers for in_le*/out_le*, would have been used. Would it help fix (or
cover up) bugs that are in PPC but not x86?
Tony
On Sun, Nov 14, 2004 at 05:29:46AM +0800, Antonino A. Daplas wrote:
> On Sunday 14 November 2004 02:00, Linus Torvalds wrote:
> > On Sat, 13 Nov 2004, Antonino A. Daplas wrote:
> > > Why not use in_be* and out_be* for __raw_read and raw_write?
> >
> > Please don't start using some stupid magic ppc-specific macros for a
> > driver that has no reason to be PPC-specific. It then only causes bugs
> > that show on one platform and not another.
>
> I'm not. I'm just wondering that if the other approach was taken (keep the
> hardware in little endian mode), then the write/read* macros, which are just
> wrappers for in_le*/out_le*, would have been used. Would it help fix (or
> cover up) bugs that are in PPC but not x86?
XFree86 switches the card to big endian mode anyway. Running rivafb in
little endian might cause us great deals of pain for little gain.
Cheers,
-- Guido
On Sat, 2004-11-13 at 12:22 +0100, Guido Guenther wrote:
> > {in,out}_8 are ppc-specific things that are identical to readb/writeb
> > indeed, with barriers.
> In 2.6.10-rc1-mm5 {in,out}_8 and read/writeb are exactly identical, only
> __raw_{read,write}b is different. So you mean __raw_{read,write}b in the
> above? (no nitpicking, just want to be sure I understand this
> correctly).
I just meant they are identical and they have both barriers, sorry.
Ben.
On Sun, 2004-11-14 at 05:29 +0800, Antonino A. Daplas wrote:
> On Sunday 14 November 2004 02:00, Linus Torvalds wrote:
> > On Sat, 13 Nov 2004, Antonino A. Daplas wrote:
> > > Why not use in_be* and out_be* for __raw_read and raw_write?
> >
> > Please don't start using some stupid magic ppc-specific macros for a
> > driver that has no reason to be PPC-specific. It then only causes bugs
> > that show on one platform and not another.
>
> I'm not. I'm just wondering that if the other approach was taken (keep the
> hardware in little endian mode), then the write/read* macros, which are just
> wrappers for in_le*/out_le*, would have been used. Would it help fix (or
> cover up) bugs that are in PPC but not x86?
If you switch the HW to LE, I'm afraid you'll lockup when VT switching
back & forth with X ...
Ben.
At Sat, 13 Nov 2004 21:20:03 +0900,
yashi wrote:
[...]
> > Why not just use __raw_readl/__raw_writel?
> >
> > That's what they exist for, and they still do any IO accesses correctly,
> > which a direct store does not do (it would seriously break on older
> > alphas, for example).
>
> sorry for a dumb question but should readl/writel on big endian system
> swap like ppc does?
I guess everyone is busy hacking. but can at least someone give me a
hit?
I'm worring about this issue because I'm about to use two deferent
linux arch on same board. it's based on reconfigurable device so I
can configure to have deferent cpu on it.
if you are using two deferent arch of linux, it's natual to think you
want to share all device drivers. but if one arch swap with readl and
the other doesn't, I have to abstruct these low level access
methods. (given that those to arch are same endian and connected with
same bus and to the same devices)
is there any rule we should follow? Is the ppc way the right
direction to follow? I can ifdef anytime for my own use but I just
want to know what _should_ be done.
thanks,
--
yashi
Yasushi SHOJI wrote:
> At Sat, 13 Nov 2004 21:20:03 +0900,
> yashi wrote:
> [...]
>
>>>Why not just use __raw_readl/__raw_writel?
>>>
>>>That's what they exist for, and they still do any IO accesses correctly,
>>>which a direct store does not do (it would seriously break on older
>>>alphas, for example).
>>
>>sorry for a dumb question but should readl/writel on big endian system
>>swap like ppc does?
>
>
> I guess everyone is busy hacking. but can at least someone give me a
> hit?
>
> I'm worring about this issue because I'm about to use two deferent
> linux arch on same board. it's based on reconfigurable device so I
> can configure to have deferent cpu on it.
>
> if you are using two deferent arch of linux, it's natual to think you
> want to share all device drivers. but if one arch swap with readl and
> the other doesn't, I have to abstruct these low level access
> methods. (given that those to arch are same endian and connected with
> same bus and to the same devices)
>
> is there any rule we should follow? Is the ppc way the right
> direction to follow? I can ifdef anytime for my own use but I just
> want to know what _should_ be done.
readl()/writel() are defined as being for the PCI bus (little endian).
As such, they should swap on big endian platforms.
Jeff
Hi Jeff,
At Thu, 18 Nov 2004 23:09:19 -0500,
Jeff Garzik wrote:
>
> > is there any rule we should follow? Is the ppc way the right
> > direction to follow? I can ifdef anytime for my own use but I just
> > want to know what _should_ be done.
>
>
> readl()/writel() are defined as being for the PCI bus (little endian).
> As such, they should swap on big endian platforms.
thanks for your input. so read*() and write*() family should _only_
be used for pci bus?
the "Understanding the linux kernel" state that those macro should be
used for i/o shared memory. and it has three bus listed: isa, vlb,
pci.
I'm a bit confused... i know some arch have bus-dedicated read and
write macro, ie isa_readl(). is this the way to go?
sorry for dumb questions. if this is RTFM thing, just tell me.
thanks,
--
yashi