-
Notifications
You must be signed in to change notification settings - Fork 50
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
Ara integration in Cheshire (2) #160
base: mp/ara-pulpv1
Are you sure you want to change the base?
Conversation
Co-authored-by: Vincenzo Maisto <[email protected]> Co-authored-by: Matteo Perotti <[email protected]> Signed-off-by: Moritz Imfeld <[email protected]>
18cd941
to
ce46d3c
Compare
6d4a76f
to
858569a
Compare
858569a
to
dc2cbd1
Compare
6170122
to
dc79fa1
Compare
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 very clean as far as the diff in this repository.
The back-referencing setup however, while an intriguing construction, seems unnecessary. We can further discuss this offline, but:
- To avoid the hardware back-reference (Bender script inject):
- It is perfectly fine to unconditionally add Bender flags required by Ara to Cheshire; CVA6 does this too.
- If you need these flags to be configurable (
nr_lanes
,vlen
), it is best to introduce variables for them in the Cheshire make fragment (like those for the PLIC, CLINT etc.) and insert them in the build targets for the analyze scripts. This way, they can be overriden the same way as all other non-SV IP parameters. - If I missed something and you need to dynamically generate RTL (I don't think so), it would be great if you could follow the established method of creating an includable Make fragment in Ara to reconfigure its RTL from Cheshire as needed.
- For the device tree, each (FPGA) target may have multiple device trees; you can simply add one called
cheshire.vcu128_ara.dts
usingriscv,isa = "rv64imafdcv"
instead of patchingcheshire.dtsi
. - For Linux, I don't necessarily mind if an Ara-ready image cannot (yet) be built directly from Cheshire. How we build and manage Linux is about to fundamentally change anyways, so adding a back-reference for this now is perhaps untimely. We can address this in a cleaner way in a later PR.
`endif | ||
`ifdef ARA | ||
ret.Ara = 1; | ||
ret.AraVLEN = `ifdef VLEN `VLEN `else 0 `endif; |
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.
Having these as defines is fine, but perhaps they should be prefixed to clarify they affect Ara
This is the second PR for Ara's FPGA and OS support. See #112 for the bare-metal one.
Before merging, Ara and CVA6 should be referenced with TAGs laying on the
main
branch.