FPU integration in cv32e40p_wrapper

[pascalgouedo]

Signed-off-by: Pascal Gouedo [email protected]

[davideschiavone]

hi @pascalgouedo , thanks for the contribution!
I am wondering whether we should have 2 separate manifest files or only one. FuseSoc would generate two different depending on the parameters. what do you suggest @MikeOpenHWGroup, @Silabs-ArjanB ?

[MikeOpenHWGroup]

I am in favour of having a single manifest that could be used by all tools and handle all supported parameterization of the CV32E40P. I believe, but do not know for certain, that fusesoc can support this. Having said that, a decision to implement the CV32E40P manifest as a fusesoc core file is bigger than this pull-request.

[davideschiavone]

hi @MikeOpenHWGroup , I don't mean to do it with fusesoc, I mean whether we should have a manifest file with and one without the FPU files, so that people that do not use the FPU are not compiling it.

[pascalgouedo]

hi @MikeOpenHWGroup , I don't mean to do it with fusesoc, I mean whether we should have a manifest file with and one without the FPU files, so that people that do not use the FPU are not compiling it.

Hi @davideschiavone & @MikeOpenHWGroup
I will have a look to how to do that in core-v-verif makefiles.
Would mean that FPU presence information should be given to makeuvmt as a parameter.
Or maybe use new cfg files I created which contain fpu in their name ...

[Silabs-ArjanB]

Hi @davideschiavone I am in favor of 2 manifest files here because there are too many new files being included together with the FPU that are not needed otherwise and that are not conforming to our coding guidelines (e.g. use of ifdefs),

[pascalgouedo]

Hi @davideschiavone I am in favor of 2 manifest files here because there are too many new files being included together with the FPU that are not needed otherwise and that are not conforming to our coding guidelines (e.g. use of ifdefs),

Hi @Silabs-ArjanB
As cv32e40p_wrapper.sv becomes a new top level delivery file, I guess I need to remove those ifdef and replace them with generates ?
Or maybe the new e40p + fpu wrapper should not contain any verification stuff (tracers, ...) and use a renamed wrapper (verif_wrapper ?) to keep them apart from rtl in bhv:
e40p + fpu : rtl/cv32e40p_wrapper.sv
wrapper + tracers : bhv/cv32e40p_verif_wrapper.sv

[pascalgouedo]

Another point about manifests, I would be in favour of separating what is purely RTL design from what concerns verification environment.
That would mean that e40p rtl manifest would not have any +incdir (as packages are not include files I would move them from include to rtl as well) which are generally a mess for some implementation tools.
And then maybe another manifest (in either e40p repo or core-v-verif one) for tb/verification stuff.

[Silabs-ArjanB]

As cv32e40p_wrapper.sv becomes a new top level delivery file, I guess I need to remove those ifdef and replace them with generates ?

My remark was about all the ifdefs in actual RTL files within the vendor directory.

Another point about manifests, I would be in favour of separating what is purely RTL design from what concerns verification environment.

Yes, the split between RTL and non-RTL needs to be 100% clear. So far all RTL was in a directory called 'rtl' (or its subdirectories. Stuff meant for verification was in 'bhv'. We care a bit less about the use of ifdefs in the 'bhv' directory, but within the 'rtl' directory it is a no-go for us. I did not check the 'vendor' directory to see if it has a clean split between RTL and non-RTL.

[pascalgouedo]

• There are ifdef in cv32e40p_wrapper.sv so I will split wrapper in rtl/cv32e40p_wrapper.sv and in bhv/cv32e40p_wrapper_for_tb.sv
• In vendor files effectively used for FPU, there are some ifdef in pulp_platform_common_cells/src/cf_math_pkg.sv and pulp_platform_common_cells/src/rr_arb_tree.sv related to VERILATOR.
  If we change that, I don't know how we will manage consistency with cvfpu?

[davideschiavone]

why did we delete the tracer_pkg?

[pascalgouedo]

I didn't remove it, just moved it at the bottom before tb_wrapper to have clear separation between CV32 IP and behavioural/test-bench stuff.

[pascalgouedo]

Another point about manifests, I would be in favour of separating what is purely RTL design from what concerns verification environment.
That would mean that e40p rtl manifest would not have any +incdir (as packages are not include files I would move them from include to rtl as well) which are generally a mess for some implementation tools.
And then maybe another manifest (in either e40p repo or core-v-verif one) for tb/verification stuff.

[MikeOpenHWGroup]

Another point about manifests, I would be in favour of separating what is purely RTL design from what concerns verification environment.

That is a good idea @pascalgouedo. Please create an issue on this repo and assign it to @Silabs-ArjanB and @davideschiavone as I would also be interested in their opinions.

[Silabs-ArjanB]

Another point about manifests, I would be in favour of separating what is purely RTL design from what concerns verification environment.

That is a good idea @pascalgouedo. Please create an issue on this repo and assign it to @Silabs-ArjanB and @davideschiavone as I would also be interested in their opinions.

Moving files around is just going to cause work to us, so from our side we do not see a need for it.

[pascalgouedo]

Moving files around is just going to cause work to us, so from our side we do not see a need for it.

Ok let it as it is