Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vendor.openlane: OpenLANE ASIC Platform #644

Closed
wants to merge 36 commits into from

Conversation

lethalbit
Copy link

@lethalbit lethalbit commented Oct 24, 2021

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.py that 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~

nmigen/back/rtlil.py Outdated Show resolved Hide resolved

toolchain = "OpenLANE"

_INVK_DIR = os.getcwd()
Copy link
Member

@whitequark whitequark Oct 25, 2021

Choose a reason for hiding this comment

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

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).

]

def __init__(self):
super().__init__()
Copy link
Member

@whitequark whitequark Oct 25, 2021

Choose a reason for hiding this comment

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

This isn't doing anything useful.

super().__init__()

@property
def default_clk_frequency(self):
Copy link
Member

@whitequark whitequark Oct 25, 2021

Choose a reason for hiding this comment

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

Platforms should not set default_clk_frequency. That's only for boards, in case they have an oscillator.

Copy link
Author

@lethalbit lethalbit Oct 25, 2021

Choose a reason for hiding this comment

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

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

set ::env(CLOCK_NET) $::env(CLOCK_PORT)
{% else %}
# Disable the clock
set ::env(CLOCK_TREE_SYNTH) 0
Copy link
Member

@whitequark whitequark Oct 25, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Author

@lethalbit lethalbit Oct 25, 2021

Choose a reason for hiding this comment

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

Correct, this does need to be fixed, along with the rest of the clock constraints

_UID = os.getuid()
_GID = os.getgid()

openlane_root = abstractproperty()
Copy link
Member

@whitequark whitequark Oct 25, 2021

Choose a reason for hiding this comment

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

This shouldn't be a platform property, but an environment variable set in NMIGEN_ENV_OpenLANE.


command_templates = [
r"""
{{invoke_tool("docker")}}
Copy link
Member

@whitequark whitequark Oct 25, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Author

@lethalbit lethalbit Oct 25, 2021

Choose a reason for hiding this comment

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

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,

Copy link

@olofk olofk Oct 25, 2021

Choose a reason for hiding this comment

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

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

def default_clk_frequency(self):
return 4e6

# This was lifted directly from the TemplatedPlatform.toolchain_prepare because I needed to tweak it a bit
Copy link
Member

@whitequark whitequark Oct 25, 2021

Choose a reason for hiding this comment

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

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.

plan.add_file(filename, content)
return plan

def create_missing_domain(self, ports):
Copy link
Member

@whitequark whitequark Oct 25, 2021

Choose a reason for hiding this comment

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

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
Copy link

codecov bot commented Oct 25, 2021

Codecov Report

Merging #644 (4b64bb6) into master (0b28a97) will decrease coverage by 0.11%.
The diff coverage is n/a.

Current head 4b64bb6 differs from pull request most recent head 84fcf5e. Consider uploading reports for the commit 84fcf5e to get more accurate results
Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
nmigen/tracer.py 89.18% <0.00%> (-5.41%) ⬇️
nmigen/_toolchain/cxx.py 94.73% <0.00%> (-5.27%) ⬇️
nmigen/_toolchain/yosys.py 61.01% <0.00%> (-1.70%) ⬇️
nmigen/hdl/ir.py 95.20% <0.00%> (-0.26%) ⬇️
nmigen/build/run.py 22.05% <0.00%> (ø)
nmigen/back/rtlil.py 80.62% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b28a97...84fcf5e. Read the comment docs.

@codecov-commenter
Copy link

codecov-commenter commented Dec 13, 2021

Codecov Report

Merging #644 (e87cbff) into main (9a5a614) will not change coverage.
The diff coverage is n/a.

@@           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     
Impacted Files Coverage Δ
amaranth/build/run.py 21.73% <0.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

lethalbit and others added 24 commits Apr 27, 2022
…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
…d to override build and pull it out myself, id don't like it any more than you do
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants