* Re: [PATCH] fbdev: Fix IO access in rivafb [not found] <200411080521.iA85LbG6025914@hera.kernel.org> @ 2004-11-08 5:57 ` Benjamin Herrenschmidt 2004-11-08 8:33 ` [Linux-fbdev-devel] " Antonino A. Daplas 2004-11-08 9:06 ` Antonino A. Daplas 0 siblings, 2 replies; 32+ messages in thread From: Benjamin Herrenschmidt @ 2004-11-08 5:57 UTC (permalink / raw) To: Linux Kernel list; +Cc: Antonino A. Daplas, Linux Fbdev development list > 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Linux-fbdev-devel] Re: [PATCH] fbdev: Fix IO access in rivafb 2004-11-08 5:57 ` [PATCH] fbdev: Fix IO access in rivafb Benjamin Herrenschmidt @ 2004-11-08 8:33 ` Antonino A. Daplas 2004-11-08 21:50 ` Benjamin Herrenschmidt 2004-11-08 9:06 ` Antonino A. Daplas 1 sibling, 1 reply; 32+ messages in thread From: Antonino A. Daplas @ 2004-11-08 8:33 UTC (permalink / raw) To: linux-fbdev-devel, Benjamin Herrenschmidt, Linux Kernel list Cc: Linux Fbdev development list 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Linux-fbdev-devel] Re: [PATCH] fbdev: Fix IO access in rivafb 2004-11-08 8:33 ` [Linux-fbdev-devel] " Antonino A. Daplas @ 2004-11-08 21:50 ` Benjamin Herrenschmidt 2004-11-08 22:18 ` Antonino A. Daplas 0 siblings, 1 reply; 32+ messages in thread From: Benjamin Herrenschmidt @ 2004-11-08 21:50 UTC (permalink / raw) To: adaplas; +Cc: Linux Fbdev development list, Linux Kernel list 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Linux-fbdev-devel] Re: [PATCH] fbdev: Fix IO access in rivafb 2004-11-08 21:50 ` Benjamin Herrenschmidt @ 2004-11-08 22:18 ` Antonino A. Daplas 0 siblings, 0 replies; 32+ messages in thread From: Antonino A. Daplas @ 2004-11-08 22:18 UTC (permalink / raw) To: Benjamin Herrenschmidt, adaplas Cc: Linux Fbdev development list, Linux Kernel list, Guido Guenther 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Linux-fbdev-devel] Re: [PATCH] fbdev: Fix IO access in rivafb 2004-11-08 5:57 ` [PATCH] fbdev: Fix IO access in rivafb Benjamin Herrenschmidt 2004-11-08 8:33 ` [Linux-fbdev-devel] " Antonino A. Daplas @ 2004-11-08 9:06 ` Antonino A. Daplas 2004-11-08 9:55 ` Geert Uytterhoeven ` (2 more replies) 1 sibling, 3 replies; 32+ messages in thread From: Antonino A. Daplas @ 2004-11-08 9:06 UTC (permalink / raw) To: linux-fbdev-devel, Benjamin Herrenschmidt, Linux Kernel list Cc: Linux Fbdev development list, Andrew Morton, Linus Torvalds 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 <adaplas@pol.net> --- 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) ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Linux-fbdev-devel] Re: [PATCH] fbdev: Fix IO access in rivafb 2004-11-08 9:06 ` Antonino A. Daplas @ 2004-11-08 9:55 ` Geert Uytterhoeven 2004-11-08 15:21 ` Linus Torvalds 2004-11-08 21:52 ` [Linux-fbdev-devel] Re: [PATCH] fbdev: Fix IO access in rivafb Benjamin Herrenschmidt 2 siblings, 0 replies; 32+ messages in thread From: Geert Uytterhoeven @ 2004-11-08 9:55 UTC (permalink / raw) To: Linux Fbdev development list Cc: Benjamin Herrenschmidt, Linux Kernel list, Andrew Morton, Linus Torvalds 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 <adaplas@pol.net> > --- > > 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 -- 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Linux-fbdev-devel] Re: [PATCH] fbdev: Fix IO access in rivafb 2004-11-08 9:06 ` Antonino A. Daplas 2004-11-08 9:55 ` Geert Uytterhoeven @ 2004-11-08 15:21 ` Linus Torvalds 2004-11-08 20:02 ` Antonino A. Daplas 2004-11-13 12:20 ` Yasushi SHOJI 2004-11-08 21:52 ` [Linux-fbdev-devel] Re: [PATCH] fbdev: Fix IO access in rivafb Benjamin Herrenschmidt 2 siblings, 2 replies; 32+ messages in thread From: Linus Torvalds @ 2004-11-08 15:21 UTC (permalink / raw) To: adaplas Cc: Linux Fbdev development list, Benjamin Herrenschmidt, Linux Kernel list, Andrew Morton 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Linux-fbdev-devel] Re: [PATCH] fbdev: Fix IO access in rivafb 2004-11-08 15:21 ` Linus Torvalds @ 2004-11-08 20:02 ` Antonino A. Daplas 2004-11-08 20:13 ` Linus Torvalds 2004-11-13 12:20 ` Yasushi SHOJI 1 sibling, 1 reply; 32+ messages in thread From: Antonino A. Daplas @ 2004-11-08 20:02 UTC (permalink / raw) To: linux-fbdev-devel, Linus Torvalds Cc: Linux Fbdev development list, Benjamin Herrenschmidt, Linux Kernel list, Andrew Morton 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 <adaplas@pol.net> --- 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) ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Linux-fbdev-devel] Re: [PATCH] fbdev: Fix IO access in rivafb 2004-11-08 20:02 ` Antonino A. Daplas @ 2004-11-08 20:13 ` Linus Torvalds 2004-11-08 22:08 ` Antonino A. Daplas 0 siblings, 1 reply; 32+ messages in thread From: Linus Torvalds @ 2004-11-08 20:13 UTC (permalink / raw) To: adaplas Cc: Linux Fbdev development list, Benjamin Herrenschmidt, Linux Kernel list, Andrew Morton 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Linux-fbdev-devel] Re: [PATCH] fbdev: Fix IO access in rivafb 2004-11-08 20:13 ` Linus Torvalds @ 2004-11-08 22:08 ` Antonino A. Daplas 2004-11-08 22:25 ` Linus Torvalds 0 siblings, 1 reply; 32+ messages in thread From: Antonino A. Daplas @ 2004-11-08 22:08 UTC (permalink / raw) To: linux-fbdev-devel, Linus Torvalds Cc: Linux Fbdev development list, Benjamin Herrenschmidt, Linux Kernel list, Andrew Morton, Guido Guenther 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 <adaplas@pol.net> --- 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) ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Linux-fbdev-devel] Re: [PATCH] fbdev: Fix IO access in rivafb 2004-11-08 22:08 ` Antonino A. Daplas @ 2004-11-08 22:25 ` Linus Torvalds 2004-11-12 12:51 ` Guido Guenther 0 siblings, 1 reply; 32+ messages in thread From: Linus Torvalds @ 2004-11-08 22:25 UTC (permalink / raw) To: adaplas Cc: Linux Fbdev development list, Benjamin Herrenschmidt, Linux Kernel list, Andrew Morton, Guido Guenther 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Linux-fbdev-devel] Re: [PATCH] fbdev: Fix IO access in rivafb 2004-11-08 22:25 ` Linus Torvalds @ 2004-11-12 12:51 ` Guido Guenther 2004-11-12 16:01 ` Linus Torvalds 0 siblings, 1 reply; 32+ messages in thread From: Guido Guenther @ 2004-11-12 12:51 UTC (permalink / raw) To: Linus Torvalds Cc: adaplas, Linux Fbdev development list, Benjamin Herrenschmidt, Linux Kernel list, Andrew Morton 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Linux-fbdev-devel] Re: [PATCH] fbdev: Fix IO access in rivafb 2004-11-12 12:51 ` Guido Guenther @ 2004-11-12 16:01 ` Linus Torvalds 2004-11-12 19:18 ` Guido Guenther 0 siblings, 1 reply; 32+ messages in thread From: Linus Torvalds @ 2004-11-12 16:01 UTC (permalink / raw) To: Guido Guenther Cc: adaplas, Linux Fbdev development list, Benjamin Herrenschmidt, Linux Kernel list, Andrew Morton 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Linux-fbdev-devel] Re: [PATCH] fbdev: Fix IO access in rivafb 2004-11-12 16:01 ` Linus Torvalds @ 2004-11-12 19:18 ` Guido Guenther 2004-11-12 19:32 ` Linus Torvalds 2004-11-13 1:39 ` Benjamin Herrenschmidt 0 siblings, 2 replies; 32+ messages in thread From: Guido Guenther @ 2004-11-12 19:18 UTC (permalink / raw) To: Linus Torvalds Cc: adaplas, Linux Fbdev development list, Benjamin Herrenschmidt, Linux Kernel list, Andrew Morton 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 <agx@sigxcpu.org> > 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Linux-fbdev-devel] Re: [PATCH] fbdev: Fix IO access in rivafb 2004-11-12 19:18 ` Guido Guenther @ 2004-11-12 19:32 ` Linus Torvalds 2004-11-13 12:57 ` Guido Guenther 2004-11-13 1:39 ` Benjamin Herrenschmidt 1 sibling, 1 reply; 32+ messages in thread From: Linus Torvalds @ 2004-11-12 19:32 UTC (permalink / raw) To: Guido Guenther Cc: adaplas, Linux Fbdev development list, Benjamin Herrenschmidt, Linux Kernel list, Andrew Morton 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Linux-fbdev-devel] Re: [PATCH] fbdev: Fix IO access in rivafb 2004-11-12 19:32 ` Linus Torvalds @ 2004-11-13 12:57 ` Guido Guenther 0 siblings, 0 replies; 32+ messages in thread From: Guido Guenther @ 2004-11-13 12:57 UTC (permalink / raw) To: Linus Torvalds Cc: adaplas, Linux Fbdev development list, Benjamin Herrenschmidt, Linux Kernel list, Andrew Morton 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 <agx@sigxcpu.org> ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Linux-fbdev-devel] Re: [PATCH] fbdev: Fix IO access in rivafb 2004-11-12 19:18 ` Guido Guenther 2004-11-12 19:32 ` Linus Torvalds @ 2004-11-13 1:39 ` Benjamin Herrenschmidt 2004-11-13 11:22 ` Guido Guenther 1 sibling, 1 reply; 32+ messages in thread From: Benjamin Herrenschmidt @ 2004-11-13 1:39 UTC (permalink / raw) To: Guido Guenther Cc: Linus Torvalds, adaplas, Linux Fbdev development list, Linux Kernel list, Andrew Morton 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Linux-fbdev-devel] Re: [PATCH] fbdev: Fix IO access in rivafb 2004-11-13 1:39 ` Benjamin Herrenschmidt @ 2004-11-13 11:22 ` Guido Guenther 2004-11-13 12:00 ` Antonino A. Daplas 2004-11-13 23:49 ` Benjamin Herrenschmidt 0 siblings, 2 replies; 32+ messages in thread From: Guido Guenther @ 2004-11-13 11:22 UTC (permalink / raw) To: Benjamin Herrenschmidt Cc: Linus Torvalds, adaplas, Linux Fbdev development list, Linux Kernel list, Andrew Morton 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Linux-fbdev-devel] Re: [PATCH] fbdev: Fix IO access in rivafb 2004-11-13 11:22 ` Guido Guenther @ 2004-11-13 12:00 ` Antonino A. Daplas 2004-11-13 13:02 ` Guido Guenther 2004-11-13 18:00 ` Linus Torvalds 2004-11-13 23:49 ` Benjamin Herrenschmidt 1 sibling, 2 replies; 32+ messages in thread From: Antonino A. Daplas @ 2004-11-13 12:00 UTC (permalink / raw) To: linux-fbdev-devel, Guido Guenther, Benjamin Herrenschmidt Cc: Linus Torvalds, adaplas, Linux Fbdev development list, Linux Kernel list, Andrew Morton 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Linux-fbdev-devel] Re: [PATCH] fbdev: Fix IO access in rivafb 2004-11-13 12:00 ` Antonino A. Daplas @ 2004-11-13 13:02 ` Guido Guenther 2004-11-13 18:00 ` Linus Torvalds 1 sibling, 0 replies; 32+ messages in thread From: Guido Guenther @ 2004-11-13 13:02 UTC (permalink / raw) To: adaplas Cc: linux-fbdev-devel, Benjamin Herrenschmidt, Linus Torvalds, Linux Kernel list, Andrew Morton 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Linux-fbdev-devel] Re: [PATCH] fbdev: Fix IO access in rivafb 2004-11-13 12:00 ` Antonino A. Daplas 2004-11-13 13:02 ` Guido Guenther @ 2004-11-13 18:00 ` Linus Torvalds 2004-11-13 21:29 ` Antonino A. Daplas 1 sibling, 1 reply; 32+ messages in thread From: Linus Torvalds @ 2004-11-13 18:00 UTC (permalink / raw) To: adaplas Cc: Linux Fbdev development list, Guido Guenther, Benjamin Herrenschmidt, Linux Kernel list, Andrew Morton 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Linux-fbdev-devel] Re: [PATCH] fbdev: Fix IO access in rivafb 2004-11-13 18:00 ` Linus Torvalds @ 2004-11-13 21:29 ` Antonino A. Daplas 2004-11-13 22:54 ` Guido Guenther 2004-11-13 23:52 ` Benjamin Herrenschmidt 0 siblings, 2 replies; 32+ messages in thread From: Antonino A. Daplas @ 2004-11-13 21:29 UTC (permalink / raw) To: linux-fbdev-devel, Linus Torvalds, adaplas Cc: Linux Fbdev development list, Guido Guenther, Benjamin Herrenschmidt, Linux Kernel list, Andrew Morton 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Linux-fbdev-devel] Re: [PATCH] fbdev: Fix IO access in rivafb 2004-11-13 21:29 ` Antonino A. Daplas @ 2004-11-13 22:54 ` Guido Guenther 2004-11-13 23:52 ` Benjamin Herrenschmidt 1 sibling, 0 replies; 32+ messages in thread From: Guido Guenther @ 2004-11-13 22:54 UTC (permalink / raw) To: adaplas Cc: linux-fbdev-devel, Linus Torvalds, Benjamin Herrenschmidt, Linux Kernel list, Andrew Morton 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Linux-fbdev-devel] Re: [PATCH] fbdev: Fix IO access in rivafb 2004-11-13 21:29 ` Antonino A. Daplas 2004-11-13 22:54 ` Guido Guenther @ 2004-11-13 23:52 ` Benjamin Herrenschmidt 1 sibling, 0 replies; 32+ messages in thread From: Benjamin Herrenschmidt @ 2004-11-13 23:52 UTC (permalink / raw) To: adaplas Cc: Linux Fbdev development list, Linus Torvalds, Guido Guenther, Linux Kernel list, Andrew Morton 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Linux-fbdev-devel] Re: [PATCH] fbdev: Fix IO access in rivafb 2004-11-13 11:22 ` Guido Guenther 2004-11-13 12:00 ` Antonino A. Daplas @ 2004-11-13 23:49 ` Benjamin Herrenschmidt 1 sibling, 0 replies; 32+ messages in thread From: Benjamin Herrenschmidt @ 2004-11-13 23:49 UTC (permalink / raw) To: Guido Guenther Cc: Linus Torvalds, adaplas, Linux Fbdev development list, Linux Kernel list, Andrew Morton 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Linux-fbdev-devel] Re: [PATCH] fbdev: Fix IO access in rivafb 2004-11-08 15:21 ` Linus Torvalds 2004-11-08 20:02 ` Antonino A. Daplas @ 2004-11-13 12:20 ` Yasushi SHOJI 2004-11-19 1:48 ` readl/writel: swap or not to swap (was: [PATCH] fbdev: Fix IO access in rivafb) Yasushi SHOJI 1 sibling, 1 reply; 32+ messages in thread From: Yasushi SHOJI @ 2004-11-13 12:20 UTC (permalink / raw) To: Linus Torvalds Cc: adaplas, Linux Fbdev development list, Benjamin Herrenschmidt, Linux Kernel list, Andrew Morton, uClinux development list 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* readl/writel: swap or not to swap (was: [PATCH] fbdev: Fix IO access in rivafb) 2004-11-13 12:20 ` Yasushi SHOJI @ 2004-11-19 1:48 ` Yasushi SHOJI 2004-11-19 4:09 ` readl/writel: swap or not to swap Jeff Garzik 0 siblings, 1 reply; 32+ messages in thread From: Yasushi SHOJI @ 2004-11-19 1:48 UTC (permalink / raw) To: Linux Kernel list 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: readl/writel: swap or not to swap 2004-11-19 1:48 ` readl/writel: swap or not to swap (was: [PATCH] fbdev: Fix IO access in rivafb) Yasushi SHOJI @ 2004-11-19 4:09 ` Jeff Garzik 2004-11-19 7:24 ` Yasushi SHOJI 0 siblings, 1 reply; 32+ messages in thread From: Jeff Garzik @ 2004-11-19 4:09 UTC (permalink / raw) To: Yasushi SHOJI; +Cc: Linux Kernel list 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: readl/writel: swap or not to swap 2004-11-19 4:09 ` readl/writel: swap or not to swap Jeff Garzik @ 2004-11-19 7:24 ` Yasushi SHOJI 0 siblings, 0 replies; 32+ messages in thread From: Yasushi SHOJI @ 2004-11-19 7:24 UTC (permalink / raw) To: Jeff Garzik; +Cc: Linux Kernel list 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Linux-fbdev-devel] Re: [PATCH] fbdev: Fix IO access in rivafb 2004-11-08 9:06 ` Antonino A. Daplas 2004-11-08 9:55 ` Geert Uytterhoeven 2004-11-08 15:21 ` Linus Torvalds @ 2004-11-08 21:52 ` Benjamin Herrenschmidt 2004-11-08 23:07 ` Giuseppe Bilotta 2 siblings, 1 reply; 32+ messages in thread From: Benjamin Herrenschmidt @ 2004-11-08 21:52 UTC (permalink / raw) To: adaplas Cc: Linux Fbdev development list, Linux Kernel list, Andrew Morton, Linus Torvalds 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Linux-fbdev-devel] Re: [PATCH] fbdev: Fix IO access in rivafb 2004-11-08 21:52 ` [Linux-fbdev-devel] Re: [PATCH] fbdev: Fix IO access in rivafb Benjamin Herrenschmidt @ 2004-11-08 23:07 ` Giuseppe Bilotta 2004-11-13 23:22 ` Guido Guenther 0 siblings, 1 reply; 32+ messages in thread From: Giuseppe Bilotta @ 2004-11-08 23:07 UTC (permalink / raw) To: linux-kernel; +Cc: linux-fbdev-devel Benjamin Herrenschmidt wrote: > 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. Could this be the reason why in 2.6.7 I get solid lockups when switching to nv-driven X and rivafb-driven console? Is there something I could test to see if this is the reason? -- Giuseppe "Oblomov" Bilotta Can't you see It all makes perfect sense Expressed in dollar and cents Pounds shillings and pence (Roger Waters) ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Linux-fbdev-devel] Re: [PATCH] fbdev: Fix IO access in rivafb 2004-11-08 23:07 ` Giuseppe Bilotta @ 2004-11-13 23:22 ` Guido Guenther 0 siblings, 0 replies; 32+ messages in thread From: Guido Guenther @ 2004-11-13 23:22 UTC (permalink / raw) To: Giuseppe Bilotta; +Cc: linux-kernel, linux-fbdev-devel On Tue, Nov 09, 2004 at 12:07:58AM +0100, Giuseppe Bilotta wrote: > Benjamin Herrenschmidt wrote: > > 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. > > Could this be the reason why in 2.6.7 I get solid lockups when > switching to nv-driven X and rivafb-driven console? Is there > something I could test to see if this is the reason? Please try 2.6.10-rc1-mm5. It should work there. -- Guido ^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2004-11-19 7:25 UTC | newest] Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <200411080521.iA85LbG6025914@hera.kernel.org> 2004-11-08 5:57 ` [PATCH] fbdev: Fix IO access in rivafb Benjamin Herrenschmidt 2004-11-08 8:33 ` [Linux-fbdev-devel] " Antonino A. Daplas 2004-11-08 21:50 ` Benjamin Herrenschmidt 2004-11-08 22:18 ` Antonino A. Daplas 2004-11-08 9:06 ` Antonino A. Daplas 2004-11-08 9:55 ` Geert Uytterhoeven 2004-11-08 15:21 ` Linus Torvalds 2004-11-08 20:02 ` Antonino A. Daplas 2004-11-08 20:13 ` Linus Torvalds 2004-11-08 22:08 ` Antonino A. Daplas 2004-11-08 22:25 ` Linus Torvalds 2004-11-12 12:51 ` Guido Guenther 2004-11-12 16:01 ` Linus Torvalds 2004-11-12 19:18 ` Guido Guenther 2004-11-12 19:32 ` Linus Torvalds 2004-11-13 12:57 ` Guido Guenther 2004-11-13 1:39 ` Benjamin Herrenschmidt 2004-11-13 11:22 ` Guido Guenther 2004-11-13 12:00 ` Antonino A. Daplas 2004-11-13 13:02 ` Guido Guenther 2004-11-13 18:00 ` Linus Torvalds 2004-11-13 21:29 ` Antonino A. Daplas 2004-11-13 22:54 ` Guido Guenther 2004-11-13 23:52 ` Benjamin Herrenschmidt 2004-11-13 23:49 ` Benjamin Herrenschmidt 2004-11-13 12:20 ` Yasushi SHOJI 2004-11-19 1:48 ` readl/writel: swap or not to swap (was: [PATCH] fbdev: Fix IO access in rivafb) Yasushi SHOJI 2004-11-19 4:09 ` readl/writel: swap or not to swap Jeff Garzik 2004-11-19 7:24 ` Yasushi SHOJI 2004-11-08 21:52 ` [Linux-fbdev-devel] Re: [PATCH] fbdev: Fix IO access in rivafb Benjamin Herrenschmidt 2004-11-08 23:07 ` Giuseppe Bilotta 2004-11-13 23:22 ` Guido Guenther
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).