i.MX28: Drop __naked function from spl_mem_init

PATCH Superseded Marek Vasut 3 participants 9 messages Mar 16, 2012 – Mar 20, 2012 1 message not in the archive Arm (32-bit)

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): Message-Id: <[email protected]

Marek Vasut

author

2012-03-16 21:32 UTC #1 lore raw

PATCH v2 1/1 i.MX28: Drop __naked function from spl_mem_init

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 <[email protected]>

Cc Stefano Babic <[email protected]>

Cc Tom Rini <[email protected]>

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

arch/arm/cpu/arm926ejs/mx28/spl_mem_init.c +3 -7

index 43a90ff..911bbef 100644
@@ -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);

Signature · 1 line

1.7.9

Stefano Babic

2012-03-20 07:57 UTC #2 lore raw

PATCH v2 1/1 i.MX28: Drop __naked function from spl_mem_init

Quoted 11 lines · 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 <[email protected]>
Cc Stefano Babic <[email protected]>
Cc Tom Rini <[email protected]>
---

Hi Marek,

Quoted 17 lines

 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

arch/arm/cpu/arm926ejs/mx28/spl_mem_init.c +0 -4

index 43a90ff..911bbef 100644
@@ -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");

arch/arm/cpu/arm926ejs/mx28/spl_mem_init.c void mx28_mem_setup_vddd(void) · line 183

Quoted 10 lines

arch/arm/cpu/arm926ejs/mx28/spl_mem_init.c +2 -2

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

Signature · 5 lines

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

Marek Vasut

author

2012-03-20 08:21 UTC #3 lore raw

PATCH v2 1/1 i.MX28: Drop __naked function from spl_mem_init

Dear Stefano Babic,

Quoted 47 lines · Marek Vasut

Quoted 10 lines
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 <[email protected]>
Cc Stefano Babic <[email protected]>
Cc Tom Rini <[email protected]>
---
Hi Marek,
Quoted 33 lines
 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
(quoted diff) +0 -0
b/arch/arm/cpu/arm926ejs/mx28/spl_mem_init.c index 43a90ff..911bbef 100644
arch/arm/cpu/arm926ejs/mx28/spl_mem_init.c +0 -6
@@ -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;
 

Quoted 8 lines

arch/arm/cpu/arm926ejs/mx28/spl_mem_init.c void mx28_mem_setup_vddd(void) · line 183
Quoted 2 lines
arch/arm/cpu/arm926ejs/mx28/spl_mem_init.c +2 -0
+	/* 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 :)

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 3 lines

Regards,
Stefano

Best regards,
Marek Vasut

Wolfgang Denk

2012-03-20 08:39 UTC #4 lore raw

PATCH v2 1/1 i.MX28: Drop __naked function from spl_mem_init

In reply to Stefano Babic

Dear Stefano,

Quoted 3 lines · In message <[email protected]> you wrote:

Quoted 3 lines

arch/arm/cpu/arm926ejs/mx28/spl_mem_init.c +2 -0

+	/* The following is "subs pc, r14, #4", used as return from DABT. */
+	const uint32_t data_abort_memdetect_handler = 0xe25ef004;

...

Quoted 5 lines

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.

Quoted 2 lines

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

Quoted 2 lines

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

Signature · 8 lines

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

Marek Vasut

author

2012-03-20 09:09 UTC #5 lore raw

PATCH v2 1/1 i.MX28: Drop __naked function from spl_mem_init

Dear Wolfgang Denk,

Quoted 8 lines

Dear Stefano,
Quoted 2 lines · In message <[email protected]> you wrote:
Quoted 2 lines
arch/arm/cpu/arm926ejs/mx28/spl_mem_init.c +2 -0
+	/* The following is "subs pc, r14, #4", used as return from DABT. */
+	const uint32_t data_abort_memdetect_handler = 0xe25ef004;
...

Quoted 7 lines

Quoted 5 lines
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.

Quoted 15 lines

Quoted 3 lines
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 :-)
Quoted 2 lines
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.

Quoted 4 lines

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.

Quoted 4 lines

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

Quoted 3 lines

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

Ack

Quoted 7 lines

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

Stefano Babic

2012-03-20 09:21 UTC #6 lore raw

PATCH v2 1/1 i.MX28: Drop __naked function from spl_mem_init

Quoted 4 lines · Marek Vasut

Quoted 1 line
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

Quoted 5 lines

Quoted 4 lines
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.

Quoted 7 lines

Or put it into start.S
Quoted 3 lines
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

Signature · 5 lines

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

Wolfgang Denk

2012-03-20 17:38 UTC #7 lore raw

PATCH v2 1/1 i.MX28: Drop __naked function from spl_mem_init

In reply to Marek Vasut

Dear Marek Vasut,

Quoted 2 lines · In message <[email protected]> you wrote:

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
arch/arm/cpu/arm926ejs/mx28/spl_mem_init.c +1 -0
+	const uint32_t data_abort_memdetect_handler = 0xe25ef004;

...

Quoted 2 lines

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

Signature · 6 lines

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

Stefano Babic

2012-03-20 09:17 UTC #8 lore raw

PATCH v2 1/1 i.MX28: Drop __naked function from spl_mem_init

In reply to Wolfgang Denk

Quoted 2 lines · Wolfgang Denk

Dear Stefano,

Hi Wolfgang,

Quoted 6 lines

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

Quoted 5 lines

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

Quoted 10 lines

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

Signature · 5 lines

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

Stefano Babic

2012-03-20 10:43 UTC #9 lore raw

PATCH v2 1/1 i.MX28: Drop __naked function from spl_mem_init

In reply to Marek Vasut

Quoted 11 lines · 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 <[email protected]>
Cc Stefano Babic <[email protected]>
Cc Tom Rini <[email protected]>
---

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

Best regards,
Stefano Babic

Signature · 5 lines

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