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

cpu/nrf52 i2c: Always buffer writes #20298

Merged
merged 2 commits into from
Jan 25, 2024
Merged

cpu/nrf52 i2c: Always buffer writes #20298

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b80d1e6
cpu/nrf52 i2c: Always buffer writes
chrysn Jan 25, 2024
52a976e
cpu/nrf52 i2c: Add shortcut when data is in RAM
chrysn Jan 25, 2024
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
35 changes: 34 additions & 1 deletion cpu/nrf5x_common/periph/i2c_nrf52_nrf9160.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@
* 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:
* <https://devzone.nordicsemi.com/f/nordic-q-a/66615/nrf52840-twim-how-to-write-multiple-buffers-without-repeated-start-condition>,

Check warning on line 25 in cpu/nrf5x_common/periph/i2c_nrf52_nrf9160.c

View workflow job for this annotation

GitHub Actions / static-tests 

line is longer than 100 characters
* 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).

Check warning on line 27 in cpu/nrf5x_common/periph/i2c_nrf52_nrf9160.c

View workflow job for this annotation

GitHub Actions / static-tests 

line is longer than 100 characters
* Due to this shortcoming in the hardware, any operations with I2C_NOSTART
* fail.
*
Expand Down Expand Up @@ -92,6 +92,14 @@
return i2c_config[dev].dev;
}

/**
* @brief Like i2c_write_bytes, but with the constraint (created by the
* hardware) that data is in RAM
*/
static int direct_i2c_write_bytes(i2c_t dev, uint16_t addr, const void *data,
size_t len,
uint8_t flags);

/**
* Block until the interrupt described by inten_success_flag or
* TWIM_INTEN_ERROR_Msk fires.
Expand Down Expand Up @@ -262,7 +270,7 @@
}

memcpy(&tx_buf[reg_addr_len], data, len);
int ret = i2c_write_bytes(dev, addr, tx_buf, reg_addr_len + len, flags);
int ret = direct_i2c_write_bytes(dev, addr, tx_buf, reg_addr_len + len, flags);

/* Release tx_buf */
mutex_unlock(&buffer_lock);
Expand Down Expand Up @@ -331,6 +339,31 @@

int i2c_write_bytes(i2c_t dev, uint16_t addr, const void *data, size_t len,
uint8_t flags)
{
if ((unsigned int)data >= CPU_RAM_BASE && (unsigned int)data < CPU_RAM_BASE + CPU_RAM_SIZE) {
Copy link
Contributor

@kaspar030 kaspar030 Jan 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is done a bit simpler for spi:

RIOT/cpu/nrf5x_common/periph/spi_nrf52_nrf9160.c

Line 60 in 1073df2

return ((uint32_t)data & RAM_MASK);

Would that work here, too? Is that too relaxed for SPI?

Copy link
Contributor

Choose a reason for hiding this comment

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

... embassy-nrf does this:

const SRAM_LOWER: usize = 0x2000_0000;
const SRAM_UPPER: usize = 0x3000_0000;
ptr >= SRAM_LOWER && (ptr + len * core::mem::size_of::<T>()) < SRAM_UPPER

Copy link
Member Author

Choose a reason for hiding this comment

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

The SPI version is probably good enough, we could unify. The embassy version is IMO excessive, as the slice is a single object, and objects don't span memmapping areas.

return direct_i2c_write_bytes(dev, addr, data, len, flags);
}

/* These are critical for the memcpy; direct_i2c_write_bytes makes some
benpicco marked this conversation as resolved.
Show resolved Hide resolved
* more */
assert((len > 0) && (len < 256));

/* Lock tx_buf */
mutex_lock(&buffer_lock);

memcpy(tx_buf, data, len);

int result = direct_i2c_write_bytes(dev, addr, tx_buf, len, flags);

/* Release tx_buf */
mutex_unlock(&buffer_lock);

return result;
}

int direct_i2c_write_bytes(i2c_t dev, uint16_t addr, const void *data,
size_t len,
uint8_t flags)
{
assert((dev < I2C_NUMOF) && data && (len > 0) && (len < 256));

Expand Down
Loading