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

Enable stm32 core coupled memory #897

Closed
wants to merge 3 commits into from

Conversation

UncleGrumpy
Copy link
Collaborator

@UncleGrumpy UncleGrumpy commented Oct 26, 2023

Enable using the Core Coupled Memory present in all stm32f4 and other mid to high
end stm32 MCUs. CCM is faster than sram as it is tightly integrated with the processor. The 64k CCM is used for .data and .bss segments (aprox 2.5k)
as well as the stack for the VM. This leaves more of slower sram
available for user application stack and heap. Interrupt handler code may also be
placed in the core coupled memory, this will eliminate delays from loading the
interupt handler function from flash. Overall performance could potentially be improved by placing frequently used functions into CCM using __attribute(section("ccm"))__.

These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).

SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later

Import origional libopencm3 linker script.

Signed-off-by: Winford <winford@object.stream>
Enable using the Core Coupled Memory present in all stm32f4 and other mid to high
end stm32 MCUs. The 64k CCM is used for `.data` and `.bss` segments (aprox 2.5k)
as well as the `stack` for the VM. This leaves more of the 256k of slower sram
available for user application stack and heap. Interrupt handler code may also be
placed in the core coupled memory, this will eliminate delays from loading the
interupt handler function from flash.

Signed-off-by: Winford <winford@object.stream>
Interrupt handlers should be placed in CCM to avoid delays loading the
handler function from flash.
Remove `TRACE` lines from within `exti#_isr` handlers and added extra diagnosic
info in `isr_handler`, because the console print used by `TRACE` is a blocking
function and should not be used inside interrupt handlers.

Signed-off-by: Winford <winford@object.stream>
@fadushin
Copy link
Collaborator

fadushin commented Nov 1, 2023

Can you explain what these changes to in the description? I am not really versed on the STM32, so it might be good for us to have something here for posterity.

* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* This library is distributed in the hope that it will be useful,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have an issue distributing this under the Apache 2.0 license? I am not sure what the implications are of this. @bettio any thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would like to avoid (L)GPL only sources, in order to keep licensing simple.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Then maybe we really need to consider basing this port on something other than libopencm3 as it is licensed under GPL-3.0, LGPL-3.0 licenses.

Copy link
Collaborator

@fadushin fadushin left a comment

Choose a reason for hiding this comment

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

Just one GPL/Apache 2.0 question.

Copy link
Collaborator

@bettio bettio left a comment

Choose a reason for hiding this comment

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

Some thoughts about this PR:

  • Do we have any performance measurement?
  • When we take an existing source file we should clearly mark what we changed (this is useful in a number of scenarios). This is not clear to me at first glance
  • Can we get the same result without using an entire linker script?
  • We should discuss how to handle the licensing issue

* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* This library is distributed in the hope that it will be useful,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would like to avoid (L)GPL only sources, in order to keep licensing simple.

* You should have received a copy of the GNU Lesser General Public License
* along with this library. If not, see <http://www.gnu.org/licenses/>.
*
* SPDX-License-Identifier: LGPL-2.1-or-later
Copy link
Collaborator

Choose a reason for hiding this comment

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

This tag is wrong anyway I think.

@UncleGrumpy
Copy link
Collaborator Author

UncleGrumpy commented Nov 5, 2023

I do have some performance metrics using @pguyot 's atomvm_benchmark.

Master:

pingpong_speed_test: 6129000
prime_speed_test: 19732000
prng_test: 385000
pi_test: 27073000

With CCM enabled:

pingpong_speed_test: 5871000
prime_speed_test: 17070000
prng_test: 369000
pi_test: 25659000

Equally important is that more sram is available for stack and heap in user beam applications, by moving the VM stack to the core coupled memory.

I do not think there is any other way to assign the stack to CCM, as this is one of the two primary roles of the linker script, to let the mcu know where the various memory peripherals should be mapped.

@UncleGrumpy
Copy link
Collaborator Author

Since the format of the linker script is dictated by the gcc-arm-none-eabi toolchain, and the libopencm3 linker script follows the exact same format as any other linker script used by libraries with other licenses, there is probably no harm in just removing the original comments and copyright, and replacing them with AtomVM Apache-2 licensed (or more correctly BSD-3) copyright. I only copied the original over to show that it was based on a correct format, and I was not just reinventing the wheel.

In fact after further looking I believe libopencm3 might be in the wrong for using the lgpl-3 license as their script looks suspiciously similar to the BSD-3 license used by STMicroelectronics for the reference linker script, as seen in a version used by Mbed-OS for their stm32 support:

https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_STM/TARGET_STM32F4/TARGET_STM32F407xE/TOOLCHAIN_GCC_ARM/stm32f407xe.ld

@UncleGrumpy
Copy link
Collaborator Author

As far as making the changes clear, this is why I did not flatten the first two commits, but adding a comment before each change would probably be helpful for future maintainers to see exactly which sections of memory have been moved from default sram to ccm.

@UncleGrumpy
Copy link
Collaborator Author

I Have been looking further and there does seem to be a way to enable this in the application rather than the linker. I will see if I can get it working programmatically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants