vendor.openlane: OpenLANE ASIC Platform #644
Conversation
nmigen/vendor/openlane.py
Outdated
|
|
||
| toolchain = "OpenLANE" | ||
|
|
||
| _INVK_DIR = os.getcwd() |
There was a problem hiding this comment.
You definitely should not call os.getcwd in toplevel code (though I don't think this entire part of the platform should exist, see below).
nmigen/vendor/openlane.py
Outdated
| ] | ||
|
|
||
| def __init__(self): | ||
| super().__init__() |
There was a problem hiding this comment.
This isn't doing anything useful.
nmigen/vendor/openlane.py
Outdated
| super().__init__() | ||
|
|
||
| @property | ||
| def default_clk_frequency(self): |
There was a problem hiding this comment.
Platforms should not set default_clk_frequency. That's only for boards, in case they have an oscillator.
There was a problem hiding this comment.
This is a temporary stop-gap to ensure that designs synth until the proper clock constraints have been gotten working, it's not intended to stay
nmigen/vendor/openlane.py
Outdated
| set ::env(CLOCK_NET) $::env(CLOCK_PORT) | ||
| {% else %} | ||
| # Disable the clock | ||
| set ::env(CLOCK_TREE_SYNTH) 0 |
There was a problem hiding this comment.
Missing a default clock doesn't mean missing every clock, so disabling clock tree synthesis if you don't have a default clock seems wrong. Indeed, in the SDC file generator, you do create clock constraints even if there's no default clock defined.
There was a problem hiding this comment.
Correct, this does need to be fixed, along with the rest of the clock constraints
nmigen/vendor/openlane.py
Outdated
| _UID = os.getuid() | ||
| _GID = os.getgid() | ||
|
|
||
| openlane_root = abstractproperty() |
There was a problem hiding this comment.
This shouldn't be a platform property, but an environment variable set in NMIGEN_ENV_OpenLANE.
nmigen/vendor/openlane.py
Outdated
|
|
||
| command_templates = [ | ||
| r""" | ||
| {{invoke_tool("docker")}} |
There was a problem hiding this comment.
I'm not sure if nMigen should be invoking Docker? According to the documentation, that's not the only way to run OpenLANE, and if you hardcode Docker into the build script, anyone not using Docker will have issues.
Invoking Docker here also makes the platform a lot more complicated than it should be, with things like platform._UID and platform._INVK_DIR that really shouldn't be a a part of it. It also breaks remote builds, since the UID, GID, and CWD will not match.
There was a problem hiding this comment.
I agree that invoking docker does make things more complicated, however as I understood it that is the most "clean" way to go about running the flows.
When i have some time i'll look into the others ways to invoke the flow and have a poke about with it,
There was a problem hiding this comment.
FYI, I did it a bit differently for the Edalize OpenLane backend and tried to disentangle the docker parts in a more structured manner. Happy to go into more detail if you're interested. Of course, you could also just pick out the relevant parts from that
nmigen/vendor/openlane.py
Outdated
| def default_clk_frequency(self): | ||
| return 4e6 | ||
|
|
||
| # This was lifted directly from the TemplatedPlatform.toolchain_prepare because I needed to tweak it a bit |
There was a problem hiding this comment.
What does "tweak it a bit" mean, exactly? I don't like seeing the entire thing be duplicated, and since you don't describe what you needed that the TemplatedPlatform.toolchain_prepare function doesn't provide, we can't discuss a better solution.
nmigen/vendor/openlane.py
Outdated
| plan.add_file(filename, content) | ||
| return plan | ||
|
|
||
| def create_missing_domain(self, ports): |
There was a problem hiding this comment.
The semantics of this function doesn't match the way default clock/reset works in nMigen. In other platforms, if there's no clock or reset defined, they aren't created at all. This platform should follow the suit.
Codecov Report
@@ Coverage Diff @@
## master #644 +/- ##
==========================================
- Coverage 81.44% 81.33% -0.12%
==========================================
Files 49 49
Lines 6452 6449 -3
Branches 1349 1286 -63
==========================================
- Hits 5255 5245 -10
- Misses 1003 1013 +10
+ Partials 194 191 -3
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## main #644 +/- ##
=======================================
Coverage 81.22% 81.22%
=======================================
Files 49 49
Lines 6467 6467
Branches 1526 1526
=======================================
Hits 5253 5253
+ Misses 1021 1020 -1
- Partials 193 194 +1
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
…hey are spaces now
…ssue with the clock constraints file
…roperly and should now "mostly" work for creating the sync domain
…he Inverter example
…ner as well as the pdk path assumption
…t2` function by just extracting the logic we need from the function itself
…ote that this platform is explicitly for sky130 at the moment
… specialized platform and flow settings property
… remaining work needed to get full clock constraints and resources working
…w sees the net properly
… clock resource now works
…ild_dir` instead
…r `AMARANTH_ENV_*` env variables now
…e no longer so arcane
…ainerized run to the template
…d to override build and pull it out myself, id don't like it any more than you do
This PR adds initial support for running the OpenLANE ASIC flow with the sky130A PDK.
This PR is initially a draft as there are still some things to be worked out with clock constraints, documentation, and resources need to be done still, but starting the review process now as to fix any egregious issues before it's considered "done" is ideal, also to allow for feedback into how the platform is consumed and used.
There is a simple example in
examples/vendor/openlane_asic.pythat fully synth, and run through the ASIC flow, both a simple combinatorial design, as well as a simple synchronous design. @DX-MON has also successfully run their OpenPICle PIC16 core through the flow as well.Any and all feedback is appreciated~