Update proxying to do SDA arbitration properly (#312)

Author: Aaron-Hartwig

hdl/ip/vhd/vunit_components/i2c_target/i2c_target_vc.vhd

@@ -90,7 +90,7 @@ begin
             when IDLE =>
                 wait on start_condition;
                 state <= START;
-            
+
             when START =>
                 event_msg   := new_msg(got_start);
                 send(net, I2C_TARGET_VC.p_actor, event_msg);

hdl/ip/vhd/vunit_components/i2c_target/i2c_target_vc_pkg.vhd

@@ -51,6 +51,11 @@ package i2c_target_vc_pkg is
         constant expected_msg   : msg_type_t;
     );
 
+    procedure expect_start (
+        signal net  : inout network_t;
+        constant vc : i2c_target_vc_t;
+    );
+
     procedure expect_stop (
         signal net  : inout network_t;
         constant vc : i2c_target_vc_t;
@@ -128,6 +133,14 @@ package body i2c_target_vc_pkg is
         end if;
     end procedure;
 
+    procedure expect_start (
+        signal net      : inout network_t;
+        constant vc     : i2c_target_vc_t;
+    ) is
+    begin
+        expect_message(net, vc, got_start);
+    end procedure;
+
     procedure expect_stop (
         signal net      : inout network_t;
         constant vc     : i2c_target_vc_t;

hdl/projects/cosmo_seq/spd_proxy/sims/spd_proxy_top_tb.vhd

@@ -42,11 +42,9 @@ begin
         variable i2c_ctrlr_msg : msg_t;
 
         variable command    : cmd_t;
-        variable ack        : boolean := false;
 
         variable data       : std_logic_vector(7 downto 0);
         variable exp_addr   : std_logic_vector(7 downto 0);
-        variable exp_data   : std_logic_vector(7 downto 0);
         variable byte_len   : natural;
         variable byte_idx   : natural;
 
@@ -59,6 +57,15 @@ begin
         -- attempting to interrupt
         procedure init_controller is
         begin
+            -- we gate the FPGA controller on seeing CPU activity before it will start talking, so
+            -- fake that out
+            i2c_ctrlr_msg := new_msg(i2c_send_start);
+            send(net, I2C_CTRL_VC.p_actor, i2c_ctrlr_msg);
+            expect_start(net, I2C_TGT_VC);
+            i2c_ctrlr_msg := new_msg(i2c_send_stop);
+            send(net, I2C_CTRL_VC.p_actor, i2c_ctrlr_msg);
+            expect_stop(net, I2C_TGT_VC);
+
             -- arbitrary for the test
             exp_addr    := X"00";
             byte_len    := 8;

hdl/projects/cosmo_seq/spd_proxy/spd_proxy_top.vhd

@@ -40,18 +40,31 @@ entity spd_proxy_top is
 end entity;
 
 architecture rtl of spd_proxy_top is
-    constant CPU_I2C_TSP_CYCLES : integer :=
+    -- TODO: Just use a single TSP constant?
+    constant DIMM_I2C_TSP_CYCLES : integer :=
         to_integer(calc_ns(get_i2c_settings(I2C_MODE).tsp_ns, CLK_PER_NS, 8));
+    constant CPU_I2C_TSP_CYCLES : integer :=
+        to_integer(calc_ns(get_i2c_settings(STANDARD).tsp_ns, CLK_PER_NS, 8));
     signal cpu_scl_filt         : std_logic;
     signal cpu_scl_fedge        : std_logic;
+    signal cpu_scl_redge        : std_logic;
+    signal cpu_sda_filt         : std_logic;
     signal cpu_sda_fedge        : std_logic;
     signal cpu_sda_redge        : std_logic;
     signal cpu_start_detected   : std_logic;
     signal cpu_stop_detected    : std_logic;
-    signal cpu_busy             : boolean;
-    signal cpu_has_mux          : boolean;
+    signal cpu_busy             : std_logic;
+    signal cpu_has_mux          : std_logic;
+    signal cpu_first_start_seen : boolean;
+
+    signal dimm_scl_filt        : std_logic;
+    signal dimm_sda_filt        : std_logic;
+    signal dimm_sda_fedge       : std_logic;
+    signal dimm_sda_redge       : std_logic;
+
+    signal cpu_sda_oe           : std_logic;
+    signal dimm_sda_oe          : std_logic;
 
-    signal ctrlr_idle           : std_logic;
     signal ctrlr_scl_if         : tristate;
     signal ctrlr_sda_if         : tristate;
     signal ctrlr_has_int_mux    : boolean;
@@ -68,11 +81,34 @@ architecture rtl of spd_proxy_top is
     signal sda_sim          : std_logic;
     signal sda_sim_fedge    : std_logic;
     signal start_simulated  : std_logic;
+
+    signal cpu_seen         : boolean;
+    signal fpga_txn_valid   : std_logic;
+
+    signal cpu_has_sda      : std_logic;
+    signal dimm_has_sda     : std_logic;
 begin
+    dimm_glitch_filter_inst: entity work.i2c_glitch_filter
+        generic map(
+            filter_cycles   => DIMM_I2C_TSP_CYCLES
+        )
+        port map(
+            clk             => clk,
+            reset           => reset,
+            raw_scl         => dimm_scl_if.i,
+            raw_sda         => dimm_sda_if.i,
+            filtered_scl    => dimm_scl_filt,
+            scl_redge       => open,
+            scl_fedge       => open,
+            filtered_sda    => dimm_sda_filt,
+            sda_redge       => dimm_sda_redge,
+            sda_fedge       => dimm_sda_fedge
+        );
+
     --
     -- CPU bus monitoring
     --
-    i2c_glitch_filter_inst: entity work.i2c_glitch_filter
+    cpu_glitch_filter_inst: entity work.i2c_glitch_filter
         generic map(
             filter_cycles   => CPU_I2C_TSP_CYCLES
         )
@@ -82,9 +118,9 @@ begin
             raw_scl         => cpu_scl_if.i,
             raw_sda         => cpu_sda_if.i,
             filtered_scl    => cpu_scl_filt,
-            scl_redge       => open,
+            scl_redge       => cpu_scl_redge,
             scl_fedge       => cpu_scl_fedge,
-            filtered_sda    => open,
+            filtered_sda    => cpu_sda_filt,
             sda_redge       => cpu_sda_redge,
             sda_fedge       => cpu_sda_fedge
         );
@@ -97,22 +133,33 @@ begin
     bus_monitor: process(clk, reset)
     begin
         if reset then
-            cpu_busy    <= false;
-            need_start  <= false;
+            cpu_busy                <= '0';
+            need_start              <= false;
+            cpu_first_start_seen    <= false;
+            cpu_seen                <= false;
         elsif rising_edge(clk) then
             if cpu_start_detected then
-                cpu_busy    <= true;
+                cpu_busy                <= '1';
+                cpu_first_start_seen    <= true;
             elsif cpu_stop_detected then
-                cpu_busy    <= false;
+                cpu_busy    <= '0';
             end if;
 
             -- The FPGA still owns the bus and the START hold time as elapsed. This means before
             -- the mux is swapped we need to simulate a START condition .
-            if ctrlr_idle = '0' and cpu_scl_fedge = '1' then
+            if i2c_ctrlr_idle = '0' and cpu_scl_fedge = '1' then
                 need_start  <= true;
             elsif start_simulated then
                 need_start  <= false;
             end if;
+
+            -- TODO: enabling the internal controller should probably just be an external thing
+            -- from registers the SP can set. While proxy is not enabled, we should just assume the
+            -- CPU always owns the bus.
+            -- after the first START/STOP detection, register that we've seen CPU activity
+            if cpu_first_start_seen and cpu_stop_detected = '1' and not cpu_seen then
+                cpu_seen    <= true;
+            end if;
         end if;
     end process;
 
@@ -158,6 +205,8 @@ begin
             done    => start_simulated
         );
 
+    fpga_txn_valid <= '1' when cpu_seen and i2c_command_valid = '1' else '0';
+
     -- FPGA I2C controller
     i2c_ctrl_txn_layer_inst: entity work.i2c_ctrl_txn_layer
         generic map(
@@ -170,34 +219,52 @@ begin
             scl_if      => ctrlr_scl_if,
             sda_if      => ctrlr_sda_if,
             cmd         => i2c_command,
-            cmd_valid   => i2c_command_valid,
-            abort       => cpu_start_detected,
+            cmd_valid   => fpga_txn_valid,
+            abort       => cpu_busy,
             core_ready  => i2c_ctrlr_idle,
             tx_st_if    => i2c_tx_st_if,
             rx_st_if    => i2c_rx_st_if
         );
 
-    -- This mux controls if the I2C controller or the simulated START generator control the
-    -- FPGA internal I2C bux.
+    -- for the internal bus, mux between our simulated start and internal controller
     ctrlr_has_int_mux   <= not need_start or i2c_ctrlr_idle = '0';
     fpga_scl_if.o       <= ctrlr_scl_if.o when ctrlr_has_int_mux else scl_sim;
-    fpga_scl_if.oe      <= ctrlr_scl_if.oe when ctrlr_has_int_mux else not scl_sim;
+    fpga_scl_if.oe      <= ctrlr_scl_if.oe when ctrlr_has_int_mux else '1';
     fpga_sda_if.o       <= ctrlr_sda_if.o when ctrlr_has_int_mux else sda_sim;
-    fpga_sda_if.oe      <= ctrlr_sda_if.oe when ctrlr_has_int_mux else not sda_sim;
+    fpga_sda_if.oe      <= ctrlr_sda_if.oe when ctrlr_has_int_mux else '1';
+
+    -- Break the fpga input from the bus when it doesn't have the bus
+    -- The I2C link layer filters SDA so we will feed it the unfiltered signal
+    ctrlr_scl_if.i  <= '1' when cpu_has_mux else dimm_scl_filt;
+    ctrlr_sda_if.i  <= '1' when cpu_has_mux else dimm_sda_if.i; 
+    fpga_scl_if.i   <= ctrlr_scl_if.i;
+    fpga_sda_if.i   <= ctrlr_sda_if.i;
+
+    sda_arbiter_inst: entity work.sda_arbiter
+        generic map(
+            HYSTERESIS_CYCLES => DIMM_I2C_TSP_CYCLES + 7 -- 7 is a bit of a swag given Ruby testing
+        )
+        port map(
+            clk     => clk,
+            reset   => reset,
+            a       => cpu_sda_filt,
+            b       => dimm_sda_filt,
+            enabled => cpu_has_mux,
+            a_grant => cpu_has_sda,
+            b_grant => dimm_has_sda
+        );
 
-    -- Break the controller input from the bus when it does not have this mux
-    ctrlr_scl_if.i      <= fpga_scl_if.i when ctrlr_has_int_mux else '1';
-    ctrlr_sda_if.i      <= fpga_sda_if.i when ctrlr_has_int_mux else '1';
+    cpu_has_mux     <= '1' when cpu_busy = '1' and i2c_ctrlr_idle = '1' and not need_start 
+                        else '0';
+    dimm_scl_if.oe  <= '1' when cpu_has_mux else fpga_scl_if.oe;
+    dimm_scl_if.o   <= cpu_scl_filt when cpu_has_mux else fpga_scl_if.o;
 
-    -- This mux controls if the CPU or the FPGA control the bus out to the DIMMs
-    cpu_has_mux     <= cpu_busy and i2c_ctrlr_idle = '1' and not need_start;
-    dimm_scl_if.o   <= cpu_scl_if.i when cpu_has_mux else fpga_scl_if.o;
-    dimm_scl_if.oe  <= not cpu_scl_if.i when cpu_has_mux else fpga_scl_if.oe;
-    dimm_sda_if.o   <= cpu_sda_if.i when cpu_has_mux else fpga_sda_if.o;
-    dimm_sda_if.oe  <= not cpu_sda_if.i when cpu_has_mux else fpga_sda_if.oe;
+    dimm_sda_oe     <= not cpu_sda_filt when cpu_has_sda else '0';
+    dimm_sda_if.oe  <= dimm_sda_oe when cpu_has_mux else fpga_sda_if.oe;
+    dimm_sda_if.o   <= '0' when cpu_has_mux else fpga_sda_if.o;
 
-    -- Break the fpga input from the bus when it doesn't have the bus
-    fpga_scl_if.i  <= '1' when cpu_has_mux else dimm_scl_if.i;
-    fpga_sda_if.i  <= '1' when cpu_has_mux else dimm_sda_if.i;
+    cpu_sda_oe      <= not dimm_sda_filt when dimm_has_sda else '0';
+    cpu_sda_if.oe   <= cpu_sda_oe when cpu_has_mux else '0';
+    cpu_sda_if.o    <= '0';
 
 end architecture;