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

Feature: Identify STM32MP15x_CM4 coprocessor #1546

Merged
merged 4 commits into from
Sep 23, 2023

Conversation

ALTracer
Copy link
Contributor

Detailed description

  • This is a new feature.
  • BMD can scan and attach to the Cortex-M4 coprocessor of STM32MP157 platform, but there's no memory map.
  • This PR solves that by implementing MP15x-specific detection and registering a SRAM map.

STM32MP157, MP153, MP151 is a MPU, microprocessor SoC with dual Cortex-A7 cores and one Cortex-M4 microcontroller/coprocessor core all connected to a lot of STM32-standard peripherals over quite a few multilayer buses. It has a proper SWJ-DP exposing JTAG and SWD capable pins. These are used for custom platform bring-up (CA7 secure bootloader chain) and for runtime-debugging coprocessor firmware.

The stm32mp15_probe() I implement is sufficient to work with coprocessor firmware. I also pulled a couple functions from H7/H5 target driver code to read out silicon revision and electronic signature unique ID.

MP15x has a complicated SWJ Access Port setup which interferes with debugging the CA7 cores by BMD. The recommended solution by ST is to use upstream OpenOCD or their fork, and that accesses a few registers over all three APs.
MP13x doesn't have a coprocessor and is outside this PR's scope.
MP2 is not released to General Availability yet.

Engineering Boot exposes the CM4 immediately (spinning in a loop in RETRAM), but Production Boot starts up the SoC with CM4 hidden. From my experience it appears later when OpenSTLinux boots, initializes remoteproc framework and manipulates DBGMCU_CR.

We did not significantly alter TrustZone-A settings in our software platform, but take note it can manifest in problems, too. Same on sleep states -- not using any, so no problems triggered by C-Sleep/C-Stop.

Your checklist for this pull request

  • I've read the Code of Conduct
  • I've read the guidelines for contributing to this repository
  • It builds for hardware native (make PROBE_HOST=native)
  • It builds as BMDA (make PROBE_HOST=hosted)
  • I've tested it to the best of my ability
  • My commit messages provide a useful short description of what the commits do

Closing issues

@dragonmux dragonmux added the New Target New debug target label Jul 19, 2023
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.

This is looking good. There are a few items of refinement we'd like to see and a couple of mistakes need fixing, but this is excellent work.

src/target/target_probe.h Outdated Show resolved Hide resolved
src/target/stm32mp15.c Outdated Show resolved Hide resolved
src/target/stm32mp15.c Outdated Show resolved Hide resolved
src/target/stm32mp15.c Outdated Show resolved Hide resolved
src/target/stm32mp15.c Outdated Show resolved Hide resolved
src/target/stm32mp15.c Outdated Show resolved Hide resolved
src/target/stm32mp15.c Outdated Show resolved Hide resolved
@dragonmux dragonmux added this to the v1.10 milestone Jul 19, 2023
@dragonmux
Copy link
Member

Additional review note: Per the contribution guidelines, the target/ prefix on the target reference in the commit message is not required, stm32mp15: instead of target/stm32mp15: is sufficient as a prefix

@ALTracer
Copy link
Contributor Author

ALTracer commented Aug 1, 2023

Additional review note: Per the contribution guidelines, the target/ prefix on the target reference in the commit message is not required, stm32mp15: instead of target/stm32mp15: is sufficient as a prefix

The first commit introduces a new target driver, so I thought to write the full prefix for context. Following commits omit the target/, but I might have said stm32mp1 in some of them. This was how it used to be referred to prior to introduction of STM32MP13x in 2022.

Applied the review changes trying to keep logic intact, as separate small commits specific to review items.

Tested with a stm32f103cb bluepill running stlink platform (and as hosted over that via bmp-remote, HL/non-HL) against the STM32MP157C target idling in Engineering Boot. This seems to still work both on old main and current main, after sorting out some minor breakage from DPv0 and DP power-down code (with your help, much appreciated!). Builds for native and hosted, +460 bytes of flash for native and stlink, both still fit.

Please let me know whether I can squash these cosmetic commits into one, and when I can fast-forward the main in my fork and rebase this on top of current main regardless of squashing.

@dragonmux
Copy link
Member

It seems like after addressing some of the review items above in your working copy, you never pushed, so we've added notes where that is obvious to us. We'd like to get this merged ASAP.

Regarding the structuring of the fixup commits, we'd appreciate the fixes being applied as fixups to the base commits of the PR and we'll then rebase merge the resulting commit series. For example, an initial commit that provides the probe changes, the next adding memory maps, and another couple after that implementing Flash routines (erase, then write), is what we typically aim for in this kind of scenario.

@ALTracer
Copy link
Contributor Author

This should look better. If something else is wrong, please submit additional review on top of updated patchset contents, preferably with (updated) line numbers. Otherwise, since the feature/style pass is complete, I am aiming to commence the rebase pass this week to get into v1.10 release.

Regarding the structuring of the fixup commits, we'd appreciate the fixes being applied as fixups to the base commits of the PR and we'll then rebase merge the resulting commit series. For example, an initial commit that provides the probe changes, the next adding memory maps, and another couple after that implementing Flash routines (erase, then write), is what we typically aim for in this kind of scenario.

There is only one base commit I see, the other 9 are minor edits (fixups). There is no flash in MP1. To reduce the size of one giant +170 commit, I propose splitting it into three functional compileable parts:

  1. Identification, probe hierarchy hooks, new TU (with license header) of minimal functionality (RAM map);
  2. UID & rev/IDCODE commands implementation (mostly H7 code + MP1 values);
  3. DBGMCU operations on attach/detach, priv

As of now this patchset takes up +432 bytes on native and still fits when cherry-picked verbatim to latest main (1236 bytes left). Using 12.2.Rel1 Toolchain.

@dragonmux
Copy link
Member

Your proposed patch series structure seems fine - we'll let you get the rebase done and review the result as we think that'll probably then be accepting the change set for merge.

* STM32MP157/3/1 has Cortex-A7 core(s) and Cortex-M4 coprocessor
* Detect the CM4 (when booting SoC in Engineering mode,
  and when CA7 starts a CM4 firmware in Production mode)
* Provide default SRAM map as visible to M4
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.

The only item preventing this PR from being ready for merge, is the one on the RAM base macro naming. Once that's solved this looks ready to go and we can approve for merge.

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.

LGTM, we'll get this merged after we merge the Cortex-A fix PR.

@dragonmux dragonmux merged commit 30e9fb6 into blackmagic-debug:main Sep 23, 2023
6 checks passed
@ALTracer ALTracer deleted the stm32mp15 branch August 20, 2024 23:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New Target New debug target
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants