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

cpu/stm32: fix riotboot settings for L4 and WB #19618

Merged
merged 1 commit into from
May 23, 2023

Conversation

gschorcht
Copy link
Contributor

@gschorcht gschorcht commented May 19, 2023

Contribution description

This PR fixes the riotboot configuration for L4 and WB.

The family is not called stm32l4 or stm32wb but l4 and wb. That is, the riotboot configuration didn't work at all. Furthermore, a minimum RIOTBOOT_LEN of 0x2000 is required for L4.

Found when investigating the compilation errors for bootloaders/riotboot_serial in PR #19576.

Testing procedure

  1. Green CI.
  2. Use the following commands:
    BOARD=nucleo-l496zg make -C tests/riotboot info-debug-variable-RIOTBOOT_HDR_LEN
    BOARD=p-nucleo-wb55 make -C tests/riotboot info-debug-variable-RIOTBOOT_HDR_LEN
    
    In master these commands give
    0x400
    
    With this PR these commands give
    0x200
    
    as expected.
  3. Use the following commands:
    BOARD=nucleo-l496zg make -C tests/riotboot info-debug-variable-RIOTBOOT_LEN
    BOARD=p-nucleo-wb55 make -C tests/riotboot info-debug-variable-RIOTBOOT_LEN
    
    In master these commands give
    0x1000
    
    With this PR these commands give
    0x2000
    
    as expected.

Issues/PRs references

@github-actions github-actions bot added Area: build system Area: Build system Area: cpu Area: CPU/MCU ports Platform: ARM Platform: This PR/issue effects ARM-based platforms labels May 19, 2023
@gschorcht gschorcht added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 19, 2023
@riot-ci
Copy link

riot-ci commented May 19, 2023

Murdock results

✔️ PASSED

e6c1aec cpu/stm32: fix riotboot settings for L4 and WB

Success Failures Total Runtime
6945 0 6946 10m:35s

Artifacts

@aabadie
Copy link
Contributor

aabadie commented May 22, 2023

I tested tests/riotboot with this PR on nucleo-l476rg and p-l496g-cell02: it works for both. But it also works on master so maybe I'm not testing the right thing ?

@gschorcht
Copy link
Contributor Author

I tested tests/riotboot with this PR on nucleo-l476rg and p-l496g-cell02: it works for both. But it also works on master so maybe I'm not testing the right thing ?

It works since it used the default value of 0x400 for RIOTBOOT_HDR_LEN set in cpu/cortex_common/Makefile.include. You can use

BOARD=nucleo-l496zg make -C tests/riotboot info-debug-variable-RIOTBOOT_HDR_LEN

to verify.

@gschorcht
Copy link
Contributor Author

I have changed as suggested and squashed it directly.

To check you should use the following commands:

BOARD=nucleo-l496zg make -C tests/riotboot info-debug-variable-RIOTBOOT_HDR_LEN
BOARD=p-nucleo-wb55 make -C tests/riotboot info-debug-variable-RIOTBOOT_HDR_LEN
BOARD=nucleo-l496zg make -C tests/riotboot info-debug-variable-RIOTBOOT_LEN
BOARD=p-nucleo-wb55 make -C tests/riotboot info-debug-variable-RIOTBOOT_LEN

You should observe the wrong values are given in master.

Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

On L4 the number of IRQs varies from 83 (L412) to 95 (L4R5 and the like). So 0x200 is enough in all cases.

I tested the values are correct following #19618 (comment)

ACK

@aabadie
Copy link
Contributor

aabadie commented May 23, 2023

bors merge

@aabadie
Copy link
Contributor

aabadie commented May 23, 2023

bors cancel

@bors
Copy link
Contributor

bors bot commented May 23, 2023

Canceled.

@aabadie
Copy link
Contributor

aabadie commented May 23, 2023

bors merge

bors bot added a commit that referenced this pull request May 23, 2023
19602: dist/tools/compile_commands: add another workaround r=chrysn a=maribu

### Contribution description

Filter out GCC only `--param=min-pagesize=0` in `clangd` mode. This fixes compilation of rust applications, that now fails with:

    thread 'main' panicked at 'Unable to generate bindings: ClangDiagnostic("error: argument unused during compilation: '--param=min-pagesize=0' [-Wunused-command-line-argument]\n")', /home/maribu/.cargo/git/checkouts/rust-riot-sys-d12733b89271907c/b4bd4bd/build.rs:224:10


19618: cpu/stm32: fix riotboot settings for L4 and WB r=aabadie a=gschorcht

### Contribution description

This PR fixes the `riotboot` configuration for L4 and WB.

The family is not called `stm32l4` or `stm32wb` but `l4` and `wb`. That is, the `riotboot` configuration didn't work at all. Furthermore, a minimum `RIOTBOOT_LEN` of `0x2000` is required for L4.

Found when investigating the compilation errors for `bootloaders/riotboot_serial` in PR #19576.

### Testing procedure

1. Green CI.
2. Use the following commands:
    ```
    BOARD=nucleo-l496zg make -C tests/riotboot info-debug-variable-RIOTBOOT_HDR_LEN
    BOARD=p-nucleo-wb55 make -C tests/riotboot info-debug-variable-RIOTBOOT_HDR_LEN
    ```
    In master these commands give
    ```
    0x400
    ```
    With this PR these commands give
    ```
    0x200
    ```
    as expected.
3. Use the following commands:
    ```
    BOARD=nucleo-l496zg make -C tests/riotboot info-debug-variable-RIOTBOOT_LEN
    BOARD=p-nucleo-wb55 make -C tests/riotboot info-debug-variable-RIOTBOOT_LEN
    ```
    In master these commands give
    ```
    0x1000
    ```
    With this PR these commands give
    ```
    0x2000
    ```
    as expected.

### Issues/PRs references


19643: examples/suit_update: some test fixes r=aabadie a=kaspar030



Co-authored-by: Marian Buschsieweke <marian.buschsieweke@ovgu.de>
Co-authored-by: Gunar Schorcht <gunar@schorcht.net>
Co-authored-by: Kaspar Schleiser <kaspar@schleiser.de>
@bors
Copy link
Contributor

bors bot commented May 23, 2023

Build failed (retrying...):

bors bot added a commit that referenced this pull request May 23, 2023
19618: cpu/stm32: fix riotboot settings for L4 and WB r=aabadie a=gschorcht

### Contribution description

This PR fixes the `riotboot` configuration for L4 and WB.

The family is not called `stm32l4` or `stm32wb` but `l4` and `wb`. That is, the `riotboot` configuration didn't work at all. Furthermore, a minimum `RIOTBOOT_LEN` of `0x2000` is required for L4.

Found when investigating the compilation errors for `bootloaders/riotboot_serial` in PR #19576.

### Testing procedure

1. Green CI.
2. Use the following commands:
    ```
    BOARD=nucleo-l496zg make -C tests/riotboot info-debug-variable-RIOTBOOT_HDR_LEN
    BOARD=p-nucleo-wb55 make -C tests/riotboot info-debug-variable-RIOTBOOT_HDR_LEN
    ```
    In master these commands give
    ```
    0x400
    ```
    With this PR these commands give
    ```
    0x200
    ```
    as expected.
3. Use the following commands:
    ```
    BOARD=nucleo-l496zg make -C tests/riotboot info-debug-variable-RIOTBOOT_LEN
    BOARD=p-nucleo-wb55 make -C tests/riotboot info-debug-variable-RIOTBOOT_LEN
    ```
    In master these commands give
    ```
    0x1000
    ```
    With this PR these commands give
    ```
    0x2000
    ```
    as expected.

### Issues/PRs references


19643: examples/suit_update: some test fixes r=aabadie a=kaspar030



Co-authored-by: Gunar Schorcht <gunar@schorcht.net>
Co-authored-by: Kaspar Schleiser <kaspar@schleiser.de>
@bors
Copy link
Contributor

bors bot commented May 23, 2023

Build failed (retrying...):

@aabadie
Copy link
Contributor

aabadie commented May 23, 2023

Hmm, Murdock spotted a problem with nucleo-l4r5zi

@gschorcht
Copy link
Contributor Author

gschorcht commented May 23, 2023

Hmm, Murdock spotted a problem with nucleo-l4r5zi

Due to the bug, RIOTBOOT_LEN wasn't set to 0x2000 here

RIOTBOOT_LEN ?= 0x2000
but in cpu/cortexm_common:
ifneq (,$(filter usbus_dfu tinyusb_dfu,$(USEMODULE)))
RIOTBOOT_LEN ?= 0x4000
else
RIOTBOOT_LEN ?= 0x1000
endif
Therefore, RIOTBOOT_LEN was set to 0x4000 in case usbus_dfu or tinyusb_dfu are used. It seems that we also need to deal with this case for L4 and WB.

bors bot added a commit that referenced this pull request May 23, 2023
19618: cpu/stm32: fix riotboot settings for L4 and WB r=aabadie a=gschorcht

### Contribution description

This PR fixes the `riotboot` configuration for L4 and WB.

The family is not called `stm32l4` or `stm32wb` but `l4` and `wb`. That is, the `riotboot` configuration didn't work at all. Furthermore, a minimum `RIOTBOOT_LEN` of `0x2000` is required for L4.

Found when investigating the compilation errors for `bootloaders/riotboot_serial` in PR #19576.

### Testing procedure

1. Green CI.
2. Use the following commands:
    ```
    BOARD=nucleo-l496zg make -C tests/riotboot info-debug-variable-RIOTBOOT_HDR_LEN
    BOARD=p-nucleo-wb55 make -C tests/riotboot info-debug-variable-RIOTBOOT_HDR_LEN
    ```
    In master these commands give
    ```
    0x400
    ```
    With this PR these commands give
    ```
    0x200
    ```
    as expected.
3. Use the following commands:
    ```
    BOARD=nucleo-l496zg make -C tests/riotboot info-debug-variable-RIOTBOOT_LEN
    BOARD=p-nucleo-wb55 make -C tests/riotboot info-debug-variable-RIOTBOOT_LEN
    ```
    In master these commands give
    ```
    0x1000
    ```
    With this PR these commands give
    ```
    0x2000
    ```
    as expected.

### Issues/PRs references


19636: sys: model ecc, evtimer, pipe and shell_lock in kconfig r=aabadie a=aabadie



19639: tests/net/gnrc_mac_timeout: add automated test r=aabadie a=aabadie



Co-authored-by: Gunar Schorcht <gunar@schorcht.net>
Co-authored-by: Alexandre Abadie <alexandre.abadie@inria.fr>
@aabadie
Copy link
Contributor

aabadie commented May 23, 2023

bors cancel

@bors
Copy link
Contributor

bors bot commented May 23, 2023

Canceled.

@gschorcht
Copy link
Contributor Author

gschorcht commented May 23, 2023

Therefore, RIOTBOOT_LEN was set to 0x4000 in case usbus_dfu or tinyusb_dfu are used. It seems that we also need to deal with this case for L4 and WB.

However, I wonder why we should increase RIOTBOOT_LEN to 8 KByte for all WB and L4 boards, when it worked with the bug even with only 4 kByte, especially for MCUs with only 64 kByte or 128 kByte of flash. The problem appeared only with stm32l496g-disco, because it must use additionally the module periph_lpuart and 4 KByte as RIOTBOOT_LEN are just 200 byte too little.

On the other hand, I'm not sure whether it would make sense to define RIOTBOOT_LEN on board level.

@gschorcht
Copy link
Contributor Author

gschorcht commented May 23, 2023

If I'm right, for WB family a RIOTBOOT_LEN of 0x1000 is definitely wrong since WB MCUs have a flash page size of 4 kByte so that the minimum RIOTBOOT_LEN has to 16 kByte to get a multiple of 4 kBytes when dividing total_flash_size - RIOTBOOT_LEN by 2.

The family is not called `stm32l4` or `stm32wb` but `l4` and `wb`. That is, the `riotboot` configuration didn't work. A minimum `RIOTBOOT_LEN` of `0x2000` is required for WB.
@gschorcht gschorcht force-pushed the cpu/stm32/fix_riotboot_settings branch from 2b96630 to e6c1aec Compare May 23, 2023 15:47
@gschorcht
Copy link
Contributor Author

I have changed it back for L4 so that RIOTBOOT_LEN is set to the default values as defined in cpu/cortexm_common/Makefile.include. I fixed RIOTBOOT_LEN for WB. The problem with stm32l496g-disco is solved on board level.

@benpicco
Copy link
Contributor

bors merge

@bors
Copy link
Contributor

bors bot commented May 23, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit a272abb into RIOT-OS:master May 23, 2023
@gschorcht
Copy link
Contributor Author

Thanks for reviewing and merging 😄

@gschorcht gschorcht deleted the cpu/stm32/fix_riotboot_settings branch May 25, 2023 15:23
@benpicco benpicco added this to the Release 2023.07 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: build system Area: Build system Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants