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

riscv : fix the inverted logc interrupt-are-disabled-while-single-ste… #1767

Merged

Conversation

mean00
Copy link

@mean00 mean00 commented Feb 16, 2024

Detailed description

There is a small inverted logic in the "disable-interrupt-while-single-stepping" code
The STEPIE bit actually enables the interrupt while single stepping
The change inverts the logic (and let is "off" when leaving single step mode)
With that patch you dont go into timer interrupt as soon as you hit next with gdb
(on some platforms that bit is glued to zero, so the problem may not show)

Your checklist for this pull request

@klenSA
Copy link

klenSA commented Jul 30, 2024

Hello! I would really like to advance the state of the BMP code for using with ch32v30x , please give me a usage scenario - I will try to test the work.

@dragonmux
Copy link
Member

Having finally managed to actually review the debug manual and this patch, the original behaviour is intended in that, with interrupts disabled it'll substantially change execution of the user firmware while stepping. Most parts we've seen seem lockstep their peripherals to core halt, allowing you to step through exactly how the firmware would execute without the debugger present. It would be interesting to see if the part you're debugging does not do this, leading to wanting interrupts while stepping to be disabled.

In either case though, if this patch is actually warranted, then not manipulating STEPIE at all would be more correct as the bit defaults to 0 on reset, and does not want to be on when executing normally - only potentially while single-stepping.

@mean00
Copy link
Author

mean00 commented Jul 31, 2024

Thank you for the review
The part is CH32Vxxx
The goal was to mimick the Arm behavour within the blackmagic, where the Systick+ external configurable interrupts are disabled while single stepping
I'll have to look at it in detail, but on the CH32vxx you can lock timers, i2c etc.. to the debug state but apparently not the systick, so it was firing all the time (chapter 34.2.1 of the datasheet for reference).
I'll look at it again & see if just not touching STEPIE does the trick

@mean00
Copy link
Author

mean00 commented Aug 3, 2024

I did a couple of quick tryouts :

  • Not touching STEPIE at all as you suggested => works
  • Additionally, i tried to let the STEPIE as it is but also setting STOP_COUNTERS and STOP_TIME=> does not seem to work (might be hardwired to zero, i'll look deeper into that).

Tested on the CH32V3xx, probably the same for CH32V2XX.
Thank you

@dragonmux
Copy link
Member

Please rebase this PR on main and we will get it reviewed with a view to getting this merged.

@dragonmux dragonmux added this to the v2.0 release milestone Nov 20, 2024
@dragonmux dragonmux added the Bug Confirmed bug label Nov 20, 2024
@dragonmux
Copy link
Member

With your permission, we'll do the rebase ourself so we can get progress on this PR and try to get it merged for v2.0

@mean00 mean00 force-pushed the riscv_fix_interrupt_while_stepping branch from 632674d to 10ec2da Compare November 21, 2024 05:44
@mean00
Copy link
Author

mean00 commented Nov 21, 2024

Sorry, i didnt notice the previous notification
I've quickly rebased the patch i've been using for a while. It works with the CH32vXX chips and contains the stop_time/stop_counter settings, those have no effect on CH32VXX but they may work on other chips
Thank you

@mean00 mean00 force-pushed the riscv_fix_interrupt_while_stepping branch from 10ec2da to 3b50115 Compare November 21, 2024 05:53
Copy link
Member

@dragonmux dragonmux left a comment

Choose a reason for hiding this comment

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

If you can please rebase the 3 commits into one, keeping only the commit message with the first, fixed to remove the extra in front of the :, then we will get this merged. This looks good as implemented.

Thank you for the contribution!

@mean00 mean00 force-pushed the riscv_fix_interrupt_while_stepping branch from 3b50115 to 5652979 Compare November 21, 2024 18:44
@mean00
Copy link
Author

mean00 commented Nov 21, 2024

did that too quickly, should be okay now
Thank you

@dragonmux dragonmux merged commit 5652979 into blackmagic-debug:main Nov 21, 2024
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Confirmed bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants