pulp-platform / pulp_soc

pulp_soc is the core building component of PULP based SoCs
Other
78 stars 81 forks source link

Bender integration, sram update #45

Closed micprog closed 3 years ago

micprog commented 3 years ago

Added Bender.yml file and bump dependencies for compatibility to bender. Updated tech_cells_generic, changed deprecated generic_memory to tc_sram.

@bluewww @meggiman Please let me know if any other changes are required Replaces https://github.com/pulp-platform/pulp_soc/pull/33

micprog commented 3 years ago

Small update: in my most recent tests building pulpissimo with this pulp_soc, I encountered an issue with two different definitions of FLL_BUS due to updating apb_fll_if, now fixed with the latest commit.

micprog commented 3 years ago

Hi @meggiman @bluewww, With the most recent changes, hwpe-mac-engine and riscv should no longer point to my personal repository. I had some challenges with cv32e40p, as the repository is now named differently this may cause confusion, but this only refers to the directory name with IPApproX, and the repository link. Please let me know if any further changes are needed. I have not yet tested the bender build, as I am currently working with pulp, not pulpissimo, and I believe the integration there is not yet complete, but can look into it if desired.

meggiman commented 3 years ago

The commit id of Ibex in this PR seems to be invalid or the commit doesn't exist anymore. Could you please update this?

meggiman commented 3 years ago

Also I just noticed that there are many version conflicts when trying a bender checkout. If possible we should have the master branch in a state, where version conflicts do not have to be resolved manually or with an override file. This will probably require a lot of version bumping to align the repositories version requirements...

micprog commented 3 years ago

The commit id of Ibex in this PR seems to be invalid or the commit doesn't exist anymore. Could you please update this?

The pulpissimo branch in ibex has been moved to a newer version of ibex, referenced in #47. There are a few options to fix this, depends on what you would prefer.

  1. I can reference my personal repo for this commit until #47 is merged, which still has the commit (I could add a pulpissimo-v6.0.1 tag).
  2. Alternatively, ips_list.yml could reference the pulpissimo-v6.0.0, which is the version previously used, however does not include updates for bender and minor bugfixes.
  3. If the older version referenced here should originate from the lowRISC repository, the appropriate commits would need to be re-introduced, with tags (@vogelpi's help here would be needed).
  4. I could re-arrange the update to ibex in #47 such that it can be merged first, and an available version of ibex referenced in this PR.
meggiman commented 3 years ago

I personally would prefer option 4. Merging this PR should yield a working Bender flow which would not work with option 2.

micprog commented 3 years ago

Also I just noticed that there are many version conflicts when trying a bender checkout. If possible we should have the master branch in a state, where version conflicts do not have to be resolved manually or with an override file. This will probably require a lot of version bumping to align the repositories version requirements...

While behavior would definitely be improved, these updates are not always that easy. For specifics:

I will start with these updates, but strongly suggest the use of a Bender.local file for overrides in the main pulpissimo and pulp repositories, as certain mismatches seem to be prevalent in pulp.

micprog commented 3 years ago

I personally would prefer option 4. Merging this PR should yield a working Bender flow which would not work with option 2.

Done, I will update the dependencies in this PR once #47 is merged

vogelpi commented 3 years ago

Thanks for reworking #47 @micprog this is the cleanest solution.

bluewww commented 3 years ago

47 is merged

bluewww commented 3 years ago

Can you also add the override file to this pr you use to the bender checkout? This would allow users to get a working state of this project withouth having to tell bender which ip version to use. I believe this is a good short term solution.

meggiman commented 3 years ago

@micprog Regarding the version alignment issues of all the dependencies I can see the dificulty, although using an override file kind of defeats the purpose of Benders semantic versioning resolution. In order to finally bring bender support to pulp_soc without further delays I would thus suggest that we temporarily add a bender.local file with the necessary overrides and later on remove it step by step once the other IPs become aligned. Otherwise requiring less experienced pulp user to decide which version to check out is a fool proof way to completely overwhelm ourselves with support requests once we point to the newest pulp_soc version in PULPissimo and pulp-open. What do you think?

micprog commented 3 years ago

@bluewww @meggiman Based on some quick tests, the bender config file used for overrides can currently only be placed in the top-level repository, i.e. the PULPissimo or pulp-open repositories, not this repo. This would also make sense to keep all overrides in one central location. I agree that users should not be required to choose dependency versions, unless they are working on dependency updates, so integrating Bender to the top repositories will require adding a config file to those PRs, but unfortunately adding one here will not make a difference. Is this an acceptable approach?

bluewww commented 3 years ago

Ok thats fine by me.

Do you want me to put a rev tag onto the cv32e40p branch?

micprog commented 3 years ago

Do you want me to put a rev tag onto the cv32e40p branch?

Would probably be cleanest, just know that dependency resolution will not work the same as with version tags. The benefits are limited to persistence within the repository, and legibility, which I think both is worth it.

bluewww commented 3 years ago

Ok I created a tag.