| Message ID | 1296788903-7604-1-git-send-email-leiwen@marvell.com |
|---|---|
| State | Rejected |
| Headers | show |
On Fri, 4 Feb 2011 11:08:23 +0800 Lei Wen <leiwen@marvell.com> wrote: > Original env buf directly locate at stack lead large stack footprint > when call those env functions. It is not good when the system memory > is critical or only want the uboot run at restrict range, that is not > to touch the memory of other place at its best. > > So now this patch move the env buf to the heap area, which reduce the > area uboot need to touch. > > Signed-off-by: Lei Wen <leiwen@marvell.com> > --- > common/env_dataflash.c | 8 +++++++- > common/env_eeprom.c | 8 +++++++- > common/env_mgdisk.c | 10 +++++++++- > common/env_mmc.c | 9 ++++++++- > common/env_nand.c | 10 +++++++++- > common/env_nvram.c | 9 ++++++++- > common/env_sf.c | 9 ++++++++- > 7 files changed, 56 insertions(+), 7 deletions(-) NAND bits are Acked-by: Scott Wood <scottwood@freescale.com> -Scott
Dear Lei Wen, In message <1296788903-7604-1-git-send-email-leiwen@marvell.com> you wrote: > Original env buf directly locate at stack lead large stack footprint > when call those env functions. It is not good when the system memory > is critical or only want the uboot run at restrict range, that is not > to touch the memory of other place at its best. > > So now this patch move the env buf to the heap area, which reduce the > area uboot need to touch. In which way do you think this will save any memory? Your switch from automatic variables to manually allocated ones has, in my opinion several disadvantages: 1) It increases the code size. 2) It increases the probability of bugs like memory leaks etc. 3) Instead of using the stack, which gets allocated fully dynamically and gets freed guaranteed completely when the function returns, you now need at least the same space in the malloc arena - where you have to allocate it statically, and where allocations of buffers like that can easily lead to memory fragmentation, increasing the memory footprint further. Did you do any specific measurements of memory footprint, or what makes you believe your code is preferrable? Best regards, Wolfgang Denk
Hi Wolfgang, On Sat, Feb 5, 2011 at 5:18 AM, Wolfgang Denk <wd@denx.de> wrote: > Dear Lei Wen, > > In message <1296788903-7604-1-git-send-email-leiwen@marvell.com> you wrote: >> Original env buf directly locate at stack lead large stack footprint >> when call those env functions. It is not good when the system memory >> is critical or only want the uboot run at restrict range, that is not >> to touch the memory of other place at its best. >> >> So now this patch move the env buf to the heap area, which reduce the >> area uboot need to touch. > > In which way do you think this will save any memory? This patch is not intend to save memory... > > Your switch from automatic variables to manually allocated ones has, > in my opinion several disadvantages: > > 1) It increases the code size. Yes, I agree. > > 2) It increases the probability of bugs like memory leaks etc. > > 3) Instead of using the stack, which gets allocated fully dynamically > and gets freed guaranteed completely when the function returns, > you now need at least the same space in the malloc arena - where > you have to allocate it statically, and where allocations of > buffers like that can easily lead to memory fragmentation, > increasing the memory footprint further. > > Did you do any specific measurements of memory footprint, or what > makes you believe your code is preferrable? One of our project need to confine the ddr usage of uboot in the smallest case, not to pollute other area. So for us, the small stack is good one... For now the uboot is relocated to the end of the dram, and malloc area is almost a fix value, uboot would live happily in this area. But for env case, it allocate a range which could be large, due to the CONFIG_ENV_SIZE could be a big one, in the stack range. Because the stack is grown downwards, so it takes more memory range than it is allocated in the malloc method. Best regards, Lei
Dear Lei Wen, In message <AANLkTin-EBt+PA0TmdLBPAqo3pBjwmbNLdFUj2K-JOnX@mail.gmail.com> you wrote: > > > In which way do you think this will save any memory? > > This patch is not intend to save memory... Then I don't understand at all what the benefit of that patch would be. > One of our project need to confine the ddr usage of uboot in the smallest case, > not to pollute other area. So for us, the small stack is good one... So it is still about saving memory... > For now the uboot is relocated to the end of the dram, and malloc area is > almost a fix value, uboot would live happily in this area. But for env case, > it allocate a range which could be large, due to the CONFIG_ENV_SIZE > could be a big one, in the stack range. Because the stack is grown downwards, > so it takes more memory range than it is allocated in the malloc method. malloc arena and stack are adjacent. If you have enough free room in the malloc arena for the environment buffers, but cannot afford to have them on your stack, then this means your malloc arena has lots of unused space. Decrease the size of your malloc arena by the size of the environment buffer, and you reach the same goal as with your patch - actually you save more memory, as the code size of u-boot shrinks. Can you please provide exact numbers? How big is your RAM? What is the debug output of arch/*/lib/board.c without and with this patch on your system? How big is your environment resp. the environment sectors? Best regards, Wolfgang Denk
On 05/02/11 19:30, Wolfgang Denk wrote: > Dear Lei Wen, > > In message <AANLkTin-EBt+PA0TmdLBPAqo3pBjwmbNLdFUj2K-JOnX@mail.gmail.com> you wrote: >> >>> In which way do you think this will save any memory? >> >> This patch is not intend to save memory... > > Then I don't understand at all what the benefit of that patch would > be. > >> One of our project need to confine the ddr usage of uboot in the smallest case, >> not to pollute other area. So for us, the small stack is good one... > > So it is still about saving memory... > >> For now the uboot is relocated to the end of the dram, and malloc area is >> almost a fix value, uboot would live happily in this area. But for env case, >> it allocate a range which could be large, due to the CONFIG_ENV_SIZE >> could be a big one, in the stack range. Because the stack is grown downwards, >> so it takes more memory range than it is allocated in the malloc method. > Sounds like you have allocated too small a region for your stack - try increasing CONFIG_SYS_STACK_SIZE and decreasing CONFIG_SYS_MALLOC_LEN > malloc arena and stack are adjacent. If you have enough free room in > the malloc arena for the environment buffers, but cannot afford to > have them on your stack, then this means your malloc arena has lots of > unused space. Decrease the size of your malloc arena by the size of > the environment buffer, and you reach the same goal as with your > patch - actually you save more memory, as the code size of u-boot > shrinks. > > Can you please provide exact numbers? How big is your RAM? What is > the debug output of arch/*/lib/board.c without and with this patch on > your system? How big is your environment resp. the environment > sectors? > The only other thing I could think of that would make sense would be having the malloc heap in a completely different memory segment to the stack - odd, but possible Regards, Graeme
Dear Graeme Russ, In message <4D4D1987.5010405@gmail.com> you wrote: > > Sounds like you have allocated too small a region for your stack - try > increasing CONFIG_SYS_STACK_SIZE and decreasing CONFIG_SYS_MALLOC_LEN Most systems don't have anything like CONFIG_SYS_STACK_SIZE; the stack grows downward into free memory, and is only limited by the RAM size and the memory areas you might be using in RAM for example to load images or the like - the x86 implementation might be different, > The only other thing I could think of that would make sense would be having > the malloc heap in a completely different memory segment to the stack - > odd, but possible But I doubt that would actually save memory either. Best regards, Wolfgang Denk
Hi Wolfgang, On Sat, Feb 5, 2011 at 4:30 PM, Wolfgang Denk <wd@denx.de> wrote: > Dear Lei Wen, > > In message <AANLkTin-EBt+PA0TmdLBPAqo3pBjwmbNLdFUj2K-JOnX@mail.gmail.com> you wrote: >> >> > In which way do you think this will save any memory? >> >> This patch is not intend to save memory... > > Then I don't understand at all what the benefit of that patch would > be. > >> One of our project need to confine the ddr usage of uboot in the smallest case, >> not to pollute other area. So for us, the small stack is good one... > > So it is still about saving memory... Yes, it also be deemed as saving memory... > >> For now the uboot is relocated to the end of the dram, and malloc area is >> almost a fix value, uboot would live happily in this area. But for env case, >> it allocate a range which could be large, due to the CONFIG_ENV_SIZE >> could be a big one, in the stack range. Because the stack is grown downwards, >> so it takes more memory range than it is allocated in the malloc method. > > malloc arena and stack are adjacent. If you have enough free room in > the malloc arena for the environment buffers, but cannot afford to > have them on your stack, then this means your malloc arena has lots of > unused space. Decrease the size of your malloc arena by the size of > the environment buffer, and you reach the same goal as with your > patch - actually you save more memory, as the code size of u-boot > shrinks. Em, what you said is also right for my case. But I think making the malloc heap area large while limit the stack usage could benefit those function use malloc, while keep user in mind how many memory he is using on that board. For now, uboot just print uboot self-reserved, malloc size, board info, global data size for the uesr. For the stack usage, user is not quite aware of. Just like the env case, if that ENV_SIZE goes too large, user may not notice the memory usage just from the debug info. > > Can you please provide exact numbers? How big is your RAM? What is > the debug output of arch/*/lib/board.c without and with this patch on > your system? How big is your environment resp. the environment > sectors? > My ram is big, 512MB. But for my case, I need uboot use the minimum memory as it can. The debug output is same, for this patch don't change the heap or other part's size. My env is configured as 0x20000, as one nand block size. Best regards, Lei
Dear Lei Wen, In message <AANLkTin1U7WZnVKhOaTzkW4UEDQ1iZtLZBo6-qBMoUzY@mail.gmail.com> you wrote: > > > malloc arena and stack are adjacent. Â If you have enough free room in > > the malloc arena for the environment buffers, but cannot afford to > > have them on your stack, then this means your malloc arena has lots of > > unused space. Decrease the size of your malloc arena by the size of > > the environment buffer, and you reach the same goal as with your > > patch - actually you save more memory, as the code size of u-boot > > shrinks. > > Em, what you said is also right for my case. > But I think making the malloc heap area large while limit the stack usage > could benefit those function use malloc, while keep user in mind how many > memory he is using on that board. For now, uboot just print uboot self-reserved, > malloc size, board info, global data size for the uesr. For the stack usage, > user is not quite aware of. > > Just like the env case, if that ENV_SIZE goes too large, user may not notice > the memory usage just from the debug info. We should pay attention to use the correct terms here. The "user", i. e. the end user running the U-Boot image on some system, will most probably not bother at all wether you use space on the stack or in the malloc arena, or how much. The only thing that concerns him is that the thing "just works". It is our, the developer's, task, to make sure his expectations are met. And from the robustness and reliability, but also from the memory footprint point of view, may favours go clearly with automatic variables on the stack over manual allocation. I mentioned this before: the stack can temporarily use arbitrary amounts of storage, whil still allowing you to use a maximum of free RAM at other times. I don't really care how much memory is needed on the stack for environment buffers when importing or exporting the environment from/to external storage, because this memory is freed again and usable for other purposes when I need it, for example to load some big OS and/or file system images. With malloc(), such memory is permantly reseverd, and thus permantly lost. In my opinion this is a clear disadvantage of the malloc() based approach. > My ram is big, 512MB. But for my case, I need uboot use the minimum memory > as it can. The debug output is same, for this patch don't change the Can you please explain your requirements in a bit more detail, and especially, how your patch is supposed to help? I would expect that the time when the environment buffers need space on the stack is NOT the time when you need so much of memory? Best regards, Wolfgang Denk
Hi Wolfgang, On Mon, Feb 7, 2011 at 12:08 AM, Wolfgang Denk <wd@denx.de> wrote: > Dear Lei Wen, > > In message <AANLkTin1U7WZnVKhOaTzkW4UEDQ1iZtLZBo6-qBMoUzY@mail.gmail.com> you wrote: >> >> > malloc arena and stack are adjacent. If you have enough free room in >> > the malloc arena for the environment buffers, but cannot afford to >> > have them on your stack, then this means your malloc arena has lots of >> > unused space. Decrease the size of your malloc arena by the size of >> > the environment buffer, and you reach the same goal as with your >> > patch - actually you save more memory, as the code size of u-boot >> > shrinks. >> >> Em, what you said is also right for my case. >> But I think making the malloc heap area large while limit the stack usage >> could benefit those function use malloc, while keep user in mind how many >> memory he is using on that board. For now, uboot just print uboot self-reserved, >> malloc size, board info, global data size for the uesr. For the stack usage, >> user is not quite aware of. >> >> Just like the env case, if that ENV_SIZE goes too large, user may not notice >> the memory usage just from the debug info. > > We should pay attention to use the correct terms here. The "user", > i. e. the end user running the U-Boot image on some system, will most > probably not bother at all wether you use space on the stack or in the > malloc arena, or how much. The only thing that concerns him is that > the thing "just works". > Sorry for the wrong using of "user" term here... > It is our, the developer's, task, to make sure his expectations are > met. And from the robustness and reliability, but also from the > memory footprint point of view, may favours go clearly with automatic > variables on the stack over manual allocation. > > I mentioned this before: the stack can temporarily use arbitrary > amounts of storage, whil still allowing you to use a maximum of free > RAM at other times. > > I don't really care how much memory is needed on the stack for > environment buffers when importing or exporting the environment > from/to external storage, because this memory is freed again and > usable for other purposes when I need it, for example to load some big > OS and/or file system images. With malloc(), such memory is permantly > reseverd, and thus permantly lost. I am a little confused here... Why the malloc would make the memory permantly reserved and lost... In this patch, I just temp malloc one buffer, then free it after I use it... > > In my opinion this is a clear disadvantage of the malloc() based > approach. > >> My ram is big, 512MB. But for my case, I need uboot use the minimum memory >> as it can. The debug output is same, for this patch don't change the > > Can you please explain your requirements in a bit more detail, and > especially, how your patch is supposed to help? I would expect that > the time when the environment buffers need space on the stack is NOT > the time when you need so much of memory? > My current implementation is to do a memory dump by using the uboot, while uboot's only task is to compress and write the memory to the sd card. For the compress and write part need a lot of heap, 350k+, if still keep the env buf in the stack, it would increase the total memory touched by uboot... As dump usage need uboot touch the least memory, so this is the purpose I submit this patch... Reuse the heap area at its best, and don't increase stack much in the runtime... Best regards, Lei
On Mon, Feb 7, 2011 at 3:38 PM, Lei Wen <adrian.wenl@gmail.com> wrote: > Hi Wolfgang, > > On Mon, Feb 7, 2011 at 12:08 AM, Wolfgang Denk <wd@denx.de> wrote: >> Dear Lei Wen, >> >> In message <AANLkTin1U7WZnVKhOaTzkW4UEDQ1iZtLZBo6-qBMoUzY@mail.gmail.com> you wrote: >>> > My current implementation is to do a memory dump by using the uboot, > while uboot's only task is to compress and write the memory to the sd card. > For the compress and write part need a lot of heap, 350k+, if still keep the > env buf in the stack, it would increase the total memory touched by uboot... > > As dump usage need uboot touch the least memory, so this is the purpose > I submit this patch... Reuse the heap area at its best, and don't increase stack > much in the runtime... > Ah, so with the env buffer on the stack, the bottom of the stack is being pushed into the memory you want to dump and corrupting it - Makes sense to me now that you would resort to moving the env buffer to the heap. There are probably a number of other areas in u-boot where large buffers are put on the stack while the malloc pool is relatively empty with the end result being that u-boot is using more memory than strictly required. It would be an interesting profiling excercise Regards, Graeme
Dear Lei Wen, In message <AANLkTim4-Nny184WXzcspcAEtAeSe8+M4y08B3aLJtYw@mail.gmail.com> you wrote: > > > I mentioned this before: the stack can temporarily use arbitrary > > amounts of storage, whil still allowing you to use a maximum of free > > RAM at other times. > > > > I don't really care how much memory is needed on the stack for > > environment buffers when importing or exporting the environment > > from/to external storage, because this memory is freed again and > > usable for other purposes when I need it, for example to load some big > > OS and/or file system images. With malloc(), such memory is permantly > > reseverd, and thus permantly lost. > > I am a little confused here... Why the malloc would make the memory permantly > reserved and lost... In this patch, I just temp malloc one buffer, then free it > after I use it... Yes, you free it, so it becomes usable for other malloc() calls (eventually - in real life fragmentation might play a role, too), but you sneed to reserve the full emount of buffer space in the malloc arena, for the whole like time of U-Boot. On the othern hand, when using buffers on the stack, then these buffers are only temporarily used, when the functions are actually being called. In this specific case (keep in mind that we are talking about env_relocate_spec() this happens exactlky once, very early in the system initialization, i. e. long before you are able to run any user commands. > My current implementation is to do a memory dump by using the uboot, > while uboot's only task is to compress and write the memory to the sd card. > For the compress and write part need a lot of heap, 350k+, if still keep the > env buf in the stack, it would increase the total memory touched by uboot... This statement makes no sense at all. The environment buffers are only used once, long before you are able to run any user commands. Wehn you can start doing your stuff, they are long gone, and all of it's memory has been reclaimed. > As dump usage need uboot touch the least memory, so this is the purpose > I submit this patch... Reuse the heap area at its best, and don't increase stack > much in the runtime... But it statically increases the size of the malloc areane, so you are just shifting the memory from location A to B. For the last time I repeat my question: can you please explain how your patch is supposed to reduce the memory footprint of your system, and document your acchievments with actual measurements? I would expect that the time when the environment buffers need space on the stack is NOT the time when you need so much of memory? Best regards, Wolfgang Denk
Dear Graeme Russ, In message <AANLkTi=--aNmH1H0JbUFgcW+qL3qeHvOzb9CcUA=PX-B@mail.gmail.com> you wrote: > > > My current implementation is to do a memory dump by using the uboot, > > while uboot's only task is to compress and write the memory to the sd card. > > For the compress and write part need a lot of heap, 350k+, if still keep the > > env buf in the stack, it would increase the total memory touched by uboot... > > > > As dump usage need uboot touch the least memory, so this is the purpose > > I submit this patch... Reuse the heap area at its best, and don't increase stack > > much in the runtime... > > Ah, so with the env buffer on the stack, the bottom of the stack is being > pushed into the memory you want to dump and corrupting it - Makes sense to > me now that you would resort to moving the env buffer to the heap. No, this makes zero sense, as there is only a single time when the environment buffers are allocated on the stack, and this is very early in the initialization, and then the buffers are released and will never be used again - they are gone without any trace when Lei Wen can run any of his code. On the other hand, it sounds as if he ad a huge malloc arena, which is statically reserved and thus unusable for other pruposed for the whole U-Boot run time. > There are probably a number of other areas in u-boot where large buffers > are put on the stack while the malloc pool is relatively empty with the > end result being that u-boot is using more memory than strictly required. > > It would be an interesting profiling excercise Indeed there is probably potential for optimization - but usually it's the other way round: using buffers on the stack is frequently what results in smaller code and a smaller overall memory footprint; not to mention the issue of memory leaks when using malloc(). I seriously doubt that Lei Wen as able to show any improvement of the memory footprint by his code. In the best case, he already has reserved a huge malloc arena so the environment buffers can be allocated there. In this case, the overall effect of his patch is the increased code size which _reduces_ the free memory on his system. He does NOT save any memory on the stack, because the env_relocate_spec() simply never ever gets called when he is running his code. Best regards, Wolfgang Denk
Hi Wolfgang, On Mon, Feb 7, 2011 at 2:58 PM, Wolfgang Denk <wd@denx.de> wrote: > Dear Lei Wen, > > In message <AANLkTim4-Nny184WXzcspcAEtAeSe8+M4y08B3aLJtYw@mail.gmail.com> you wrote: >> >> > I mentioned this before: the stack can temporarily use arbitrary >> > amounts of storage, whil still allowing you to use a maximum of free >> > RAM at other times. >> > >> > I don't really care how much memory is needed on the stack for >> > environment buffers when importing or exporting the environment >> > from/to external storage, because this memory is freed again and >> > usable for other purposes when I need it, for example to load some big >> > OS and/or file system images. With malloc(), such memory is permantly >> > reseverd, and thus permantly lost. >> >> I am a little confused here... Why the malloc would make the memory permantly >> reserved and lost... In this patch, I just temp malloc one buffer, then free it >> after I use it... > > Yes, you free it, so it becomes usable for other malloc() calls > (eventually - in real life fragmentation might play a role, too), > but you sneed to reserve the full emount of buffer space in the malloc > arena, for the whole like time of U-Boot. > > On the othern hand, when using buffers on the stack, then these > buffers are only temporarily used, when the functions are actually > being called. In this specific case (keep in mind that we are talking > about env_relocate_spec() this happens exactlky once, very early in > the system initialization, i. e. long before you are able to run any > user commands. > >> My current implementation is to do a memory dump by using the uboot, >> while uboot's only task is to compress and write the memory to the sd card. >> For the compress and write part need a lot of heap, 350k+, if still keep the >> env buf in the stack, it would increase the total memory touched by uboot... > > This statement makes no sense at all. The environment buffers are > only used once, long before you are able to run any user commands. > Wehn you can start doing your stuff, they are long gone, and all of > it's memory has been reclaimed. > >> As dump usage need uboot touch the least memory, so this is the purpose >> I submit this patch... Reuse the heap area at its best, and don't increase stack >> much in the runtime... > > But it statically increases the size of the malloc areane, so you are > just shifting the memory from location A to B. > > > For the last time I repeat my question: can you please explain how > your patch is supposed to reduce the memory footprint of your system, > and document your acchievments with actual measurements? I would > expect that the time when the environment buffers need space on the > stack is NOT the time when you need so much of memory? > The measurement is simple, for I don't change other part, the heap size and etc part size, but only move the temp buffer to the heap area instead of stack area. So this reduce the large memory need in the env_relocate_spec() function. So that the stack would not touch those memory since most function don't cost so much stack as env_relocate_spec() does... Also what you said for it is not the same time to different function usage is also true here. But what I want this patch is for I want the uboot touch the minimum memory, not others... The heap area is already enough for the env_relocate_spec() in our platform. Maybe you concern is for those platform don't allocate enough heap size for the env?... Also fragmentation problem I am not figure out clearly... Temporarily use large heap then free would make uboot unable to malloc enough memory in latter allocate?... Best regards, Lei
Dear Lei Wen, In message <AANLkTi=KNpJ02SHCp5FYqukYdMAXUN+LgVpK7O-6Veex@mail.gmail.com> you wrote: > > The measurement is simple, for I don't change other part, the heap > size and etc part > size, but only move the temp buffer to the heap area instead of stack area. > So this reduce the large memory need in the env_relocate_spec() function. > So that the stack would not touch those memory since most function > don't cost so much > stack as env_relocate_spec() does... You don't provide any real measurement results here. Also, you did not report what happens when you - for example - reduce the size of your malloc arena by the size... > The heap area is already enough for the env_relocate_spec() in our > platform. Maybe > you concern is for those platform don't allocate enough heap size for > the env?... > Also fragmentation problem I am not figure out clearly... Temporarily > use large heap then free > would make uboot unable to malloc enough memory in latter allocate?... To sum it up: - Your patch increases the code size, and the code complexity, and increases the risk of memory leaks. - Your patch requires on many systems to increase the size of the malloc arena, with the result that the available memory at runtim becomes lower. - Your patch fails to adjust the malloc arena size on the affected boards (and eventually any board may be affected, so all will have to be checked and verified). - You claim that this patch helps to use less memory on your system, but you fail to prove this with actual numbers. So we have a number of disadvantages for all users, a lot of effort for all users (they all need to re-test their boards), and a claimed but otherwise unproven advantage for a simgle user (you). I hereby reject your patch. If you really think it, you can still maintain it as a local, out-of-tree patch. Thanks. Wolfgang Denk
On Mon, Feb 7, 2011 at 4:59 PM, Wolfgang Denk <wd@denx.de> wrote: > Dear Lei Wen, > > In message <AANLkTi=KNpJ02SHCp5FYqukYdMAXUN+LgVpK7O-6Veex@mail.gmail.com> you wrote: >> >> The measurement is simple, for I don't change other part, the heap >> size and etc part >> size, but only move the temp buffer to the heap area instead of stack area. >> So this reduce the large memory need in the env_relocate_spec() function. >> So that the stack would not touch those memory since most function >> don't cost so much >> stack as env_relocate_spec() does... > > You don't provide any real measurement results here. Also, you did > not report what happens when you - for example - reduce the size of > your malloc arena by the size... > >> The heap area is already enough for the env_relocate_spec() in our >> platform. Maybe >> you concern is for those platform don't allocate enough heap size for >> the env?... >> Also fragmentation problem I am not figure out clearly... Temporarily >> use large heap then free >> would make uboot unable to malloc enough memory in latter allocate?... > > To sum it up: > > - Your patch increases the code size, and the code complexity, and > increases the risk of memory leaks. > > - Your patch requires on many systems to increase the size of the > malloc arena, with the result that the available memory at runtim > becomes lower. > > - Your patch fails to adjust the malloc arena size on the affected > boards (and eventually any board may be affected, so all will have > to be checked and verified). > > - You claim that this patch helps to use less memory on your system, > but you fail to prove this with actual numbers. > > > So we have a number of disadvantages for all users, a lot of effort > for all users (they all need to re-test their boards), and a claimed > but otherwise unproven advantage for a simgle user (you). > > I hereby reject your patch. If you really think it, you can still > maintain it as a local, out-of-tree patch. Understand... It is true for this patch that don't consider for other platform... Thanks, Lei
Hi, I have can compile the uboot 2010.09 with the eldk compiler succesfully and it runs on the target system. If I compile the same uboot with gcc 4.5.2, binutils 2.11 and glibc 2.13 the uboot compiles also successfully but it didn't work on the taget system. The uboot don't have any outputs on the debug console. What can be the problem? Best regards / Mit freundlichen Grüßen Maik Hänig
Dear =?UTF-8?B?TWFpayBIw6RuaWc=?=, In message <4D4FBEAD.1030703@hrz.tu-chemnitz.de> you wrote: > > If I compile the same uboot with gcc 4.5.2, binutils 2.11 and glibc 2.13 > the uboot compiles also successfully but it didn't work on the taget > system. The uboot don't have any outputs on the debug console. There are a number of known (and probably more unknown) compiler bugs in GCC 4.5.x. Workarounds for some of them have been added recently in mainline, so you might try switching to current top of tree instead (especially if you're on an ARM system). Best regards, Wolfgang Denk
Dear Wolfgang Denk, should I better use gcc 4.4? And yes I'm working on ARM. Best regards / Mit freundlichen Grüßen Maik Hänig Am 07.02.2011 11:17, schrieb Wolfgang Denk: > Dear =?UTF-8?B?TWFpayBIw6RuaWc=?=, > > In message<4D4FBEAD.1030703@hrz.tu-chemnitz.de> you wrote: >> >> If I compile the same uboot with gcc 4.5.2, binutils 2.11 and glibc 2.13 >> the uboot compiles also successfully but it didn't work on the taget >> system. The uboot don't have any outputs on the debug console. > > There are a number of known (and probably more unknown) compiler bugs > in GCC 4.5.x. > > Workarounds for some of them have been added recently in mainline, so > you might try switching to current top of tree instead (especially if > you're on an ARM system). > > Best regards, > > Wolfgang Denk >
Dear =?ISO-8859-1?Q?Maik_H=E4nig?=, In message <4D4FC86C.2080003@hrz.tu-chemnitz.de> you wrote: > > should I better use gcc 4.4? No, you should go forward, i. e. rather use current U-Boot code that is supposed to fix these problems (at last the most obvious ones - and if there are any left, you'd help to detec and eventually fix these, too). > And yes I'm working on ARM. Thought so from the symptoms you reported... Best regards, Wolfgang Denk
diff --git a/common/env_dataflash.c b/common/env_dataflash.c index 1d57079..a5a409a 100644 --- a/common/env_dataflash.c +++ b/common/env_dataflash.c @@ -50,11 +50,17 @@ uchar env_get_char_spec(int index) void env_relocate_spec(void) { - char buf[CONFIG_ENV_SIZE]; + char *buf; + buf = malloc(CONFIG_ENV_SIZE); + if (!buf) { + set_default_env("!dataflash env buf malloc failed"); + return; + } read_dataflash(CONFIG_ENV_ADDR, CONFIG_ENV_SIZE, buf); env_import(buf, 1); + free(buf); } #ifdef CONFIG_ENV_OFFSET_REDUND diff --git a/common/env_eeprom.c b/common/env_eeprom.c index 0a179ad..ed7d2d7 100644 --- a/common/env_eeprom.c +++ b/common/env_eeprom.c @@ -113,9 +113,14 @@ uchar env_get_char_spec (int index) void env_relocate_spec (void) { - char buf[CONFIG_ENV_SIZE]; + char *buf; unsigned int off = CONFIG_ENV_OFFSET; + buf = malloc(CONFIG_ENV_SIZE); + if (!buf) { + set_default_env("!eeprom env buf malloc failed"); + return; + } #ifdef CONFIG_ENV_OFFSET_REDUND if (gd->env_valid == 2) off = CONFIG_ENV_OFFSET_REDUND; @@ -126,6 +131,7 @@ void env_relocate_spec (void) CONFIG_ENV_SIZE); env_import(buf, 1); + free(buf); } int saveenv(void) diff --git a/common/env_mgdisk.c b/common/env_mgdisk.c index a69923b..7638ac3 100644 --- a/common/env_mgdisk.c +++ b/common/env_mgdisk.c @@ -43,22 +43,30 @@ uchar env_get_char_spec(int index) void env_relocate_spec(void) { - char buf[CONFIG_ENV_SIZE]; + char *buf; unsigned int err, rc; + buf = malloc(CONFIG_ENV_SIZE); + if (!buf) { + set_default_env("!mgdisk env buf malloc failed"); + return; + } err = mg_disk_init(); if (err) { set_default_env("!mg_disk_init error"); + free(buf); return; } err = mg_disk_read(CONFIG_ENV_ADDR, buf, CONFIG_ENV_SIZE); if (err) { set_default_env("!mg_disk_read error"); + free(buf); return; } env_import(buf, 1); + free(buf); } int saveenv(void) diff --git a/common/env_mmc.c b/common/env_mmc.c index 71dcc4c..2eb2953 100644 --- a/common/env_mmc.c +++ b/common/env_mmc.c @@ -141,7 +141,7 @@ inline int read_env(struct mmc *mmc, unsigned long size, void env_relocate_spec(void) { #if !defined(ENV_IS_EMBEDDED) - char buf[CONFIG_ENV_SIZE]; + char *buf; struct mmc *mmc = find_mmc_device(CONFIG_SYS_MMC_ENV_DEV); @@ -150,12 +150,19 @@ void env_relocate_spec(void) return; } + buf = malloc(CONFIG_ENV_SIZE); + if (!buf) { + set_default_env("!mmc env buf malloc failed"); + return; + } if (read_env(mmc, CONFIG_ENV_SIZE, CONFIG_ENV_OFFSET, buf)) { use_default(); + free(buf); return; } env_import(buf, 1); + free(buf); #endif } diff --git a/common/env_nand.c b/common/env_nand.c index 2682f07..3efc23d 100644 --- a/common/env_nand.c +++ b/common/env_nand.c @@ -416,8 +416,13 @@ void env_relocate_spec (void) { #if !defined(ENV_IS_EMBEDDED) int ret; - char buf[CONFIG_ENV_SIZE]; + char *buf; + buf = malloc(CONFIG_ENV_SIZE); + if (!buf) { + set_default_env("!nand env buf malloc failed"); + return; + } #if defined(CONFIG_ENV_OFFSET_OOB) ret = get_nand_env_oob(&nand_info[0], &nand_env_oob_offset); /* @@ -428,6 +433,7 @@ void env_relocate_spec (void) printf("Found Environment offset in OOB..\n"); } else { set_default_env("!no env offset in OOB"); + free(buf); return; } #endif @@ -435,10 +441,12 @@ void env_relocate_spec (void) ret = readenv(CONFIG_ENV_OFFSET, (u_char *)buf); if (ret) { set_default_env("!readenv() failed"); + free(buf); return; } env_import(buf, 1); + free(buf); #endif /* ! ENV_IS_EMBEDDED */ } #endif /* CONFIG_ENV_OFFSET_REDUND */ diff --git a/common/env_nvram.c b/common/env_nvram.c index 544ce47..7619039 100644 --- a/common/env_nvram.c +++ b/common/env_nvram.c @@ -76,14 +76,21 @@ uchar env_get_char_spec(int index) void env_relocate_spec(void) { - char buf[CONFIG_ENV_SIZE]; + char *buf; + buf = malloc(CONFIG_ENV_SIZE); + if (!buf) { + set_default_env("!nvram env buf malloc failed"); + return; + } +#if defined(CONFIG_ENV_OFFSET_OOB) #if defined(CONFIG_SYS_NVRAM_ACCESS_ROUTINE) nvram_read(buf, CONFIG_ENV_ADDR, CONFIG_ENV_SIZE); #else memcpy(buf, (void*)CONFIG_ENV_ADDR, CONFIG_ENV_SIZE); #endif env_import(buf, 1); + free(buf); } int saveenv(void) diff --git a/common/env_sf.c b/common/env_sf.c index 41cc00a..a26865d 100644 --- a/common/env_sf.c +++ b/common/env_sf.c @@ -345,13 +345,19 @@ int saveenv(void) void env_relocate_spec(void) { - char buf[CONFIG_ENV_SIZE]; + char *buf; int ret; + buf = malloc(CONFIG_ENV_SIZE); + if (!buf) { + set_default_env("!sf env buf malloc failed"); + return; + } env_flash = spi_flash_probe(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS, CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE); if (!env_flash) { set_default_env("!spi_flash_probe() failed"); + free(buf); return; } @@ -369,6 +375,7 @@ void env_relocate_spec(void) out: spi_flash_free(env_flash); env_flash = NULL; + free(buf); } #endif
Original env buf directly locate at stack lead large stack footprint when call those env functions. It is not good when the system memory is critical or only want the uboot run at restrict range, that is not to touch the memory of other place at its best. So now this patch move the env buf to the heap area, which reduce the area uboot need to touch. Signed-off-by: Lei Wen <leiwen@marvell.com> --- common/env_dataflash.c | 8 +++++++- common/env_eeprom.c | 8 +++++++- common/env_mgdisk.c | 10 +++++++++- common/env_mmc.c | 9 ++++++++- common/env_nand.c | 10 +++++++++- common/env_nvram.c | 9 ++++++++- common/env_sf.c | 9 ++++++++- 7 files changed, 56 insertions(+), 7 deletions(-)