[PATCH] arch/powerpc/lib/copy_32.S: Use alternate memcpy for MPC512x and MPC52xx

[PATCH] arch/powerpc/lib/copy_32.S: Use alternate memcpy for MPC512x and MPC52xx

[Steve Deiters]

These processors will corrupt data if accessing the local bus with
unaligned
addresses. This version fixes the typical case of copying from Flash on
the
local bus by keeping the source address always aligned.

Signed-off-by: Steve Deiters <SteveDeiters@basler.com>
---
 arch/powerpc/lib/copy_32.S |   56
++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 56 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/lib/copy_32.S b/arch/powerpc/lib/copy_32.S
index 74a7f41..42e7df5 100644
--- a/arch/powerpc/lib/copy_32.S
+++ b/arch/powerpc/lib/copy_32.S
@@ -226,6 +226,60 @@ _GLOBAL(memmove)
 	bgt	backwards_memcpy
 	/* fall through */
=20
+#if defined(CONFIG_PPC_MPC512x) || defined(CONFIG_PPC_MPC52xx)
+
+/*
+ * Alternate memcpy for MPC512x and MPC52xx to guarantee source
+ * address is always aligned to prevent corruption issues when
+ * copying unaligned from the local bus. This only fixes the usage
+ * when copying from the local bus (e.g. Flash) and will not fix
+ * issues copying to the local bus
+ */
+_GLOBAL(memcpy)
+	srwi.	r7,r5,3
+	addi	r6,r3,-4
+	addi	r4,r4,-4
+	beq	2f			/* if less than 8 bytes to do */
+	andi.	r0,r4,3			/* get src word aligned */
+	mtctr	r7
+	bne	5f
+1:	lwz	r7,4(r4)
+	lwzu	r8,8(r4)
+	stw	r7,4(r6)
+	stwu	r8,8(r6)
+	bdnz	1b
+	andi.	r5,r5,7
+2:	cmplwi	0,r5,4
+	blt	3f
+	andi.	r0,r4,3
+	bne	3f
+	lwzu	r0,4(r4)
+	addi	r5,r5,-4
+	stwu	r0,4(r6)
+3:	cmpwi	0,r5,0
+	beqlr
+	mtctr	r5
+	addi	r4,r4,3
+	addi	r6,r6,3
+4:	lbzu	r0,1(r4)
+	stbu	r0,1(r6)
+	bdnz	4b
+	blr
+5:	subfic	r0,r0,4
+	mtctr	r0
+6:	lbz	r7,4(r4)
+	addi	r4,r4,1
+	stb	r7,4(r6)
+	addi	r6,r6,1
+	bdnz	6b
+	subf	r5,r0,r5
+	rlwinm.	r7,r5,32-3,3,31
+	beq	2b
+	mtctr	r7
+	b	1b
+
+#else
+
 _GLOBAL(memcpy)
 	srwi.	r7,r5,3
 	addi	r6,r3,-4
@@ -267,6 +321,8 @@ _GLOBAL(memcpy)
 	mtctr	r7
 	b	1b
=20
+#endif
+
 _GLOBAL(backwards_memcpy)
 	rlwinm.	r7,r5,32-3,3,31		/* r0 =3D r5 >> 3 */
 	add	r6,r3,r5
--=20
1.5.4.3

Re: [PATCH] arch/powerpc/lib/copy_32.S: Use alternate memcpy for MPC512x and MPC52xx

[Segher Boessenkool]

> These processors will corrupt data if accessing the local bus with
> unaligned
> addresses. This version fixes the typical case of copying from  
> Flash on
> the
> local bus by keeping the source address always aligned.

On many platforms accessing ROM as RAM simply doesn't work(*).  You  
shouldn't
map ROM as if it is RAM, and shouldn't use the same access functions  
on it.


Segher


(*) Example: any existing 970 system will checkstop as soon as you  
try to
do any cacheable access to some ROM.  Another example of course is  
unaligned
accesses on pretty much any system, no matter whether it's called a  
bug or
a feature on that system :-P

Re: [PATCH] arch/powerpc/lib/copy_32.S: Use alternate memcpy for MPC512x and MPC52xx

[Benjamin Herrenschmidt]

On Tue, 2010-06-29 at 11:04 -0500, Steve Deiters wrote:
> These processors will corrupt data if accessing the local bus with
> unaligned
> addresses. This version fixes the typical case of copying from Flash on
> the
> local bus by keeping the source address always aligned.

Shouldn't this be solved by using memcpy_to/fromio ?

Cheers,
Ben.

> Signed-off-by: Steve Deiters <SteveDeiters@basler.com>
> ---
>  arch/powerpc/lib/copy_32.S |   56
> ++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 56 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/powerpc/lib/copy_32.S b/arch/powerpc/lib/copy_32.S
> index 74a7f41..42e7df5 100644
> --- a/arch/powerpc/lib/copy_32.S
> +++ b/arch/powerpc/lib/copy_32.S
> @@ -226,6 +226,60 @@ _GLOBAL(memmove)
>  	bgt	backwards_memcpy
>  	/* fall through */
>  
> +#if defined(CONFIG_PPC_MPC512x) || defined(CONFIG_PPC_MPC52xx)
> +
> +/*
> + * Alternate memcpy for MPC512x and MPC52xx to guarantee source
> + * address is always aligned to prevent corruption issues when
> + * copying unaligned from the local bus. This only fixes the usage
> + * when copying from the local bus (e.g. Flash) and will not fix
> + * issues copying to the local bus
> + */
> +_GLOBAL(memcpy)
> +	srwi.	r7,r5,3
> +	addi	r6,r3,-4
> +	addi	r4,r4,-4
> +	beq	2f			/* if less than 8 bytes to do */
> +	andi.	r0,r4,3			/* get src word aligned */
> +	mtctr	r7
> +	bne	5f
> +1:	lwz	r7,4(r4)
> +	lwzu	r8,8(r4)
> +	stw	r7,4(r6)
> +	stwu	r8,8(r6)
> +	bdnz	1b
> +	andi.	r5,r5,7
> +2:	cmplwi	0,r5,4
> +	blt	3f
> +	andi.	r0,r4,3
> +	bne	3f
> +	lwzu	r0,4(r4)
> +	addi	r5,r5,-4
> +	stwu	r0,4(r6)
> +3:	cmpwi	0,r5,0
> +	beqlr
> +	mtctr	r5
> +	addi	r4,r4,3
> +	addi	r6,r6,3
> +4:	lbzu	r0,1(r4)
> +	stbu	r0,1(r6)
> +	bdnz	4b
> +	blr
> +5:	subfic	r0,r0,4
> +	mtctr	r0
> +6:	lbz	r7,4(r4)
> +	addi	r4,r4,1
> +	stb	r7,4(r6)
> +	addi	r6,r6,1
> +	bdnz	6b
> +	subf	r5,r0,r5
> +	rlwinm.	r7,r5,32-3,3,31
> +	beq	2b
> +	mtctr	r7
> +	b	1b
> +
> +#else
> +
>  _GLOBAL(memcpy)
>  	srwi.	r7,r5,3
>  	addi	r6,r3,-4
> @@ -267,6 +321,8 @@ _GLOBAL(memcpy)
>  	mtctr	r7
>  	b	1b
>  
> +#endif
> +
>  _GLOBAL(backwards_memcpy)
>  	rlwinm.	r7,r5,32-3,3,31		/* r0 = r5 >> 3 */
>  	add	r6,r3,r5

Re: [PATCH] arch/powerpc/lib/copy_32.S: Use alternate memcpy for MPC512x and MPC52xx

[Grant Likely]

On Wed, Jul 7, 2010 at 11:10 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Tue, 2010-06-29 at 11:04 -0500, Steve Deiters wrote:
>> These processors will corrupt data if accessing the local bus with
>> unaligned
>> addresses. This version fixes the typical case of copying from Flash on
>> the
>> local bus by keeping the source address always aligned.
>
> Shouldn't this be solved by using memcpy_to/fromio ?

Maybe.  plain memcpy() access to anything localbus-attached on the
mpc5xxx is the wrong thing to do.  memcpy_to/fromio might do the right
thing; but the caller must understand the limitations of the localplus
bus.  The byte-wise alignment that memcpy_to/fromio() does may not
work (depending on configuration).

Steve, what code is doing a memcpy from flash?

g.

RE: [PATCH] arch/powerpc/lib/copy_32.S: Use alternate memcpy for MPC512x and MPC52xx

[Steve Deiters]

> -----Original Message-----
> From: glikely@secretlab.ca [mailto:glikely@secretlab.ca] On=20
> Behalf Of Grant Likely
> Sent: Thursday, July 08, 2010 12:38 AM
> To: Benjamin Herrenschmidt
> Cc: Steve Deiters; linuxppc-dev@lists.ozlabs.org
> Subject: Re: [PATCH] arch/powerpc/lib/copy_32.S: Use=20
> alternate memcpy for MPC512x and MPC52xx
>=20
> On Wed, Jul 7, 2010 at 11:10 PM, Benjamin Herrenschmidt=20
> <benh@kernel.crashing.org> wrote:
> > On Tue, 2010-06-29 at 11:04 -0500, Steve Deiters wrote:
> >> These processors will corrupt data if accessing the local bus with=20
> >> unaligned addresses. This version fixes the typical case=20
> of copying=20
> >> from Flash on the local bus by keeping the source address always=20
> >> aligned.
> >
> > Shouldn't this be solved by using memcpy_to/fromio ?
>=20
> Maybe.  plain memcpy() access to anything localbus-attached=20
> on the mpc5xxx is the wrong thing to do.  memcpy_to/fromio=20
> might do the right thing; but the caller must understand the=20
> limitations of the localplus bus.  The byte-wise alignment=20
> that memcpy_to/fromio() does may not work (depending on=20
> configuration).
>=20
> Steve, what code is doing a memcpy from flash?
>=20
> g.

This was done in the JFFS2 code, in fs/jffs2/scan.c.  At least in one
function, in jffs2_scan_dirent_node it was using memcpy on a localbus
mapped structure.  I was getting JFFS2 filesystem corruption because of
this.  In fact I first tried changing this to memcpy_fromio and it fixed
that particular problem.  I was concerned though that other places I was
not aware of might be using memcpy from the localbus in a similar manner
so I decided to just tweak the memcpy routine.

Just out of curiousity, what configuration might cause a byte-wise
alignment not to work?

Re: [PATCH] arch/powerpc/lib/copy_32.S: Use alternate memcpy for MPC512x and MPC52xx

[Grant Likely]

On Thu, Jul 8, 2010 at 8:38 AM, Steve Deiters <SteveDeiters@basler.com> wro=
te:
>> -----Original Message-----
>> From: glikely@secretlab.ca [mailto:glikely@secretlab.ca] On
>> Behalf Of Grant Likely
>> Sent: Thursday, July 08, 2010 12:38 AM
>> To: Benjamin Herrenschmidt
>> Cc: Steve Deiters; linuxppc-dev@lists.ozlabs.org
>> Subject: Re: [PATCH] arch/powerpc/lib/copy_32.S: Use
>> alternate memcpy for MPC512x and MPC52xx
>>
>> On Wed, Jul 7, 2010 at 11:10 PM, Benjamin Herrenschmidt
>> <benh@kernel.crashing.org> wrote:
>> > On Tue, 2010-06-29 at 11:04 -0500, Steve Deiters wrote:
>> >> These processors will corrupt data if accessing the local bus with
>> >> unaligned addresses. This version fixes the typical case
>> of copying
>> >> from Flash on the local bus by keeping the source address always
>> >> aligned.
>> >
>> > Shouldn't this be solved by using memcpy_to/fromio ?
>>
>> Maybe. =A0plain memcpy() access to anything localbus-attached
>> on the mpc5xxx is the wrong thing to do. =A0memcpy_to/fromio
>> might do the right thing; but the caller must understand the
>> limitations of the localplus bus. =A0The byte-wise alignment
>> that memcpy_to/fromio() does may not work (depending on
>> configuration).
>>
>> Steve, what code is doing a memcpy from flash?
>>
>> g.
>
> This was done in the JFFS2 code, in fs/jffs2/scan.c. =A0At least in one
> function, in jffs2_scan_dirent_node it was using memcpy on a localbus
> mapped structure. =A0I was getting JFFS2 filesystem corruption because of
> this. =A0In fact I first tried changing this to memcpy_fromio and it fixe=
d
> that particular problem. =A0I was concerned though that other places I wa=
s
> not aware of might be using memcpy from the localbus in a similar manner
> so I decided to just tweak the memcpy routine.

[cc'ing David Woodhouse]

Sounds to me like the right thing to do is to fix the jffs2 code.

> Just out of curiousity, what configuration might cause a byte-wise
> alignment not to work?

Can't remember the register configuration, but I worked on one project
where this was the case.  In hindsight, it was probably a
mis-configuration of the localbus CS for the particular device.

g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] arch/powerpc/lib/copy_32.S: Use alternate memcpy for MPC512x and MPC52xx

[Albrecht Dreß]

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

Am 08.07.10 17:22 schrieb(en) Grant Likely:
>> Just out of curiousity, what configuration might cause a byte-wise alignment not to work?
> 
> Can't remember the register configuration, but I worked on one project where this was the case.  In hindsight, it was probably a mis-configuration of the localbus CS for the particular device.

Not sure if you're thinking of this configuration, but if you attach a device in 16-bit mode (i.e. 16 data lines) to the LPB, byte writes simply don't work.  I ran into that problem as I have a nvram attached this way to a 5200b.  Using the device as mtd-ram with a jffs2 file system on it I also sometimes saw corruption after a write.

I had a patch for that last year, but it was actually badly crafted (see <http://lists.ozlabs.org/pipermail/linuxppc-dev/2009-June/072903.html>).  I still use the mpc52xx_memcpy2lpb16() function somewhere in my current code which is actually an ugly hack (but it works...).

Actually, this is something which might need closer attention - and maybe some support in the device tree indicating which read or write width a device can accept?

Best, Albrecht.

[-- Attachment #2: Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH] arch/powerpc/lib/copy_32.S: Use alternate memcpy for MPC512x and MPC52xx

[Segher Boessenkool]

> Actually, this is something which might need closer attention - and  
> maybe some support in the device tree indicating which read or  
> write width a device can accept?

There already is "device-width"; the drivers never should use any other
access width unless they *know* that will work.


Segher

Re: [PATCH] arch/powerpc/lib/copy_32.S: Use alternate memcpy for MPC512x and MPC52xx

[Scott Wood]

On Thu, 8 Jul 2010 21:30:33 +0200
Segher Boessenkool <segher@kernel.crashing.org> wrote:

> > Actually, this is something which might need closer attention -
> > and maybe some support in the device tree indicating which read or  
> > write width a device can accept?
> 
> There already is "device-width"; the drivers never should use any
> other access width unless they *know* that will work.

Wouldn't you want to use "bank-width" instead?  "device-width" might
not even work if, say, you've got two 8-bit chips interleaved, feeding
into a bus controller in 16-bit mode that only accepts 16-bit accesses.

It would be nice to have a device tree property that can specify that
all access widths supported by the CPU will work, though.

-Scott

Re: [PATCH] arch/powerpc/lib/copy_32.S: Use alternate memcpy for MPC512x and MPC52xx

[Albrecht Dreß]

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

Am 08.07.10 21:30 schrieb(en) Segher Boessenkool:
>> Actually, this is something which might need closer attention - and maybe some support in the device tree indicating which read or write width a device can accept?
> 
> There already is "device-width"; the drivers never should use any other access width unless they *know* that will work.

Hmm, unfortunately, it's usage is not clearly documented in mtd-physmap.txt, so I never thought of this parameter.  And IMHO the problem goes further - basically *any* chip which is attached to the LPB can be affected by this problem, so it might be better to have a more general approach like a "chip select property".

Cheers, Albrecht.

[-- Attachment #2: Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH] arch/powerpc/lib/copy_32.S: Use alternate memcpy for MPC512x and MPC52xx

[Segher Boessenkool]

>>> Actually, this is something which might need closer attention -
>>> and maybe some support in the device tree indicating which read or
>>> write width a device can accept?
>>
>> There already is "device-width"; the drivers never should use any
>> other access width unless they *know* that will work.
>
> Wouldn't you want to use "bank-width" instead?

We were talking about single devices.  But, sure, when you have
multiple devices in parallel the driver needs to know about that.

> It would be nice to have a device tree property that can specify that
> all access widths supported by the CPU will work, though.

Oh please no.  A device binding should not depend on what CPU there
is in the system.  There could be multiple CPUs of different
architectures, even.

To figure out how to access a device, the driver looks at the device's
node, and all its parent nodes (or asks generic code to do that, or
platform code).


Segher

Re: [PATCH] arch/powerpc/lib/copy_32.S: Use alternate memcpy for MPC512x and MPC52xx

[Segher Boessenkool]

> Hmm, unfortunately, it's usage is not clearly documented in mtd- 
> physmap.txt,

It's pretty clear I think.  Patches for making it better are welcome  
of course.

> so I never thought of this parameter.  And IMHO the problem goes  
> further - basically *any* chip which is attached to the LPB can be  
> affected by this problem, so it might be better to have a more  
> general approach like a "chip select property".

You cannot treat devices on the LPB as random access, that's all.   
Drivers
that assume they can, cannot be used for devices on the LPB.


Segher

Re: [PATCH] arch/powerpc/lib/copy_32.S: Use alternate memcpy for MPC512x and MPC52xx

[Scott Wood]

On Fri, 9 Jul 2010 14:59:09 +0200
Segher Boessenkool <segher@kernel.crashing.org> wrote:

> >>> Actually, this is something which might need closer attention -
> >>> and maybe some support in the device tree indicating which read or
> >>> write width a device can accept?
> >>
> >> There already is "device-width"; the drivers never should use any
> >> other access width unless they *know* that will work.
> >
> > Wouldn't you want to use "bank-width" instead?
> 
> We were talking about single devices.  But, sure, when you have
> multiple devices in parallel the driver needs to know about that.
> 
> > It would be nice to have a device tree property that can specify
> > that all access widths supported by the CPU will work, though.
> 
> Oh please no.  A device binding should not depend on what CPU there
> is in the system.  There could be multiple CPUs of different
> architectures, even.

What I meant by that was that the flash interface was claiming that it
is not the limiting factor in which access widths are useable -- it
would be a way to claim that it is as flexible as ordinary memory in
that regard.

If there is a transaction size that is capable of being presented to
this component that it cannot handle, it would not present this
property.

> To figure out how to access a device, the driver looks at the device's
> node, and all its parent nodes (or asks generic code to do that, or
> platform code).

"looks" or "should look"? :-)

If there are transaction sizes supported by the CPU that won't work
with a given device through no fault of that device (or the interface
to that device for which we don't have a separate node), then in
theory, yes, it should be described at a higher level.

In reality, device tree parsing code is not AI, so rather than say "the
driver looks at this and figures it out" it would be better to provide
a more specific proposal of how a device tree might express this and
what the driver would look for, if you think the simple solution is
not expressive enough.

-Scott

Re: arch/powerpc/lib/copy_32.S: Use alternate memcpy for MPC512x and MPC52xx

[Milton Miller]

Steve wrote:
> These processors will corrupt data if accessing the local bus with
> unaligned addresses. This version fixes the typical case of copying from
> Flash on the local bus by keeping the source address always aligned.

As Dave said in May of 2008[1], the map driver is advertising xip access
to userspace so jffs2 is taking the XIP path and doing straight memcpy.

My reading of the mtd code says this is because map_is_linear() is true
a cfi cmdset 0001 nor flash will create a point function which jffs2
calls and it finds it will map the whole fs, so it just reads at will
from the direct mapping.

If the point fails to map the whole device, then jffs2 will use the
mtd mapping driver to copy to a buffer, and the default mapping drivers
will use memcpy_fromio to fill this buffer.

Steve, can you verify this by setting phys to NO_XIP in the mapping
driver and setting CONFIG_MTD_COMPLEX_MAPPINGS?  This is just a test,
as it is aparent this driver needs to have a custom mapping driver
that handles the alignment and read/modfiy/write issues -- the existing
memcopy_*io assumes byte writes are always ok which is not the case for
this processor and bus.


[1] http://thread.gmane.org/gmane.linux.drivers.mtd/21521
(found from the link in Albrecht's reply)

milton