dft: implementing interface to support different test modes #6533
Conversation
This is a refactoring change + public API change where we start supporting different tests modes. The test mode currently does nothing except for independendly architect and stitch each test mode. In the future, different tests modes may have different types of scan (ex: internal scan, scan compression, boundary scan, etc) and may support to include/exclude cells in certain modes. Signed-off-by: Felipe Garay <[email protected]>
Signed-off-by: Felipe Garay <[email protected]>
Signed-off-by: Felipe Garay <[email protected]>
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
| @@ -9,6 +9,8 @@ link_design sub_modules | |||
|
|
|||
| create_clock -name main_clock -period 2.0000 -waveform {0.0000 1.0000} [get_ports {clock}] | |||
|
|
|||
| set_dft_config -max_chains 1 | |||
There was a problem hiding this comment.
This has no effect on the ok file and doesn't relate to test modes - what is the purpose?
There was a problem hiding this comment.
Yes. The user should always set at leaast a max_length or a chain_count. I added now some validation logic to force this.
| ScanArchitectConfig* getMutableScanArchitectConfig(); | ||
| const ScanArchitectConfig& getScanArchitectConfig() const; |
There was a problem hiding this comment.
Is there a reason one is by pointer and the other by reference?
There was a problem hiding this comment.
This is to copy the protocol buffer's API were we explicitly need to use a mutable method to modify the objects:
There was a problem hiding this comment.
Yes but why the difference in & vs * ?
src/dft/src/config/TestModeConfig.hh
Outdated
| // Not copyable or movable. | ||
| TestModeConfig(const TestModeConfig&) = delete; | ||
| TestModeConfig& operator=(const TestModeConfig&) = delete; |
There was a problem hiding this comment.
You have no prevented moving, only copying.
There was a problem hiding this comment.
Fixed here and everywhere else. I created a macro to prevent me to duplicate the correct constructors in all of my classes.
| @@ -48,6 +48,8 @@ class ScanArchitectConfig | |||
| ClockMix // We architect the flops of different clock and edge together | |||
| }; | |||
|
|
|||
| ScanArchitectConfig() : clock_mixing_(ClockMixing::NoMix) {} | |||
There was a problem hiding this comment.
You can just do
ClockMixing clock_mixing_{NoMix};
and skip the ctor. Does this relate to test mode?
There was a problem hiding this comment.
This is related to the TestModeConfig as we need a default constructor if I am deleting the copy and move cosntructors.
Anyway, I changed this so we create a default ctor now.
| if (name == kDefaultTestModeName) { | ||
| return createTestMode(name); | ||
| } | ||
| return nullptr; |
There was a problem hiding this comment.
getOrDefaultMutableTestModeConfig sounds like it should return default if not found.
|
It would be nice to add a test with multiple test modes even if it doesn't do much yet. |
Also, make it clear that we only support one test mode for now Signed-off-by: Felipe Garay <[email protected]>
Signed-off-by: Felipe Garay <[email protected]>
Signed-off-by: Felipe Garay <[email protected]>
Signed-off-by: Felipe Garay <[email protected]>
Signed-off-by: Felipe Garay <[email protected]>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
src/dft/src/config/DftConfig.cpp
Outdated
| { | ||
| return scan_architect_config_; | ||
| if (test_modes_config_.size() >= 1) { |
There was a problem hiding this comment.
warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
| if (test_modes_config_.size() >= 1) { | |
| if (!test_modes_config_.empty()) { |
Additional context
/usr/include/c++/11/bits/unordered_map.h:304: method 'unordered_map'::empty() defined here
empty() const noexcept
^Signed-off-by: Felipe Garay <[email protected]>
There is not much to test now as I just wanted to finish the config hierarchy for other developers. Test modes need more work to be implements. For one we need to add new ports to select the current active mode, ways to include/exclude cells in each test mode and add muxes in the scan chain signals depending on what cells are part of what test mode. For sure more work than can be done in just one PR :) I added an error message notifying that we don't currently support more than one test mode. |
Signed-off-by: Felipe Garay <[email protected]>
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
This is a refactoring change + public API change where we start supporting different tests modes. The test mode currently does nothing except for independendly architect and stitch each test mode.
In the future, different tests modes may have different types of scan (ex: internal scan, scan compression, boundary scan, etc) and may support to include/exclude cells in certain modes.