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

Scan mode should not be synchronized in reset controller #218

Closed
nstewart-amd opened this issue Sep 14, 2023 · 5 comments · Fixed by #240
Closed

Scan mode should not be synchronized in reset controller #218

nstewart-amd opened this issue Sep 14, 2023 · 5 comments · Fixed by #240
Labels
bug Something isn't working

Comments

@nstewart-amd
Copy link
Contributor

nstewart-amd commented Sep 14, 2023

Scan mode override should be straight async control.
Current design has scan mode sychronization. However, the synchronizer will be scan replaced, defeating the purpose of the scan mode override.

File: src/rtl/caliptra/integration/rtl/caliptra_top.sv

always_ff @(posedge clk or negedge cptra_pwrgood) begin
        if (~cptra_pwrgood) begin
            cptra_scan_mode_Latched_d <= '0;
            cptra_scan_mode_Latched_f <= '0;
        end
        else begin
            cptra_scan_mode_Latched_d <= scan_mode;
            cptra_scan_mode_Latched_f <= cptra_scan_mode_Latched_d;
        end
    end

MicrosoftTeams-image

@Nitsirks
Copy link
Contributor

Review of this also exposed the lack of dft override for reset during scan mode.

For maximum scan coverage, we'll need to remove the synchronizer from this path as well as allow the cptra_rst_b pin to drive the uc and noncore resets during scan mode.

@Nitsirks Nitsirks added the bug Something isn't working label Sep 22, 2023
@nstewart-amd
Copy link
Contributor Author

Michael,

"src/soc_ifc/rtl/soc_ifc_top.sv"

The scan mode p logic won't really work.

  1. It will be scan replaced.

  2. If it were not scan replaced, it still needs syncronization.

Synchronization example:
Current:

531 // Generate a pulse to set the interrupt bit
532 always_ff @(posedge clk or negedge cptra_noncore_rst_b) begin
533     if (~cptra_noncore_rst_b) begin
534         scan_mode_f <= '0;
535     end
536     else begin
537         scan_mode_f <= scan_mode;
538     end
539 end
540
541 always_comb scan_mode_p = scan_mode & ~scan_mode_f;

Needed:

531 // Generate a pulse to set the interrupt bit
//synchronize scan mode
sync sync_inst (.clk(clk), .rstn(cptra_noncore_rst_b), .in(scan_mode), .out(scan_mode_sync));
//generate pulse after sync
532 always_ff @(posedge clk or negedge cptra_noncore_rst_b) begin
533     if (~cptra_noncore_rst_b) begin
534         scan_mode_f <= '0;
535     end
536     else begin
537         scan_mode_f <= scan_mode_sync;
538     end
539 end
540
541 always_comb scan_mode_p = scan_mode_sync & ~scan_mode_f;
  1. Recommend any scan mode toggle
    541 always_comb scan_mode_p = scan_mode_sync ^ scan_mode_f;

@nstewart-amd
Copy link
Contributor Author

@Nitsirks - We've re-run DFT-DRC checks.
The michnorris-msft-issue218 branch change resolved the previously reported issue.

However, the scan mode pulse concern remains.

@nstewart-amd
Copy link
Contributor Author

nstewart-amd commented Sep 25, 2023

I recommend that such scan mode detection logic be placed in a dedicated module/instance and it will need to be excluded from scan chain.
I suggest flops are named something like "xyz_n0scan". It's also helpful if the module is named "scan_mode_detect_n0scan".

The module should be a synchronizer + shift register. The pulse should reset the shift register. The shift register should be bitwise AND/OR and fan in to Caliptra reset for critical structures (through both scan and non-scan reset branches). It is assumed that the user must clock N cycles to clear the shift register before scan vectors can be successfully shifted through the scan chains. Further, I recommend that the pulse detector is looking for any change of state (XOR) or scan mode. This will force pre+post clear of the critical structures.

@Nitsirks Nitsirks closed this as completed Oct 5, 2023
@Nitsirks Nitsirks linked a pull request Oct 5, 2023 that will close this issue
@Nitsirks Nitsirks removed a link to a pull request Oct 5, 2023
@Nitsirks Nitsirks linked a pull request Oct 5, 2023 that will close this issue
@calebofearth
Copy link
Collaborator

A follow up to explain why synchronizers were not added

A synchronizer is not necessary because the scan_mode must be synchronous to Caliptra's clock, as described in Caliptra Integration Specification, Table 11.

This table also stipulates that the signal Must be set before entering scan mode., so there is no concern about the operation of a scan operation preventing this value from being detected and operated upon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants