From 6dd5973670a44cb1faa1b8a8f526d5a6f77b6693 Mon Sep 17 00:00:00 2001 From: Matt Rossouw Date: Fri, 13 Sep 2024 12:20:43 +1000 Subject: [PATCH] Implemented timeout handler for I2C driver Signed-off-by: Matt Rossouw --- drivers/i2c/meson/i2c.c | 92 +++++++++++++++++++++++++++-------------- 1 file changed, 62 insertions(+), 30 deletions(-) diff --git a/drivers/i2c/meson/i2c.c b/drivers/i2c/meson/i2c.c index 904ad9e64..e02799e40 100644 --- a/drivers/i2c/meson/i2c.c +++ b/drivers/i2c/meson/i2c.c @@ -365,6 +365,38 @@ static inline uint8_t i2c_token_convert(i2c_token_t token) } } +/** + * Return current buffer to sender and reset interface state for next transaction. + */ +static inline void i2c_return_and_reset(void) +{ + volatile struct i2c_regs *regs = (volatile struct i2c_regs *)i2c_regs; + LOG_DRIVER("returning to virtualiser!\n"); + LOG_DRIVER("curr_response_len : 0x%lx, curr_request_len : 0x%lx, return address is 0x%lx\n", + i2c_ifState.curr_response_len, i2c_ifState.curr_request_len, i2c_ifState.addr); + LOG_DRIVER("enqueueing response with size: %d\n\n", i2c_ifState.curr_response_len + RESPONSE_DATA_OFFSET); + + // response length is + 2 (RESPONSE_DATA_OFFSET = 2) because of the error tokens at the start + int ret = i2c_enqueue_response(queue_handle, i2c_ifState.addr, + (size_t) i2c_ifState.curr_data - DATA_REGIONS_START, + i2c_ifState.curr_response_len + RESPONSE_DATA_OFFSET); + if (ret) { + LOG_DRIVER_ERR("Failed to enqueue response\n"); + } + + // reset driver state for another request + // load tokens resets the interface registers so no need here + i2c_ifState.curr_response_len = 0; + i2c_ifState.curr_data = NULL; + i2c_ifState.curr_request_len = 0; + i2c_ifState.remaining = 0; + i2c_ifState.addr = 0; + + microkit_notify(VIRTUALISER_CH); + + i2c_halt(regs); // stop condition +} + /** * Loads tokens onto specified i2c interface registers * This function resets interface registers before uploading data @@ -566,16 +598,38 @@ static inline void handle_request(void) } } -/* Right now we do not do anything to handle a response timeout, even though we should - * be. More investigation is required. - * We observed very frequent timeout IRQs in our example system using an I2C card reader, - * we believe the source of this problem is a misconfiguration with the clocks, and will - * hopefully be resovled once we have an actual clock driver. - * See https://github.com/au-ts/sddf/issues/212 for more details. - */ static void handle_response_timeout(void) { LOG_DRIVER("handling timeout IRQ\n"); + + /* A timeout IRQ notionally occurs when a device has successfully ACKed a transaction + * but stops responding sometime thereafter while we are awaiting an R/W operation. + * The danger is that the device has not actually died and partially receives the + * transaction and we give up on it too quickly, leaving the device in a potentially + * incoherent state. + * + * Unfortunately, the datasheet describes nothing about this IRQ so it is hard to reason + * about the true conditions that give rise to it. As a result, we aim to abort the + * transaction gracefully and allow the client application to recover instead. + */ + + // IRQs still arrive when nothing is happening! + // @mattr: since the behaviour of this IRQ is unknown, this handler may be a vector for + // bugs in the future. If you ever find transactions being terminated with no + // clear electrical reason, inspect this! + if (i2c_ifState.curr_data) { + volatile struct i2c_regs *regs = (volatile struct i2c_regs *)i2c_regs; + + // Get state + uint8_t curr_token = 0; + uint8_t bytes_read = 0; + bool write_error = i2c_get_error(regs, &bytes_read, &curr_token); + i2c_token_t *return_buffer = i2c_ifState.curr_data; + + return_buffer[RESPONSE_ERR] = I2C_ERR_TIMEOUT; + // Token that caused the error + return_buffer[RESPONSE_ERR_TOKEN] = curr_token; + } return; } @@ -639,29 +693,7 @@ static void handle_response(void) // If request is completed or there was an error, return data to server and notify. if (write_error || !i2c_ifState.remaining) { - LOG_DRIVER("request completed or error, hence returning response to server\n"); - LOG_DRIVER("curr_response_len : 0x%lx, curr_request_len : 0x%lx, return address is 0x%lx\n", - i2c_ifState.curr_response_len, i2c_ifState.curr_request_len, i2c_ifState.addr); - LOG_DRIVER("enguing response with size: %d\n\n", i2c_ifState.curr_response_len + RESPONSE_DATA_OFFSET); - // response length is + 2 (RESPONSE_DATA_OFFSET = 2) because of the error tokens at the start - int ret = i2c_enqueue_response(queue_handle, i2c_ifState.addr, - (size_t) i2c_ifState.curr_data - DATA_REGIONS_START, - i2c_ifState.curr_response_len + RESPONSE_DATA_OFFSET); - if (ret) { - LOG_DRIVER_ERR("Failed to enqueue response\n"); - } - - // reset driver state for another request - // load tokens resets the interface registers so no need here - i2c_ifState.curr_response_len = 0; - i2c_ifState.curr_data = NULL; - i2c_ifState.curr_request_len = 0; - i2c_ifState.remaining = 0; - i2c_ifState.addr = 0; - - microkit_notify(VIRTUALISER_CH); - - i2c_halt(regs); // stop condition + i2c_return_and_reset(); } // If the driver was notified while this transaction was in progress, immediately start working on the next one.