fix: handle error response for overlapped registers with read-only and write-only attributes (#178)

Sebastien Baillou · amykyta3 · commit efbddccc5477 · 2025-11-15T19:33:21.000-08:00
diff --git a/src/peakrdl_regblock/addr_decode.py b/src/peakrdl_regblock/addr_decode.py
@@ -141,23 +141,26 @@ def _add_addressablenode_decoding_flags(self, node: 'AddressableNode') -> None:
             readable = node.is_sw_readable
             writable = node.is_sw_writable
             if readable and writable:
-                rhs_invalid_rw = "'0"
+                pass
             elif readable and not writable:
                 rhs = f"{addr_decoding_str} & !cpuif_req_is_wr"
-                rhs_invalid_rw = f"{addr_decoding_str} & cpuif_req_is_wr"
             elif not readable and writable:
                 rhs = f"{addr_decoding_str} & cpuif_req_is_wr"
-                rhs_invalid_rw = f"{addr_decoding_str} & !cpuif_req_is_wr"
             else:
                 raise RuntimeError
         # Add decoding flags
         self.add_content(f"{self.addr_decode.get_external_block_access_strobe(node)} = {rhs};")
         self.add_content(f"is_external |= {rhs};")
-        if self.addr_decode.exp.ds.err_if_bad_addr:
+        # Also assign is_valid_adddr when err_if_bad_rw is set so that it can be used to catch
+        # invalid RW accesses on existing registers only.
+        if self.addr_decode.exp.ds.err_if_bad_addr or self.addr_decode.exp.ds.err_if_bad_rw:
             self.add_content(f"is_valid_addr |= {rhs_valid_addr};")
         if isinstance(node, MemNode):
             if self.addr_decode.exp.ds.err_if_bad_rw:
-                self.add_content(f"is_invalid_rw |= {rhs_invalid_rw};")
+                self.add_content(f"is_valid_rw |= {rhs};")
+        else:
+            # For external register blocks, all accesses are valid RW
+            self.add_content(f"is_valid_rw |= {rhs_valid_addr};")
 
 
     def enter_AddressableComponent(self, node: 'AddressableNode') -> Optional[WalkerAction]:
@@ -206,13 +209,10 @@ def _add_reg_decoding_flags(self,
         writable = node.has_sw_writable
         if readable and writable:
             rhs = addr_decoding_str
-            rhs_invalid_rw = "'0"
         elif readable and not writable:
             rhs = f"{addr_decoding_str} & !cpuif_req_is_wr"
-            rhs_invalid_rw = f"{addr_decoding_str} & cpuif_req_is_wr"
         elif not readable and writable:
             rhs = f"{addr_decoding_str} & cpuif_req_is_wr"
-            rhs_invalid_rw = f"{addr_decoding_str} & !cpuif_req_is_wr"
         else:
             raise RuntimeError
         # Add decoding flags
@@ -222,10 +222,12 @@ def _add_reg_decoding_flags(self,
             self.add_content(f"{self.addr_decode.get_access_strobe(node)}[{subword_index}] = {rhs};")
         if node.external:
             self.add_content(f"is_external |= {rhs};")
-        if self.addr_decode.exp.ds.err_if_bad_addr:
+        # Also assign is_valid_adddr when err_if_bad_rw is set so that it can be used to catch
+        # invalid RW accesses on existing registers only.
+        if self.addr_decode.exp.ds.err_if_bad_addr or self.addr_decode.exp.ds.err_if_bad_rw:
             self.add_content(f"is_valid_addr |= {rhs_valid_addr};")
         if self.addr_decode.exp.ds.err_if_bad_rw:
-            self.add_content(f"is_invalid_rw |= {rhs_invalid_rw};")
+            self.add_content(f"is_valid_rw |= {rhs};")
 
     def enter_Reg(self, node: RegNode) -> None:
         regwidth = node.get_property('regwidth')
diff --git a/src/peakrdl_regblock/module_tmpl.sv b/src/peakrdl_regblock/module_tmpl.sv
@@ -121,19 +121,31 @@ module {{ds.module_name}}
 
     always_comb begin
         automatic logic is_valid_addr;
-        automatic logic is_invalid_rw;
+        automatic logic is_valid_rw;
     {%- if ds.has_external_addressable %}
         automatic logic is_external;
         is_external = '0;
     {%- endif %}
-    {%- if ds.err_if_bad_addr %}
+    {%- if ds.err_if_bad_addr or ds.err_if_bad_rw %}
         is_valid_addr = '0;
     {%- else %}
-        is_valid_addr = '1; // No error checking on valid address access
+        is_valid_addr = '1; // No valid address check
+    {%- endif %}
+    {%- if ds.err_if_bad_rw %}
+        is_valid_rw = '0;
+    {%- else %}
+        is_valid_rw = '1; // No valid RW check
     {%- endif %}
-        is_invalid_rw = '0;
         {{address_decode.get_implementation()|indent(8)}}
-        decoded_err = (~is_valid_addr | is_invalid_rw) & decoded_req;
+    {%- if ds.err_if_bad_addr and ds.err_if_bad_rw %}
+        decoded_err = (~is_valid_addr | (is_valid_addr & ~is_valid_rw)) & decoded_req;
+    {%- elif ds.err_if_bad_addr %}
+        decoded_err = ~is_valid_addr & decoded_req;
+    {%- elif ds.err_if_bad_rw %}
+        decoded_err = (is_valid_addr & ~is_valid_rw) & decoded_req;
+    {%- else %}
+        decoded_err = '0;
+    {%- endif %}
     {%- if ds.has_external_addressable %}
         decoded_strb_is_external = is_external;
         external_req = is_external;
diff --git a/tests/test_cpuif_err_rsp/regblock.rdl b/tests/test_cpuif_err_rsp/regblock.rdl
@@ -3,6 +3,7 @@ addrmap top {
     default sw=rw;
     default hw=na;
 
+    // Internal registers
     reg {
         field {
             sw=rw; hw=na; // Storage element
@@ -13,32 +14,27 @@ addrmap top {
         field {
             sw=r; hw=na; // Wire/Bus - constant value
         } f[31:0] = 80;
-    } r_r;
+    } r_ro;
 
     reg {
         field {
             sw=w; hw=r; // Storage element
         } f[31:0] = 100;
-    } r_w;
+    } r_wo;
 
-    external reg {
-        field {
-            sw=rw; hw=na; // Storage element
-        } f[31:0];
-    } er_rw;
-
-    external reg {
-        field {
-            sw=r; hw=na; // Wire/Bus - constant value
-        } f[31:0];
-    } er_r;
-
-    external reg {
-        field {
-            sw=w; hw=na; // Storage element
-        } f[31:0];
-    } er_w;
+    // Combined read-only and write-only register
+    reg {
+        default sw = w;
+        default hw = r;
+        field {} wvalue[32] = 0;
+    } writeonly @ 0x1C;
+    reg {
+        default sw = r;
+        default hw = na;
+        field {} rvalue[32] = 200;
+    } readonly @ 0x1C;
 
+    // External memories
     external mem {
         memwidth = 32;
         mementries = 2;
@@ -48,12 +44,33 @@ addrmap top {
         memwidth = 32;
         mementries = 2;
         sw=r;
-    } mem_r @ 0x28;
+    } mem_ro @ 0x28;
 
     external mem {
         memwidth = 32;
         mementries = 2;
         sw=w;
-    } mem_w @ 0x30;
+    } mem_wo @ 0x30;
+
+    // External block
+    external regfile {
+      // Placeholder registers
+      reg {
+          field {
+              sw=rw; hw=na;
+          } f[31:0] = 40;
+      } r_rw;
+
+      reg {
+          field {
+              sw=r; hw=na;
+          } f[31:0] = 80;
+      } r_ro;
 
+      reg {
+          field {
+              sw=w; hw=r;
+          } f[31:0] = 100;
+      } r_wo;
+    } external_rf @ 0x40;
 };
diff --git a/tests/test_cpuif_err_rsp/tb_template.sv b/tests/test_cpuif_err_rsp/tb_template.sv
@@ -3,45 +3,6 @@
 {%- block dut_support %}
     {% sv_line_anchor %}
 
-    external_reg ext_reg_inst (
-        .clk(clk),
-        .rst(rst),
-
-        .req(hwif_out.er_rw.req),
-        .req_is_wr(hwif_out.er_rw.req_is_wr),
-        .wr_data(hwif_out.er_rw.wr_data),
-        .wr_biten(hwif_out.er_rw.wr_biten),
-        .rd_ack(hwif_in.er_rw.rd_ack),
-        .rd_data(hwif_in.er_rw.rd_data),
-        .wr_ack(hwif_in.er_rw.wr_ack)
-    );
-
-    external_reg ro_reg_inst (
-        .clk(clk),
-        .rst(rst),
-
-        .req(hwif_out.er_r.req),
-        .req_is_wr(hwif_out.er_r.req_is_wr),
-        .wr_data(32'b0),
-        .wr_biten(32'b0),
-        .rd_ack(hwif_in.er_r.rd_ack),
-        .rd_data(hwif_in.er_r.rd_data),
-        .wr_ack()
-    );
-
-    external_reg wo_reg_inst (
-        .clk(clk),
-        .rst(rst),
-
-        .req(hwif_out.er_w.req),
-        .req_is_wr(hwif_out.er_w.req_is_wr),
-        .wr_data(hwif_out.er_w.wr_data),
-        .wr_biten(hwif_out.er_w.wr_biten),
-        .rd_ack(),
-        .rd_data(),
-        .wr_ack(hwif_in.er_w.wr_ack)
-    );
-
     external_block #(
         .ADDR_WIDTH(3)
     ) mem_rw_inst (
@@ -64,14 +25,14 @@
         .clk(clk),
         .rst(rst),
 
-        .req(hwif_out.mem_r.req),
-        .req_is_wr(hwif_out.mem_r.req_is_wr),
-        .addr(hwif_out.mem_r.addr),
+        .req(hwif_out.mem_ro.req),
+        .req_is_wr(hwif_out.mem_ro.req_is_wr),
+        .addr(hwif_out.mem_ro.addr),
         .wr_data(32'b0),
         .wr_biten(32'b0),
-        .rd_ack(hwif_in.mem_r.rd_ack),
-        .rd_data(hwif_in.mem_r.rd_data),
-        .wr_ack(hwif_in.mem_r.wr_ack)
+        .rd_ack(hwif_in.mem_ro.rd_ack),
+        .rd_data(hwif_in.mem_ro.rd_data),
+        .wr_ack(hwif_in.mem_ro.wr_ack)
     );
 
     external_block #(
@@ -80,17 +41,33 @@
         .clk(clk),
         .rst(rst),
 
-        .req(hwif_out.mem_w.req),
-        .req_is_wr(hwif_out.mem_w.req_is_wr),
-        .addr(hwif_out.mem_w.addr),
-        .wr_data(hwif_out.mem_w.wr_data),
-        .wr_biten(hwif_out.mem_w.wr_biten),
+        .req(hwif_out.mem_wo.req),
+        .req_is_wr(hwif_out.mem_wo.req_is_wr),
+        .addr(hwif_out.mem_wo.addr),
+        .wr_data(hwif_out.mem_wo.wr_data),
+        .wr_biten(hwif_out.mem_wo.wr_biten),
         .rd_ack(),
         .rd_data(),
-        .wr_ack(hwif_in.mem_w.wr_ack)
+        .wr_ack(hwif_in.mem_wo.wr_ack)
+    );
+    assign hwif_in.mem_wo.rd_ack = '0;
+    assign hwif_in.mem_wo.rd_data = '0;
+
+    external_block #(
+        .ADDR_WIDTH(4)
+    ) external_rf_inst (
+        .clk(clk),
+        .rst(rst),
+
+        .req(hwif_out.external_rf.req),
+        .req_is_wr(hwif_out.external_rf.req_is_wr),
+        .addr(hwif_out.external_rf.addr),
+        .wr_data(hwif_out.external_rf.wr_data),
+        .wr_biten(hwif_out.external_rf.wr_biten),
+        .rd_ack(hwif_in.external_rf.rd_ack),
+        .rd_data(hwif_in.external_rf.rd_data),
+        .wr_ack(hwif_in.external_rf.wr_ack)
     );
-    assign hwif_in.mem_w.rd_ack = '0;
-    assign hwif_in.mem_w.rd_data = '0;
 
 {%- endblock %}
 
@@ -102,7 +79,7 @@ logic expected_rd_err;
 logic bad_addr_expected_err;
 logic bad_rw_expected_wr_err;
 logic bad_rw_expected_rd_err;
-logic [5:0] addr;
+logic [7:0] addr;
 
     {% sv_line_anchor %}
     ##1;
@@ -139,53 +116,29 @@ logic [5:0] addr;
     cpuif.write(addr, 81, .expects_err(expected_wr_err));
     cpuif.assert_read(addr, 80, .expects_err(expected_rd_err));
 
-    // r_w - sw=w; hw=r; // Storage element
+    // r_wo - sw=w; hw=r; // Storage element
     addr = 'h8;
     expected_rd_err = bad_rw_expected_rd_err;
     expected_wr_err = 'h0;
     cpuif.assert_read(addr, 0, .expects_err(expected_rd_err));
-    assert(cb.hwif_out.r_w.f.value == 100);
+    assert(cb.hwif_out.r_wo.f.value == 100);
 
     cpuif.write(addr, 101, .expects_err(expected_wr_err));
     cpuif.assert_read(addr, 0, .expects_err(expected_rd_err));
-    assert(cb.hwif_out.r_w.f.value == 101);
-
-    // External registers
-    // er_rw - sw=rw; hw=na; // Storage element
-    addr = 'hC;
-    expected_rd_err = 'h0;
-    expected_wr_err = 'h0;
-    ext_reg_inst.value = 'h8C;
-    cpuif.assert_read(addr, 'h8C, .expects_err(expected_rd_err));
-    cpuif.write(addr, 'h8D, .expects_err(expected_wr_err));
-    cpuif.assert_read(addr, 'h8D, .expects_err(expected_rd_err));
-
-    // er_r - sw=r; hw=na; // Wire/Bus - constant value
-    addr = 'h10;
-    expected_rd_err = 'h0;
-    expected_wr_err = bad_rw_expected_wr_err;
-    ro_reg_inst.value = 'hB4;
-    cpuif.assert_read(addr, 'hB4, .expects_err(expected_rd_err));
-    cpuif.write(addr, 'hB5, .expects_err(expected_wr_err));
-    cpuif.assert_read(addr, 'hB4, .expects_err(expected_rd_err));
-
-    // er_w - sw=w; hw=r; // Storage element
-    addr = 'h14;
-    expected_rd_err = bad_rw_expected_rd_err;
-    expected_wr_err = 'h0;
-    wo_reg_inst.value = 'hC8;
-    cpuif.assert_read(addr, 0, .expects_err(expected_rd_err));
-    assert(wo_reg_inst.value == 'hC8);
-
-    cpuif.write(addr, 'hC9, .expects_err(expected_wr_err));
-    cpuif.assert_read(addr, 0, .expects_err(expected_rd_err));
-    assert(wo_reg_inst.value == 'hC9);
+    assert(cb.hwif_out.r_wo.f.value == 101);
 
     // Reading/writing from/to non existing register
     addr = 'h18;
     cpuif.assert_read(addr, 0, .expects_err(bad_addr_expected_err));
     cpuif.write(addr, 'h8C, .expects_err(bad_addr_expected_err));
 
+    // Reading/writing from/to combined read AND write only register
+    addr = 'h1C;
+    expected_rd_err = 'h0;
+    expected_wr_err = 'h0;
+    cpuif.assert_read(addr, 200, .expects_err(expected_rd_err));
+    cpuif.write(addr, 'h8C, .expects_err(expected_wr_err));
+
     // External memories
     // mem_rw - sw=rw;
     addr = 'h20;
@@ -218,4 +171,11 @@ logic [5:0] addr;
     cpuif.assert_read(addr, 0, .expects_err(expected_rd_err));
     assert(mem_wo_inst.mem[0] == 'hC9);
 
+    // External rf;
+    addr = 'h40;
+    expected_rd_err = 'h0;
+    expected_wr_err = 'h0;
+    cpuif.assert_read(addr, 'h0, .expects_err(expected_rd_err));
+    cpuif.write(addr, 'hD0, .expects_err(expected_wr_err));
+    cpuif.assert_read(addr, 'hD0, .expects_err(expected_rd_err));
 {% endblock %}
