Skip to content

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

Merged
merged 3 commits into from
Oct 23, 2017

Conversation

tung7970
Copy link
Contributor

@tung7970 tung7970 commented Oct 5, 2017

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

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 5, 2017

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?

@tung7970
Copy link
Contributor Author

tung7970 commented Oct 5, 2017

I have updated description of this PR and will update commit messages.

The first patch is to cleanup ICF related files, no logic change.
The second patch is to adjust memory regions to solve zero padding issues by removing redundant regions, and by merging existing ram regions.

@Archcady Please help review, and if you'd like to take over this PR, please fork from mine and amend with your updates.

Copy link
Contributor

@0xc0170 0xc0170 left a 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;
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

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

@tung7970
Copy link
Contributor Author

tung7970 commented Oct 6, 2017

greentea tests OK.

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 6, 2017

@Archcady please review

@studavekar for visibility, we will test this if the time improves

@Archcady
Copy link
Contributor

Archcady commented Oct 9, 2017

IAR greentea test finished in 1266.63 sec using command mbed test -t IAR -v -c
mbedgt: completed in 1266.63 sec
To compare the time, I use command mbed test -t ARM -v -c and finish testes in 1503.98 sec.

@tung7970 tung7970 force-pushed the fix-iar branch 2 times, most recently from 7774623 to 4ac86fe Compare October 9, 2017 12:34
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]>
@mbed-ci
Copy link

mbed-ci commented Oct 11, 2017

Build : SUCCESS

Build number : 123
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5260/

Triggering tests

/test mbed-os

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 13, 2017

@studavekar Bump for review

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 19, 2017

/morph build

1 similar comment
@studavekar
Copy link
Contributor

/morph build

@mbed-ci
Copy link

mbed-ci commented Oct 19, 2017

Build : SUCCESS

Build number : 282
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5260/

Triggering tests

/morph test
/morph uvisor-test

@mbed-ci
Copy link

mbed-ci commented Oct 19, 2017

@mbed-ci
Copy link

mbed-ci commented Oct 20, 2017

Build : SUCCESS

Build number : 291
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5260/

Triggering tests

/morph test
/morph uvisor-test

@mbed-ci
Copy link

mbed-ci commented Oct 20, 2017

Build : SUCCESS

Build number : 294
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5260/

Triggering tests

/morph test
/morph uvisor-test

@mbed-ci
Copy link

mbed-ci commented Oct 20, 2017

@studavekar
Copy link
Contributor

Build times have are lower with as seen from logs 👍

@theotherjimmy theotherjimmy merged commit d2b7620 into ARMmbed:master Oct 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants