[PATCH] OMAP: use fncpy to copy the PM code functions to SRAM

[PATCH] OMAP: use fncpy to copy the PM code functions to SRAM

[jean.pihet at newoldbits.com]

From: Jean Pihet <j-pihet@ti.com>

The new fncpy API is better suited for copying some
code to SRAM at runtime. This patch changes the ad-hoc
code to the more generic fncpy API.

Tested OK on OMAP3 in low power modes (RET/OFF)
with !CONFIG_THUMB2_KERNEL

Signed-off-by: Jean Pihet <j-pihet@ti.com>
---
 arch/arm/plat-omap/sram.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/arm/plat-omap/sram.c b/arch/arm/plat-omap/sram.c
index e26e504..e2982b0 100644
--- a/arch/arm/plat-omap/sram.c
+++ b/arch/arm/plat-omap/sram.c
@@ -23,7 +23,7 @@
 
 #include <asm/tlb.h>
 #include <asm/cacheflush.h>
-
+#include <asm/fncpy.h>
 #include <asm/mach/map.h>
 
 #include <plat/sram.h>
@@ -251,9 +251,8 @@ void * omap_sram_push(void * start, unsigned long size)
 
 	omap_sram_ceil -= size;
 	omap_sram_ceil = ROUND_DOWN(omap_sram_ceil, sizeof(void *));
-	memcpy((void *)omap_sram_ceil, start, size);
-	flush_icache_range((unsigned long)omap_sram_ceil,
-		(unsigned long)(omap_sram_ceil + size));
+
+	fncpy((void *)omap_sram_ceil, start, size);
 
 	return (void *)omap_sram_ceil;
 }
-- 
1.7.2.3

[PATCH] OMAP: use fncpy to copy the PM code functions to SRAM

[Jean Pihet]

On Fri, Jan 14, 2011 at 4:21 PM,  <jean.pihet@newoldbits.com> wrote:
> From: Jean Pihet <j-pihet@ti.com>
>
> The new fncpy API is better suited for copying some
> code to SRAM at runtime. This patch changes the ad-hoc
> code to the more generic fncpy API.
>
> Tested OK on OMAP3 in low power modes (RET/OFF)
> with !CONFIG_THUMB2_KERNEL
>
> Signed-off-by: Jean Pihet <j-pihet@ti.com>

This patch depends on Dave's '[PATCH v3 1/1] ARM: Thumb-2: Symbol
manipulation macros for function' patch, cf.
http://marc.info/?l=linux-arm-kernel&m=129495865831165&w=2

Regards,
Jean

> ---
> ?arch/arm/plat-omap/sram.c | ? ?7 +++----
> ?1 files changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/plat-omap/sram.c b/arch/arm/plat-omap/sram.c
> index e26e504..e2982b0 100644
> --- a/arch/arm/plat-omap/sram.c
> +++ b/arch/arm/plat-omap/sram.c
> @@ -23,7 +23,7 @@
>
> ?#include <asm/tlb.h>
> ?#include <asm/cacheflush.h>
> -
> +#include <asm/fncpy.h>
> ?#include <asm/mach/map.h>
>
> ?#include <plat/sram.h>
> @@ -251,9 +251,8 @@ void * omap_sram_push(void * start, unsigned long size)
>
> ? ? ? ?omap_sram_ceil -= size;
> ? ? ? ?omap_sram_ceil = ROUND_DOWN(omap_sram_ceil, sizeof(void *));
> - ? ? ? memcpy((void *)omap_sram_ceil, start, size);
> - ? ? ? flush_icache_range((unsigned long)omap_sram_ceil,
> - ? ? ? ? ? ? ? (unsigned long)(omap_sram_ceil + size));
> +
> + ? ? ? fncpy((void *)omap_sram_ceil, start, size);
>
> ? ? ? ?return (void *)omap_sram_ceil;
> ?}
> --
> 1.7.2.3
>
>

[PATCH] OMAP: use fncpy to copy the PM code functions to SRAM

[Russell King - ARM Linux]

On Fri, Jan 14, 2011 at 04:21:10PM +0100, jean.pihet at newoldbits.com wrote:
> From: Jean Pihet <j-pihet@ti.com>
> 
> The new fncpy API is better suited for copying some
> code to SRAM at runtime. This patch changes the ad-hoc
> code to the more generic fncpy API.
> 
> Tested OK on OMAP3 in low power modes (RET/OFF)
> with !CONFIG_THUMB2_KERNEL
> 
> Signed-off-by: Jean Pihet <j-pihet@ti.com>
> ---
>  arch/arm/plat-omap/sram.c |    7 +++----
>  1 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/plat-omap/sram.c b/arch/arm/plat-omap/sram.c
> index e26e504..e2982b0 100644
> --- a/arch/arm/plat-omap/sram.c
> +++ b/arch/arm/plat-omap/sram.c
> @@ -23,7 +23,7 @@
>  
>  #include <asm/tlb.h>
>  #include <asm/cacheflush.h>
> -
> +#include <asm/fncpy.h>
>  #include <asm/mach/map.h>
>  
>  #include <plat/sram.h>
> @@ -251,9 +251,8 @@ void * omap_sram_push(void * start, unsigned long size)
>  
>  	omap_sram_ceil -= size;
>  	omap_sram_ceil = ROUND_DOWN(omap_sram_ceil, sizeof(void *));
> -	memcpy((void *)omap_sram_ceil, start, size);
> -	flush_icache_range((unsigned long)omap_sram_ceil,
> -		(unsigned long)(omap_sram_ceil + size));
> +
> +	fncpy((void *)omap_sram_ceil, start, size);
>  
>  	return (void *)omap_sram_ceil;

That's actually wrong usage, as you won't get the T bit set if the original
function had it.

The right solution to this is to change omap_sram_push() to become just an
allocator, and then use fncpy() outside of that.

So:

extern int my_func_size;
extern void my_func(int blah);
void (*sram_my_func)(int);

	void *sram = omap_sram_push(my_func_size);
	if (sram)
		sram_my_func = fncpy(sram, my_func, my_func_size);

Two benefits: 1. you get the thumb mode bit propagated (which is the
point of fncpy), and 2. you get the security of type safety between
my_func and the sram function pointer.

If you cast things to a void pointer and ignore the return value of fncpy
then you lose the whole point of this API _and_ any form of type safety.

[PATCH] OMAP: use fncpy to copy the PM code functions to SRAM

[Jean Pihet]

On Fri, Jan 14, 2011 at 4:58 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Jan 14, 2011 at 04:21:10PM +0100, jean.pihet at newoldbits.com wrote:
>> From: Jean Pihet <j-pihet@ti.com>
>>
>> The new fncpy API is better suited for copying some
>> code to SRAM at runtime. This patch changes the ad-hoc
>> code to the more generic fncpy API.
>>
>> Tested OK on OMAP3 in low power modes (RET/OFF)
>> with !CONFIG_THUMB2_KERNEL
>>
>> Signed-off-by: Jean Pihet <j-pihet@ti.com>
>> ---
>> ?arch/arm/plat-omap/sram.c | ? ?7 +++----
>> ?1 files changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm/plat-omap/sram.c b/arch/arm/plat-omap/sram.c
>> index e26e504..e2982b0 100644
>> --- a/arch/arm/plat-omap/sram.c
>> +++ b/arch/arm/plat-omap/sram.c
>> @@ -23,7 +23,7 @@
>>
>> ?#include <asm/tlb.h>
>> ?#include <asm/cacheflush.h>
>> -
>> +#include <asm/fncpy.h>
>> ?#include <asm/mach/map.h>
>>
>> ?#include <plat/sram.h>
>> @@ -251,9 +251,8 @@ void * omap_sram_push(void * start, unsigned long size)
>>
>> ? ? ? omap_sram_ceil -= size;
>> ? ? ? omap_sram_ceil = ROUND_DOWN(omap_sram_ceil, sizeof(void *));
>> - ? ? memcpy((void *)omap_sram_ceil, start, size);
>> - ? ? flush_icache_range((unsigned long)omap_sram_ceil,
>> - ? ? ? ? ? ? (unsigned long)(omap_sram_ceil + size));
>> +
>> + ? ? fncpy((void *)omap_sram_ceil, start, size);
>>
>> ? ? ? return (void *)omap_sram_ceil;
>
> That's actually wrong usage, as you won't get the T bit set if the original
> function had it.
Oops ok got it.

>
> The right solution to this is to change omap_sram_push() to become just an
> allocator, and then use fncpy() outside of that.
>
> So:
>
> extern int my_func_size;
> extern void my_func(int blah);
> void (*sram_my_func)(int);
>
> ? ? ? ?void *sram = omap_sram_push(my_func_size);
> ? ? ? ?if (sram)
> ? ? ? ? ? ? ? ?sram_my_func = fncpy(sram, my_func, my_func_size);
Is the name 'omap_sram_push' wrong then?
What about the following?
@@ -251,9 +251,8 @@ void * omap_sram_push(void * start, unsigned long size)

       omap_sram_ceil -= size;
       omap_sram_ceil = ROUND_DOWN(omap_sram_ceil, sizeof(void *));
 -     memcpy((void *)omap_sram_ceil, start, size);
 -     flush_icache_range((unsigned long)omap_sram_ceil,
 -             (unsigned long)(omap_sram_ceil + size));

 -     return (void *)omap_sram_ceil;
 +    return fncpy((void *)omap_sram_ceil, start, size);

> Two benefits: 1. you get the thumb mode bit propagated (which is the
> point of fncpy), and 2. you get the security of type safety between
> my_func and the sram function pointer.
>
> If you cast things to a void pointer and ignore the return value of fncpy
> then you lose the whole point of this API _and_ any form of type safety.
>

Thanks for reviewing the patch.

Regards,
Jean

[PATCH] OMAP: use fncpy to copy the PM code functions to SRAM

[Russell King - ARM Linux]

On Fri, Jan 14, 2011 at 05:13:01PM +0100, Jean Pihet wrote:
> Is the name 'omap_sram_push' wrong then?
> What about the following?
> @@ -251,9 +251,8 @@ void * omap_sram_push(void * start, unsigned long size)
> 
>        omap_sram_ceil -= size;
>        omap_sram_ceil = ROUND_DOWN(omap_sram_ceil, sizeof(void *));
>  -     memcpy((void *)omap_sram_ceil, start, size);
>  -     flush_icache_range((unsigned long)omap_sram_ceil,
>  -             (unsigned long)(omap_sram_ceil + size));
> 
>  -     return (void *)omap_sram_ceil;
>  +    return fncpy((void *)omap_sram_ceil, start, size);

It's more correct, but still missing out on the type safety which we've
tried to provide with fncpy.  Note also the other issue with Dave Martin
has raised though - this isn't ready for merging yet.

[PATCH] OMAP: use fncpy to copy the PM code functions to SRAM

[Jean Pihet]

On Fri, Jan 14, 2011 at 6:34 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Jan 14, 2011 at 05:13:01PM +0100, Jean Pihet wrote:
>> Is the name 'omap_sram_push' wrong then?
>> What about the following?
>> @@ -251,9 +251,8 @@ void * omap_sram_push(void * start, unsigned long size)
>>
>> ? ? ? ?omap_sram_ceil -= size;
>> ? ? ? ?omap_sram_ceil = ROUND_DOWN(omap_sram_ceil, sizeof(void *));
>> ?- ? ? memcpy((void *)omap_sram_ceil, start, size);
>> ?- ? ? flush_icache_range((unsigned long)omap_sram_ceil,
>> ?- ? ? ? ? ? ? (unsigned long)(omap_sram_ceil + size));
>>
>> ?- ? ? return (void *)omap_sram_ceil;
>> ?+ ? ?return fncpy((void *)omap_sram_ceil, start, size);
>
> It's more correct, but still missing out on the type safety which we've
> tried to provide with fncpy.
IIUC the type of the function is propagated from the 2nd argument
(funcp) to the return value, which is fine here. The (void)* is here
only to avoid a warning thrown by memcpy.

>?Note also the other issue with Dave Martin
> has raised though - this isn't ready for merging yet.
Ok I am using the latest version now and will re-spin the patch.

Regards,
Jean

[PATCH] OMAP: use fncpy to copy the PM code functions to SRAM

[Dave Martin]

On Mon, Jan 17, 2011 at 2:01 PM, Jean Pihet <jean.pihet@newoldbits.com> wrote:
> On Fri, Jan 14, 2011 at 6:34 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>> On Fri, Jan 14, 2011 at 05:13:01PM +0100, Jean Pihet wrote:
>>> Is the name 'omap_sram_push' wrong then?
>>> What about the following?
>>> @@ -251,9 +251,8 @@ void * omap_sram_push(void * start, unsigned long size)
>>>
>>> ? ? ? ?omap_sram_ceil -= size;
>>> ? ? ? ?omap_sram_ceil = ROUND_DOWN(omap_sram_ceil, sizeof(void *));
>>> ?- ? ? memcpy((void *)omap_sram_ceil, start, size);
>>> ?- ? ? flush_icache_range((unsigned long)omap_sram_ceil,
>>> ?- ? ? ? ? ? ? (unsigned long)(omap_sram_ceil + size));
>>>
>>> ?- ? ? return (void *)omap_sram_ceil;
>>> ?+ ? ?return fncpy((void *)omap_sram_ceil, start, size);
>>
>> It's more correct, but still missing out on the type safety which we've
>> tried to provide with fncpy.
> IIUC the type of the function is propagated from the 2nd argument
> (funcp) to the return value, which is fine here. The (void)* is here
> only to avoid a warning thrown by memcpy.

I think Russell's argument was that the compiler will not notice if
you mismatch the return type and the function to be copied, because
the casts to/from void * make the compiler blind to the real types
involved.  So:

int f(int);
size_t size_of_f;

void *buffer;
int (*_copied_f1)(int);
char *(*_copied_f2)(char *);

_copied_f1 = fncpy(buffer, f, size_of_f); /* OK */
_copied_f2 = fncpy(buffer, f, size_of_f); /* compile error */
_copied_f1 = omap_sram_push(f, size_of_f); /* OK */
_copied_f2 = opam_sram_push(f, size_of_f); /* type mismatch, but
compilation succeeds */


One way to work around this is would be to make omap_sram_push() a macro:

#define omap_sram_push(funcp, size) \
    (typeof(funcp))_do_omap_sram_push((void *)(funcp), size)

... where the definition of _do_omap_sram_push() is the same is the
existing definition of omap_sram_push().  Providing
_do_omap_sram_push() is not called directly, this should now be
type-safe.


Cheers
---Dave

[PATCH] OMAP: use fncpy to copy the PM code functions to SRAM

[Jean Pihet]

Dave, Russell,

On Mon, Jan 17, 2011 at 4:46 PM, Dave Martin <dave.martin@linaro.org> wrote:
> On Mon, Jan 17, 2011 at 2:01 PM, Jean Pihet <jean.pihet@newoldbits.com> wrote:
>> On Fri, Jan 14, 2011 at 6:34 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>>> On Fri, Jan 14, 2011 at 05:13:01PM +0100, Jean Pihet wrote:
>>>> Is the name 'omap_sram_push' wrong then?
>>>> What about the following?
>>>> @@ -251,9 +251,8 @@ void * omap_sram_push(void * start, unsigned long size)
>>>>
>>>> ? ? ? ?omap_sram_ceil -= size;
>>>> ? ? ? ?omap_sram_ceil = ROUND_DOWN(omap_sram_ceil, sizeof(void *));
>>>> ?- ? ? memcpy((void *)omap_sram_ceil, start, size);
>>>> ?- ? ? flush_icache_range((unsigned long)omap_sram_ceil,
>>>> ?- ? ? ? ? ? ? (unsigned long)(omap_sram_ceil + size));
>>>>
>>>> ?- ? ? return (void *)omap_sram_ceil;
>>>> ?+ ? ?return fncpy((void *)omap_sram_ceil, start, size);
>>>
>>> It's more correct, but still missing out on the type safety which we've
>>> tried to provide with fncpy.
>> IIUC the type of the function is propagated from the 2nd argument
>> (funcp) to the return value, which is fine here. The (void)* is here
>> only to avoid a warning thrown by memcpy.
>
> I think Russell's argument was that the compiler will not notice if
> you mismatch the return type and the function to be copied, because
> the casts to/from void * make the compiler blind to the real types
> involved. ?So:
>
> int f(int);
> size_t size_of_f;
>
> void *buffer;
> int (*_copied_f1)(int);
> char *(*_copied_f2)(char *);
>
> _copied_f1 = fncpy(buffer, f, size_of_f); /* OK */
> _copied_f2 = fncpy(buffer, f, size_of_f); /* compile error */
> _copied_f1 = omap_sram_push(f, size_of_f); /* OK */
> _copied_f2 = opam_sram_push(f, size_of_f); /* type mismatch, but
> compilation succeeds */
>
>
> One way to work around this is would be to make omap_sram_push() a macro:
>
> #define omap_sram_push(funcp, size) \
> ? ?(typeof(funcp))_do_omap_sram_push((void *)(funcp), size)
>
> ... where the definition of _do_omap_sram_push() is the same is the
> existing definition of omap_sram_push(). ?Providing
> _do_omap_sram_push() is not called directly, this should now be
> type-safe.
>
Ok I reworked the patch from your suggestions. Indeed a few functions
types mismatch have been spotted and corrected using the fncpy API.

New patch sent as '[PATCH v2] OMAP: use fncpy to copy the PM code
functions to SRAM'.
>
> Cheers
> ---Dave
>

Thanks,
Jean

[PATCH] OMAP: use fncpy to copy the PM code functions to SRAM

[Russell King - ARM Linux]

On Tue, Jan 18, 2011 at 01:05:49PM +0100, Jean Pihet wrote:
> Dave, Russell,
> 
> On Mon, Jan 17, 2011 at 4:46 PM, Dave Martin <dave.martin@linaro.org> wrote:
> > One way to work around this is would be to make omap_sram_push() a macro:
> >
> > #define omap_sram_push(funcp, size) \
> > ? ?(typeof(funcp))_do_omap_sram_push((void *)(funcp), size)
> >
> > ... where the definition of _do_omap_sram_push() is the same is the
> > existing definition of omap_sram_push(). ?Providing
> > _do_omap_sram_push() is not called directly, this should now be
> > type-safe.
> >
> Ok I reworked the patch from your suggestions. Indeed a few functions
> types mismatch have been spotted and corrected using the fncpy API.
> 
> New patch sent as '[PATCH v2] OMAP: use fncpy to copy the PM code
> functions to SRAM'.

Looks good, thanks.  Next problem to sort out is who's taking the
patches...

[PATCH] OMAP: use fncpy to copy the PM code functions to SRAM

[Tony Lindgren]

* Russell King - ARM Linux <linux@arm.linux.org.uk> [110118 15:41]:
> On Tue, Jan 18, 2011 at 01:05:49PM +0100, Jean Pihet wrote:
> > Dave, Russell,
> > 
> > On Mon, Jan 17, 2011 at 4:46 PM, Dave Martin <dave.martin@linaro.org> wrote:
> > > One way to work around this is would be to make omap_sram_push() a macro:
> > >
> > > #define omap_sram_push(funcp, size) \
> > > ? ?(typeof(funcp))_do_omap_sram_push((void *)(funcp), size)
> > >
> > > ... where the definition of _do_omap_sram_push() is the same is the
> > > existing definition of omap_sram_push(). ?Providing
> > > _do_omap_sram_push() is not called directly, this should now be
> > > type-safe.
> > >
> > Ok I reworked the patch from your suggestions. Indeed a few functions
> > types mismatch have been spotted and corrected using the fncpy API.
> > 
> > New patch sent as '[PATCH v2] OMAP: use fncpy to copy the PM code
> > functions to SRAM'.
> 
> Looks good, thanks.  Next problem to sort out is who's taking the
> patches...

You can take them but we should have at least Kevin test and ack them.

Tony

[PATCH] OMAP: use fncpy to copy the PM code functions to SRAM

[Jean Pihet]

On Wed, Jan 19, 2011 at 12:44 AM, Tony Lindgren <tony@atomide.com> wrote:
> * Russell King - ARM Linux <linux@arm.linux.org.uk> [110118 15:41]:
>> On Tue, Jan 18, 2011 at 01:05:49PM +0100, Jean Pihet wrote:
>> > Dave, Russell,
>> >
>> > On Mon, Jan 17, 2011 at 4:46 PM, Dave Martin <dave.martin@linaro.org> wrote:
>> > > One way to work around this is would be to make omap_sram_push() a macro:
>> > >
>> > > #define omap_sram_push(funcp, size) \
>> > > ? ?(typeof(funcp))_do_omap_sram_push((void *)(funcp), size)
>> > >
>> > > ... where the definition of _do_omap_sram_push() is the same is the
>> > > existing definition of omap_sram_push(). ?Providing
>> > > _do_omap_sram_push() is not called directly, this should now be
>> > > type-safe.
>> > >
>> > Ok I reworked the patch from your suggestions. Indeed a few functions
>> > types mismatch have been spotted and corrected using the fncpy API.
>> >
>> > New patch sent as '[PATCH v2] OMAP: use fncpy to copy the PM code
>> > functions to SRAM'.
>>
>> Looks good, thanks. ?Next problem to sort out is who's taking the
>> patches...
>
> You can take them but we should have at least Kevin test and ack them.
Sure, this needs some testing on OMAP1 & 2 platforms. It has only been
compile tested on those (means: compile OK, functions types mismatches
fixed).

Anyone with OMAP1 & 2 boards willing to test?

Thanks,
Jean

>
> Tony
>

[PATCH] OMAP: use fncpy to copy the PM code functions to SRAM

[Tony Lindgren]

* Jean Pihet <jean.pihet@newoldbits.com> [110119 00:05]:
> On Wed, Jan 19, 2011 at 12:44 AM, Tony Lindgren <tony@atomide.com> wrote:
> > * Russell King - ARM Linux <linux@arm.linux.org.uk> [110118 15:41]:
> >> On Tue, Jan 18, 2011 at 01:05:49PM +0100, Jean Pihet wrote:
> >> > Dave, Russell,
> >> >
> >> > On Mon, Jan 17, 2011 at 4:46 PM, Dave Martin <dave.martin@linaro.org> wrote:
> >> > > One way to work around this is would be to make omap_sram_push() a macro:
> >> > >
> >> > > #define omap_sram_push(funcp, size) \
> >> > > ? ?(typeof(funcp))_do_omap_sram_push((void *)(funcp), size)
> >> > >
> >> > > ... where the definition of _do_omap_sram_push() is the same is the
> >> > > existing definition of omap_sram_push(). ?Providing
> >> > > _do_omap_sram_push() is not called directly, this should now be
> >> > > type-safe.
> >> > >
> >> > Ok I reworked the patch from your suggestions. Indeed a few functions
> >> > types mismatch have been spotted and corrected using the fncpy API.
> >> >
> >> > New patch sent as '[PATCH v2] OMAP: use fncpy to copy the PM code
> >> > functions to SRAM'.
> >>
> >> Looks good, thanks. ?Next problem to sort out is who's taking the
> >> patches...
> >
> > You can take them but we should have at least Kevin test and ack them.
> Sure, this needs some testing on OMAP1 & 2 platforms. It has only been
> compile tested on those (means: compile OK, functions types mismatches
> fixed).
> 
> Anyone with OMAP1 & 2 boards willing to test?

Can you please repost the whole set one more time or have them in
some git branch? That way I can pull them into linux-omap master
branch for testing to make sure omap1 and 2 boards don't break.

Tony

[PATCH] OMAP: use fncpy to copy the PM code functions to SRAM

[Jean Pihet]

Hi Tony,

On Wed, Jan 19, 2011 at 8:10 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Jean Pihet <jean.pihet@newoldbits.com> [110119 00:05]:
>> On Wed, Jan 19, 2011 at 12:44 AM, Tony Lindgren <tony@atomide.com> wrote:
>> > * Russell King - ARM Linux <linux@arm.linux.org.uk> [110118 15:41]:
>> >> On Tue, Jan 18, 2011 at 01:05:49PM +0100, Jean Pihet wrote:
>> >> > Dave, Russell,
>> >> >
>> >> > On Mon, Jan 17, 2011 at 4:46 PM, Dave Martin <dave.martin@linaro.org> wrote:
>> >> > > One way to work around this is would be to make omap_sram_push() a macro:
>> >> > >
>> >> > > #define omap_sram_push(funcp, size) \
>> >> > > ? ?(typeof(funcp))_do_omap_sram_push((void *)(funcp), size)
>> >> > >
>> >> > > ... where the definition of _do_omap_sram_push() is the same is the
>> >> > > existing definition of omap_sram_push(). ?Providing
>> >> > > _do_omap_sram_push() is not called directly, this should now be
>> >> > > type-safe.
>> >> > >
>> >> > Ok I reworked the patch from your suggestions. Indeed a few functions
>> >> > types mismatch have been spotted and corrected using the fncpy API.
>> >> >
>> >> > New patch sent as '[PATCH v2] OMAP: use fncpy to copy the PM code
>> >> > functions to SRAM'.
>> >>
>> >> Looks good, thanks. ?Next problem to sort out is who's taking the
>> >> patches...
>> >
>> > You can take them but we should have at least Kevin test and ack them.
>> Sure, this needs some testing on OMAP1 & 2 platforms. It has only been
>> compile tested on those (means: compile OK, functions types mismatches
>> fixed).
>>
>> Anyone with OMAP1 & 2 boards willing to test?
>
> Can you please repost the whole set one more time or have them in
> some git branch? That way I can pull them into linux-omap master
> branch for testing to make sure omap1 and 2 boards don't break.
There is only patch to apply: '[PATCH v2] OMAP: use fncpy to copy the
PM code functions to SRAM'
(http://marc.info/?l=linux-omap&m=129535214414192&w=2) which depends
on Dave Martin's ([PATCH v4] ARM: Thumb-2: Symbol manipulation macros
for function body copying'
(http://marc.info/?l=linux-omap&m=129503990527072&w=2).

Is this enough? If not I can have them in a branch of a gitorious tree.

>
> Tony
>

Thanks,
Jean

[PATCH] OMAP: use fncpy to copy the PM code functions to SRAM

[Tony Lindgren]

* Jean Pihet <jean.pihet@newoldbits.com> [110119 13:36]:
> Hi Tony,
> 
> On Wed, Jan 19, 2011 at 8:10 PM, Tony Lindgren <tony@atomide.com> wrote:
> > * Jean Pihet <jean.pihet@newoldbits.com> [110119 00:05]:
> >> On Wed, Jan 19, 2011 at 12:44 AM, Tony Lindgren <tony@atomide.com> wrote:
> >> > * Russell King - ARM Linux <linux@arm.linux.org.uk> [110118 15:41]:
> >> >> On Tue, Jan 18, 2011 at 01:05:49PM +0100, Jean Pihet wrote:
> >> >> > Dave, Russell,
> >> >> >
> >> >> > On Mon, Jan 17, 2011 at 4:46 PM, Dave Martin <dave.martin@linaro.org> wrote:
> >> >> > > One way to work around this is would be to make omap_sram_push() a macro:
> >> >> > >
> >> >> > > #define omap_sram_push(funcp, size) \
> >> >> > > ? ?(typeof(funcp))_do_omap_sram_push((void *)(funcp), size)
> >> >> > >
> >> >> > > ... where the definition of _do_omap_sram_push() is the same is the
> >> >> > > existing definition of omap_sram_push(). ?Providing
> >> >> > > _do_omap_sram_push() is not called directly, this should now be
> >> >> > > type-safe.
> >> >> > >
> >> >> > Ok I reworked the patch from your suggestions. Indeed a few functions
> >> >> > types mismatch have been spotted and corrected using the fncpy API.
> >> >> >
> >> >> > New patch sent as '[PATCH v2] OMAP: use fncpy to copy the PM code
> >> >> > functions to SRAM'.
> >> >>
> >> >> Looks good, thanks. ?Next problem to sort out is who's taking the
> >> >> patches...
> >> >
> >> > You can take them but we should have at least Kevin test and ack them.
> >> Sure, this needs some testing on OMAP1 & 2 platforms. It has only been
> >> compile tested on those (means: compile OK, functions types mismatches
> >> fixed).
> >>
> >> Anyone with OMAP1 & 2 boards willing to test?
> >
> > Can you please repost the whole set one more time or have them in
> > some git branch? That way I can pull them into linux-omap master
> > branch for testing to make sure omap1 and 2 boards don't break.
> There is only patch to apply: '[PATCH v2] OMAP: use fncpy to copy the
> PM code functions to SRAM'
> (http://marc.info/?l=linux-omap&m=129535214414192&w=2) which depends
> on Dave Martin's ([PATCH v4] ARM: Thumb-2: Symbol manipulation macros
> for function body copying'
> (http://marc.info/?l=linux-omap&m=129503990527072&w=2).
> 
> Is this enough? If not I can have them in a branch of a gitorious tree.

OK, will apply them into omap-testing. Then hopefully within next
few days Russell can set up some immutable branch that we can merge
too as some PM patches may conflict with these.

Regards,

Tony

[PATCH] omap: use fncpy to copy the PM code functions to SRAM

[jean.pihet at newoldbits.com]

From: Jean Pihet <j-pihet@ti.com>

The new fncpy API is better suited* for copying some
code to SRAM at runtime. This patch changes the ad-hoc
code to the more generic fncpy API.

*: 1. fncpy ensures that the thumb mode bit is propagated,
   2. fncpy provides the security of type safety between the
     original function and the sram function pointer.

Tested OK on OMAP3 in low power modes (RET/OFF)
using omap2plus_defconfig with !CONFIG_THUMB2_KERNEL.
Compile tested on OMAP1/2 using omap1_defconfig.

Signed-off-by: Jean Pihet <j-pihet@ti.com>
Acked-by: Kevin Hilman <khilman@ti.com>
Acked-by: Tony Lindgren <tony@atomide.com>
Reviewed-by: Dave Martin <dave.martin@linaro.org>

Boot tested on OMAP1 & OMAP2
Tested OK with suspend/resume on OMAP2420/n810
Tested-by: Kevin Hilman <khilman@ti.com>

Boots fine on osk5912 and n800
Tested-by: Tony Lindgren <tony@atomide.com>
---
 arch/arm/mach-omap1/pm.h               |    6 +++---
 arch/arm/mach-omap1/sleep.S            |    3 +++
 arch/arm/mach-omap1/sram.S             |    1 +
 arch/arm/mach-omap2/pm.h               |    2 +-
 arch/arm/mach-omap2/sleep24xx.S        |    2 ++
 arch/arm/mach-omap2/sleep34xx.S        |    2 ++
 arch/arm/mach-omap2/sram242x.S         |    3 +++
 arch/arm/mach-omap2/sram243x.S         |    3 +++
 arch/arm/mach-omap2/sram34xx.S         |    1 +
 arch/arm/plat-omap/include/plat/sram.h |   14 +++++++++++++-
 arch/arm/plat-omap/sram.c              |   14 +++++++++-----
 11 files changed, 41 insertions(+), 10 deletions(-)

diff --git a/arch/arm/mach-omap1/pm.h b/arch/arm/mach-omap1/pm.h
index 56a6479..cd926dc 100644
--- a/arch/arm/mach-omap1/pm.h
+++ b/arch/arm/mach-omap1/pm.h
@@ -123,9 +123,9 @@ extern void allow_idle_sleep(void);
 extern void omap1_pm_idle(void);
 extern void omap1_pm_suspend(void);
 
-extern void omap7xx_cpu_suspend(unsigned short, unsigned short);
-extern void omap1510_cpu_suspend(unsigned short, unsigned short);
-extern void omap1610_cpu_suspend(unsigned short, unsigned short);
+extern void omap7xx_cpu_suspend(unsigned long, unsigned long);
+extern void omap1510_cpu_suspend(unsigned long, unsigned long);
+extern void omap1610_cpu_suspend(unsigned long, unsigned long);
 extern void omap7xx_idle_loop_suspend(void);
 extern void omap1510_idle_loop_suspend(void);
 extern void omap1610_idle_loop_suspend(void);
diff --git a/arch/arm/mach-omap1/sleep.S b/arch/arm/mach-omap1/sleep.S
index ef771ce..c875bdc 100644
--- a/arch/arm/mach-omap1/sleep.S
+++ b/arch/arm/mach-omap1/sleep.S
@@ -58,6 +58,7 @@
  */
 
 #if defined(CONFIG_ARCH_OMAP730) || defined(CONFIG_ARCH_OMAP850)
+	.align	3
 ENTRY(omap7xx_cpu_suspend)
 
 	@ save registers on stack
@@ -137,6 +138,7 @@ ENTRY(omap7xx_cpu_suspend_sz)
 #endif /* CONFIG_ARCH_OMAP730 || CONFIG_ARCH_OMAP850 */
 
 #ifdef CONFIG_ARCH_OMAP15XX
+	.align	3
 ENTRY(omap1510_cpu_suspend)
 
 	@ save registers on stack
@@ -211,6 +213,7 @@ ENTRY(omap1510_cpu_suspend_sz)
 #endif /* CONFIG_ARCH_OMAP15XX */
 
 #if defined(CONFIG_ARCH_OMAP16XX)
+	.align	3
 ENTRY(omap1610_cpu_suspend)
 
 	@ save registers on stack
diff --git a/arch/arm/mach-omap1/sram.S b/arch/arm/mach-omap1/sram.S
index 7724e52..692587d 100644
--- a/arch/arm/mach-omap1/sram.S
+++ b/arch/arm/mach-omap1/sram.S
@@ -18,6 +18,7 @@
 /*
  * Reprograms ULPD and CKCTL.
  */
+	.align	3
 ENTRY(omap1_sram_reprogram_clock)
 	stmfd	sp!, {r0 - r12, lr}		@ save registers on stack
 
diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
index 1c1b0ab..39580e6 100644
--- a/arch/arm/mach-omap2/pm.h
+++ b/arch/arm/mach-omap2/pm.h
@@ -92,7 +92,7 @@ extern void omap24xx_idle_loop_suspend(void);
 extern void omap24xx_cpu_suspend(u32 dll_ctrl, void __iomem *sdrc_dlla_ctrl,
 					void __iomem *sdrc_power);
 extern void omap34xx_cpu_suspend(u32 *addr, int save_state);
-extern void save_secure_ram_context(u32 *addr);
+extern int save_secure_ram_context(u32 *addr);
 extern void omap3_save_scratchpad_contents(void);
 
 extern unsigned int omap24xx_idle_loop_suspend_sz;
diff --git a/arch/arm/mach-omap2/sleep24xx.S b/arch/arm/mach-omap2/sleep24xx.S
index c7780cc..b5071a4 100644
--- a/arch/arm/mach-omap2/sleep24xx.S
+++ b/arch/arm/mach-omap2/sleep24xx.S
@@ -47,6 +47,7 @@
  * Note: This code get's copied to internal SRAM at boot. When the OMAP
  *	 wakes up it continues execution at the point it went to sleep.
  */
+	.align	3
 ENTRY(omap24xx_idle_loop_suspend)
 	stmfd	sp!, {r0, lr}		@ save registers on stack
 	mov	r0, #0			@ clear for mcr setup
@@ -82,6 +83,7 @@ ENTRY(omap24xx_idle_loop_suspend_sz)
  * The DLL load value is not kept in RETENTION or OFF.	It needs to be restored
  * at wake
  */
+	.align	3
 ENTRY(omap24xx_cpu_suspend)
 	stmfd	sp!, {r0 - r12, lr}	@ save registers on stack
 	mov	r3, #0x0		@ clear for mcr call
diff --git a/arch/arm/mach-omap2/sleep34xx.S b/arch/arm/mach-omap2/sleep34xx.S
index 98d8232..951a0be 100644
--- a/arch/arm/mach-omap2/sleep34xx.S
+++ b/arch/arm/mach-omap2/sleep34xx.S
@@ -118,6 +118,7 @@ ENTRY(enable_omap3630_toggle_l2_on_restore)
 
 	.text
 /* Function to call rom code to save secure ram context */
+	.align	3
 ENTRY(save_secure_ram_context)
 	stmfd	sp!, {r1-r12, lr}	@ save registers on stack
 	adr	r3, api_params		@ r3 points to parameters
@@ -169,6 +170,7 @@ ENTRY(save_secure_ram_context_sz)
  *   depending on the low power mode (non-OFF vs OFF modes),
  *   cf. 'Resume path for xxx mode' comments.
  */
+	.align	3
 ENTRY(omap34xx_cpu_suspend)
 	stmfd	sp!, {r0-r12, lr}	@ save registers on stack
 
diff --git a/arch/arm/mach-omap2/sram242x.S b/arch/arm/mach-omap2/sram242x.S
index 055310c..ff9b9db 100644
--- a/arch/arm/mach-omap2/sram242x.S
+++ b/arch/arm/mach-omap2/sram242x.S
@@ -39,6 +39,7 @@
 
 	.text
 
+	.align	3
 ENTRY(omap242x_sram_ddr_init)
 	stmfd	sp!, {r0 - r12, lr}	@ save registers on stack
 
@@ -143,6 +144,7 @@ ENTRY(omap242x_sram_ddr_init_sz)
  * r0 = [PRCM_FULL | PRCM_HALF] r1 = SDRC_DLLA_CTRL value r2 = [DDR | SDR]
  * PRCM_FULL = 2, PRCM_HALF = 1, DDR = 1, SDR = 0
  */
+	.align	3
 ENTRY(omap242x_sram_reprogram_sdrc)
 	stmfd	sp!, {r0 - r10, lr}	@ save registers on stack
 	mov	r3, #0x0		@ clear for mrc call
@@ -238,6 +240,7 @@ ENTRY(omap242x_sram_reprogram_sdrc_sz)
 /*
  * Set dividers and pll. Also recalculate DLL value for DDR and unlock mode.
  */
+	.align	3
 ENTRY(omap242x_sram_set_prcm)
 	stmfd	sp!, {r0-r12, lr}	@ regs to stack
 	adr	r4, pbegin		@ addr of preload start
diff --git a/arch/arm/mach-omap2/sram243x.S b/arch/arm/mach-omap2/sram243x.S
index f900758..7673020 100644
--- a/arch/arm/mach-omap2/sram243x.S
+++ b/arch/arm/mach-omap2/sram243x.S
@@ -39,6 +39,7 @@
 
 	.text
 
+	.align	3
 ENTRY(omap243x_sram_ddr_init)
 	stmfd	sp!, {r0 - r12, lr}	@ save registers on stack
 
@@ -143,6 +144,7 @@ ENTRY(omap243x_sram_ddr_init_sz)
  * r0 = [PRCM_FULL | PRCM_HALF] r1 = SDRC_DLLA_CTRL value r2 = [DDR | SDR]
  * PRCM_FULL = 2, PRCM_HALF = 1, DDR = 1, SDR = 0
  */
+	.align	3
 ENTRY(omap243x_sram_reprogram_sdrc)
 	stmfd	sp!, {r0 - r10, lr}	@ save registers on stack
 	mov	r3, #0x0		@ clear for mrc call
@@ -238,6 +240,7 @@ ENTRY(omap243x_sram_reprogram_sdrc_sz)
 /*
  * Set dividers and pll. Also recalculate DLL value for DDR and unlock mode.
  */
+	.align	3
 ENTRY(omap243x_sram_set_prcm)
 	stmfd	sp!, {r0-r12, lr}	@ regs to stack
 	adr	r4, pbegin		@ addr of preload start
diff --git a/arch/arm/mach-omap2/sram34xx.S b/arch/arm/mach-omap2/sram34xx.S
index 7f893a2..25011ca 100644
--- a/arch/arm/mach-omap2/sram34xx.S
+++ b/arch/arm/mach-omap2/sram34xx.S
@@ -111,6 +111,7 @@
  * since it will cause the ARM MMU to attempt to walk the page tables.
  * These crashes may be intermittent.
  */
+	.align	3
 ENTRY(omap3_sram_configure_core_dpll)
 	stmfd	sp!, {r1-r12, lr}	@ store regs to stack
 
diff --git a/arch/arm/plat-omap/include/plat/sram.h b/arch/arm/plat-omap/include/plat/sram.h
index 9967d5e..f500fc3 100644
--- a/arch/arm/plat-omap/include/plat/sram.h
+++ b/arch/arm/plat-omap/include/plat/sram.h
@@ -12,7 +12,19 @@
 #define __ARCH_ARM_OMAP_SRAM_H
 
 #ifndef __ASSEMBLY__
-extern void * omap_sram_push(void * start, unsigned long size);
+#include <asm/fncpy.h>
+
+extern void *omap_sram_push_address(unsigned long size);
+
+/* Macro to push a function to the internal SRAM, using the fncpy API */
+#define omap_sram_push(funcp, size) ({				\
+	typeof(&(funcp)) _res = NULL;				\
+	void *_sram_address = omap_sram_push_address(size);	\
+	if (_sram_address)					\
+		_res = fncpy(_sram_address, &(funcp), size);	\
+	_res;							\
+})
+
 extern void omap_sram_reprogram_clock(u32 dpllctl, u32 ckctl);
 
 extern void omap2_sram_ddr_init(u32 *slow_dll_ctrl, u32 fast_dll_ctrl,
diff --git a/arch/arm/plat-omap/sram.c b/arch/arm/plat-omap/sram.c
index aedcb3b..916c662 100644
--- a/arch/arm/plat-omap/sram.c
+++ b/arch/arm/plat-omap/sram.c
@@ -242,7 +242,14 @@ static void __init omap_map_sram(void)
 	       omap_sram_size - SRAM_BOOTLOADER_SZ);
 }
 
-void * omap_sram_push(void * start, unsigned long size)
+/*
+ * Memory allocator for SRAM: calculates the new ceiling address
+ * for pushing a function using the fncpy API.
+ *
+ * Note that fncpy requires the returned address to be aligned
+ * to an 8-byte boundary.
+ */
+void *omap_sram_push_address(unsigned long size)
 {
 	if (size > (omap_sram_ceil - (omap_sram_base + SRAM_BOOTLOADER_SZ))) {
 		printk(KERN_ERR "Not enough space in SRAM\n");
@@ -250,10 +257,7 @@ void * omap_sram_push(void * start, unsigned long size)
 	}
 
 	omap_sram_ceil -= size;
-	omap_sram_ceil = ROUND_DOWN(omap_sram_ceil, sizeof(void *));
-	memcpy((void *)omap_sram_ceil, start, size);
-	flush_icache_range((unsigned long)omap_sram_ceil,
-		(unsigned long)(omap_sram_ceil + size));
+	omap_sram_ceil = ROUND_DOWN(omap_sram_ceil, FNCPY_ALIGN);
 
 	return (void *)omap_sram_ceil;
 }
-- 
1.7.2.3