Skip to content

Conversation

@sethp
Copy link
Contributor

@sethp sethp commented May 15, 2023

This change follows through on relocating the _vector_table, _start_trap, and _start_trap_rust functions for all present build/link modes for the 'c2, 'c3, 'c6, and 'h2.

It has been tested by running the software_interrupts example for the 'c3 in direct-boot and esp-bootloader contexts, but I wasn't able to identify how to run the mcu-boot mode for the 'c3, nor do I have present access to any of the other devices for testing.

Must

  • The code compiles without errors or warnings.
    • at least, it introduces no new ones: I do see something about Timer0::steal for all devices and RtcFastClockXtalD2 for the 'h2
  • All examples work.
    • that I was able to test, anyway (see above)
  • cargo fmt was run.
  • Your changes were added to the CHANGELOG.md in the proper section.
  • You updated existing examples or added examples (if applicable).
  • Added examples are checked in CI

Nice to have

  • You add a description of your work to this PR.
  • You added proper docs for your newly added features and code.

sethp added 2 commits May 10, 2023 17:24
Previously, the trap vector itself and its immediate callees
(`_start_trap` and `_start_trap_rust_hal`) were located in the mapped
instruction flash range `0x420..`, increasing cache pressure and adding
variable latency to the very beginning of the interrupt/exception
service flow.

This change places those routines in iram directly:

```
   Num:    Value  Size Type    Bind   Vis      Ndx Name
 48177: 40380280  2428 FUNC    GLOBAL DEFAULT    6 _start_trap_rust_hal
 48197: 40380bfc    54 FUNC    GLOBAL DEFAULT    6 _start_trap_rust
 48265: 40380200     0 FUNC    GLOBAL DEFAULT    6 _vector_table
 48349: 40380100     0 NOTYPE  GLOBAL DEFAULT    6 default_start_trap
 48350: 40380100     0 NOTYPE  GLOBAL DEFAULT    6 _start_trap
```

As seen via `readelf -W -s -C ./target/riscv32imc-unknown-none-elf/debug/examples/gpio_interrupt | grep -E _start_trap\|_vector\|Ndx`
This change follows through on relocating the `_vector_table`,
`_start_trap`, and `_start_trap_rust` functions for all present
build/link modes for the 'c2, 'c3, 'c6, and 'h2.

It has been tested by running the `software_interrupts` example for the
'c3 in direct-boot and esp-bootloader contexts, but I wasn't able to
identify how to run the `mcu-boot` mode for the 'c3, nor do I have
present access to any of the other devices for testing.
@sethp
Copy link
Contributor Author

sethp commented May 15, 2023

Hmm, I'm not sure whether to add a CHANGELOG entry. I see at the top that it says:

Please note that only changes to the esp-hal-common package are tracked in this CHANGELOG.

Seems like we ought to either remove that caveat or update the changelog CI action to reflect it. What do you think?

@bjoernQ bjoernQ added the skip-changelog No changelog modification needed label May 15, 2023
@bjoernQ
Copy link
Contributor

bjoernQ commented May 15, 2023

The skip-changelog label should disable the check (in rare cases where a PR doesn't add something that should get mentioned in the CHANGELOG)

@sethp sethp changed the title Feat/riscv isr ram ld scripts feat(riscv): relocate .trap machinery to RAM May 15, 2023
Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

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

So it looks like this change moves about 2775 bytes into RAM. I don't think that will a be a huge deal, especially since the hal trap function is actually taking up most of that:

riscv32-esp-elf-objdump -t target/riscv32imc-unknown-none-elf/release/examples/software_interrupts| grep start_trap_rust_hal
40380880 g     F .trap  0000095a _start_trap_rust_hal

I imagine improvements in other PRs will reduce this usage quite a bit.

In my opinion this LGTM. Will let @bjoernQ voice any concerns about the memory usage if he wants.

@MabezDev MabezDev requested a review from bjoernQ May 22, 2023 11:47
@bjoernQ
Copy link
Contributor

bjoernQ commented May 22, 2023

I guess 2775 bytes should be okay. If this becomes a problem in future, we can still make it an opt-out feature

But I have no idea why the changelog-check still fails with the label 🤔 On the other hand probably this is really a change that deserves a changelog-entry in the "changed" section

@MabezDev
Copy link
Member

On the other hand probably this is really a change that deserves a changelog-entry in the "changed" section

I agree, @sethp would you mind adding a quick changelog entry under the "changed" section?

@MabezDev MabezDev removed the skip-changelog No changelog modification needed label May 22, 2023
@sethp
Copy link
Contributor Author

sethp commented May 22, 2023

Yeah for sure: sorry for the confusion, I was just thrown by the "only changes to esp-hal-common" bit when this doesn't actually touch that directory at all.

@sethp
Copy link
Contributor Author

sethp commented May 22, 2023

Also, I'm totally on board for gating this somehow, I'm just not sure what that looks like with linker scripts. A change to the build.rs that generates/copies different files depending on feature flags was the least-bad idea I had, and that seemed like it'd fall down ~immediately once there was a second feature to wrangle that way.

But if y'all want that, I'm happy to take it on: just let me know!

@bjoernQ
Copy link
Contributor

bjoernQ commented May 22, 2023

Also, I'm totally on board for gating this somehow, I'm just not sure what that looks like with linker scripts. A change to the build.rs that generates/copies different files depending on feature flags was the least-bad idea I had, and that seemed like it'd fall down ~immediately once there was a second feature to wrangle that way.

But if y'all want that, I'm happy to take it on: just let me know!

The build.rs way is already used for ESP32 to reserve more of the RAM for bluetooth - t.b.h. I haven't had any better (or any other) idea

@MabezDev MabezDev merged commit 171e66e into esp-rs:main May 26, 2023
@sethp sethp deleted the feat/riscv-isr-ram-ld-scripts branch May 28, 2023 18:07
MabezDev pushed a commit to MabezDev/esp-hal that referenced this pull request Jun 1, 2023
* feat: relocate riscv isr to iram

Previously, the trap vector itself and its immediate callees
(`_start_trap` and `_start_trap_rust_hal`) were located in the mapped
instruction flash range `0x420..`, increasing cache pressure and adding
variable latency to the very beginning of the interrupt/exception
service flow.

This change places those routines in iram directly:

```
   Num:    Value  Size Type    Bind   Vis      Ndx Name
 48177: 40380280  2428 FUNC    GLOBAL DEFAULT    6 _start_trap_rust_hal
 48197: 40380bfc    54 FUNC    GLOBAL DEFAULT    6 _start_trap_rust
 48265: 40380200     0 FUNC    GLOBAL DEFAULT    6 _vector_table
 48349: 40380100     0 NOTYPE  GLOBAL DEFAULT    6 default_start_trap
 48350: 40380100     0 NOTYPE  GLOBAL DEFAULT    6 _start_trap
```

As seen via `readelf -W -s -C ./target/riscv32imc-unknown-none-elf/debug/examples/gpio_interrupt | grep -E _start_trap\|_vector\|Ndx`

* feat(riscv): place .trap in RAM

This change follows through on relocating the `_vector_table`,
`_start_trap`, and `_start_trap_rust` functions for all present
build/link modes for the 'c2, 'c3, 'c6, and 'h2.

It has been tested by running the `software_interrupts` example for the
'c3 in direct-boot and esp-bootloader contexts, but I wasn't able to
identify how to run the `mcu-boot` mode for the 'c3, nor do I have
present access to any of the other devices for testing.

* docs: Update CHANGELOG.md
MabezDev pushed a commit to MabezDev/esp-hal that referenced this pull request Jun 1, 2023
* feat: relocate riscv isr to iram

Previously, the trap vector itself and its immediate callees
(`_start_trap` and `_start_trap_rust_hal`) were located in the mapped
instruction flash range `0x420..`, increasing cache pressure and adding
variable latency to the very beginning of the interrupt/exception
service flow.

This change places those routines in iram directly:

```
   Num:    Value  Size Type    Bind   Vis      Ndx Name
 48177: 40380280  2428 FUNC    GLOBAL DEFAULT    6 _start_trap_rust_hal
 48197: 40380bfc    54 FUNC    GLOBAL DEFAULT    6 _start_trap_rust
 48265: 40380200     0 FUNC    GLOBAL DEFAULT    6 _vector_table
 48349: 40380100     0 NOTYPE  GLOBAL DEFAULT    6 default_start_trap
 48350: 40380100     0 NOTYPE  GLOBAL DEFAULT    6 _start_trap
```

As seen via `readelf -W -s -C ./target/riscv32imc-unknown-none-elf/debug/examples/gpio_interrupt | grep -E _start_trap\|_vector\|Ndx`

* feat(riscv): place .trap in RAM

This change follows through on relocating the `_vector_table`,
`_start_trap`, and `_start_trap_rust` functions for all present
build/link modes for the 'c2, 'c3, 'c6, and 'h2.

It has been tested by running the `software_interrupts` example for the
'c3 in direct-boot and esp-bootloader contexts, but I wasn't able to
identify how to run the `mcu-boot` mode for the 'c3, nor do I have
present access to any of the other devices for testing.

* docs: Update CHANGELOG.md
SergioGasquez pushed a commit to SergioGasquez/esp-hal that referenced this pull request Jun 9, 2023
* feat: relocate riscv isr to iram

Previously, the trap vector itself and its immediate callees
(`_start_trap` and `_start_trap_rust_hal`) were located in the mapped
instruction flash range `0x420..`, increasing cache pressure and adding
variable latency to the very beginning of the interrupt/exception
service flow.

This change places those routines in iram directly:

```
   Num:    Value  Size Type    Bind   Vis      Ndx Name
 48177: 40380280  2428 FUNC    GLOBAL DEFAULT    6 _start_trap_rust_hal
 48197: 40380bfc    54 FUNC    GLOBAL DEFAULT    6 _start_trap_rust
 48265: 40380200     0 FUNC    GLOBAL DEFAULT    6 _vector_table
 48349: 40380100     0 NOTYPE  GLOBAL DEFAULT    6 default_start_trap
 48350: 40380100     0 NOTYPE  GLOBAL DEFAULT    6 _start_trap
```

As seen via `readelf -W -s -C ./target/riscv32imc-unknown-none-elf/debug/examples/gpio_interrupt | grep -E _start_trap\|_vector\|Ndx`

* feat(riscv): place .trap in RAM

This change follows through on relocating the `_vector_table`,
`_start_trap`, and `_start_trap_rust` functions for all present
build/link modes for the 'c2, 'c3, 'c6, and 'h2.

It has been tested by running the `software_interrupts` example for the
'c3 in direct-boot and esp-bootloader contexts, but I wasn't able to
identify how to run the `mcu-boot` mode for the 'c3, nor do I have
present access to any of the other devices for testing.

* docs: Update CHANGELOG.md
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