[flash_ctrl] Separate invalid and disabled states.
- fixes #11825
Part of the D2S design fixes was to unify the disabled behavior across
more of the flash controller state machines. However this created a
problem for RMA entry.
When RMA entry is done, the RMA fsm also requests flash operationst to be
disabled. Previosuly, this caused the RMA fsm itself to transition into
an invalid state. This caused 2 problems:
1. At the end of RMA, a fatal alert is generated. While this is
not bad behavior in any meaningful way, it is very inconsistent with alert
defintion because no fatal event has occurred. This leads to unnecessary
DV work.
2. Since the RMA fsm transitions out of its ack state, it means the ack
indication sent to life cycle controller will be held for only 1 cycle.
This is most definitely not enough for CDC and means RMA cannot be completed.
This commit separates the idea of "disabled" and "invalid" for most of the fsms.
Disabled simply means functionally disabled from future operations, while
invalid is both disabled AND alert generation.
At the end of RMA transition, the device is simply "disabled" and not "invalid".
Invalid then can only be caused by an explicit error, or the FSMs going
completely to an undefined state.
This makes the overall definition more consistent.
Signed-off-by: Timothy Chen <timothytim@google.com>
diff --git a/hw/ip/flash_ctrl/data/flash_ctrl_pkg.sv.tpl b/hw/ip/flash_ctrl/data/flash_ctrl_pkg.sv.tpl
index 0382672..1ff3826 100644
--- a/hw/ip/flash_ctrl/data/flash_ctrl_pkg.sv.tpl
+++ b/hw/ip/flash_ctrl/data/flash_ctrl_pkg.sv.tpl
@@ -586,17 +586,18 @@
// Minimum Hamming weight: 3
// Maximum Hamming weight: 6
//
- localparam int RmaStateWidth = 10;
+ localparam int RmaStateWidth = 11;
typedef enum logic [RmaStateWidth-1:0] {
- StRmaIdle = 10'b1101000011,
- StRmaPageSel = 10'b0010111001,
- StRmaErase = 10'b1111010100,
- StRmaEraseWait = 10'b0111010101,
- StRmaWordSel = 10'b0001011111,
- StRmaProgram = 10'b0110001110,
- StRmaProgramWait = 10'b1000110110,
- StRmaRdVerify = 10'b1011101010,
- StRmaInvalid = 10'b1100101101
+ StRmaIdle = 11'b11110001010,
+ StRmaPageSel = 11'b10111100111,
+ StRmaErase = 11'b11000010111,
+ StRmaEraseWait = 11'b01010100110,
+ StRmaWordSel = 11'b00010011001,
+ StRmaProgram = 11'b11011111101,
+ StRmaProgramWait = 11'b00111110000,
+ StRmaRdVerify = 11'b00101001100,
+ StRmaDisabled = 11'b01001011010,
+ StRmaInvalid = 11'b10100111011
} rma_state_e;
diff --git a/hw/ip/flash_ctrl/doc/_index.md b/hw/ip/flash_ctrl/doc/_index.md
index 9f592ea..dd90102 100644
--- a/hw/ip/flash_ctrl/doc/_index.md
+++ b/hw/ip/flash_ctrl/doc/_index.md
@@ -425,7 +425,7 @@
- The flash protocol controller [memory protection]({{< relref "#memory-protection" >}}) errors back all controller initiated operations.
- The host-facing tlul adapter errors back all host initiated operations.
- The flash physical controller completes any existing stateful operations (program or erase) and drops all future flash transactions.
-- The flash protocol controller arbiter completes any existing software issued commands and enters an invalid state where no new transactions can be issued.
+- The flash protocol controller arbiter completes any existing software issued commands and enters a disabled state where no new transactions can be issued.
### Flash Physical Controller
diff --git a/hw/ip/flash_ctrl/rtl/flash_ctrl_arb.sv b/hw/ip/flash_ctrl/rtl/flash_ctrl_arb.sv
index 8a4b1ce..8520238 100644
--- a/hw/ip/flash_ctrl/rtl/flash_ctrl_arb.sv
+++ b/hw/ip/flash_ctrl/rtl/flash_ctrl_arb.sv
@@ -118,11 +118,12 @@
//
localparam int StateWidth = 10;
typedef enum logic [StateWidth-1:0] {
- StReset = 10'b1010101011,
- StHw = 10'b1111010001,
- StSwActive = 10'b1011001100,
- StSwIdle = 10'b0101100111,
- StInvalid = 10'b0111111010
+ StReset = 10'b0011010110,
+ StHw = 10'b1111101110,
+ StSwActive = 10'b1100101001,
+ StSwIdle = 10'b1000000010,
+ StDisabled = 10'b0100010101,
+ StInvalid = 10'b0011001001
} arb_state_e;
flash_sel_e func_sel;
@@ -167,7 +168,7 @@
if (prim_mubi_pkg::mubi4_test_true_loose(disable_i)) begin
// Do not randomly switch unless idle as it may cause stateful operations to be
// disturbed
- state_d = StInvalid;
+ state_d = StDisabled;
end else if (hw_req_i) begin
// if hardware request comes in the middle, wipe fifos and enable
// switch to hardware interface
@@ -188,7 +189,13 @@
end
end
+ StDisabled: begin
+ state_d = StDisabled;
+ end
+
+
StInvalid: begin
+ state_d = StInvalid;
fsm_err_o = 1'b1;
end
diff --git a/hw/ip/flash_ctrl/rtl/flash_ctrl_lcmgr.sv b/hw/ip/flash_ctrl/rtl/flash_ctrl_lcmgr.sv
index c15f590..cdd09f8 100644
--- a/hw/ip/flash_ctrl/rtl/flash_ctrl_lcmgr.sv
+++ b/hw/ip/flash_ctrl/rtl/flash_ctrl_lcmgr.sv
@@ -5,7 +5,10 @@
// Flash Controller for life cycle / key management handling
//
-module flash_ctrl_lcmgr import flash_ctrl_pkg::*; #(
+module flash_ctrl_lcmgr
+ import flash_ctrl_pkg::*;
+ import lc_ctrl_pkg::lc_tx_t;
+#(
parameter flash_key_t RndCnstAddrKey = RndCnstAddrKeyDefault,
parameter flash_key_t RndCnstDataKey = RndCnstDataKeyDefault
) (
@@ -45,8 +48,8 @@
// external rma request
// This should be simplified to just multi-bit request and multi-bit response
- input lc_ctrl_pkg::lc_tx_t rma_req_i,
- output lc_ctrl_pkg::lc_tx_t rma_ack_o,
+ input lc_tx_t rma_req_i,
+ output lc_tx_t rma_ack_o,
// seeds to the outside world,
output logic [NumSeeds-1:0][SeedWidth-1:0] seeds_o,
@@ -79,12 +82,14 @@
input [BusWidth-1:0] rand_i,
// disable access to flash
- output lc_ctrl_pkg::lc_tx_t dis_access_o,
+ output lc_tx_t dis_access_o,
// init ongoing
output logic init_busy_o
);
+ import lc_ctrl_pkg::lc_tx_test_true_strict;
+
// total number of pages to be wiped during RMA entry
localparam int unsigned WipeIdxWidth = prim_util_pkg::vbits(WipeEntries);
localparam int unsigned MaxWipeEntry = WipeEntries - 1;
@@ -120,16 +125,17 @@
//
localparam int StateWidth = 11;
typedef enum logic [StateWidth-1:0] {
- StIdle = 11'b01010101111,
- StReqAddrKey = 11'b01001110011,
- StReqDataKey = 11'b11010000100,
- StReadSeeds = 11'b10001010101,
- StReadEval = 11'b11110110010,
- StWait = 11'b00111101010,
- StEntropyReseed = 11'b11101001000,
- StRmaWipe = 11'b00010011001,
- StRmaRsp = 11'b10100100001,
- StInvalid = 11'b10100011110
+ StIdle = 11'b10001000001,
+ StReqAddrKey = 11'b01110101100,
+ StReqDataKey = 11'b01110010001,
+ StReadSeeds = 11'b11011111110,
+ StReadEval = 11'b01000100111,
+ StWait = 11'b00100111011,
+ StEntropyReseed = 11'b00011000110,
+ StRmaWipe = 11'b10010110101,
+ StRmaRsp = 11'b10110001010,
+ StDisabled = 11'b11111100011,
+ StInvalid = 11'b11101011000
} lcmgr_state_e;
lcmgr_state_e state_q, state_d;
@@ -138,9 +144,9 @@
//SEC_CM: CTRL.FSM.SPARSE
`PRIM_FLOP_SPARSE_FSM(u_state_regs, state_d, state_q, lcmgr_state_e, StIdle)
- lc_ctrl_pkg::lc_tx_t err_sts_d, err_sts_q;
+ lc_tx_t err_sts_d, err_sts_q;
logic err_sts_set;
- lc_ctrl_pkg::lc_tx_t rma_ack_d, rma_ack_q;
+ lc_tx_t rma_ack_d, rma_ack_q;
logic validate_q, validate_d;
logic [SeedCntWidth-1:0] seed_cnt_q;
logic [SeedRdsWidth-1:0] addr_cnt_q;
@@ -297,7 +303,7 @@
RmaReqLast
} rma_req_idx_e;
- lc_ctrl_pkg::lc_tx_t [RmaReqLast-1:0] rma_req;
+ lc_tx_t [RmaReqLast-1:0] rma_req;
prim_lc_sync #(
.NumCopies(int'(RmaReqLast))
) u_sync_rma_req (
@@ -359,6 +365,10 @@
///////////////////////////////
// Hardware Interface FSM
///////////////////////////////
+ logic rma_done;
+ assign rma_done = lc_tx_test_true_strict(
+ lc_ctrl_pkg::lc_tx_and_hi(rma_req_i,rma_ack_d));
+
always_comb begin
// phases of the hardware interface
@@ -409,7 +419,7 @@
// Since init has not been called, there are no guarantees
// to entropy behavior, thus do not reseed
StIdle: begin
- if (rma_req[RmaReqInit] == lc_ctrl_pkg::On) begin
+ if (lc_tx_test_true_strict(rma_req[RmaReqInit])) begin
state_d = StRmaWipe;
end else if (init_q) begin
state_d = StReqAddrKey;
@@ -419,7 +429,7 @@
StReqAddrKey: begin
phase = PhaseSeed;
addr_key_req_d = 1'b1;
- if (rma_req[RmaReqKey] == lc_ctrl_pkg::On) begin
+ if (lc_tx_test_true_strict(rma_req[RmaReqKey])) begin
state_d = StRmaWipe;
end else if (addr_key_ack_q) begin
state_d = StReqDataKey;
@@ -429,7 +439,7 @@
StReqDataKey: begin
phase = PhaseSeed;
data_key_req_d = 1'b1;
- if (rma_req[RmaReqKey] == lc_ctrl_pkg::On) begin
+ if (lc_tx_test_true_strict(rma_req[RmaReqKey])) begin
state_d = StRmaWipe;
end else if (data_key_ack_q) begin
// provision_en is only a "good" value after otp/lc initialization
@@ -474,7 +484,7 @@
// Waiting for an rma entry command
StWait: begin
rd_buf_en_o = 1'b1;
- if (rma_req[RmaReqWait] == lc_ctrl_pkg::On) begin
+ if (lc_tx_test_true_strict(rma_req[RmaReqWait])) begin
state_d = StEntropyReseed;
end
end
@@ -516,6 +526,14 @@
end
end
+ // Disabled state is functionally equivalent to invalid, just without the
+ // the explicit error-ing
+ StDisabled: begin
+ dis_access_o = lc_ctrl_pkg::On;
+ rma_ack_d = lc_ctrl_pkg::Off;
+ state_d = StDisabled;
+ end
+
StInvalid: begin
dis_access_o = lc_ctrl_pkg::On;
state_err = 1'b1;
@@ -523,6 +541,7 @@
// https://github.com/lowRISC/opentitan/issues/10204
//phase = PhaseInvalid;
rma_ack_d = lc_ctrl_pkg::Off;
+ state_d = StInvalid;
end
// Invalid catch-all state
@@ -533,14 +552,23 @@
endcase // unique case (state_q)
- // this fsm does not directly interface with flash so can be
- // be transitioned to invalid immediately
- if (prim_mubi_pkg::mubi4_test_true_loose(disable_i)) begin
- state_d = StInvalid;
+ // This fsm does not directly interface with flash so can be
+ // be transitioned to invalid immediately.
+ // If rma transition is successful however, do not transition
+ // and continue acking the life cycle controller, as disable is
+ // expected behavior under this situation.
+ if (prim_mubi_pkg::mubi4_test_true_loose(disable_i) &&
+ state_d != StInvalid &&
+ !rma_done) begin
+ state_d = StDisabled;
end
end // always_comb
+ // if disable is seen any state other than StRmaRsp, transition to invalid state
+ `ASSERT(Disable_Invalid_A, prim_mubi_pkg::mubi4_test_true_loose(disable_i) & state_q != StRmaRsp
+ |=> state_q == StInvalid)
+
///////////////////////////////
// RMA wiping Mechanism
///////////////////////////////
@@ -668,7 +696,7 @@
// On - no errors
// Off - errors were observed
logic [lc_ctrl_pkg::TxWidth-1:0] err_sts_raw_q;
- assign err_sts_q = lc_ctrl_pkg::lc_tx_t'(err_sts_raw_q);
+ assign err_sts_q = lc_tx_t'(err_sts_raw_q);
assign err_sts_d = err_sts_set && (err_sts_q != lc_ctrl_pkg::Off) ? lc_ctrl_pkg::Off : err_sts_q;
// This primitive is used to place a size-only constraint on the flops in order to prevent
// optimizations. Without this Vivado may infer combo loops. For details, see
@@ -738,7 +766,7 @@
// and that is considered an extremely invasive attack.
StRmaIdle: begin
if (prim_mubi_pkg::mubi4_test_true_loose(disable_i)) begin
- rma_state_d = StRmaInvalid;
+ rma_state_d = StRmaDisabled;
end else if (rma_wipe_req_int) begin
rma_state_d = StRmaPageSel;
page_cnt_ld = 1'b1;
@@ -747,7 +775,7 @@
StRmaPageSel: begin
if (prim_mubi_pkg::mubi4_test_true_loose(disable_i)) begin
- rma_state_d = StRmaInvalid;
+ rma_state_d = StRmaDisabled;
end else if (page_cnt < end_page) begin
rma_state_d = StRmaErase;
end else begin
@@ -773,7 +801,7 @@
StRmaWordSel: begin
if (prim_mubi_pkg::mubi4_test_true_loose(disable_i)) begin
- rma_state_d = StRmaInvalid;
+ rma_state_d = StRmaDisabled;
end else if (word_cnt < BusWordsPerPage) begin
rma_state_d = StRmaProgram;
end else begin
@@ -820,7 +848,12 @@
end
end
+ StRmaDisabled: begin
+ rma_state_d = StRmaDisabled;
+ end
+
StRmaInvalid: begin
+ rma_state_d = StRmaInvalid;
err_sts_set = 1'b1;
fsm_err = 1'b1;
end
diff --git a/hw/ip/flash_ctrl/rtl/flash_ctrl_pkg.sv b/hw/ip/flash_ctrl/rtl/flash_ctrl_pkg.sv
index c364dd5..9a9f9f8 100644
--- a/hw/ip/flash_ctrl/rtl/flash_ctrl_pkg.sv
+++ b/hw/ip/flash_ctrl/rtl/flash_ctrl_pkg.sv
@@ -585,17 +585,18 @@
// Minimum Hamming weight: 3
// Maximum Hamming weight: 6
//
- localparam int RmaStateWidth = 10;
+ localparam int RmaStateWidth = 11;
typedef enum logic [RmaStateWidth-1:0] {
- StRmaIdle = 10'b1101000011,
- StRmaPageSel = 10'b0010111001,
- StRmaErase = 10'b1111010100,
- StRmaEraseWait = 10'b0111010101,
- StRmaWordSel = 10'b0001011111,
- StRmaProgram = 10'b0110001110,
- StRmaProgramWait = 10'b1000110110,
- StRmaRdVerify = 10'b1011101010,
- StRmaInvalid = 10'b1100101101
+ StRmaIdle = 11'b11110001010,
+ StRmaPageSel = 11'b10111100111,
+ StRmaErase = 11'b11000010111,
+ StRmaEraseWait = 11'b01010100110,
+ StRmaWordSel = 11'b00010011001,
+ StRmaProgram = 11'b11011111101,
+ StRmaProgramWait = 11'b00111110000,
+ StRmaRdVerify = 11'b00101001100,
+ StRmaDisabled = 11'b01001011010,
+ StRmaInvalid = 11'b10100111011
} rma_state_e;
diff --git a/hw/ip/flash_ctrl/rtl/flash_phy_core.sv b/hw/ip/flash_ctrl/rtl/flash_phy_core.sv
index c3c1630..6d2a83c 100644
--- a/hw/ip/flash_ctrl/rtl/flash_phy_core.sv
+++ b/hw/ip/flash_ctrl/rtl/flash_phy_core.sv
@@ -88,12 +88,12 @@
//
localparam int StateWidth = 10;
typedef enum logic [StateWidth-1:0] {
- StIdle = 10'b1011011110,
+ StIdle = 10'b1011011110,
StCtrlRead = 10'b0010100110,
StCtrlProg = 10'b1111101101,
- StCtrl = 10'b1101000010,
- StDisable = 10'b0000111011,
- StInvalid = 10'b0101110100
+ StCtrl = 10'b1101000010,
+ StDisable = 10'b0000111011,
+ StInvalid = 10'b0101110100
} state_e;
state_e state_q, state_d;
diff --git a/hw/ip/flash_ctrl/rtl/flash_phy_prog.sv b/hw/ip/flash_ctrl/rtl/flash_phy_prog.sv
index 31d7b59..1986327 100644
--- a/hw/ip/flash_ctrl/rtl/flash_phy_prog.sv
+++ b/hw/ip/flash_ctrl/rtl/flash_phy_prog.sv
@@ -75,17 +75,18 @@
//
localparam int StateWidth = 11;
typedef enum logic [StateWidth-1:0] {
- StIdle = 11'b00101010010,
- StPrePack = 11'b00110101001,
- StPackData = 11'b00000011101,
- StPostPack = 11'b11111101100,
- StCalcPlainEcc = 11'b10110011110,
- StReqFlash = 11'b01111000111,
- StWaitFlash = 11'b11001110101,
- StCalcMask = 11'b01000100000,
- StScrambleData = 11'b11001001010,
- StCalcEcc = 11'b11110110011,
- StInvalid = 11'b10011000001
+ StIdle = 11'b11111111110,
+ StPrePack = 11'b00001110111,
+ StPackData = 11'b10100100011,
+ StPostPack = 11'b11010000101,
+ StCalcPlainEcc = 11'b01101011011,
+ StReqFlash = 11'b01010110010,
+ StWaitFlash = 11'b00100111000,
+ StCalcMask = 11'b00000001110,
+ StScrambleData = 11'b00011101001,
+ StCalcEcc = 11'b00111010100,
+ StDisabled = 11'b10001000000,
+ StInvalid = 11'b10010011011
} state_e;
state_e state_d, state_q;
@@ -190,7 +191,7 @@
// only disable during idle state to ensure program is able to gracefully complete
// this is important as we do not want to accidentally disturb any electrical procedure
// internal to the flash macro
- state_d = StInvalid;
+ state_d = StDisabled;
end else if (req_i && |sel_i) begin
state_d = StPrePack;
end else if (req_i) begin
@@ -281,7 +282,12 @@
end
end
+ StDisabled: begin
+ state_d = StDisabled;
+ end
+
StInvalid: begin
+ state_d = StInvalid;
fsm_err_o = 1'b1;
end
diff --git a/hw/top_earlgrey/ip/flash_ctrl/rtl/autogen/flash_ctrl_pkg.sv b/hw/top_earlgrey/ip/flash_ctrl/rtl/autogen/flash_ctrl_pkg.sv
index c946f3d..ee70364 100644
--- a/hw/top_earlgrey/ip/flash_ctrl/rtl/autogen/flash_ctrl_pkg.sv
+++ b/hw/top_earlgrey/ip/flash_ctrl/rtl/autogen/flash_ctrl_pkg.sv
@@ -591,17 +591,18 @@
// Minimum Hamming weight: 3
// Maximum Hamming weight: 6
//
- localparam int RmaStateWidth = 10;
+ localparam int RmaStateWidth = 11;
typedef enum logic [RmaStateWidth-1:0] {
- StRmaIdle = 10'b1101000011,
- StRmaPageSel = 10'b0010111001,
- StRmaErase = 10'b1111010100,
- StRmaEraseWait = 10'b0111010101,
- StRmaWordSel = 10'b0001011111,
- StRmaProgram = 10'b0110001110,
- StRmaProgramWait = 10'b1000110110,
- StRmaRdVerify = 10'b1011101010,
- StRmaInvalid = 10'b1100101101
+ StRmaIdle = 11'b11110001010,
+ StRmaPageSel = 11'b10111100111,
+ StRmaErase = 11'b11000010111,
+ StRmaEraseWait = 11'b01010100110,
+ StRmaWordSel = 11'b00010011001,
+ StRmaProgram = 11'b11011111101,
+ StRmaProgramWait = 11'b00111110000,
+ StRmaRdVerify = 11'b00101001100,
+ StRmaDisabled = 11'b01001011010,
+ StRmaInvalid = 11'b10100111011
} rma_state_e;
