Skip to content

Leave only some parts of SDK in iram #1710

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.

Sign up for GitHub

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 2 commits into from
Jan 9, 2017
Merged

Leave only some parts of SDK in iram #1710

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7945a22
Leave only some parts of SDK in iram
djphoenix Jan 9, 2017
f3cbeb8
Add note about libraries that excluded from iROM
djphoenix Jan 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion ld/nodemcu.ld
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,24 @@ SECTIONS
*(.init.literal)
*(.init)

*sdk/esp_iot_sdk_*lib/lib*.a:*(.literal .text)
/* SDK libraries that used in bootup process, interruption handling
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could I request a comment here stating that we're intentionally excluding libwps.a even though it claims to have iram'd functions?

* and other ways where flash cache (iROM) is unavailable: */
*libmain.a:*(.literal .text)
*libnet80211.a:*(.literal .text)
*libphy.a:*(.literal .text)
*libpp.a:*(.literal .text)
*libgcc.a:*(.literal .text)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this list is complete. Have a look at the output of this:

nodemcu-firmware/sdk/esp_iot_sdk_v2.0.0/lib$ xtensa-lx106-elf-objdump -h *.a|grep -v '2\*\*0' | grep -v irom|grep -E 'format|.text|archive'

It seems the following libs also have some things in IRAM:

  • libat.a (don't care, we don't use this)
  • libcrypto.a
  • libdriver.a (not sure if we're using any of this by now?)
  • libespnow.a (not relevant yet, but might be worth listing here anyway)
  • libmesh.a (as above)
  • liblwip_536.a (crikey, espconn_buf.o has nearly 1.3k in IRAM! Lots of other bits too...)
  • libpwm.a (we have our own pwm driver last I checked)
  • libwpa.a
  • libwps.a (intentionally excluded)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This list is incomplete for "all libs that have something in iRAM" but complete for "all libs that should have something in iRAM".
All libs that you listed above is called in flash-cache-safe ways. Originally in #1566 I was tested firmware carefully for hashing, WPA/WEP connections, and more ways, and doesn't found any issues with it. But later we choose the way to keep all SDK libs in iRAM.

Rationale of my way is to keep iRAM as clean as possible. Sure, originally Espressif wasn't just include "safe unaligned access" patch in core library, so I think there was something similar in their internal testing process (if it was generally), so after tons of changes, them kept listed libs in iRAM just for historical reasons.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sorry I forgot the testing you did back on that PR. And yeah, I'm not against kicking things out of IRAM if they don't have to be there, I'd just like it to be a very conscious decision, with a note about each lib we've done that to left in there.

Regarding unaligned accesses though, the constraints are the same for mapped flash and IRAM, but it's all a bit tangential to the main discussion :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So okay, I was added a note about it. I think this is enough to go.
Also I propose to include SDK upgrade guide (to check iRAM/iROM contents) in contributing.md

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a good idea!


/* Following SDK libraries have .text sections, but not included in iRAM: */
/* *libat.a:*(.literal .text) - not used anywhere in NodeMCU */
/* *libcrypto.a:*(.literal .text) - tested that safe to keep in iROM */
/* *libdriver.a:*(.literal .text) - not used anywhere in NodeMCU */
/* *libespnow.a:*(.literal .text) - not used anywhere in NodeMCU */
/* *libmesh.a:*(.literal .text) - not used anywhere in NodeMCU */
/* *liblwip_536.a:*(.literal .text) - source-based library used instead */
/* *libpwm.a:*(.literal .text) - our own implementation used instead */
/* *libwpa.a:*(.literal .text) - tested that safe to keep in iROM */
/* *libwps.a:*(.literal .text) - tested that safe to keep in iROM */

*(.iram.text .iram0.text)

Expand Down