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

coresight: discovery: Avoid double offset in root component creation #1717

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

konkers
Copy link

@konkers konkers commented Aug 7, 2024

Because the APAccessMemoryInterface includes and offset and the component factory API includes an address base, the base address offset at cpmid.address was being applied twice.

Because the APAccessMemoryInterface includes and offset and the component
factory API includes an address base, the base address offset at
cpmid.address was being applied twice.
@@ -262,7 +262,7 @@ def _create_root_component(self, cmpid):
memif = APAccessMemoryInterface(self.dp, ap_address)

# Instantiate the component and attach to the target.
component = cmpid.factory(memif, cmpid, cmpid.address)
component = cmpid.factory(memif, cmpid, 0)
Copy link
Member

Choose a reason for hiding this comment

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

I tracked down this parametter to be set to CoreSightComponent._address.

So if I understand correctly, this should be set to None instead.

I can see that the AP Address is used as an offset in APAccessMemoryInterface. Could that be the issue ?
Are all read/write operations to this root AP meant to be relative to the base address?

EG, hypothetically, if there’s an SCS right on the root AP, then there could be some read/write in the 0xExxx_xxxx space even if the root’s base address is in 0xFxxx_xxxx, couldn’t it?

Could be please point to where it is applied a second time?

Copy link
Author

Choose a reason for hiding this comment

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

I tracked down this parametter to be set to CoreSightComponent._address.

So if I understand correctly, this should be set to None instead.

None makes sense instead of 0 here.

I can see that the AP Address is used as an offset in APAccessMemoryInterface. Could that be the issue ? Are all read/write operations to this root AP meant to be relative to the base address?

EG, hypothetically, if there’s an SCS right on the root AP, then there could be some read/write in the 0xExxx_xxxx space even if the root’s base address is in 0xFxxx_xxxx, couldn’t it?

Could be please point to where it is applied a second time?

I was debugging an issue where a funnel was faulting on access: https://github.com/pyocd/pyOCD/blob/main/pyocd/coresight/funnel.py#L47

Since it's using the instantiated APAccessMemoryInterface which is applying cmpid.address as an offset to all accesses the self.address + TraceFunnel.DEVID results in a double offset.

I don't have enough of a high level view to understand which application of this makes sense. Perhaps instantiating APAccessMemoryIterface w/o an offset (or an offset derived from somewhere else ) makes sense?

Copy link
Author

Choose a reason for hiding this comment

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

Since it's today instead of yesterday I can say that the chip I was debugging was the rp2350 (see #1718). The error ends up being ignored and you can flash, erase, reset the rp2350 fine.

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.

2 participants