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

Include specify blocks in cell libraries #386

Closed
wants to merge 1 commit into from
Closed

Include specify blocks in cell libraries #386

wants to merge 1 commit into from

Conversation

mole99
Copy link
Contributor

@mole99 mole99 commented Jul 17, 2023

This PR includes the specify blocks in all sky130 cell libraries.

  • enables dospecify to include the specify blocks in inc_verilog.py
    (dospecify was removed from fix_verilog.py because it doesn't do anything there)
  • fixes issues inside the specify blocks
    • different naming (e.g. RESETB_delayed -> RESET_B_delayed)
    • lower-case (e.g. AWAKE -> awake)
  • fixes issues inside the cells
    • some cells contained text fragments that definitely did not belong there
      (e.g. sky130_fd_sc_hs__a2bb2o on line 59 and 60)

The following cell libraries have been successfully read in with Icarus Verilog. All four combinations of USE_POWER_PINS defined/not-defined and FUNCTIONAL defined/not-defined were tested.

- `sky130_fd_io/verilog/sky130_ef_io.v`
- `sky130_fd_io/verilog/sky130_fd_io.v`
- `sky130_fd_sc_hd/verilog/primitives.v`
- `sky130_fd_sc_hd/verilog/sky130_fd_sc_hd.v`
- `sky130_fd_sc_hdll/verilog/primitives.v`
- `sky130_fd_sc_hdll/verilog/sky130_fd_sc_hdll.v`
- `sky130_fd_sc_hs/verilog/primitives.v`
- `sky130_fd_sc_hs/verilog/sky130_fd_sc_hs.v`
- `sky130_fd_sc_hvl/verilog/primitives.v`
- `sky130_fd_sc_hvl/verilog/sky130_fd_sc_hvl.v`
- `sky130_fd_sc_lp/verilog/primitives.v`
- `sky130_fd_sc_lp/verilog/sky130_fd_sc_lp.v`
- `sky130_fd_sc_ls/verilog/primitives.v`
- `sky130_fd_sc_ls/verilog/sky130_fd_sc_ls.v`
- `sky130_fd_sc_ms/verilog/primitives.v`
- `sky130_fd_sc_ms/verilog/sky130_fd_sc_ms.v`

Additionally, a simple simulation with IOPATH delays was run for sky130_fd_sc_hd.

Problem: SDF-File

Currently, the SDF file generated by OpenROAD annotates the drive-strength specific cells:

 (CELL
  (CELLTYPE "sky130_fd_sc_hd__inv_1")
  (INSTANCE _43_)
  (DELAY
   (ABSOLUTE
    (IOPATH A Y (0.059:0.059:0.059) (0.041:0.041:0.041))
   )
  )
 )

But this PR adds the specify paths to the base cells. This seems to be the right thing to do here, since the cells are structured in a way that all conditions, notifiers, delayed signals for the timing checks are in the base cell. Moving them to the drive-strength specific cells would mean a major rewrite of the cell libraries.

Therefore, the SDF file needs to be changed like so:

 (CELL
  (CELLTYPE "sky130_fd_sc_hd__inv")
  (INSTANCE _43_.base)
  (DELAY
   (ABSOLUTE
    (IOPATH A Y (0.059:0.059:0.059) (0.041:0.041:0.041))
   )
  )
 )

I will open an Issue with OpenROAD to ask ask if OpenSTA's write_sdf can be changed to output the SDF file in this format.

@RTimothyEdwards
Copy link
Owner

@mole99 : Wouldn't it be wrong to annotate the base cell, anyway? Because the SDF file is declaring the internal timing between input A and output Y, which is different for every strength variant.

I know it's a lot of work to rewrite the standard cell library repositories, but I can take care of that part.

Copy link
Owner

@RTimothyEdwards RTimothyEdwards left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@RTimothyEdwards
Copy link
Owner

Pulled and merged (to opencircuitdesign.com). Closing this PR now; comments about changing the underlying open PDK sources will be handled in a more sweeping change that is coming soon but is not going to be handled here.

@mole99
Copy link
Contributor Author

mole99 commented Jul 18, 2023

@RTimothyEdwards It is not wrong per se: The IOPATH delays are defined for each instance of the base cell and can therefore be of different values.

This is also necessary for when no base cells are used, since the different instances of the same cell type can have different delays. Here is a snippet of an SDF file generated by OpenROAD with two instances of the same cell type:

 (CELL
  (CELLTYPE "sky130_fd_sc_hd__and3_1")
  (INSTANCE _220_)
  (DELAY
   (ABSOLUTE
    (IOPATH A X (0.152:0.152:0.152) (0.142:0.142:0.142))
    (IOPATH B X (0.143:0.143:0.144) (0.159:0.161:0.163))
    (IOPATH C X (0.147:0.147:0.147) (0.177:0.177:0.177))
   )
  )
 )
 ...
 (CELL
  (CELLTYPE "sky130_fd_sc_hd__and3_1")
  (INSTANCE _225_)
  (DELAY
   (ABSOLUTE
    (IOPATH A X (0.124:0.124:0.124) (0.126:0.126:0.126))
    (IOPATH B X (0.149:0.150:0.152) (0.173:0.175:0.176))
    (IOPATH C X (0.148:0.148:0.148) (0.171:0.171:0.171))
   )
  )
 )

(I too would have thought that the same cell types would have the same delays, but it appears that these instances have different delays due to external circumstances.)


Nevertheless, I agree that the best solution is too rewrite the cell libraries, because that's what tools and people expect and can properly work with 👍️

RTimothyEdwards added a commit that referenced this pull request Jul 18, 2023
which modifies the inc_verilog.py script for sky130 to enable the
specify block, which should no longer be a syntax issue with
iverilog after the merge of Leo's pull request on the iverilog
code base from yesterday.  Also makes some additional syntax
fixes to the verilog sources from the sky130 PDK.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants