Skip to content

fix: add missing deps and sources in Bender.yml - #155

Merged
cfuguet merged 2 commits into
openhwfoundation:masterfrom
ricted98:fix/bender
Sep 12, 2026
Merged

cfuguet merged 2 commits into
openhwfoundation:masterfrom
ricted98:fix/bender

Conversation

@ricted98

Copy link
Copy Markdown
Contributor

No description provided.

@ricted98
ricted98 requested a review from cfuguet as a code owner September 11, 2026 12:31
@cfuguet

cfuguet commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Hi @ricted98,

Thanks for your proposal.

AXI-related modules wereintentionally kept out of the manifests (Flist and Bender.yaml) of this repository because they are not instantiated directly in the HPDcache. AXI-related modules are instantiated at the core or SoC level if AXI is the implemented NoC interface.

My feeling was then that those files should be referred to in the core's or SoC's manifests.

@cfuguet cfuguet added the fixed Waiting for validation label Sep 11, 2026
@ricted98

ricted98 commented Sep 11, 2026

Copy link
Copy Markdown
Contributor Author

Hi @cfuguet, thanks for the feedback.

I would actually argue in favor of adding them in this repo's manifest, as I see two potential pitfalls:

  1. Although the dependency on AXI is quite light (mostly axi_pkg), failing to pin a version here risks silent incompatibilities later at the SoC level. Explicitly declaring it in the manifest either prevents the conflict or makes it immediately obvious.
  2. In a fully Bender-based setup (without Git submodules), dependencies are cloned into a .bender directory with dynamically generated paths (I cannot recall the exact details on how they are chosen). This makes it difficult for the parent manifest to manually point to these files. Thus, the standard approach in the PULP ecosystem is for the provider repository to expose all potentially necessary files.

As a more elegant solution, what if we add a target to make their inclusion optional? For example, Bender would only include them if --target hpdcache_axi_adapters (or something similar) is passed at the SoC level. What do you think? Nevertheless, this means a project where the HPDCache is a nested dependency (e.g. from CVA6) requires including such target explicitly.

@cfuguet

cfuguet commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

@ricted98 I like your proposal of adding a specific target. I prefer this. If AXI is not used, no need to bring that dependency.

Let's make it shorter: what about --axi ?

Thanks !

@ricted98

Copy link
Copy Markdown
Contributor Author

Thanks! I added a commit showing how we can use the bender Target feature to conditionally include AXI related modules. This solution would require the user to add -t/--target hpdcache_axi to its invokation of bender on the top level project. I would opt for a "vendorized" target with hpdcache_* to avoid unwanted iteraction with other deps manifests. Let me know if this is aligned to what you imagined!

@cfuguet

cfuguet commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Yes, this looks good.

I did not know that targets passed on the top level project are global (propagated to all dependencies). In this case, it is indeed a good idea to prefix with hpdcache_.

Anyway, in the CVA6 project, as the most common case is to use the AXI interface of the HPDcache, we could use the pass_targets configuration option to pass the hpdcache_axi target to the HPDcache dependency in the cva6/bender.yml. This way, we do not need to add the -t hpdcache_axi option explicitly when calling bender from the CVA6 project. Right?

@ricted98

Copy link
Copy Markdown
Contributor Author

Yes indeed! I honestly forgot about that option, as it was likely added recently and I still use it rarely. It would nevertheless be a perfect fit for this case. I tried on my local clone of Cheshire + CVA6 (master_candidate) + HPDCache and it works.

@cfuguet
cfuguet merged commit a23eac2 into openhwfoundation:master Sep 12, 2026
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fixed Waiting for validation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants