Skip to content
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

Add option to use CCMRAM on F303xE. #11756

Merged
merged 1 commit into from Oct 30, 2019

Conversation

@JammuKekkonen
Copy link
Contributor

JammuKekkonen commented Oct 28, 2019

Description (required)

Add option to place data to ccm ram on STM32F303xE. Similar functionality exists already for at least STM32F437xG target.


Pull request type (required)

[X] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results (required)

[] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers (optional)

@anttiylitokola @teetak01


Release Notes (required for feature/major PRs)

Summary of changes
Impact of changes
Migration actions required
@ciarmcom ciarmcom requested review from anttiylitokola, teetak01 and ARMmbed/mbed-os-maintainers Oct 28, 2019
@ciarmcom

This comment has been minimized.

Copy link
Member

ciarmcom commented Oct 28, 2019

@JammuKekkonen, thank you for your changes.
@anttiylitokola @teetak01 @ARMmbed/mbed-os-maintainers please review.

@0xc0170 0xc0170 requested a review from ARMmbed/team-st-mcd Oct 28, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 28, 2019

CI started

@JanneKiiskila

This comment has been minimized.

Copy link
Contributor

JanneKiiskila commented Oct 28, 2019

@ARMmbed/team-st-mcd for your review, too.

@JanneKiiskila

This comment has been minimized.

Copy link
Contributor

JanneKiiskila commented Oct 28, 2019

@0xc0170 @adbridge - preferrably to next Mbed OS patch release, please.

@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Oct 28, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

@@ -56,6 +56,10 @@ LR_IROM1 MBED_APP_START MBED_APP_SIZE { ; load region size_region
.ANY (+RW +ZI)
}

RW_IRAM2 (0x10000000) (0x4000) { ; RW data

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Oct 29, 2019

Contributor

Could you reorder this? It's confusing because you've put the CCM RAM in between RW_IRAM1 and ARM_LIB_STACK, which are adjacent.

It's probably a good idea to name the execution region CCMRAM too, if you're manually directing stuff there. RW_IRAM2 could be construed as general-purpose RAM.

(I'm assuming you're not viewing it as general-purpose RAM, or you would be directing general stuff there. Comment should probably say that rather than "RW data").

This comment has been minimized.

Copy link
@JammuKekkonen

JammuKekkonen Oct 29, 2019

Author Contributor

Current way follows the naming and location in F437xG.

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Oct 29, 2019

Contributor

Feel free to fix that too.

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Oct 29, 2019

Contributor

Ooh, 437's sct is a mess. It's got 5 RAM regions, three of which are adjacent, but they're all separated in the map.

The three together are (RW_m_crash_data, RW_IRAM1, ARM_LIB_STACK) in that order, then RW_IRAM2 (CCMRAM) and RW_IRAM3 (BKPSRAM) are separate.

Its GCC ld and IAR icf seem neat enough.

This comment has been minimized.

Copy link
@JammuKekkonen

JammuKekkonen Oct 29, 2019

Author Contributor

fixed the order and renamed to CCMRAM

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

kjbracey-arm commented Oct 29, 2019

General thought, which you don't need to address here. Any platform which does have CCM RAM probably should use it for at least the boot/interrupt stack, and maybe the main thread stack.

Those are a lot of your scratch space which you'd want to be fast, and you probably get away with avoiding any bus mastering issues that stop you viewing it as general-purpose static/heap RAM - you won't be generally trying to DMA into the stack.

And gives you a RAM saving without any explicit application area attribute additions.

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 29, 2019

@ARMmbed/team-st-mcd ^^ review the suggestion above

@JammuKekkonen JammuKekkonen force-pushed the JammuKekkonen:add_ccmram_section_for_f303re branch from 1b906c2 to 4dc4bff Oct 29, 2019
@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

kjbracey-arm commented Oct 29, 2019

Discussed with @JammuKekkonen - this is fine as-is, but the F303 doesn't have Ethernet, and there's no DMA in the HAL, so we can see no reason not to treat the CCMRAM as general-purpose RAM for Mbed OS. So a later extension might be to pop a .ANY(+RW,+ZI) in there as well as this .ANY(CCMRAM) that gives you manual direction. And maybe the ARM_LIB_STACK.

The F437 does have Ethernet, but some boards don't. Maybe the F437 scatter file could do the same based on a #if !DEVICE_EMAC.

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 29, 2019

CI started

Still waiting for @ARMmbed/team-st-mcd review

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Oct 29, 2019
@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Oct 29, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 2
Build artifacts

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Oct 30, 2019
@0xc0170 0xc0170 merged commit a072866 into ARMmbed:master Oct 30, 2019
26 checks passed
26 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(-72 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8664 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8420B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
Copy link
Contributor

jeromecoutant left a comment

ST CI OK with NUCLEO_F303ZE and NUCLEO_F303RE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.