From fa4baa2800178a9caaea18c4915ab39f04875c66 Mon Sep 17 00:00:00 2001
From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: Thu, 25 Aug 2022 13:14:20 +1000
Subject: [PATCH] Fix PLRU

Jacob Lifshay found a couple of issues with the PLRU implementation:

 - The tree array is one bit too long. This is harmless as this bit is never
accessed and thus should be optimized out

 - The PLRU read is using the wrong nodes when going down the tree, which leads
to incorrect results.

This fixes it and improves the test bench a bit. I have verified the expected
output using a hand-written tree states, observed the mismatch with the
current implementation and verified the fix.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 plru.vhdl    | 19 +++++++++++++++----
 plru_tb.vhdl | 18 +++++++++++++++++-
 2 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/plru.vhdl b/plru.vhdl
index 9d7d9c1..1c6533d 100644
--- a/plru.vhdl
+++ b/plru.vhdl
@@ -19,8 +19,12 @@ entity plru is
 end entity plru;
 
 architecture rtl of plru is
+    -- Each level of the tree (from leaf to root) has half the number of nodes
+    -- of the previous level. So for a 2^N bits LRU, we have a level of N/2 bits
+    -- one of N/4 bits etc.. down to 1. This gives us 2^N-1 nodes. Ie, 2 bits
+    -- LRU has 3 nodes (2 + 1), 4 bits LRU has 15 nodes (8 + 4 + 2 + 1) etc...
     constant count : positive := 2 ** BITS - 1;
-    subtype node_t is integer range 0 to count;
+    subtype node_t is integer range 0 to count - 1;
     type tree_t is array(node_t) of std_ulogic;
 
     signal tree: tree_t;
@@ -31,14 +35,16 @@ begin
     -- in term of FPGA resource usage...
     get_lru: process(tree)
         variable node : node_t;
+        variable abit : std_ulogic;
     begin
         node := 0;
         for i in 0 to BITS-1 loop
 --          report "GET: i:" & integer'image(i) & " node:" & integer'image(node) & " val:" & std_ulogic'image(tree(node));
-            lru(BITS-1-i) <= tree(node);
+            abit := tree(node);
+            lru(BITS-1-i) <= abit;
             if i /= BITS-1 then
                 node := node * 2;
-                if tree(node) = '1' then
+                if abit = '1' then
                     node := node + 2;
                 else
                     node := node + 1;
@@ -69,7 +75,12 @@ begin
                         end if;
                     end if;
                 end loop;
-            end if;            
+            end if;
         end if;
+--        if falling_edge(clk) then
+--            if acc_en = '1' then
+--                report "UPD: tree:" & to_string(tree);
+--            end if;
+--        end if;
     end process;
 end;
diff --git a/plru_tb.vhdl b/plru_tb.vhdl
index 63ca52d..2cd09f9 100644
--- a/plru_tb.vhdl
+++ b/plru_tb.vhdl
@@ -56,59 +56,75 @@ begin
     begin
         test_runner_setup(runner, runner_cfg);
 
-        wait for 4*clk_period;
+        wait for 8*clk_period;
+
+        info("reset state:" & to_hstring(lru));
+        check_equal(lru, 0, result("LRU"));
 
         info("accessing 1:");
         acc <= "001";
         acc_en <= '1';
         wait for clk_period;
         info("lru:" & to_hstring(lru));
+        check_equal(lru, 4, result("LRU"));
 
         info("accessing 2:");
         acc <= "010";
         wait for clk_period;
         info("lru:" & to_hstring(lru));
+        check_equal(lru, 4, result("LRU"));
 
         info("accessing 7:");
         acc <= "111";
         wait for clk_period;
         info("lru:" & to_hstring(lru));
+        check_equal(lru, 0, result("LRU"));
 
         info("accessing 4:");
         acc <= "100";
         wait for clk_period;
         info("lru:" & to_hstring(lru));
+        check_equal(lru, 0, result("LRU"));
 
         info("accessing 3:");
         acc <= "011";
         wait for clk_period;
         info("lru:" & to_hstring(lru));
+        check_equal(lru, 6, result("LRU"));
 
         info("accessing 5:");
         acc <= "101";
         wait for clk_period;
         info("lru:" & to_hstring(lru));
+        check_equal(lru, 0, result("LRU"));
 
         info("accessing 3:");
         acc <= "011";
         wait for clk_period;
         info("lru:" & to_hstring(lru));
+        check_equal(lru, 6, result("LRU"));
 
         info("accessing 5:");
         acc <= "101";
         wait for clk_period;
         info("lru:" & to_hstring(lru));
+        check_equal(lru, 0, result("LRU"));
 
         info("accessing 6:");
         acc <= "110";
         wait for clk_period;
         info("lru:" & to_hstring(lru));
+        check_equal(lru, 0, result("LRU"));
 
         info("accessing 0:");
         acc <= "000";
         wait for clk_period;
         info("lru:" & to_hstring(lru));
+        check_equal(lru, 4, result("LRU"));
 
+        wait for clk_period;
+        wait for clk_period;
+        
         test_runner_cleanup(runner);
     end process;
 end;