vram: document MemArbiter, fix round-robin ordering bug

vram/MemArbiter.bsv

@@ -8,25 +8,26 @@ export MemArbiterServer(..);
 export MemArbiterClient(..);
 export MemArbiter(..), mkPriorityMemArbiter, mkRoundRobinMemArbiter;
 
+// A MemArbiterOp is an operation that a client is seeking permission
+// to perform.
 typedef struct {
    Bool write;
    addr addr;
 } MemArbiterOp#(type addr) deriving (Bits, Eq, FShow);
 
-// A MemArbiterServer receives requests for memory access and emits
+// A MemArbiterServer receives requests and emits grants.
-// grants.
 interface MemArbiterServer#(type addr);
    method Action request(MemArbiterOp#(addr) req);
    method Bool grant();
 endinterface
 
-// A MemArbiterClient emits requests for memory access and emits
+// A MemArbiterClient emits requests and receives grants.
-// grants.
 interface MemArbiterClient#(type addr);
    method Maybe#(MemArbiterOp#(addr)) request();
    method Action grant();
 endinterface
 
+// Arbiter clients and servers can be connected in the obvious way.
 instance Connectable#(MemArbiterClient#(addr), MemArbiterServer#(addr));
    module mkConnection(MemArbiterClient#(addr) client, MemArbiterServer#(addr) server, Empty ifc);
       rule send_request (client.request matches tagged Valid .req);
@@ -39,12 +40,42 @@ instance Connectable#(MemArbiterClient#(addr), MemArbiterServer#(addr));
    endmodule
 endinstance
 
+// A MemArbiter manages concurrent access to a memory port.
 interface MemArbiter#(numeric type num_clients, type addr);
+   // ports allow clients to request memory access.
    interface Vector#(num_clients, MemArbiterServer#(addr)) ports;
+
+   // The following methods are to support arbiter chaining.
+   //
+   // Suppose you're arbitrating access to a dual-port
+   // memory. Typically, such a memory specifies that if one port is
+   // writing to an address, the other must not concurrently read or
+   // write that same address. This means the arbiters attached to
+   // each memory port must cooperate to avoid simultaneously granting
+   // conflicting requests from their clients.
+   //
+   // Calling forbid_addr prevents the arbiter from granting a
+   // concurrent request to access the given address. forbidden_addr
+   // emits the address for which a write access is being granted.
+   //
+   // MemArbiter intances are Connectable: mkConnection(a, b) gives
+   // conflict priority to a. That is, b will not grant requests that
+   // conflict with the grant that a has emitted.
    method Action forbid_addr(addr addr);
    method addr forbidden_addr();
 endinterface
 
+instance Connectable#(MemArbiter#(m, addr), MemArbiter#(n, addr));
+   module mkConnection(MemArbiter#(m, addr) a, MemArbiter#(n, addr) b, Empty ifc);
+      (* fire_when_enabled *)
+      rule forward_forbid;
+         b.forbid_addr(a.forbidden_addr);
+      endrule
+   endmodule
+endinstance
+
+// mkPriorityMemArbiter returns a MemArbiter that gives priority to
+// lower numbered ports.
 module mkPriorityMemArbiter(MemArbiter#(num_clients, addr))
    provisos (Bits#(addr, _),
              Eq#(addr),
@@ -98,8 +129,9 @@ typedef struct {
    Maybe#(addr) blocked_addr;
 } GrantResult#(numeric type n, type addr) deriving (Bits, Eq, FShow);
 
+// select_grant computes which one entry of requests should be granted.
 function GrantResult#(n, addr) select_grant(Vector#(n, Maybe#(MemArbiterOp#(addr))) requests,
-                                            UInt#(TLog#(n)) lopri,
+                                            UInt#(TLog#(n)) hipri,
                                             Maybe#(addr) block_addr)
    provisos (Eq#(addr));
 
@@ -130,7 +162,7 @@ function GrantResult#(n, addr) select_grant(Vector#(n, Maybe#(MemArbiterOp#(addr
    endfunction
 
    let in = zip(map(fromInteger, genVector()), requests);
-   let rot = reverse(rotateBy(reverse(in), lopri));
+   let rot = rotateBy(in, fromInteger(valueOf(n)-1)-hipri+1);
    let seed = GrantResult{
       granted: False,
       grant_vec: replicate(False),
@@ -151,10 +183,11 @@ module mkRoundRobinMemArbiter(MemArbiter#(num_clients, addr))
    RWire#(addr) blocked_in <- mkRWire();
    Wire#(Maybe#(addr)) blocked_out <- mkBypassWire();
 
-   // low_priority is the index of the client that should be last in
+   // high_prio is the index of the client that should be first in
    // line to receive access. Every time we grant access to a client,
-   // that client becomes low_priority for the next round.
+   // the one after that in sequence becomes high_prio in the next
-   Reg#(UInt#(TLog#(num_clients))) low_priority <- mkReg(0);
+   // round.
+   Reg#(UInt#(TLog#(num_clients))) high_prio <- mkReg(0);
 
    function Maybe#(_t) get_mreq(RWire#(_t) w);
       return w.wget();
@@ -162,11 +195,14 @@ module mkRoundRobinMemArbiter(MemArbiter#(num_clients, addr))
 
    rule grant;
       let in = map(get_mreq, reqs);
-      let res = select_grant(in, low_priority, blocked_in.wget());
+      let res = select_grant(in, high_prio, blocked_in.wget());
 
       grants <= res.grant_vec;
       if (res.granted)
-         low_priority <= res.selected+1;
+         if (res.selected == fromInteger(valueOf(num_clients)-1))
+            high_prio <= 0;
+         else
+            high_prio <= res.selected+1;
       blocked_out <= res.blocked_addr;
    endrule
 

vram/MemArbiter_Test.bsv

@@ -125,7 +125,7 @@ module mkArbiterTB(MemArbiter#(n, Addr) dut, Vector#(m, TestCase#(n)) tests, TB
 
       $display("RUN %s (%0d)", tests[idx].name, idx);
       if (got_grants != want_grants || got_forbid_addr != want_forbid_addr) begin
-         $display(" input:");
+         $display("input:");
          for (Integer i=0; i<valueOf(n); i=i+1)
             $display("    ", $format("%0d", i), ": ", req_s(reqs[i]));
          $display("    forbid: ", addr_s(test.forbid_addr));
@@ -227,77 +227,89 @@ module mkTB(Empty);
    let rrTests = vec(
       // Simple grants
       testCase("All idle",
-               vec(idle, idle, idle), noForbid,
+               vec(idle, idle, idle, idle), noForbid,
                noGrant, noForbid),
       testCase("Port 0 read",
-               vec(read(1), idle, idle), noForbid,
+               vec(read(1), idle, idle, idle), noForbid,
                grant(0), noForbid),
       testCase("Port 0 write",
-               vec(write(1), idle, idle), noForbid,
+               vec(write(1), idle, idle, idle), noForbid,
                grant(0), forbid(1)),
       testCase("Port 1 read",
-               vec(idle, read(1), idle), noForbid,
+               vec(idle, read(1), idle, idle), noForbid,
                grant(1), noForbid),
       testCase("Port 1 write",
-               vec(idle, write(1), idle), noForbid,
+               vec(idle, write(1), idle, idle), noForbid,
                grant(1), forbid(1)),
       testCase("Port 2 read",
-               vec(idle, idle, read(1)), noForbid,
+               vec(idle, idle, read(1), idle), noForbid,
                grant(2), noForbid),
       testCase("Port 2 write",
-               vec(idle, idle, write(1)), noForbid,
+               vec(idle, idle, write(1), idle), noForbid,
                grant(2), forbid(1)),
+      testCase("Port 3 read",
+               vec(idle, idle, idle, read(1)), noForbid,
+               grant(3), noForbid),
+      testCase("Port 3 write",
+               vec(idle, idle, idle, write(1)), noForbid,
+               grant(3), forbid(1)),
 
       // Priorities
-      testCase("Port 2 to reset RR",
+      testCase("Port 3 to reset RR",
-               vec(idle, idle, read(1)), noForbid,
+               vec(idle, idle, idle, read(1)), noForbid,
-               grant(2), noForbid),
+               grant(3), noForbid),
       testCase("Port 0+1 #1",
-               vec(read(1), read(2), idle), noForbid,
+               vec(read(1), read(2), idle, idle), noForbid,
                grant(0), noForbid),
       testCase("Port 0+1 #2",
-               vec(read(1), read(2), idle), noForbid,
+               vec(read(1), read(2), idle, idle), noForbid,
                grant(1), noForbid),
       testCase("Port 0+1 #3",
-               vec(read(1), read(2), idle), noForbid,
+               vec(read(1), read(2), idle, idle), noForbid,
                grant(0), noForbid),
       testCase("Port 0+2 #1",
-               vec(read(1), idle, read(2)), noForbid,
+               vec(read(1), idle, read(2), idle), noForbid,
                grant(2), noForbid),
       testCase("Port 0+2 #2",
-               vec(read(1), idle, read(2)), noForbid,
+               vec(read(1), idle, read(2), idle), noForbid,
                grant(0), noForbid),
-      testCase("Port 0+1+2 #1",
+      testCase("Port 0+1+2+3 #1",
-               vec(read(1), read(2), read(3)), noForbid,
+               vec(read(1), read(2), read(3), read(4)), noForbid,
                grant(1), noForbid),
-      testCase("Port 0+1+2 #2",
+      testCase("Port 0+1+2+3 #2",
-               vec(read(1), read(2), read(3)), noForbid,
+               vec(read(1), read(2), read(3), read(4)), noForbid,
                grant(2), noForbid),
-      testCase("Port 0+1+2 #3",
+      testCase("Port 0+1+2+3 #3",
-               vec(read(1), read(2), read(3)), noForbid,
+               vec(read(1), read(2), read(3), read(4)), noForbid,
+               grant(3), noForbid),
+      testCase("Port 0+1+2+3 #4",
+               vec(read(1), read(2), read(3), read(4)), noForbid,
                grant(0), noForbid),
-      testCase("Port 0+1+2 #4",
+      testCase("Port 0+1+2+3 #5",
-               vec(read(1), read(2), read(3)), noForbid,
+               vec(read(1), read(2), read(3), read(4)), noForbid,
                grant(1), noForbid),
 
       // Forbidden addrs
-      testCase("Port 2 to reset RR",
+      testCase("Port 3 to reset RR",
-               vec(idle, idle, read(1)), noForbid,
+               vec(idle, idle, idle, read(1)), noForbid,
-               grant(2), noForbid),
+               grant(3), noForbid),
       testCase("RR with denied writes #1",
-               vec(read(1), write(2), read(3)), forbid(3),
+               vec(read(1), write(2), read(3), read(4)), forbid(3),
                grant(0), noForbid),
       testCase("RR with denied writes #2",
-               vec(read(1), write(2), read(3)), forbid(3),
+               vec(read(1), write(2), read(3), read(4)), forbid(3),
                grant(1), forbid(2)),
       testCase("RR with denied writes #3",
-               vec(read(1), write(2), read(3)), forbid(3),
+               vec(read(1), write(2), read(3), read(4)), forbid(3),
-               grant(0), noForbid),
+               grant(3), noForbid),
       testCase("RR with denied writes #4",
-               vec(read(1), write(2), read(3)), forbid(3),
+               vec(read(1), write(2), read(3), read(4)), forbid(3),
+               grant(0), noForbid),
+      testCase("RR with denied writes #5",
+               vec(read(1), write(2), read(3), read(4)), forbid(3),
                grant(1), forbid(2))
       );
-   MemArbiter#(3, Addr) rr <- mkRoundRobinMemArbiter();
+   MemArbiter#(4, Addr) rr <- mkRoundRobinMemArbiter();
    let rrTB <- mkArbiterTB(rr, rrTests);
 
    runTest(100,