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

merge axi and l15 ports into noc port #1267

Conversation

JeanRochCoulon
Copy link
Contributor

Signed-off-by: Jean-Roch Coulon jean-roch.coulon@thalesgroup.com

Signed-off-by: Jean-Roch Coulon <jean-roch.coulon@thalesgroup.com>
Copy link
Member

@MikeOpenHWGroup MikeOpenHWGroup left a comment

Choose a reason for hiding this comment

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

LGTM @JeanRochCoulon. I am approving this PR, but please do not merge based only on my approval - I am just the verification geek and we should obtain an approval from more qualified team members.

@github-actions
Copy link
Contributor

❌ failed run, report available here.

@JeanRochCoulon
Copy link
Contributor Author

@Jbalkind As I have no way to test the modification in openpiton/ariane_verilog_wrap.sv, could you have a look and maybe try it? Thanks

Copy link
Contributor

@zarubaf zarubaf left a comment

Choose a reason for hiding this comment

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

looks very good!

@Jbalkind
Copy link
Contributor

I just cleaned up some syntax errors but the design will not build and pass tests. I will have to spend some time to dig into this and figure out what's broken. The core makes its first request and receives the response but then it gets stuck.

@jquevremont
Copy link
Contributor

CVA6 does not come with a NoC (i.e. a complex interconnect). I suggest to use the "socket" word instead to qualify this interface, as it can be used to plug CVA6 on a bus or a NoC.

.l15_req_o ( l15_req_o ),
.l15_rtrn_i ( l15_rtrn_i )
.l15_req_o ( noc_req_o ),
.l15_rtrn_i ( noc_rtrn_i )
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a typo - should be noc_resp_i instead of noc_rtrn_i. If you fix this, our simulation seems to be passing once again

@Jbalkind
Copy link
Contributor

Jbalkind commented Jul 2, 2023

I just made a PR to the ThalesSiliconSecurity/cva6 fork for fixes that make this PR work with openpiton. I don't think any of the fixes involve anything used outside of openpiton so it should be trivial to merge

@Jbalkind
Copy link
Contributor

Jbalkind commented Jul 2, 2023

CVA6 does not come with a NoC (i.e. a complex interconnect). I suggest to use the "socket" word instead to qualify this interface, as it can be used to plug CVA6 on a bus or a NoC.

We discussed this when we had the meeting proposing this change and agreed on noc. I'm not a huge fan of socket as it's used for other purposes in other cva6 users' projects (e.g. ESP, DECADES). Perhaps "memintf" or "l1intf" or similar?

@JeanRochCoulon JeanRochCoulon deleted the cvvdev/dev/noc branch July 21, 2023 15:47
@JeanRochCoulon JeanRochCoulon restored the cvvdev/dev/noc branch July 21, 2023 20:13
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.

5 participants