Skip to content

Conversation

a-will
Copy link
Contributor

@a-will a-will commented Jun 7, 2024

Add back prim_pkg enum for legacy support, and mention that it is deprecated. Some IPs reference the enums directly.

@a-will a-will force-pushed the fusesoc-compat branch 2 times, most recently from 65eb0ec to 19af483 Compare June 7, 2024 07:04
@marnovandermaas marnovandermaas added the Type:Enhancement Feature requests, enhancements label Jun 7, 2024
@a-will a-will force-pushed the fusesoc-compat branch 2 times, most recently from 458c9e4 to 4009fe6 Compare June 9, 2024 23:45
@a-will
Copy link
Contributor Author

a-will commented Jun 9, 2024

Interesting, it looks like mixing with a newer fusesoc might require at least Python 3.9 to work with the other dependencies.

ERROR: Ignored the following versions that require a different python version: 1.0.0 Requires-Python >=3.9,<4.0; 1.1.0 Requires-Python >=3.9
ERROR: Could not find a version that satisfies the requirement jsonschema2md==1.1.0 (from versions: 0.1.0, 0.1.1, 0.2.0.post1, 0.2.1, 0.3.0, 0.4.0, 0.9.0)
ERROR: No matching distribution found for jsonschema2md==1.1.0

@a-will a-will force-pushed the fusesoc-compat branch 2 times, most recently from e7dd88c to b2b9aab Compare June 10, 2024 06:02
@a-will
Copy link
Contributor Author

a-will commented Jun 10, 2024

Newer fusesoc also has a different directory layout compared to the OT fork. This was the OT fork's layout:

  • {--build-root}/
    • sim-vcs/
      • fusesoc-created metadata, Makefile, and file list
    • src/
      • Exported source files

But newer fusesoc has this directory layout:

  • {--work-root}/
    • Contains fusesoc-created metadata, Makefile, and file list
    • src/
      • Exported source files

Accommodating the new tree needed fixes to various paths, including CFLAGS definitions in hjson files. In the current example, {work-root} is set to dvsim's {build_dir}/fusesoc-work.

In addition, for ralgen, the position field was needed to be set to prepend for generated cores, else the files would come after the *_env_pkg.sv that would depend on them.

Now, some simulations work, but there is still a gap...

@a-will a-will force-pushed the fusesoc-compat branch 4 times, most recently from 787aca8 to 5344016 Compare June 17, 2024 04:34
@a-will
Copy link
Contributor Author

a-will commented Oct 31, 2024

This PR is way out of date now, but since @olofk has released fusesoc 2.4, I think we don't need my random development tag anymore. 2.4 should be able to handle virtual cores as needed.

@olofk
Copy link
Contributor

olofk commented Oct 31, 2024

@a-will I think that depends on the OT requirements. If you are fine with pulling in cores from one vendor during a build, then virtual cores should do the trick.

However, as I understand it, OT wants to be able to select implementation at compile-time, and in that case you still need something like primgen.

With that said, I believe FuseSoC 2.4 should have all the functionality needed to implement what you need. I have done some experimenting implementing the equivalent to primgen as a FuseSoC filter instead of a generator and it looks promising.

Happy to discuss this further.

@a-will
Copy link
Contributor Author

a-will commented Oct 31, 2024

@a-will I think that depends on the OT requirements. If you are fine with pulling in cores from one vendor during a build, then virtual cores should do the trick.

However, as I understand it, OT wants to be able to select implementation at compile-time, and in that case you still need something like primgen.

With that said, I believe FuseSoC 2.4 should have all the functionality needed to implement what you need. I have done some experimenting implementing the equivalent to primgen as a FuseSoC filter instead of a generator and it looks promising.

Happy to discuss this further.

It's unclear to me if OT actually needs that, though. All of our in-tree top-level fusesoc cores end up binding a specific prim library, and an out-of-tree integrator would almost certainly do the same for their actual chip. The build recipes for synthesis often can't be reused across technologies, and across integrations for the same opentitan IP, the top-level generally uses different RTL, so we end up with independent top-level cores anyway. We'd end up with different YAML for each real target, so sharing that core file across implementations doesn't seem to provide a benefit.

There may be a bit of a rub with gate-level simulation applications, though, especially if you just want to redo the existing block-level simulations with your own prim library. I'd guess that the full integration level still uses its own core file for GLS, but is doing block-level simulation with a different prim library a supported activity? I'm not sure.

We'd probably achieve the original setup's capabilities if only we could provide fusesoc (as a parameter to its invocation) which implementations of virtual cores to include for the specified top-level core file. For example, it could be arguments to provide additional VLNVs to add to the build (using their default target). Then the top-level core file wouldn't need to explicitly pull them in.

@a-will
Copy link
Contributor Author

a-will commented Nov 1, 2024

For example, it could be arguments to provide additional VLNVs to add to the build (using their default target). Then the top-level core file wouldn't need to explicitly pull them in.

@olofk If that bit above is interesting, the specifying of additional VLNVs would be akin to how hierarchical synthesis flows work. The top-level (and other sub-cores) may have dangling references, but the missing netlists would get specified as additional sources, then linked in during the build. For fusesoc, this would mean writing in a "depends" arrow from the top-level VLNV to the additional VLNVs, and the solver should handle them as though they were written in the core file Edit: Actually, this might be just additional cores thrown into the Requirement, with no need to modify the dependency graph.

@HU90m HU90m force-pushed the fusesoc-compat branch 2 times, most recently from 980f9e1 to 90f83e5 Compare November 25, 2024 18:19
@a-will
Copy link
Contributor Author

a-will commented Nov 26, 2024

Looks like fusesoc 2.4 isn't being used in the sim runs in CI, and something broke with the CI file changes. I guess the API is a bit different for github actions. Maybe s/parameters/inputs?

Copy link
Contributor

@marnovandermaas marnovandermaas left a comment

Choose a reason for hiding this comment

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

Amazing PR! Left a few comments from looking at the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are these CDC waivers tested in CI? If not, have we tested this ourselves?

@olofk
Copy link
Contributor

olofk commented Nov 26, 2024

Regarding paths, I haven't checked exactly what the problem is, but I would recommend using --work-root instead of --build-root since that gives you more deterministic behaviour. You might also want to use --system-name to get a fixed name for the build artifacts.

@a-will
Copy link
Contributor Author

a-will commented Nov 26, 2024

Regarding paths, I haven't checked exactly what the problem is, but I would recommend using --work-root instead of --build-root since that gives you more deterministic behaviour.

That is what I did in this PR: https://github.com/lowRISC/opentitan/pull/23555/files#diff-8ed9c1256f9282d31ab3a7f150e2e95eaae62a546151c7203117aaa70451129e

Comment on lines +16 to +18
sv_flist_gen_flags: ["--flag=fileset_{design_level}",
"--mapping=lowrisc:prim_generic:all:0.1"]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we're going to set a default mapping here, we should document how to override sv_flist_gen_flags + what is ordinarily set here. I feel iffy about putting prim selection in a generically-named property like this, though. It probably should be called out more specifically.

For dvsim, I think we should document the base configurations and explicitly require fusesoc as a dependency. If there were any hope of somehow abstracting away that dependency, that is gone now, hehe. Downstream user configuration files will explicitly have fusesoc-specific parameters in them.

This can all be done in future PRs.

use_cfgs: [{ name: mbx
fusesoc_core: lowrisc:ip:mbx
import_cfgs: ["{proj_root}/hw/lint/tools/dvsim/common_lint_cfg.hjson"]
additional_fusesoc_argument: "--mapping=lowrisc:systems:top_darjeeling:0.1"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

additional_fusesoc_argument sounds a bit hokey. In a future PR, we should probably get more specific about prims and make the property names more obviously related to the intended use.

Comment on lines +27 to +28
# files:
# - lint/physical_pads.waiver
Copy link
Contributor Author

Choose a reason for hiding this comment

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

nit: Should we simply remove these lines instead of commenting them out?

home_dir = "{}/homeless-shelter".format(out_dir)

cache_dir = "{}/fusesoc-cache".format(out_dir)
cache_dir = "/tmp/fusesoc-cache"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should create a TODO out of this and implement in a future PR. 😄

HU90m and others added 11 commits June 4, 2025 11:22
Because the prim hierarchy has changed, reusing the path would target
the flop directly for forcing. This meant that the forcing would delay
u_state_flop from snapping back to the FsmError state that should've
been held by the flop. Instead, the original path caused the flop in the
new hierarchy to be targeted *directly*, delaying update until the next
posedge, instead of the negedge when forcing is released.

Retarget forcing to prim_sparse_fsm_flop's state_o output.

Signed-off-by: Alexander Williams <[email protected]>
Some core old files were left after pulling otp out of the prims.

Signed-off-by: Hugo McNally <[email protected]>
These mappings will be used in a future commit. As of this commit, this
will be ignored by FuseSoC as an unknown property.

Signed-off-by: Hugo McNally <[email protected]>
…tants

These mappings will be used in a future commit. As of this commit, this
will be ignored by FuseSoC as an unknown property.

Signed-off-by: Hugo McNally <[email protected]>
This option will be used in future commits, but is ignored for now.

Signed-off-by: Hugo McNally <[email protected]>
This option will be used in future commits, but is ignored for now.

Signed-off-by: Hugo McNally <[email protected]>
In addition to changing to virtual prim cores, quite a few paths had to
be updated for FuseSoC's new build directory layout.

We are now on upstream FuseSoC to 2.4.3.

Note, prim_pkg still exists as prim_pkg_legacy because some RTL beyond
the old primitive wrappers depends on the implementation enum.
prim_pkg_legacy has been labeled as legacy.

Co-authored-by: Alexander Williams <[email protected]>
Signed-off-by: Hugo McNally <[email protected]>
We expect file names to match module names. Now that the IPs are virtual
cores, rename the files to match the module names that are the new "ABI"
(so to speak).

Adjust prim_generic, prim_xilinx, and prim_xilinx_ultrascale libraries.

Co-authored-by: Alexander Williams <[email protected]>
Signed-off-by: Hugo McNally <[email protected]>
HU90m and others added 3 commits June 4, 2025 12:08
The macros depend on ipgen outputs, so avoid pulling them in with the
ordinary prims.

Co-authored-by: Alexander Williams <[email protected]>
Signed-off-by: Hugo McNally <[email protected]>
Add the choice of prims to the fusesoc call instead.

Co-authored-by: Hugo McNally <[email protected]>
Signed-off-by: Alexander Williams <[email protected]>
@a-will
Copy link
Contributor Author

a-will commented Jun 4, 2025

It looks like comments have been addressed, either by making the change here or deferring to a future PR.

Tests were reported passing, and my changes since then have almost entirely been comments. The only new bit was creating a core for top_englishbreakfast_racl_pkg and using it in the englishbreakfast systems (instead of earlgrey's). The CW305 build passes.

The failure for Darjeeling DV is a temporary network failure and is unrelated to this PR.

Merging now!

@a-will a-will merged commit 2259612 into lowRISC:master Jun 4, 2025
33 of 37 checks passed
@a-will a-will deleted the fusesoc-compat branch June 4, 2025 20:40
@olofk
Copy link
Contributor

olofk commented Jun 5, 2025

Woohoo! Never thought I'd live to see the day :P Great work, everyone involved!

Now as for the FuseSoC-dvsim integration...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type:Enhancement Feature requests, enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[primgen/fusesoc] Generated primitive cores cannot depend on other generated primitive cores
10 participants