Merge pull request tgingold-cern#22 from lorenzschmid/access-error

Return error upon invalid access

doc/cheby-ug.txt

@@ -276,14 +276,18 @@ compatibility with Xilinx Vivado graphical tools.
 
 `bus-error`:: Use the dedicated bus features to signal an error in case of
 an addressing error, i.e. when accessing an address without corresponding
-mapping. The value is a boolean. By default this option is disabled. Only the
-following bus interfaces are supported:
+mapping, or an invalid request, i.e. when making a write request to a
+write-only register. The value is a boolean. By default this option is
+disabled. Only the following bus interfaces are supported:
 
-** `wb-*`: In case of an addressing error, the request is not acknowledged (`ACK`
-stays deasserted) but instead an error is returned (`ERR` is asserted).
+** `apb-32`: In case of an error, the APB slave error signal (`PSLVERR`) is
+asserted as soon as the request is acknowledged (`PREADY` is asserted).
 
-** `axi4-lite-32`: In case of an addressing error, the AXI4 Lite response signal
-(`BRESP` or `RRESP`) is non-zero returning `SLVERR` (`0b10`).
+** `axi4-lite-32`: In case of an error, the AXI4 Lite response signal (`BRESP`
+or `RRESP`) is non-zero returning `SLVERR` (`0b10`).
+
+** `wb-*`: In case of an error, the request is not acknowledged (`ACK` stays
+deasserted) but instead an error is returned (`ERR` is asserted).
 
 === Registers
 

proto/cheby/gen_hdl.py

@@ -173,10 +173,15 @@ def add_read(s, n, off):
             s.append(HDLComment("{} {}".format(n.NAME, n.c_name)))
             n.h_gen.gen_read(s, off, ibus, rdproc)
         else:
-            # By default, acknowledge request to unknown address but return
-            # error
-            s.append(HDLAssign(ibus.rd_ack, ibus.rd_req))
-            s.append(HDLAssign(ibus.rd_err, ibus.rd_req))
+            # By default, acknowledge request to unknown address but return error:
+            # Use delayed request signal if available
+            if ibus.rd_req_del:
+                rd_req = ibus.rd_req_del
+            else:
+                rd_req = ibus.rd_req
+
+            s.append(HDLAssign(ibus.rd_ack, rd_req))
+            s.append(HDLAssign(ibus.rd_err, rd_req))
 
     stmts = []
     add_decoder(root, stmts, rd_adr, root, add_read)
@@ -199,10 +204,15 @@ def add_write(s, n, off):
             s.append(HDLComment("{} {}".format(n.NAME, n.c_name)))
             n.h_gen.gen_write(s, off, ibus, wrproc)
         else:
-            # By default, acknowledge request to unknown address but return
-            # error
-            s.append(HDLAssign(ibus.wr_ack, ibus.wr_req))
-            s.append(HDLAssign(ibus.wr_err, ibus.wr_req))
+            # By default, acknowledge request to unknown address but return error
+            # Use delayed request signal if available
+            if ibus.wr_req_del:
+                wr_req = ibus.wr_req_del
+            else:
+                wr_req = ibus.wr_req
+
+            s.append(HDLAssign(ibus.wr_ack, wr_req))
+            s.append(HDLAssign(ibus.wr_err, wr_req))
 
     stmts = []
     add_decoder(root, stmts, wr_adr, root, add_write)

proto/cheby/hdl/apbbus.py

@@ -86,6 +86,22 @@ def expand_bus_w(self, root, module, ibus, opts):
         module.stmts.append(HDLAssign(ibus.wr_adr, root.h_bus["paddr"]))
         module.stmts.append(HDLAssign(ibus.wr_dat, root.h_bus["pwdata"]))
 
+        if opts.bus_error:
+            # Delay write request: Used in case of a read request to a write only
+            # register. In such a case, the write request is directly returned in form
+            # of the write acknowledge. Thereby and without pipelining, the PREADY
+            # signal is already and only asserted during the setup phase. Hence, one
+            # cycle to early. Delaying the signal circumvents this problem.
+            ibus.wr_req_del = module.new_HDLSignal("wr_req_del")
+
+            proc = HDLSync(
+                root.h_bus["clk"], root.h_bus["brst"], rst_sync=gconfig.rst_sync
+            )
+            proc.rst_stmts.append(HDLAssign(ibus.wr_req_del, bit_0))
+            proc.sync_stmts.append(HDLAssign(ibus.wr_req_del, ibus.wr_req))
+
+            module.stmts.append(proc)
+
         # Translate Byte-wise write mask of APB bus to bit-wise write mask of ibus
         proc = HDLComb()
         proc.sensitivity.extend([root.h_bus["pstrb"]])

proto/cheby/hdl/genreg.py

@@ -479,6 +479,7 @@ def gen_processes(self, ibus):
 
     def gen_read(self, s, off, ibus, rdproc):
         n = self.n
+
         # Strobe
         if n.h_rreq_port is not None:
             s.append(HDLAssign(self.strobe_index(off, n.h_rreq_port), ibus.rd_req))
@@ -487,14 +488,23 @@ def gen_read(self, s, off, ibus, rdproc):
                 v = self.strobe_init()
                 rdproc.stmts.append(HDLAssign(n.h_rreq_port, v))
 
-        # Acknowledge and address error
+        # Acknowledge
         if n.h_rack_port is not None:
             rack = n.h_rack_port
             rack = self.strobe_index(off, rack)
+        elif ibus.rd_req_del:
+            rack = ibus.rd_req_del
         else:
             rack = ibus.rd_req
         s.append(HDLAssign(ibus.rd_ack, rack))
-        s.append(HDLAssign(ibus.rd_err, bit_0))
+
+        # Error
+        if n.access == 'wo':
+            # Return error if read request to write-only register is made
+            s.append(HDLAssign(ibus.rd_err, rack))
+        else:
+            # Return no error
+            s.append(HDLAssign(ibus.rd_err, bit_0))
 
         # Data
         if n.access == 'wo':
@@ -528,6 +538,7 @@ def pad(first):
 
     def gen_write(self, s, off, ibus, wrproc):
         n = self.n
+
         # Strobe
         if n.h_wreq is not None:
             s.append(HDLAssign(self.strobe_index(off, n.h_wreq), ibus.wr_req))
@@ -536,11 +547,20 @@ def gen_write(self, s, off, ibus, wrproc):
                 v = self.strobe_init()
                 wrproc.stmts.append(HDLAssign(n.h_wreq, v))
 
-        # Acknowledge and address error
+        # Acknowledge
         wack = n.h_wack_port or n.h_wack
         if wack is not None:
             wack = self.strobe_index(off, wack)
+        elif ibus.wr_req_del:
+            wack = ibus.wr_req_del
         else:
             wack = ibus.wr_req
         s.append(HDLAssign(ibus.wr_ack, wack))
-        s.append(HDLAssign(ibus.wr_err, bit_0))
+
+        # Error
+        if n.access == 'ro':
+            # Return error if write request to read-only register is made
+            s.append(HDLAssign(ibus.wr_err, wack))
+        else:
+            # Return no error
+            s.append(HDLAssign(ibus.wr_err, bit_0))

proto/cheby/hdl/ibus.py

@@ -15,19 +15,24 @@ def __init__(self):
         self.addr_size = None
         self.addr_low = None
         # Read signals (in and out)
-        self.rd_req = None
-        self.rd_ack = None
-        self.rd_err = None
-        self.rd_dat = None
-        self.rd_adr = None
+        self.rd_req = None      # Read request
+        self.rd_req_del = None  # Delayed read request
+                                # (sometimes used to delay acknowledges in case of
+                                #  request errors)
+        self.rd_ack = None      # Read acknowledge
+        self.rd_err = None      # Read error
+        self.rd_adr = None      # Read address
+        self.rd_dat = None      # Read data
         # Write signals (in and out)
-        self.wr_req = None
-        self.wr_ack = None
-        self.wr_err = None
-        self.wr_dat = None
-        self.wr_adr = None
-        self.wr_sel = None
-
+        self.wr_req = None      # Write request
+        self.wr_req_del = None  # Delayed write request
+                                # (sometimes used to delay acknowledges in case of
+                                #  request errors)
+        self.wr_ack = None      # Write acknowledge
+        self.wr_err = None      # Write error
+        self.wr_adr = None      # Write address
+        self.wr_dat = None      # Write data
+        self.wr_sel = None      # Write mask
 
     def pipeline(self, root, module, conds, suffix):
         """Create a new ibus by adding registers to self according to :param conds:
@@ -43,6 +48,7 @@ def pipeline(self, root, module, conds, suffix):
         names = []
         c_ri = 'rd-in' in conds
         names.extend([('rd_req', c_ri, 'i', None, None),
+                      ('rd_req_del', c_ri, 'i', None, None),
                       ('rd_adr', c_ri, 'i', self.addr_size, self.addr_low)])
         c_ro = 'rd-out' in conds
         names.extend([('rd_ack', c_ro, 'o', None, None),
@@ -51,6 +57,7 @@ def pipeline(self, root, module, conds, suffix):
         c_wi = 'wr-in' in conds
         copy_wa = (self.rd_adr == self.wr_adr) and (c_wi == c_ri)
         names.extend([('wr_req', c_wi, 'i', None, None),
+                      ('wr_req_del', c_wi, 'i', None, None),
                       ('wr_adr', c_wi, 'i', self.addr_size, self.addr_low),
                       ('wr_dat', c_wi, 'i', self.data_size, 0),
                       # The write mask of the internal bus operates on a per bit level.

testfiles/tb/buserr.cheby

@@ -4,21 +4,32 @@ memory-map:
   x-hdl:
     bus-error: True
   children:
-  - reg:
-      name: reg1
-      type: unsigned
-      width: 32
-      access: rw
-      preset: 0x12345678
-  - reg:
-      name: reg2
-      type: unsigned
-      width: 32
-      access: rw
-      preset: 0x12345678
-  - reg:
-      name: reg3
-      type: unsigned
-      width: 32
-      access: rw
-      preset: 0x12345678
+    - reg:
+        name: rw0
+        type: unsigned
+        width: 32
+        access: rw
+        preset: 0x12345678
+    - reg:
+        name: rw1
+        type: unsigned
+        width: 32
+        access: rw
+        preset: 0x23456789
+    - reg:
+        name: rw2
+        type: unsigned
+        width: 32
+        access: rw
+        preset: 0x3456789a
+    - reg:
+        name: ro0
+        type: unsigned
+        width: 32
+        access: ro
+    - reg:
+        name: wo0
+        type: unsigned
+        width: 32
+        access: wo
+        preset: 0x56789abc

testfiles/tb/buserr_apb_tb.vhdl

@@ -15,9 +15,11 @@ architecture tb of buserr_apb_tb is
   signal apb_in  : t_apb_master_in;
   signal apb_out : t_apb_master_out;
 
-  signal reg1   : std_logic_vector(31 downto 0);
-  signal reg2   : std_logic_vector(31 downto 0);
-  signal reg3   : std_logic_vector(31 downto 0);
+  signal reg_rw0 : std_logic_vector(31 downto 0);
+  signal reg_rw1 : std_logic_vector(31 downto 0);
+  signal reg_rw2 : std_logic_vector(31 downto 0);
+  signal reg_ro0 : std_logic_vector(31 downto 0);
+  signal reg_wo0 : std_logic_vector(31 downto 0);
 
   signal end_of_test : boolean := False;
 begin
@@ -40,7 +42,7 @@ begin
     port map (
       pclk    => clk,
       presetn => rst_n,
-      paddr   => apb_out.paddr(3 downto 2),
+      paddr   => apb_out.paddr(4 downto 2),
       psel    => apb_out.psel,
       pwrite  => apb_out.pwrite,
       penable => apb_out.penable,
@@ -49,11 +51,15 @@ begin
       pstrb   => apb_out.pstrb,
       prdata  => apb_in.prdata,
       pslverr => apb_in.pslverr,
-      reg1_o  => reg1,
-      reg2_o  => reg2,
-      reg3_o  => reg3
+      rw0_o   => reg_rw0,
+      rw1_o   => reg_rw1,
+      rw2_o   => reg_rw2,
+      ro0_i   => reg_ro0,
+      wo0_o   => reg_wo0
     );
 
+  reg_ro0 <= x"4567_89ab";
+
   main : process is
     variable v : std_logic_vector(31 downto 0);
   begin
@@ -73,30 +79,50 @@ begin
 
     --  Testing regular read
     report "Testing regular read" severity note;
-    apb_read (clk, apb_out, apb_in, x"0000_0000", v, '0');
-    assert reg1 = x"1234_5678" severity error;
+    apb_read(clk, apb_out, apb_in, x"0000_0000", v, '0');
+    assert reg_rw0 = x"1234_5678" severity error;
     assert v = x"1234_5678" severity error;
 
     -- Testing regular write
     report "Testing regular write" severity note;
-    apb_write (clk, apb_out, apb_in, x"0000_0004", x"9abc_def0", '0');
-    assert reg2 = x"9abc_def0" severity error;
-    apb_read (clk, apb_out, apb_in, x"0000_0004", v, '0');
+    apb_write(clk, apb_out, apb_in, x"0000_0004", x"9abc_def0", '0');
+    assert reg_rw1 = x"9abc_def0" severity error;
+    apb_read(clk, apb_out, apb_in, x"0000_0004", v, '0');
     assert v = x"9abc_def0" severity error;
 
     --  Testing erroneous read
     report "Testing erroneous read" severity note;
-    apb_read (clk, apb_out, apb_in, x"0000_000c", v, '1');
+    apb_read(clk, apb_out, apb_in, x"0000_0014", v, '1');
 
     --  Testing regular read 2
     report "Testing regular read 2" severity note;
-    apb_read (clk, apb_out, apb_in, x"0000_0008", v, '0');
-    assert reg3 = x"1234_5678" severity error;
-    assert v = x"1234_5678" severity error;
+    apb_read(clk, apb_out, apb_in, x"0000_0008", v, '0');
+    assert reg_rw2 = x"3456_789a" severity error;
+    assert v = x"3456_789a" severity error;
 
     --  Testing erroneous write
     report "Testing erroneous write" severity note;
-    apb_write (clk, apb_out, apb_in, x"0000_000c", x"5678_9abc", '1');
+    apb_write(clk, apb_out, apb_in, x"0000_0014", x"5678_9abc", '1');
+
+    --  Testing regular read 3
+    report "Testing regular read 3" severity note;
+    apb_read(clk, apb_out, apb_in, x"0000_000c", v, '0');
+    assert reg_ro0 = x"4567_89ab" severity error;
+    assert v = x"4567_89ab" severity error;
+
+    --  Testing erroneous write to read-only register
+    report "Testing erroneous write to read-only register" severity note;
+    apb_write(clk, apb_out, apb_in, x"0000_000c", x"1234_5678", '1');
+
+    --  Testing regular write 2
+    report "Testing regular write 2" severity note;
+    apb_write(clk, apb_out, apb_in, x"0000_0010", x"1234_5678", '0');
+    wait until rising_edge(clk);
+    assert reg_wo0 = x"1234_5678" severity error;
+
+    --  Testing erroneous read to write-only register
+    report "Testing erroneous read to write-only register" severity note;
+    apb_read(clk, apb_out, apb_in, x"0000_0010", v, '1');
 
     wait until rising_edge(clk);
     wait until rising_edge(clk);