riotboot/Makefile.include: increase RIOTBOOT_HRD_LEN for ARMv7*-M#11641
riotboot/Makefile.include: increase RIOTBOOT_HRD_LEN for ARMv7*-M#11641fjmolinas merged 2 commits intoRIOT-OS:masterfrom
Conversation
fjmolinas
added
Platform: ARM
|
The testing procedure in here cannot work currently. On my side, I currently have no output for my |
|
Would it be possible to assert this requirement in the code too ? |
|
I'm seeing other options for inforcing this... Alignment could be forced in the linker script itself |
fjmolinas
added
the
Discussion: RFC
|
@fjmolinas you should rebase this PR on the current master, it does not include the |
|
@cladmi done, it should work now |
|
@fjmolinas indeed it is better now :D It now runs without crash with this diff: diff --git a/boards/pba-d-01-kw2x/Makefile.features b/boards/pba-d-01-kw2x/Makefile.features
index ee69accdd..a87c01d0d 100644
--- a/boards/pba-d-01-kw2x/Makefile.features
+++ b/boards/pba-d-01-kw2x/Makefile.features
@@ -8,6 +8,8 @@ FEATURES_PROVIDED += periph_spi
FEATURES_PROVIDED += periph_timer
FEATURES_PROVIDED += periph_uart
+FEATURES_PROVIDED += riotboot
+
# The board MPU family (used for grouping by the CI system)
FEATURES_MCU_GROUP = cortex_m4_3
and I have the crash in master as shown (with the diff applied too). |
|
One good thing for the variable, we put the start address of a slot in the slot header and so it is not set at from compile time configuration in the type/bootloader code. (was a good choice @kYc0o :)) Which means that a bootloader compiled now would work in the future even if changing the So it is not required to always put the highest possible value as it is really board+firmware specific. Having it by default to this value could still be a sane default. Local test to verify this: |
This isn't actually needed, if riotboot is not provided you get an "expect errors message" put the application will still compile (and in this case work).
Indeed, it could also be defined for the specific boards/cpu (as done in #11642 and #11643). The good thing about this is that since by default it won't work, it will force the developer to look more carefully into The thing is that if we are setting a default value, I think the default value should be sow that it works for most cases
This could be done by the bootloader, prevent a jump if the it is not aligned. |
Indeed, I forgot that it is put in For reference, without the And the value of |
Agreed, so a sane "too big" default is not bad and could still be overwritten in the board.
The bootloader does not know the required alignment, as it depends on the number of exceptions. |
There is a |
|
@fjmolinas Ah, so it is not application specific? I mean could a specifically crafted application/configuration increase or reduce the number of interrupts ? This information would also really be useful in #11597 |
Yes and No, you could but that would mean meddling with the Lines 141 to 147 in 7a264b2 When the cpu was ported a specific amount of interrupts where defined. If another new peripheral is added that could mean adding a new interrupt, that maybe wasn't added before. But as I understand by looking at the code, even if not used the vector is defined and therefor included in the vector table. So for me it is not application specific, but CPU specific. |
|
I can confirm is CPU specific and actually it MUST be 256B aligned according to the Cortex-M user manual. For this architecture I think there must be something similar, but I'd say to align it at 256B systematically so we don't risk too much. For now, most of supported CPUs have less than 256 IRQ (mostly Cortex-M0+ and M3), then the M4 and M7 I think they have more, so they need at least a 512B for that. |
cladmi
left a comment
cladmi
left a comment
There was a problem hiding this comment.
Some wording remarks, plus would like to have the link to the datasheet.
It would allow updating it later for new architecture and have all the context for justifying the values.
As it is CPU specific, its good to put the maximum possible required value by default.
sys/riotboot/Makefile.include
Outdated
| # the Cortex-M0+/3/4/7 architecture | ||
| RIOTBOOT_HDR_LEN ?= 0x100 | ||
| # Indicate the reserved space for a header. | ||
| # Notice that it must be at least 256B aligned. This is restricted by |
There was a problem hiding this comment.
Could you update to remove the previous Notice that it must be at least 256 aligned and include this part in the following part. Currently it has two different informations mixed.
sys/riotboot/Makefile.include
Outdated
| # Notice that it must be at least 256B aligned. This is restricted by | ||
| # the Cortex-M0+/3/4/7 architecture. "The Vector table must be naturally | ||
| # aligned to a power of two whose alignment value is greater than or equal | ||
| # to number of Exceptions supported x 4" |
There was a problem hiding this comment.
It would be good if you have a link for this information, it would be useful for porting future boards and justifying the values.
sys/riotboot/Makefile.include
Outdated
| # the Cortex-M0+/3/4/7 architecture. "The Vector table must be naturally | ||
| # aligned to a power of two whose alignment value is greater than or equal | ||
| # to number of Exceptions supported x 4" | ||
| # For ARMv7-M/ARMv7E-M (M4, M3, M7). Maximum of 256 exceptions (256*4 bytes == 0x400). |
There was a problem hiding this comment.
You can put it as markdown style items with # * lalala
|
@cladmi Addressed your comments, let me know what you think. |
|
The text is good for me, I could find the information in the datasheet, so any further modifications have everything needed. I however just realized this is set in Question for further steps too. Is this supposed to remove the need for the With this pull request alone, I get When rebased on master I get the kinetis configured value |
Good question, I was actually wandering the same thing and was going to point it out, I decided against it since for now riotboot is only really supported for cortex-m. I also though that having it here documents for some stuff a developer has to watch out when implementing riotboot. But re-thinking about it now this should be don in #11597. If we move it I think a small comment should still be present mentioning generic alignment and maybe poiting out to
I think we should keep it, every byte can count and if possible this should be optimized. Maybe we can add a comment that states this. |
I like this
Somehow saying in the definition file "this is the theoretical maximum, you may have a smaller one for your specific cpu/board" |
- Since the Vector table must be naturally aligned to the next power of two of the amount of supported ISR, and the table will be placed after riotboot_hdr, we must ensure RIOTBOOT_HRD_LEN has the same alignment.
fjmolinas
force-pushed
the
pr_riotboot_m4
branch
from
c7d5dde to
75efed1
Compare
|
@cladmi I split the commit into two: one adding the cortex-m configuration and another adapting the comment on the generic I get the following outputs for different cpu's: Which matches the expected behavior. What do you think? |
cladmi
added
the
CI: ready for build
cladmi
left a comment
cladmi
left a comment
There was a problem hiding this comment.
ACK good for me. The detailed explanation gives the required context to find the reason and future changes.
I get the same output for the tests as the last comment.
|
Please merge when CI is green, I will be afk in the next days. |
|
Merging since @cladmi would be ACK, GO! |
3 participants
Contribution description
Currently by default
RIOTBOOT_HDR_LENis defined at0x100this is fine for M0/M0+/M1 who only have 48 interrupts soround_po2(4x48) = 0x100. But for ARMv7*-M the maximum number of interrups is 256 and in this caseround_po2(4x256) = 0x400.In must cases supported ISR in our boards is around 100, not more than 127. But the default case should work for all cpu/boards and when implementing
riotbootfor specific board this should be trimmed accordingly (for most ARMv7*-M0x200would be enough)If the alignment isn't fixed form the
0x100a hardfault occurs when trying to make riotboot work on otherwise almost enabled boards.Testing procedure
With this PR running the following command succeeds with no hard fault:
FEATURES_REQUIRED+=riotboot BOARD=pba-d-01-kw2x make -C tests/xtimer_usleep/ clean riotboot/flash termIssues/PRs references