RE: [PATCH 1/1] edac x38: new MC driver module

RE: [PATCH 1/1] edac x38: new MC driver module

[H. Peter Anvin]

The cast is unnecessary; one can argue for a cast to (u32) for clarity, but personally I think writel() is a clear enough especialy given the context.

-- 
Sent from my mobile phone (pardon any lack of formatting)


-----Original Message-----
From: Hitoshi Mitake <h.mitake@gmail.com>
Sent: Friday, November 28, 2008 16:11
To: Hitoshi Mitake <h.mitake@gmail.com>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>; H. Peter Anvin <hpa@zytor.com>; Luck, Tony <tony.luck@intel.com>; Russell King <rmk+lkml@arm.linux.org.uk>; Ralf Baechle <ralf@linux-mips.org>; Andrew Morton <akpm@linux-foundation.org>; Doug Thompson <norsk5@yahoo.com>; dougthompson@xmission.com <dougthompson@xmission.com>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; linux-arch@vger.kernel.org <linux-arch@vger.kernel.org>
Subject: Re: [PATCH 1/1] edac x38: new MC driver module

On Wed, 26 Nov 2008 01:10:59 +0900
Hitoshi Mitake <h.mitake@gmail.com> wrote:

> On Tue, 25 Nov 2008 16:46:18 +0100 (CET)
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> 
> > On Wed, 26 Nov 2008, Hitoshi Mitake wrote:
> > > On Mon, 24 Nov 2008 21:13:58 -0800
> > > "H. Peter Anvin" <hpa@zytor.com> wrote:
> > > > Are you planning to add writeq() as well?
> > > 
> > > Yes, I want to add writeq().
> > > But there's a problem that
> > > I don't have a plan to use writeq() now, so I can't test writeq() soon.
> > > 
> > > How is this? I think it isn't bad. I want to hear your opinion.
> > > 
> > > static inline void writeq(__u64 val, volatile void __iomem *addr)
> > > {
> > > 	writel((unsigned int)val, addr);
> > > 	writel((unsigned int)(val >> 32), addr+1);
> >                                                ^
> > 					       4
> > 
> > > }
> > 
> 
> Thanks, I missed about void pointer...
> 

I rewrote the patch. I think newest version is good enough.
Could you please review this?

description of this patch
Adding implementation of readq/writeq to x86_32,
and adding config value to x86 architecture to determine existence of readq/writeq

Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com>
---
 arch/x86/Kconfig          |    4 ++++
 arch/x86/include/asm/io.h |   24 ++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index ac22bb7..10bd84c 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -11,9 +11,13 @@ config 64BIT
 
 config X86_32
 	def_bool !64BIT
+	select ARCH_HAS_READQ
+	select ARCH_HAS_WRITEQ
 
 config X86_64
 	def_bool 64BIT
+	select ARCH_HAS_READQ
+	select ARCH_HAS_WRITEQ
 
 ### Arch settings
 config X86
diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index ac2abc8..ac15d0e 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -4,6 +4,7 @@
 #define ARCH_HAS_IOREMAP_WC
 
 #include <linux/compiler.h>
+#include <asm-generic/int-ll64.h>
 
 #define build_mmio_read(name, size, type, reg, barrier) \
 static inline type name(const volatile void __iomem *addr) \
@@ -57,6 +58,29 @@ build_mmio_write(__writeq, "q", unsigned long, "r", )
 /* Let people know we have them */
 #define readq readq
 #define writeq writeq
+
+#else  /* CONFIG_X86_32 from here */
+
+static inline __u64 readq(const volatile void __iomem *addr)
+{
+	const volatile u32 __iomem *p = addr;
+	u32 l, h;
+
+	l = readl(p);
+	h = readl(p+1);
+
+	return l + ((u64)h << 32);
+}
+
+static inline void writeq(__u64 val, volatile void __iomem *addr)
+{
+	writel((unsigned int)val, addr);
+	writel((unsigned int)(val >> 32), addr+4);
+}
+
+#define readq readq
+#define writeq writeq
+
 #endif
 
 extern int iommu_bio_merge;
-- 
1.5.6.5

Re: [PATCH 1/1] edac x38: new MC driver module

[Hitoshi Mitake]

On Fri, 28 Nov 2008 16:56:39 -0800
"H. Peter Anvin" <hpa@zytor.com> wrote:

> The cast is unnecessary; one can argue for a cast to (u32) for clarity, but personally I think writel() is a clear enough especialy given the context.
> 

Thanks for your advice. I removed redundant cat. How is this?

description of this patch
Adding implementation of readq/writeq to x86_32,
and adding config value to x86 architecture to determine existence of readq/writeq

Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com>
---
 arch/x86/Kconfig          |    4 ++++
 arch/x86/include/asm/io.h |   24 ++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index ac22bb7..10bd84c 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -11,9 +11,13 @@ config 64BIT
 
 config X86_32
 	def_bool !64BIT
+	select ARCH_HAS_READQ
+	select ARCH_HAS_WRITEQ
 
 config X86_64
 	def_bool 64BIT
+	select ARCH_HAS_READQ
+	select ARCH_HAS_WRITEQ
 
 ### Arch settings
 config X86
diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index ac2abc8..ddc67aa 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -4,6 +4,7 @@
 #define ARCH_HAS_IOREMAP_WC
 
 #include <linux/compiler.h>
+#include <asm-generic/int-ll64.h>
 
 #define build_mmio_read(name, size, type, reg, barrier) \
 static inline type name(const volatile void __iomem *addr) \
@@ -57,6 +58,29 @@ build_mmio_write(__writeq, "q", unsigned long, "r", )
 /* Let people know we have them */
 #define readq readq
 #define writeq writeq
+
+#else  /* CONFIG_X86_32 from here */
+
+static inline __u64 readq(const volatile void __iomem *addr)
+{
+	const volatile u32 __iomem *p = addr;
+	u32 l, h;
+
+	l = readl(p);
+	h = readl(p+1);
+
+	return l + ((u64)h << 32);
+}
+
+static inline void writeq(__u64 val, volatile void __iomem *addr)
+{
+	writel(val, addr);
+	writel(val >> 32, addr+4);
+}
+
+#define readq readq
+#define writeq writeq
+
 #endif
 
 extern int iommu_bio_merge;
-- 
1.5.6.5

Re: [PATCH 1/1] edac x38: new MC driver module

[Ingo Molnar]

* Hitoshi Mitake <h.mitake@gmail.com> wrote:

> +#define readq readq
> +#define writeq writeq

hm, that's done to override the generic definition? Looks weird and i 
think that's rather fragile - it's easy to somehow get the generic header 
without this override.

	Ingo

Re: [PATCH 1/1] edac x38: new MC driver module

[Hitoshi Mitake]

On Sat, 29 Nov 2008 10:38:58 +0100
Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Hitoshi Mitake <h.mitake@gmail.com> wrote:
> 
> > +#define readq readq
> > +#define writeq writeq
> 
> hm, that's done to override the generic definition? Looks weird and i 
> think that's rather fragile - it's easy to somehow get the generic header 
> without this override.

No, the purpose of this #define is to let user of this function to know there's readq/writeq.
Like this,

#ifdef readq
/* do something */
#endif

But this is old way. ARCH_HAS_READQ and ARCH_HAS_WRITEQ are new ways
to determine existence of readq/writeq. Drivers which use readq/writeq should
depend on these values in their Kconfig file.

This definitions may be redundant. But there are some architectures
which already have this definition for same purpose. So I added.

Should I remove these?

Re: [PATCH 1/1] edac x38: new MC driver module

[Sam Ravnborg]

> 
> But this is old way. ARCH_HAS_READQ and ARCH_HAS_WRITEQ are new ways
> to determine existence of readq/writeq. Drivers which use readq/writeq should
> depend on these values in their Kconfig file.

If we look at arch/x86/Kconfig we see:
### Arch settings
config X86
        def_bool y
        select HAVE_AOUT if X86_32
        select HAVE_UNSTABLE_SCHED_CLOCK
        select HAVE_IDE
        select HAVE_OPROFILE
        select HAVE_IOREMAP_PROT
        select HAVE_KPROBES
        select ARCH_WANT_OPTIONAL_GPIOLIB
	...

So the normal syntax here is "HAVE_XXX_XXX" - not ARCH_HAS_XXX_XXX

If you update your patch please use this syntax,
and locate the select under X86 - not under the 32/64 entries.

But I do not see why adding these in the first place.

See following advice from Linus:
http://marc.info/?l=linux-arch&m=121710129310710&w=2

===> Quote:
I really think that whoever started that 'HAVE_ARCH_x'/'ARCH_HAS_x' mess 
with totally random symbols that have NOTHING WHAT-SO-EVER to do with the 
actual symbols in question (so they do _not_ show up in grep'ing for some 
use) should be shot. 

We should never _ever_ use that model. And we use it way too much.

We should generally strive for the simpler and much more obvious

	/* Generic definition */
	#ifndef symbol
	int symbol(..)
	...
	#endif

and then architecture code can do

	#define symbol(x) ...

or if they want to do a function, and you _really_ don't like the '__weak' 
part (or you want to make it an inline function and don't want the clash 
with the real declaration), then you can just do

	static inline int symbol(x)
	{
		...
	}
	#define symbol symbol

and again it all works fine WITHOUT having to introduce some idiotic new 
and unrelated element called ARCH_HAS_SYMBOL.

<====

	Sam

Re: [PATCH 1/1] edac x38: new MC driver module

[Hitoshi Mitake]

On Sat, 29 Nov 2008 11:52:29 +0100
Sam Ravnborg <sam@ravnborg.org> wrote:

> > 
> > But this is old way. ARCH_HAS_READQ and ARCH_HAS_WRITEQ are new ways
> > to determine existence of readq/writeq. Drivers which use readq/writeq should
> > depend on these values in their Kconfig file.
> 
> If we look at arch/x86/Kconfig we see:
> ### Arch settings
> config X86
>         def_bool y
>         select HAVE_AOUT if X86_32
>         select HAVE_UNSTABLE_SCHED_CLOCK
>         select HAVE_IDE
>         select HAVE_OPROFILE
>         select HAVE_IOREMAP_PROT
>         select HAVE_KPROBES
>         select ARCH_WANT_OPTIONAL_GPIOLIB
> 	...
> 
> So the normal syntax here is "HAVE_XXX_XXX" - not ARCH_HAS_XXX_XXX
> 
> If you update your patch please use this syntax,
> and locate the select under X86 - not under the 32/64 entries.

Thanks for your notification. I didn't notice that syntax rule.

> 
> But I do not see why adding these in the first place.
> 

Andrew Morton told that drivers which need readq/writeq should use ones of kernel,
and if architecture part of kernel does not provide readq/writeq, drivers should be disabled.

This is Andrew's mail:
http://marc.info/?l=linux-kernel&m=122625885124798&w=2
===> Quote:

#ifdef readq

Is a suitable way of determining whether the architecture implements
readq and writeq.  It isn't pretty, but it will suffice.

A problem with it is that drivers will then do

#ifndef readq
<provide a local implementation here>
#endif

which rather sucks - we don't want lots of little private readq/writeq
implementations all over the tree.

Perhaps it would be better to have a CONFIG_ARCH_HAS_READQ and to then
disable these drivers on the architectures which don't provide
readq/writeq support.

<====

This is new patch.

description of this patch
Adding implementation of readq/writeq to x86_32,
and adding config value to x86 architecture to determine existence of readq/writeq

Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com>
---
 arch/x86/Kconfig          |    2 ++
 arch/x86/include/asm/io.h |   24 ++++++++++++++++++++++++
 2 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index ac22bb7..75408fe 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -34,6 +34,8 @@ config X86
 	select HAVE_ARCH_TRACEHOOK
 	select HAVE_GENERIC_DMA_COHERENT if X86_32
 	select HAVE_EFFICIENT_UNALIGNED_ACCESS
+	select HAVE_READQ
+	select HAVE_WRITEQ
 
 config ARCH_DEFCONFIG
 	string
diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index ac2abc8..ddc67aa 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -4,6 +4,7 @@
 #define ARCH_HAS_IOREMAP_WC
 
 #include <linux/compiler.h>
+#include <asm-generic/int-ll64.h>
 
 #define build_mmio_read(name, size, type, reg, barrier) \
 static inline type name(const volatile void __iomem *addr) \
@@ -57,6 +58,29 @@ build_mmio_write(__writeq, "q", unsigned long, "r", )
 /* Let people know we have them */
 #define readq readq
 #define writeq writeq
+
+#else  /* CONFIG_X86_32 from here */
+
+static inline __u64 readq(const volatile void __iomem *addr)
+{
+	const volatile u32 __iomem *p = addr;
+	u32 l, h;
+
+	l = readl(p);
+	h = readl(p+1);
+
+	return l + ((u64)h << 32);
+}
+
+static inline void writeq(__u64 val, volatile void __iomem *addr)
+{
+	writel(val, addr);
+	writel(val >> 32, addr+4);
+}
+
+#define readq readq
+#define writeq writeq
+
 #endif
 
 extern int iommu_bio_merge;
-- 
1.5.6.5

Re: [PATCH 1/1] edac x38: new MC driver module

[Sam Ravnborg]

On Sat, Nov 29, 2008 at 10:24:49PM +0900, Hitoshi Mitake wrote:
> On Sat, 29 Nov 2008 11:52:29 +0100
> Sam Ravnborg <sam@ravnborg.org> wrote:
> 
> > > 
> > > But this is old way. ARCH_HAS_READQ and ARCH_HAS_WRITEQ are new ways
> > > to determine existence of readq/writeq. Drivers which use readq/writeq should
> > > depend on these values in their Kconfig file.
> > 
> > If we look at arch/x86/Kconfig we see:
> > ### Arch settings
> > config X86
> >         def_bool y
> >         select HAVE_AOUT if X86_32
> >         select HAVE_UNSTABLE_SCHED_CLOCK
> >         select HAVE_IDE
> >         select HAVE_OPROFILE
> >         select HAVE_IOREMAP_PROT
> >         select HAVE_KPROBES
> >         select ARCH_WANT_OPTIONAL_GPIOLIB
> > 	...
> > 
> > So the normal syntax here is "HAVE_XXX_XXX" - not ARCH_HAS_XXX_XXX
> > 
> > If you update your patch please use this syntax,
> > and locate the select under X86 - not under the 32/64 entries.
> 
> Thanks for your notification. I didn't notice that syntax rule.
> 
> > 
> > But I do not see why adding these in the first place.
> > 
> 
> Andrew Morton told that drivers which need readq/writeq should use ones of kernel,
> and if architecture part of kernel does not provide readq/writeq, drivers should be disabled.
> 
> This is Andrew's mail:
> http://marc.info/?l=linux-kernel&m=122625885124798&w=2
> ===> Quote:
> 
> #ifdef readq
> 
> Is a suitable way of determining whether the architecture implements
> readq and writeq.  It isn't pretty, but it will suffice.
> 
> A problem with it is that drivers will then do
> 
> #ifndef readq
> <provide a local implementation here>
> #endif
> 
> which rather sucks - we don't want lots of little private readq/writeq
> implementations all over the tree.
> 
> Perhaps it would be better to have a CONFIG_ARCH_HAS_READQ and to then
> disable these drivers on the architectures which don't provide
> readq/writeq support.
> 
> <====
I see both rationales and you combine them in your patch - OK.

And the reason why you cannot just add this to
include/linux/io.h is that not all architectures
provide a readl()/writel() I assume.

Feel free to add my Acked-by: to the patch.

	Sam

Re: [PATCH 1/1] edac x38: new MC driver module

[Hitoshi Mitake]

On Sat, 29 Nov 2008 19:01:44 +0100
Sam Ravnborg <sam@ravnborg.org> wrote:

> I see both rationales and you combine them in your patch - OK.
> 
> And the reason why you cannot just add this to
> include/linux/io.h is that not all architectures
> provide a readl()/writel() I assume.
>
Yes, I can't say all architectures provide readl/writel.
And there may be some architecture depended problems,
so I can't decide to add my readq/writeq as architecture independent ones.

> Feel free to add my Acked-by: to the patch.

Thanks, I added your Acked-by to new patch.


description of this patch
Adding implementation of readq/writeq to x86_32,
and adding config value to x86 architecture to determine existence of readq/writeq

Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com>
Acked-by: Sam Ravnborg <sam@ravnborg.org>
---
 arch/x86/Kconfig          |    2 ++
 arch/x86/include/asm/io.h |   24 ++++++++++++++++++++++++
 2 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index ac22bb7..75408fe 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -34,6 +34,8 @@ config X86
 	select HAVE_ARCH_TRACEHOOK
 	select HAVE_GENERIC_DMA_COHERENT if X86_32
 	select HAVE_EFFICIENT_UNALIGNED_ACCESS
+	select HAVE_READQ
+	select HAVE_WRITEQ
 
 config ARCH_DEFCONFIG
 	string
diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index ac2abc8..ddc67aa 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -4,6 +4,7 @@
 #define ARCH_HAS_IOREMAP_WC
 
 #include <linux/compiler.h>
+#include <asm-generic/int-ll64.h>
 
 #define build_mmio_read(name, size, type, reg, barrier) \
 static inline type name(const volatile void __iomem *addr) \
@@ -57,6 +58,29 @@ build_mmio_write(__writeq, "q", unsigned long, "r", )
 /* Let people know we have them */
 #define readq readq
 #define writeq writeq
+
+#else  /* CONFIG_X86_32 from here */
+
+static inline __u64 readq(const volatile void __iomem *addr)
+{
+	const volatile u32 __iomem *p = addr;
+	u32 l, h;
+
+	l = readl(p);
+	h = readl(p+1);
+
+	return l + ((u64)h << 32);
+}
+
+static inline void writeq(__u64 val, volatile void __iomem *addr)
+{
+	writel(val, addr);
+	writel(val >> 32, addr+4);
+}
+
+#define readq readq
+#define writeq writeq
+
 #endif
 
 extern int iommu_bio_merge;
-- 
1.5.6.5

Re: [PATCH 1/1] edac x38: new MC driver module

[Ingo Molnar]

* Hitoshi Mitake <h.mitake@gmail.com> wrote:

> On Sat, 29 Nov 2008 19:01:44 +0100
> Sam Ravnborg <sam@ravnborg.org> wrote:
> 
> > I see both rationales and you combine them in your patch - OK.
> > 
> > And the reason why you cannot just add this to
> > include/linux/io.h is that not all architectures
> > provide a readl()/writel() I assume.
> >
> Yes, I can't say all architectures provide readl/writel.
> And there may be some architecture depended problems,
> so I can't decide to add my readq/writeq as architecture independent ones.
> 
> > Feel free to add my Acked-by: to the patch.
> 
> Thanks, I added your Acked-by to new patch.

applied to tip/x86/io, thanks! I also did the small cleanup below.

	Ingo

------------>
From 458102461ccedc3ab7847132b5db6a53782dc9a8 Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@elte.hu>
Date: Sun, 30 Nov 2008 09:33:55 +0100
Subject: [PATCH] x86: provide readq()/writeq() on 32-bit too, cleanup

Impact: cleanup

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/include/asm/io.h |   17 +++++++----------
 1 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index 2594644..3ccfaf6 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -55,21 +55,17 @@ build_mmio_write(__writeq, "q", unsigned long, "r", )
 #define __raw_readq __readq
 #define __raw_writeq writeq
 
-/* Let people know we have them */
-#define readq readq
-#define writeq writeq
-
 #else  /* CONFIG_X86_32 from here */
 
 static inline __u64 readq(const volatile void __iomem *addr)
 {
 	const volatile u32 __iomem *p = addr;
-	u32 l, h;
+	u32 low, high;
 
-	l = readl(p);
-	h = readl(p + 1);
+	low = readl(p);
+	high = readl(p + 1);
 
-	return l + ((u64)h << 32);
+	return low + ((u64)high << 32);
 }
 
 static inline void writeq(__u64 val, volatile void __iomem *addr)
@@ -78,11 +74,12 @@ static inline void writeq(__u64 val, volatile void __iomem *addr)
 	writel(val >> 32, addr+4);
 }
 
+#endif
+
+/* Let people know that we have them */
 #define readq		readq
 #define writeq		writeq
 
-#endif
-
 extern int iommu_bio_merge;
 
 #ifdef CONFIG_X86_32

Re: [PATCH 1/1] edac x38: new MC driver module

[Ingo Molnar]

the 32-bit build broke promptly - readq/writeq is a family of APIs that 
has to be either fully provided or not provided at all. The fix is below.

	Ingo

--------------->
From 16ebd68883e3583a66733d8b12ba55c8985af3f6 Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@elte.hu>
Date: Sun, 30 Nov 2008 10:20:20 +0100
Subject: [PATCH] x86: provide readq()/writeq() on 32-bit too, complete

if HAVE_READQ/HAVE_WRITEQ are defined, the full range of readq/writeq
APIs has to be provided to drivers:

 drivers/infiniband/hw/amso1100/c2.c: In function 'c2_tx_ring_alloc':
 drivers/infiniband/hw/amso1100/c2.c:133: error: implicit declaration of function '__raw_writeq'

So provide them on 32-bit as well. Also, map all the APIs to the
strongest ordering variant. It's way too easy to mess such details
up in drivers and the difference between "memory" and "" constrained
asm() constructs is in the noise range.

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/include/asm/io.h |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index 3ccfaf6..9036156 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -46,16 +46,11 @@ build_mmio_write(__writel, "l", unsigned int, "r", )
 #define mmiowb() barrier()
 
 #ifdef CONFIG_X86_64
+
 build_mmio_read(readq, "q", unsigned long, "=r", :"memory")
-build_mmio_read(__readq, "q", unsigned long, "=r", )
 build_mmio_write(writeq, "q", unsigned long, "r", :"memory")
-build_mmio_write(__writeq, "q", unsigned long, "r", )
-
-#define readq_relaxed(a) __readq(a)
-#define __raw_readq __readq
-#define __raw_writeq writeq
 
-#else  /* CONFIG_X86_32 from here */
+#else
 
 static inline __u64 readq(const volatile void __iomem *addr)
 {
@@ -76,9 +71,14 @@ static inline void writeq(__u64 val, volatile void __iomem *addr)
 
 #endif
 
+#define readq_relaxed(a)	readq(a)
+
+#define __raw_readq(a)		readq(a)
+#define __raw_writeq(a)		writeq(a)
+
 /* Let people know that we have them */
-#define readq		readq
-#define writeq		writeq
+#define readq			readq
+#define writeq			writeq
 
 extern int iommu_bio_merge;
 

Re: [PATCH 1/1] edac x38: new MC driver module

[Ingo Molnar]

the 32-bit build broke promptly - readq/writeq is a family of APIs that 
has to be either fully provided or not provided at all. The fix is below.

	Ingo

--------------->
From 16ebd68883e3583a66733d8b12ba55c8985af3f6 Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@elte.hu>
Date: Sun, 30 Nov 2008 10:20:20 +0100
Subject: [PATCH] x86: provide readq()/writeq() on 32-bit too, complete

if HAVE_READQ/HAVE_WRITEQ are defined, the full range of readq/writeq
APIs has to be provided to drivers:

 drivers/infiniband/hw/amso1100/c2.c: In function 'c2_tx_ring_alloc':
 drivers/infiniband/hw/amso1100/c2.c:133: error: implicit declaration of function '__raw_writeq'

So provide them on 32-bit as well. Also, map all the APIs to the
strongest ordering variant. It's way too easy to mess such details
up in drivers and the difference between "memory" and "" constrained
asm() constructs is in the noise range.

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/include/asm/io.h |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index 3ccfaf6..9036156 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -46,16 +46,11 @@ build_mmio_write(__writel, "l", unsigned int, "r", )
 #define mmiowb() barrier()
 
 #ifdef CONFIG_X86_64
+
 build_mmio_read(readq, "q", unsigned long, "=r", :"memory")
-build_mmio_read(__readq, "q", unsigned long, "=r", )
 build_mmio_write(writeq, "q", unsigned long, "r", :"memory")
-build_mmio_write(__writeq, "q", unsigned long, "r", )
-
-#define readq_relaxed(a) __readq(a)
-#define __raw_readq __readq
-#define __raw_writeq writeq
 
-#else  /* CONFIG_X86_32 from here */
+#else
 
 static inline __u64 readq(const volatile void __iomem *addr)
 {
@@ -76,9 +71,14 @@ static inline void writeq(__u64 val, volatile void __iomem *addr)
 
 #endif
 
+#define readq_relaxed(a)	readq(a)
+
+#define __raw_readq(a)		readq(a)
+#define __raw_writeq(a)		writeq(a)
+
 /* Let people know that we have them */
-#define readq		readq
-#define writeq		writeq
+#define readq			readq
+#define writeq			writeq
 
 extern int iommu_bio_merge;
 

Re: [PATCH 1/1] edac x38: new MC driver module

[Hitoshi Mitake]

On Sun, 30 Nov 2008 10:24:07 +0100
Ingo Molnar <mingo@elte.hu> wrote:

> 
> the 32-bit build broke promptly - readq/writeq is a family of APIs that 
> has to be either fully provided or not provided at all. The fix is below.

Thanks for your fix and adding!
When will this patch be added to mainline?
I want to rewrite x38_edac.c to adapt new APIs.

Re: [PATCH 1/1] edac x38: new MC driver module

[Ingo Molnar]

* Hitoshi Mitake <h.mitake@gmail.com> wrote:

> On Sun, 30 Nov 2008 10:24:07 +0100
> Ingo Molnar <mingo@elte.hu> wrote:
> 
> > 
> > the 32-bit build broke promptly - readq/writeq is a family of APIs that 
> > has to be either fully provided or not provided at all. The fix is below.
> 
> Thanks for your fix and adding!
> When will this patch be added to mainline?
> I want to rewrite x38_edac.c to adapt new APIs.

v2.6.29 at the earliest - if there are no regressions. A number of 
drivers use these APIs and usage is a bit messy - so bugs could be 
triggered, etc.

	Ingo

Re: [PATCH 1/1] edac x38: new MC driver module

[Hitoshi Mitake]

On Mon, Dec 1, 2008 at 01:15, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Hitoshi Mitake <h.mitake@gmail.com> wrote:
>
>> On Sun, 30 Nov 2008 10:24:07 +0100
>> Ingo Molnar <mingo@elte.hu> wrote:
>>
>> >
>> > the 32-bit build broke promptly - readq/writeq is a family of APIs that
>> > has to be either fully provided or not provided at all. The fix is below.
>>
>> Thanks for your fix and adding!
>> When will this patch be added to mainline?
>> I want to rewrite x38_edac.c to adapt new APIs.
>
> v2.6.29 at the earliest - if there are no regressions. A number of
> drivers use these APIs and usage is a bit messy - so bugs could be
> triggered, etc.
>
Thanks. What is URL of your repository?
I want to look your tree and test it.

Re: [PATCH 1/1] edac x38: new MC driver module

[Ingo Molnar]

* Hitoshi Mitake <h.mitake@gmail.com> wrote:

> On Mon, Dec 1, 2008 at 01:15, Ingo Molnar <mingo@elte.hu> wrote:
> >
> > * Hitoshi Mitake <h.mitake@gmail.com> wrote:
> >
> >> On Sun, 30 Nov 2008 10:24:07 +0100
> >> Ingo Molnar <mingo@elte.hu> wrote:
> >>
> >> >
> >> > the 32-bit build broke promptly - readq/writeq is a family of APIs that
> >> > has to be either fully provided or not provided at all. The fix is below.
> >>
> >> Thanks for your fix and adding!
> >> When will this patch be added to mainline?
> >> I want to rewrite x38_edac.c to adapt new APIs.
> >
> > v2.6.29 at the earliest - if there are no regressions. A number of
> > drivers use these APIs and usage is a bit messy - so bugs could be
> > triggered, etc.
> >
> Thanks. What is URL of your repository?
> I want to look your tree and test it.

you can pick up tip/master via:

  http://people.redhat.com/mingo/tip.git/README

	Ingo

Re: [PATCH 1/1] edac x38: new MC driver module

[Hitoshi Mitake]

On Mon, Dec 1, 2008 at 22:59, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Hitoshi Mitake <h.mitake@gmail.com> wrote:
>
>> On Mon, Dec 1, 2008 at 01:15, Ingo Molnar <mingo@elte.hu> wrote:
>> >
>> > * Hitoshi Mitake <h.mitake@gmail.com> wrote:
>> >
>> >> On Sun, 30 Nov 2008 10:24:07 +0100
>> >> Ingo Molnar <mingo@elte.hu> wrote:
>> >>
>> >> >
>> >> > the 32-bit build broke promptly - readq/writeq is a family of APIs that
>> >> > has to be either fully provided or not provided at all. The fix is below.
>> >>
>> >> Thanks for your fix and adding!
>> >> When will this patch be added to mainline?
>> >> I want to rewrite x38_edac.c to adapt new APIs.
>> >
>> > v2.6.29 at the earliest - if there are no regressions. A number of
>> > drivers use these APIs and usage is a bit messy - so bugs could be
>> > triggered, etc.
>> >
>> Thanks. What is URL of your repository?
>> I want to look your tree and test it.
>
> you can pick up tip/master via:
>
>  http://people.redhat.com/mingo/tip.git/README
>

Thanks!

Re: [PATCH 1/1] edac x38: new MC driver module

[Hitoshi Mitake]

On Tue, 2 Dec 2008 08:58:19 +0900
"Hitoshi Mitake" <h.mitake@gmail.com> wrote:

> On Mon, Dec 1, 2008 at 22:59, Ingo Molnar <mingo@elte.hu> wrote:
> >
> > * Hitoshi Mitake <h.mitake@gmail.com> wrote:
> >
> >> On Mon, Dec 1, 2008 at 01:15, Ingo Molnar <mingo@elte.hu> wrote:
> >> >
> >> > * Hitoshi Mitake <h.mitake@gmail.com> wrote:
> >> >
> >> >> On Sun, 30 Nov 2008 10:24:07 +0100
> >> >> Ingo Molnar <mingo@elte.hu> wrote:
> >> >>
> >> >> >
> >> >> > the 32-bit build broke promptly - readq/writeq is a family of APIs that
> >> >> > has to be either fully provided or not provided at all. The fix is below.
> >> >>
> >> >> Thanks for your fix and adding!
> >> >> When will this patch be added to mainline?
> >> >> I want to rewrite x38_edac.c to adapt new APIs.
> >> >
> >> > v2.6.29 at the earliest - if there are no regressions. A number of
> >> > drivers use these APIs and usage is a bit messy - so bugs could be
> >> > triggered, etc.
> >> >
> >> Thanks. What is URL of your repository?
> >> I want to look your tree and test it.
> >
> > you can pick up tip/master via:
> >
> >  http://people.redhat.com/mingo/tip.git/README
> >
> 

I build your tree with allmodconfig, but there's no compile error.

And I wrote a patch to add HAVE_READQ and HAVE_WRITEQ to all architecutres without x86.

Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com>
---
 arch/alpha/Kconfig   |    2 ++
 arch/ia64/Kconfig    |    2 ++
 arch/mips/Kconfig    |    2 ++
 arch/parisc/Kconfig  |    2 ++
 arch/powerpc/Kconfig |    2 ++
 arch/sh/Kconfig      |    2 ++
 arch/sparc64/Kconfig |    2 ++
 7 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/arch/alpha/Kconfig b/arch/alpha/Kconfig
index 6110197..4355170 100644
--- a/arch/alpha/Kconfig
+++ b/arch/alpha/Kconfig
@@ -8,6 +8,8 @@ config ALPHA
 	select HAVE_AOUT
 	select HAVE_IDE
 	select HAVE_OPROFILE
+	select HAVE_READQ
+	select HAVE_WRITEQ
 	help
 	  The Alpha is a 64-bit general-purpose processor designed and
 	  marketed by the Digital Equipment Corporation of blessed memory,
diff --git a/arch/ia64/Kconfig b/arch/ia64/Kconfig
index 6bd91ed..833bfdd 100644
--- a/arch/ia64/Kconfig
+++ b/arch/ia64/Kconfig
@@ -24,6 +24,8 @@ config IA64
 	select HAVE_DMA_ATTRS
 	select HAVE_KVM
 	select HAVE_ARCH_TRACEHOOK
+	select HAVE_READQ
+	select HAVE_WRITEQ
 	default y
 	help
 	  The Itanium Processor Family is Intel's 64-bit successor to
diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
index f4af967..cf4f046 100644
--- a/arch/mips/Kconfig
+++ b/arch/mips/Kconfig
@@ -7,6 +7,8 @@ config MIPS
 	# Horrible source of confusion.  Die, die, die ...
 	select EMBEDDED
 	select RTC_LIB
+	select HAVE_READQ
+	select HAVE_WRITEQ
 
 mainmenu "Linux/MIPS Kernel Configuration"
 
diff --git a/arch/parisc/Kconfig b/arch/parisc/Kconfig
index 644a70b..4eef1f0 100644
--- a/arch/parisc/Kconfig
+++ b/arch/parisc/Kconfig
@@ -11,6 +11,8 @@ config PARISC
 	select HAVE_OPROFILE
 	select RTC_CLASS
 	select RTC_DRV_PARISC
+	select HAVE_READQ
+	select HAVE_WRITEQ
 	help
 	  The PA-RISC microprocessor is designed by Hewlett-Packard and used
 	  in many of their workstations & servers (HP9000 700 and 800 series,
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 525c13a..9e1701f 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -121,6 +121,8 @@ config PPC
 	select HAVE_DMA_ATTRS if PPC64
 	select USE_GENERIC_SMP_HELPERS if SMP
 	select HAVE_OPROFILE
+	select HAVE_READQ
+	select HAVE_WRITEQ
 
 config EARLY_PRINTK
 	bool
diff --git a/arch/sh/Kconfig b/arch/sh/Kconfig
index 80119b3..410e1fe 100644
--- a/arch/sh/Kconfig
+++ b/arch/sh/Kconfig
@@ -13,6 +13,8 @@ config SUPERH
 	select HAVE_OPROFILE
 	select HAVE_GENERIC_DMA_COHERENT
 	select HAVE_IOREMAP_PROT if MMU
+	select HAVE_READQ
+	select HAVE_WRITEQ
 	help
 	  The SuperH is a RISC processor targeted for use in embedded systems
 	  and consumer electronics; it was also used in the Sega Dreamcast
diff --git a/arch/sparc64/Kconfig b/arch/sparc64/Kconfig
index 3b96e70..b92409f 100644
--- a/arch/sparc64/Kconfig
+++ b/arch/sparc64/Kconfig
@@ -24,6 +24,8 @@ config SPARC64
 	select RTC_DRV_BQ4802
 	select RTC_DRV_SUN4V
 	select RTC_DRV_STARFIRE
+	select HAVE_READQ
+	select HAVE_WRITEQ
 
 config GENERIC_TIME
 	bool
-- 
1.5.6.5

Re: [PATCH 1/1] edac x38: new MC driver module

[Hitoshi Mitake]

On Tue, 2 Dec 2008 08:58:19 +0900
"Hitoshi Mitake" <h.mitake@gmail.com> wrote:

> On Mon, Dec 1, 2008 at 22:59, Ingo Molnar <mingo@elte.hu> wrote:
> >
> > * Hitoshi Mitake <h.mitake@gmail.com> wrote:
> >
> >> On Mon, Dec 1, 2008 at 01:15, Ingo Molnar <mingo@elte.hu> wrote:
> >> >
> >> > * Hitoshi Mitake <h.mitake@gmail.com> wrote:
> >> >
> >> >> On Sun, 30 Nov 2008 10:24:07 +0100
> >> >> Ingo Molnar <mingo@elte.hu> wrote:
> >> >>
> >> >> >
> >> >> > the 32-bit build broke promptly - readq/writeq is a family of APIs that
> >> >> > has to be either fully provided or not provided at all. The fix is below.
> >> >>
> >> >> Thanks for your fix and adding!
> >> >> When will this patch be added to mainline?
> >> >> I want to rewrite x38_edac.c to adapt new APIs.
> >> >
> >> > v2.6.29 at the earliest - if there are no regressions. A number of
> >> > drivers use these APIs and usage is a bit messy - so bugs could be
> >> > triggered, etc.
> >> >
> >> Thanks. What is URL of your repository?
> >> I want to look your tree and test it.
> >
> > you can pick up tip/master via:
> >
> >  http://people.redhat.com/mingo/tip.git/README
> >
> 
> Thanks!


Hi,

Very sorry for long distance between my previous post and this...

I wrote a patch to add HAVE_READQ and HAVE_WRITEQ to each architecture's Kconfig file
which have readq() and writeq().

But there is problem.
I wrote helps for HAVE_READQ and HAVE_WRITEQ in Kconfig file
accodring to the advice by Russell King ( http://marc.info/?l=linux-kernel&m=122701161824218&w=2 ),
but these helps are invisible when I doing menuconfig.
(when type '/' and search readq string, HAVE_READQ found, but
help string is not printed...)

Do you have some nice technique that make these helps visible easily?
I paste a patch for x86 as sample. If I could find a good way,
I'll rewrite patches and send them to maintainers of each architectures.


Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com>

---
 arch/x86/Kconfig |   21 +++++++++++++++++++--
 1 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 9c39095..d22f9a6 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -19,8 +19,6 @@ config X86_64
 config X86
 	def_bool y
 	select HAVE_AOUT if X86_32
-	select HAVE_READQ
-	select HAVE_WRITEQ
 	select HAVE_UNSTABLE_SCHED_CLOCK
 	select HAVE_IDE
 	select HAVE_OPROFILE
@@ -1980,6 +1978,25 @@ config HAVE_ATOMIC_IOMAP
 	def_bool y
 	depends on X86_32
 
+config HAVE_READQ
+	def_bool y
+	help
+	  This is a sign to represent that this architecture provides
+	  readq() function. readq() is a function to read 8 bytes from
+	  I/O space. Each drivers use readq() must depend on this symbol.
+	  Because lots of little private readq() implementations
+	  all over the tree is sucks.
+
+config HAVE_WRITEQ
+	def_bool y
+	help
+	  This is a sign to represent that this architecture provides
+	  writeq() function. writeq() is a function to read 8 bytes from
+	  I/O space. Each drivers use writeq() must depend on this symbol.
+	  Because lots of little private writeq() implementations
+	  all over the tree is sucks.
+
+
 source "net/Kconfig"
 
 source "drivers/Kconfig"
-- 
1.6.1.2

Re: [PATCH 1/1] edac x38: new MC driver module

[Russell King]

On Sat, Feb 21, 2009 at 07:11:01PM +0900, Hitoshi Mitake wrote:
> But there is problem.
> I wrote helps for HAVE_READQ and HAVE_WRITEQ in Kconfig file
> accodring to the advice by Russell King ( http://marc.info/?l=linux-kernel&m=122701161824218&w=2 ),
> but these helps are invisible when I doing menuconfig.
> (when type '/' and search readq string, HAVE_READQ found, but
> help string is not printed...)

Yes, they will be, because users can't change the options.  The point
of providing a help text for them is so that when someone comes across
the options in a few months or years time, they know what the options
are (supposed to be) doing.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCH 1/1] edac x38: new MC driver module

[Sam Ravnborg]

> Hi,
> 
> Very sorry for long distance between my previous post and this...
> 
> I wrote a patch to add HAVE_READQ and HAVE_WRITEQ to each architecture's Kconfig file
> which have readq() and writeq().
> 
> But there is problem.
> I wrote helps for HAVE_READQ and HAVE_WRITEQ in Kconfig file
> accodring to the advice by Russell King ( http://marc.info/?l=linux-kernel&m=122701161824218&w=2 ),
> but these helps are invisible when I doing menuconfig.
> (when type '/' and search readq string, HAVE_READQ found, but
> help string is not printed...)
> 
> Do you have some nice technique that make these helps visible easily?

The options are not visible in menuconfig and therefore the is not
much point in displaying help for them thre when you search for the symbol.
But the help contained in the KConfig file is fully justified as it is
now documented why/when to select these options.

	Sam

Re: [PATCH 1/1] edac x38: new MC driver module

[Hitoshi Mitake]

On Sat, 21 Feb 2009 14:09:37 +0100
Sam Ravnborg <sam@ravnborg.org> wrote:

> > Hi,
> > 
> > Very sorry for long distance between my previous post and this...
> > 
> > I wrote a patch to add HAVE_READQ and HAVE_WRITEQ to each architecture's Kconfig file
> > which have readq() and writeq().
> > 
> > But there is problem.
> > I wrote helps for HAVE_READQ and HAVE_WRITEQ in Kconfig file
> > accodring to the advice by Russell King ( http://marc.info/?l=linux-kernel&m=122701161824218&w=2 ),
> > but these helps are invisible when I doing menuconfig.
> > (when type '/' and search readq string, HAVE_READQ found, but
> > help string is not printed...)
> > 
> > Do you have some nice technique that make these helps visible easily?
> 

Thank you for your replying, Russell and Sam.

> The options are not visible in menuconfig and therefore the is not
> much point in displaying help for them thre when you search for the symbol.
> But the help contained in the KConfig file is fully justified as it is
> now documented why/when to select these options.
> 
> 	Sam

Hmm, so I try to send my patches to each maintainers of architectures.

description of the patch: Adding HAVE_READQ, HAVE_WRITEQ and their help texts
to each architecture's Kconfig file which have readq() and writeq().

Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com>

---
 arch/alpha/Kconfig |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/arch/alpha/Kconfig b/arch/alpha/Kconfig
index 9fb8aae..ecca919 100644
--- a/arch/alpha/Kconfig
+++ b/arch/alpha/Kconfig
@@ -672,3 +672,20 @@ source "crypto/Kconfig"
 
 source "lib/Kconfig"
 
+config HAVE_READQ
+	def_bool y
+	help
+	  This is a sign to represent that this architecture provides
+	  readq() function. readq() is a function to read 8 bytes from
+	  I/O space. Each drivers use readq() must depend on this symbol.
+	  Because lots of little private readq() implementations
+	  all over the tree is sucks.
+
+config HAVE_WRITEQ
+	def_bool y
+	help
+	  This is a sign to represent that this architecture provides
+	  writeq() function. writeq() is a function to read 8 bytes from
+	  I/O space. Each drivers use writeq() must depend on this symbol.
+	  Because lots of little private writeq() implementations
+	  all over the tree is sucks.
-- 
1.6.1.2

Re: [PATCH 1/1] edac x38: new MC driver module

[Hitoshi Mitake]

On Sat, 21 Feb 2009 14:09:37 +0100
Sam Ravnborg <sam@ravnborg.org> wrote:

> > Hi,
> > 
> > Very sorry for long distance between my previous post and this...
> > 
> > I wrote a patch to add HAVE_READQ and HAVE_WRITEQ to each architecture's Kconfig file
> > which have readq() and writeq().
> > 
> > But there is problem.
> > I wrote helps for HAVE_READQ and HAVE_WRITEQ in Kconfig file
> > accodring to the advice by Russell King ( http://marc.info/?l=linux-kernel&m=122701161824218&w=2 ),
> > but these helps are invisible when I doing menuconfig.
> > (when type '/' and search readq string, HAVE_READQ found, but
> > help string is not printed...)
> > 
> > Do you have some nice technique that make these helps visible easily?
> 
> The options are not visible in menuconfig and therefore the is not
> much point in displaying help for them thre when you search for the symbol.
> But the help contained in the KConfig file is fully justified as it is
> now documented why/when to select these options.
> 
> 	Sam

description of the patch: Adding HAVE_READQ, HAVE_WRITEQ and their help texts
to each architecture's Kconfig file which have readq() and writeq().

Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com>

---
 arch/ia64/Kconfig |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/arch/ia64/Kconfig b/arch/ia64/Kconfig
index 6183aec..2e035b0 100644
--- a/arch/ia64/Kconfig
+++ b/arch/ia64/Kconfig
@@ -691,3 +691,21 @@ config IOMMU_HELPER
 
 config IOMMU_API
 	def_bool (DMAR)
+
+config HAVE_READQ
+	def_bool y
+	help
+	  This is a sign to represent that this architecture provides
+	  readq() function. readq() is a function to read 8 bytes from
+	  I/O space. Each drivers use readq() must depend on this symbol.
+	  Because lots of little private readq() implementations
+	  all over the tree is sucks.
+
+config HAVE_WRITEQ
+	def_bool y
+	help
+	  This is a sign to represent that this architecture provides
+	  writeq() function. writeq() is a function to read 8 bytes from
+	  I/O space. Each drivers use writeq() must depend on this symbol.
+	  Because lots of little private writeq() implementations
+	  all over the tree is sucks.
-- 
1.6.1.2

Re: [PATCH 1/1] edac x38: new MC driver module

[Hitoshi Mitake]

On Sat, 21 Feb 2009 14:09:37 +0100
Sam Ravnborg <sam@ravnborg.org> wrote:

> > Hi,
> > 
> > Very sorry for long distance between my previous post and this...
> > 
> > I wrote a patch to add HAVE_READQ and HAVE_WRITEQ to each architecture's Kconfig file
> > which have readq() and writeq().
> > 
> > But there is problem.
> > I wrote helps for HAVE_READQ and HAVE_WRITEQ in Kconfig file
> > accodring to the advice by Russell King ( http://marc.info/?l=linux-kernel&m=122701161824218&w=2 ),
> > but these helps are invisible when I doing menuconfig.
> > (when type '/' and search readq string, HAVE_READQ found, but
> > help string is not printed...)
> > 
> > Do you have some nice technique that make these helps visible easily?
> 
> The options are not visible in menuconfig and therefore the is not
> much point in displaying help for them thre when you search for the symbol.
> But the help contained in the KConfig file is fully justified as it is
> now documented why/when to select these options.
> 
> 	Sam

description of the patch: Adding HAVE_READQ, HAVE_WRITEQ and their help texts
to each architecture's Kconfig file which have readq() and writeq().

Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com>

---
 arch/ia64/Kconfig |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/arch/ia64/Kconfig b/arch/ia64/Kconfig
index 6183aec..2e035b0 100644
--- a/arch/ia64/Kconfig
+++ b/arch/ia64/Kconfig
@@ -691,3 +691,21 @@ config IOMMU_HELPER
 
 config IOMMU_API
 	def_bool (DMAR)
+
+config HAVE_READQ
+	def_bool y
+	help
+	  This is a sign to represent that this architecture provides
+	  readq() function. readq() is a function to read 8 bytes from
+	  I/O space. Each drivers use readq() must depend on this symbol.
+	  Because lots of little private readq() implementations
+	  all over the tree is sucks.
+
+config HAVE_WRITEQ
+	def_bool y
+	help
+	  This is a sign to represent that this architecture provides
+	  writeq() function. writeq() is a function to read 8 bytes from
+	  I/O space. Each drivers use writeq() must depend on this symbol.
+	  Because lots of little private writeq() implementations
+	  all over the tree is sucks.
-- 
1.6.1.2

Re: [PATCH 1/1] edac x38: new MC driver module

[Hitoshi Mitake]

On Sat, 21 Feb 2009 14:09:37 +0100
Sam Ravnborg <sam@ravnborg.org> wrote:

> > Hi,
> > 
> > Very sorry for long distance between my previous post and this...
> > 
> > I wrote a patch to add HAVE_READQ and HAVE_WRITEQ to each architecture's Kconfig file
> > which have readq() and writeq().
> > 
> > But there is problem.
> > I wrote helps for HAVE_READQ and HAVE_WRITEQ in Kconfig file
> > accodring to the advice by Russell King ( http://marc.info/?l=linux-kernel&m=122701161824218&w=2 ),
> > but these helps are invisible when I doing menuconfig.
> > (when type '/' and search readq string, HAVE_READQ found, but
> > help string is not printed...)
> > 
> > Do you have some nice technique that make these helps visible easily?
> 
> The options are not visible in menuconfig and therefore the is not
> much point in displaying help for them thre when you search for the symbol.
> But the help contained in the KConfig file is fully justified as it is
> now documented why/when to select these options.
> 
> 	Sam

Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com>

description of the patch: Adding HAVE_READQ, HAVE_WRITEQ and their help texts
to each architecture's Kconfig file which have readq() and writeq().

---
 arch/mips/Kconfig |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
index 600eef3..44aeeff 100644
--- a/arch/mips/Kconfig
+++ b/arch/mips/Kconfig
@@ -2132,3 +2132,21 @@ source "security/Kconfig"
 source "crypto/Kconfig"
 
 source "lib/Kconfig"
+
+config HAVE_READQ
+	def_bool y
+	help
+	  This is a sign to represent that this architecture provides
+	  readq() function. readq() is a function to read 8 bytes from
+	  I/O space. Each drivers use readq() must depend on this symbol.
+	  Because lots of little private readq() implementations
+	  all over the tree is sucks.
+
+config HAVE_WRITEQ
+	def_bool y
+	help
+	  This is a sign to represent that this architecture provides
+	  writeq() function. writeq() is a function to read 8 bytes from
+	  I/O space. Each drivers use writeq() must depend on this symbol.
+	  Because lots of little private writeq() implementations
+	  all over the tree is sucks.
-- 
1.6.1.2

Re: [PATCH 1/1] edac x38: new MC driver module

[Hitoshi Mitake]

On Sat, 21 Feb 2009 14:09:37 +0100
Sam Ravnborg <sam@ravnborg.org> wrote:

> > Hi,
> > 
> > Very sorry for long distance between my previous post and this...
> > 
> > I wrote a patch to add HAVE_READQ and HAVE_WRITEQ to each architecture's Kconfig file
> > which have readq() and writeq().
> > 
> > But there is problem.
> > I wrote helps for HAVE_READQ and HAVE_WRITEQ in Kconfig file
> > accodring to the advice by Russell King ( http://marc.info/?l=linux-kernel&m=122701161824218&w=2 ),
> > but these helps are invisible when I doing menuconfig.
> > (when type '/' and search readq string, HAVE_READQ found, but
> > help string is not printed...)
> > 
> > Do you have some nice technique that make these helps visible easily?
> 
> The options are not visible in menuconfig and therefore the is not
> much point in displaying help for them thre when you search for the symbol.
> But the help contained in the KConfig file is fully justified as it is
> now documented why/when to select these options.
> 
> 	Sam

Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com>

description of the patch: Adding HAVE_READQ, HAVE_WRITEQ and their help texts
to each architecture's Kconfig file which have readq() and writeq().

---
 arch/mips/Kconfig |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
index 600eef3..44aeeff 100644
--- a/arch/mips/Kconfig
+++ b/arch/mips/Kconfig
@@ -2132,3 +2132,21 @@ source "security/Kconfig"
 source "crypto/Kconfig"
 
 source "lib/Kconfig"
+
+config HAVE_READQ
+	def_bool y
+	help
+	  This is a sign to represent that this architecture provides
+	  readq() function. readq() is a function to read 8 bytes from
+	  I/O space. Each drivers use readq() must depend on this symbol.
+	  Because lots of little private readq() implementations
+	  all over the tree is sucks.
+
+config HAVE_WRITEQ
+	def_bool y
+	help
+	  This is a sign to represent that this architecture provides
+	  writeq() function. writeq() is a function to read 8 bytes from
+	  I/O space. Each drivers use writeq() must depend on this symbol.
+	  Because lots of little private writeq() implementations
+	  all over the tree is sucks.
-- 
1.6.1.2

Re: [PATCH 1/1] edac x38: new MC driver module

[Hitoshi Mitake]

On Sat, 21 Feb 2009 14:09:37 +0100
Sam Ravnborg <sam@ravnborg.org> wrote:

> > Hi,
> > 
> > Very sorry for long distance between my previous post and this...
> > 
> > I wrote a patch to add HAVE_READQ and HAVE_WRITEQ to each architecture's Kconfig file
> > which have readq() and writeq().
> > 
> > But there is problem.
> > I wrote helps for HAVE_READQ and HAVE_WRITEQ in Kconfig file
> > accodring to the advice by Russell King ( http://marc.info/?l=linux-kernel&m=122701161824218&w=2 ),
> > but these helps are invisible when I doing menuconfig.
> > (when type '/' and search readq string, HAVE_READQ found, but
> > help string is not printed...)
> > 
> > Do you have some nice technique that make these helps visible easily?
> 
> The options are not visible in menuconfig and therefore the is not
> much point in displaying help for them thre when you search for the symbol.
> But the help contained in the KConfig file is fully justified as it is
> now documented why/when to select these options.
> 
> 	Sam

description of the patch: Adding HAVE_READQ, HAVE_WRITEQ and their help texts
to each architecture's Kconfig file which have readq() and writeq().

Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com>

---
 arch/powerpc/Kconfig |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 74cc312..a517a8f 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -900,3 +900,21 @@ config PPC_LIB_RHEAP
 	bool
 
 source "arch/powerpc/kvm/Kconfig"
+
+config HAVE_READQ
+	def_bool y
+	help
+	  This is a sign to represent that this architecture provides
+	  readq() function. readq() is a function to read 8 bytes from
+	  I/O space. Each drivers use readq() must depend on this symbol.
+	  Because lots of little private readq() implementations
+	  all over the tree is sucks.
+
+config HAVE_WRITEQ
+	def_bool y
+	help
+	  This is a sign to represent that this architecture provides
+	  writeq() function. writeq() is a function to read 8 bytes from
+	  I/O space. Each drivers use writeq() must depend on this symbol.
+	  Because lots of little private writeq() implementations
+	  all over the tree is sucks.
-- 
1.6.1.2

Re: [PATCH 1/1] edac x38: new MC driver module

[Hitoshi Mitake]

On Sat, 21 Feb 2009 14:09:37 +0100
Sam Ravnborg <sam@ravnborg.org> wrote:

> > Hi,
> > 
> > Very sorry for long distance between my previous post and this...
> > 
> > I wrote a patch to add HAVE_READQ and HAVE_WRITEQ to each architecture's Kconfig file
> > which have readq() and writeq().
> > 
> > But there is problem.
> > I wrote helps for HAVE_READQ and HAVE_WRITEQ in Kconfig file
> > accodring to the advice by Russell King ( http://marc.info/?l=linux-kernel&m=122701161824218&w=2 ),
> > but these helps are invisible when I doing menuconfig.
> > (when type '/' and search readq string, HAVE_READQ found, but
> > help string is not printed...)
> > 
> > Do you have some nice technique that make these helps visible easily?
> 
> The options are not visible in menuconfig and therefore the is not
> much point in displaying help for them thre when you search for the symbol.
> But the help contained in the KConfig file is fully justified as it is
> now documented why/when to select these options.
> 
> 	Sam

description of the patch: Adding HAVE_READQ, HAVE_WRITEQ and their help texts
to each architecture's Kconfig file which have readq() and writeq().

Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com>

---
 arch/sh/Kconfig |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/arch/sh/Kconfig b/arch/sh/Kconfig
index ebabe51..7b1f664 100644
--- a/arch/sh/Kconfig
+++ b/arch/sh/Kconfig
@@ -699,3 +699,21 @@ source "security/Kconfig"
 source "crypto/Kconfig"
 
 source "lib/Kconfig"
+
+config HAVE_READQ
+	def_bool y
+	help
+	  This is a sign to represent that this architecture provides
+	  readq() function. readq() is a function to read 8 bytes from
+	  I/O space. Each drivers use readq() must depend on this symbol.
+	  Because lots of little private readq() implementations
+	  all over the tree is sucks.
+
+config HAVE_WRITEQ
+	def_bool y
+	help
+	  This is a sign to represent that this architecture provides
+	  writeq() function. writeq() is a function to read 8 bytes from
+	  I/O space. Each drivers use writeq() must depend on this symbol.
+	  Because lots of little private writeq() implementations
+	  all over the tree is sucks.
-- 
1.6.1.2

Re: [PATCH 1/1] edac x38: new MC driver module

[Hitoshi Mitake]

On Sat, 21 Feb 2009 14:09:37 +0100
Sam Ravnborg <sam@ravnborg.org> wrote:

> > Hi,
> > 
> > Very sorry for long distance between my previous post and this...
> > 
> > I wrote a patch to add HAVE_READQ and HAVE_WRITEQ to each architecture's Kconfig file
> > which have readq() and writeq().
> > 
> > But there is problem.
> > I wrote helps for HAVE_READQ and HAVE_WRITEQ in Kconfig file
> > accodring to the advice by Russell King ( http://marc.info/?l=linux-kernel&m=122701161824218&w=2 ),
> > but these helps are invisible when I doing menuconfig.
> > (when type '/' and search readq string, HAVE_READQ found, but
> > help string is not printed...)
> > 
> > Do you have some nice technique that make these helps visible easily?
> 
> The options are not visible in menuconfig and therefore the is not
> much point in displaying help for them thre when you search for the symbol.
> But the help contained in the KConfig file is fully justified as it is
> now documented why/when to select these options.
> 
> 	Sam

description of the patch: Adding HAVE_READQ, HAVE_WRITEQ and their help texts
to each architecture's Kconfig file which have readq() and writeq().

Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com>

---
 arch/sparc/Kconfig |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/arch/sparc/Kconfig b/arch/sparc/Kconfig
index c3ea215..eb5efa3 100644
--- a/arch/sparc/Kconfig
+++ b/arch/sparc/Kconfig
@@ -527,3 +527,23 @@ source "security/Kconfig"
 source "crypto/Kconfig"
 
 source "lib/Kconfig"
+
+config HAVE_READQ
+	def_bool y
+	depends on SPARC64
+	help
+	  This is a sign to represent that this architecture provides
+	  readq() function. readq() is a function to read 8 bytes from
+	  I/O space. Each drivers use readq() must depend on this symbol.
+	  Because lots of little private readq() implementations
+	  all over the tree is sucks.
+
+config HAVE_WRITEQ
+	def_bool y
+	depends on SPARC64
+	help
+	  This is a sign to represent that this architecture provides
+	  writeq() function. writeq() is a function to read 8 bytes from
+	  I/O space. Each drivers use writeq() must depend on this symbol.
+	  Because lots of little private writeq() implementations
+	  all over the tree is sucks.
-- 
1.6.1.2

Re: [PATCH 1/1] edac x38: new MC driver module

[Andrew Morton]

(cc linux-arch)

On Mon, 10 Nov 2008 00:10:34 +0900 "Hitoshi Mitake" <h.mitake@gmail.com> wrote:

> ...
>
> >> -static u64 x38_readq(const void __iomem *addr)
> >> +#ifndef CONFIG_X86_64
> >> +static u64 readq(const void __iomem *addr)
> >
> > hm, it'd be nice if there was some more general way of determining
> > whether the architecture provides readq/writeq.
> >
> 
> 
> I found this code in include/asm-x86/io.h
> 
> #ifdef CONFIG_X86_64
> 
> ...
> 
> /* Let people know we have them */
> #define readq readq
> #define writeq writeq
> #endif
> 
> x86 programmers are able to know existence of readq/writeq by using
> this definition.
> 
> And I grepped,
> 
> % grep readq `find include/asm-* -name "*.h"`
> include/asm-mips/io.h:			".set	mips3"		"\t\t# __readq"	"\n\t"	\
> include/asm-mips/io.h:#define readq_relaxed			readq
> include/asm-mips/io.h:#define readq				readq
> include/asm-mips/txx9/tx4927.h:	((__u32)__raw_readq(&tx4927_ccfgptr->crir)
> >> 16)
> include/asm-mips/txx9/tx4927.h:#define
> TX4927_SDRAMC_CR(ch)	__raw_readq(&tx4927_sdramcptr->cr[(ch)])
> include/asm-mips/txx9/tx4927.h:#define
> TX4927_EBUSC_CR(ch)	__raw_readq(&tx4927_ebuscptr->cr[(ch)])
> include/asm-mips/txx9/tx4927.h:	____raw_writeq(____raw_readq(adr) & ~bits, adr);
> include/asm-mips/txx9/tx4927.h:	____raw_writeq(____raw_readq(adr) | bits, adr);
> include/asm-mips/txx9/tx4927.h:	____raw_writeq(____raw_readq(&tx4927_ccfgptr->ccfg)
> include/asm-mips/txx9/tx4927.h:	____raw_writeq((____raw_readq(&tx4927_ccfgptr->ccfg)
> include/asm-mips/txx9/tx4927.h:	____raw_writeq((____raw_readq(&tx4927_ccfgptr->ccfg)
> include/asm-mips/txx9/tx4938.h:	((__u32)__raw_readq(&tx4938_ccfgptr->crir)
> >> 16)
> include/asm-parisc/io.h:static inline unsigned long long
> gsc_readq(unsigned long addr)
> include/asm-parisc/io.h:static inline unsigned long long
> __raw_readq(const volatile void __iomem *addr)
> include/asm-parisc/io.h:#define readq(addr) __fswab64(__raw_readq(addr))
> include/asm-parisc/io.h:#define readq_relaxed(addr) readq(addr)
> include/asm-x86/io.h:build_mmio_read(readq, "q", unsigned long, "=r", :"memory")
> include/asm-x86/io.h:build_mmio_read(__readq, "q", unsigned long, "=r", )
> include/asm-x86/io.h:#define readq_relaxed(a) __readq(a)
> include/asm-x86/io.h:#define __raw_readq __readq
> include/asm-x86/io.h:#define readq readq
> 
> It seems that architectures that provide readq/writeq are
> mips, parisc and x86 (and x86_64).
> 
> mips and x86 provides this line
> #define readq readq
> to let user know existence of readq (and writeq),
> and parisc doesn't provide.
> But there is,
> #define readq(addr) __fswab64(__raw_readq(addr))
> in parisc.
> 
> There is a difference between mips and x86's readq/writeq and
> parisc's readq/writeq. mips and x86's definition is only token,
> but parisc's definition is macro function.
> 
> But these defines can be used to determine existence of readq/writeq
> by common preprocessor like this,
> #ifndef readq
> /* programmer can define private version of readq (or writeq) */
> #endif
> 
> Is this way enough general for our requirement?
> (If we use this as general way to determine existence of readq/writeq,
> I want other architecture's developer (whose architecture provides
> readq/writeq in the future)
> to support same way.)

Yes, I'd say that

#ifdef readq

Is a suitable way of determining whether the architecture implements
readq and writeq.  It isn't pretty, but it will suffice.

A problem with it is that drivers will then do

#ifndef readq
<provide a local implementation here>
#endif

which rather sucks - we don't want lots of little private readq/writeq
implementations all over the tree.

Perhaps it would be better to have a CONFIG_ARCH_HAS_READQ and to then
disable these drivers on the architectures which don't provide
readq/writeq support.


Also, I'm not sure that we have sufficiently defined the semantics of
these operations, and whether all the architectures which do purport to
implement them actually implement them with the same semantics.

Re: [PATCH 1/1] edac x38: new MC driver module

[Paul Mundt]

On Sun, Nov 09, 2008 at 11:26:46AM -0800, Andrew Morton wrote:
> (cc linux-arch)
> 
> > It seems that architectures that provide readq/writeq are
> > mips, parisc and x86 (and x86_64).
> > 
There are more than that, grep arch/*/include also.

In addition to mips, parisc, and x86, there is ia64, alpha, sh, and
sparc.

> #ifdef readq
> 
> Is a suitable way of determining whether the architecture implements
> readq and writeq.  It isn't pretty, but it will suffice.
> 
> A problem with it is that drivers will then do
> 
> #ifndef readq
> <provide a local implementation here>
> #endif
> 
> which rather sucks - we don't want lots of little private readq/writeq
> implementations all over the tree.
> 
> Perhaps it would be better to have a CONFIG_ARCH_HAS_READQ and to then
> disable these drivers on the architectures which don't provide
> readq/writeq support.
> 
However this is handled, we don't want a rehash of the read/writes{b,w,l} fiasco.

Allowing drivers to do their own local implementations of these things
has always been a complete disaster. A Kconfig option will at least take
care of having these craptastic ifdef lists for architectures in every
driver that rolls its own implementation.

Even a sub-optimal asm-generic version would be preferable, if the
semantics are well enough defined and consistently adhered to.

Re: [PATCH 1/1] edac x38: new MC driver module

[Hitoshi Mitake]

On Tue, 11 Nov 2008 15:11:40 +0900
Paul Mundt <lethal@linux-sh.org> wrote:

> On Sun, Nov 09, 2008 at 11:26:46AM -0800, Andrew Morton wrote:
> > (cc linux-arch)
> > 
> > > It seems that architectures that provide readq/writeq are
> > > mips, parisc and x86 (and x86_64).
> > > 
> There are more than that, grep arch/*/include also.
> 
> In addition to mips, parisc, and x86, there is ia64, alpha, sh, and
> sparc.

I didn't noticed these functions, thanks.

> 
> > #ifdef readq
> > 
> > Is a suitable way of determining whether the architecture implements
> > readq and writeq.  It isn't pretty, but it will suffice.
> > 
> > A problem with it is that drivers will then do
> > 
> > #ifndef readq
> > <provide a local implementation here>
> > #endif
> > 
> > which rather sucks - we don't want lots of little private readq/writeq
> > implementations all over the tree.
> > 
> > Perhaps it would be better to have a CONFIG_ARCH_HAS_READQ and to then
> > disable these drivers on the architectures which don't provide
> > readq/writeq support.
> > 
> However this is handled, we don't want a rehash of the read/writes{b,w,l} fiasco.
> 
> Allowing drivers to do their own local implementations of these things
> has always been a complete disaster. A Kconfig option will at least take
> care of having these craptastic ifdef lists for architectures in every
> driver that rolls its own implementation.
> 
> Even a sub-optimal asm-generic version would be preferable, if the
> semantics are well enough defined and consistently adhered to.

I found nice source file, lib/iomap.c.
There are architecture independent read/write{b,w,l} (named like ioread8).

I want to implement architecture independent readq/writeq in lib/iomap.c .
Andrew told,

> Perhaps it would be better to have a CONFIG_ARCH_HAS_READQ and to then
> disable these drivers on the architectures which don't provide
> readq/writeq support.

But I want to use x38_edac.c on x86_32 environment,
and I think similar desire may occur in the future.

Is this way good enough? Request for comment.

Re: [PATCH 1/1] edac x38: new MC driver module

[Ralf Baechle]

On Sun, Nov 09, 2008 at 11:26:46AM -0800, Andrew Morton wrote:

> Perhaps it would be better to have a CONFIG_ARCH_HAS_READQ and to then
> disable these drivers on the architectures which don't provide
> readq/writeq support.

And we also need to define the exact semantics.  Questions coming to mind:

  o are implementations performing 2 32-bit accesses acceptable?
  o if so, what ordering for the two accesses is acceptable?

  Ralf

Re: [PATCH 1/1] edac x38: new MC driver module

[Russell King]

On Tue, Nov 18, 2008 at 12:16:20PM +0000, Ralf Baechle wrote:
> On Sun, Nov 09, 2008 at 11:26:46AM -0800, Andrew Morton wrote:
> 
> > Perhaps it would be better to have a CONFIG_ARCH_HAS_READQ and to then
> > disable these drivers on the architectures which don't provide
> > readq/writeq support.
> 
> And we also need to define the exact semantics.  Questions coming to mind:
> 
>   o are implementations performing 2 32-bit accesses acceptable?
>   o if so, what ordering for the two accesses is acceptable?

and don't forget to document the semantics.  If we're going to end up
with CONFIG_ARCH_HAS_READQ which architectures can select, I suggest
putting it in the help for that symbol.  Why not another random file
in Documentation/ ?  Because it's a random file in Documentation/
that'll be overlooked when someone decided to select ARCH_HAS_READQ.
If it's along side the relevent config option, there is a higher
chance it will be noticed.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCH 1/1] edac x38: new MC driver module

[Hitoshi Mitake]

On Tue, 18 Nov 2008 12:32:15 +0000
Russell King <rmk+lkml@arm.linux.org.uk> wrote:

> On Tue, Nov 18, 2008 at 12:16:20PM +0000, Ralf Baechle wrote:
> > On Sun, Nov 09, 2008 at 11:26:46AM -0800, Andrew Morton wrote:
> > 
> > > Perhaps it would be better to have a CONFIG_ARCH_HAS_READQ and to then
> > > disable these drivers on the architectures which don't provide
> > > readq/writeq support.
> > 
> > And we also need to define the exact semantics.  Questions coming to mind:
> > 
> >   o are implementations performing 2 32-bit accesses acceptable?
> >   o if so, what ordering for the two accesses is acceptable?
> 
> and don't forget to document the semantics.  If we're going to end up
> with CONFIG_ARCH_HAS_READQ which architectures can select, I suggest
> putting it in the help for that symbol.  Why not another random file
> in Documentation/ ?  Because it's a random file in Documentation/
> that'll be overlooked when someone decided to select ARCH_HAS_READQ.
> If it's along side the relevent config option, there is a higher
> chance it will be noticed.
> 

Sorry for my late response...

I knew that implementing architecture-independed readq/writeq is too hard.
To check that implementation is good for every architecture and test that readq/writeq are
difficult works.

So I wrote patch in Andrew's way.
This patch adds ARCH_HAS_READQ to X86_32 and X86_64, adds ARCH_HAS_WRITEQ to X86_64
and adds readq() to X86_32 (writeq is yet).

I want someone to review it. If this patch is good enough, I'll write help document and more patch
adding ARCH_HAS_READQ and ARCH_HAS_WRITEQ to other architectre which has readq/writeq.

description of this patch: Adding config value to x86 architecture to determine existence of readq/writeq


Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com>
---
 arch/x86/Kconfig          |    3 +++
 arch/x86/include/asm/io.h |    8 ++++++++
 2 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index ac22bb7..8f3c949 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -11,9 +11,12 @@ config 64BIT
 
 config X86_32
 	def_bool !64BIT
+	select ARCH_HAS_READQ
 
 config X86_64
 	def_bool 64BIT
+	select ARCH_HAS_READQ
+	select ARCH_HAS_WRITEQ
 
 ### Arch settings
 config X86
diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index ac2abc8..2a8fc26 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -57,6 +57,14 @@ build_mmio_write(__writeq, "q", unsigned long, "r", )
 /* Let people know we have them */
 #define readq readq
 #define writeq writeq
+
+#else  /* CONFIG_X86_32 */
+
+static inline unsigned long readq(const volatile void __iomem *addr)
+{
+	return readl(addr) | (((u64)readl(addr + 4)) << 32);
+}
+
 #endif
 
 extern int iommu_bio_merge;
-- 
1.5.6.5

Re: [PATCH 1/1] edac x38: new MC driver module

[H. Peter Anvin]

Hitoshi Mitake wrote:
> --- a/arch/x86/include/asm/io.h
> +++ b/arch/x86/include/asm/io.h
> @@ -57,6 +57,14 @@ build_mmio_write(__writeq, "q", unsigned long, "r", )
>  /* Let people know we have them */
>  #define readq readq
>  #define writeq writeq
> +
> +#else  /* CONFIG_X86_32 */
> +
> +static inline unsigned long readq(const volatile void __iomem *addr)
> +{
> +	return readl(addr) | (((u64)readl(addr + 4)) << 32);
> +}
> +
>  #endif
>  

Let's see:

* undefined ordering of operations (at least on x86, they really should
be performed in LSB first order.)

* Using "unsigned long" for a 64-bit number on a 32-bit architecture.

* Arithmetic on a void pointer.


Try something like:

static inline u64 readq(const volative void __iomem *addr)
{
	volatile u32 __iomem *__p = addr;
	u32 __l, __h;

	__l = readl(p);
	__h = readl(p+1);

	return __l + ((u64)__h << 32);
}

	-hpa

RE: [PATCH 1/1] edac x38: new MC driver module

[Luck, Tony]

>       volatile u32 __iomem *__p = addr;
>       u32 __l, __h;

Do we really need all the "__" inside an inline function?  Why
not just call these "p", "l" and "h"?

-Tony

Re: [PATCH 1/1] edac x38: new MC driver module

[H. Peter Anvin]

Luck, Tony wrote:
>>       volatile u32 __iomem *__p = addr;
>>       u32 __l, __h;
> 
> Do we really need all the "__" inside an inline function?  Why
> not just call these "p", "l" and "h"?

Sorry, user space habit (in userspace, and in anything that can be
included from userspace, one needs the __ to keep the namespace clean.)

	-hpa

Re: [PATCH 1/1] edac x38: new MC driver module

[Hitoshi Mitake]

On Mon, 24 Nov 2008 10:02:40 -0800
"H. Peter Anvin" <hpa@zytor.com> wrote:

Thanks for your reviews and advices, Peter and Tony.

> Luck, Tony wrote:
> >>       volatile u32 __iomem *__p = addr;
> >>       u32 __l, __h;
> > 
> > Do we really need all the "__" inside an inline function?  Why
> > not just call these "p", "l" and "h"?
> 
> Sorry, user space habit (in userspace, and in anything that can be
> included from userspace, one needs the __ to keep the namespace clean.)

I think which need __ is variable type name, not each variable name.
Name of each local variables can't effect namespace.
And I found this comment in asm-generic/int-ll64.h,
/*
 * __xx is ok: it doesn't pollute the POSIX namespace. Use these in the
 * header files exported to user space
 */

According to your advice, I rewrote the patch, how is this?

description of this patch: Adding config value to x86 architecture to determine existence of readq/writeq

Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com>
---
 arch/x86/Kconfig          |    3 +++
 arch/x86/include/asm/io.h |   17 +++++++++++++++++
 2 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index ac22bb7..8f3c949 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -11,9 +11,12 @@ config 64BIT
 
 config X86_32
 	def_bool !64BIT
+	select ARCH_HAS_READQ
 
 config X86_64
 	def_bool 64BIT
+	select ARCH_HAS_READQ
+	select ARCH_HAS_WRITEQ
 
 ### Arch settings
 config X86
diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index ac2abc8..5322d18 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -4,6 +4,7 @@
 #define ARCH_HAS_IOREMAP_WC
 
 #include <linux/compiler.h>
+#include <asm-generic/int-ll64.h>
 
 #define build_mmio_read(name, size, type, reg, barrier) \
 static inline type name(const volatile void __iomem *addr) \
@@ -57,6 +58,22 @@ build_mmio_write(__writeq, "q", unsigned long, "r", )
 /* Let people know we have them */
 #define readq readq
 #define writeq writeq
+
+#else  /* CONFIG_X86_32 from here */
+
+static inline __u64 readq(const volatile void __iomem *addr)
+{
+	const volatile u32 __iomem *p = addr;
+	u32 l, h;
+
+	l = readl(p);
+	h = readl(p+1);
+
+	return l + ((u64)h << 32);
+}
+
+#define readq readq
+
 #endif
 
 extern int iommu_bio_merge;
-- 
1.5.6.5

Re: [PATCH 1/1] edac x38: new MC driver module

[H. Peter Anvin]

Hitoshi Mitake wrote:
> I think which need __ is variable type name, not each variable name.
> Name of each local variables can't effect namespace.

Wrong.  It affects the namespace in the sense that it can interfere with
user-created macros.  Again, this is only an issue for user space.

> And I found this comment in asm-generic/int-ll64.h,
> /*
>  * __xx is ok: it doesn't pollute the POSIX namespace. Use these in the
>  * header files exported to user space
>  */
> 
> According to your advice, I rewrote the patch, how is this?

Are you planning to add writeq() as well?

	-hpa

Re: [PATCH 1/1] edac x38: new MC driver module

[Hitoshi Mitake]

On Mon, 24 Nov 2008 21:13:58 -0800
"H. Peter Anvin" <hpa@zytor.com> wrote:

> Hitoshi Mitake wrote:
> > I think which need __ is variable type name, not each variable name.
> > Name of each local variables can't effect namespace.
> 
> Wrong.  It affects the namespace in the sense that it can interfere with
> user-created macros.  Again, this is only an issue for user space.

Sorry, I've misunderstood. I didn't think about macros.

> 
> > And I found this comment in asm-generic/int-ll64.h,
> > /*
> >  * __xx is ok: it doesn't pollute the POSIX namespace. Use these in the
> >  * header files exported to user space
> >  */
> > 
> > According to your advice, I rewrote the patch, how is this?
> 
> Are you planning to add writeq() as well?
> 

Yes, I want to add writeq().
But there's a problem that
I don't have a plan to use writeq() now, so I can't test writeq() soon.

How is this? I think it isn't bad. I want to hear your opinion.

static inline void writeq(__u64 val, volatile void __iomem *addr)
{
	writel((unsigned int)val, addr);
	writel((unsigned int)(val >> 32), addr+1);
}

Re: [PATCH 1/1] edac x38: new MC driver module

[Geert Uytterhoeven]

On Wed, 26 Nov 2008, Hitoshi Mitake wrote:
> On Mon, 24 Nov 2008 21:13:58 -0800
> "H. Peter Anvin" <hpa@zytor.com> wrote:
> > Are you planning to add writeq() as well?
> 
> Yes, I want to add writeq().
> But there's a problem that
> I don't have a plan to use writeq() now, so I can't test writeq() soon.
> 
> How is this? I think it isn't bad. I want to hear your opinion.
> 
> static inline void writeq(__u64 val, volatile void __iomem *addr)
> {
> 	writel((unsigned int)val, addr);
> 	writel((unsigned int)(val >> 32), addr+1);
                                               ^
					       4

> }

Gr{oetje,eeting}s,

						Geert

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

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

Re: [PATCH 1/1] edac x38: new MC driver module

[Hitoshi Mitake]

On Tue, 25 Nov 2008 16:46:18 +0100 (CET)
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> On Wed, 26 Nov 2008, Hitoshi Mitake wrote:
> > On Mon, 24 Nov 2008 21:13:58 -0800
> > "H. Peter Anvin" <hpa@zytor.com> wrote:
> > > Are you planning to add writeq() as well?
> > 
> > Yes, I want to add writeq().
> > But there's a problem that
> > I don't have a plan to use writeq() now, so I can't test writeq() soon.
> > 
> > How is this? I think it isn't bad. I want to hear your opinion.
> > 
> > static inline void writeq(__u64 val, volatile void __iomem *addr)
> > {
> > 	writel((unsigned int)val, addr);
> > 	writel((unsigned int)(val >> 32), addr+1);
>                                                ^
> 					       4
> 
> > }
> 

Thanks, I missed about void pointer...

Re: [PATCH 1/1] edac x38: new MC driver module

[Hitoshi Mitake]

On Wed, 26 Nov 2008 01:10:59 +0900
Hitoshi Mitake <h.mitake@gmail.com> wrote:

> On Tue, 25 Nov 2008 16:46:18 +0100 (CET)
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> 
> > On Wed, 26 Nov 2008, Hitoshi Mitake wrote:
> > > On Mon, 24 Nov 2008 21:13:58 -0800
> > > "H. Peter Anvin" <hpa@zytor.com> wrote:
> > > > Are you planning to add writeq() as well?
> > > 
> > > Yes, I want to add writeq().
> > > But there's a problem that
> > > I don't have a plan to use writeq() now, so I can't test writeq() soon.
> > > 
> > > How is this? I think it isn't bad. I want to hear your opinion.
> > > 
> > > static inline void writeq(__u64 val, volatile void __iomem *addr)
> > > {
> > > 	writel((unsigned int)val, addr);
> > > 	writel((unsigned int)(val >> 32), addr+1);
> >                                                ^
> > 					       4
> > 
> > > }
> > 
> 
> Thanks, I missed about void pointer...
> 

I rewrote the patch. I think newest version is good enough.
Could you please review this?

description of this patch
Adding implementation of readq/writeq to x86_32,
and adding config value to x86 architecture to determine existence of readq/writeq

Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com>
---
 arch/x86/Kconfig          |    4 ++++
 arch/x86/include/asm/io.h |   24 ++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index ac22bb7..10bd84c 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -11,9 +11,13 @@ config 64BIT
 
 config X86_32
 	def_bool !64BIT
+	select ARCH_HAS_READQ
+	select ARCH_HAS_WRITEQ
 
 config X86_64
 	def_bool 64BIT
+	select ARCH_HAS_READQ
+	select ARCH_HAS_WRITEQ
 
 ### Arch settings
 config X86
diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index ac2abc8..ac15d0e 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -4,6 +4,7 @@
 #define ARCH_HAS_IOREMAP_WC
 
 #include <linux/compiler.h>
+#include <asm-generic/int-ll64.h>
 
 #define build_mmio_read(name, size, type, reg, barrier) \
 static inline type name(const volatile void __iomem *addr) \
@@ -57,6 +58,29 @@ build_mmio_write(__writeq, "q", unsigned long, "r", )
 /* Let people know we have them */
 #define readq readq
 #define writeq writeq
+
+#else  /* CONFIG_X86_32 from here */
+
+static inline __u64 readq(const volatile void __iomem *addr)
+{
+	const volatile u32 __iomem *p = addr;
+	u32 l, h;
+
+	l = readl(p);
+	h = readl(p+1);
+
+	return l + ((u64)h << 32);
+}
+
+static inline void writeq(__u64 val, volatile void __iomem *addr)
+{
+	writel((unsigned int)val, addr);
+	writel((unsigned int)(val >> 32), addr+4);
+}
+
+#define readq readq
+#define writeq writeq
+
 #endif
 
 extern int iommu_bio_merge;
-- 
1.5.6.5