rockchip: Reconfigure the malloc based to point to system memory

PATCH New Sjoerd Simons 2 participants 4 messages Oct 1, 2015

Subsystems (3)

Arm (32-bit) Arm sub-architectures Arm Rockchip

lore

Share & tools

lore

Sjoerd Simons

author

2015-10-01 09:10 UTC #0 lore raw

PATCH 1/1 rockchip: Reconfigure the malloc based to point to system memory

When malloc_base initially gets setup in the SPL it is based on the current (early) stack pointer, which for rockchip is pointing into SRAM. This means simple memory allocations happen in SRAM space, which is somewhat unfortunate. Specifically a bounce buffer for the mmc allocated in SRAM space seems to cause the mmc engine to stall/fail causing timeouts and a failure to load the main u-boot image.

To resolve this, reconfigure the malloc_base to start at the relocated stack pointer after DRAM has been setup.

For reference, things did work fine on rockchip before 596380db was merged to fix memalign_simple due to a combination of rockchip SDRAM starting at address 0 and the dw_mmc driver not checking errors from bounce_buffer_start. As a result, when a bounce buffer needed to be allocated mem_align simple would fail and return NULL. The mmc driver ignored the error and happily continued with the bounce buffer address being set to 0, which just happened to work fine..

Signed-off-by Sjoerd Simons <[email protected]>

---
A potentially better fix for this issue would be to reconfigure the malloc_base in spl_relocate_stack_gd following the same steps as is done for the initial setup. However at this point in the release cycle i preferred to do a minimal rockchip only fix (so those boards become bootable again) for this issue to minimize the potential impact on other boards.

 arch/arm/mach-rockchip/board-spl.c | 4 ++++
 1 file changed, 4 insertions(+)

arch/arm/mach-rockchip/board-spl.c +4 -0

index a241d96..5daced7 100644
@@ -217,6 +217,10 @@ void board_init_f(ulong dummy)
 		debug("DRAM init failed: %d\n", ret);
 		return;
 	}
+
+	/* Now that DRAM is initialized setup base pointer for simple malloc
+	 * into RAM */
+	gd->malloc_base = CONFIG_SPL_STACK_R_ADDR;
 }
 
 static int setup_led(void)

Signature · 1 line

2.5.3

Hans de Goede

2015-10-01 10:08 UTC #1 lore raw

PATCH 1/1 rockchip: Reconfigure the malloc based to point to system memory

Hi Sjoerd,

Quoted 18 lines · Sjoerd Simons

When malloc_base initially gets setup in the SPL it is based on the current (early) stack pointer, which for rockchip is pointing into SRAM. This means simple memory allocations happen in SRAM space, which is somewhat unfortunate. Specifically a bounce buffer for the mmc allocated in SRAM space seems to cause the mmc engine to stall/fail causing timeouts and a failure to load the main u-boot image.
To resolve this, reconfigure the malloc_base to start at the relocated stack pointer after DRAM has been setup.
For reference, things did work fine on rockchip before 596380db was merged to fix memalign_simple due to a combination of rockchip SDRAM starting at address 0 and the dw_mmc driver not checking errors from bounce_buffer_start. As a result, when a bounce buffer needed to be allocated mem_align simple would fail and return NULL. The mmc driver ignored the error and happily continued with the bounce buffer address being set to 0, which just happened to work fine..

Quoted 6 lines

Signed-off-by Sjoerd Simons <[email protected]>
---
A potentially better fix for this issue would be to reconfigure the malloc_base in spl_relocate_stack_gd following the same steps as is done for the initial setup.

I actually have a patch series pending for this:

http://patchwork.ozlabs.org/patch/517191/
http://patchwork.ozlabs.org/patch/517194/
http://patchwork.ozlabs.org/patch/517193/
http://patchwork.ozlabs.org/patch/517195/
http://patchwork.ozlabs.org/patch/517196/

(I've omitted 2 uninteresting patches)

Your review of / input on this series would be appreciated.

Quoted 4 lines

However at this point in the release cycle i
preferred to do a minimal rockchip only fix (so those boards become bootable again) for this issue to minimize the potential impact on other boards.

I agree that a minimal rockchip only fix likely is best at this time, however your fix seems wrong:

Quoted 8 lines

  arch/arm/mach-rockchip/board-spl.c | 4 ++++
  1 file changed, 4 insertions(+)

arch/arm/mach-rockchip/board-spl.c +0 -0

index a241d96..5daced7 100644

arch/arm/mach-rockchip/board-spl.c void board_init_f(ulong dummy) · line 226

Quoted 10 lines

arch/arm/mach-rockchip/board-spl.c +4 -0

@@ -217,6 +217,10 @@ void board_init_f(ulong dummy)
  		debug("DRAM init failed: %d\n", ret);
  		return;
  	}
+
+	/* Now that DRAM is initialized setup base pointer for simple malloc
+	 * into RAM */
+	gd->malloc_base = CONFIG_SPL_STACK_R_ADDR;
  }
 
  static int setup_led(void)

SPL_STACK_R_ADDR is where the stack will be put by spl_relocate_stack_gd so now you've the stack and the heap overlapping.

You likely will want to make CONFIG_SPL_STACK_R_ADDR be something like 0x80000 and then do:

	gd->malloc_base = 0x00000000;
	gd->malloc_limit = CONFIG_SPL_STACK_R_ADDR;
	gd->malloc_ptr = 0;

Here to stop the 2 from overlapping, things are likely working with your patch since you're not resetting gd->malloc_ptr, so you keep space for the stack for whatever that is set to at that point.

Regards,

Hans

Sjoerd Simons

author

2015-10-01 11:25 UTC #2 lore raw

PATCH 1/1 rockchip: Reconfigure the malloc based to point to system memory

Hey Hans,

Quoted 44 lines · Hans de Goede

Hi Sjoerd,
Quoted 25 lines · Sjoerd Simons
When malloc_base initially gets setup in the SPL it is based on the current (early) stack pointer, which for rockchip is pointing into SRAM.
This means simple memory allocations happen in SRAM space, which is somewhat unfortunate. Specifically a bounce buffer for the mmc allocated
in SRAM space seems to cause the mmc engine to stall/fail causing timeouts and a failure to load the main u-boot image.
To resolve this, reconfigure the malloc_base to start at the relocated
stack pointer after DRAM has been setup.
For reference, things did work fine on rockchip before 596380db was merged to fix memalign_simple due to a combination of rockchip SDRAM
starting at address 0 and the dw_mmc driver not checking errors from
bounce_buffer_start. As a result, when a bounce buffer needed to be allocated mem_align simple would fail and return NULL. The mmc driver
ignored the error and happily continued with the bounce buffer address
being set to 0, which just happened to work fine..
Quoted 7 lines
Signed-off-by Sjoerd Simons <[email protected]>
---
A potentially better fix for this issue would be to reconfigure the malloc_base in spl_relocate_stack_gd following the same steps as is done
for the initial setup.
I actually have a patch series pending for this:
http://patchwork.ozlabs.org/patch/517191/
http://patchwork.ozlabs.org/patch/517194/
http://patchwork.ozlabs.org/patch/517193/
http://patchwork.ozlabs.org/patch/517195/
http://patchwork.ozlabs.org/patch/517196/

Quoted 3 lines

(I've omitted 2 uninteresting patches)
Your review of / input on this series would be appreciated.

Cool, I'll try to make some time to give that a closer look.

Quoted 28 lines

Quoted 5 lines
However at this point in the release cycle i
preferred to do a minimal rockchip only fix (so those boards become bootable again) for this issue to minimize the potential impact on other
boards.
I agree that a minimal rockchip only fix likely is best at this time, however your fix seems wrong:
Quoted 19 lines
  arch/arm/mach-rockchip/board-spl.c | 4 ++++
  1 file changed, 4 insertions(+)
arch/arm/mach renamed from arch/arm/mach-rockchip/board-spl.c +0 -1
-rockchip/board-spl.c
index a241d96..5daced7 100644
arch/arm/mach-rockchip/board-spl.c +2 -0
@@ -217,6 +217,10 @@ void board_init_f(ulong dummy)
  		debug("DRAM init failed: %d\n", ret);
  		return;
  	}
+
+	/* Now that DRAM is initialized setup base pointer for
simple malloc
arch/arm/mach-rockchip/board-spl.c +2 -0
+	 * into RAM */
+	gd->malloc_base = CONFIG_SPL_STACK_R_ADDR;
  }

Quoted 5 lines

Quoted 1 line
static int setup_led(void)
SPL_STACK_R_ADDR is where the stack will be put by spl_relocate_stack_gd
so now you've the stack and the heap overlapping.

If i'm not mistaken the stack grows downward, while the heap grows upwards so there shouldn't be a conflict. In my understanding the memory layout after spl_relocate_stack_gd should look something like this

0x0
.
<misc, other>
.
CONFIG_SPL_STACK_ADDR_R - sizeof(gd_t): relocated Stack pointer (growing downwards) CONFIG_SPL_STACK_ADDR_R - sizeof(gd_t): global data
CONFIG_SPL_STACK_ADDR_R : Start of heap (growing upward>
CONFIG_SPL_STACK_ADDR_R + CONFIG_SYS_MALLOC_F_LEN: End of heap

I'm pretty sure that's correct, well either that, or i'm missing something obvious and spl_relocate_stack_gd doesn't make any sense (as it als puts the new stack pointer to start at the gd location) :)

Quoted 4 lines

You likely will want to make CONFIG_SPL_STACK_R_ADDR be something like 0x80000
and then do:

Quoted 8 lines

	gd->malloc_base = 0x00000000;
	gd->malloc_limit = CONFIG_SPL_STACK_R_ADDR;
	gd->malloc_ptr = 0;
Here to stop the 2 from overlapping, things are likely working with your patch since you're not resetting gd->malloc_ptr, so you keep space
for the stack for whatever that is set to at that point.

Actually i've been tempted to put the malloc_base at 0x0, but that would mean the first malloc_simple call will return 0 so is indistinguishable from an error.

Signature · 2 lines

Sjoerd Simons
Collabora Ltd.

Hans de Goede

2015-10-01 12:19 UTC #3 lore raw

PATCH 1/1 rockchip: Reconfigure the malloc based to point to system memory

Hi,

Quoted 101 lines · Sjoerd Simons

Hey Hans,
Quoted 44 lines · Hans de Goede
Hi Sjoerd,
Quoted 25 lines · Sjoerd Simons
When malloc_base initially gets setup in the SPL it is based on the current (early) stack pointer, which for rockchip is pointing into SRAM.
This means simple memory allocations happen in SRAM space, which is somewhat unfortunate. Specifically a bounce buffer for the mmc allocated
in SRAM space seems to cause the mmc engine to stall/fail causing timeouts and a failure to load the main u-boot image.
To resolve this, reconfigure the malloc_base to start at the relocated
stack pointer after DRAM has been setup.
For reference, things did work fine on rockchip before 596380db was merged to fix memalign_simple due to a combination of rockchip SDRAM
starting at address 0 and the dw_mmc driver not checking errors from
bounce_buffer_start. As a result, when a bounce buffer needed to be allocated mem_align simple would fail and return NULL. The mmc driver
ignored the error and happily continued with the bounce buffer address
being set to 0, which just happened to work fine..
Quoted 7 lines
Signed-off-by Sjoerd Simons <[email protected]>
---
A potentially better fix for this issue would be to reconfigure the malloc_base in spl_relocate_stack_gd following the same steps as is done
for the initial setup.
I actually have a patch series pending for this:
http://patchwork.ozlabs.org/patch/517191/
http://patchwork.ozlabs.org/patch/517194/
http://patchwork.ozlabs.org/patch/517193/
http://patchwork.ozlabs.org/patch/517195/
http://patchwork.ozlabs.org/patch/517196/
Quoted 3 lines
(I've omitted 2 uninteresting patches)
Your review of / input on this series would be appreciated.
Cool, I'll try to make some time to give that a closer look.
Quoted 28 lines
Quoted 5 lines
However at this point in the release cycle i
preferred to do a minimal rockchip only fix (so those boards become bootable again) for this issue to minimize the potential impact on other
boards.
I agree that a minimal rockchip only fix likely is best at this time, however your fix seems wrong:
Quoted 19 lines
   arch/arm/mach-rockchip/board-spl.c | 4 ++++
   1 file changed, 4 insertions(+)
arch/arm/mach renamed from arch/arm/mach-rockchip/board-spl.c +0 -1
-rockchip/board-spl.c
index a241d96..5daced7 100644
arch/arm/mach-rockchip/board-spl.c +2 -0
@@ -217,6 +217,10 @@ void board_init_f(ulong dummy)
   		debug("DRAM init failed: %d\n", ret);
   		return;
   	}
+
+	/* Now that DRAM is initialized setup base pointer for
simple malloc
arch/arm/mach-rockchip/board-spl.c +2 -0
+	 * into RAM */
+	gd->malloc_base = CONFIG_SPL_STACK_R_ADDR;
   }
Quoted 5 lines
Quoted 1 line
static int setup_led(void)
SPL_STACK_R_ADDR is where the stack will be put by spl_relocate_stack_gd
so now you've the stack and the heap overlapping.
If i'm not mistaken the stack grows downward, while the heap grows upwards so there shouldn't be a conflict. In my understanding the memory layout after spl_relocate_stack_gd should look something like this
0x0
.
<misc, other>
.
CONFIG_SPL_STACK_ADDR_R - sizeof(gd_t): relocated Stack pointer (growing downwards) CONFIG_SPL_STACK_ADDR_R - sizeof(gd_t): global data
CONFIG_SPL_STACK_ADDR_R : Start of heap (growing upward>
CONFIG_SPL_STACK_ADDR_R + CONFIG_SYS_MALLOC_F_LEN: End of heap

Quoted 3 lines

I'm pretty sure that's correct, well either that, or i'm missing something obvious and spl_relocate_stack_gd doesn't make any sense (as it als puts the new stack pointer to start at the gd location) :)

Ah yes you're right, and since the stack grows downwards I guess that CONFIG_SPL_STACK_ADDR_R already is not 0 for rockchip :)

You should probably still reset gd->malloc_ptr to 0, otherwise the new DRAM heap will begin at CONFIG_SPL_STACK_ADDR_R + gd->malloc_ptr and it will be only CONFIG_SYS_MALLOC_F_LEN - gd->malloc_ptr bytes large.

Regards,

Hans