[U-Boot] AM335x not working anymore

[U-Boot] AM335x not working anymore

[Hannes Petermaier]

Hi,

i've just pulled newest u-boot and tried it on my custom AM335x board 
without success. It hangs after (successful) running init_s function.

....
Enable clock module - 44e0001c
Enable clock module - 44e000d0
Enable clock module - 44e00028

is the last debug output.
After some time the watchdog is doing his job and restarts again.

Okay ... thinking maybe something had become incompatible with my 
customization ... so let's try the beaglebone board.
Same behaviour :-(

I debugged around and found finally this commit, which breaks the support:
/
//commit id: f0c3a6c4ad09210d5d4aeafe87685ee75e5683d6//
//
//    ARM: SPL: do not set gd again//
////
//    Just before calling board_init_f, crt0.S has already//
//    reserved space for the initial gd on the stack. There//
//    should be no need to allocate it again.//
////
//    cc: Albert ARIBAUD <albert.u.boot@aribaud.net>//
//    Signed-off-by: Jeroen Hofstee <jeroen@myspectrum.nl>/

I have reverted this commit in my branch and everything works fine.
Finally i have to admit, that i do not fully understand whats going on 
there in detail.

What i can see, is that there live more than one instance of the "gd". 
One is setup in arch/arm/lib/cpu/armv7/lowlevel_init.S, another one is 
setup in arch/arm/lib/crt0.S.
Within the "gd-ptr" is loaded to &gdata (gdata __attribute__ 
((section(".data")))

I am a bit confused all about this :-)

Any ideas ?

best regards,
Hannes

[U-Boot] AM335x not working anymore

[Tom Rini]

On Tue, Sep 16, 2014 at 02:17:01PM +0200, Hannes Petermaier wrote:

> Hi,
> 
> i've just pulled newest u-boot and tried it on my custom AM335x
> board without success. It hangs after (successful) running init_s
> function.
> 
> ....
> Enable clock module - 44e0001c
> Enable clock module - 44e000d0
> Enable clock module - 44e00028
> 
> is the last debug output.
> After some time the watchdog is doing his job and restarts again.
> 
> Okay ... thinking maybe something had become incompatible with my
> customization ... so let's try the beaglebone board.
> Same behaviour :-(
> 
> I debugged around and found finally this commit, which breaks the support:
> /
> //commit id: f0c3a6c4ad09210d5d4aeafe87685ee75e5683d6//
> //
> //    ARM: SPL: do not set gd again//
> ////
> //    Just before calling board_init_f, crt0.S has already//
> //    reserved space for the initial gd on the stack. There//
> //    should be no need to allocate it again.//
> ////
> //    cc: Albert ARIBAUD <albert.u.boot@aribaud.net>//
> //    Signed-off-by: Jeroen Hofstee <jeroen@myspectrum.nl>/
> 
> I have reverted this commit in my branch and everything works fine.
> Finally i have to admit, that i do not fully understand whats going
> on there in detail.
> 
> What i can see, is that there live more than one instance of the
> "gd". One is setup in arch/arm/lib/cpu/armv7/lowlevel_init.S,
> another one is setup in arch/arm/lib/crt0.S.
> Within the "gd-ptr" is loaded to &gdata (gdata __attribute__
> ((section(".data")))
> 
> I am a bit confused all about this :-)

I just finished bisecting down to this being a problem commit as well.
As far as I can see, the commit in question doesn't make sense.  Or
perhaps rather, the comments in crt0.S aren't quite right in the SPL
case where we don't place the initial gd on the stack, it's in the .data
section.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140916/aca2bf2d/attachment.pgp>

[U-Boot] AM335x not working anymore

[Jeroen Hofstee]

Hello Hannes, Tom,

On di, 2014-09-16 at 09:56 -0400, Tom Rini wrote:
> On Tue, Sep 16, 2014 at 02:17:01PM +0200, Hannes Petermaier wrote:
> 
> > Hi,
> > 
> > i've just pulled newest u-boot and tried it on my custom AM335x
> > board without success. It hangs after (successful) running init_s
> > function.
> > 
> > ....
> > Enable clock module - 44e0001c
> > Enable clock module - 44e000d0
> > Enable clock module - 44e00028
> > 
> > is the last debug output.
> > After some time the watchdog is doing his job and restarts again.
> > 
> > Okay ... thinking maybe something had become incompatible with my
> > customization ... so let's try the beaglebone board.
> > Same behaviour :-(
> > 
> > I debugged around and found finally this commit, which breaks the support:
> > /
> > //commit id: f0c3a6c4ad09210d5d4aeafe87685ee75e5683d6//
> > //
> > //    ARM: SPL: do not set gd again//
> > ////
> > //    Just before calling board_init_f, crt0.S has already//
> > //    reserved space for the initial gd on the stack. There//
> > //    should be no need to allocate it again.//
> > ////
> > //    cc: Albert ARIBAUD <albert.u.boot@aribaud.net>//
> > //    Signed-off-by: Jeroen Hofstee <jeroen@myspectrum.nl>/
> > 
> > I have reverted this commit in my branch and everything works fine.
> > Finally i have to admit, that i do not fully understand whats going
> > on there in detail.
> > 
> > What i can see, is that there live more than one instance of the
> > "gd". One is setup in arch/arm/lib/cpu/armv7/lowlevel_init.S,
> > another one is setup in arch/arm/lib/crt0.S.
> > Within the "gd-ptr" is loaded to &gdata (gdata __attribute__
> > ((section(".data")))
> > 
> > I am a bit confused all about this :-)
> 
> I just finished bisecting down to this being a problem commit as well.
> As far as I can see, the commit in question doesn't make sense.  Or
> perhaps rather, the comments in crt0.S aren't quite right in the SPL
> case where we don't place the initial gd on the stack, it's in the .data
> section.
> 

As far as I can tell there are two gdata's is the SPL case.
One on the stack and one in gdata. Can't the later not be
removed, so we end up with a single gd init in crt0.S?
Before this patch the code switches back and forth between
the two, which is at least a bit weird.

I will have a closer look at the call path, especially why
it used to work...

Regards,
Jeroen

[U-Boot] AM335x not working anymore

[Jeroen Hofstee]

Hi,

On 16-09-14 16:11, Jeroen Hofstee wrote:
> Hello Hannes, Tom,
>
> On di, 2014-09-16 at 09:56 -0400, Tom Rini wrote:
>> On Tue, Sep 16, 2014 at 02:17:01PM +0200, Hannes Petermaier wrote:
>>
>>> Hi,
>>>
>>> i've just pulled newest u-boot and tried it on my custom AM335x
>>> board without success. It hangs after (successful) running init_s
>>> function.
>>>
>>> ....
>>> Enable clock module - 44e0001c
>>> Enable clock module - 44e000d0
>>> Enable clock module - 44e00028
>>>
>>> is the last debug output.
>>> After some time the watchdog is doing his job and restarts again.
>>>
>>> Okay ... thinking maybe something had become incompatible with my
>>> customization ... so let's try the beaglebone board.
>>> Same behaviour :-(
>>>
>>> I debugged around and found finally this commit, which breaks the support:
>>> /
>>> //commit id: f0c3a6c4ad09210d5d4aeafe87685ee75e5683d6//
>>> //
>>> //    ARM: SPL: do not set gd again//
>>> ////
>>> //    Just before calling board_init_f, crt0.S has already//
>>> //    reserved space for the initial gd on the stack. There//
>>> //    should be no need to allocate it again.//
>>> ////
>>> //    cc: Albert ARIBAUD <albert.u.boot@aribaud.net>//
>>> //    Signed-off-by: Jeroen Hofstee <jeroen@myspectrum.nl>/
>>>
>>> I have reverted this commit in my branch and everything works fine.
>>> Finally i have to admit, that i do not fully understand whats going
>>> on there in detail.
>>>
>>> What i can see, is that there live more than one instance of the
>>> "gd". One is setup in arch/arm/lib/cpu/armv7/lowlevel_init.S,
>>> another one is setup in arch/arm/lib/crt0.S.
>>> Within the "gd-ptr" is loaded to &gdata (gdata __attribute__
>>> ((section(".data")))
>>>
>>> I am a bit confused all about this :-)
>> I just finished bisecting down to this being a problem commit as well.
>> As far as I can see, the commit in question doesn't make sense.  Or
>> perhaps rather, the comments in crt0.S aren't quite right in the SPL
>> case where we don't place the initial gd on the stack, it's in the .data
>> section.
>>
> As far as I can tell there are two gdata's is the SPL case.
> One on the stack and one in gdata. Can't the later not be
> removed, so we end up with a single gd init in crt0.S?
> Before this patch the code switches back and forth between
> the two, which is at least a bit weird.
>
> I will have a closer look at the call path, especially why
> it used to work...
>

While in my opinion the omap code is to blame for doing
more then needed in cpu_init_crit and eventually in s_init,
(and hence needing its own gdata) I am more then fine
with reverting this patch for now, but I would like to
have only a single gdata eventually, initialized only once.

Can you try this patch [1], just for educational purposes. It
will scare Tom a bit since dram is setup before the console,
but I still have to find an user of that.

Regards,
Jeroen

[1] http://patchwork.ozlabs.org/patch/354535/