diff mbox

[U-Boot] Revert "arm: Switch 32-bit ARM to using generic global_data setup"

Message ID CAOMZO5CjUmjHpD-LvTEz8=nxtuFN-DuKZNzgAxk6nV3rnbQvAg@mail.gmail.com
State Superseded
Headers show

Commit Message

Fabio Estevam Nov. 10, 2015, 2:50 p.m. UTC
Hi Simon,

On Tue, Nov 10, 2015 at 12:41 PM, Simon Glass <sjg@chromium.org> wrote:

> We're at the very start the release process, so I wonder if we can try
> to figure out what is wrong here?
>
> Is it because malloc() is not working, perhaps?

Yes, exactly. malloc() is not working.

Issue happens on Congatec board, but the SPL patch for Congatec has
not reached mainline yet.

It is simple to reproduce the problem on a mx6sabresd board with the
following change:

It is the ame issue I reported back in August:
http://lists.denx.de/pipermail/u-boot/2015-August/222406.html

After I followed your suggestion to call spl_init() prior to malloc()
the issue went away.

However with 5ba534d247d4 applied, this no longer works, so I am
interested in knowing the appropriate way to fix this malloc() issue
inside SPL.

> The C code should be roughly equivalent to the assembly code. Albert
> found a problem with the code on toolchain 5.2.1 to do with setting
> 'gd', so may have some thoughts on this. But this might be a different
> problem.

I am using 4.7.3 here.

Thanks,

Fabio Estevam

Comments

Simon Glass Nov. 10, 2015, 3:21 p.m. UTC | #1
Hi Fabio,

On 10 November 2015 at 06:50, Fabio Estevam <festevam@gmail.com> wrote:
> Hi Simon,
>
> On Tue, Nov 10, 2015 at 12:41 PM, Simon Glass <sjg@chromium.org> wrote:
>
>> We're at the very start the release process, so I wonder if we can try
>> to figure out what is wrong here?
>>
>> Is it because malloc() is not working, perhaps?
>
> Yes, exactly. malloc() is not working.
>
> Issue happens on Congatec board, but the SPL patch for Congatec has
> not reached mainline yet.
>
> It is simple to reproduce the problem on a mx6sabresd board with the
> following change:
>
> --- a/board/freescale/mx6sabresd/mx6sabresd.c
> +++ b/board/freescale/mx6sabresd/mx6sabresd.c
> @@ -30,6 +30,7 @@
>  #include "../common/pfuze.h"
>  #include <asm/arch/mx6-ddr.h>
>  #include <usb.h>
> +#include <malloc.h>
>
>  DECLARE_GLOBAL_DATA_PTR;
>
> @@ -833,6 +834,8 @@ static void spl_dram_init(void)
>
>  void board_init_f(ulong dummy)
>  {
> +    void __iomem *ptr;
> +
>      /* setup AIPS and disable watchdog */
>      arch_cpu_init();
>
> @@ -848,6 +851,12 @@ void board_init_f(ulong dummy)
>      /* UART clocks enabled and gd valid - init serial console */
>      preloader_console_init();
>
> +    spl_init();
> +
> +    ptr = malloc(64);
> +    if (!ptr)
> +        puts("******* malloc returned NULL\n");
> +

Is this patch against mainline or does your tree have other changes?

>      /* DDR initialization */
>      spl_dram_init();
>
> It is the ame issue I reported back in August:
> http://lists.denx.de/pipermail/u-boot/2015-August/222406.html
>
> After I followed your suggestion to call spl_init() prior to malloc()
> the issue went away.
>
> However with 5ba534d247d4 applied, this no longer works, so I am
> interested in knowing the appropriate way to fix this malloc() issue
> inside SPL.

Are you using CONFIG_SYS_MALLOC_F?

>
>> The C code should be roughly equivalent to the assembly code. Albert
>> found a problem with the code on toolchain 5.2.1 to do with setting
>> 'gd', so may have some thoughts on this. But this might be a different
>> problem.
>
> I am using 4.7.3 here.
>
> Thanks,
>
> Fabio Estevam

Regards,
Simon
Fabio Estevam Nov. 10, 2015, 3:38 p.m. UTC | #2
On Tue, Nov 10, 2015 at 1:21 PM, Simon Glass <sjg@chromium.org> wrote:

> Is this patch against mainline or does your tree have other changes?

This change is against a clean mainline tree.


>>      /* DDR initialization */
>>      spl_dram_init();
>>
>> It is the ame issue I reported back in August:
>> http://lists.denx.de/pipermail/u-boot/2015-August/222406.html
>>
>> After I followed your suggestion to call spl_init() prior to malloc()
>> the issue went away.
>>
>> However with 5ba534d247d4 applied, this no longer works, so I am
>> interested in knowing the appropriate way to fix this malloc() issue
>> inside SPL.
>
> Are you using CONFIG_SYS_MALLOC_F?

No, I am not, but I have also tried adding CONFIG_SYS_MALLOC_F_LEN
inside configs/mx6sabresd_spl_defconfig, but it did not help.

Regards,

Fabio Estevam
Fabio Estevam Nov. 10, 2015, 9:16 p.m. UTC | #3
On Tue, Nov 10, 2015 at 1:38 PM, Fabio Estevam <festevam@gmail.com> wrote:

>> Are you using CONFIG_SYS_MALLOC_F?
>
> No, I am not, but I have also tried adding CONFIG_SYS_MALLOC_F_LEN
> inside configs/mx6sabresd_spl_defconfig, but it did not help.

Sorry, you asked for CONFIG_SYS_MALLOC_F. I do have CONFIG_SYS_MALLOC_F selected

Thanks
Simon Glass Nov. 10, 2015, 9:19 p.m. UTC | #4
Hi Fabio,

On 10 November 2015 at 14:16, Fabio Estevam <festevam@gmail.com> wrote:
> On Tue, Nov 10, 2015 at 1:38 PM, Fabio Estevam <festevam@gmail.com> wrote:
>
>>> Are you using CONFIG_SYS_MALLOC_F?
>>
>> No, I am not, but I have also tried adding CONFIG_SYS_MALLOC_F_LEN
>> inside configs/mx6sabresd_spl_defconfig, but it did not help.
>
> Sorry, you asked for CONFIG_SYS_MALLOC_F. I do have CONFIG_SYS_MALLOC_F selected

Then I wonder why malloc() is failing? Is it because there is too much
being allocated, or is the simple malloc region not being set up
correctly?

Regards,
Simon
Fabio Estevam Nov. 10, 2015, 9:23 p.m. UTC | #5
Hi Simon,

On Tue, Nov 10, 2015 at 7:19 PM, Simon Glass <sjg@chromium.org> wrote:

> Then I wonder why malloc() is failing? Is it because there is too much

In the example I sent I only allocate 64 bytes. malloc() does fail
even if I decrease this number.

> being allocated, or is the simple malloc region not being set up
> correctly?

This puzzles me too. I am not very familiar with this code and I don't
know the reason why reverting 5ba534d247d41 'fixes' the problem.

Regards,

Fabio Estevam
Simon Glass Nov. 10, 2015, 9:29 p.m. UTC | #6
Hi Fabio,

On 10 November 2015 at 14:23, Fabio Estevam <festevam@gmail.com> wrote:
> Hi Simon,
>
> On Tue, Nov 10, 2015 at 7:19 PM, Simon Glass <sjg@chromium.org> wrote:
>
>> Then I wonder why malloc() is failing? Is it because there is too much
>
> In the example I sent I only allocate 64 bytes. malloc() does fail
> even if I decrease this number.
>
>> being allocated, or is the simple malloc region not being set up
>> correctly?
>
> This puzzles me too. I am not very familiar with this code and I don't
> know the reason why reverting 5ba534d247d41 'fixes' the problem.

Are you able to check what is happening in malloc_simple()? It is a
really simple function.

Regards,
Simon
Fabio Estevam Nov. 10, 2015, 9:47 p.m. UTC | #7
On Tue, Nov 10, 2015 at 7:29 PM, Simon Glass <sjg@chromium.org> wrote:

> Are you able to check what is happening in malloc_simple()? It is a
> really simple function.

Yes, I turned on debug inside malloc_simple() and it returns NULL:

U-Boot SPL 2015.10-00523-ge490ad2-dirty (Nov 10 2015 - 19:44:06)
malloc_simple: size=40, ptr=40, limit=1000
**** malloc_simple returns NULL
******* malloc returned NULL

map_sysmem() is returning NULL inside malloc_simple().

Regards,

Fabio Estevam
Simon Glass Nov. 10, 2015, 9:52 p.m. UTC | #8
Hi Fabio,

On 10 November 2015 at 14:47, Fabio Estevam <festevam@gmail.com> wrote:
> On Tue, Nov 10, 2015 at 7:29 PM, Simon Glass <sjg@chromium.org> wrote:
>
>> Are you able to check what is happening in malloc_simple()? It is a
>> really simple function.
>
> Yes, I turned on debug inside malloc_simple() and it returns NULL:
>
> U-Boot SPL 2015.10-00523-ge490ad2-dirty (Nov 10 2015 - 19:44:06)
> malloc_simple: size=40, ptr=40, limit=1000
> **** malloc_simple returns NULL
> ******* malloc returned NULL
>
> map_sysmem() is returning NULL inside malloc_simple().

That suggests that gd->malloc_base is not being set up, or is being
overwritten later. Presumably it should not be NULL?

Is it possible that you have a memset() somewhere which is zeroing gd?
It would be after board_init_f_mem() and before your malloc().

Regards,
Simon
Fabio Estevam Nov. 11, 2015, 8:19 p.m. UTC | #9
On Tue, Nov 10, 2015 at 7:52 PM, Simon Glass <sjg@chromium.org> wrote:

> That suggests that gd->malloc_base is not being set up, or is being
> overwritten later. Presumably it should not be NULL?

Yes, gd->malloc_base is NULL. I just prepared a patch to fix this
issue and will submit it soon.

Regards,

Fabio Estevam
diff mbox

Patch

--- a/board/freescale/mx6sabresd/mx6sabresd.c
+++ b/board/freescale/mx6sabresd/mx6sabresd.c
@@ -30,6 +30,7 @@ 
 #include "../common/pfuze.h"
 #include <asm/arch/mx6-ddr.h>
 #include <usb.h>
+#include <malloc.h>

 DECLARE_GLOBAL_DATA_PTR;

@@ -833,6 +834,8 @@  static void spl_dram_init(void)

 void board_init_f(ulong dummy)
 {
+    void __iomem *ptr;
+
     /* setup AIPS and disable watchdog */
     arch_cpu_init();

@@ -848,6 +851,12 @@  void board_init_f(ulong dummy)
     /* UART clocks enabled and gd valid - init serial console */
     preloader_console_init();

+    spl_init();
+
+    ptr = malloc(64);
+    if (!ptr)
+        puts("******* malloc returned NULL\n");
+
     /* DDR initialization */
     spl_dram_init();