dft: stitch configuration, stitched scan chains in odb #6412
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
donn
force-pushed
the
dft_tweaks
branch
2 times, most recently
from
680b23b to
384887b
Compare
donn
force-pushed
the
dft_tweaks
branch
2 times, most recently
from
6af52bc to
a9ff57d
Compare
donn
force-pushed
the
dft_tweaks
branch
2 times, most recently
from
245cca5 to
c3c3a60
Compare
|
@maliberty When you have a chance… |
|
dft.one_cell_sky130.tcl failed in pr-merge. I recently updated that test so you might need to merge master and check it. |
|
@fgaray please review if you have a chance. |
donn
force-pushed
the
dft_tweaks
branch
2 times, most recently
from
999f541 to
487c2f7
Compare
|
lgtm but I'll wait a bit to merge in case @fgaray wants to comment. |
|
Checking now |
There was a problem hiding this comment.
Thanks for your contribution!
Some questions that I have:
- For the per chain enable, what is the use case?, Usually we have only one scan enable and the way we control different scan chains is by a test mode port. We don't have an implementation currently but I will be happy to help you design one if you want.
- writeScanChains: Do you have a list of other DFT tools that could consume this format? The standard for sharing scan data is the scan def formats.
- What are reset domains about? The scan chain only cares if data can be shifted in, that's why the clock is important so we know how to order them from falling edge -> rising edge. The reset does not play a role in shifting. I may be missing something here but happy to discuss.
Thanks!
src/dft/src/stitch/ScanStitch.cpp
Outdated
| // TODO: For now we will create a new scan_out pin at the top level if we need | ||
| // one. We need to support defining DFT signals for scan_out | ||
| const char* kind = "scan-out"; | ||
| auto term_info = split_term_identifier(with_name, kind, logger); |
There was a problem hiding this comment.
Try to minimize the usage of auto: https://abseil.io/tips/232
This changeset makes the DFT module a bit more flexible.
The highlights are new ports don't have to be punched- you can connect to any existing ITerm or BTerm and new ports are only punched as a fallback. Additionally, the scan chain order can be written in a JSON format so it may be consumed by other utilities.
---
* dft::Dft::insertDft: Add capability for per-chain enable, ability to provide runtime format strings for scan enable/in/out patterns
* dft::ScanStitch:
* if a name pattern includes an un-escaped forward slash (/), steps are to attempt to find an existing ITerm instead of creating a new BTerm
* else, if a BTerm already exists, the BTerm is reused.
* finally, a new BTerm is created.
* dft::Dft: create writeScanChains, which writes the scan chains (currently just names and ordering of flip-flops) in JSON format. Allows data to be passed to other DFT utilities.
* dft: create ResetDomain, which encapsulates reset domains similar to how dft::ClockDomain encapsulates clock domains. currently unused
---
There is one breaking change, which is that scan_enable_{}/scan_in_{}/ etc are numbered starting from 0 instead of 1. The rationale is the chains themselves are numbered starting from 0 and there appears to be no good justification to start them from 1. Otherwise, tests will (hopefully) show this PR as fully backwards-compatible.
Signed-off-by: Mohamed Gaber <[email protected]>
Signed-off-by: Mohamed Gaber <[email protected]>
Signed-off-by: Mohamed Gaber <[email protected]>
* created dft::ScanStitchConfig, to be part of dft::DftConfig, as a configuration object for dft::ScanStitch * moved relevant config variables there * made per-chain enable an enumeration with "enable mode" for more flexibility down the line Signed-off-by: Mohamed Gaber <[email protected]>
Signed-off-by: Mohamed Gaber <[email protected]>
Signed-off-by: Mohamed Gaber <[email protected]>
|
clang-tidy review says "All clean, LGTM! 👍" |
* dft::Dft::insertDft: stitched scan chains now committed into odb, so it can be stored in the def and accessed using the relevant APIs * dft::ScanChain: add setters and getters for scan in/out/enable drivers and loads * dft::ScanCell: add getter for scan in terminal * dft::ScanStitch: Now stores the scan in/out/enable drivers and loads in the scan chain via new setters * dft::ScanPin: Accept a sum type in the constructor, reorder sum type to match that of db.h for easier exporting * removed dft::Dft::writeScanChains: functionality can now be achieved using an Odb script of some kind Signed-off-by: Mohamed Gaber <[email protected]>
donn
changed the title
|
clang-tidy review says "All clean, LGTM! 👍" |
|
@fgaray I've removed writeScanChains and ResetDomain. Wrt per-chain enable, I'm going to be trying some weird DFT architectures with OpenROAD and would appreciate the flexibility. |
Signed-off-by: Mohamed Gaber <[email protected]>
|
clang-tidy review says "All clean, LGTM! 👍" |
|
@fgaray any further concerns? |
|
Give me today to take a look. Thanks! |
There was a problem hiding this comment.
Added a few more comments.
I think we should discuss a little bit about what do you have in mind as that will drive the design of what is being implemented here. I would like to be as flexible as possible but test modes are a bit tricky.
The simplest implementation that may not be too hard to work on, would be to have a map like <string, ModeConfig>. String in this case is the test mode name and ModeConfig is a class that holds ScanArchitectConfig + ScanStichConfig. This map will be inside DftConfig.
So, basically, something like
std::unordered_map<std::string, DftModeConfig> scan_modes_;
class DftModeConfig {
private:
ScanArchitectConfig scan_architect_config_;
ScanStitchConfig scan_stitch_config_;
};
Now, every time we try to architect and stitch, we iterate over the modes and pass the config. For example, for stitching:
for (const auto& [mode_name, mode_config]: dft_config.getSanModes()) {
ScanArchitect scan_architect(mode_config.getScanArchitectConfig());
Stitch stitch(mode_config.getScanStitchConfig()); // from my memory, it may require more arguments ;)
}
Now we need to talk about the API for this. I would propose something like:
create_test_mode -name "TEST_MODE_NAME" # new command
set_dft_config \
-mode "TEST_MODE_NAME" #new option
....etc
With this, we will make set_dft_config test mode aware.
The scan_enable_name_pattern and the rest LGTM. I would be happy to discuss more about my proposal to the TCL API.
Will this be something that makes sense with what you need?
misc. gardening to the abseil codex Signed-off-by: Mohamed Gaber <[email protected]>
|
@fgaray In the spirit of YAGNI, I've went ahead and removed per-chain enables entirely. Please re-review. Thanks. EDIT: I will say it's pretty weird and just violates POLA that the enable, of all signals, can only be 0. If a user wants them to all share enables, all they would have to do is opt in to it intuitively by setting the enable name pattern to something without braces in it. Which, for the record, we did do exactly that. What I was trying to do is an escape hatch to more predictable behavior while maintaining backcompat. But if this is gonna block this PR, then let's not. |
|
clang-tidy review says "All clean, LGTM! 👍" |
2 similar comments
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
This changeset makes the DFT module a bit more flexible.
The highlights are new ports don't have to be punched- you can connect to any existing ITerm or BTerm and new ports are only punched as a fallback. Additionally, the scan chain order can be written in a JSON format so it may be consumed by other utilities.
There is one breaking change, which is that scan_enable_{}/scan_in_{}/ etc are numbered starting from 0 instead of 1. The rationale is the chains themselves are numbered starting from 0 and there appears to be no good justification to start them from 1. Otherwise, tests will (hopefully) show this PR as fully backwards-compatible.