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 WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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ß wrote:
> 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?

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.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

[Albrecht Dreß]

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.


pgpK0T238yLSx.pgp
Description: PGP signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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 wrote:
>> 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)?

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 LPB?

It doesn't solve my concerns.  bankwidth==2 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.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

[Wolfram Sang]

> 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/  |


signature.asc
Description: Digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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ß wrote:
> 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] 
> [2] 
>
> Signed-off-by: Albrecht Dreß 
> Cc: Grant Likely 
> Cc: David Woodhouse 
> Cc: linuxppc-...@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.0 +0200
> +++ linux-2.6.29.1/arch/powerpc/platforms/52xx/mpc52xx_common.c 2009-06-09
> 21:16:22.0 +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;

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

> +               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;

ditto.

> +       }
> +
> +       __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.0 +0200
> +++ linux-2.6.29.1/arch/powerpc/include/asm/mpc52xx.h   2009-06-09
> 21:14:31.0 +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.0 +0200
> +++ linux-2.6.29.1/include/linux/mtd/map.h      2009-06-08
> 14:28:05.0 +0200
> @@ -13,6 +13,9 @@
>  #include 
>  #include 
>  #include 
> +#ifdef CONFIG_PPC_MPC52xx
> +#include 
> +#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);
>  }

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.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozla

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

[Albrecht Dreß]

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]  


[2] 

Signed-off-by: Albrecht Dreß 
Cc: Grant Likely 
Cc: David Woodhouse 
Cc: linuxppc-...@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.0 +0200
+++ linux-2.6.29.1/arch/powerpc/platforms/52xx/mpc52xx_common.c  
2009-06-09 21:16:22.0 +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.0 +0200
+++ linux-2.6.29.1/arch/powerpc/include/asm/mpc52xx.h   2009-06-09  
21:14:31.0 +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.0 +0200
+++ linux-2.6.29.1/include/linux/mtd/map.h  2009-06-08  
14:28:05.0 +0200

@@ -13,6 +13,9 @@
 #include 
 #include 
 #include 
+#ifdef CONFIG_PPC_MPC52xx
+#include 
+#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);
 }



pgpVaj8YdBwdW.pgp
Description: PGP signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev