-
Notifications
You must be signed in to change notification settings - Fork 3k
RTL8195AM - Fix IAR ielftool zero-padding issue #5260
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
How is this fixing the issue? the diff is a bit bigger, and commit does not provide explanation for these changes. What is the time now (how much in percetage it build now/before) ? What about implications having this fix in, no fine grain control? |
I have updated description of this PR and will update commit messages. The first patch is to cleanup ICF related files, no logic change. @Archcady Please help review, and if you'd like to take over this PR, please fork from mine and amend with your updates. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding space, how much are we losing, if any. what other impacts does this have?
define symbol __ICFEDIT_size_heap__ = 0x19000; | ||
/**** End of ICF editor section. ###ICF###*/ | ||
|
||
define symbol __SRAM_start__ = 0x10007000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wondering why also name change:
- it was BD_RAM , now SRAM
- it was SDRAM, now DRAM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not losing any space. Removed regions are for ROM and TCM. ROM region is there for linking only, which can be replaced with a symbol file. TCM region can be directly accessed in user program.
Well, I found it very confusing when I was working on RTL8195AM. The BD_RAM is actually a fast SRAM. In GCC's case, it's named SRAM1 and SRAM2. I will check if I can unify naming for all compilers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I am on the same page, was looking at the linker scripts earlier and was confused. If they get unified , would be +1
greentea tests OK. |
@Archcady please review @studavekar for visibility, we will test this if the time improves |
IAR greentea test finished in 1266.63 sec using command mbed test -t IAR -v -c |
7774623
to
4ac86fe
Compare
Cleanup ICF file. No logic change. Signed-off-by: Tony Wu <[email protected]>
Remove redundant memory regions, and merge multiple RAM regions into one to solve ielftool zero padding issue. The side effect is less control over object files placement. It's all up to linker's best effort. Signed-off-by: Tony Wu <[email protected]>
This file is unused and redundant. Signed-off-by: Tony Wu <[email protected]>
Build : SUCCESSBuild number : 123 Triggering tests/test mbed-os |
@studavekar Bump for review |
/morph build |
1 similar comment
/morph build |
Build : SUCCESSBuild number : 282 Triggering tests/morph test |
Test : SUCCESSBuild number : 128 |
Build : SUCCESSBuild number : 291 Triggering tests/morph test |
Build : SUCCESSBuild number : 294 Triggering tests/morph test |
Test : SUCCESSBuild number : 134 |
Build times have are lower with as seen from logs 👍 |
Description
Due to multiple memory regions defined in RTL8195AM's icf file, ielftool does zero-padding between memory regions. This behavior prolongs conversion time and generates huge binary file.
The reason for having multiple memory regions is to do fine-grain control of object files, but this is not necessary for most cases.
This patch set tries to fix zero-padding issue by replacing multiple memory regions with one region with discontinuous segments. The side effect is the placement of object files is up to linker's best-effort.
The conversion time is greatly reduced, measured around 0.061s per conversion.
Related issue: #5241
Status
READY
Migrations
NO
Related PRs
NO
Todos
NO
Deploy notes
NO
Steps to test or reproduce
mbed compile -m REALTEK_RTL8195AM -t IAR