-
-
Notifications
You must be signed in to change notification settings - Fork 783
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: Multi-AP handling on STM32MP15 #1757
base: main
Are you sure you want to change the base?
Feature: Multi-AP handling on STM32MP15 #1757
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking pretty decent and we can mostly work out the rationale behind each change (even if it took us a minute). We have some suggestions to improve that and a request to write out a little more of the intent in comments to aid with that, but with those taken care of we think this will be good to merge.
extern void cortexar_detach(target_s *target); | ||
|
||
/* | ||
* Override memory r/w operations to go via the MEM-AP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're all MEM-AP's, think you mean AXI AP (AP0)
?
src/target/stm32mp15.c
Outdated
return true; | ||
} | ||
|
||
static void stm32mp15_setup_apbd_ap(target_s *const target) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given how the setup functions are named (and we agree with this naming, though aren't sure where the d
comes from in apbd
here?), we suggest being absolutely consistent below - apb_ap
, axi_ap
, so the code's a little easier to follow. Either that or using the AP number instead of bus type for the naming - ap0
, ap1
? That one's up to you which you think would be more beneficial here.
f7476a6
to
fc8ec1c
Compare
stm32mp15: Manage the AXI AP in CA7 target case (experimental)
fc8ec1c
to
74e35f4
Compare
(void)argc; | ||
(void)argv; | ||
/* TODO: argv parsing for mode and baudrate */ | ||
adiv5_access_port_s *const ap_apbd = (adiv5_access_port_s *)target->target_storage; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As with the other places this apbd
naming has come up, we do not know where this d
is coming from in the naming - please either name by the AP number this AP represents, or just say ap_apb
- if it's supposed to mean "debug", that's already implied by it being the bus attached to the AP, but those busses aren't some special thing.. they are normal APB's and AHB's, so including the d
in the nomenclature just muddies things in our opinion. Readers not familiar with things would try searching for an APBD
and get no results which is counter-productive, for example.
Detailed description
This was written as a PoC in December, before the cortexar STC/LDC DTRRX speedups got developed and tested. Using AXI-AP may provide background access to entire DDR3 and SYSRAM without halting either CA7 of the platform (or single CA7 on MP151).
I need in particular the APB-D to be able to setup the CoreSight SWO block in SoC system scope, which is shared between CA7 MPCore and CM4 and hence neither of them can write to its configuration registers page. The rest (ITM, DWT, RCC) are accessible in some sort and can be poked via Orbuculum's gdbtrace.init script.
Your checklist for this pull request
Closing issues