west_commands: runners: openocd: clear DHCSR after flashing completes #33163

ioannisg · 2021-03-08T21:49:45Z

openOCD does not clear DHCSR.DEBUGEN bit after flashing
completes, effectively leaving the Cortex-M processor in
debug mode. Manually clean DHCSR becore exiting the flash
operation, to work around the problem.

Signed-off-by: Ioannis Glaropoulos [email protected]

ioannisg · 2021-03-08T21:51:03Z

This is just a trial for now - do not merge, since the fix should only be applicable for Cortex-M.
@erwango could you check if this would work for openOCD-flashed targets?

scripts/west_commands/runners/openocd.py

mbolivar-nordic

Thanks @ioannisg . I read the discussion in upstream openocd and it looks like you really found something new here. A couple of minor requests and then I am +1 from the runner perspective.

However, I want to confirm with @erwango and other openocd users that we really want this to be the default behavior before merging, since the openocd maintainers named some use cases where they didn't think it made a sensible default.

mbolivar-nordic · 2021-03-09T23:36:08Z

scripts/west_commands/runners/openocd.py

-               ['-c', 'reset run',
-                '-c', 'shutdown'])
+               ['-c', 'reset run'] +
+               clear_halt_debug_en_cmd +


This name is confusing mechanism with policy. Please rename it to post_reset_cmd instead.

Yeap, that's fine.

Renaming all the occurrences and related variables

mbolivar-nordic · 2021-03-09T23:37:00Z

boards/common/openocd.board.cmake

@@ -16,7 +16,13 @@ endif()
 set(OPENOCD_CMD_LOAD_DEFAULT "${OPENOCD_FLASH}")
 set(OPENOCD_CMD_VERIFY_DEFAULT "verify_image")

+# Cortex-M-specific halt debug enable bit clearing


Can you add some more comments here, since this looks "weird"? Maybe just linking to the openocd PR you opened would be good enough.

I'll comment properly.

ioannisg · 2021-03-12T13:11:44Z

However, I want to confirm with @erwango and other openocd users that we really want this to be the default behavior before merging, since the openocd maintainers named some use cases where they didn't think it made a sensible default.

Fine. I 've, also added the HW block label, so this is tested before it (ever) gets merged.

My understanding is that "west flash" shall only program the binary and reset the core to a state as close as possible to the (cold) reset state. Since the internal runner implementation does not guarantee this, we are left with the option to do this in the runner.

Add a new option to the openocd.py script, for clearing the halt debug enable state. Signed-off-by: Ioannis Glaropoulos <[email protected]>

Introduce a command in openocd.board.cmake for cortex-m targets, which clears the halt debug enable bit flag, upon openocd exit. This is required since openocd does not clear the state itself when it shuts down. Signed-off-by: Ioannis Glaropoulos <[email protected]>

erwango · 2021-03-12T14:41:15Z

@ioannisg is this PR still required if http://openocd.zylin.com/#/c/6093/1 gets merged ?

Introduce a command in pyocd.board.cmake for ARM Cortex-m targets, which clears the halt debug enable bit flag, after flashing is complete and before pyocd exit. This is required since pyocd regular flashing command does not clear the state itself when it completes, nor when pyocd session shuts down. Signed-off-by: Ioannis Glaropoulos <[email protected]>

Add a new option to the pyocd.py script, for clearing the halt debug enable state. The option results in a command that is called after the flashing is complete. Signed-off-by: Ioannis Glaropoulos <[email protected]>

erwango · 2021-03-15T15:44:17Z

One consideration on current behavior is that having DHCSR set after flash is that it prevents target entering sleep states when testing PM feature.
In case DHCSR is cleared and PM sleep state is entered, flashing using openocd is not possible anymore and another flashing method shoud be used.
So, having DHCSR set after flash:

is bad for measurements... BUT
allows PM testing in CI with existing flashing methods.

mbolivar-nordic · 2021-03-15T16:07:20Z

scripts/west_commands/runners/pyocd.py

@@ -158,6 +163,24 @@ def flash(self, **kwargs):
        self.logger.info('Flashing file: {}'.format(fname))
        self.check_call(cmd)

+        if self.post_flash_cmd is not []:


This is not correct usage. In Python, foo is bar and foo is not bar basically work like pointer comparison in C.

Example:

>>> [] is [] False

This should be if self.post_flash_cmd: instead, because an empty list is boolean false.

mbolivar-nordic · 2021-03-15T16:08:45Z

boards/common/pyocd.board.cmake

 board_set_flasher_ifnset(pyocd)
 board_set_debugger_ifnset(pyocd)
-board_finalize_runner_args(pyocd "--dt-flash=y")
+board_finalize_runner_args(pyocd "--dt-flash=y"
+    --post-flash "${PYOCD_CMD_HALT_DEBUG_EN_CLEAR}")


This is reading from an uninitialized variable on non-Cortex-M

erwango

Nacking until we have an alternative viable option to allow Power Management testing in CI runs.

As previously stated, current openocd behavior regarding debug enable bit allows to flash target after sleep mode code has been exercised, while the proposed change prevents this.

galak · 2021-03-30T17:55:38Z

@ioannisg any updates here? We should set the default of the feature to disabled until we resolve a number of these issues.

github-actions · 2021-07-14T00:29:23Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

ioannisg added the DNM This PR should not be merged (Do Not Merge) label Mar 8, 2021

ioannisg requested review from erwango and galak March 8, 2021 21:49

galak requested a review from mbolivar-nordic March 8, 2021 22:07

ioannisg requested a review from MaureenHelm March 8, 2021 23:03

mbolivar-nordic reviewed Mar 8, 2021

View reviewed changes

scripts/west_commands/runners/openocd.py Outdated Show resolved Hide resolved

ioannisg force-pushed the west_openocd_clear_debug_mode branch from ae5af78 to 96d55e1 Compare March 9, 2021 15:53

ioannisg marked this pull request as ready for review March 9, 2021 15:54

ioannisg added block: HW Test Testing on hardware required before merging and removed DNM This PR should not be merged (Do Not Merge) labels Mar 9, 2021

ioannisg requested a review from mbolivar-nordic March 9, 2021 15:54

github-actions bot added the area: Boards label Mar 9, 2021

ioannisg force-pushed the west_openocd_clear_debug_mode branch from 96d55e1 to 5281083 Compare March 9, 2021 16:29

mbolivar-nordic suggested changes Mar 9, 2021

View reviewed changes

zephyrbot added the area: West West utility label Mar 10, 2021

zephyrbot requested a review from carlescufi March 10, 2021 11:14

zephyrbot assigned mbolivar-nordic Mar 10, 2021

ioannisg force-pushed the west_openocd_clear_debug_mode branch from 5281083 to 3807c1f Compare March 12, 2021 13:00

ioannisg added 2 commits March 12, 2021 15:34

scripts: openocd.py: add option to clear halt debug enable state

661dd53

Add a new option to the openocd.py script, for clearing the halt debug enable state. Signed-off-by: Ioannis Glaropoulos <[email protected]>

ioannisg force-pushed the west_openocd_clear_debug_mode branch from 3807c1f to 5627839 Compare March 12, 2021 14:35

ioannisg added 2 commits March 12, 2021 16:24

scripts: pyocd.py: add option to clear halt debug enable state

e14cc2a

Add a new option to the pyocd.py script, for clearing the halt debug enable state. The option results in a command that is called after the flashing is complete. Signed-off-by: Ioannis Glaropoulos <[email protected]>

mbolivar-nordic suggested changes Mar 15, 2021

View reviewed changes

erwango requested changes Mar 16, 2021

View reviewed changes

github-actions bot added the Stale label Jul 14, 2021

github-actions bot closed this Jul 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

west_commands: runners: openocd: clear DHCSR after flashing completes #33163

west_commands: runners: openocd: clear DHCSR after flashing completes #33163

ioannisg commented Mar 8, 2021 •

edited

Loading

ioannisg commented Mar 8, 2021

mbolivar-nordic left a comment

mbolivar-nordic Mar 9, 2021

ioannisg Mar 12, 2021

ioannisg Mar 12, 2021

mbolivar-nordic Mar 9, 2021

ioannisg Mar 12, 2021

ioannisg commented Mar 12, 2021

erwango commented Mar 12, 2021

erwango commented Mar 15, 2021

mbolivar-nordic Mar 15, 2021

mbolivar-nordic Mar 15, 2021

erwango left a comment •

edited

Loading

galak commented Mar 30, 2021

github-actions bot commented Jul 14, 2021

west_commands: runners: openocd: clear DHCSR after flashing completes #33163

west_commands: runners: openocd: clear DHCSR after flashing completes #33163

Conversation

ioannisg commented Mar 8, 2021 • edited Loading

ioannisg commented Mar 8, 2021

mbolivar-nordic left a comment

Choose a reason for hiding this comment

mbolivar-nordic Mar 9, 2021

Choose a reason for hiding this comment

ioannisg Mar 12, 2021

Choose a reason for hiding this comment

ioannisg Mar 12, 2021

Choose a reason for hiding this comment

mbolivar-nordic Mar 9, 2021

Choose a reason for hiding this comment

ioannisg Mar 12, 2021

Choose a reason for hiding this comment

ioannisg commented Mar 12, 2021

erwango commented Mar 12, 2021

erwango commented Mar 15, 2021

mbolivar-nordic Mar 15, 2021

Choose a reason for hiding this comment

mbolivar-nordic Mar 15, 2021

Choose a reason for hiding this comment

erwango left a comment • edited Loading

Choose a reason for hiding this comment

galak commented Mar 30, 2021

github-actions bot commented Jul 14, 2021

ioannisg commented Mar 8, 2021 •

edited

Loading

erwango left a comment •

edited

Loading