From b91161dfcec02efd5e6692c0af7aaae5c0d79849 Mon Sep 17 00:00:00 2001 From: chrysn Date: Mon, 22 Jan 2024 01:46:34 +0100 Subject: [PATCH 1/2] cpu/nrf5x i2c: Set up correct interrupts after NOSTOP transmissions --- cpu/nrf5x_common/periph/i2c_nrf52_nrf9160.c | 35 ++++++++++++++++----- 1 file changed, 28 insertions(+), 7 deletions(-) diff --git a/cpu/nrf5x_common/periph/i2c_nrf52_nrf9160.c b/cpu/nrf5x_common/periph/i2c_nrf52_nrf9160.c index 436fe7f9348b..d028573b33df 100644 --- a/cpu/nrf5x_common/periph/i2c_nrf52_nrf9160.c +++ b/cpu/nrf5x_common/periph/i2c_nrf52_nrf9160.c @@ -69,11 +69,23 @@ static inline NRF_TWIM_Type *bus(i2c_t dev) return i2c_config[dev].dev; } -static int finish(i2c_t dev) +/** + * Block until the interrupt described by inten_success_flag or + * TWIM_INTEN_ERROR_Msk fires. + * + * Allowed values for inten_success_flag are + * * TWIM_INTEN_STOPPED_Msk (when a stop condition is to be set and the short + * circuit will pull TWIM into the stopped condition) + * * TWIM_INTEN_LASTTX_Msk (when sending without a stop condition) + * * TWIM_INTEN_LASTRX_Msk (when reading without a stop condition) + * + * Any addition needs to be added to the mask in i2c_isr_handler. + */ +static int finish(i2c_t dev, int inten_success_flag) { - DEBUG("[i2c] waiting for STOPPED or ERROR event\n"); + DEBUG("[i2c] waiting for success (STOPPED/LAST.X) or ERROR event\n"); /* Unmask interrupts */ - bus(dev)->INTENSET = TWIM_INTEN_STOPPED_Msk | TWIM_INTEN_ERROR_Msk; + bus(dev)->INTENSET = inten_success_flag | TWIM_INTEN_ERROR_Msk; mutex_lock(&busy[dev]); if ((bus(dev)->EVENTS_STOPPED)) { @@ -248,16 +260,19 @@ int i2c_read_bytes(i2c_t dev, uint16_t addr, void *data, size_t len, bus(dev)->RXD.PTR = (uint32_t)data; bus(dev)->RXD.MAXCNT = (uint8_t)len; + int inten_success_flag; if (!(flags & I2C_NOSTOP)) { bus(dev)->SHORTS = TWIM_SHORTS_LASTRX_STOP_Msk; + inten_success_flag = TWIM_INTEN_STOPPED_Msk; } else { bus(dev)->SHORTS = 0; + inten_success_flag = TWIM_INTEN_LASTRX_Msk; } /* Start transmission */ bus(dev)->TASKS_STARTRX = 1; - return finish(dev); + return finish(dev, inten_success_flag); } int i2c_read_regs(i2c_t dev, uint16_t addr, uint16_t reg, @@ -285,15 +300,18 @@ int i2c_read_regs(i2c_t dev, uint16_t addr, uint16_t reg, bus(dev)->TXD.PTR = (uint32_t)® bus(dev)->RXD.PTR = (uint32_t)data; bus(dev)->RXD.MAXCNT = (uint8_t)len; + + int inten_success_flag = TWIM_INTEN_LASTRX_Msk; bus(dev)->SHORTS = (TWIM_SHORTS_LASTTX_STARTRX_Msk); if (!(flags & I2C_NOSTOP)) { bus(dev)->SHORTS |= TWIM_SHORTS_LASTRX_STOP_Msk; + inten_success_flag = TWIM_INTEN_STOPPED_Msk; } /* Start transfer */ bus(dev)->TASKS_STARTTX = 1; - return finish(dev); + return finish(dev, inten_success_flag); } int i2c_write_bytes(i2c_t dev, uint16_t addr, const void *data, size_t len, @@ -309,15 +327,18 @@ int i2c_write_bytes(i2c_t dev, uint16_t addr, const void *data, size_t len, bus(dev)->ADDRESS = addr; bus(dev)->TXD.PTR = (uint32_t)data; bus(dev)->TXD.MAXCNT = (uint8_t)len; + int inten_success_flag; if (!(flags & I2C_NOSTOP)) { bus(dev)->SHORTS = TWIM_SHORTS_LASTTX_STOP_Msk; + inten_success_flag = TWIM_INTEN_STOPPED_Msk; } else { bus(dev)->SHORTS = 0; + inten_success_flag = TWIM_INTEN_LASTTX_Msk; } bus(dev)->TASKS_STARTTX = 1; - return finish(dev); + return finish(dev, inten_success_flag); } void i2c_isr_handler(void *arg) @@ -325,7 +346,7 @@ void i2c_isr_handler(void *arg) i2c_t dev = (i2c_t)(uintptr_t)arg; /* Mask interrupts to ensure that they only trigger once */ - bus(dev)->INTENCLR = TWIM_INTEN_STOPPED_Msk | TWIM_INTEN_ERROR_Msk; + bus(dev)->INTENCLR = TWIM_INTEN_STOPPED_Msk | TWIM_INTEN_ERROR_Msk | TWIM_INTEN_LASTTX_Msk | TWIM_INTEN_LASTRX_Msk; mutex_unlock(&busy[dev]); } From fce82dec6eaf3dbadc53abeeb974588feaab66e4 Mon Sep 17 00:00:00 2001 From: chrysn Date: Mon, 22 Jan 2024 23:29:24 +0100 Subject: [PATCH 2/2] cpu/nrf5x i2c: Forbid receive NOSTOP, document rationale --- cpu/nrf5x_common/periph/i2c_nrf52_nrf9160.c | 53 +++++++++++++-------- 1 file changed, 34 insertions(+), 19 deletions(-) diff --git a/cpu/nrf5x_common/periph/i2c_nrf52_nrf9160.c b/cpu/nrf5x_common/periph/i2c_nrf52_nrf9160.c index d028573b33df..8f420deba33d 100644 --- a/cpu/nrf5x_common/periph/i2c_nrf52_nrf9160.c +++ b/cpu/nrf5x_common/periph/i2c_nrf52_nrf9160.c @@ -19,6 +19,29 @@ * @author Hauke Petersen * @author Dylan Laduranty * + * As this implementation is based on the nRF5x TWIM peripheral, it can not + * issue a read following a read (or a write following a write) without + * creating a (repeated) start condition: + * , + * backed also by later experiments discussed in the [Rust embedded + * channel](https://matrix.to/#/!BHcierreUuwCMxVqOf:matrix.org/$JwNejRaeJx_tvqKgS88GenDG8ZNHrkTW09896dIehQ8?via=matrix.org&via=catircservices.org&via=tchncs.de). + * Due to this shortcoming in the hardware, any operations with I2C_NOSTART + * fail. + * + * Relatedly, the successful termination of a read or write can not be detected + * by an interrupt (only the eventual STOPPED condition after the event + * short-circuiting of LASTTX/LASTRX to STOP triggers one). There are LASTTX / + * LASTRX interrupts, but while the LASTTX is sensible enough (the last byte + * has been read, is being written, the caller may now repurpose the buffers), + * the LASTRX interrupt fires at the start of the last byte reading, and the + * user can not reliably know when the last byte was written (at least not + * easily). Therefore, reads with I2C_NOSTOP are not supported. + * + * In combination, these still allow the typical I2C operations: A single + * write, and a write (selecting a register) followed by a read, as well as + * stand-alone reads. More complex patterns are not supported; in particular, + * scatter-gather reads or writes are not possible. + * * @} */ @@ -77,13 +100,15 @@ static inline NRF_TWIM_Type *bus(i2c_t dev) * * TWIM_INTEN_STOPPED_Msk (when a stop condition is to be set and the short * circuit will pull TWIM into the stopped condition) * * TWIM_INTEN_LASTTX_Msk (when sending without a stop condition) - * * TWIM_INTEN_LASTRX_Msk (when reading without a stop condition) + * + * (TWIM_INTEN_LASTRX_Msk makes no sense here because that interrupt fires + * before the data is ready). * * Any addition needs to be added to the mask in i2c_isr_handler. */ static int finish(i2c_t dev, int inten_success_flag) { - DEBUG("[i2c] waiting for success (STOPPED/LAST.X) or ERROR event\n"); + DEBUG("[i2c] waiting for success (STOPPED/LASTTX) or ERROR event\n"); /* Unmask interrupts */ bus(dev)->INTENSET = inten_success_flag | TWIM_INTEN_ERROR_Msk; mutex_lock(&busy[dev]); @@ -251,7 +276,7 @@ int i2c_read_bytes(i2c_t dev, uint16_t addr, void *data, size_t len, { assert((dev < I2C_NUMOF) && data && (len > 0) && (len < 256)); - if (flags & (I2C_NOSTART | I2C_ADDR10)) { + if (flags & (I2C_NOSTART | I2C_ADDR10 | I2C_NOSTOP)) { return -EOPNOTSUPP; } DEBUG("[i2c] read_bytes: %i bytes from addr 0x%02x\n", (int)len, (int)addr); @@ -261,14 +286,8 @@ int i2c_read_bytes(i2c_t dev, uint16_t addr, void *data, size_t len, bus(dev)->RXD.MAXCNT = (uint8_t)len; int inten_success_flag; - if (!(flags & I2C_NOSTOP)) { - bus(dev)->SHORTS = TWIM_SHORTS_LASTRX_STOP_Msk; - inten_success_flag = TWIM_INTEN_STOPPED_Msk; - } - else { - bus(dev)->SHORTS = 0; - inten_success_flag = TWIM_INTEN_LASTRX_Msk; - } + bus(dev)->SHORTS = TWIM_SHORTS_LASTRX_STOP_Msk; + inten_success_flag = TWIM_INTEN_STOPPED_Msk; /* Start transmission */ bus(dev)->TASKS_STARTRX = 1; @@ -280,7 +299,7 @@ int i2c_read_regs(i2c_t dev, uint16_t addr, uint16_t reg, { assert((dev < I2C_NUMOF) && data && (len > 0) && (len < 256)); - if (flags & (I2C_NOSTART | I2C_ADDR10)) { + if (flags & (I2C_NOSTART | I2C_ADDR10 | I2C_NOSTOP)) { return -EOPNOTSUPP; } @@ -301,12 +320,8 @@ int i2c_read_regs(i2c_t dev, uint16_t addr, uint16_t reg, bus(dev)->RXD.PTR = (uint32_t)data; bus(dev)->RXD.MAXCNT = (uint8_t)len; - int inten_success_flag = TWIM_INTEN_LASTRX_Msk; - bus(dev)->SHORTS = (TWIM_SHORTS_LASTTX_STARTRX_Msk); - if (!(flags & I2C_NOSTOP)) { - bus(dev)->SHORTS |= TWIM_SHORTS_LASTRX_STOP_Msk; - inten_success_flag = TWIM_INTEN_STOPPED_Msk; - } + int inten_success_flag = TWIM_INTEN_STOPPED_Msk; + bus(dev)->SHORTS = TWIM_SHORTS_LASTTX_STARTRX_Msk | TWIM_SHORTS_LASTRX_STOP_Msk; /* Start transfer */ bus(dev)->TASKS_STARTTX = 1; @@ -346,7 +361,7 @@ void i2c_isr_handler(void *arg) i2c_t dev = (i2c_t)(uintptr_t)arg; /* Mask interrupts to ensure that they only trigger once */ - bus(dev)->INTENCLR = TWIM_INTEN_STOPPED_Msk | TWIM_INTEN_ERROR_Msk | TWIM_INTEN_LASTTX_Msk | TWIM_INTEN_LASTRX_Msk; + bus(dev)->INTENCLR = TWIM_INTEN_STOPPED_Msk | TWIM_INTEN_ERROR_Msk | TWIM_INTEN_LASTTX_Msk; mutex_unlock(&busy[dev]); }