From dc6ca1da2cf616ab0196f3317e4e97312bb934e7 Mon Sep 17 00:00:00 2001 From: Nathanael Huffman Date: Tue, 28 Jan 2025 21:56:14 -0600 Subject: [PATCH] Address PR feedback --- .../i2c/io_expanders/PCA9506ish/pca9506_pkg.vhd | 7 +++++++ .../i2c/muxes/PCA9545ish/pca9545ish_function.vhd | 14 +++++++------- hdl/ip/vhd/i2c/muxes/PCA9545ish/pca9545ish_top.vhd | 4 ++-- .../muxes/PCA9545ish/sims/i2c_pca9545ish_th.vhd | 11 ++++++----- 4 files changed, 22 insertions(+), 14 deletions(-) diff --git a/hdl/ip/vhd/i2c/io_expanders/PCA9506ish/pca9506_pkg.vhd b/hdl/ip/vhd/i2c/io_expanders/PCA9506ish/pca9506_pkg.vhd index 7024e1bd..4542c0af 100644 --- a/hdl/ip/vhd/i2c/io_expanders/PCA9506ish/pca9506_pkg.vhd +++ b/hdl/ip/vhd/i2c/io_expanders/PCA9506ish/pca9506_pkg.vhd @@ -30,6 +30,13 @@ package pca9506_pkg is reg_mask: io_type ) return std_logic; + -- PCA9506 defines a category as a set of contiguous registers of the + -- same function for each of the 5 ports. When auto-incrementing is + -- enabled, the pointer will wrap around to the first register of the + -- category when it reaches the last register of the category. + -- Because there are 5 ports, there are 5 registers in each category + -- so we can't just use a power of 2 that wraps and we wrote this + -- function. function category_wrapping_increment( pointer: std_logic_vector(5 downto 0) -- register number ) return std_logic_vector; diff --git a/hdl/ip/vhd/i2c/muxes/PCA9545ish/pca9545ish_function.vhd b/hdl/ip/vhd/i2c/muxes/PCA9545ish/pca9545ish_function.vhd index a0df546a..4de5f854 100644 --- a/hdl/ip/vhd/i2c/muxes/PCA9545ish/pca9545ish_function.vhd +++ b/hdl/ip/vhd/i2c/muxes/PCA9545ish/pca9545ish_function.vhd @@ -29,7 +29,7 @@ entity pca9545ish_function is reset : in std_logic; mux_reset: in std_logic; mux_sel : out std_logic_vector(1 downto 0); - other_mux_selected : in std_logic; + allowed_to_enable : in std_logic; -- instruction interface inst_data : in std_logic_vector(7 downto 0); inst_valid : in std_logic; @@ -50,10 +50,10 @@ end entity; architecture rtl of pca9545ish_function is function is_valid( data : std_logic_vector(7 downto 0); - other_mux_selected : std_logic) return boolean is + allowed_to_enable : std_logic) return boolean is begin - -- if another mux is selected, only writes of 0 are allowed - if other_mux_selected = '1' and data /= "00000000" then + -- allow only writes of 0 even when we're not allowed to enable + if allowed_to_enable = '0' and data /= 0 then return false; end if; -- only allow clear and one-hot bits 0-2 @@ -77,7 +77,7 @@ architecture rtl of pca9545ish_function is begin inst_ready <= '1'; -- never block writes - resp_valid <= is_our_transaction; -- never block reads + resp_valid <= '1'; -- never block reads resp_data <= pack(control_reg); -- Only one register to read so hand it back always -- register block @@ -91,7 +91,7 @@ begin if mux_reset then control_reg <= rec_reset; elsif inst_valid = '1' and inst_ready = '1' - and is_valid(inst_data, other_mux_selected) + and is_valid(inst_data, allowed_to_enable) and is_valid_write(txn_header) and is_our_transaction = '1' then control_reg <= unpack(inst_data); @@ -128,7 +128,7 @@ begin if in_ack_phase_last = '1' and in_ack_phase = '0' then ack_next <= '0'; -- Ack on valid data when we're in the middle of a transaction (and we're being addressed) - elsif inst_valid = '1' and inst_ready = '1' and is_valid(inst_data, other_mux_selected) and is_our_transaction = '1' then + elsif inst_valid = '1' and inst_ready = '1' and is_valid(inst_data, allowed_to_enable) and is_our_transaction = '1' then ack_next <= '1'; -- Ack on the address byte when we're in the start of a transaction and we're being addressed elsif txn_header.tgt_addr = i2c_addr and txn_header.valid = '1' and is_our_transaction = '0' then diff --git a/hdl/ip/vhd/i2c/muxes/PCA9545ish/pca9545ish_top.vhd b/hdl/ip/vhd/i2c/muxes/PCA9545ish/pca9545ish_top.vhd index 40a1b8c6..016a0828 100644 --- a/hdl/ip/vhd/i2c/muxes/PCA9545ish/pca9545ish_top.vhd +++ b/hdl/ip/vhd/i2c/muxes/PCA9545ish/pca9545ish_top.vhd @@ -40,7 +40,7 @@ entity pca9545ish_top is -- Signal to reset the mux-state out of band if desired. mux_reset: in std_logic; - other_mux_selected: in std_logic := '0'; + allowed_to_enable: in std_logic; -- I2C bus mux endpoint for control -- Does not support clock-stretching scl : in std_logic; @@ -103,7 +103,7 @@ begin reset => reset, mux_reset => mux_reset, mux_sel => mux_sel, - other_mux_selected => other_mux_selected, + allowed_to_enable => allowed_to_enable, inst_data => inst_data, inst_valid => inst_valid, inst_ready => inst_ready, diff --git a/hdl/ip/vhd/i2c/muxes/PCA9545ish/sims/i2c_pca9545ish_th.vhd b/hdl/ip/vhd/i2c/muxes/PCA9545ish/sims/i2c_pca9545ish_th.vhd index 4072b9fc..ae9f52c3 100644 --- a/hdl/ip/vhd/i2c/muxes/PCA9545ish/sims/i2c_pca9545ish_th.vhd +++ b/hdl/ip/vhd/i2c/muxes/PCA9545ish/sims/i2c_pca9545ish_th.vhd @@ -41,7 +41,7 @@ architecture th of i2c_pca9545ish_th is signal mux_reset : std_logic := '0'; signal mux0_sel : std_logic_vector(1 downto 0); signal mux1_sel : std_logic_vector(1 downto 0); - signal other_mux_selected : std_logic_vector(1 downto 0); + signal allowed_to_enable : std_logic_vector(1 downto 0); begin @@ -62,8 +62,9 @@ begin i2c_bus_scl <= i2c_bus_scl_o when i2c_bus_scl_oe = '1' else 'H'; i2c_bus_sda <= i2c_bus_sda_o when i2c_bus_sda_oe = '1' else 'H'; - other_mux_selected(0) <= '1' when mux1_sel /= "11" else '0'; - other_mux_selected(1) <= '1' when mux0_sel /= "11" else '0'; + -- "11" is de-selected + allowed_to_enable(0) <= '1' when mux1_sel = "11" else '0'; + allowed_to_enable(1) <= '1' when mux0_sel = "11" else '0'; DUT0: entity work.pca9545ish_top generic map( @@ -74,7 +75,7 @@ begin clk => clk, reset => reset, mux_reset => mux_reset, - other_mux_selected => other_mux_selected(0), + allowed_to_enable => allowed_to_enable(0), scl => tgt_scl(0), scl_o => tgt_scl_o(0), scl_oe => tgt_scl_oe(0), @@ -93,7 +94,7 @@ begin clk => clk, reset => reset, mux_reset => mux_reset, - other_mux_selected => other_mux_selected(1), + allowed_to_enable => allowed_to_enable(1), scl => tgt_scl(1), scl_o => tgt_scl_o(1), scl_oe => tgt_scl_oe(1),