| Message ID | 1331933563-5865-1-git-send-email-marex@denx.de |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Stefano Babic |
| Headers | show |
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
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
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
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
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
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
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
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
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);
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