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

noc_axi4_bridge fix #118

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

Conversation

NAP64
Copy link

@NAP64 NAP64 commented Oct 15, 2022

This PR fixes two issues in the noc-axi4 bridge:

  1. noc_axi4_bridge_ser skips the last NOC data package when the receiver is not ready.
  2. In noc_axi4_bridge_write, the non-cacheable AXI writes' data is not correctly shifted and the strb is not generated correctly.

@Jbalkind
Copy link
Collaborator

Thanks for the PR! I think some of these might have been fixed in @grigoriy-chirkov's other PR but it would be good to integrate them with a smaller PR. That said, I noticed your changes to noc_axi4_bridge_write no longer consider the uncacheable wire. I'm not sure that that's a correct assumption.

Also could you let us know your academic/industrial/other affiliation?

@NAP64
Copy link
Author

NAP64 commented Oct 17, 2022

If you look at the if block staring at line 202 in noc_axi4_bridge_write, offset is now being set within this if block, so uncacheable wire is still considered.

Yes, I'm from Virginia Tech CS.

@grigoriy-chirkov
Copy link
Contributor

Can you please explain why you do shifts in the other direction and how you calculate offset for non-cacheable writes?

@NAP64
Copy link
Author

NAP64 commented Oct 19, 2022

The 512-bit AXI data is created by gathering NOC packets toward MSB (noc_axi4_bridge_deser:181), therefore it needs to be shifted downward instead. The new strb now labels valid data received correctly.
The offset is calculated as if the valid part of data is gathered toward LSB first then shift upwards, which alignes with the LSB assumption in the original implementation.
Also, to align with the endianness of the Ariane CPU, less-than-8byte addressing needs to be handled in reverse. Therefore the data needs to be shifted upwards according to the lowest 3-bits in address. However, I have not tried unaligned reads and writes crossing 8-byte boundaries or a different CPU (with a different endianness), or any writing size between 8B and 64B.

@Jbalkind
Copy link
Collaborator

Ok I think this probably needs to be tested on the sparc core and/or with the other write sizes. I'd like for us to be more robust here but we need to make sure that it's not breaking other existing configurations at the same time.

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