[U-Boot] [PATCH V2] i.MX28: Drop __naked function from spl_mem_init

[U-Boot] [PATCH V2] i.MX28: Drop __naked function from spl_mem_init

[Marek Vasut]

Instead of compiling the function and using the result as a constant, simply use
the constant.

NOTE: This patch works around bug:

  http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52546

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Stefano Babic <sbabic@denx.de>
Cc: Tom Rini <trini@ti.com>
---
 arch/arm/cpu/arm926ejs/mx28/spl_mem_init.c |   10 +++-------
 1 files changed, 3 insertions(+), 7 deletions(-)

V2: Add comment that this works around bug in GCC

diff --git a/arch/arm/cpu/arm926ejs/mx28/spl_mem_init.c b/arch/arm/cpu/arm926ejs/mx28/spl_mem_init.c
index 43a90ff..911bbef 100644
--- a/arch/arm/cpu/arm926ejs/mx28/spl_mem_init.c
+++ b/arch/arm/cpu/arm926ejs/mx28/spl_mem_init.c
@@ -173,22 +173,18 @@ void mx28_mem_setup_vddd(void)
 		&power_regs->hw_power_vdddctrl);
 }
 
-void data_abort_memdetect_handler(void) __attribute__((naked));
-void data_abort_memdetect_handler(void)
-{
-	asm volatile("subs pc, r14, #4");
-}
-
 void mx28_mem_get_size(void)
 {
 	struct mx28_digctl_regs *digctl_regs =
 		(struct mx28_digctl_regs *)MXS_DIGCTL_BASE;
 	uint32_t sz, da;
 	uint32_t *vt = (uint32_t *)0x20;
+	/* The following is "subs pc, r14, #4", used as return from DABT. */
+	const uint32_t data_abort_memdetect_handler = 0xe25ef004;
 
 	/* Replace the DABT handler. */
 	da = vt[4];
-	vt[4] = (uint32_t)&data_abort_memdetect_handler;
+	vt[4] = data_abort_memdetect_handler;
 
 	sz = get_ram_size((long *)PHYS_SDRAM_1, PHYS_SDRAM_1_SIZE);
 	writel(sz, &digctl_regs->hw_digctl_scratch0);
-- 
1.7.9

[U-Boot] [PATCH V2] i.MX28: Drop __naked function from spl_mem_init

[Stefano Babic]

On 16/03/2012 22:32, Marek Vasut wrote:
> Instead of compiling the function and using the result as a constant, simply use
> the constant.
> 
> NOTE: This patch works around bug:
> 
>   http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52546
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Stefano Babic <sbabic@denx.de>
> Cc: Tom Rini <trini@ti.com>
> ---

Hi Marek,

>  arch/arm/cpu/arm926ejs/mx28/spl_mem_init.c |   10 +++-------
>  1 files changed, 3 insertions(+), 7 deletions(-)
> 
> V2: Add comment that this works around bug in GCC
> 
> diff --git a/arch/arm/cpu/arm926ejs/mx28/spl_mem_init.c b/arch/arm/cpu/arm926ejs/mx28/spl_mem_init.c
> index 43a90ff..911bbef 100644
> --- a/arch/arm/cpu/arm926ejs/mx28/spl_mem_init.c
> +++ b/arch/arm/cpu/arm926ejs/mx28/spl_mem_init.c
> @@ -173,22 +173,18 @@ void mx28_mem_setup_vddd(void)
>  		&power_regs->hw_power_vdddctrl);
>  }
>  
> -void data_abort_memdetect_handler(void) __attribute__((naked));
> -void data_abort_memdetect_handler(void)
> -{
> -	asm volatile("subs pc, r14, #4");
> -}
> -
>  void mx28_mem_get_size(void)
>  {
>  	struct mx28_digctl_regs *digctl_regs =
>  		(struct mx28_digctl_regs *)MXS_DIGCTL_BASE;
>  	uint32_t sz, da;
>  	uint32_t *vt = (uint32_t *)0x20;
> +	/* The following is "subs pc, r14, #4", used as return from DABT. */
> +	const uint32_t data_abort_memdetect_handler = 0xe25ef004;

Are we maybe becoming warning addicted ? I know the reason for this (GCC
raises a warning "-fstack-usage not supported for this target"), you
have already asked the gcc people about this issue, and I do not have an
idea how to fix this warning in a different way as you did. This is a
sort of self-modifying code.

However, the original code is quite easy to understand - I cannot say
the same after the patch, we rely on the comment to understand something.

Should we really fix such as warnings even if we generate some obscured
code ? Wolfgang, what do you think about ?

Regards,
Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] [PATCH V2] i.MX28: Drop __naked function from spl_mem_init

[Marek Vasut]

Dear Stefano Babic,

> On 16/03/2012 22:32, Marek Vasut wrote:
> > Instead of compiling the function and using the result as a constant,
> > simply use the constant.
> > 
> > NOTE: This patch works around bug:
> >   http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52546
> > 
> > Signed-off-by: Marek Vasut <marex@denx.de>
> > Cc: Stefano Babic <sbabic@denx.de>
> > Cc: Tom Rini <trini@ti.com>
> > ---
> 
> Hi Marek,
> 
> >  arch/arm/cpu/arm926ejs/mx28/spl_mem_init.c |   10 +++-------
> >  1 files changed, 3 insertions(+), 7 deletions(-)
> > 
> > V2: Add comment that this works around bug in GCC
> > 
> > diff --git a/arch/arm/cpu/arm926ejs/mx28/spl_mem_init.c
> > b/arch/arm/cpu/arm926ejs/mx28/spl_mem_init.c index 43a90ff..911bbef
> > 100644
> > --- a/arch/arm/cpu/arm926ejs/mx28/spl_mem_init.c
> > +++ b/arch/arm/cpu/arm926ejs/mx28/spl_mem_init.c
> > @@ -173,22 +173,18 @@ void mx28_mem_setup_vddd(void)
> > 
> >  		&power_regs->hw_power_vdddctrl);
> >  
> >  }
> > 
> > -void data_abort_memdetect_handler(void) __attribute__((naked));
> > -void data_abort_memdetect_handler(void)
> > -{
> > -	asm volatile("subs pc, r14, #4");
> > -}
> > -
> > 
> >  void mx28_mem_get_size(void)
> >  {
> >  
> >  	struct mx28_digctl_regs *digctl_regs =
> >  	
> >  		(struct mx28_digctl_regs *)MXS_DIGCTL_BASE;
> >  	
> >  	uint32_t sz, da;
> >  	uint32_t *vt = (uint32_t *)0x20;
> > 
> > +	/* The following is "subs pc, r14, #4", used as return from DABT. */
> > +	const uint32_t data_abort_memdetect_handler = 0xe25ef004;
> 
> Are we maybe becoming warning addicted ? I know the reason for this (GCC
> raises a warning "-fstack-usage not supported for this target"), you
> have already asked the gcc people about this issue, and I do not have an
> idea how to fix this warning in a different way as you did. This is a
> sort of self-modifying code.

I have an idea -- patch GCC >:-) Which is exactly what I'm gonna do when I have 
time ^H^H^H^H^H^H^H^H^H completely loose it :)

> 
> However, the original code is quite easy to understand - I cannot say
> the same after the patch, we rely on the comment to understand something.

Sadly, yes.

> 
> Should we really fix such as warnings even if we generate some obscured
> code ? Wolfgang, what do you think about ?

It generates warnings in our jenkins CI.

> 
> Regards,
> Stefano

Best regards,
Marek Vasut

[U-Boot] [PATCH V2] i.MX28: Drop __naked function from spl_mem_init

[Wolfgang Denk]

Dear Stefano,

In message <4F683862.4030709@denx.de> you wrote:
> 
> > +	/* The following is "subs pc, r14, #4", used as return from DABT. */
> > +	const uint32_t data_abort_memdetect_handler = 0xe25ef004;
...
> Are we maybe becoming warning addicted ? I know the reason for this (GCC
> raises a warning "-fstack-usage not supported for this target"), you
> have already asked the gcc people about this issue, and I do not have an
> idea how to fix this warning in a different way as you did. This is a
> sort of self-modifying code.

In which way is this self-modifying code? I don't think so.

> However, the original code is quite easy to understand - I cannot say
> the same after the patch, we rely on the comment to understand something.

That's what comments are made for :-)

> Should we really fix such as warnings even if we generate some obscured
> code ? Wolfgang, what do you think about ?

Yes, we should fix warnings.  If you run a MAKEALL and can be sure
that any message printed is a new problem you will not miss it, and
act as needed.  If youy know that a build will pop up a number or
warnings, it's all too easy to miss if there is a new one - and
checking takes much more concentration.  This is to be avoided.

On the other hand, I agree that we should avoid obscure code as
well.  But then, to me the assembler code "subs pc, r14, #4" is
already obscure enough - I have to admit that I don't really grok it,
nor why this needs to be a __naked function.

My understanding is that to avoid the warning we can either use this
"pre-compiled constant instruction" trick, or we would have to create
a new assembler source file for this single instruction function.

When Marek and I discussed this, the constant seemed to be the
simplest approach (not the nicest, though).

If you don't like it, then we I think we will end up with a new tiny
assembler source file.  Would that be preferred by you?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Many companies that have made themselves dependent on [the  equipment
of  a  certain  major  manufacturer] (and in doing so have sold their
soul to the devil) will collapse under the sheer weight  of  the  un-
mastered complexity of their data processing systems.
          -- Edsger W. Dijkstra, SIGPLAN Notices, Volume 17, Number 5

[U-Boot] [PATCH V2] i.MX28: Drop __naked function from spl_mem_init

[Marek Vasut]

Dear Wolfgang Denk,

> Dear Stefano,
> 
> In message <4F683862.4030709@denx.de> you wrote:
> > > +	/* The following is "subs pc, r14, #4", used as return from DABT. */
> > > +	const uint32_t data_abort_memdetect_handler = 0xe25ef004;
> 
> ...
> 
> > Are we maybe becoming warning addicted ? I know the reason for this (GCC
> > raises a warning "-fstack-usage not supported for this target"), you
> > have already asked the gcc people about this issue, and I do not have an
> > idea how to fix this warning in a different way as you did. This is a
> > sort of self-modifying code.
> 
> In which way is this self-modifying code? I don't think so.

Because it rewrites piece of RAM, which is then called in the Data abort 
context.
> 
> > However, the original code is quite easy to understand - I cannot say
> > the same after the patch, we rely on the comment to understand something.
> 
> That's what comments are made for :-)
> 
> > Should we really fix such as warnings even if we generate some obscured
> > code ? Wolfgang, what do you think about ?
> 
> Yes, we should fix warnings.  If you run a MAKEALL and can be sure
> that any message printed is a new problem you will not miss it, and
> act as needed.  If youy know that a build will pop up a number or
> warnings, it's all too easy to miss if there is a new one - and
> checking takes much more concentration.  This is to be avoided.
> 
> On the other hand, I agree that we should avoid obscure code as
> well.  But then, to me the assembler code "subs pc, r14, #4" is
> already obscure enough - I have to admit that I don't really grok it,
> nor why this needs to be a __naked function.

What it does: return from abort mode back from where it was aborted, one 
instruction further.
Why is it naked: Because you don't want to generate prelude etc. only the real 
contents of the function. That gives exactly 4 bytes. And that's what is used to 
rewrite the DABT handler.
> 
> My understanding is that to avoid the warning we can either use this
> "pre-compiled constant instruction" trick, or we would have to create
> a new assembler source file for this single instruction function.

Or put it into start.S

> 
> When Marek and I discussed this, the constant seemed to be the
> simplest approach (not the nicest, though).

Ack
> 
> If you don't like it, then we I think we will end up with a new tiny
> assembler source file.  Would that be preferred by you?
> 
> Best regards,
> 
> Wolfgang Denk

Best regards,
Marek Vasut

[U-Boot] [PATCH V2] i.MX28: Drop __naked function from spl_mem_init

[Stefano Babic]

On 20/03/2012 09:39, Wolfgang Denk wrote:
> Dear Stefano,
> 

Hi Wolfgang,

> 
> Yes, we should fix warnings.  If you run a MAKEALL and can be sure
> that any message printed is a new problem you will not miss it, and
> act as needed.  If youy know that a build will pop up a number or
> warnings, it's all too easy to miss if there is a new one - and
> checking takes much more concentration.  This is to be avoided.

Yes, I understand your point - and generally I agree with you . Only
this special case let me think if the compiler is always right...

> 
> On the other hand, I agree that we should avoid obscure code as
> well.  But then, to me the assembler code "subs pc, r14, #4" is
> already obscure enough - I have to admit that I don't really grok it,
> nor why this needs to be a __naked function.

Well, but you should admit that if the comment is really an assembly
line and the next line itself is written directly in machine code, it is
quite normal to have doubts why we need a compiler and / or an assembler...

> 
> My understanding is that to avoid the warning we can either use this
> "pre-compiled constant instruction" trick, or we would have to create
> a new assembler source file for this single instruction function.
> 
> When Marek and I discussed this, the constant seemed to be the
> simplest approach (not the nicest, though).
> 
> If you don't like it, then we I think we will end up with a new tiny
> assembler source file.  Would that be preferred by you?

The current fix is at the moment an exception - personally I am aware of
it and I can live with it.

I wanted simply to raise a question about this odd case, when we are
sometimes constrained by the compiler to do very strange things...

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] [PATCH V2] i.MX28: Drop __naked function from spl_mem_init

[Stefano Babic]

On 20/03/2012 10:09, Marek Vasut wrote:

>> In which way is this self-modifying code? I don't think so.
> 
> Because it rewrites piece of RAM, which is then called in the Data abort 
> context.

Exactly

>>
>> My understanding is that to avoid the warning we can either use this
>> "pre-compiled constant instruction" trick, or we would have to create
>> a new assembler source file for this single instruction function.
> 
> Or put it into start.S
> 
>>
>> When Marek and I discussed this, the constant seemed to be the
>> simplest approach (not the nicest, though).
> 
> Ack

Ok - Marek, I think we have clarified why we need it and why we need to
make dirty things..you do not need to change, I will apply the patch as
it is to u-boot-imx.

Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] [PATCH V2] i.MX28: Drop __naked function from spl_mem_init

[Stefano Babic]

On 16/03/2012 22:32, Marek Vasut wrote:
> Instead of compiling the function and using the result as a constant, simply use
> the constant.
> 
> NOTE: This patch works around bug:
> 
>   http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52546
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Stefano Babic <sbabic@denx.de>
> Cc: Tom Rini <trini@ti.com>
> ---

Applied to u-boot-imx (fix), thanks.

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] [PATCH V2] i.MX28: Drop __naked function from spl_mem_init

[Wolfgang Denk]

Dear Marek Vasut,

In message <201203201009.43717.marex@denx.de> you wrote:
> 
> > > > +	const uint32_t data_abort_memdetect_handler = 0xe25ef004;
...
> Because it rewrites piece of RAM, which is then called in the Data abort 
> context.

This is a mere implementation detail.  You could use a pointer
instead, which would then point to the RO data segment.  You could
even use some  __attribute__ ((section(".text")))  to make sure the
"instruction" is really in the text segment, for example (untested):

	const uint32_t insn __attribute__ ((section(".text"))) = 0xe25ef004;
	...
	vt[4] = &insn;

?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Without facts, the decision cannot be made logically. You  must  rely
on your human intuition.
	-- Spock, "Assignment: Earth", stardate unknown