dft: per-chain enables, user-configurable name patterns #6412

donn · 2024-12-22T18:08:50Z

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.

github-actions

clang-tidy made some suggestions

src/dft/src/Dft.cpp

src/dft/src/stitch/ScanStitch.cpp

src/dft/src/stitch/ScanStitch.hh

src/dft/src/utils/Utils.cpp

donn · 2025-01-01T12:10:02Z

@maliberty When you have a chance…

maliberty · 2025-01-01T16:11:03Z

dft.one_cell_sky130.tcl failed in pr-merge. I recently updated that test so you might need to merge master and check it.

maliberty · 2025-01-01T16:13:09Z

@fgaray please review if you have a chance.

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]>

maliberty · 2025-01-01T23:54:16Z

lgtm but I'll wait a bit to merge in case @fgaray wants to comment.

fgaray · 2025-01-03T20:59:04Z

Checking now

fgaray

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!

fgaray · 2025-01-03T23:36:08Z

src/dft/src/reset_domain/ResetDomain.hh

+  High = 1,  // active high
+};
+
+class ResetDomain


Could you please add a comment explaining what the reset domain is about and why we need it?

fgaray · 2025-01-03T23:40:21Z

src/dft/src/Dft.cpp

+}
+
+void Dft::insertDft(bool per_chain_enable,
+                    const std::string& scan_enable_name_pattern,


Please, add those patterns arguments to DftConfig.hh. You can even create a new class for stitching options like "ScanStitchConfig" to hold them.

See https://abseil.io/tips/173

fgaray · 2025-01-03T23:47:20Z

src/dft/src/Dft.cpp

+      root.add_child(chain->getName(), current_chain);
+    }
+
+    boost::property_tree::write_json(json_file, root);


What is the use case for writing the scan chains into a json?

If you want to provide a command like that, I would suggest to store the created scan chains after insertDft into odb (see the test here: odb/test/cpp/scan/TestScanChain.cpp)

Later in your write json command, you can read that dbScanChain and write the information out, otherwise you will be running architect again on a potentially already scan stitched design.

fgaray · 2025-01-03T23:48:17Z

src/dft/src/architect/ScanChain.hh

@@ -85,6 +85,9 @@ class ScanChain
  // Returns the name of the scan chain
  const std::string& getName() const;

+  // Changes the name of the scan chain
+  void rename(std::string& new_name);


I don't see this being used anywhere. Please remove.

fgaray · 2025-01-03T23:56:17Z

src/dft/src/stitch/ScanStitch.cpp

 }

-ScanDriver ScanStitch::FindOrCreateScanEnable(odb::dbBlock* block)
+static std::pair<std::string, std::optional<std::string>> split_term_identifier(


Use an anonymous namespace instead of static: https://abseil.io/tips/186

Also, please rename this to be consistent with the rest of the file: SplitTermIdentitifer

fgaray · 2025-01-04T00:00:50Z

src/dft/src/stitch/ScanStitch.cpp

 {
-  // 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";


Declare a constant on this file to hold this. https://abseil.io/tips/140

Something like "constexpr std::string_view kKind = "scan-out"; " with a more appropriated name.

fgaray · 2025-01-04T00:01:28Z

src/dft/src/stitch/ScanStitch.cpp

-  // 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);


Try to minimize the usage of auto: https://abseil.io/tips/232

fgaray · 2025-01-04T00:03:36Z

src/dft/src/stitch/ScanStitch.hh

-  explicit ScanStitch(odb::dbDatabase* db);
+  explicit ScanStitch(odb::dbDatabase* db,
+                      bool per_chain_enable,
+                      std::string scan_enable_name_pattern = "scan_enable_{}",


If you create the ScanStitchConfig class, then you can add the default values there instead of adding them here.

Note that per_chain_enable could also be a config set in that ScanStitchConfig class

fgaray · 2025-01-04T00:04:35Z

src/dft/src/stitch/ScanStitch.hh


  // Stitch all the cells inside each one of the scan chains together.
-  void Stitch(const std::vector<std::unique_ptr<ScanChain>>& scan_chains);
+  void Stitch(const std::vector<std::unique_ptr<ScanChain>>& scan_chains,
+              utl::Logger* logger);


All of these member functions are receiving logger, I would suggest instead to pass it in the constructor and hold a pointer to it.

fgaray · 2025-01-04T00:06:11Z

src/dft/src/stitch/ScanStitch.hh

  void Stitch(odb::dbBlock* block,
              const ScanChain& scan_chain,
-              const ScanDriver& scan_enable);
+              utl::Logger* logger,
+              size_t ordinal = 1,


What is ordinal doing here? Please add a comment. Thanks!

donn force-pushed the dft_tweaks branch from f0f7065 to 3e2b84d Compare December 22, 2024 18:10

github-actions bot reviewed Dec 22, 2024

View reviewed changes

donn force-pushed the dft_tweaks branch 2 times, most recently from 680b23b to 384887b Compare December 23, 2024 14:38

donn marked this pull request as ready for review December 24, 2024 08:41

donn force-pushed the dft_tweaks branch 2 times, most recently from 6af52bc to a9ff57d Compare December 24, 2024 14:30

donn marked this pull request as draft December 24, 2024 14:53

donn force-pushed the dft_tweaks branch 2 times, most recently from 245cca5 to c3c3a60 Compare December 30, 2024 09:45

donn marked this pull request as ready for review December 30, 2024 09:46

donn added 2 commits January 1, 2025 21:30

dft: missing clang-format

760d770

Signed-off-by: Mohamed Gaber <[email protected]>

donn force-pushed the dft_tweaks branch from c3c3a60 to 999f541 Compare January 1, 2025 19:43

dft: fix test one_cell_sky130

487c2f7

Signed-off-by: Mohamed Gaber <[email protected]>

donn force-pushed the dft_tweaks branch from 999f541 to 487c2f7 Compare January 1, 2025 19:43

maliberty approved these changes Jan 1, 2025

View reviewed changes

fgaray reviewed Jan 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dft: per-chain enables, user-configurable name patterns #6412

dft: per-chain enables, user-configurable name patterns #6412

donn commented Dec 22, 2024 •

edited

Loading

github-actions bot left a comment

donn commented Jan 1, 2025

maliberty commented Jan 1, 2025

maliberty commented Jan 1, 2025

maliberty commented Jan 1, 2025

fgaray commented Jan 3, 2025

fgaray left a comment

fgaray Jan 3, 2025

fgaray Jan 3, 2025

fgaray Jan 3, 2025

fgaray Jan 3, 2025

fgaray Jan 3, 2025

fgaray Jan 3, 2025

fgaray Jan 4, 2025

fgaray Jan 4, 2025

fgaray Jan 4, 2025

fgaray Jan 4, 2025

fgaray Jan 4, 2025

fgaray Jan 4, 2025

dft: per-chain enables, user-configurable name patterns #6412

Are you sure you want to change the base?

dft: per-chain enables, user-configurable name patterns #6412

Conversation

donn commented Dec 22, 2024 • edited Loading

github-actions bot left a comment

Choose a reason for hiding this comment

donn commented Jan 1, 2025

maliberty commented Jan 1, 2025

maliberty commented Jan 1, 2025

maliberty commented Jan 1, 2025

fgaray commented Jan 3, 2025

fgaray left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donn commented Dec 22, 2024 •

edited

Loading