Skip to contentSkip to conversation

POST: Added ECC memory test for mpc83xx.

PATCH New Michael Zaidman 5 participants 23 messages Apr 2, 2010 – Apr 20, 2010

lore
Share & tools
lore

Michael Zaidman

author
#0 lore raw

PATCH 1/1 POST: Added ECC memory test for mpc83xx.

---
 post/cpu/mpc83xx/Makefile |   30 +++++++++
 post/cpu/mpc83xx/ecc.c    |  160 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 190 insertions(+), 0 deletions(-)
 create mode 100644 post/cpu/mpc83xx/Makefile
 create mode 100644 post/cpu/mpc83xx/ecc.c
post/cpu/mpc83xx/Makefile new file +30 -0
new file mode 100644
index 0000000..86d8784
@@ -0,0 +1,30 @@
+#
+# (C) Copyright 2002-2007
+# Wolfgang Denk, DENX Software Engineering, wd at denx.de.
+#
+# See file CREDITS for list of people who contributed to this
+# project.
+#
+# 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.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+# MA 02111-1307 USA
+#
+include $(OBJTREE)/include/autoconf.mk
+
+LIB	= libpostmpc83xx.a
+
+AOBJS-$(CONFIG_HAS_POST)	+=
+COBJS-$(CONFIG_HAS_POST)	+= ecc.o
+
+include $(TOPDIR)/post/rules.mk
post/cpu/mpc83xx/ecc.c new file +160 -0

160 lines not on this page · raw message

Signature · 1 line
1.6.3.3
#1 lore raw

PATCH 1/1 POST: Added ECC memory test for mpc83xx.

Quoted 1 line · Michael Zaidman

+inline static void ecc_clear(volatile ddr83xx_t *ddr)

Please use I/O accessors instead of "volatile".

Signature · 2 lines
Timur Tabi
Linux kernel developer at Freescale
author
#2 lore raw

PATCH 1/1 POST: Added ECC memory test for mpc83xx.

Quoted 11 lines · Timur Tabi
Quoted 1 line · Michael Zaidman

+inline static void ecc_clear(volatile ddr83xx_t *ddr)

Please use I/O accessors instead of "volatile".

--
Timur Tabi
Linux kernel developer at Freescale

I do not question the rationale behind the requirement to use the I/O accessors. My question is - if there are any exceptions to the rule? Just for curiosity I compared the assembly code generated by both versions of ecc_clear routine - original and using I/O accessors. The original is of 0x3C size while "accessors" version has 0x74 size. Rewriting code to use accessors API added 56 bytes or 14 opcodes. It doubled the routine's execution time. The routine is called in the loop per each ecc word, that significantly increased execution time of the ECC POST test and consequently board startup time. -michael

#3 lore raw

PATCH 1/1 POST: Added ECC memory test for mpc83xx.

Quoted 2 lines · Michael Zaidman

I do not question the rationale behind the requirement to use the I/O accessors. My question is - if there are any exceptions to the rule?

Not really. The problem is that volatile does not take the PowerPC execution ordering into account. So even if you use volatile, the read write might not actually be performed when you think it should be performed.

Quoted 3 lines

Just for curiosity I compared the assembly code generated by both versions of ecc_clear routine ?- original and using I/O accessors. The original is of 0x3C size while "accessors" version has 0x74 size.

That's not surprising. The I/O accessors are not very efficient, since they sync after ever write.

Quoted 4 lines

Rewriting code to use accessors API added 56 bytes or 14 opcodes. It doubled the routine's execution time. The routine is called in the loop per each ecc word, that significantly increased execution time of the ECC POST test and consequently board startup time.

If this is really a problem, you can modify the code to perform the writes without the I/O accessors, and then terminate the block with a sync. You should add a comment explaining why you're not using I/O accessors. At this point, I'm not 100% clear whether the use of volatile is okay.

Signature · 2 lines
Timur Tabi
Linux kernel developer at Freescale
author
#4 lore raw

PATCH 1/1 POST: Added ECC memory test for mpc83xx.

Quoted 8 lines
Quoted 4 lines

Rewriting code to use accessors API added 56 bytes or 14 opcodes. It doubled the routine's execution time. The routine is called in the loop per each ecc word, that significantly increased execution time of the ECC POST test and consequently board startup time.

If this is really a problem, you can modify the code to perform the writes without the I/O accessors, and then terminate the block with a sync.

I thought that is what I actually do in this patch...

Quoted 2 lines

You should add a comment explaining why you're not using I/O accessors.

Ok, I will add the explanation.

Thanks for your comments.

-michael

#5 lore raw

PATCH 1/1 POST: Added ECC memory test for mpc83xx.

Quoted 1 line · Michael Zaidman

I thought that is what I actually do in this patch...

Not quite. You're missing the memory barrier that the sync() function includes. You're doing this:

      __asm__ __volatile__("sync");

But the sync() macros does this:

static inline void sync(void)

{
	__asm__ __volatile__ ("sync" : : : "memory");
}

The "memory" tells the compiler that this function is a memory barrier, which means it won't try to reorder any reads/writes *after* the "sync".

Signature · 2 lines
Timur Tabi
Linux kernel developer at Freescale
#6 lore raw

PATCH 1/1 POST: Added ECC memory test for mpc83xx.

In reply to Michael Zaidman
Quoted 9 lines · On Tue, 6 Apr 2010 18:29:08 +0300 Michael Zaidman <[email protected]>…
Quoted 8 lines
Quoted 4 lines

Rewriting code to use accessors API added 56 bytes or 14 opcodes. It doubled the routine's execution time. The routine is called in the loop per each ecc word, that significantly increased execution time of the ECC POST test and consequently board startup time.

If this is really a problem, you can modify the code to perform the writes without the I/O accessors, and then terminate the block with a sync.

Quoted 6 lines

I thought that is what I actually do in this patch...

Quoted 2 lines

You should add a comment explaining why you're not using I/O accessors.

Ok, I will add the explanation.

use raw i/o accessors please.

oh, and don't forget to sign off your work

Thanks,

Kim

#7 lore raw

PATCH 1/1 POST: Added ECC memory test for mpc83xx.

Quoted 6 lines · Kim Phillips
Quoted 4 lines
Quoted 2 lines

You should add a comment explaining why you're not using I/O accessors.

Ok, I will add the explanation.

use raw i/o accessors please.

But what about his performance concern? Shouldn't it be okay for him to do direct writes and then follow-up with a sync(), as long as he comments it?

Signature · 2 lines
Timur Tabi
Linux kernel developer at Freescale
#8 lore raw

PATCH 1/1 POST: Added ECC memory test for mpc83xx.

Quoted 5 lines · Timur Tabi
Quoted 3 lines · Kim Phillips
Quoted 3 lines
Quoted 2 lines

You should add a comment explaining why you're not using I/O accessors.

Quoted 7 lines
Quoted 3 lines
Quoted 1 line

Ok, I will add the explanation.

use raw i/o accessors please.

But what about his performance concern? Shouldn't it be okay for him to do direct writes and then follow-up with a sync(), as long as he comments it?

That's exactly what raw accessors are for. They don't include synchronization (and unfortunately this somehow gets tied up in whether endian conversions are done).

-Scott

author
#9 lore raw

PATCH 1/1 POST: Added ECC memory test for mpc83xx.

In reply to Kim Phillips
Quoted 19 lines · Kim Phillips
Quoted 9 lines · On Tue, 6 Apr 2010 18:29:08 +0300 Michael Zaidman <[email protected]>…
Quoted 8 lines
Quoted 4 lines

Rewriting code to use accessors API added 56 bytes or 14 opcodes. It doubled the routine's execution time. The routine is called in the loop per each ecc word, that significantly increased execution time of the ECC POST test and consequently board startup time.

If this is really a problem, you can modify the code to perform the writes without the I/O accessors, and then terminate the block with a sync.

Quoted 6 lines

I thought that is what I actually do in this patch...

Quoted 2 lines

You should add a comment explaining why you're not using I/O accessors.

Ok, I will add the explanation.

Quoted 8 lines

use raw i/o accessors please.

oh, and don't forget to sign off your work

Thanks,

Kim

Thanks for your feedback.

The version #2 of the patch is located here
http://lists.denx.de/pipermail/u-boot/2010-April/069579.html

-michael

#10 lore raw

PATCH 1/1 POST: Added ECC memory test for mpc83xx.

Quoted 2 lines · Michael Zaidman

The version #2 of the patch is located here
http://lists.denx.de/pipermail/u-boot/2010-April/069579.html

What is ppcDWstore(), and why do you use it sometimes instead of __raw_writel()? I see the code for ppcDWstore:

.globl ppcDWstore
ppcDWstore:

	lfd	1, 0(r4)
	stfd	1, 0(r3)
	blr

I just don't understand why it exists.

Signature · 2 lines
Timur Tabi
Linux kernel developer at Freescale
author
#11 lore raw

PATCH 1/1 POST: Added ECC memory test for mpc83xx.

Quoted 9 lines · Timur Tabi
Quoted 2 lines · Michael Zaidman

The version #2 of the patch is located here
http://lists.denx.de/pipermail/u-boot/2010-April/069579.html

What is ppcDWstore(), and why do you use it sometimes instead of __raw_writel()? ?I see the code for ppcDWstore:

Quoted 8 lines

? ? ? ?.globl ?ppcDWstore
ppcDWstore:
? ? ? ?lfd ? ? 1, 0(r4)
? ? ? ?stfd ? ?1, 0(r3)
? ? ? ?blr

I just don't understand why it exists.

This is the fastest way to copy 64 bit words.

-michael

#12 lore raw

PATCH 1/1 POST: Added ECC memory test for mpc83xx.

Quoted 1 line · Michael Zaidman

This is the fastest way to copy 64 bit words.

Of course, DW==double word. I should drink more coffee.

Signature · 2 lines
Timur Tabi
Linux kernel developer at Freescale
#13 lore raw

PATCH 1/1 POST: Added ECC memory test for mpc83xx.

In reply to Michael Zaidman
Quoted 17 lines · Timur Tabi
Quoted 15 lines
Quoted 2 lines · Michael Zaidman

The version #2 of the patch is located here
http://lists.denx.de/pipermail/u-boot/2010-April/069579.html

What is ppcDWstore(), and why do you use it sometimes instead of __raw_writel()? ?I see the code for ppcDWstore:

? ? ? ?.globl ?ppcDWstore
ppcDWstore:
? ? ? ?lfd ? ? 1, 0(r4)
? ? ? ?stfd ? ?1, 0(r3)
? ? ? ?blr

Quoted 4 lines
Quoted 2 lines

I just don't understand why it exists.

This is the fastest way to copy 64 bit words.

hmm, not all boards have a FPU(8xx and 832x). How are lfd and stfd handled there?

Jocke

author
#14 lore raw

PATCH 1/1 POST: Added ECC memory test for mpc83xx.

Quoted 20 lines · Joakim Tjernlund
Quoted 20 lines · Timur Tabi
Quoted 17 lines
Quoted 2 lines · Michael Zaidman

The version #2 of the patch is located here
http://lists.denx.de/pipermail/u-boot/2010-April/069579.html

What is ppcDWstore(), and why do you use it sometimes instead of __raw_writel()? ?I see the code for ppcDWstore:

? ? ? ?.globl ?ppcDWstore
ppcDWstore:
? ? ? ?lfd ? ? 1, 0(r4)
? ? ? ?stfd ? ?1, 0(r3)
? ? ? ?blr

I just don't understand why it exists.

Quoted 7 lines
Quoted 1 line

This is the fastest way to copy 64 bit words.

hmm, not all boards have a FPU(8xx and 832x). How are lfd and stfd handled there?

?Jocke

The ppcDWstore/ ppcDWload code is mpc83xx specific, so the mpc8xx is not relevant here. The e300c1 and e300c3 have FPU while e300c2 has not. The lfd/stfd instructions load/store data from/to memory to/from FPRs. It looks like these instructions are performed by Load/Store Unit and rather require FPRs presence than FPU. The block diagram of the e300c2 core in the e300coreRM (Rev3, 12/2006) specifies FPR File and its connection to the Load/Store Unit. On other hand the "1.3.1.1.2 Floating-Point Registers (FPRs)" chapter says that FPRs are not included in the e300c2 core. Can someone clarify this?

Thanks,
Michael

#15 lore raw

PATCH 1/1 POST: Added ECC memory test for mpc83xx.

Quoted 24 lines · On Thu, 8 Apr 2010 09:49:08 +0300 Michael Zaidman <[email protected]>…
Quoted 22 lines · Joakim Tjernlund
Quoted 21 lines · Timur Tabi
Quoted 9 lines
Quoted 2 lines · Michael Zaidman

The version #2 of the patch is located here
http://lists.denx.de/pipermail/u-boot/2010-April/069579.html

What is ppcDWstore(), and why do you use it sometimes instead of __raw_writel()? ?I see the code for ppcDWstore:

Quoted 8 lines

? ? ? ?.globl ?ppcDWstore
ppcDWstore:
? ? ? ?lfd ? ? 1, 0(r4)
? ? ? ?stfd ? ?1, 0(r3)
? ? ? ?blr

I just don't understand why it exists.

This is the fastest way to copy 64 bit words.

Quoted 7 lines
Quoted 5 lines

hmm, not all boards have a FPU(8xx and 832x). How are lfd and stfd handled there?

?Jocke

The ppcDWstore/ ppcDWload code is mpc83xx specific, so the mpc8xx is not relevant here. The e300c1 and e300c3 have FPU while e300c2 has

right (which also suggests it's badly named (at least since the advent of the e300c2)).

Quoted 7 lines

not. The lfd/stfd instructions load/store data from/to memory to/from FPRs. It looks like these instructions are performed by Load/Store Unit and rather require FPRs presence than FPU. The block diagram of the e300c2 core in the e300coreRM (Rev3, 12/2006) specifies FPR File and its connection to the Load/Store Unit. On other hand the "1.3.1.1.2 Floating-Point Registers (FPRs)" chapter says that FPRs are not included in the e300c2 core. Can someone clarify this?

The documentation is confusing: the e300c2 has its FPU chopped off - the FP registers are simply not there.

this is a good catch by Jocke - it would be best if generic 83xx code didn't depend on the ppcDW* accessors.

Kim

#16 lore raw

PATCH 1/1 POST: Added ECC memory test for mpc83xx.

Kim Phillips <[email protected]> wrote on 2010-04-08 10:27:03:

Quoted 47 lines
Quoted 24 lines · On Thu, 8 Apr 2010 09:49:08 +0300 Michael Zaidman <[email protected]>…
Quoted 22 lines · Joakim Tjernlund
Quoted 21 lines · Timur Tabi
Quoted 9 lines
Quoted 2 lines · Michael Zaidman

The version #2 of the patch is located here
http://lists.denx.de/pipermail/u-boot/2010-April/069579.html

What is ppcDWstore(), and why do you use it sometimes instead of __raw_writel()? ?I see the code for ppcDWstore:

Quoted 8 lines

? ? ? ?.globl ?ppcDWstore
ppcDWstore:
? ? ? ?lfd ? ? 1, 0(r4)
? ? ? ?stfd ? ?1, 0(r3)
? ? ? ?blr

I just don't understand why it exists.

This is the fastest way to copy 64 bit words.

Quoted 7 lines
Quoted 5 lines

hmm, not all boards have a FPU(8xx and 832x). How are lfd and stfd handled there?

?Jocke

The ppcDWstore/ ppcDWload code is mpc83xx specific, so the mpc8xx is not relevant here. The e300c1 and e300c3 have FPU while e300c2 has

right (which also suggests it's badly named (at least since the advent of the e300c2)).

Quoted 7 lines

not. The lfd/stfd instructions load/store data from/to memory to/from FPRs. It looks like these instructions are performed by Load/Store Unit and rather require FPRs presence than FPU. The block diagram of the e300c2 core in the e300coreRM (Rev3, 12/2006) specifies FPR File and its connection to the Load/Store Unit. On other hand the "1.3.1.1.2 Floating-Point Registers (FPRs)" chapter says that FPRs are not included in the e300c2 core. Can someone clarify this?

Quoted 5 lines

The documentation is confusing: the e300c2 has its FPU chopped off - the FP registers are simply not there.

this is a good catch by Jocke - it would be best if generic 83xx code didn't depend on the ppcDW* accessors.

That or one could impl. ppcDW* using normal load/store insns for 832x. Either way the ppcDW* should be inlined as the overhead for doing function calls isn't something you want when looking for speed.

Jocke

#17 lore raw

PATCH 1/1 POST: Added ECC memory test for mpc83xx.

Quoted 11 lines · On Thu, 8 Apr 2010 10:37:08 +0200 Joakim Tjernlund…

Kim Phillips <[email protected]> wrote on 2010-04-08 10:27:03:

Quoted 6 lines

The documentation is confusing: the e300c2 has its FPU chopped off - the FP registers are simply not there.

this is a good catch by Jocke - it would be best if generic 83xx code didn't depend on the ppcDW* accessors.

That or one could impl. ppcDW* using normal load/store insns for 832x. Either way the ppcDW* should be inlined as the overhead for doing function calls isn't something you want when looking for speed.

another good point, but it seems they were added primarily for code density benefits. I think we can do something like this in the meantime:

author
#18 lore raw

PATCH 1/1 POST: Added ECC memory test for mpc83xx.

Quoted 67 lines · Kim Phillips
Quoted 11 lines · On Thu, 8 Apr 2010 10:37:08 +0200 Joakim Tjernlund…

Kim Phillips <[email protected]> wrote on 2010-04-08 10:27:03:

Quoted 6 lines

The documentation is confusing: the e300c2 has its FPU chopped off - the FP registers are simply not there.

this is a good catch by Jocke - it would be best if generic 83xx code didn't depend on the ppcDW* accessors.

That or one could impl. ppcDW* using normal load/store insns for 832x. Either way the ppcDW* should be inlined as the overhead for doing function calls isn't something you want when looking for speed.

another good point, but it seems they were added primarily for code density benefits. I think we can do something like this in the meantime:

From 686d3bb7a732ec36beec169c4eaf4882382d3aa2 Mon Sep 17 00:00:00 2001
From: Kim Phillips <[email protected]>
Date: Thu, 8 Apr 2010 18:22:13 -0500
Subject: [PATCH] mpc83xx: implement ppcDW{load,store} accessors for e300c2

e300c2 core based processors (MPC832x) don't have an FPU: provide alternative, gpr based accessor functions for code compatibility.

Suggested-by Joakim Tjernlund <[email protected]>
Signed-off-by Kim Phillips <[email protected]>
---
 arch/ppc/cpu/mpc83xx/start.S |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)
arch/ppc/cpu/mpc83xx/start.S +14 -1
index 68bb620..6bfce57 100644
@@ -139,14 +139,28 @@ get_pvr:
 
       .globl  ppcDWstore
 ppcDWstore:
+#if !defined(CONFIG_MPC832x)
       lfd     1, 0(r4)
       stfd    1, 0(r3)
+#else
+       lwz     r5, 0(r4)
+       stw     r5, 0(r3)
+       lwz     r5, 4(r4)
+       stw     r5, 4(r3)
+#endif
       blr
 
       .globl  ppcDWload
 ppcDWload:
+#if !defined(CONFIG_MPC832x)
       lfd     1, 0(r3)
       stfd    1, 0(r4)
+#else
+       lwz     r5, 0(r3)
+       stw     r5, 0(r4)
+       lwz     r5, 4(r3)
+       stw     r5, 4(r4)
+#endif
       blr
 
 #ifndef CONFIG_DEFAULT_IMMR
--
Quoted 2 lines

1.7.0.5

Although this is good for most of the cases it does not fit in the algorithm implemented in the post_ecc_test. The first stw in ppcDWstore will generate ecc error (due to read-modify-write) so ecc capture data registers will capture only first word as it was written with flipped injected error bit or without depending on its position in the data_error_inject_hi or data_error_inject_lo injection mask registers. The second ecc capture data word will hold the data that was in the memory right before the ppcDWstore call. Thus, the test validation while working for stfd will fail for stw x 2. So, the algorithm need to be reworked...

I also agree with Joakim regarding the routine call overhead and replacing it by inline macro. Please review this code.

#19 lore raw

PATCH 1/1 POST: Added ECC memory test for mpc83xx.

Michael Zaidman <[email protected]> wrote on 2010/04/16 18:44:12:

Quoted 47 lines
Quoted 4 lines · Kim Phillips
Quoted 1 line · On Thu, 8 Apr 2010 10:37:08 +0200 Joakim Tjernlund…

Kim Phillips <[email protected]> wrote on 2010-04-08 10:27:03:

Although this is good for most of the cases it does not fit in the algorithm implemented in the post_ecc_test. The first stw in ppcDWstore will generate ecc error (due to read-modify-write) so ecc capture data registers will capture only first word as it was written with flipped injected error bit or without depending on its position in the data_error_inject_hi or data_error_inject_lo injection mask registers. The second ecc capture data word will hold the data that was in the memory right before the ppcDWstore call. Thus, the test validation while working for stfd will fail for stw x 2. So, the algorithm need to be reworked...

I also agree with Joakim regarding the routine call overhead and replacing it by inline macro. Please review this code.

From 5a64a5c4f480dcea89bc8f13f8464b96b888b73c Mon Sep 17 00:00:00 2001
From: Michael Zaidman <[email protected]>
Date: Fri, 16 Apr 2010 18:50:43 +0300
Subject: [U-Boot][PATCH] asm-ppc/io.h - added 64bits I/O accessors for ppc32.

Suggested-by Joakim Tjernlund <[email protected]>
Signed-off-by Michael Zaidman <[email protected]>
---
 include/asm-ppc/io.h |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 49 insertions(+), 0 deletions(-)
include/asm-ppc/io.h +4 -0
index 4ddad26..0d5e125 100644
@@ -231,6 +231,31 @@ extern inline unsigned in_be32(const volatile
 unsigned __iomem *addr)
    return ret;
 }
 
+/* 64 bits I/O read accessor for ppc32 */
+extern inline void in_be64(volatile unsigned __iomem *addr, volatile
 unsigned __iomem *ret)
+{
+/* FIXME: Add other CPUs without FPU here... */

include/asm-ppc/io.h extern inline unsigned in_be32(const volatile · line 249

Quoted 10 lines
include/asm-ppc/io.h +10 -0
+#if defined(CONFIG_MPC832x)
+   __asm__ __volatile__(
+         "sync\n"
+         "lwz%U0%X0 0,%0\n"
+         "stw%U1%X1 0,%1\n"
+         "lwz%U0%X0 0,4+%0\n"
+         "stw%U1%X1 0,4+%1\n"
+         "isync"
+         :"=m" (*ret)
+         :"m"  (*addr), "r" (addr), "r" (ret));

To emulate FP better you should do 2 loads first and then 2 stores. It is also faster.

#20 lore raw

PATCH 1/1 POST: Added ECC memory test for mpc83xx.

Quoted 54 lines

Michael Zaidman <[email protected]> wrote on 2010/04/16 18:44:12:

Quoted 52 lines
Quoted 4 lines · Kim Phillips
Quoted 1 line · On Thu, 8 Apr 2010 10:37:08 +0200 Joakim Tjernlund…

Kim Phillips <[email protected]> wrote on 2010-04-08 10:27:03:

Although this is good for most of the cases it does not fit in the algorithm implemented in the post_ecc_test. The first stw in ppcDWstore will generate ecc error (due to read-modify-write) so ecc capture data registers will capture only first word as it was written with flipped injected error bit or without depending on its position in the data_error_inject_hi or data_error_inject_lo injection mask registers. The second ecc capture data word will hold the data that was in the memory right before the ppcDWstore call. Thus, the test validation while working for stfd will fail for stw x 2. So, the algorithm need to be reworked...

I also agree with Joakim regarding the routine call overhead and replacing it by inline macro. Please review this code.

From 5a64a5c4f480dcea89bc8f13f8464b96b888b73c Mon Sep 17 00:00:00 2001
From: Michael Zaidman <[email protected]>
Date: Fri, 16 Apr 2010 18:50:43 +0300
Subject: [U-Boot][PATCH] asm-ppc/io.h - added 64bits I/O accessors for ppc32.

Suggested-by Joakim Tjernlund <[email protected]>
Signed-off-by Michael Zaidman <[email protected]>
---
 include/asm-ppc/io.h |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 49 insertions(+), 0 deletions(-)
include/asm-ppc/io.h +9 -0
index 4ddad26..0d5e125 100644
@@ -231,6 +231,31 @@ extern inline unsigned in_be32(const volatile
 unsigned __iomem *addr)
    return ret;
 }
 
+/* 64 bits I/O read accessor for ppc32 */
+extern inline void in_be64(volatile unsigned __iomem *addr, volatile
 unsigned __iomem *ret)
+{
+/* FIXME: Add other CPUs without FPU here... */
+#if defined(CONFIG_MPC832x)
+   __asm__ __volatile__(
+         "sync\n"
+         "lwz%U0%X0 0,%0\n"
+         "stw%U1%X1 0,%1\n"
Quoted 8 lines

include/asm-ppc/io.h extern inline unsigned in_be32(const volatile · line 249

Quoted 5 lines
include/asm-ppc/io.h +5 -0
+         "lwz%U0%X0 0,4+%0\n"
+         "stw%U1%X1 0,4+%1\n"
+         "isync"
+         :"=m" (*ret)
+         :"m"  (*addr), "r" (addr), "r" (ret));

To emulate FP better you should do 2 loads first and then 2 stores. It is also faster.

Forgot, you can add the MPC8xx CPU's too. These are also without FPU.

author
#21 lore raw

PATCH 1/1 POST: Added ECC memory test for mpc83xx.

Quoted 52 lines · Joakim Tjernlund
Quoted 43 lines

Michael Zaidman <[email protected]> wrote on 2010/04/16 18:44:12:

Quoted 41 lines
Quoted 4 lines · Kim Phillips
Quoted 1 line · On Thu, 8 Apr 2010 10:37:08 +0200 Joakim Tjernlund…

Kim Phillips <[email protected]> wrote on 2010-04-08 10:27:03:

I also agree with Joakim regarding the routine call overhead and replacing it by inline macro. Please review this code.

From 5a64a5c4f480dcea89bc8f13f8464b96b888b73c Mon Sep 17 00:00:00 2001
From: Michael Zaidman <[email protected]>
Date: Fri, 16 Apr 2010 18:50:43 +0300
Subject: [U-Boot][PATCH] asm-ppc/io.h - added 64bits I/O accessors for ppc32.

Suggested-by Joakim Tjernlund <[email protected]>
Signed-off-by Michael Zaidman <[email protected]>

---
?include/asm-ppc/io.h | ? 49 +++++++++++++++++++++++++++++++++++++++++++++++++ ?1 files changed, 49 insertions(+), 0 deletions(-)

include/asm-ppc/io.h +9 -0
index 4ddad26..0d5e125 100644
@@ -231,6 +231,31 @@ extern inline unsigned in_be32(const volatile
 unsigned __iomem *addr)
 ? ? return ret;
 ?}
 
+/* 64 bits I/O read accessor for ppc32 */
+extern inline void in_be64(volatile unsigned __iomem *addr, volatile
 unsigned __iomem *ret)
+{
+/* FIXME: Add other CPUs without FPU here... */
+#if defined(CONFIG_MPC832x)
+ ? __asm__ __volatile__(
+ ? ? ? ? "sync\n"
+ ? ? ? ? "lwz%U0%X0 0,%0\n"
+ ? ? ? ? "stw%U1%X1 0,%1\n"
Quoted 8 lines

include/asm-ppc/io.h extern inline unsigned in_be32(const volatile · line 249

Quoted 5 lines
include/asm-ppc/io.h +5 -0
+ ? ? ? ? "lwz%U0%X0 0,4+%0\n"
+ ? ? ? ? "stw%U1%X1 0,4+%1\n"
+ ? ? ? ? "isync"
+ ? ? ? ? :"=m" (*ret)
+ ? ? ? ? :"m" ?(*addr), "r" (addr), "r" (ret));

To emulate FP better you should do 2 loads first and then 2 stores. It is also faster.

Quoted 3 lines

Forgot, you can add the MPC8xx CPU's too. These are also without FPU.

Ok, thanks, the code below does this. I did not succeed however to force the compiler to assign register for temporary storage and used r5 explicitly.

/* 64 bits I/O read accessor for ppc32 */
extern inline void in_be64(volatile unsigned __iomem *addr, volatile unsigned __iomem *ret)
{
/* FIXME: Add other CPUs without FPU here... */

#if defined(CONFIG_MPC832x) || defined(CONFIG_MPC8xx)
       __asm__ __volatile__(
				"sync\n"
				"lwz%U0%X0 0,%0\n"
				"lwz%U0%X0 5,4+%0\n"
				"sync\n"
				"stw%U1%X1 0,%1\n"
				"stw%U1%X1 5,4+%1\n"
				"isync"
				:"=m" (*ret)
				:"m"  (*addr), "r" (addr), "r" (ret)
				:"r0", "r5"
				);
#else
       __asm__ __volatile__(
				"sync\n"
				"lfd%U0%X0  1,%0\n"
				"stfd%U1%X1 1,%1\n"
				"isync"
				:"=m" (*ret)
				:"m"  (*addr), "r" (addr), "r" (ret));

#endif
}

/* 64 bits I/O write accessor for ppc32 */
extern inline void out_be64(volatile unsigned __iomem *addr, volatile unsigned __iomem *val)
{
/* FIXME: Add other CPUs without FPU here... */

#if defined(CONFIG_MPC832x) || defined(CONFIG_MPC8xx)
       __asm__ __volatile__(
				"sync\n"
				"lwz%U0%X0 0,%0\n"
				"lwz%U0%X0 5,4+%0\n"
				"sync\n"
				"stw%U1%X1 0,%1\n"
				"stw%U1%X1 5,4+%1\n"
				:"=m" (*addr)
				:"m"  (*val), "r" (addr), "r" (val)
				:"r0", "r5"
				);
#else
       __asm__ __volatile__(
				"sync\n"
				"lfd%U1%X1  1,%1\n"
				"stfd%U0%X0 1,%0"
				:"=m" (*addr)
				:"m"  (*val), "r" (addr), "r" (val));

#endif
}

-michael