Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

I2C controller: add ability to abort transactions #279

Merged
merged 2 commits into from
Feb 8, 2025
Merged

I2C controller: add ability to abort transactions #279

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
288d08c
I2C controller: add ability to abort transactions
Aaron-Hartwig Feb 5, 2025
fc278b5
feedback from Nathanael
Aaron-Hartwig Feb 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion hdl/ip/vhd/i2c/common/sims/i2c_cmd_vc.vhd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ entity i2c_cmd_vc is
port (
cmd : out cmd_t := CMD_RESET;
valid : out std_logic := '0';
abort : out std_logic := '0';
ready : in std_logic;
);
end entity;
Expand Down Expand Up @@ -57,8 +58,16 @@ begin
command.len := pop(msg);
cmd <= command;
valid <= '1';
wait until ready;

-- once the command is accepted, release valid
wait until not ready;
valid <= '0';
elsif msg_type = abort_msg then
abort <= '1';

-- once the core is ready, release abort
wait until ready;
abort <= '0';
else
unexpected_msg_type(msg_type);
end if;
Expand Down
18 changes: 16 additions & 2 deletions hdl/ip/vhd/i2c/common/sims/i2c_cmd_vc_pkg.vhd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
-- License, v. 2.0. If a copy of the MPL was not distributed with this
-- file, You can obtain one at https://mozilla.org/MPL/2.0/.
--
-- Copyright 2024 Oxide Computer Company
-- Copyright 2025 Oxide Computer Company

library ieee;
use ieee.std_logic_1164.all;
Expand Down Expand Up @@ -32,14 +32,20 @@ package i2c_cmd_vc_pkg is
logger : logger_t := i2c_cmd_vc_logger;
) return i2c_cmd_vc_t;

constant push_i2c_cmd_msg : msg_type_t := new_msg_type("push_i2c_cmd");
constant push_i2c_cmd_msg : msg_type_t := new_msg_type("push_i2c_cmd");
constant abort_msg : msg_type_t := new_msg_type("abort");

procedure push_i2c_cmd(
signal net : inout network_t;
i2c_cmd_vc : i2c_cmd_vc_t;
cmd : cmd_t;
);

procedure push_abort(
signal net : inout network_t;
i2c_cmd_vc : i2c_cmd_vc_t;
);

end package;

package body i2c_cmd_vc_pkg is
Expand Down Expand Up @@ -82,5 +88,13 @@ package body i2c_cmd_vc_pkg is
send(net, i2c_cmd_vc.p_actor, msg);
end;

procedure push_abort(
signal net : inout network_t;
i2c_cmd_vc : i2c_cmd_vc_t;
) is
variable msg : msg_t := new_msg(abort_msg);
begin
send(net, i2c_cmd_vc.p_actor, msg);
end;

end package body;
58 changes: 38 additions & 20 deletions hdl/ip/vhd/i2c/common/sims/i2c_target_vc.vhd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,16 +98,21 @@ begin
state <= GET_START_BYTE;

when GET_START_BYTE =>
wait on rx_done;
if rx_data(7 downto 1) = address(i2c_target_vc) then
state <= SEND_ACK;
is_read := rx_data(0) = '1';
event_msg := new_msg(address_matched);
send(net, i2c_target_vc.p_actor, event_msg);
wait until rx_done or stop_condition;

if stop_condition then
state <= GET_STOP;
else
state <= SEND_NACK;
event_msg := new_msg(address_different);
send(net, i2c_target_vc.p_actor, event_msg);
if rx_data(7 downto 1) = address(i2c_target_vc) then
state <= SEND_ACK;
is_read := rx_data(0) = '1';
event_msg := new_msg(address_matched);
send(net, i2c_target_vc.p_actor, event_msg);
else
state <= SEND_NACK;
event_msg := new_msg(address_different);
send(net, i2c_target_vc.p_actor, event_msg);
end if;
end if;

when GET_BYTE =>
Expand Down Expand Up @@ -135,30 +140,43 @@ begin
reg_addr_v := unsigned(rx_data);
end if;
end if;

when SEND_ACK =>
wait until falling_edge(scl_if.i) and tx_done;
if is_read then
state <= SEND_BYTE;
wait until (falling_edge(scl_if.i) and tx_done) or stop_condition;

if stop_condition then
state <= GET_STOP;
else
state <= GET_BYTE;
if is_read then
state <= SEND_BYTE;
else
state <= GET_BYTE;
end if;
end if;

when SEND_NACK =>
wait until falling_edge(scl_if.i);
wait until falling_edge(scl_if.i) or stop_condition;
state <= GET_STOP;

when SEND_BYTE =>
wait until falling_edge(scl_if.i) and tx_done;
reg_addr_v := reg_addr + 1;
state <= GET_ACK;
wait until (falling_edge(scl_if.i) and tx_done) or stop_condition;

if stop_condition then
state <= GET_STOP;
else
reg_addr_v := reg_addr + 1;
state <= GET_ACK;
end if;

when GET_ACK =>
wait on rx_done;
wait until rx_done;
state <= SEND_BYTE when rx_ackd else GET_STOP;

when GET_STOP =>
wait until (stop_condition or stop_during_write);
if not stop_condition then
wait until stop_condition or stop_during_write;
end if;

event_msg := new_msg(got_stop);
send(net, i2c_target_vc.p_actor, event_msg);
state <= IDLE;
Expand Down
29 changes: 27 additions & 2 deletions hdl/ip/vhd/i2c/common/sims/i2c_target_vc_pkg.vhd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
-- License, v. 2.0. If a copy of the MPL was not distributed with this
-- file, You can obtain one at https://mozilla.org/MPL/2.0/.
--
-- Copyright 2024 Oxide Computer Company
-- Copyright 2025 Oxide Computer Company

library ieee;
use ieee.std_logic_1164.all;
Expand Down Expand Up @@ -69,6 +69,12 @@ package i2c_target_vc_pkg is
variable addr : std_logic_vector;
);

procedure expect_abort(
signal net : inout network_t;
constant vc : i2c_target_vc_t;
variable aborted : boolean;
);

end package;

package body i2c_target_vc_pkg is
Expand Down Expand Up @@ -115,8 +121,11 @@ package body i2c_target_vc_pkg is
variable matched : boolean;
begin
receive(net, vc.p_actor, msg);
-- a bit of a hack since check_equal is not implemented for msg_t
matched := message_type(msg) = expected_msg;
check_true(matched, "Received message did not match expected message.");
if not matched then
unexpected_msg_type(message_type(msg));
end if;
end procedure;

procedure expect_stop (
Expand Down Expand Up @@ -162,4 +171,20 @@ package body i2c_target_vc_pkg is
end if;
end procedure;

procedure expect_abort (
signal net : inout network_t;
constant vc : i2c_target_vc_t;
variable aborted : boolean;
) is
variable msg : msg_t;
begin
if aborted then
-- if we are aborted, expect the next event to to STOP
expect_stop(net, vc);
else
-- otherwise, pop an event to get it out of the queue
receive(net, vc.p_actor, msg);
end if;
end procedure;

end package body;
46 changes: 37 additions & 9 deletions hdl/ip/vhd/i2c/controller/link_layer/i2c_ctrl_link_layer.vhd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,15 @@
--
-- Copyright 2025 Oxide Computer Company

-- I2C Control Link Layer
--
-- This block handles the bit-level details of an I2C transaction. It requires higher order logic
-- to actually orchestrate the transaction and is designed for use with i2c_ctrl_txn_layer.
--
-- Notes:
-- - This block currently does not support block stretching.
-- - This block currently does not do ackknowledge-polling after a write.

library ieee;
use ieee.std_logic_1164.all;
use ieee.numeric_std_unsigned.all;
Expand Down Expand Up @@ -101,6 +110,7 @@ architecture rtl of i2c_ctrl_link_layer is
transition_sda_cntr_en : std_logic;
ack_sending : std_logic;
sr_scl_fedge_seen : std_logic;
stop_requested : std_logic;

-- interfaces
rx_data : std_logic_vector(7 downto 0);
Expand All @@ -125,6 +135,7 @@ architecture rtl of i2c_ctrl_link_layer is
'0', -- transition_sda_cntr_en
'0', -- ack_sending
'0', -- sr_scl_fedge_seen
'0', -- stop_requested
(others => '0'),-- rx_data
'0', -- rx_data_valid
(others => '0'),-- tx_data
Expand Down Expand Up @@ -255,6 +266,17 @@ begin
v.transition_sda_cntr_en := '0';
end if;

-- Set if a stop is requested to handle the case when it may need to happen before the end
-- of the transaction. This is then cleared as transition out of the STOP_SETUP state.
-- The combinatorial value of this signal is read elsewhwere in the state machine in an
-- effort to accommodate making the stop happen as quickly as possible. The intention is
-- to transition to a STOP during a normal transition point for I2C which is not necessarily
-- any cycle of the FPGA clk. At slower operating modes this does not matter much, but at
-- FAST_MODE_PLUS (1MHz) it starts to matter more.
if sm_reg.stop_requested = '0' then
v.stop_requested := '1' when tx_stop = '1' and txn_next_valid = '1' else '0';
end if;

case sm_reg.state is

-- Ready and awaiting the next transaction
Expand Down Expand Up @@ -311,12 +333,14 @@ begin
end if;

when HANDLE_NEXT =>
if txn_next_valid then
if v.stop_requested then
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One challenge with the variables and registers is that when you compare a variable, you're building a combinatorial path between that and your checking logic. If you need access to this variable the same cycle it was set, this is correct and acceptable, but if you intend to look at it one or more clock later, you're better off using the sm_reg. value as you'll get it as an output from the register.

You have a mix of these in this block, and while it may be what you intended, it is unclear why you'd pick the variable value or the registered value in some cases. Suggest evaluating and adding clarifying comments as needed. I'll point out some below.

The general pattern should be use the register value unless you need to same-cycle value. I didn't fully analyze this to decide which was needed here.
I find that auto-complete has a harder time when the register isn't named "r" and often substitutes v when I'm writing in this style which can lead to typos.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add a clarifying comments up where I drive the signal. My intention is to check it on the combinatorial path (the two cases where I use registered version don't make sense to me anymore with a fresh brain). I understand the timing pressure implications but I want to attempt to react to the abort as quickly as possible since it is not as simple as "we could just do it next cycle" because we cannot always just send a STOP. This is an optimization to catch the situation where we'd otherwise be transitioning to a new state and would not evaluate our ability to send a STOP again for some amount of time. It really doesn't make a lot of sense outside of the FastMode+ use case since there our usual FPGA clk cycle is a meaningful amount of time, so if it is causing problems I'll revisit this later.

v.state := STOP_SDA;
elsif txn_next_valid then
if tx_start then
-- A repeated start was issued mid-transaction
v.state := WAIT_REPEAT_START;
v.counter := START_SETUP_HOLD_TICKS;
v.count_load := '1';
v.state := WAIT_REPEAT_START;
v.counter := START_SETUP_HOLD_TICKS;
v.count_load := '1';
elsif tx_stop then
v.state := STOP_SDA;
elsif tx_data_valid then
Expand All @@ -335,6 +359,8 @@ begin
if sm_reg.bits_shifted = 8 then
v.state := ACK_RX;
v.bits_shifted := 0;
elsif v.stop_requested then
v.state := STOP_SDA;
else
v.sda_oe := not sm_reg.tx_data(7);
v.tx_data := sm_reg.tx_data(sm_reg.tx_data'high-1 downto sm_reg.tx_data'low) & '1';
Expand All @@ -347,7 +373,7 @@ begin
v.sda_oe := '0';

if scl_redge then
v.state := HANDLE_NEXT;
v.state := STOP_SDA when v.stop_requested else HANDLE_NEXT;
v.rx_ack := not sda_in_syncd;
v.rx_ack_valid := '1';
end if;
Expand All @@ -370,13 +396,14 @@ begin
-- at the first transition_sda pulse start sending the (N)ACK
if transition_sda = '1' then
if sm_reg.ack_sending = '0' then
v.sda_oe := tx_ack;
v.sda_oe := '1' when (tx_ack = '1' and v.stop_requested = '0')
else '0';
v.ack_sending := '1';
else
-- at the next transition point release the bus
v.sda_oe := '0';
v.ack_sending := '0';
v.state := HANDLE_NEXT;
v.state := STOP_SDA when v.stop_requested else HANDLE_NEXT;
end if;
end if;

Expand Down Expand Up @@ -404,8 +431,9 @@ begin

end case;

-- next state logic
v.ready := '1' when v.state = IDLE or v.state = HANDLE_NEXT else '0';
v.ready := '1' when v.state = IDLE or
(v.state = HANDLE_NEXT and v.stop_requested = '0')
else '0';

sm_reg_next <= v;
end process;
Expand Down
22 changes: 19 additions & 3 deletions hdl/ip/vhd/i2c/controller/txn_layer/i2c_ctrl_txn_layer.vhd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,15 @@
--
-- Copyright 2025 Oxide Computer Company

-- I2C Controller Transaction Layer
--
-- This block orchestrates the actions to complete a supplied I2C command (`cmd`).
--
-- Notes:
-- - For write transactions data is expected to be streamed in via `tx_st_if`
-- - For read transactions data is expected to be streamed in via `rx_st_if`
-- - A pulse of the `abort` signal will result in an I2C transaction being ended as fast as possible

library ieee;
use ieee.std_logic_1164.all;
use ieee.numeric_std_unsigned.all;
Expand All @@ -13,7 +22,7 @@ use work.tristate_if_pkg.all;

use work.i2c_common_pkg.all;

entity i2c_txn_layer is
entity i2c_ctrl_txn_layer is
generic (
CLK_PER_NS : positive;
MODE : mode_t;
Expand All @@ -29,6 +38,7 @@ entity i2c_txn_layer is
-- I2C command interface
cmd : in cmd_t;
cmd_valid : in std_logic;
abort : in std_logic;
core_ready : out std_logic;

-- Transmit data stream
Expand All @@ -39,7 +49,7 @@ entity i2c_txn_layer is
);
end entity;

architecture rtl of i2c_txn_layer is
architecture rtl of i2c_ctrl_txn_layer is

type state_t is (
IDLE,
Expand Down Expand Up @@ -223,6 +233,13 @@ begin
end if;
end case;

-- if a transaction is in progress and abort is asserted we should immediately STOP
if abort = '1' and sm_reg.state /= IDLE and sm_reg.state /= STOP
and sm_reg.state /= WAIT_STOP then
v.state := STOP;
v.next_valid := '1';
end if;

-- next state logic
v.do_start := '1' when v.state = START else '0';
v.do_stop := '1' when v.state = STOP else '0';
Expand All @@ -235,7 +252,6 @@ begin
sm_reg_next <= v;
end process;


reg_sm: process(clk, reset)
begin
if reset then
Expand Down
Loading