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

Detailed examples memory.x files for all current STM32H7xx device families #299

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

TomDeRybel
Copy link

@TomDeRybel TomDeRybel commented Dec 16, 2021

This PR results from the issue report and discussion #291
Please inspect it all carefully with an expert eye, as I only somewhat know what I'm doing.

  • I am entirely unsure about the itcm and flash2 sections for the double-core parts. This was taken verbatim from the @adamgreig example. Also, the alignment is 4 for itcm in the example, while I think it is usually 8? Except, it seems to copy data from flash, which is 4. I don't quite get this and it would help to add some docs regarding what is actually happening there.
  • I've added all the parts to Cargo.toml as well, making a new distinction "q parts" to deal with SMPS variations. This may not be the right thing at all.

In any case, I hope these files are useful and I definitely learned a lot more about these parts by writing them.

@TomDeRybel
Copy link
Author

I found an issue with the use of REGION_ALIAS to set FLASH and RAM.
The "flip-link" crate that I use for stack overflow protection does not like it, saying it can't find MEMORY.RAM during the linking stage.

If I disable flip-link in my cargo/config file rustflags, it all builds and works just fine.

Thus: do I forgo this bit of elegance, where MEMORY defines the chip memories and REGION_ALIAS is used to assign things? Or do I/we take it up with the Knurling team to perhaps allow for this in flip-link?

@hargoniX
Copy link
Member

Maybe we could also have a table in the README with a mapping from chip family to memory.x? Usually people that start their own project go to the HAL and copy the memory.x but now with us having multiple newcomers to the h7 family might be a bit confused at first.

@TomDeRybel
Copy link
Author

I've added such a table to the README with some explanation on how to use the files. I've also updated the supported devices table as well with the various new part numbers (hope that is allright).

@hargoniX
Copy link
Member

hargoniX commented Dec 20, 2021

Alright cool...it would be awesome if we could also get people that have these different devices to actually test the memory.x's before pushing this out so we don't cause any bugs for users. Especially since you remarked you are unsure about some things.

CC @richardeoin @ryan-summers @jordens and @ whoever else might have H7 chips that are more out of the ordinary.

Regarding the flip-link stuff mentioned above, it's probably best if we at least ask over there whether what we want is possible since there definitely are quite a lot of flip-link users (such as yourself :p)

@TomDeRybel
Copy link
Author

Thank you all for looking into these!

  • I'm by now rather convinced that my changes to the Cargo.toml are in error. If so, I'll undo them.
  • I've got some tests running using the new memory.x files, running the latest RTIC rc on:
    • NUCLEO-H723ZG board exercising USB (interrupts), GPIO, EXTI, timer 1.
      Note: this requires some fixes to stm32-rs, PR already submitted (mainly missing interrupts).
    • NUCLEO-H743ZI2 board exercising USB (polled), GPIO.
  • However: none of these tests use anything beyond the RAM and FLASH sections, so they are of limited value.

I've taken a look at the flip-link source, as well as the current open issues. The current parser is limited in what it can handle (it explicitly, and exclusively, looks for a RAM entry in the MEMORY section). However, judging by the open issues, there are hints that a new parser is being thought of. Reporting this issue would likely be useful, so it is taken into consideration if/when the parser re-design happens.

@jordens
Copy link
Member

jordens commented Dec 23, 2021

I am not sure whether these memory.x are generic and complete. At least for our use case they would only be a template. To make use of the ITCM for example, I need the changes below. Also the INSERT BEFORE .data absolutely needs to be there.

--- ../stm32h7xx-hal/memory_743_750_753.x       2021-12-21 13:28:03.338470293 +0100
+++ memory.x    2021-12-23 14:25:06.827958285 +0100
@@ -76,10 +76,13 @@
     . = ALIGN(4);
     } > FLASH2

-  .itcm (NOLOAD) : ALIGN(8) {
+  .itcm : ALIGN(8) {
+    __sitcm = .;
     *(.itcm .itcm.*);
     . = ALIGN(8);
-    } > ITCM
+    __eitcm = .;
+    } > ITCM AT>FLASH
+  __siitcm = LOADADDR(.itcm);

   .axisram (NOLOAD) : ALIGN(8) {
     *(.axisram .axisram.*);
@@ -111,4 +114,4 @@
     . = ALIGN(4);
     } > BSRAM

-};
+} INSERT BEFORE .data;

@adamgreig
Copy link
Member

Presumably you have some extra initialisation routines that use __sitcm and __siitcm to program it into memory, though, since they're not part of cortex-m-rt? In which case why do you need INSERT BEFORE .data?

@jordens
Copy link
Member

jordens commented Dec 23, 2021

Yes. ITCM needs a loader. For that the symbols need to be there. One could argue that since the loader is elsewhere all the details can also be elsewhere. But that makes the definition of the section (not the memory) here somewhat useless as well.
Because of the address pointer handling by the linker, ITCM loadaddr will be at the beginning of the flash without insert before. That collides with for example the vector table. I thought I had mentioned that in rust-embedded/cortex-m-rt#323 but i can't find it now.

@adamgreig
Copy link
Member

I agree, I don't think there's much benefit to including an ITCM section in these example memory.x without also including a loader and other such details. I think it's fair to say they're often useful as-is and otherwise useful as templates, without an expectation they'd cover every use-case?

Right, I see why you need INSERT BEFORE .data for the ITCM section when it's >ITCM AT>FLASH, that makes sense. Should only need it for the sections{} with ITCM, though, right?

@jordens
Copy link
Member

jordens commented Dec 23, 2021

Agreed. probably wise to point out in the comments that these memory.x are not complete solutions but templates.
These limitations can apply to all use cases that could involve loading/zeroing mechanisms, e.g. also data, not just the ITCM.

Copy link
Member

@richardeoin richardeoin left a comment

Choose a reason for hiding this comment

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

Even though they won't cover every use case, these are really valuable templates to have for new users. Since they won't be checked by CI, is it possible these could live outside the repo and we link to them instead? In the wiki perhaps, or even something like a blog post? Open to suggestions here. At the same time the memory.x we use for builds can be slimmed down to the minimum needed to run the examples.

I think the feature flags for the "q parts" belong in a separate PR. There are a few places in the code where (for example) the stm32h7b0 flag is used to mask unavailable pins, so those sites would need to be updated also.

block. The work-around is to comment-out the "REGION_ALIAS(RAM, DTCM)" line and
manually substitute the RAM label in the respective MEMORY entry. Eg: replace
DTCM by RAM.

#### Flash memory size
Copy link
Member

Choose a reason for hiding this comment

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

You can delete this little "Flash memory size" section, I think it's sufficiently covered by the new memory__.x files now

{
/* This file is intended for parts in the STM32H742/742v families. (RM0433) */
/* - FLASH and RAM are mandatory memory sections. */
/* - The sum of all non-FLASH sections must add to 692K total device RAM. */
Copy link
Member

Choose a reason for hiding this comment

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

The only family that supports re-assigning memory around the map is STM32H735, so I don't think there's any value in stating this sum for the other families

. = ALIGN(4);
} > SRAM1

.sram2 (NOLOAD) : ALIGN(4) {
Copy link
Member

Choose a reason for hiding this comment

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

I think it's worth another note here that sram2 is used as the location for the CPU2 stack

@@ -0,0 +1,114 @@
MEMORY
{
/* This file is intended for parts in the STM32H743/743v/753/753v families (RM0433), */
Copy link
Member

Choose a reason for hiding this comment

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

You can remove the references to the Rev V parts here, the memory layout was the same anyhow


.itcm : ALIGN(4) {
*(.itcm .itcm.*);
. = ALIGN(4);
Copy link
Member

Choose a reason for hiding this comment

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

Is this correct? Line 77 says "ITCM, DTCM and AXISRAM connect to a 64-bit wide bus -> align to 8 bytes."

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.

6 participants