[POWERPC] convert string i/o operations to C

[POWERPC] convert string i/o operations to C

[Stephen Rothwell]

This produces essentially the same code and will make the iSeries i/o
consolidation easier.

Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
 arch/powerpc/kernel/Makefile    |    2 -
 arch/powerpc/kernel/io.c        |  125 +++++++++++++++++++++++++++++++++++++++
 arch/powerpc/kernel/misc.S      |   95 ------------------------------
 arch/powerpc/kernel/ppc_ksyms.c |    7 --
 4 files changed, 126 insertions(+), 103 deletions(-)

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index 8b3f4fa..8b133af 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -51,7 +51,7 @@ extra-$(CONFIG_8xx)		:= head_8xx.o
 extra-y				+= vmlinux.lds
 
 obj-y				+= time.o prom.o traps.o setup-common.o \
-				   udbg.o misc.o
+				   udbg.o misc.o io.o
 obj-$(CONFIG_PPC32)		+= entry_32.o setup_32.o misc_32.o
 obj-$(CONFIG_PPC64)		+= misc_64.o dma_64.o iommu.o
 obj-$(CONFIG_PPC_MULTIPLATFORM)	+= prom_init.o
diff --git a/arch/powerpc/kernel/io.c b/arch/powerpc/kernel/io.c
new file mode 100644
index 0000000..c83970d
--- /dev/null
+++ b/arch/powerpc/kernel/io.c
@@ -0,0 +1,125 @@
+/*
+ * I/O string operations
+ *    Copyright (C) 1995-1996 Gary Thomas (gdt@linuxppc.org)
+ *    Copyright (C) 2006 IBM Corporation
+ *
+ * Largely rewritten by Cort Dougan (cort@cs.nmt.edu)
+ * and Paul Mackerras.
+ *
+ * Adapted for iSeries by Mike Corrigan (mikejc@us.ibm.com)
+ * PPC64 updates by Dave Engebretsen (engebret@us.ibm.com)
+ *
+ * Rewritten in C by Stephen Rothwell.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include <linux/module.h>
+
+#include <asm/io.h>
+
+void _insb(volatile u8 __iomem *port, void *buf, int ns)
+{
+	asm volatile("sync");
+	if (ns <= 0)
+		return;
+	asm volatile(
+		"mtctr	%2\n"
+		"subi	%1,%1,1\n"
+		"0:	lbz	%2,0(%0)\n"
+		"eieio\n"
+		"stbu	%2,1(%1)\n"
+		"bdnz	0b\n"
+		"twi	0,%2,0\n"
+		"isync\n"
+	: : "r" (port), "r" (buf), "r" (ns));
+}
+EXPORT_SYMBOL(_insb);
+
+void _outsb(volatile u8 __iomem *port, const void *buf, int ns)
+{
+	if (ns <= 0)
+		return;
+	asm volatile(
+		"mtctr	%2\n"
+		"subi	%1,%1,1\n"
+		"sync\n"
+		"0:	lbzu	%2,1(%1)\n"
+		"stb	%2,0(%0)\n"
+		"bdnz	0b\n"
+		"sync\n"
+	: : "r" (port), "r" (buf), "r" (ns));
+}
+EXPORT_SYMBOL(_outsb);
+
+void _insw_ns(volatile u16 __iomem *port, void *buf, int ns)
+{
+	asm volatile("sync");
+	if (ns <= 0)
+		return;
+	asm volatile(
+		"mtctr	%2\n"
+		"subi	%1,%1,2\n"
+		"0:	lhz	%2,0(%0)\n"
+		"eieio\n"
+		"sthu	%2,2(%1)\n"
+		"bdnz	0b\n"
+		"twi	0,%2,0\n"
+		"isync\n"
+	: : "r" (port), "r" (buf), "r" (ns));
+}
+EXPORT_SYMBOL(_insw_ns);
+
+void _outsw_ns(volatile u16 __iomem *port, const void *buf, int ns)
+{
+	if (ns <= 0)
+		return;
+	asm volatile(
+		"mtctr	%2\n"
+		"subi	%1,%1,2\n"
+		"sync\n"
+		"0:	lhzu	%2,2(%1)\n"
+		"sth	%2,0(%0)\n"
+		"bdnz	0b\n"
+		"sync\n"
+	: : "r" (port), "r" (buf), "r" (ns));
+}
+EXPORT_SYMBOL(_outsw_ns);
+
+void _insl_ns(volatile u32 __iomem *port, void *buf, int nl)
+{
+	asm volatile("sync");
+	if (nl <= 0)
+		return;
+	asm volatile(
+		"mtctr	%2\n"
+		"subi	%1,%1,4\n"
+		"0:	lwz	%2,0(%0)\n"
+		"eieio\n"
+		"stwu	%2,4(%1)\n"
+		"bdnz	0b\n"
+		"twi	0,%2,0\n"
+		"isync\n"
+	: : "r" (port), "r" (buf), "r" (nl));
+}
+EXPORT_SYMBOL(_insl_ns);
+
+void _outsl_ns(volatile u32 __iomem *port, const void *buf, int nl)
+{
+	if (nl <= 0)
+		return;
+	asm volatile(
+		"mtctr	%2\n"
+		"subi	%1,%1,4\n"
+		"sync\n"
+		"0:	lwzu	%2,4(%1)\n"
+		"stw	%2,0(%0)\n"
+		"bdnz	0b\n"
+		"sync\n"
+	: : "r" (port), "r" (buf), "r" (nl));
+}
+EXPORT_SYMBOL(_outsl_ns);
diff --git a/arch/powerpc/kernel/misc.S b/arch/powerpc/kernel/misc.S
index 6feb391..330c9dc 100644
--- a/arch/powerpc/kernel/misc.S
+++ b/arch/powerpc/kernel/misc.S
@@ -43,98 +43,3 @@ _GLOBAL(add_reloc_offset)
 	add	r3,r3,r5
 	mtlr	r0
 	blr
-
-/*
- * I/O string operations
- *
- * insb(port, buf, len)
- * outsb(port, buf, len)
- * insw(port, buf, len)
- * outsw(port, buf, len)
- * insl(port, buf, len)
- * outsl(port, buf, len)
- * insw_ns(port, buf, len)
- * outsw_ns(port, buf, len)
- * insl_ns(port, buf, len)
- * outsl_ns(port, buf, len)
- *
- * The *_ns versions don't do byte-swapping.
- */
-_GLOBAL(_insb)
-	sync
-	cmpwi	0,r5,0
-	mtctr	r5
-	subi	r4,r4,1
-	blelr-
-00:	lbz	r5,0(r3)
-	eieio
-	stbu	r5,1(r4)
-	bdnz	00b
-	twi	0,r5,0
-	isync
-	blr
-
-_GLOBAL(_outsb)
-	cmpwi	0,r5,0
-	mtctr	r5
-	subi	r4,r4,1
-	blelr-
-	sync
-00:	lbzu	r5,1(r4)
-	stb	r5,0(r3)
-	bdnz	00b
-	sync
-	blr
-
-_GLOBAL(_insw_ns)
-	sync
-	cmpwi	0,r5,0
-	mtctr	r5
-	subi	r4,r4,2
-	blelr-
-00:	lhz	r5,0(r3)
-	eieio
-	sthu	r5,2(r4)
-	bdnz	00b
-	twi	0,r5,0
-	isync
-	blr
-
-_GLOBAL(_outsw_ns)
-	cmpwi	0,r5,0
-	mtctr	r5
-	subi	r4,r4,2
-	blelr-
-	sync
-00:	lhzu	r5,2(r4)
-	sth	r5,0(r3)
-	bdnz	00b
-	sync
-	blr
-
-_GLOBAL(_insl_ns)
-	sync
-	cmpwi	0,r5,0
-	mtctr	r5
-	subi	r4,r4,4
-	blelr-
-00:	lwz	r5,0(r3)
-	eieio
-	stwu	r5,4(r4)
-	bdnz	00b
-	twi	0,r5,0
-	isync
-	blr
-
-_GLOBAL(_outsl_ns)
-	cmpwi	0,r5,0
-	mtctr	r5
-	subi	r4,r4,4
-	blelr-
-	sync
-00:	lwzu	r5,4(r4)
-	stw	r5,0(r3)
-	bdnz	00b
-	sync
-	blr
-
diff --git a/arch/powerpc/kernel/ppc_ksyms.c b/arch/powerpc/kernel/ppc_ksyms.c
index 75429e5..807193a 100644
--- a/arch/powerpc/kernel/ppc_ksyms.c
+++ b/arch/powerpc/kernel/ppc_ksyms.c
@@ -95,13 +95,6 @@ #ifdef CONFIG_PPC64
 EXPORT_SYMBOL(copy_4K_page);
 #endif
 
-EXPORT_SYMBOL(_insb);
-EXPORT_SYMBOL(_outsb);
-EXPORT_SYMBOL(_insw_ns);
-EXPORT_SYMBOL(_outsw_ns);
-EXPORT_SYMBOL(_insl_ns);
-EXPORT_SYMBOL(_outsl_ns);
-
 #if defined(CONFIG_PPC32) && (defined(CONFIG_BLK_DEV_IDE) || defined(CONFIG_BLK_DEV_IDE_MODULE))
 EXPORT_SYMBOL(ppc_ide_md);
 #endif
-- 
1.4.2.1

Re: [POWERPC] convert string i/o operations to C

[David Howells]

Stephen Rothwell <sfr@canb.auug.org.au> wrote:

> +void _insb(volatile u8 __iomem *port, void *buf, int ns)
> +{
> +	asm volatile("sync");
> +	if (ns <= 0)
> +		return;

What is "ns" meant to do?  It seems to degrade the _insb() op to just a sync,
but is that correct?

David

Re: [POWERPC] convert string i/o operations to C

[Linas Vepstas]

On Tue, Sep 19, 2006 at 10:23:51PM +1000, Stephen Rothwell wrote:

Perhaps this:

> +void _insb(volatile u8 __iomem *port, void *buf, int ns)
> +{
> +	asm volatile("sync");
> +	if (ns <= 0)
> +		return;
> +	asm volatile(

should be this: 

> +void _insb(volatile u8 __iomem *port, void *buf, int ns)
> +{
> +	if (ns <= 0)
> +		return;
> +	asm volatile("sync");
> +	asm volatile(

to assuage David Howell's concern.  Not that ns should be negative
in the first place ... but what the hey.

Re Davids other comment: "ns" stands for "no  byte-swap", and 
"s" stands for "string" so "insl_ns" is input string of longs, 
no byte swap.

Here's a question:

> +	asm volatile(
> +		"mtctr	%2\n"
> +		"subi	%1,%1,1\n"
> +		"0:	lbz	%2,0(%0)\n"
> +		"eieio\n"
> +		"stbu	%2,1(%1)\n"
> +		"bdnz	0b\n"
> +		"twi	0,%2,0\n"

What does this twi do? According to my powerpc docs, this would be a
no-op.  Does this have some magic synchronizing powers on certain
implementations? If so, there should be at least a comment card added
about why the twi is there. (This special ability of twi might be
well-known to some, but still, this is not immediately obvious,
and not immedately documented in e.g. the PEM.)

--linas

Re: [POWERPC] convert string i/o operations to C

[Kim Phillips]

On Tue, 19 Sep 2006 13:29:53 -0500
linas@austin.ibm.com (Linas Vepstas) wrote:

> 
> What does this twi do? According to my powerpc docs, this would be a
> no-op.  Does this have some magic synchronizing powers on certain
> implementations? If so, there should be at least a comment card added
> about why the twi is there. (This special ability of twi might be
> well-known to some, but still, this is not immediately obvious,
> and not immedately documented in e.g. the PEM.)

include/asm-p[ower]pc/io.h sheds some light on the matter:

 * With the sequence below (twi; isync; nop), we have found that
 * the machine check occurs on one of the three instructions on
 * all PPC implementations tested so far.  The twi and isync are
 * needed on the 601 (in fact twi; sync works too), the isync and
 * nop are needed on 604[e|r], and any of twi, sync or isync will
 * work on 603[e], 750, 74xx.
 * The twi creates an explicit data dependency on the returned
 * value which seems to be needed to make the 601 wait for the
 * load to finish.

Kim

Re: [POWERPC] convert string i/o operations to C

[Matt Sealey]

Shouldn't this stuff be optimized out depending on what processor you're 
ACTUALLY running?

For a generic "powerpc" kernel it can be understood, but when you 
consider that on 970/POWER4 and above they use lwsync instead of sync
(google for them and see the mailing list posts :), just to breathe back 
some performance in spinlocks and so on, surely this can be rejigged so 
that processors don't do more work than necessary..? Even a noop takes 
time doesn't it?

-- 
Matt Sealey <matt@genesi-usa.com>
Genesi, Manager, Developer Relations

Kim Phillips wrote:
> On Tue, 19 Sep 2006 13:29:53 -0500
> linas@austin.ibm.com (Linas Vepstas) wrote:
> 
>> What does this twi do? According to my powerpc docs, this would be a
>> no-op.  Does this have some magic synchronizing powers on certain
>> implementations? If so, there should be at least a comment card added
>> about why the twi is there. (This special ability of twi might be
>> well-known to some, but still, this is not immediately obvious,
>> and not immedately documented in e.g. the PEM.)
> 
> include/asm-p[ower]pc/io.h sheds some light on the matter:
> 
>  * With the sequence below (twi; isync; nop), we have found that
>  * the machine check occurs on one of the three instructions on
>  * all PPC implementations tested so far.  The twi and isync are
>  * needed on the 601 (in fact twi; sync works too), the isync and
>  * nop are needed on 604[e|r], and any of twi, sync or isync will
>  * work on 603[e], 750, 74xx.
>  * The twi creates an explicit data dependency on the returned
>  * value which seems to be needed to make the 601 wait for the
>  * load to finish.
> 
> Kim
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [POWERPC] convert string i/o operations to C

[Linas Vepstas]

On Tue, Sep 19, 2006 at 01:52:59PM -0500, Kim Phillips wrote:
> On Tue, 19 Sep 2006 13:29:53 -0500
> linas@austin.ibm.com (Linas Vepstas) wrote:
> 
> > 
> > What does this twi do? According to my powerpc docs, this would be a
> > no-op.  Does this have some magic synchronizing powers on certain
> > implementations? If so, there should be at least a comment card added
> > about why the twi is there. (This special ability of twi might be
> > well-known to some, but still, this is not immediately obvious,
> > and not immedately documented in e.g. the PEM.)
> 
> include/asm-p[ower]pc/io.h sheds some light on the matter:
> 
>  * With the sequence below (twi; isync; nop), we have found that
>  * the machine check occurs on one of the three instructions on
>  * all PPC implementations tested so far.  The twi and isync are
>  * needed on the 601 (in fact twi; sync works too), the isync and
>  * nop are needed on 604[e|r], and any of twi, sync or isync will
>  * work on 603[e], 750, 74xx.
>  * The twi creates an explicit data dependency on the returned
>  * value which seems to be needed to make the 601 wait for the
>  * load to finish.


Thanks.  I notice that this commentary appears in asm-ppc/io.h
but not in asm-powerpc/io.h -- Perhaps a patch is in order ...
I'm creating such a patch now ...

--linas

Re: [POWERPC] convert string i/o operations to C

[Segher Boessenkool]

> What does this twi do? According to my powerpc docs, this would be a
> no-op.  Does this have some magic synchronizing powers on certain
> implementations?

On all implementations; it's in the PowerPC architecture.

A data-dependent branch followed by an isync ensures that no
instructions after the isync in program order will be
(speculatively) executed, so the load that the twi depends
on has to complete before anything else is executed; in
particular, it's a barrier to keep MMIO reads ordered before
main-storage accesses.

Note that twi alone is not enough; it needs an isync as well.
Any other data-dependent branch instead of twi would do fine
as well.


Segher

Re: [POWERPC] convert string i/o operations to C

[Segher Boessenkool]

> For a generic "powerpc" kernel it can be understood, but when you
> consider that on 970/POWER4 and above they use lwsync instead of sync

lwsync (sync 1) doesn't (architecturally) do anything to order MMIO
accesses, so you're misunderstanding something.

> (google for them and see the mailing list posts :), just to breathe  
> back
> some performance in spinlocks and so on, surely this can be  
> rejigged so
> that processors don't do more work than necessary..? Even a noop takes
> time doesn't it?

No-ops don't matter for performance, compared to the cost of the I/O
itself.  It would help to avoid heavier-than-necessary synchronisation
instructions if not needed on some certain CPU (or on non-SMP kernels,
etc.)


Segher

Re: [POWERPC] convert string i/o operations to C

[Matt Sealey]

Segher Boessenkool wrote:
>> For a generic "powerpc" kernel it can be understood, but when you
>> consider that on 970/POWER4 and above they use lwsync instead of sync
> 
> lwsync (sync 1) doesn't (architecturally) do anything to order MMIO
> accesses, so you're misunderstanding something.

See below :D

>> (google for them and see the mailing list posts :), just to breathe back
>> some performance in spinlocks and so on, surely this can be rejigged so
>> that processors don't do more work than necessary..? Even a noop takes
>> time doesn't it?
> 
> No-ops don't matter for performance, compared to the cost of the I/O
> itself.  It would help to avoid heavier-than-necessary synchronisation
> instructions if not needed on some certain CPU (or on non-SMP kernels,
> etc.)

But it couldn't hurt, right? There has to be an application note per-CPU 
on the correct sequence of operations for such an access (I seem to have 
collected a directory full for firmware development), it seems a little 
odd to pick and choose one instruction over another for one thing, and 
then say you need to do it to support the 601 of all things, and run 
this code against the G3/G4/G5 which perhaps doesn't care or is more 
intelligent about it (or is guaranteed to have a more intelligent host 
bridge at least).

Maybe I'm talking crap, please say so :D

-- 
Matt Sealey <matt@genesi-usa.com>
Genesi, Manager, Developer Relations

Re: [POWERPC] convert string i/o operations to C

[Segher Boessenkool]

> But it couldn't hurt, right? There has to be an application note  
> per-CPU on the correct sequence of operations for such an access (I  
> seem to have collected a directory full for firmware development),

The architecture describes the rules already, not many CPUs
have "faster"/"better" sequences.

> it seems a little odd to pick and choose one instruction over  
> another for one thing, and then say you need to do it to support  
> the 601 of all things, and run this code against the G3/G4/G5 which  
> perhaps doesn't care or is more intelligent about it (or is  
> guaranteed to have a more intelligent host bridge at least).

The comment you're referring to is old; it doesn't talk about
synchronisation requirements, but focuses on having the CPU
trap on exactly these instructions when an access causes a
(asynchronous) machine check.  Asynchronous exceptions don't
necessarily return the instruction pointer where the real
failure was, so it's no surprise different CPUs have a different
idea about it.  It's pretty safe to assume (but not guaranteed)
that it will always be somewhere between the load and the insn
after the isync, inclusive, though.


Segher

Re: [POWERPC] convert string i/o operations to C

[Stephen Rothwell]

On Tue, 19 Sep 2006 13:42:58 +0100 David Howells <dhowells@redhat.com> wrote:
>
> Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> 
> > +void _insb(volatile u8 __iomem *port, void *buf, int ns)
> > +{
> > +	asm volatile("sync");
> > +	if (ns <= 0)
> > +		return;
> 
> What is "ns" meant to do?  It seems to degrade the _insb() op to just a sync,
> but is that correct?

"ns" is the count (maybe I should rename the parameter, but that is what
was in the prototype in io.h).  The "sync" is in the same place as it was
in the assembler code that this replaces.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

Re: [POWERPC] convert string i/o operations to C

[Benjamin Herrenschmidt]

On Tue, 2006-09-19 at 13:29 -0500, Linas Vepstas wrote:
> On Tue, Sep 19, 2006 at 10:23:51PM +1000, Stephen Rothwell wrote:
> 
> Perhaps this:
> 
> > +void _insb(volatile u8 __iomem *port, void *buf, int ns)
> > +{
> > +	asm volatile("sync");
> > +	if (ns <= 0)
> > +		return;
> > +	asm volatile(
> 
> should be this: 
> 
> > +void _insb(volatile u8 __iomem *port, void *buf, int ns)
> > +{
> > +	if (ns <= 0)
> > +		return;
> > +	asm volatile("sync");
> > +	asm volatile(

No. The barrier should be there even if somebody is lame enough to give
a count of 0.

> to assuage David Howell's concern.  Not that ns should be negative
> in the first place ... but what the hey.
> 
> Re Davids other comment: "ns" stands for "no  byte-swap", and 
> "s" stands for "string" so "insl_ns" is input string of longs, 
> no byte swap.

David's comment was about the "ns" argument which is the count (dunno
why "ns").

> Here's a question:
> 
> > +	asm volatile(
> > +		"mtctr	%2\n"
> > +		"subi	%1,%1,1\n"
> > +		"0:	lbz	%2,0(%0)\n"
> > +		"eieio\n"
> > +		"stbu	%2,1(%1)\n"
> > +		"bdnz	0b\n"
> > +		"twi	0,%2,0\n"
> 
> What does this twi do? According to my powerpc docs, this would be a
> no-op.  Does this have some magic synchronizing powers on certain
> implementations? If so, there should be at least a comment card added
> about why the twi is there. (This special ability of twi might be
> well-known to some, but still, this is not immediately obvious,
> and not immedately documented in e.g. the PEM.)
> 
> --linas
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [POWERPC] convert string i/o operations to C

[Benjamin Herrenschmidt]

On Tue, 2006-09-19 at 20:58 +0200, Matt Sealey wrote:
> Shouldn't this stuff be optimized out depending on what processor you're 
> ACTUALLY running?
> 
> For a generic "powerpc" kernel it can be understood, but when you 
> consider that on 970/POWER4 and above they use lwsync instead of sync
> (google for them and see the mailing list posts :), just to breathe back 
> some performance in spinlocks and so on, surely this can be rejigged so 
> that processors don't do more work than necessary..? Even a noop takes 
> time doesn't it?

The architecture is precise enough there. It should be twi,isync. The
goal of the isync is to make sure no subsequent instruction can execute
until the previous conditional branch has been fully resolved, which
implies loading the dependent data. An isync is the only instruction in
this specific case that is guaranteed to provide what we need by the
architecture.

Ben

Re: [POWERPC] convert string i/o operations to C

[Benjamin Herrenschmidt]

> But it couldn't hurt, right? There has to be an application note per-CPU 
> on the correct sequence of operations for such an access (I seem to have 
> collected a directory full for firmware development), it seems a little 
> odd to pick and choose one instruction over another for one thing, and 
> then say you need to do it to support the 601 of all things, and run 
> this code against the G3/G4/G5 which perhaps doesn't care or is more 
> intelligent about it (or is guaranteed to have a more intelligent host 
> bridge at least).
> 
> Maybe I'm talking crap, please say so :D

No, your basic idea is valid. One of the thing we are tackling doing is
to add to our cpu feature "nop'ing out" mecanism something closer to
what x86 does nowadays which is to have alternate sequence of
instructions depending on the CPU.

I just think however that the MMIO loads aren't a very high priority
spot for such an optimisation :) Spinlocks are more interesting and we
are looking into doing something like x86 where you don't need to build
a UP kernel, the SMP kernel will automatically replace the spinlocks
with simple preempt_disable/enable at boot if running on only one CPU.
We could do similar things with some barriers etc...

Ben.

Re: [POWERPC] convert string i/o operations to C

[Segher Boessenkool]

>> Perhaps this:
>>
>>> +void _insb(volatile u8 __iomem *port, void *buf, int ns)
>>> +{
>>> +	asm volatile("sync");
>>> +	if (ns <= 0)
>>> +		return;
>>> +	asm volatile(
>>
>> should be this:
>>
>>> +void _insb(volatile u8 __iomem *port, void *buf, int ns)
>>> +{
>>> +	if (ns <= 0)
>>> +		return;
>>> +	asm volatile("sync");
>>> +	asm volatile(
>
> No. The barrier should be there even if somebody is lame enough to  
> give
> a count of 0.

Why?  If "ns" <= 0 there is no I/O done...


Segher

Re: [POWERPC] convert string i/o operations to C

[Benjamin Herrenschmidt]

> Why?  If "ns" <= 0 there is no I/O done...

For the sake of being paranoid :) What happens on x86 if you do a rep ;
insw with a 0 count ?

Ben.

Re: [POWERPC] convert string i/o operations to C

[Segher Boessenkool]

>> Why?  If "ns" <= 0 there is no I/O done...
>
> For the sake of being paranoid :) What happens on x86 if you do a  
> rep ;
> insw with a 0 count ?

Nothing, except wasting energy.


Segher

[POWERPC] convert string i/o operations to C

[Stephen Rothwell]

This produces essentially the same code and will make the iSeries i/o
consolidation easier.

The count parameter is changed to long since that will produce the same
(better) code on 32 and 64 bit builds.

Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
 arch/powerpc/kernel/Makefile    |    2 -
 arch/powerpc/kernel/io.c        |  117 +++++++++++++++++++++++++++++++++++++++
 arch/powerpc/kernel/misc.S      |   95 --------------------------------
 arch/powerpc/kernel/ppc_ksyms.c |    7 --
 include/asm-powerpc/io.h        |   12 ++--
 include/asm-ppc/io.h            |   12 ++--
 6 files changed, 130 insertions(+), 115 deletions(-)

This version has lots more C code.  Paulus also said that the "sync"s
could go after the check for count <= 0.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index 8b3f4fa..8b133af 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -51,7 +51,7 @@ extra-$(CONFIG_8xx)		:= head_8xx.o
 extra-y				+= vmlinux.lds
 
 obj-y				+= time.o prom.o traps.o setup-common.o \
-				   udbg.o misc.o
+				   udbg.o misc.o io.o
 obj-$(CONFIG_PPC32)		+= entry_32.o setup_32.o misc_32.o
 obj-$(CONFIG_PPC64)		+= misc_64.o dma_64.o iommu.o
 obj-$(CONFIG_PPC_MULTIPLATFORM)	+= prom_init.o
diff --git a/arch/powerpc/kernel/io.c b/arch/powerpc/kernel/io.c
new file mode 100644
index 0000000..80a3209
--- /dev/null
+++ b/arch/powerpc/kernel/io.c
@@ -0,0 +1,117 @@
+/*
+ * I/O string operations
+ *    Copyright (C) 1995-1996 Gary Thomas (gdt@linuxppc.org)
+ *    Copyright (C) 2006 IBM Corporation
+ *
+ * Largely rewritten by Cort Dougan (cort@cs.nmt.edu)
+ * and Paul Mackerras.
+ *
+ * Adapted for iSeries by Mike Corrigan (mikejc@us.ibm.com)
+ * PPC64 updates by Dave Engebretsen (engebret@us.ibm.com)
+ *
+ * Rewritten in C by Stephen Rothwell.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include <linux/compiler.h>
+#include <linux/module.h>
+
+#include <asm/io.h>
+
+void _insb(volatile u8 __iomem *port, void *buf, long count)
+{
+	u8 *tbuf = buf;
+	u8 tmp;
+
+	if (unlikely(count <= 0))
+		return;
+	asm volatile("sync");
+	do {
+		tmp = *port;
+		asm volatile("eieio");
+		*tbuf++ = tmp;
+	} while (--count != 0);
+	asm volatile("twi 0,%0,0; isync" : : "r" (tmp));
+}
+EXPORT_SYMBOL(_insb);
+
+void _outsb(volatile u8 __iomem *port, const void *buf, long count)
+{
+	const u8 *tbuf = buf;
+
+	if (unlikely(count <= 0))
+		return;
+	asm volatile("sync");
+	do {
+		*port = *tbuf++;
+	} while (--count != 0);
+	asm volatile("sync");
+}
+EXPORT_SYMBOL(_outsb);
+
+void _insw_ns(volatile u16 __iomem *port, void *buf, long count)
+{
+	u16 *tbuf = buf;
+	u16 tmp;
+
+	if (unlikely(count <= 0))
+		return;
+	asm volatile("sync");
+	do {
+		tmp = *port;
+		asm volatile("eieio");
+		*tbuf++ = tmp;
+	} while (--count != 0);
+	asm volatile("twi 0,%0,0; isync" : : "r" (tmp));
+}
+EXPORT_SYMBOL(_insw_ns);
+
+void _outsw_ns(volatile u16 __iomem *port, const void *buf, long count)
+{
+	const u16 *tbuf = buf;
+
+	if (unlikely(count <= 0))
+		return;
+	asm volatile("sync");
+	do {
+		*port = *tbuf++;
+	} while (--count != 0);
+	asm volatile("sync");
+}
+EXPORT_SYMBOL(_outsw_ns);
+
+void _insl_ns(volatile u32 __iomem *port, void *buf, long count)
+{
+	u32 *tbuf = buf;
+	u32 tmp;
+
+	if (unlikely(count <= 0))
+		return;
+	asm volatile("sync");
+	do {
+		tmp = *port;
+		asm volatile("eieio");
+		*tbuf++ = tmp;
+	} while (--count != 0);
+	asm volatile("twi 0,%0,0; isync" : : "r" (tmp));
+}
+EXPORT_SYMBOL(_insl_ns);
+
+void _outsl_ns(volatile u32 __iomem *port, const void *buf, long count)
+{
+	const u32 *tbuf = buf;
+
+	if (unlikely(count <= 0))
+		return;
+	asm volatile("sync");
+	do {
+		*port = *tbuf++;
+	} while (--count != 0);
+	asm volatile("sync");
+}
+EXPORT_SYMBOL(_outsl_ns);
diff --git a/arch/powerpc/kernel/misc.S b/arch/powerpc/kernel/misc.S
index 6feb391..330c9dc 100644
--- a/arch/powerpc/kernel/misc.S
+++ b/arch/powerpc/kernel/misc.S
@@ -43,98 +43,3 @@ _GLOBAL(add_reloc_offset)
 	add	r3,r3,r5
 	mtlr	r0
 	blr
-
-/*
- * I/O string operations
- *
- * insb(port, buf, len)
- * outsb(port, buf, len)
- * insw(port, buf, len)
- * outsw(port, buf, len)
- * insl(port, buf, len)
- * outsl(port, buf, len)
- * insw_ns(port, buf, len)
- * outsw_ns(port, buf, len)
- * insl_ns(port, buf, len)
- * outsl_ns(port, buf, len)
- *
- * The *_ns versions don't do byte-swapping.
- */
-_GLOBAL(_insb)
-	sync
-	cmpwi	0,r5,0
-	mtctr	r5
-	subi	r4,r4,1
-	blelr-
-00:	lbz	r5,0(r3)
-	eieio
-	stbu	r5,1(r4)
-	bdnz	00b
-	twi	0,r5,0
-	isync
-	blr
-
-_GLOBAL(_outsb)
-	cmpwi	0,r5,0
-	mtctr	r5
-	subi	r4,r4,1
-	blelr-
-	sync
-00:	lbzu	r5,1(r4)
-	stb	r5,0(r3)
-	bdnz	00b
-	sync
-	blr
-
-_GLOBAL(_insw_ns)
-	sync
-	cmpwi	0,r5,0
-	mtctr	r5
-	subi	r4,r4,2
-	blelr-
-00:	lhz	r5,0(r3)
-	eieio
-	sthu	r5,2(r4)
-	bdnz	00b
-	twi	0,r5,0
-	isync
-	blr
-
-_GLOBAL(_outsw_ns)
-	cmpwi	0,r5,0
-	mtctr	r5
-	subi	r4,r4,2
-	blelr-
-	sync
-00:	lhzu	r5,2(r4)
-	sth	r5,0(r3)
-	bdnz	00b
-	sync
-	blr
-
-_GLOBAL(_insl_ns)
-	sync
-	cmpwi	0,r5,0
-	mtctr	r5
-	subi	r4,r4,4
-	blelr-
-00:	lwz	r5,0(r3)
-	eieio
-	stwu	r5,4(r4)
-	bdnz	00b
-	twi	0,r5,0
-	isync
-	blr
-
-_GLOBAL(_outsl_ns)
-	cmpwi	0,r5,0
-	mtctr	r5
-	subi	r4,r4,4
-	blelr-
-	sync
-00:	lwzu	r5,4(r4)
-	stw	r5,0(r3)
-	bdnz	00b
-	sync
-	blr
-
diff --git a/arch/powerpc/kernel/ppc_ksyms.c b/arch/powerpc/kernel/ppc_ksyms.c
index 75429e5..807193a 100644
--- a/arch/powerpc/kernel/ppc_ksyms.c
+++ b/arch/powerpc/kernel/ppc_ksyms.c
@@ -95,13 +95,6 @@ #ifdef CONFIG_PPC64
 EXPORT_SYMBOL(copy_4K_page);
 #endif
 
-EXPORT_SYMBOL(_insb);
-EXPORT_SYMBOL(_outsb);
-EXPORT_SYMBOL(_insw_ns);
-EXPORT_SYMBOL(_outsw_ns);
-EXPORT_SYMBOL(_insl_ns);
-EXPORT_SYMBOL(_outsl_ns);
-
 #if defined(CONFIG_PPC32) && (defined(CONFIG_BLK_DEV_IDE) || defined(CONFIG_BLK_DEV_IDE_MODULE))
 EXPORT_SYMBOL(ppc_ide_md);
 #endif
diff --git a/include/asm-powerpc/io.h b/include/asm-powerpc/io.h
index 51a5987..57e7d14 100644
--- a/include/asm-powerpc/io.h
+++ b/include/asm-powerpc/io.h
@@ -143,12 +143,12 @@ #define readw_relaxed(addr) readw(addr)
 #define readl_relaxed(addr) readl(addr)
 #define readq_relaxed(addr) readq(addr)
 
-extern void _insb(volatile u8 __iomem *port, void *buf, int ns);
-extern void _outsb(volatile u8 __iomem *port, const void *buf, int ns);
-extern void _insw_ns(volatile u16 __iomem *port, void *buf, int ns);
-extern void _outsw_ns(volatile u16 __iomem *port, const void *buf, int ns);
-extern void _insl_ns(volatile u32 __iomem *port, void *buf, int nl);
-extern void _outsl_ns(volatile u32 __iomem *port, const void *buf, int nl);
+extern void _insb(volatile u8 __iomem *port, void *buf, long count);
+extern void _outsb(volatile u8 __iomem *port, const void *buf, long count);
+extern void _insw_ns(volatile u16 __iomem *port, void *buf, long count);
+extern void _outsw_ns(volatile u16 __iomem *port, const void *buf, long count);
+extern void _insl_ns(volatile u32 __iomem *port, void *buf, long count);
+extern void _outsl_ns(volatile u32 __iomem *port, const void *buf, long count);
 
 static inline void mmiowb(void)
 {
diff --git a/include/asm-ppc/io.h b/include/asm-ppc/io.h
index 9fac420..3d9a9e6 100644
--- a/include/asm-ppc/io.h
+++ b/include/asm-ppc/io.h
@@ -327,12 +327,12 @@ #define outw_p(val, port)	outw((val), (p
 #define inl_p(port)		inl((port))
 #define outl_p(val, port)	outl((val), (port))
 
-extern void _insb(volatile u8 __iomem *port, void *buf, int ns);
-extern void _outsb(volatile u8 __iomem *port, const void *buf, int ns);
-extern void _insw_ns(volatile u16 __iomem *port, void *buf, int ns);
-extern void _outsw_ns(volatile u16 __iomem *port, const void *buf, int ns);
-extern void _insl_ns(volatile u32 __iomem *port, void *buf, int nl);
-extern void _outsl_ns(volatile u32 __iomem *port, const void *buf, int nl);
+extern void _insb(volatile u8 __iomem *port, void *buf, long count);
+extern void _outsb(volatile u8 __iomem *port, const void *buf, long count);
+extern void _insw_ns(volatile u16 __iomem *port, void *buf, long count);
+extern void _outsw_ns(volatile u16 __iomem *port, const void *buf, long count);
+extern void _insl_ns(volatile u32 __iomem *port, void *buf, long count);
+extern void _outsl_ns(volatile u32 __iomem *port, const void *buf, long count);
 
 
 #define IO_SPACE_LIMIT ~0
-- 
1.4.2.1

Re: [POWERPC] convert string i/o operations to C

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 851 bytes --]

On Wed, 2006-09-20 at 13:35 +1000, Stephen Rothwell wrote:
> This produces essentially the same code and will make the iSeries i/o
> consolidation easier.

A bit more white space would be nice :D, eg:

> +void _insb(volatile u8 __iomem *port, void *buf, long count)
> +{
> +	u8 *tbuf = buf;
> +	u8 tmp;
> +
> +	if (unlikely(count <= 0))
> +		return;

> +	asm volatile("sync");

> +	do {
> +		tmp = *port;
> +		asm volatile("eieio");
> +		*tbuf++ = tmp;
> +	} while (--count != 0);

> +	asm volatile("twi 0,%0,0; isync" : : "r" (tmp));
> +}
> +EXPORT_SYMBOL(_insb);

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 191 bytes --]