chipsalliance / caliptra-rtl

HW Design Collateral for Caliptra RoT IP
Apache License 2.0
71 stars 36 forks source link

Scan mode should not be synchronized in reset controller #218

Closed nstewart-amd closed 1 year ago

nstewart-amd commented 1 year ago

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 commented 1 year ago

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.

nstewart-amd commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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.

calebofearth commented 9 months ago

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.