Hi Simon,

On Wed, Nov 11, 2015 at 7:08 PM, Simon Glass sjg@chromium.org wrote:

That test is intended to avoid setting up simple malloc() if we plan to use full malloc() in SPL. Of course, full malloc() is set up a little later (in spl_init()). But we should not need both - either we use simple malloc() or full malloc().

But for your board I see:

$ grep CONFIG_SYS_SPL_MALLOC_SIZE b/mx6sabresd_spl/u-boot.cfg #define CONFIG_SYS_SPL_MALLOC_SIZE 0x3200000

So you will not be able to use simple malloc(). I'd suggest calling spl_init() from board_init_f() if you need malloc() there. But it

Yes, I do call spl_init() from board_init_f() prior to calling malloc() and this has been working fine prior to 5ba534d247d418.

presumably needs to be done after you have SDRAM up. So perhaps

The trick here is that I need malloc to work prior to have SDRAM configured.

On cgtqmx6eval we need to read SPI NOR to determine what kind of DDR we will need to configure.

Otavio has sent the SPL support for cgtqmx6eval, but it has not reached U-boot mainline yet.

I am reproducing the problem on a mx6sabresd_spl target with the simple example code I sent previously.

Regards,

Fabio Estevam

Hello Simon,

On Wed, 11 Nov 2015 14:49:05 -0700, Simon Glass sjg@chromium.org wrote:

Hi Fabio,

On 11 November 2015 at 14:24, Fabio Estevam festevam@gmail.com wrote:

...

Hi Simon,

On Wed, Nov 11, 2015 at 7:08 PM, Simon Glass sjg@chromium.org wrote:

...

That test is intended to avoid setting up simple malloc() if we plan to use full malloc() in SPL. Of course, full malloc() is set up a little later (in spl_init()). But we should not need both - either we use simple malloc() or full malloc().

But for your board I see:

$ grep CONFIG_SYS_SPL_MALLOC_SIZE b/mx6sabresd_spl/u-boot.cfg #define CONFIG_SYS_SPL_MALLOC_SIZE 0x3200000

So you will not be able to use simple malloc(). I'd suggest calling spl_init() from board_init_f() if you need malloc() there. But it

Yes, I do call spl_init() from board_init_f() prior to calling malloc() and this has been working fine prior to 5ba534d247d418.

...

presumably needs to be done after you have SDRAM up. So perhaps

The trick here is that I need malloc to work prior to have SDRAM configured.

On cgtqmx6eval we need to read SPI NOR to determine what kind of DDR we will need to configure.

Otavio has sent the SPL support for cgtqmx6eval, but it has not reached U-boot mainline yet.

I am reproducing the problem on a mx6sabresd_spl target with the simple example code I sent previously.

I see. That's not a use case I have seen before.

I'd suggest changing the #ifdef to always set up early malloc() if CONFIG_SYS_MALLOC_F is set. There will of course be a new malloc() once spi_init() is called, but that does not matter.

In your patch, please be careful to add some docs to the README on this point (the early malloc() is only there to permit DRAM init, etc.). It could get quite confusing...

I'm not sure all details are clear to me so let me chime in on this.

IIUC, what Fabio needs is a working C runtime including malloc, based on some IRAM rather than SDRAM.

This means there will be two phases where malloc can be used, the usual one after SDRAM init, and a new one before SDRAM init.

Of course, the amount of memory reserved for the malloc arena cannot be the same in IRAM as it will be in SDRAM, meaning that the routine which reserves the arena must handle both cases, by detecting whether it is running before or after SDRAM init and choosing the right malloc size macro, or by just bein passed that info from the code responsible for maintaining the C runtime environment (arch/arm/lib/crt0.C in the ARM case) -- via the 'chunk_id' method I described earlier in https://www.mail-archive.com/u-boot@lists.denx.de/msg191898.html

There is a *VERY* *BIG* *PROBLEM* to the whole idea of malloc-before-SDRAM:

We could manage to transfer the pre-SDRAM-init maloc arena content to the post-SDRAM-init malloc arena, thus preserving all pre- SDRAM-init mallocs() across the stack switch.

*BUT*: all (variable-stored) pointers to pre-SDRAM-malloc'ed space will become dangling pointers, because we have no easy way of telling where these pointers are and relocating them.

This means that the mallocs() done before SDRAM init are lost and that al pointers to them /will/ become dangling, including any copies and derivatives of these; all code which depend these pointers and their will have to handle the situation, which will prove *quite* complex or, to be blunter, will prove not actually done, and will prove so through weird bugs creeping up that we'll take some time to relate to malloc issues, and to fix.

Of course, we could just decide that any malloc done before SDRAM init is lost and that user code should deal with it. But I fear it will do so just as badly as it would handle the "maloc transfer" alternative I just described. Plus, if the malloc'ed data contains hardware module state or any non-recoverable-again information, then we *cannot* just drop it.

Now, there is a systemic solution: that all code which stores a malloc-arena-based pointer value in memory call a system-provided function which, if running pre-SDRAM-init, would record the location of the pointer in a list; upon C environment switch from pre- to post-SDRAM contexts, the list would be run through and each pointer would be 'relocated' from the old to the new maloc arena. Post-init, the functon would not do any recording. That causes a slight performance hit on mallocs, but I don't think it'll affect boot time that much.

Only code which has to run before SDRAM init would have to use the function.

The benefit of this approach is that maloc'ed space would remain malloc'ed after SDRAM init and declared pointers to it would remain valid. Code which has to malloc before SDRAM init would not have to re-malloc or fix anything, or even realize the stack and malloc arena have moved, as long as it has declared its malloc-related pointer(s).

Of course the list would be limited. But seeing as we'd be running in a tight context and only to get SDRAM running, the list would not extend much.

I imagine a scheme where the list is kept in the malloc arena. The malloc space itself would still grow 'down' from the stack top while the pointer list would grow 'up' from the stack 'bottom' limit.

I could whip up an RFC patch (with ARM support, to be extended to other platforms as my recent experience showed I'm not that good at NIOS2 for instance) if people are interested.

Regards, Simon

Amicalement,

Hi Albert,

On 11 November 2015 at 23:57, Albert ARIBAUD albert.u.boot@aribaud.net wrote:

Hello Simon,

On Wed, 11 Nov 2015 14:49:05 -0700, Simon Glass sjg@chromium.org wrote:

...

Hi Fabio,

On 11 November 2015 at 14:24, Fabio Estevam festevam@gmail.com wrote:

...

Hi Simon,

On Wed, Nov 11, 2015 at 7:08 PM, Simon Glass sjg@chromium.org wrote:

...

That test is intended to avoid setting up simple malloc() if we plan to use full malloc() in SPL. Of course, full malloc() is set up a little later (in spl_init()). But we should not need both - either we use simple malloc() or full malloc().

But for your board I see:

$ grep CONFIG_SYS_SPL_MALLOC_SIZE b/mx6sabresd_spl/u-boot.cfg #define CONFIG_SYS_SPL_MALLOC_SIZE 0x3200000

So you will not be able to use simple malloc(). I'd suggest calling spl_init() from board_init_f() if you need malloc() there. But it

Yes, I do call spl_init() from board_init_f() prior to calling malloc() and this has been working fine prior to 5ba534d247d418.

...

presumably needs to be done after you have SDRAM up. So perhaps

The trick here is that I need malloc to work prior to have SDRAM configured.

On cgtqmx6eval we need to read SPI NOR to determine what kind of DDR we will need to configure.

Otavio has sent the SPL support for cgtqmx6eval, but it has not reached U-boot mainline yet.

I am reproducing the problem on a mx6sabresd_spl target with the simple example code I sent previously.

I see. That's not a use case I have seen before.

I'd suggest changing the #ifdef to always set up early malloc() if CONFIG_SYS_MALLOC_F is set. There will of course be a new malloc() once spi_init() is called, but that does not matter.

In your patch, please be careful to add some docs to the README on this point (the early malloc() is only there to permit DRAM init, etc.). It could get quite confusing...

I'm not sure all details are clear to me so let me chime in on this.

IIUC, what Fabio needs is a working C runtime including malloc, based on some IRAM rather than SDRAM.

This means there will be two phases where malloc can be used, the usual one after SDRAM init, and a new one before SDRAM init.

Of course, the amount of memory reserved for the malloc arena cannot be the same in IRAM as it will be in SDRAM, meaning that the routine which reserves the arena must handle both cases, by detecting whether it is running before or after SDRAM init and choosing the right malloc size macro, or by just bein passed that info from the code responsible for maintaining the C runtime environment (arch/arm/lib/crt0.C in the ARM case) -- via the 'chunk_id' method I described earlier in https://www.mail-archive.com/u-boot@lists.denx.de/msg191898.html

There is a *VERY* *BIG* *PROBLEM* to the whole idea of malloc-before-SDRAM:

We could manage to transfer the pre-SDRAM-init maloc arena content to the post-SDRAM-init malloc arena, thus preserving all pre- SDRAM-init mallocs() across the stack switch.

*BUT*: all (variable-stored) pointers to pre-SDRAM-malloc'ed space will become dangling pointers, because we have no easy way of telling where these pointers are and relocating them.

This means that the mallocs() done before SDRAM init are lost and that al pointers to them /will/ become dangling, including any copies and derivatives of these; all code which depend these pointers and their will have to handle the situation, which will prove *quite* complex or, to be blunter, will prove not actually done, and will prove so through weird bugs creeping up that we'll take some time to relate to malloc issues, and to fix.

Of course, we could just decide that any malloc done before SDRAM init is lost and that user code should deal with it. But I fear it will do so just as badly as it would handle the "maloc transfer" alternative I just described. Plus, if the malloc'ed data contains hardware module state or any non-recoverable-again information, then we *cannot* just drop it.

Now, there is a systemic solution: that all code which stores a malloc-arena-based pointer value in memory call a system-provided function which, if running pre-SDRAM-init, would record the location of the pointer in a list; upon C environment switch from pre- to post-SDRAM contexts, the list would be run through and each pointer would be 'relocated' from the old to the new maloc arena. Post-init, the functon would not do any recording. That causes a slight performance hit on mallocs, but I don't think it'll affect boot time that much.

Only code which has to run before SDRAM init would have to use the function.

The benefit of this approach is that maloc'ed space would remain malloc'ed after SDRAM init and declared pointers to it would remain valid. Code which has to malloc before SDRAM init would not have to re-malloc or fix anything, or even realize the stack and malloc arena have moved, as long as it has declared its malloc-related pointer(s).

Of course the list would be limited. But seeing as we'd be running in a tight context and only to get SDRAM running, the list would not extend much.

I imagine a scheme where the list is kept in the malloc arena. The malloc space itself would still grow 'down' from the stack top while the pointer list would grow 'up' from the stack 'bottom' limit.

I could whip up an RFC patch (with ARM support, to be extended to other platforms as my recent experience showed I'm not that good at NIOS2 for instance) if people are interested.

I'm really not sure that this problem is worth solving.

We have quite a hard boundary between board_init_f() and board_init_r() in U-Boot proper. For some platforms the memory available in the former is not available in the latter. People understand (I think) that pre-relocation stuff goes away and exists only to assist with bringing up the new environment.

Driver model is started up in board_init_f() but then the whole thing is thrown away and started again in board_init_r(). So if someone kept a device pointer around for later (e.g. in global_data) it would not work (Don't Do That!). This approach has not caused any reported problems yet.

While you can copy memory and fix up pointers returned by malloc(), pointers to rodata and other things may still exist. It would be tedious or error-prone to relocate these.

My preference would be to keep it simple, and just make it clear that board_init_f() may have an early malloc(), but even if it does it goes away in board_init_r(). That rule can apply to SPL just as easily as U-Boot proper.

Regards, Simon

Hi Albert,

On 12 November 2015 at 09:13, Albert ARIBAUD albert.u.boot@aribaud.net wrote:

Hello Simon,

On Thu, 12 Nov 2015 08:59:54 -0700, Simon Glass sjg@chromium.org wrote:

...

Hi Albert,

On 11 November 2015 at 23:57, Albert ARIBAUD albert.u.boot@aribaud.net wrote:

...

Hello Simon,

On Wed, 11 Nov 2015 14:49:05 -0700, Simon Glass sjg@chromium.org wrote:

...

Hi Fabio,

On 11 November 2015 at 14:24, Fabio Estevam festevam@gmail.com wrote:

...

Hi Simon,

On Wed, Nov 11, 2015 at 7:08 PM, Simon Glass sjg@chromium.org wrote:

...

That test is intended to avoid setting up simple malloc() if we plan to use full malloc() in SPL. Of course, full malloc() is set up a little later (in spl_init()). But we should not need both - either we use simple malloc() or full malloc().

But for your board I see:

$ grep CONFIG_SYS_SPL_MALLOC_SIZE b/mx6sabresd_spl/u-boot.cfg #define CONFIG_SYS_SPL_MALLOC_SIZE 0x3200000

So you will not be able to use simple malloc(). I'd suggest calling spl_init() from board_init_f() if you need malloc() there. But it

Yes, I do call spl_init() from board_init_f() prior to calling malloc() and this has been working fine prior to 5ba534d247d418.

...

presumably needs to be done after you have SDRAM up. So perhaps

The trick here is that I need malloc to work prior to have SDRAM configured.

On cgtqmx6eval we need to read SPI NOR to determine what kind of DDR we will need to configure.

Otavio has sent the SPL support for cgtqmx6eval, but it has not reached U-boot mainline yet.

I am reproducing the problem on a mx6sabresd_spl target with the simple example code I sent previously.

I see. That's not a use case I have seen before.

I'd suggest changing the #ifdef to always set up early malloc() if CONFIG_SYS_MALLOC_F is set. There will of course be a new malloc() once spi_init() is called, but that does not matter.

In your patch, please be careful to add some docs to the README on this point (the early malloc() is only there to permit DRAM init, etc.). It could get quite confusing...

I'm not sure all details are clear to me so let me chime in on this.

IIUC, what Fabio needs is a working C runtime including malloc, based on some IRAM rather than SDRAM.

This means there will be two phases where malloc can be used, the usual one after SDRAM init, and a new one before SDRAM init.

Of course, the amount of memory reserved for the malloc arena cannot be the same in IRAM as it will be in SDRAM, meaning that the routine which reserves the arena must handle both cases, by detecting whether it is running before or after SDRAM init and choosing the right malloc size macro, or by just bein passed that info from the code responsible for maintaining the C runtime environment (arch/arm/lib/crt0.C in the ARM case) -- via the 'chunk_id' method I described earlier in https://www.mail-archive.com/u-boot@lists.denx.de/msg191898.html

There is a *VERY* *BIG* *PROBLEM* to the whole idea of malloc-before-SDRAM:

We could manage to transfer the pre-SDRAM-init maloc arena content to the post-SDRAM-init malloc arena, thus preserving all pre- SDRAM-init mallocs() across the stack switch.

*BUT*: all (variable-stored) pointers to pre-SDRAM-malloc'ed space will become dangling pointers, because we have no easy way of telling where these pointers are and relocating them.

This means that the mallocs() done before SDRAM init are lost and that al pointers to them /will/ become dangling, including any copies and derivatives of these; all code which depend these pointers and their will have to handle the situation, which will prove *quite* complex or, to be blunter, will prove not actually done, and will prove so through weird bugs creeping up that we'll take some time to relate to malloc issues, and to fix.

Of course, we could just decide that any malloc done before SDRAM init is lost and that user code should deal with it. But I fear it will do so just as badly as it would handle the "maloc transfer" alternative I just described. Plus, if the malloc'ed data contains hardware module state or any non-recoverable-again information, then we *cannot* just drop it.

Now, there is a systemic solution: that all code which stores a malloc-arena-based pointer value in memory call a system-provided function which, if running pre-SDRAM-init, would record the location of the pointer in a list; upon C environment switch from pre- to post-SDRAM contexts, the list would be run through and each pointer would be 'relocated' from the old to the new maloc arena. Post-init, the functon would not do any recording. That causes a slight performance hit on mallocs, but I don't think it'll affect boot time that much.

Only code which has to run before SDRAM init would have to use the function.

The benefit of this approach is that maloc'ed space would remain malloc'ed after SDRAM init and declared pointers to it would remain valid. Code which has to malloc before SDRAM init would not have to re-malloc or fix anything, or even realize the stack and malloc arena have moved, as long as it has declared its malloc-related pointer(s).

Of course the list would be limited. But seeing as we'd be running in a tight context and only to get SDRAM running, the list would not extend much.

I imagine a scheme where the list is kept in the malloc arena. The malloc space itself would still grow 'down' from the stack top while the pointer list would grow 'up' from the stack 'bottom' limit.

I could whip up an RFC patch (with ARM support, to be extended to other platforms as my recent experience showed I'm not that good at NIOS2 for instance) if people are interested.

I'm really not sure that this problem is worth solving.

We have quite a hard boundary between board_init_f() and board_init_r() in U-Boot proper. For some platforms the memory available in the former is not available in the latter. People understand (I think) that pre-relocation stuff goes away and exists only to assist with bringing up the new environment.

(I fear that half the question is what people understand, and then the other half is what they think they understand and have not -- here, since malloc and DM is available before and after SDRAM init, people might think it is just the *same* DM across the transition -- or, eventually, want it to be.)

Possibly, but we can add documentation in the code to correct this view.

...

Driver model is started up in board_init_f() but then the whole thing is thrown away and started again in board_init_r(). So if someone kept a device pointer around for later (e.g. in global_data) it would not work (Don't Do That!). This approach has not caused any reported problems yet.

Isn't it a potential problem that the driver model is started (and therefore some devices intialized) and then thrown away? Some devices might not like (or even permit) being initialized twice, or that may cause glitches, for instance.

Yes. This is called out in the driver model README for example. It hasn't caused any problems yet though.

...

While you can copy memory and fix up pointers returned by malloc(), pointers to rodata and other things may still exist. It would be tedious or error-prone to relocate these.

rodata does not move during SDRAM init (and the stack swap we're considering here). It moves during U-Boot relocation though; and yes, pointers to ro data pre-relocation will become stale after relocation, but this has been the case even way before DM existed.

Conceptually I am trying to make board_init_f() and board_init_r() mean the same thing in SPL and U-Boot proper. That is, the SDRAM init happens in board_init_f() and there is an early malloc() available then. Once you get to board_init_r() you have a new malloc() which sits in SDRAM. For both SPL and U-Boot proper I don't think the boundary should be when SDRAM is inited, but when board_init_r() is called.

...

My preference would be to keep it simple, and just make it clear that board_init_f() may have an early malloc(), but even if it does it goes away in board_init_r(). That rule can apply to SPL just as easily as U-Boot proper.

IMO, keeping it simple does not play well with making DM and malloc available pre-SDRAM init :) --- but don't miscontrue my meaning: I'm fine with DM and malloc pre-SDRAM on platforms which have enough IRAM / SRAM / lockable cache / whatever for it. My point is just that sooner or later, someone will start wanting not to do again after SDRAM init what they've already done before it, because it avoids double HW init issues, becaue it saves time, etc. And that's going to be way sooner than later, IMO.

Yes that is quite possibility true. I'm happy to review any patches if it helps. My comment about it possibly not being worthwhile solving is just that. Things have a way of changing, so what looks unnecessary now might look essential tomorrow. In a way, the simple malloc() fits into that category.

Regards, Simon