Skip to content

arm: Use AP specific types & enumerate Addresses rather than APs - #2683

Merged
bugadani merged 1 commit into
probe-rs:masterfrom
ithinuel:add-adiv6-support-step-2
Jul 31, 2024
Merged

arm: Use AP specific types & enumerate Addresses rather than APs#2683
bugadani merged 1 commit into
probe-rs:masterfrom
ithinuel:add-adiv6-support-step-2

Conversation

@ithinuel

@ithinuel ithinuel commented Jul 15, 2024

Copy link
Copy Markdown
Contributor
  • Enumerate Memory AP addresses instead of giving the number of valid
    (Generic and Memory) APs.
    This will enable future development where APv2’s live in disjoint memory
    spaces.
  • Use AP specific types
    This should allow future developments to enabled things like secure
    debug, memory tagging, etc
  • Drop ApInformation and MemoryApInformation
    The AP specific types carry these information internally so these types
    are no longer required.

Follows:

Comment thread probe-rs/src/architecture/arm/ap/generic_ap.rs
@ithinuel
ithinuel force-pushed the add-adiv6-support-step-2 branch from 7961632 to 693b4b0 Compare July 16, 2024 21:32
@github-actions

Copy link
Copy Markdown
Contributor

The following changelog fragments do not match the expected pattern:

  • changelog/draft.md

Files should start with one of the categories followed by a dash, and end with '.md'
For example: 'added-foo-bar.md'

Valid categories are:

  • added
  • changed
  • fixed
  • removed

@ithinuel
ithinuel force-pushed the add-adiv6-support-step-2 branch 5 times, most recently from 4571ff7 to e829bff Compare July 21, 2024 21:37
Comment thread changelog/changed-use-docsplay-for-ArmError.md Outdated
@ithinuel
ithinuel force-pushed the add-adiv6-support-step-2 branch 7 times, most recently from c7b6def to c33729a Compare July 23, 2024 21:07
@ithinuel
ithinuel marked this pull request as ready for review July 23, 2024 21:07
@ithinuel

Copy link
Copy Markdown
Contributor Author

@bugadani I’m sorry the last commit is bigger, I didn’t really now how to detangle the various parts.

Comment thread changelog/changed-accessport-to-accessporttype.md Outdated
Comment thread changelog/changed-factorize-MemoryInterface-slight-variations.md Outdated
Comment thread changelog/changed-valid-access-ports-return-type.md Outdated
Comment thread probe-rs/Cargo.toml
Comment thread probe-rs/src/memory/mod.rs Outdated
@ithinuel
ithinuel force-pushed the add-adiv6-support-step-2 branch from c33729a to dcee752 Compare July 24, 2024 06:21
@ithinuel

Copy link
Copy Markdown
Contributor Author

There are a few things that I’m not fully satisfied in this PR but I had to stop somewhere :P
Just to list a few:

  • The methods remaining in ArmMemoryInterface.
    • The way we have to return Result<&mut _, _> etc. Something feels off about it.
    • The newly introduced base_address.
  • The mocks that seem to implement the same trait in slightly different ways (based on the tests needs).
    This adds lots of noise and require changes anytime there’s an unrelated change in the trait.
    It’d be easier with 1 well features mock for the other tests to use.

There are more refactor coming with the introduction of APv2 & ADIv6 (mostly moving some modules around to make place for an "apv2" module.

Comment thread probe-rs/src/architecture/arm/ap/generic_ap.rs
@ithinuel
ithinuel force-pushed the add-adiv6-support-step-2 branch from dcee752 to 2a5d786 Compare July 24, 2024 18:26
Comment thread probe-rs/src/memory/mod.rs Outdated
@ithinuel
ithinuel force-pushed the add-adiv6-support-step-2 branch 2 times, most recently from 00b02b0 to 74233de Compare July 24, 2024 23:10
Comment thread probe-rs/src/architecture/arm/ap/mod.rs Outdated
@bugadani

Copy link
Copy Markdown
Contributor

NGL this is very hard to review. I don't have experience with the code, and even though I assume most of this is just pushing stuff around, I can't track what goes where. The size of the whole diff makes my computer cry, and the online vscode is just not optimal to read diffs either :)

Because we squash PRs into single commits, I would recommend to split out the independent parts into separate PRs. That would make them easier to review and approve, as well as change tracking/blaming/reverting independent parts becomes about an order of magnitude easier.

I would recommend submitting the initial refactors in one go, the bugfix in a second phase, and the last commit's madness last. As you said the last commit is hard to split up, I guess I'll have to bite the bullet there, but it will take time and it would be unfortunate if the PR gathered conflicts in the meantime - merging the rest should somewhat reduce the diff surface to help with that.

@ithinuel
ithinuel force-pushed the add-adiv6-support-step-2 branch from 74233de to bac99d0 Compare July 25, 2024 18:53
@ithinuel
ithinuel marked this pull request as draft July 25, 2024 18:53
@ithinuel

ithinuel commented Jul 25, 2024

Copy link
Copy Markdown
Contributor Author

@ithinuel
ithinuel force-pushed the add-adiv6-support-step-2 branch 2 times, most recently from 94a6415 to 3685203 Compare July 25, 2024 19:12
@ithinuel
ithinuel force-pushed the add-adiv6-support-step-2 branch from 3685203 to 21ef060 Compare July 28, 2024 08:09
@ithinuel
ithinuel marked this pull request as ready for review July 28, 2024 08:10
@ithinuel
ithinuel force-pushed the add-adiv6-support-step-2 branch 2 times, most recently from 1eea56d to a7b8185 Compare July 28, 2024 23:42
@ithinuel ithinuel changed the title arm: changes a bunch of things in prep for ADIv6 arm: Use AP specific types & enumerate Addresses rather than APs Jul 29, 2024

@bugadani bugadani left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not done reviewing yet, completing this will take time.

Comment thread probe-rs/src/architecture/arm/ap/memory_ap/mod.rs Outdated
Comment thread probe-rs/src/probe/stlink/mod.rs
@ithinuel
ithinuel force-pushed the add-adiv6-support-step-2 branch from a7b8185 to f9ea724 Compare July 29, 2024 16:55
Comment thread probe-rs/src/architecture/arm/ap/memory_ap/mock.rs Outdated
Comment thread probe-rs/src/architecture/arm/ap/memory_ap/amba_ahb3.rs Outdated
@ithinuel
ithinuel force-pushed the add-adiv6-support-step-2 branch from f9ea724 to f57c24c Compare July 30, 2024 19:49
Comment thread probe-rs/src/architecture/arm/ap/memory_ap/amba_ahb3.rs Outdated
@ithinuel
ithinuel force-pushed the add-adiv6-support-step-2 branch 2 times, most recently from 8b99a1e to 083c6b1 Compare July 31, 2024 06:56
- Enumerate Memory AP addresses instead of giving the number of valid
  (Generic and Memory) APs.
  This will enable future development where APv2’s live in disjoint memory
  spaces.
- Use AP specific types
  This should allow future developments to enabled things like secure
  debug, memory tagging, etc
- Drop `ApInformation` and `MemoryApInformation`
  The AP specific types carry these information internally so these types
  are no longer required.
@ithinuel
ithinuel force-pushed the add-adiv6-support-step-2 branch from 083c6b1 to 93fe5ed Compare July 31, 2024 07:25
@bugadani

Copy link
Copy Markdown
Contributor

@Tiwalun should I leave this PR unmerged for a short while, or you'll only have comments after 5 minutes after it gets merged? 🫠

@Tiwalun

Tiwalun commented Jul 31, 2024

Copy link
Copy Markdown
Member

I think my interpretation of a short while is different than yours 😄

Feel free to merge this, I don't think I have time to look at this before the weekend.

@bugadani
bugadani added this pull request to the merge queue Jul 31, 2024
Merged via the queue into probe-rs:master with commit 9eb1c2f Jul 31, 2024
@ithinuel
ithinuel deleted the add-adiv6-support-step-2 branch July 31, 2024 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants