Fix for debugging when trying to load to ram. #87423
Conversation
zephyrbot
requested review from
butok,
carlescufi,
dleach02,
mbolivar,
mmahadevan108,
pdgendt and
swinslow
| @@ -6,10 +6,7 @@ | |||
|
|
|||
| if(CONFIG_SOC_MIMXRT1176_CM7 OR CONFIG_SECOND_CORE_MCUX) | |||
| board_runner_args(pyocd "--target=mimxrt1170_cm7") | |||
| board_runner_args(jlink "--device=MIMXRT1176xxxA_M7" "--reset-after-load") | |||
There was a problem hiding this comment.
don't we want to reset after load for the more expected use case of actually booting with the ROM? or is this just what is now the default?
There was a problem hiding this comment.
This should be the default, this seems to be the default for the other boards as well which is one step that prevented them from running into this issue.
| # If the flash node points to ram, linserver treats | ||
| # the ram as inaccessible and does not flash. | ||
| gdb_cmd += ['-ex', 'set mem inaccessible-by-default off'] | ||
| gdb_cmd += ['-ex', 'monitor reset', '-ex', 'load'] |
There was a problem hiding this comment.
isn't this a problem with linkserver and not this zephyr runner?
| @@ -421,7 +421,7 @@ def do_run_common_image(command, user_args, user_runner_args, used_cmds, | |||
| # the board has enabled this functionality, check if the board should be | |||
| # reset or not. If this is not specified in the board/soc file, leave it up to | |||
| # the runner's default configuration to decide if a reset should occur. | |||
| if runner_cls.capabilities().reset and '--no-reset' not in final_argv: | |||
| if runner_cls.capabilities().reset and '--no-reset' not in final_argv and len(board_image_count): | |||
There was a problem hiding this comment.
the comment should be updated about what is this doing, I don't understand
There was a problem hiding this comment.
This is not something that should be addressed in this PR. The intention of the comment makes sense, the implementation uses a default set of conditions for multiple images if none are passed in as parameters, the only update I made is it doesn't properly check or test rather this code runs if there's only one image so I added the check. I have nothing to do with the implementation so maybe my assumption of the comment is wrong as well thus I cannot make the change.
| # ITCM is not defined in RT1170's LinkServer device file | ||
| board_runner_args(linkserver "--override=/device/memory/-=\{\"location\":\"0x00000000\",\ | ||
| \"size\":\"0x00040000\",\"type\":\"RAM\"\}") |
There was a problem hiding this comment.
the comment here makes me think once again that the device memory map linkserver issue is not a zephyr problem
There was a problem hiding this comment.
This code is actually wrong, and Linkserver has the correct mapping for this address space. Specifically, we gave the wrong size when this line was created but again no longer needed since Linkserver has the correct address mapping.
EmilioCBen
changed the title
| @@ -421,7 +421,7 @@ def do_run_common_image(command, user_args, user_runner_args, used_cmds, | |||
| # the board has enabled this functionality, check if the board should be | |||
| # reset or not. If this is not specified in the board/soc file, leave it up to | |||
| # the runner's default configuration to decide if a reset should occur. | |||
| if runner_cls.capabilities().reset and '--no-reset' not in final_argv: | |||
| if runner_cls.capabilities().reset and '--no-reset' not in final_argv and len(board_image_count): | |||
| if board_image_count is not None: | |||
| reset = True | |||
There was a problem hiding this comment.
@nordicjm your PR #69748 made the initial change here with respect to board_image_count and the forced addition of reset. What we found is that the test here for not None is always true because board_image_count is instantiated but doesn't have any elements in it... so not None. This results in the runner always adding a reset to the gdb commands. I suspect, from your comment block above, that this wasn't your intention.
We didn't notice this change because in the normal debug situation, you wouldn't care if the gdb did reset, load, reset if the image was being put in flash. But if you are targeting an image to a RAM space, the reset, load, reset causes the bootrom to load the flash image preventing you from debugging the image you created for RAM loading/debugging.
In parallel to this PR being added, I just saw this other PR [#87138] (@VynDragon) that just merged to force the larger logic to NOT ignore the no-reset runner option. We could have done the same thing now that I see this but I'm still questioning the logic of "if board_image_count not None" when combined with the comment block about multiple images.
There was a problem hiding this comment.
board_image_count is init in the common function, it should not not ever be none. As for why I put the none check in, I do not remember
There was a problem hiding this comment.
so in this particular case, the len(board_image_count) was 0 and we needed to have this check to keep the rest of your logic from doing another reset.
decsny
dismissed
their stale review
removing block because lost track of this PR
EmilioCBen
force-pushed
the
fix/mimxrt1170_flash_ram
branch
from
cd5a1a8 to
edae439
Compare
When users try to debug while flashing into ram, JLink/Linkserver would fail to flash straight to ram. Each with their own issues. Both would reset after load which would flush the ram and point the PC to flash. Linkserver treated the ram as inaccessible, and the added memory region in board.cmake has the incorrect size for the region and is already included inside linkserver. Signed-off-by: Emilio Benavente <[email protected]>
EmilioCBen
force-pushed
the
fix/mimxrt1170_flash_ram
branch
from
edae439 to
add46b5
Compare
|
Re-assigned as I can't comment on the changes. |
Fixes #87422
When users try to debug while flashing into ram,
JLink/Linkserver would fail to flash straight to ram. Each with their own issues. Both would reset after load which would flush the ram and point the PC to flash. Linkserver treated the ram as inaccessible, and the added memory region in board.cmake has the incorrect size for the region and is already included inside linkserver.