openhwgroup / cva6

The CORE-V CVA6 is an Application class 6-stage RISC-V CPU capable of booting Linux
https://docs.openhwgroup.org/projects/cva6-user-manual/
Other
2.2k stars 670 forks source link

Multiple definitions of AXI parameters #1196

Closed cfuguet closed 1 month ago

cfuguet commented 1 year ago

Hello @JeanRochCoulon, @zarubaf

I know that this Pull Request is now merged and closed, but I think we have an issue here: we are keeping a redundancy in the definitions for the AXI interface.

In the modifications that @niwis introduced some months ago, AXI definitions are passed as parameters to the CVA6 module. In my humble opinion, this is good and should be the unique definition of AXI parameters. By the way, for completeness, I think that types for the B and R channels shall also be passed as parameters.

If we define AXI parameters in the different packages (CVA6 configuration file + ariane_axi_pkg + ariane_pkg) we have a potential misalignment in values between those passed as parameters and those in packages.

Moreover, I do not think that AXI definitions shall be part of CVA6 configuration packages. The core is one thing, the NoC interface is another. Currently, CVA6 cache subsystem supports two different interfaces: OpenPiton and AXI. And in the future, there could be others such as CHI, ACE, etc... In my opinion NoC interface depends on the system integration/configuration. The CVA6 configuration shall be used to define ISA extensions and microarchitectural parameters (e.g. scoreboard entries). Not SoC level parameters such as the NoC interface. If NoC parameters are defined in the CVA6 configuration file, we will see the number of configuration files grow in polynomial manner.

My two cents,

César

Originally posted by @cfuguet in https://github.com/openhwgroup/cva6/issues/1182#issuecomment-1512048752

zarubaf commented 1 year ago

Fully agree. We neeed to clean this up. How shall we go about it? Can you propose something as a PR?

cfuguet commented 1 year ago

Yes. I will make some modifications and propose a PR.

cfuguet commented 1 month ago

This issue is obsolete. CVA6 parametrization has evolved significantly