From b1d9f87f5538c39ebe8075d16b4dc9c94530e79e Mon Sep 17 00:00:00 2001 From: Tom Burdick Date: Tue, 15 Oct 2024 16:07:12 -0500 Subject: [PATCH 1/3] i2c: Drop TXRX from default RTIO handler TXRX is meant specifically to handle a full duplex bus like SPI, I2C is half duplex meaning only read or write can be performed at once. Drop TXRX as a supported operation code for the default I2C submission path. Signed-off-by: Tom Burdick --- drivers/i2c/i2c_rtio_default.c | 24 ------------------------ 1 file changed, 24 deletions(-) diff --git a/drivers/i2c/i2c_rtio_default.c b/drivers/i2c/i2c_rtio_default.c index c7f94ac9bd2544..048b5b42d79a59 100644 --- a/drivers/i2c/i2c_rtio_default.c +++ b/drivers/i2c/i2c_rtio_default.c @@ -60,27 +60,6 @@ static int i2c_iodev_submit_tiny_tx(struct rtio_iodev_sqe *iodev_sqe, struct i2c return 0; } -static int i2c_iodev_submit_txrx(struct rtio_iodev_sqe *iodev_sqe, struct i2c_msg msgs[2], - uint8_t *num_msgs) -{ - __ASSERT_NO_MSG(iodev_sqe->sqe.op == RTIO_OP_TXRX); - - msgs[0].buf = (uint8_t *)iodev_sqe->sqe.txrx.tx_buf; - msgs[0].len = iodev_sqe->sqe.txrx.buf_len; - msgs[0].flags = - ((iodev_sqe->sqe.iodev_flags & RTIO_IODEV_I2C_10_BITS) ? I2C_MSG_ADDR_10_BITS : 0) | - I2C_MSG_WRITE; - msgs[1].buf = iodev_sqe->sqe.txrx.rx_buf; - msgs[1].len = iodev_sqe->sqe.txrx.buf_len; - msgs[1].flags = - ((iodev_sqe->sqe.iodev_flags & RTIO_IODEV_I2C_STOP) ? I2C_MSG_STOP : 0) | - ((iodev_sqe->sqe.iodev_flags & RTIO_IODEV_I2C_RESTART) ? I2C_MSG_RESTART : 0) | - ((iodev_sqe->sqe.iodev_flags & RTIO_IODEV_I2C_10_BITS) ? I2C_MSG_ADDR_10_BITS : 0) | - I2C_MSG_READ; - *num_msgs = 2; - return 0; -} - void i2c_iodev_submit_work_handler(struct rtio_iodev_sqe *iodev_sqe) { const struct i2c_dt_spec *dt_spec = (const struct i2c_dt_spec *)iodev_sqe->sqe.iodev->data; @@ -105,9 +84,6 @@ void i2c_iodev_submit_work_handler(struct rtio_iodev_sqe *iodev_sqe) case RTIO_OP_TINY_TX: rc = i2c_iodev_submit_tiny_tx(transaction_current, msgs, &num_msgs); break; - case RTIO_OP_TXRX: - rc = i2c_iodev_submit_txrx(transaction_current, msgs, &num_msgs); - break; default: LOG_ERR("Invalid op code %d for submission %p", transaction_current->sqe.op, (void *)&transaction_current->sqe); From fafa353b76711db90765775084c3e22cfe33e848 Mon Sep 17 00:00:00 2001 From: Tom Burdick Date: Fri, 18 Oct 2024 09:55:25 -0500 Subject: [PATCH 2/3] i2c: Fix default RTIO handler transactions Transactions from RTIO should result in single calls to i2c_transfer. This corrects the default handler to first count the number of submissions in the transaction, allocate on the stack, and then copy over each submission to an equivalent i2c_msg. It also cleans up the helper functions to be infallible, taking only the submission and msg to copy to. Signed-off-by: Tom Burdick --- drivers/i2c/Kconfig | 14 ++++ drivers/i2c/i2c_rtio_default.c | 116 +++++++++++++++++++++------------ 2 files changed, 87 insertions(+), 43 deletions(-) diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig index 3651b4b09ee719..86e7165bf080c9 100644 --- a/drivers/i2c/Kconfig +++ b/drivers/i2c/Kconfig @@ -97,6 +97,20 @@ config I2C_RTIO_CQ_SIZE is going to be 4 given the device address, register address, and a value to be read or written. +config I2C_RTIO_FALLBACK_MSGS + int "Number of available i2c_msg structs for the default handler to use" + default 4 + help + When RTIO is used with a driver that does not yet implement the submit API + natively the submissions are converted back to struct i2c_msg values that + are given to i2c_transfer. This requires some number of msgs be available to convert + the submissions into on the stack. MISRA rules dictate we must know this in + advance. + + In all likelihood 4 is going to work for everyone, but in case you do end up with + an issue where you are using RTIO, your driver does not implement submit natively, + and get an error relating to not enough i2c msgs this is the Kconfig to manipulate. + endif # I2C_RTIO diff --git a/drivers/i2c/i2c_rtio_default.c b/drivers/i2c/i2c_rtio_default.c index 048b5b42d79a59..db278393343d2d 100644 --- a/drivers/i2c/i2c_rtio_default.c +++ b/drivers/i2c/i2c_rtio_default.c @@ -12,97 +12,127 @@ #include LOG_MODULE_DECLARE(i2c_rtio, CONFIG_I2C_LOG_LEVEL); -static int i2c_iodev_submit_rx(struct rtio_iodev_sqe *iodev_sqe, struct i2c_msg msgs[2], - uint8_t *num_msgs) +static inline void i2c_msg_from_rx(const struct rtio_iodev_sqe *iodev_sqe, struct i2c_msg *msg) { __ASSERT_NO_MSG(iodev_sqe->sqe.op == RTIO_OP_RX); - msgs[0].buf = iodev_sqe->sqe.rx.buf; - msgs[0].len = iodev_sqe->sqe.rx.buf_len; - msgs[0].flags = + msg->buf = iodev_sqe->sqe.rx.buf; + msg->len = iodev_sqe->sqe.rx.buf_len; + msg->flags = ((iodev_sqe->sqe.iodev_flags & RTIO_IODEV_I2C_STOP) ? I2C_MSG_STOP : 0) | ((iodev_sqe->sqe.iodev_flags & RTIO_IODEV_I2C_RESTART) ? I2C_MSG_RESTART : 0) | ((iodev_sqe->sqe.iodev_flags & RTIO_IODEV_I2C_10_BITS) ? I2C_MSG_ADDR_10_BITS : 0) | I2C_MSG_READ; - *num_msgs = 1; - return 0; } -static int i2c_iodev_submit_tx(struct rtio_iodev_sqe *iodev_sqe, struct i2c_msg msgs[2], - uint8_t *num_msgs) +static inline void i2c_msg_from_tx(const struct rtio_iodev_sqe *iodev_sqe, struct i2c_msg *msg) { __ASSERT_NO_MSG(iodev_sqe->sqe.op == RTIO_OP_TX); - msgs[0].buf = (uint8_t *)iodev_sqe->sqe.tx.buf; - msgs[0].len = iodev_sqe->sqe.tx.buf_len; - msgs[0].flags = + msg->buf = (uint8_t *)iodev_sqe->sqe.tx.buf; + msg->len = iodev_sqe->sqe.tx.buf_len; + msg->flags = ((iodev_sqe->sqe.iodev_flags & RTIO_IODEV_I2C_STOP) ? I2C_MSG_STOP : 0) | ((iodev_sqe->sqe.iodev_flags & RTIO_IODEV_I2C_RESTART) ? I2C_MSG_RESTART : 0) | ((iodev_sqe->sqe.iodev_flags & RTIO_IODEV_I2C_10_BITS) ? I2C_MSG_ADDR_10_BITS : 0) | I2C_MSG_WRITE; - *num_msgs = 1; - return 0; } -static int i2c_iodev_submit_tiny_tx(struct rtio_iodev_sqe *iodev_sqe, struct i2c_msg msgs[2], - uint8_t *num_msgs) +static inline void i2c_msg_from_tiny_tx(const struct rtio_iodev_sqe *iodev_sqe, struct i2c_msg *msg) { __ASSERT_NO_MSG(iodev_sqe->sqe.op == RTIO_OP_TINY_TX); - msgs[0].buf = (uint8_t *)iodev_sqe->sqe.tiny_tx.buf; - msgs[0].len = iodev_sqe->sqe.tiny_tx.buf_len; - msgs[0].flags = + msg->buf = (uint8_t *)iodev_sqe->sqe.tiny_tx.buf; + msg->len = iodev_sqe->sqe.tiny_tx.buf_len; + msg->flags = ((iodev_sqe->sqe.iodev_flags & RTIO_IODEV_I2C_STOP) ? I2C_MSG_STOP : 0) | ((iodev_sqe->sqe.iodev_flags & RTIO_IODEV_I2C_RESTART) ? I2C_MSG_RESTART : 0) | ((iodev_sqe->sqe.iodev_flags & RTIO_IODEV_I2C_10_BITS) ? I2C_MSG_ADDR_10_BITS : 0) | I2C_MSG_WRITE; - *num_msgs = 1; - return 0; } -void i2c_iodev_submit_work_handler(struct rtio_iodev_sqe *iodev_sqe) +void i2c_iodev_submit_work_handler(struct rtio_iodev_sqe *txn_first) { - const struct i2c_dt_spec *dt_spec = (const struct i2c_dt_spec *)iodev_sqe->sqe.iodev->data; + const struct i2c_dt_spec *dt_spec = (const struct i2c_dt_spec *)txn_first->sqe.iodev->data; const struct device *dev = dt_spec->bus; - LOG_DBG("Sync RTIO work item for: %p", (void *)iodev_sqe); - - struct rtio_iodev_sqe *transaction_current = iodev_sqe; - struct i2c_msg msgs[2]; - uint8_t num_msgs; + LOG_DBG("Sync RTIO work item for: %p", (void *)txn_first); + uint32_t num_msgs = 0; int rc = 0; + struct rtio_iodev_sqe *txn_last = txn_first; + /* We allocate the i2c_msg's on the stack, to do so + * the count of messages needs to be determined to + * ensure we don't go over the statically sized array. + */ do { - /* Convert the iodev_sqe back to an i2c_msg */ - switch (transaction_current->sqe.op) { + switch (txn_last->sqe.op) { + case RTIO_OP_RX: + case RTIO_OP_TX: + case RTIO_OP_TINY_TX: + num_msgs++; + break; + default: + LOG_ERR("Invalid op code %d for submission %p", txn_last->sqe.op, + (void *)&txn_last->sqe); + rc = -EIO; + break; + } + txn_last = rtio_txn_next(txn_last); + } while (rc == 0 && txn_last != NULL); + + if (rc != 0) { + rtio_iodev_sqe_err(txn_first, rc); + return; + } + + /* Allocate msgs on the stack, MISRA doesn't like VLAs so we need a statically + * sized array here. It's pretty unlikely we have more than 4 i2c messages + * in a transaction as we typically would only have 2, one to write a + * register address, and another to read/write the register into an array + */ + if (num_msgs > CONFIG_I2C_RTIO_FALLBACK_MSGS) { + LOG_ERR("At most CONFIG_I2C_RTIO_FALLBACK_MSGS" + " submissions in a transaction are" + " allowed in the default handler"); + rtio_iodev_sqe_err(txn_first, -ENOMEM); + return; + } + struct i2c_msg msgs[CONFIG_I2C_RTIO_FALLBACK_MSGS]; + + rc = 0; + txn_last = txn_first; + + /* Copy the transaction into the stack allocated msgs */ + for (int i = 0; i < num_msgs; i++) { + switch (txn_last->sqe.op) { case RTIO_OP_RX: - rc = i2c_iodev_submit_rx(transaction_current, msgs, &num_msgs); + i2c_msg_from_rx(txn_last, &msgs[i]); break; case RTIO_OP_TX: - rc = i2c_iodev_submit_tx(transaction_current, msgs, &num_msgs); + i2c_msg_from_tx(txn_last, &msgs[i]); break; case RTIO_OP_TINY_TX: - rc = i2c_iodev_submit_tiny_tx(transaction_current, msgs, &num_msgs); + i2c_msg_from_tiny_tx(txn_last, &msgs[i]); break; default: - LOG_ERR("Invalid op code %d for submission %p", transaction_current->sqe.op, - (void *)&transaction_current->sqe); rc = -EIO; break; } - if (rc == 0) { - __ASSERT_NO_MSG(num_msgs > 0); + txn_last = rtio_txn_next(txn_last); + } - rc = i2c_transfer(dev, msgs, num_msgs, dt_spec->addr); - transaction_current = rtio_txn_next(transaction_current); - } - } while (rc == 0 && transaction_current != NULL); + if (rc == 0) { + __ASSERT_NO_MSG(num_msgs > 0); + + rc = i2c_transfer(dev, msgs, num_msgs, dt_spec->addr); + } if (rc != 0) { - rtio_iodev_sqe_err(iodev_sqe, rc); + rtio_iodev_sqe_err(txn_first, rc); } else { - rtio_iodev_sqe_ok(iodev_sqe, 0); + rtio_iodev_sqe_ok(txn_first, 0); } } From 8cc92afe387dcdd7e3eb350aba09f2d2a97ed719 Mon Sep 17 00:00:00 2001 From: Tom Burdick Date: Tue, 22 Oct 2024 09:12:00 -0500 Subject: [PATCH 3/3] i2c: Drop transceive test, fix transfer call count Transactions should result in a single transfer call not multiple transfer calls. Transceive isn't supported by i2c and so the TXRX op isn't validated for success anymore. Signed-off-by: Tom Burdick --- tests/subsys/rtio/rtio_i2c/src/main.cpp | 38 +------------------------ 1 file changed, 1 insertion(+), 37 deletions(-) diff --git a/tests/subsys/rtio/rtio_i2c/src/main.cpp b/tests/subsys/rtio/rtio_i2c/src/main.cpp index 7ffeae0feb183b..e58412ad9ca678 100644 --- a/tests/subsys/rtio/rtio_i2c/src/main.cpp +++ b/tests/subsys/rtio/rtio_i2c/src/main.cpp @@ -117,42 +117,6 @@ ZTEST(rtio_i2c, test_fallback_submit_tiny_tx) rtio_cqe_release(&test_rtio_ctx, cqe); } -ZTEST(rtio_i2c, test_fallback_submit_txrx) -{ - uint8_t tx_data[] = {0x01, 0x02, 0x03}; - uint8_t rx_data[ARRAY_SIZE(tx_data)] = {0}; - struct rtio_sqe *sqe = rtio_sqe_acquire(&test_rtio_ctx); - - blocking_emul_i2c_transfer_fake.custom_fake = - [&tx_data](const struct emul *, struct i2c_msg *msgs, int msg_count, int) { - zassert_equal(2, msg_count); - // First message should be a 'tx' - zassert_equal(ARRAY_SIZE(tx_data), msgs[0].len); - zassert_mem_equal(tx_data, msgs[0].buf, msgs[0].len); - zassert_equal(I2C_MSG_WRITE, msgs[0].flags); - // Second message should be an 'rx' - zassert_equal(ARRAY_SIZE(tx_data), msgs[1].len); - zassert_equal(I2C_MSG_READ | I2C_MSG_STOP, msgs[1].flags); - for (uint8_t i = 0; i < msgs[1].len; ++i) { - msgs[1].buf[i] = msgs[0].buf[i]; - } - return 0; - }; - - zassert_not_null(sqe); - rtio_sqe_prep_transceive(sqe, &blocking_emul_iodev, RTIO_PRIO_NORM, tx_data, rx_data, - ARRAY_SIZE(tx_data), NULL); - zassert_ok(rtio_submit(&test_rtio_ctx, 1)); - zassert_equal(1, blocking_emul_i2c_transfer_fake.call_count); - - struct rtio_cqe *cqe = rtio_cqe_consume_block(&test_rtio_ctx); - - zassert_ok(cqe->result); - zassert_mem_equal(tx_data, rx_data, ARRAY_SIZE(tx_data)); - - rtio_cqe_release(&test_rtio_ctx, cqe); -} - ZTEST(rtio_i2c, test_fallback_submit_rx) { uint8_t expected_buffer[] = {0x00, 0x01, 0x02}; @@ -237,7 +201,7 @@ ZTEST(rtio_i2c, test_fallback_transaction) phase1->flags |= RTIO_SQE_TRANSACTION; zassert_ok(rtio_submit(&test_rtio_ctx, 2)); - zassert_equal(2, blocking_emul_i2c_transfer_fake.call_count); + zassert_equal(1, blocking_emul_i2c_transfer_fake.call_count); struct rtio_cqe *cqe;