[U-Boot] [PATCH] arm: fix a build error with CONFIG_USE_IRQ

[U-Boot] [PATCH] arm: fix a build error with CONFIG_USE_IRQ

[Masahiro Yamada]

Commit 41623c91 moved exception handling to arch/arm/lib/vectors.S,
breaking CONFIG_USE_IRQ feature.

If CONFIG_USE_IRQ is enabled, undefined reference error occurs.

  arch/arm/lib/built-in.o: In function `interrupt_init':
  arch/arm/lib/interrupts.c:37: undefined reference to `IRQ_STACK_START'
  arch/arm/lib/interrupts.c:37: undefined reference to `FIQ_STACK_START'
  make: *** [u-boot] Error 1

Because arch/arm/lib/vectors.S includes references to
CONFIG_SYS_DV_NOR_BOOT_CFG and CONFIG_USE_IRQ, it must include <config.h>.

Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com>
Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
---
 arch/arm/lib/vectors.S | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/lib/vectors.S b/arch/arm/lib/vectors.S
index d68cc47..fad00da 100644
--- a/arch/arm/lib/vectors.S
+++ b/arch/arm/lib/vectors.S
@@ -13,6 +13,8 @@
  * SPDX-License-Identifier:	GPL-2.0+
  */
 
+#include <config.h>
+
 /*
  *************************************************************************
  *
-- 
1.9.1

[U-Boot] [PATCH] arm: fix a build error with CONFIG_USE_IRQ

[Albert ARIBAUD]

Hi Masahiro,

On Mon, 26 May 2014 19:43:50 +0900, Masahiro Yamada
<yamada.m@jp.panasonic.com> wrote:

> Commit 41623c91 moved exception handling to arch/arm/lib/vectors.S,
> breaking CONFIG_USE_IRQ feature.
> 
> If CONFIG_USE_IRQ is enabled, undefined reference error occurs.
> 
>   arch/arm/lib/built-in.o: In function `interrupt_init':
>   arch/arm/lib/interrupts.c:37: undefined reference to `IRQ_STACK_START'
>   arch/arm/lib/interrupts.c:37: undefined reference to `FIQ_STACK_START'
>   make: *** [u-boot] Error 1
> 
> Because arch/arm/lib/vectors.S includes references to
> CONFIG_SYS_DV_NOR_BOOT_CFG and CONFIG_USE_IRQ, it must include <config.h>.

I routinely build all of ARM (32-bit) and AARCH64 (64-bit) targets and
make sure all of them build properly, and concerning CONFIG_USE_IRQ, I
even have a branch set to remove this option since a

	git grep CONFIG_USE_IRQ | grep -vE \
	'(#define|#ifdef|#ifndef|#if.*defined|#undef|#endif)'

shows that it is defined nowhere in our code.

Which source code are you using?

Amicalement,
-- 
Albert.

[U-Boot] [PATCH] arm: fix a build error with CONFIG_USE_IRQ

[Masahiro Yamada]

Hi Albert,

On Mon, 9 Jun 2014 09:14:11 +0200
Albert ARIBAUD <albert.u.boot@aribaud.net> wrote:

> Hi Masahiro,
> 
> On Mon, 26 May 2014 19:43:50 +0900, Masahiro Yamada
> <yamada.m@jp.panasonic.com> wrote:
> 
> > Commit 41623c91 moved exception handling to arch/arm/lib/vectors.S,
> > breaking CONFIG_USE_IRQ feature.
> > 
> > If CONFIG_USE_IRQ is enabled, undefined reference error occurs.
> > 
> >   arch/arm/lib/built-in.o: In function `interrupt_init':
> >   arch/arm/lib/interrupts.c:37: undefined reference to `IRQ_STACK_START'
> >   arch/arm/lib/interrupts.c:37: undefined reference to `FIQ_STACK_START'
> >   make: *** [u-boot] Error 1
> > 
> > Because arch/arm/lib/vectors.S includes references to
> > CONFIG_SYS_DV_NOR_BOOT_CFG and CONFIG_USE_IRQ, it must include <config.h>.
> 
> I routinely build all of ARM (32-bit) and AARCH64 (64-bit) targets and
> make sure all of them build properly, and concerning CONFIG_USE_IRQ, I
> even have a branch set to remove this option since a
> 
> 	git grep CONFIG_USE_IRQ | grep -vE \
> 	'(#define|#ifdef|#ifndef|#if.*defined|#undef|#endif)'
> 
> shows that it is defined nowhere in our code.
> 
> Which source code are you using?

Out-of-tree board support code, maintained locally.


Best Regards
Masahiro Yamada

[U-Boot] [PATCH] arm: fix a build error with CONFIG_USE_IRQ

[Albert ARIBAUD]

Hi Masahiro,

On Mon, 09 Jun 2014 16:36:56 +0900, Masahiro Yamada
<yamada.m@jp.panasonic.com> wrote:

> Hi Albert,
> 
> On Mon, 9 Jun 2014 09:14:11 +0200
> Albert ARIBAUD <albert.u.boot@aribaud.net> wrote:
> 
> > Hi Masahiro,
> > 
> > On Mon, 26 May 2014 19:43:50 +0900, Masahiro Yamada
> > <yamada.m@jp.panasonic.com> wrote:
> > 
> > > Commit 41623c91 moved exception handling to arch/arm/lib/vectors.S,
> > > breaking CONFIG_USE_IRQ feature.
> > > 
> > > If CONFIG_USE_IRQ is enabled, undefined reference error occurs.
> > > 
> > >   arch/arm/lib/built-in.o: In function `interrupt_init':
> > >   arch/arm/lib/interrupts.c:37: undefined reference to `IRQ_STACK_START'
> > >   arch/arm/lib/interrupts.c:37: undefined reference to `FIQ_STACK_START'
> > >   make: *** [u-boot] Error 1
> > > 
> > > Because arch/arm/lib/vectors.S includes references to
> > > CONFIG_SYS_DV_NOR_BOOT_CFG and CONFIG_USE_IRQ, it must include <config.h>.
> > 
> > I routinely build all of ARM (32-bit) and AARCH64 (64-bit) targets and
> > make sure all of them build properly, and concerning CONFIG_USE_IRQ, I
> > even have a branch set to remove this option since a
> > 
> > 	git grep CONFIG_USE_IRQ | grep -vE \
> > 	'(#define|#ifdef|#ifndef|#if.*defined|#undef|#endif)'
> > 
> > shows that it is defined nowhere in our code.
> > 
> > Which source code are you using?
> 
> Out-of-tree board support code, maintained locally.

Then I'm afraid you'll have to keep this patch local too -- and note
that I am considering overhauling, and possibly removing entirely,
CONFIG_USE_IRQ (my grep command comes from my WIP on this).

> Best Regards
> Masahiro Yamada

Amicalement,
-- 
Albert.

[U-Boot] [PATCH] arm: fix a build error with CONFIG_USE_IRQ

[Masahiro Yamada]

Hi Albert,


On Mon, 9 Jun 2014 10:35:06 +0200
Albert ARIBAUD <albert.u.boot@aribaud.net> wrote:

> Hi Masahiro,
> 
> On Mon, 09 Jun 2014 16:36:56 +0900, Masahiro Yamada
> <yamada.m@jp.panasonic.com> wrote:
> 
> > Hi Albert,
> > 
> > On Mon, 9 Jun 2014 09:14:11 +0200
> > Albert ARIBAUD <albert.u.boot@aribaud.net> wrote:
> > 
> > > Hi Masahiro,
> > > 
> > > On Mon, 26 May 2014 19:43:50 +0900, Masahiro Yamada
> > > <yamada.m@jp.panasonic.com> wrote:
> > > 
> > > > Commit 41623c91 moved exception handling to arch/arm/lib/vectors.S,
> > > > breaking CONFIG_USE_IRQ feature.
> > > > 
> > > > If CONFIG_USE_IRQ is enabled, undefined reference error occurs.
> > > > 
> > > >   arch/arm/lib/built-in.o: In function `interrupt_init':
> > > >   arch/arm/lib/interrupts.c:37: undefined reference to `IRQ_STACK_START'
> > > >   arch/arm/lib/interrupts.c:37: undefined reference to `FIQ_STACK_START'
> > > >   make: *** [u-boot] Error 1
> > > > 
> > > > Because arch/arm/lib/vectors.S includes references to
> > > > CONFIG_SYS_DV_NOR_BOOT_CFG and CONFIG_USE_IRQ, it must include <config.h>.
> > > 
> > > I routinely build all of ARM (32-bit) and AARCH64 (64-bit) targets and
> > > make sure all of them build properly, and concerning CONFIG_USE_IRQ, I
> > > even have a branch set to remove this option since a
> > > 
> > > 	git grep CONFIG_USE_IRQ | grep -vE \
> > > 	'(#define|#ifdef|#ifndef|#if.*defined|#undef|#endif)'
> > > 
> > > shows that it is defined nowhere in our code.
> > > 
> > > Which source code are you using?
> > 
> > Out-of-tree board support code, maintained locally.
> 
> Then I'm afraid you'll have to keep this patch local too -- and note
> that I am considering overhauling, and possibly removing entirely,
> CONFIG_USE_IRQ (my grep command comes from my WIP on this).


You are not understanding at all what is the problem.

You changed the behaviour of three boards,
"enbw_cmc", "da850evm_direct_nor", "calimain"!
Probably they are broken.

These boards expects "0x00000011" (=CONFIG_SYS_DV_NOR_BOOT_CFG)
at the beginning of the image.
But since commit 41623c91, that is missing.

If you still don't understand,  you should checkout   41623c91^ and 41623c91
and compare u-boot.dis.


Best Regards
Masahiro Yamada

[U-Boot] [PATCH] arm: fix a build error with CONFIG_USE_IRQ

[Albert ARIBAUD]

Hi Masahiro,

On Mon, 09 Jun 2014 18:29:26 +0900, Masahiro Yamada
<yamada.m@jp.panasonic.com> wrote:

> Hi Albert,
> 
> 
> On Mon, 9 Jun 2014 10:35:06 +0200
> Albert ARIBAUD <albert.u.boot@aribaud.net> wrote:
> 
> > Hi Masahiro,
> > 
> > On Mon, 09 Jun 2014 16:36:56 +0900, Masahiro Yamada
> > <yamada.m@jp.panasonic.com> wrote:
> > 
> > > Hi Albert,
> > > 
> > > On Mon, 9 Jun 2014 09:14:11 +0200
> > > Albert ARIBAUD <albert.u.boot@aribaud.net> wrote:
> > > 
> > > > Hi Masahiro,
> > > > 
> > > > On Mon, 26 May 2014 19:43:50 +0900, Masahiro Yamada
> > > > <yamada.m@jp.panasonic.com> wrote:
> > > > 
> > > > > Commit 41623c91 moved exception handling to arch/arm/lib/vectors.S,
> > > > > breaking CONFIG_USE_IRQ feature.
> > > > > 
> > > > > If CONFIG_USE_IRQ is enabled, undefined reference error occurs.
> > > > > 
> > > > >   arch/arm/lib/built-in.o: In function `interrupt_init':
> > > > >   arch/arm/lib/interrupts.c:37: undefined reference to `IRQ_STACK_START'
> > > > >   arch/arm/lib/interrupts.c:37: undefined reference to `FIQ_STACK_START'
> > > > >   make: *** [u-boot] Error 1
> > > > > 
> > > > > Because arch/arm/lib/vectors.S includes references to
> > > > > CONFIG_SYS_DV_NOR_BOOT_CFG and CONFIG_USE_IRQ, it must include <config.h>.
> > > > 
> > > > I routinely build all of ARM (32-bit) and AARCH64 (64-bit) targets and
> > > > make sure all of them build properly, and concerning CONFIG_USE_IRQ, I
> > > > even have a branch set to remove this option since a
> > > > 
> > > > 	git grep CONFIG_USE_IRQ | grep -vE \
> > > > 	'(#define|#ifdef|#ifndef|#if.*defined|#undef|#endif)'
> > > > 
> > > > shows that it is defined nowhere in our code.
> > > > 
> > > > Which source code are you using?
> > > 
> > > Out-of-tree board support code, maintained locally.
> > 
> > Then I'm afraid you'll have to keep this patch local too -- and note
> > that I am considering overhauling, and possibly removing entirely,
> > CONFIG_USE_IRQ (my grep command comes from my WIP on this).
> 
> 
> You are not understanding at all what is the problem.
> 
> You changed the behaviour of three boards,
> "enbw_cmc", "da850evm_direct_nor", "calimain"!
> Probably they are broken.
> 
> These boards expects "0x00000011" (=CONFIG_SYS_DV_NOR_BOOT_CFG)
> at the beginning of the image.
> But since commit 41623c91, that is missing.
> 
> If you still don't understand,  you should checkout   41623c91^ and 41623c91
> and compare u-boot.dis.

Sorry, I though your patch was about CONFIG_USE_IRQ and its related
feature, as the subject and commit message both heavily refer to it
and mention CONFIG_SYS_DV_NOR_BOOT_CFG almost incidentally.

Since the issue is that CONFIG_SYS_DV_NOR_BOOT_CFG definition is broken
for three mainline targets, then then please repost a V2 of your
patch, with an amended commit message (and subject) which make it clear
that the problem is with CONFIG_SYS_DV_NOR_BOOT_CFG breaking these
boards, not with CONFIG_USE_IRQ which is unused anyway. I will then make
sure to apply your V2 patch ASAP as it is a bug fix.

> Best Regards
> Masahiro Yamada

Amicalement,
-- 
Albert.

[U-Boot] [PATCH] arm: fix a build error with CONFIG_USE_IRQ

[Masahiro Yamada]

Hi Albert,

On Mon, 9 Jun 2014 10:35:06 +0200
Albert ARIBAUD <albert.u.boot@aribaud.net> wrote:

> Hi Masahiro,
> 
> On Mon, 09 Jun 2014 16:36:56 +0900, Masahiro Yamada
> <yamada.m@jp.panasonic.com> wrote:
> 
> > Hi Albert,
> > 
> > On Mon, 9 Jun 2014 09:14:11 +0200
> > Albert ARIBAUD <albert.u.boot@aribaud.net> wrote:
> > 
> > > Hi Masahiro,
> > > 
> > > On Mon, 26 May 2014 19:43:50 +0900, Masahiro Yamada
> > > <yamada.m@jp.panasonic.com> wrote:
> > > 
> > > > Commit 41623c91 moved exception handling to arch/arm/lib/vectors.S,
> > > > breaking CONFIG_USE_IRQ feature.
> > > > 
> > > > If CONFIG_USE_IRQ is enabled, undefined reference error occurs.
> > > > 
> > > >   arch/arm/lib/built-in.o: In function `interrupt_init':
> > > >   arch/arm/lib/interrupts.c:37: undefined reference to `IRQ_STACK_START'
> > > >   arch/arm/lib/interrupts.c:37: undefined reference to `FIQ_STACK_START'
> > > >   make: *** [u-boot] Error 1
> > > > 
> > > > Because arch/arm/lib/vectors.S includes references to
> > > > CONFIG_SYS_DV_NOR_BOOT_CFG and CONFIG_USE_IRQ, it must include <config.h>.
> > > 
> > > I routinely build all of ARM (32-bit) and AARCH64 (64-bit) targets and
> > > make sure all of them build properly, and concerning CONFIG_USE_IRQ, I
> > > even have a branch set to remove this option since a
> > > 
> > > 	git grep CONFIG_USE_IRQ | grep -vE \
> > > 	'(#define|#ifdef|#ifndef|#if.*defined|#undef|#endif)'
> > > 
> > > shows that it is defined nowhere in our code.
> > > 
> > > Which source code are you using?
> > 
> > Out-of-tree board support code, maintained locally.
> 
> Then I'm afraid you'll have to keep this patch local too -- and note
> that I am considering overhauling, and possibly removing entirely,
> CONFIG_USE_IRQ (my grep command comes from my WIP on this).


BTW,  I don't think it is a good idea to postpone fixing the problem,
depending on  the future development.

 - We should  support features which exist in the code base.
    (I mean if it is referenced, even without definition)
 - If we are not willing to support the feature any more,
    it should be removed immediately from the code base.

In this case, CONFIG_USE_IRQ is still referenced
and if you don't remove it now,
we must maintain that macro.


Best Regards
Masahiro Yamada

[U-Boot] [PATCH] arm: fix a build error with CONFIG_USE_IRQ

[Masahiro Yamada]

Hi Albert,



> Sorry, I though your patch was about CONFIG_USE_IRQ and its related
> feature, as the subject and commit message both heavily refer to it
> and mention CONFIG_SYS_DV_NOR_BOOT_CFG almost incidentally.
> 
> Since the issue is that CONFIG_SYS_DV_NOR_BOOT_CFG definition is broken
> for three mainline targets, then then please repost a V2 of your
> patch, with an amended commit message (and subject) which make it clear
> that the problem is with CONFIG_SYS_DV_NOR_BOOT_CFG breaking these
> boards, not with CONFIG_USE_IRQ which is unused anyway. I will then make
> sure to apply your V2 patch ASAP as it is a bug fix.
> 

The CONFIG_SYS_DV_NOR_BOOT_CFG problem cannot be detected
by build test only.

Because you only check if all ARM boards succeed in compile or not,
I thought  CONFIG_USE_IRQ problem would be clearer to understand.

But  the fact was opposite from my expectation.  :-(


Anyway,  I've posted  v2  with amended comments.


Best Regards
Masahiro Yamada

[U-Boot] enbw_cmc, da850evm_direct_nor, and calimain vectors table misaligned (was: [PATCH] arm: fix a build error with CONFIG_USE_IRQ)

[Albert ARIBAUD]

Hi Masahiro,

(to: the board maintainers for enbw_cmc, da850evm_direct_nor, and
calimain)

On Mon, 09 Jun 2014 18:29:26 +0900, Masahiro Yamada
<yamada.m@jp.panasonic.com> wrote:

> Hi Albert,

> You changed the behaviour of three boards,
> "enbw_cmc", "da850evm_direct_nor", "calimain"!
> Probably they are broken.
> 
> These boards expects "0x00000011" (=CONFIG_SYS_DV_NOR_BOOT_CFG)
> at the beginning of the image.
> But since commit 41623c91, that is missing.
> 
> If you still don't understand,  you should checkout   41623c91^ and
> 41623c91 and compare u-boot.dis.

Your description of the effects of my change is correct. However, this
raises another question which I would like to see discussed before
doing anything about these boards.

Taking the last commit where the prefix word was actually emitted (that
is, 41623c91^, which is actually 60a4f39f, "arm: remove unused _end_vect
and _vectors_end symbols"), and reading arch/arm/cpu/arm926ejs/start.S,
I see that the start of the image looks like this:

offset   Content
+0x0000  prefix word CONFIG_SYS_DV_NOR_BOOT_CFG
+0x0004  reset vector
+0x0008  undefined instruction vector
+0x000c  software interrupt vector
+0x0010  prefetch abort vector
+0x0014  data abort vector
+0x0018  unused
+0x001c  irq vector
+0x0020  fiq vector
+0x0024  (end of vectors table)

And that is /wrong/: the vectors table is misaligned by 4 bytes.

Obviously, the boards have been working fine for a long time, because
no exception vector was used apparently (or because when exceptions did
happen, the error was debugged without the need to analyze the
exception).

I suspect we could just remove the '.word CONFIG_SYS_DV_NOR_BOOT_CFG'
line from the vectors.S file and prepend the word to the image /after/
linking.

This, of course, requires confirmation from maintainers.

Manfred, Christian, Sudhakar, Heiko: can any one of you let us know the
reason for this signature word exactly, and how exactly it is used by
the board? Ideally, can you also:

- test a current build (which does not have the signature word) and
  confirm it fails to load);

- test the same build with the 4-byte signature manually prepended
  (this may possibly require padding the image);

> Best Regards
> Masahiro Yamada

Amicalement,
-- 
Albert.

[U-Boot] enbw_cmc, da850evm_direct_nor, and calimain vectors table misaligned

[Heiko Schocher]

Hello Albert,

Am 11.06.2014 09:14, schrieb Albert ARIBAUD:
> Hi Masahiro,
>
> (to: the board maintainers for enbw_cmc, da850evm_direct_nor, and
> calimain)
>
> On Mon, 09 Jun 2014 18:29:26 +0900, Masahiro Yamada
> <yamada.m@jp.panasonic.com>  wrote:
>
>> Hi Albert,
>
>> You changed the behaviour of three boards,
>> "enbw_cmc", "da850evm_direct_nor", "calimain"!
>> Probably they are broken.
>>
>> These boards expects "0x00000011" (=CONFIG_SYS_DV_NOR_BOOT_CFG)
>> at the beginning of the image.
>> But since commit 41623c91, that is missing.
>>
>> If you still don't understand,  you should checkout   41623c91^ and
>> 41623c91 and compare u-boot.dis.
>
> Your description of the effects of my change is correct. However, this
> raises another question which I would like to see discussed before
> doing anything about these boards.
>
> Taking the last commit where the prefix word was actually emitted (that
> is, 41623c91^, which is actually 60a4f39f, "arm: remove unused _end_vect
> and _vectors_end symbols"), and reading arch/arm/cpu/arm926ejs/start.S,
> I see that the start of the image looks like this:
>
> offset   Content
> +0x0000  prefix word CONFIG_SYS_DV_NOR_BOOT_CFG
> +0x0004  reset vector
> +0x0008  undefined instruction vector
> +0x000c  software interrupt vector
> +0x0010  prefetch abort vector
> +0x0014  data abort vector
> +0x0018  unused
> +0x001c  irq vector
> +0x0020  fiq vector
> +0x0024  (end of vectors table)
>
> And that is /wrong/: the vectors table is misaligned by 4 bytes.
>
> Obviously, the boards have been working fine for a long time, because
> no exception vector was used apparently (or because when exceptions did
> happen, the error was debugged without the need to analyze the
> exception).
>
> I suspect we could just remove the '.word CONFIG_SYS_DV_NOR_BOOT_CFG'
> line from the vectors.S file and prepend the word to the image /after/
> linking.
>
> This, of course, requires confirmation from maintainers.
>
> Manfred, Christian, Sudhakar, Heiko: can any one of you let us know the
> reason for this signature word exactly, and how exactly it is used by
> the board? Ideally, can you also:

See "AM18xx Bootloader Application Report":
http://www.ti.com/lit/pdf/spraba5

Section "3.1 NOR Boot" on page 3ff

This word is used to setup some settings ...

> - test a current build (which does not have the signature word) and
>    confirm it fails to load);
>
> - test the same build with the 4-byte signature manually prepended
>    (this may possibly require padding the image);

I try to find some time for it ...

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] enbw_cmc, da850evm_direct_nor, and calimain vectors table misaligned

[Christian Riesch]

Hi all,

On Wed, Jun 11, 2014 at 9:47 AM, Heiko Schocher <hs@denx.de> wrote:
> Hello Albert,
>
> Am 11.06.2014 09:14, schrieb Albert ARIBAUD:
>>
>> Hi Masahiro,
>>
>> (to: the board maintainers for enbw_cmc, da850evm_direct_nor, and
>> calimain)
>>
>> On Mon, 09 Jun 2014 18:29:26 +0900, Masahiro Yamada
>> <yamada.m@jp.panasonic.com>  wrote:
>>
>>> Hi Albert,
>>
>>
>>> You changed the behaviour of three boards,
>>> "enbw_cmc", "da850evm_direct_nor", "calimain"!
>>> Probably they are broken.
>>>
>>> These boards expects "0x00000011" (=CONFIG_SYS_DV_NOR_BOOT_CFG)
>>> at the beginning of the image.
>>> But since commit 41623c91, that is missing.
>>>
>>> If you still don't understand,  you should checkout   41623c91^ and
>>> 41623c91 and compare u-boot.dis.
>>
>>
>> Your description of the effects of my change is correct. However, this
>> raises another question which I would like to see discussed before
>> doing anything about these boards.
>>
>> Taking the last commit where the prefix word was actually emitted (that
>> is, 41623c91^, which is actually 60a4f39f, "arm: remove unused _end_vect
>> and _vectors_end symbols"), and reading arch/arm/cpu/arm926ejs/start.S,
>> I see that the start of the image looks like this:
>>
>> offset   Content
>> +0x0000  prefix word CONFIG_SYS_DV_NOR_BOOT_CFG
>> +0x0004  reset vector
>> +0x0008  undefined instruction vector
>> +0x000c  software interrupt vector
>> +0x0010  prefetch abort vector
>> +0x0014  data abort vector
>> +0x0018  unused
>> +0x001c  irq vector
>> +0x0020  fiq vector
>> +0x0024  (end of vectors table)
>>
>> And that is /wrong/: the vectors table is misaligned by 4 bytes.
>>
>> Obviously, the boards have been working fine for a long time, because
>> no exception vector was used apparently (or because when exceptions did
>> happen, the error was debugged without the need to analyze the
>> exception).

Probably a bit of both ;-)

I don't know much about exceptions, so maybe this is just stupid, but:
I had a look at some ARM documentation (and the datasheets of the
AM1808 that is used on the calimain board). According to these, the
exception vector table of this CPU is located at 0xffff0000. I had a
look at the memory there, and the content of this table seems to be
branches to the ROM bootloader located at 0xfffd0000. So my question
is: Are we actually setting up the ARM's exception table for this
device? And if yes, where is this done in the code?

>> I suspect we could just remove the '.word CONFIG_SYS_DV_NOR_BOOT_CFG'
>> line from the vectors.S file and prepend the word to the image /after/
>> linking.

Yes, I think this would be the right way to do it.

>> This, of course, requires confirmation from maintainers.
>>
>> Manfred, Christian, Sudhakar, Heiko: can any one of you let us know the
>> reason for this signature word exactly, and how exactly it is used by
>> the board? Ideally, can you also:
>
>
> See "AM18xx Bootloader Application Report":
> http://www.ti.com/lit/pdf/spraba5
>
> Section "3.1 NOR Boot" on page 3ff
>
> This word is used to setup some settings ...

Exactly. 0x60000000 is the start of the NOR flash. 0x00000011 as the
first word tells the ROM bootloader to use the "Direct NOR boot"
method, i.e. to branch to 0x60000004, which is the start of u-boot.
u-boot then relocates itself to RAM.

>
>> - test a current build (which does not have the signature word) and
>>    confirm it fails to load);

I tested a current build for calimain, and yes, it fails to load.

>>
>> - test the same build with the 4-byte signature manually prepended
>>    (this may possibly require padding the image);

No, this didn't work, I guess because the relocation offsets are wrong
now. I tried to set CONFIG_SYS_TEXT_BASE in include/configs/calimain.h
to 0x60000004, but this resulted in some strange padding (28 bytes set
to 0x00 prepend the u-boot image) which I currently don't understand.

Christian

[U-Boot] enbw_cmc, da850evm_direct_nor, and calimain vectors table misaligned

[Christian Riesch]

On Wed, Jun 11, 2014 at 3:15 PM, Christian Riesch
<christian.riesch@omicron.at> wrote:
>>> - test the same build with the 4-byte signature manually prepended
>>>    (this may possibly require padding the image);
>
> No, this didn't work, I guess because the relocation offsets are wrong
> now. I tried to set CONFIG_SYS_TEXT_BASE in include/configs/calimain.h
> to 0x60000004, but this resulted in some strange padding (28 bytes set
> to 0x00 prepend the u-boot image) which I currently don't understand.

"Which I currently don't understand" means: I looks like the begin of
the vector table is padded to align to 32 bytes. But I don't
understand why and where this is set in the linker script etc.
Christian

[U-Boot] enbw_cmc, da850evm_direct_nor, and calimain vectors table misaligned (was: [PATCH] arm: fix a build error with CONFIG_USE_IRQ)

[Masahiro Yamada]

Hi Albert.

On Wed, 11 Jun 2014 09:14:49 +0200
Albert ARIBAUD <albert.u.boot@aribaud.net> wrote:

> Hi Masahiro,
> 
> (to: the board maintainers for enbw_cmc, da850evm_direct_nor, and
> calimain)
> 
> On Mon, 09 Jun 2014 18:29:26 +0900, Masahiro Yamada
> <yamada.m@jp.panasonic.com> wrote:
> 
> > Hi Albert,
> 
> > You changed the behaviour of three boards,
> > "enbw_cmc", "da850evm_direct_nor", "calimain"!
> > Probably they are broken.
> > 
> > These boards expects "0x00000011" (=CONFIG_SYS_DV_NOR_BOOT_CFG)
> > at the beginning of the image.
> > But since commit 41623c91, that is missing.
> > 
> > If you still don't understand,  you should checkout   41623c91^ and
> > 41623c91 and compare u-boot.dis.
> 
> Your description of the effects of my change is correct. However, this
> raises another question which I would like to see discussed before
> doing anything about these boards.

OK, then postpone (or disregard eventually) my patch, please.


Best Regards
Masahiro Yamada

[U-Boot] enbw_cmc, da850evm_direct_nor, and calimain vectors table misaligned (was: [PATCH] arm: fix a build error with CONFIG_USE_IRQ)

[Christian Riesch]

Hi Albert,
I had one more look at this, please see my comments below.

On Wed, Jun 11, 2014 at 9:14 AM, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:
> Hi Masahiro,
>
> (to: the board maintainers for enbw_cmc, da850evm_direct_nor, and
> calimain)
>
> On Mon, 09 Jun 2014 18:29:26 +0900, Masahiro Yamada
> <yamada.m@jp.panasonic.com> wrote:
>
>> Hi Albert,
>
>> You changed the behaviour of three boards,
>> "enbw_cmc", "da850evm_direct_nor", "calimain"!
>> Probably they are broken.
>>
>> These boards expects "0x00000011" (=CONFIG_SYS_DV_NOR_BOOT_CFG)
>> at the beginning of the image.
>> But since commit 41623c91, that is missing.
>>
>> If you still don't understand,  you should checkout   41623c91^ and
>> 41623c91 and compare u-boot.dis.
>
> Your description of the effects of my change is correct. However, this
> raises another question which I would like to see discussed before
> doing anything about these boards.
>
> Taking the last commit where the prefix word was actually emitted (that
> is, 41623c91^, which is actually 60a4f39f, "arm: remove unused _end_vect
> and _vectors_end symbols"), and reading arch/arm/cpu/arm926ejs/start.S,
> I see that the start of the image looks like this:
>
> offset   Content
> +0x0000  prefix word CONFIG_SYS_DV_NOR_BOOT_CFG
> +0x0004  reset vector
> +0x0008  undefined instruction vector
> +0x000c  software interrupt vector
> +0x0010  prefetch abort vector
> +0x0014  data abort vector
> +0x0018  unused
> +0x001c  irq vector
> +0x0020  fiq vector
> +0x0024  (end of vectors table)
>
> And that is /wrong/: the vectors table is misaligned by 4 bytes.

Let's have a look at the calimain board. The vector exception table of
this CPU (ARM926EJS) can be located either at 0x00000000 or at
0xffff0000 (depending on CONFIG_SYS_EXCEPTION_VECTORS_HIGH). This SoC
(Texas Instruments AM1808) has no physical memory at 0x00000000,
therefore CONFIG_SYS_EXCEPTION_VECTOR must be defined. The exception
vector table is located in the internal RAM of the device, that is
located at 0xffff0000.

However, CONFIG_SYS_TEXT_BASE is 0x60000000, that is the begin of NOR
flash memory on this device. Later, u-boot relocates itself to some
place DDR2 memory. So in the begin u-boot's vector table is located at
0x60000004. Later, it is relocated to somewhere in the DDR2 memory.
There is no code that actually touches the exception vector table at
0xffff0000. Exceptions are not used at all and therefore the location
of this table in memory is totally irrelevant.

What we actually need would be some code that copies the vector table
to the right location (0xffff0000). But this code could copy the table
from anywhere, so I don't understand why the
CONFIG_SYS_DV_NOR_BOOT_CFG word would disturb the alignment of the
table.

If we accept that we do not use any exceptions we could either restore
the old behavior:

--- a/arch/arm/lib/vectors.S
+++ b/arch/arm/lib/vectors.S
@@ -13,6 +13,8 @@
  * SPDX-License-Identifier:    GPL-2.0+
  */

+#include <config.h>
+
 /*
  *************************************************************************
  *
@@ -49,7 +51,6 @@ _start:
        .word   CONFIG_SYS_DV_NOR_BOOT_CFG
 #endif

-_start:
        ldr     pc, _reset


>
> Obviously, the boards have been working fine for a long time, because
> no exception vector was used apparently (or because when exceptions did
> happen, the error was debugged without the need to analyze the
> exception).
>
> I suspect we could just remove the '.word CONFIG_SYS_DV_NOR_BOOT_CFG'
> line from the vectors.S file and prepend the word to the image /after/
> linking.

Or we could remove .word CONFIG_SYS_DV_NOR_BOOT_CFG as you suggested
and later add the word after linking. But for this case we should be
able to set CONFIG_SYS_TEXT_BASE to 0x60000004. But due to the .align
5 statements below in arch/arm/lib/vectors.S this leads to a padding
at the start of u-boot.bin, since the entire .vectors section will be
aligned to 32 bytes:

 hexdump -C u-boot.bin
00000000  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
00000010  00 00 00 00 00 00 00 00  00 00 00 00 18 f0 9f e5  |................|
00000020  18 f0 9f e5 18 f0 9f e5  18 f0 9f e5 18 f0 9f e5  |................|

Now the ldr pc, _reset is at the wrong location, u-boot does not boot.

Am I missing something here? What would be the preferred solution to
make the board working again?

Best regards,
Christian

[U-Boot] enbw_cmc, da850evm_direct_nor, and calimain vectors table misaligned (was: [PATCH] arm: fix a build error with CONFIG_USE_IRQ)

[Christian Riesch]

On Wed, Jun 18, 2014 at 2:55 PM, Christian Riesch
<christian.riesch@omicron.at> wrote:
[...]
>> And that is /wrong/: the vectors table is misaligned by 4 bytes.
>
> Let's have a look at the calimain board. The vector exception table of
> this CPU (ARM926EJS) can be located either at 0x00000000 or at
> 0xffff0000 (depending on CONFIG_SYS_EXCEPTION_VECTORS_HIGH). This SoC
> (Texas Instruments AM1808) has no physical memory at 0x00000000,
> therefore CONFIG_SYS_EXCEPTION_VECTOR must be defined. The exception

s/CONFIG_SYS_EXCEPTION_VECTOR/CONFIG_SYS_EXCEPTION_VECTORS_HIGH/

:-)
Christian

[U-Boot] enbw_cmc, da850evm_direct_nor, and calimain vectors table misaligned (was: [PATCH] arm: fix a build error with CONFIG_USE_IRQ)

[Christian Riesch]

Hello Albert,

On Wed, Jun 18, 2014 at 2:55 PM, Christian Riesch
<christian.riesch@omicron.at> wrote:
> Am I missing something here? What would be the preferred solution to
> make the board working again?

Any comments on this? What shall we do to get the boards working again?
Thanks,
Christian

[U-Boot] enbw_cmc, da850evm_direct_nor, and calimain vectors table misaligned (was: [PATCH] arm: fix a build error with CONFIG_USE_IRQ)

[Albert ARIBAUD]

Hi Christian,

On Wed, 2 Jul 2014 15:45:17 +0200, Christian Riesch
<christian.riesch@omicron.at> wrote:

> Hello Albert,
> 
> On Wed, Jun 18, 2014 at 2:55 PM, Christian Riesch
> <christian.riesch@omicron.at> wrote:
> > Am I missing something here? What would be the preferred solution to
> > make the board working again?
> 
> Any comments on this? What shall we do to get the boards working again?

Sorry for the delay.

My opinion is that for these boards, there should actually be *no*
exception vectors at the start of the image, since it would never serve
any purpose anyway unless as you suggest we copy the vectors table from
the image to the actual vectors table location (or set the VBAR to the
image vector table location for those targets which allow it).

(and I don't think this is limited to the three boards discussed here.
IOW, the current exception/interrupt system in ARM U-Boot is FUBAR.)

As far as fixing the boards, Masahiro had posted a series where one
patch fixed the issue. I NAKed the submission for several reasons; I
had expected a followup (v3) which I failed to ping for. 

MAsahiro: do you have a v3 ready?

> Thanks,
> Christian

Amicalement,
-- 
Albert.

[U-Boot] enbw_cmc, da850evm_direct_nor, and calimain vectors table misaligned (was: [PATCH] arm: fix a build error with CONFIG_USE_IRQ)

[Christian Riesch]

Hi Albert,

On Fri, Jul 4, 2014 at 10:35 PM, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:
> Hi Christian,
>
> On Wed, 2 Jul 2014 15:45:17 +0200, Christian Riesch
> <christian.riesch@omicron.at> wrote:
>
>> Hello Albert,
>>
>> On Wed, Jun 18, 2014 at 2:55 PM, Christian Riesch
>> <christian.riesch@omicron.at> wrote:
>> > Am I missing something here? What would be the preferred solution to
>> > make the board working again?
>>
>> Any comments on this? What shall we do to get the boards working again?
>
> Sorry for the delay.
>
> My opinion is that for these boards, there should actually be *no*
> exception vectors at the start of the image, since it would never serve
> any purpose anyway unless as you suggest we copy the vectors table from
> the image to the actual vectors table location (or set the VBAR to the
> image vector table location for those targets which allow it).
>
> (and I don't think this is limited to the three boards discussed here.
> IOW, the current exception/interrupt system in ARM U-Boot is FUBAR.)
>
> As far as fixing the boards, Masahiro had posted a series where one
> patch fixed the issue. I NAKed the submission for several reasons; I
> had expected a followup (v3) which I failed to ping for.
>
> MAsahiro: do you have a v3 ready?

I assume you are talking about [1]. Adding this #include alone does
not solve the issue for the calimain board.

With adding the include, we would end up with

#include <config.h>

#ifdef CONFIG_SYS_DV_NOR_BOOT_CFG
        .word   CONFIG_SYS_DV_NOR_BOOT_CFG
#endif
_start:

        ldr     pc, _reset
...

in arch/arm/lib/vectors.S. As a result, __image_copy_start points to
0x60000000 (CONFIG_SYS_TEXT_BASE) and _start points is 0x60000004. The
relocation code in arch/arm/lib/relocate.S calculates its relocation
offset based on __image_copy_start. However, arch/arm/lib/board.c
calculates the offset as

gd->reloc_off = addr - (ulong)&_start;

As the two are different, we have different offsets for the relocation
code in relocate.S (based on __image_copy_start) and the calculation
of the lr register in arch/arm/lib/crt0.S (gd->reloc_off). After
relocation, the CPU jumps to the wrong address.

So we can either unify the offset calculation (which of the two is the
correct one?) or put the _start label where it was in earlier versions
of u-boot:

#include <config.h>

_start:
#ifdef CONFIG_SYS_DV_NOR_BOOT_CFG
        .word   CONFIG_SYS_DV_NOR_BOOT_CFG
#endif

        ldr     pc, _reset
...

What do you think?
Thanks, Christian

[1] http://lists.denx.de/pipermail/u-boot/2014-June/181334.html

[U-Boot] enbw_cmc, da850evm_direct_nor, and calimain vectors table misaligned (was: [PATCH] arm: fix a build error with CONFIG_USE_IRQ)

[Christian Riesch]

Hi again,

On Mon, Jul 7, 2014 at 9:15 AM, Christian Riesch
<christian.riesch@omicron.at> wrote:
> [...] As a result, __image_copy_start points to
> 0x60000000 (CONFIG_SYS_TEXT_BASE) and _start points is 0x60000004. The
> relocation code in arch/arm/lib/relocate.S calculates its relocation
> offset based on __image_copy_start. However, arch/arm/lib/board.c
> calculates the offset as
>
> gd->reloc_off = addr - (ulong)&_start;

I just noticed that I did my test without CONFIG_SYS_GENERIC_BOARD.
Adding CONFIG_SYS_GENERIC_BOARD solves that problem, as the relocation
offset calculation in common/board_f.c is based on
CONFIG_SYS_TEXT_BASE.

So adding the missing #include in vectors.S and adding
CONFIG_SYS_GENERIC_BOARD (patch is already in u-boot-arm/master) fixes
the calimain board.

I'll post a patch for the missing include.

Christian