[PATCH] powerpc/mpc52xx/mtd: fix mtd-ram access for 16-bit Local Plus Bus

[PATCH] powerpc/mpc52xx/mtd: fix mtd-ram access for 16-bit Local Plus Bus

[Albrecht Dreß]

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

Hi all,

this patch adds support for RAM chips connected to the Local Plus Bus  
of a MPC5200B in 16-bit mode.  As no single byte write accesses are  
allowed by the bus in this mode, a byte write has to be split into a  
word read - modify - write sequence (mpc52xx_memcpy2lpb16, as  
fix/extension for memcpy_toio; note that memcpy_fromio *does* work just  
fine).  It has been tested in conjunction with Wolfram Sang's mtd-ram  
[1] and Sascha Hauer's jffs unaligned access [2] patches on 2.6.29.1,  
with a Renesas static RAM connected in 16-bit "Large Flash" mode.

[1]  
<http://lists.ozlabs.org/pipermail/linuxppc-dev/2009-June/072794.html>
[2] <http://article.gmane.org/gmane.linux.drivers.mtd/21521>

Signed-off-by: Albrecht Dreß <albrecht.dress@arcor.de>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: linuxppc-dev@ozlabs.org

---

diff -u  
linux-2.6.29.1.orig/arch/powerpc/platforms/52xx/mpc52xx_common.c  
linux-2.6.29.1/arch/powerpc/platforms/52xx/mpc52xx_common.c
--- linux-2.6.29.1.orig/arch/powerpc/platforms/52xx/mpc52xx_common.c     
2009-04-02 22:55:27.000000000 +0200
+++ linux-2.6.29.1/arch/powerpc/platforms/52xx/mpc52xx_common.c  
2009-06-09 21:16:22.000000000 +0200
@@ -225,3 +225,59 @@

         while (1);
  }
+
+/**
+ * mpc52xx_memcpy2lpb16: copy data to the Local Plus Bus in 16-bit  
mode which
+ * doesn't allow byte accesses
+ */
+void
+mpc52xx_memcpy2lpb16(volatile void __iomem *dest, const void *src,
+                    unsigned long n)
+{
+       void *vdest = (void __force *) dest;
+
+       __asm__ __volatile__ ("sync" : : : "memory");
+
+       if (((unsigned long) vdest & 1) != 0) {
+               u8 buf[2];
+
+               *(u16 *)buf = *((volatile u16 *)(vdest - 1));
+               buf[1] = *((u8 *)src);
+               *((volatile u16 *)(vdest - 1)) = *(u16 *)buf;
+               src++;
+               vdest++;
+               n--;
+       }
+
+       /* looks weird, but helps the optimiser... */
+       if (n >= 4) {
+               unsigned long chunks = n >> 2;
+               volatile u32 * _dst = (volatile u32 *)(vdest - 4);
+               volatile u32 * _src = (volatile u32 *)(src - 4);
+
+               vdest += chunks << 2;
+               src += chunks << 2;
+               do {
+                       *++_dst = *++_src;
+               } while (--chunks);
+               n &= 3;
+       }
+
+       if (n >= 2) {
+               *((volatile u16 *)vdest) = *((volatile u16 *)src);
+               src += 2;
+               vdest += 2;
+               n -= 2;
+       }
+
+       if (n > 0) {
+               u8 buf[2];
+
+               *(u16 *)buf = *((volatile u16 *)vdest);
+               buf[0] = *((u8 *)src);
+               *((volatile u16 *)vdest) = *(u16 *)buf;
+       }
+
+       __asm__ __volatile__ ("sync" : : : "memory");
+}
+EXPORT_SYMBOL(mpc52xx_memcpy2lpb16);
diff -u linux-2.6.29.1.orig/arch/powerpc/include/asm/mpc52xx.h  
linux-2.6.29.1/arch/powerpc/include/asm/mpc52xx.h
--- linux-2.6.29.1.orig/arch/powerpc/include/asm/mpc52xx.h       
2009-04-02 22:55:27.000000000 +0200
+++ linux-2.6.29.1/arch/powerpc/include/asm/mpc52xx.h   2009-06-09  
21:14:31.000000000 +0200
@@ -274,6 +274,8 @@
  extern void mpc52xx_map_common_devices(void);
  extern int mpc52xx_set_psc_clkdiv(int psc_id, int clkdiv);
  extern void mpc52xx_restart(char *cmd);
+extern void mpc52xx_memcpy2lpb16(volatile void __iomem *dest, const  
void *src,
+                                unsigned long n);

  /* mpc52xx_pic.c */
  extern void mpc52xx_init_irq(void);
diff -u linux-2.6.29.1.orig/include/linux/mtd/map.h  
linux-2.6.29.1/include/linux/mtd/map.h
--- linux-2.6.29.1.orig/include/linux/mtd/map.h 2009-04-02  
22:55:27.000000000 +0200
+++ linux-2.6.29.1/include/linux/mtd/map.h      2009-06-08  
14:28:05.000000000 +0200
@@ -13,6 +13,9 @@
  #include <asm/unaligned.h>
  #include <asm/system.h>
  #include <asm/io.h>
+#ifdef CONFIG_PPC_MPC52xx
+#include <asm/mpc52xx.h>
+#endif

  #ifdef CONFIG_MTD_MAP_BANK_WIDTH_1
  #define map_bankwidth(map) 1
@@ -417,6 +420,11 @@

  static inline void inline_map_copy_to(struct map_info *map, unsigned  
long to, const void *from, ssize_t len)
  {
+#ifdef CONFIG_PPC_MPC52xx
+       if (map->bankwidth == 2)
+               mpc52xx_memcpy2lpb16(map->virt + to, from, len);
+       else
+#endif
         memcpy_toio(map->virt + to, from, len);
  }


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

Re: [PATCH] powerpc/mpc52xx/mtd: fix mtd-ram access for 16-bit Local Plus Bus

[Grant Likely]

On Tue, Jun 9, 2009 at 1:46 PM, Albrecht Dre=DF<albrecht.dress@arcor.de> wr=
ote:
> Hi all,
>
> this patch adds support for RAM chips connected to the Local Plus Bus of =
a
> MPC5200B in 16-bit mode. =A0As no single byte write accesses are allowed =
by
> the bus in this mode, a byte write has to be split into a word read - mod=
ify
> - write sequence (mpc52xx_memcpy2lpb16, as fix/extension for memcpy_toio;
> note that memcpy_fromio *does* work just fine). =A0It has been tested in
> conjunction with Wolfram Sang's mtd-ram [1] and Sascha Hauer's jffs
> unaligned access [2] patches on 2.6.29.1, with a Renesas static RAM
> connected in 16-bit "Large Flash" mode.
>
> [1] <http://lists.ozlabs.org/pipermail/linuxppc-dev/2009-June/072794.html=
>
> [2] <http://article.gmane.org/gmane.linux.drivers.mtd/21521>
>
> Signed-off-by: Albrecht Dre=DF <albrecht.dress@arcor.de>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: linuxppc-dev@ozlabs.org
>
> ---
>
> diff -u linux-2.6.29.1.orig/arch/powerpc/platforms/52xx/mpc52xx_common.c
> linux-2.6.29.1/arch/powerpc/platforms/52xx/mpc52xx_common.c
> --- linux-2.6.29.1.orig/arch/powerpc/platforms/52xx/mpc52xx_common.c
> =A02009-04-02 22:55:27.000000000 +0200
> +++ linux-2.6.29.1/arch/powerpc/platforms/52xx/mpc52xx_common.c 2009-06-0=
9
> 21:16:22.000000000 +0200
> @@ -225,3 +225,59 @@
>
> =A0 =A0 =A0 =A0while (1);
> =A0}
> +
> +/**
> + * mpc52xx_memcpy2lpb16: copy data to the Local Plus Bus in 16-bit mode
> which
> + * doesn't allow byte accesses
> + */
> +void
> +mpc52xx_memcpy2lpb16(volatile void __iomem *dest, const void *src,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0unsigned long n)
> +{
> + =A0 =A0 =A0 void *vdest =3D (void __force *) dest;
> +
> + =A0 =A0 =A0 __asm__ __volatile__ ("sync" : : : "memory");
> +
> + =A0 =A0 =A0 if (((unsigned long) vdest & 1) !=3D 0) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 u8 buf[2];
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 *(u16 *)buf =3D *((volatile u16 *)(vdest - =
1));
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 buf[1] =3D *((u8 *)src);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 *((volatile u16 *)(vdest - 1)) =3D *(u16 *)=
buf;

what is the purpose of volatile here?  If you need io barriers, then
use the in_/out_be16 macros.

> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 src++;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 vdest++;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 n--;
> + =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 /* looks weird, but helps the optimiser... */
> + =A0 =A0 =A0 if (n >=3D 4) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 unsigned long chunks =3D n >> 2;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 volatile u32 * _dst =3D (volatile u32 *)(vd=
est - 4);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 volatile u32 * _src =3D (volatile u32 *)(sr=
c - 4);
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 vdest +=3D chunks << 2;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 src +=3D chunks << 2;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 do {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 *++_dst =3D *++_src;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 } while (--chunks);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 n &=3D 3;
> + =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 if (n >=3D 2) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 *((volatile u16 *)vdest) =3D *((volatile u1=
6 *)src);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 src +=3D 2;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 vdest +=3D 2;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 n -=3D 2;
> + =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 if (n > 0) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 u8 buf[2];
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 *(u16 *)buf =3D *((volatile u16 *)vdest);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 buf[0] =3D *((u8 *)src);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 *((volatile u16 *)vdest) =3D *(u16 *)buf;

ditto.

> + =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 __asm__ __volatile__ ("sync" : : : "memory");
> +}
> +EXPORT_SYMBOL(mpc52xx_memcpy2lpb16);
> diff -u linux-2.6.29.1.orig/arch/powerpc/include/asm/mpc52xx.h
> linux-2.6.29.1/arch/powerpc/include/asm/mpc52xx.h
> --- linux-2.6.29.1.orig/arch/powerpc/include/asm/mpc52xx.h =A0 =A0 =A0200=
9-04-02
> 22:55:27.000000000 +0200
> +++ linux-2.6.29.1/arch/powerpc/include/asm/mpc52xx.h =A0 2009-06-09
> 21:14:31.000000000 +0200
> @@ -274,6 +274,8 @@
> =A0extern void mpc52xx_map_common_devices(void);
> =A0extern int mpc52xx_set_psc_clkdiv(int psc_id, int clkdiv);
> =A0extern void mpc52xx_restart(char *cmd);
> +extern void mpc52xx_memcpy2lpb16(volatile void __iomem *dest, const void
> *src,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0unsigned=
 long n);
>
> =A0/* mpc52xx_pic.c */
> =A0extern void mpc52xx_init_irq(void);
> diff -u linux-2.6.29.1.orig/include/linux/mtd/map.h
> linux-2.6.29.1/include/linux/mtd/map.h
> --- linux-2.6.29.1.orig/include/linux/mtd/map.h 2009-04-02
> 22:55:27.000000000 +0200
> +++ linux-2.6.29.1/include/linux/mtd/map.h =A0 =A0 =A02009-06-08
> 14:28:05.000000000 +0200
> @@ -13,6 +13,9 @@
> =A0#include <asm/unaligned.h>
> =A0#include <asm/system.h>
> =A0#include <asm/io.h>
> +#ifdef CONFIG_PPC_MPC52xx
> +#include <asm/mpc52xx.h>
> +#endif
>
> =A0#ifdef CONFIG_MTD_MAP_BANK_WIDTH_1
> =A0#define map_bankwidth(map) 1
> @@ -417,6 +420,11 @@
>
> =A0static inline void inline_map_copy_to(struct map_info *map, unsigned l=
ong
> to, const void *from, ssize_t len)
> =A0{
> +#ifdef CONFIG_PPC_MPC52xx
> + =A0 =A0 =A0 if (map->bankwidth =3D=3D 2)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 mpc52xx_memcpy2lpb16(map->virt + to, from, =
len);
> + =A0 =A0 =A0 else
> +#endif
> =A0 =A0 =A0 =A0memcpy_toio(map->virt + to, from, len);
> =A0}

Blech.  ugly #ifdef and not really multiplatform safe (yeah, I know it
shouldn't break non-5200 platforms, but it does have an undesirable
impact).  There's got to be a better way.

g.

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

Re: [PATCH] powerpc/mpc52xx/mtd: fix mtd-ram access for 16-bit Local Plus Bus

[Wolfram Sang]

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

> Blech.  ugly #ifdef and not really multiplatform safe (yeah, I know it
> shouldn't break non-5200 platforms, but it does have an undesirable
> impact).  There's got to be a better way.

What about putting the special memcpy in asm/io.h (like it is done for eeh)?
Will this cause too much overhead for a memcpy which does not go to the LPB?

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: [PATCH] powerpc/mpc52xx/mtd: fix mtd-ram access for 16-bit Local Plus Bus

[Grant Likely]

On Thu, Jun 11, 2009 at 10:55 AM, Wolfram Sang<w.sang@pengutronix.de> wrote=
:
>> Blech. =A0ugly #ifdef and not really multiplatform safe (yeah, I know it
>> shouldn't break non-5200 platforms, but it does have an undesirable
>> impact). =A0There's got to be a better way.
>
> What about putting the special memcpy in asm/io.h (like it is done for ee=
h)?

It is a beefy enough function that it is probably a net loss to make
it an inline static in the header file.  It should be an exported
function.

> Will this cause too much overhead for a memcpy which does not go to the L=
PB?

It doesn't solve my concerns.  bankwidth=3D=3D2 is the wrong test; plenty
of non-mpc5200 platforms could have the same property set without
needing the workaround.

inline_map_copy_to is used in exactly 2 places in drivers/mtd/* and
include/linux/mtd/*:
1) the definition of map_copy_to() in include/linux/mtd/map.h
- map_copy_to() is only used by maprap_write() which is a mtd_info hook.
2) the definition of simple_map_copy_to() in drivers/mtd/maps/map_funcs.c
- a map_info hook.

So; the solution to me seems to be on an MPC5200 platform replace the
offending hooks with MPC5200 specific variants at runtime.

g.

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

Re: [PATCH] powerpc/mpc52xx/mtd: fix mtd-ram access for 16-bit Local Plus Bus

[Albrecht Dreß]

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

Am 11.06.09 18:27 schrieb(en) Grant Likely:
>> +               *(u16 *)buf = *((volatile u16 *)(vdest - 1));
>> +               buf[1] = *((u8 *)src);
>> +               *((volatile u16 *)(vdest - 1)) = *(u16 *)buf;
> 
> what is the purpose of volatile here?  If you need io barriers, then  
> use the in_/out_be16 macros.

Yes, you're right - should be completely superfluous here.  A result of  
copy & paste without thinking... :-(

> Blech.  ugly #ifdef and not really multiplatform safe (yeah, I know  
> it shouldn't break non-5200 platforms, but it does have an  
> undesirable impact).  There's got to be a better way.

Ouch, yes - I completely forgot the possibility of multi-platform  
builds.

Am 11.06.09 19:28 schrieb(en) Grant Likely:
> So; the solution to me seems to be on an MPC5200 platform replace the  
> offending hooks with MPC5200 specific variants at runtime.

Will re-work the patch that way!  BTW, a dumb question: what is the  
proper way to determine which cpu the system is running on?  Check the  
CPU node of the of tree?

Thanks, Albrecht.

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

Re: [PATCH] powerpc/mpc52xx/mtd: fix mtd-ram access for 16-bit Local Plus Bus

[Grant Likely]

On Sat, Jun 13, 2009 at 10:45 AM, Albrecht Dre=DF<albrecht.dress@arcor.de> =
wrote:
> Am 11.06.09 18:27 schrieb(en) Grant Likely:
>>>
>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 *(u16 *)buf =3D *((volatile u16 *)(vdest =
- 1));
>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 buf[1] =3D *((u8 *)src);
>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 *((volatile u16 *)(vdest - 1)) =3D *(u16 =
*)buf;
>>
>> what is the purpose of volatile here? =A0If you need io barriers, then u=
se
>> the in_/out_be16 macros.
>
> Yes, you're right - should be completely superfluous here. =A0A result of=
 copy
> & paste without thinking... :-(
>
>> Blech. =A0ugly #ifdef and not really multiplatform safe (yeah, I know it
>> shouldn't break non-5200 platforms, but it does have an undesirable impa=
ct).
>> =A0There's got to be a better way.
>
> Ouch, yes - I completely forgot the possibility of multi-platform builds.
>
> Am 11.06.09 19:28 schrieb(en) Grant Likely:
>>
>> So; the solution to me seems to be on an MPC5200 platform replace the
>> offending hooks with MPC5200 specific variants at runtime.
>
> Will re-work the patch that way! =A0BTW, a dumb question: what is the pro=
per
> way to determine which cpu the system is running on? =A0Check the CPU nod=
e of
> the of tree?

In this case I'd walk up the parent nodes in the tree looking for an
"fsl,mpc5200-lpb" compatible value.  That way you only apply the
workaround if the device is on the locaplus bus, and not for example
below the PCI bus.

g.

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

Re: [PATCH] powerpc/mpc52xx/mtd: fix mtd-ram access for 16-bit Local Plus Bus

[David Woodhouse]

On Sat, 2009-06-13 at 18:45 +0200, Albrecht Dreß wrote:
> Am 11.06.09 19:28 schrieb(en) Grant Likely:
> > So; the solution to me seems to be on an MPC5200 platform replace the  
> > offending hooks with MPC5200 specific variants at runtime.
> 
> Will re-work the patch that way!  BTW, a dumb question: what is the  
> proper way to determine which cpu the system is running on?  Check the  
> CPU node of the of tree?

Surely the solution is for it to be a 'complex' mapping, where it can
provide its own I/O functions instead of polluting the inline versions
designed for 'simple' maps with special cases?

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation