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

Ara integration in Cheshire (1) #112

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Draft

Ara integration in Cheshire (1) #112

wants to merge 9 commits into from

Conversation

mp-17
Copy link

@mp-17 mp-17 commented Mar 13, 2024

This is the first PR for Ara's support. See #160 for OS support. #160 is sufficiently stable to boot Linux with Ara.

Add bare-metal Ara to Cheshire.

  1. Add Ara dependency to Bender and bump CVA6.
  2. Integrate Ara in cheshire_soc. The current configuration can host a 2-lane Ara without performance degradations.
  3. Ara can be enabled or disabled with the corresponding configuration switch with a parameter. Two parameters configure the Ara instance (AraNrLanes to determine the number of vector lanes and VLEN to determine the bit width of a single vector register).
  4. The FPGA flow implements Cheshire without Ara.

@mp-17 mp-17 self-assigned this Mar 13, 2024
@MarekPikula
Copy link

Hi, I noticed that there are multiple Ara/Cheshire/MMU-related branches. I wanted to evaluate Ara with Linux, but for that to work, I suppose that it requires proper MMU support. Since Cheshire is supposed to run Linux, I wonder if this PR has Ara MMU support. I noticed that in Bender config, it refers to branches that don't include MMU patches from the mp/chs-ara branch.

Could you let me know if there is any working setup to make Ara work with Linux (with MMU support)? I can see that there are multiple branches (i.e., mp/chs-ara, rebase/mp/chs-ara, and the original @MaistoV fork), but it's not clear to me which of these is the most "current". Did you test it in simulation or in FPGA?

@mp-17
Copy link
Author

mp-17 commented May 17, 2024

Hello @MarekPikula,

We are internally working on a version of Ara that supports Linux on FPGA, which we plan to release in the next months gradually. The version that can now boot Linux is a bit messy and needs clean-up.
Nevertheless, if you are interested and not afraid of the mess, there is something on GitHub as well:

  1. Ara: https://github.com/mp-17/ara/tree/mp/chs-ara
  2. CVA6: https://github.com/mp-17/cva6/commits/ara_cheshire
  3. Cheshire: https://github.com/mp-17/cheshire_fork/tree/mp/chs-ara
  4. SDK: https://github.com/MaistoV/cva6-sdk_fork/tree/cheshire

As a note: this is not what you see in this PR.

@MarekPikula
Copy link

Nice, thank you for the pointers to the right places. I tested it in FireSim on an AWS F1 FPGA, and it seems to be working fine. I haven't run any extensive tests or benchmarks yet, only some small testing algorithms I had at hand, but man, it is exciting.

@mp-17 mp-17 force-pushed the mp/ara-pulpv1 branch 2 times, most recently from a814e99 to 5a3ce5d Compare July 3, 2024 16:18
Aquaticfuller pushed a commit that referenced this pull request Jul 16, 2024
@krabo0om
Copy link

Hi, @mp-17 thanks for the effort integrating ARA! How close is the current PR to be merged? Can we use this PR as-is on an FPGA with Linux?

@mp-17
Copy link
Author

mp-17 commented Aug 26, 2024

Hello @krabo0om,

I will soon rework this PR and write down how to use Ara on FPGA since we have reached a sufficiently stable version.

@MarekPikula
Copy link

Hi @mp-17, today I was trying to update my FireSim setup with the updated Ara (from the master branch) but stumbled upon the following synthesis error from Vivado:

[Synth 8-2105] illegal cast operation

For the following lines:

If I remove the vlen_t'(...) cast, it gets synthesized without error (or warning), but I guess it's not really a good idea to do this without understanding what exactly causes the error. I have the new VLEN parameter initialized, so I'm at a loss as to what could cause this error. Did you encounter anything similar during your work towards FPGA compatibility?

@mp-17
Copy link
Author

mp-17 commented Sep 16, 2024

Hello @MarekPikula,

can the problem be linked to the fact that vlen_t depends on VLEN, which we now pass as a parameter? (it was from an included package before)

It's an ugly fix, but if you include VLEN from a package, the error should go away. Let me know.

@fulcrum34
Copy link

Hello @mp-17
Following the information in cheshire's mp/chs-ara, I was able to build cheshire-ara for genesys2 but seems its too big for it.
I tried removing VGA, DMA, GPIO, VIO, ILA but without much gains.
Below is the utilization report for Genesys2 with default config.
Config NrLanes = 2
VLEN = 2048 (1024 per lane as in ara.mk)

Just want to ask, What am I missing here? Should disabling vector FPU be of any help?

Screenshot from 2024-10-07 18-04-56

@mp-17 mp-17 force-pushed the mp/ara-pulpv1 branch 4 times, most recently from d214d1d to 41dbd1b Compare October 10, 2024 13:44
@mp-17
Copy link
Author

mp-17 commented Oct 10, 2024

Hello @fulcrum34. We have only tried it on the VCU128, not yet on the Genesys2 board. You can try removing the FPUs and see if it fits.

@fulcrum34
Copy link

fulcrum34 commented Oct 11, 2024

Hello @fulcrum34. We have only tried it on the VCU128, not yet on the Genesys2 board. You can try removing the FPUs and see if it fits.

Hi, Thanks!
Yes I tried with FPUSupportNone and it fits easily but I have trouble getting it to boot linux or run baremetal.
I have also worked a version with FPU but unsupported single Lane config (pulp-platform/ara#75) to fit in.
Another thing I noticed, 200MHz clock for dram_wrapper does'nt meet timing. I used 100Mhz to be safe but I believe we can go a bit higher.

I can connect using Openocd. See openocd.log and gdb.log but the uart terminal despite correct port and baud prints ?????
The linux image does'nt boot nor does show up anything in the uart console.
Not sure what am I missing...
Once I reach a fix, I will release a PR. Hope it will help people out there with genesys2.
Good day!!! 🙂

@mp-17 mp-17 marked this pull request as ready for review October 11, 2024 13:22
@@ -22,7 +22,8 @@ dependencies:
clint: { git: "https://github.com/pulp-platform/clint.git", version: 0.2.0 }
common_cells: { git: "https://github.com/pulp-platform/common_cells.git", version: 1.33.0 }
common_verification: { git: "https://github.com/pulp-platform/common_verification.git", version: 0.2.0 }
cva6: { git: "https://github.com/pulp-platform/cva6.git", rev: pulp-v1.0.0 }
cva6: { git: "https://github.com/pulp-platform/cva6.git", rev: pulp-v1 }
Copy link
Collaborator

@paulsc96 paulsc96 Oct 11, 2024

Choose a reason for hiding this comment

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

It is better to make new release here; pointing to a moving branch head is not a good idea.

Copy link
Author

Choose a reason for hiding this comment

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

I have released a second PR that will have tagged commits on main branches.

Copy link
Collaborator

@paulsc96 paulsc96 left a comment

Choose a reason for hiding this comment

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

Thanks for this PR (sorry for the stray comment above).

Generally speaking, there is nothing to fundamentally object to, but some fundamental things must be done before merging:

  • Add the new feature and its parameters to the documentation
  • Add the new feature to some FPGA config and have it tested somehow in internal CI
  • Discuss how it constrains overall parameterization (we should do this offline).

@@ -22,7 +22,8 @@ dependencies:
clint: { git: "https://github.com/pulp-platform/clint.git", version: 0.2.0 }
common_cells: { git: "https://github.com/pulp-platform/common_cells.git", version: 1.33.0 }
common_verification: { git: "https://github.com/pulp-platform/common_verification.git", version: 0.2.0 }
cva6: { git: "https://github.com/pulp-platform/cva6.git", rev: pulp-v1.0.0 }
cva6: { git: "https://github.com/pulp-platform/cva6.git", rev: pulp-v1 }
ara: { git: "https://github.com/pulp-platform/ara.git", rev: mp/cva6-pulpv1/rebase }
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should also be merged and get a release before merging this PR.

Copy link
Author

Choose a reason for hiding this comment

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

I have released a second PR that will have tagged commits on main branches.

cheshire.mk Outdated Show resolved Hide resolved
hw/cheshire_pkg.sv Outdated Show resolved Hide resolved
RVC : 1,
RVH : 1,
RVH : ~cfg.Ara,
Copy link
Collaborator

Choose a reason for hiding this comment

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

If there are complex parameterization constraints affecting the rest of the system (which is okay generally), we need to talk these over offline and document them carefully.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, we can discuss this offline!

hw/cheshire_soc.sv Outdated Show resolved Hide resolved
hw/cheshire_soc.sv Outdated Show resolved Hide resolved
hw/cheshire_soc.sv Show resolved Hide resolved
@mp-17 mp-17 changed the title Ara integration in Cheshire Ara integration in Cheshire (1) Oct 15, 2024
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