Open bjarki-andreasen opened 1 month ago
I believe this will cause certain helpers to be invalid, please review them
It would be nice to see a preliminary PR for introducing these changes. That would make it easier to judge the impact of the API changes (breakage, deprecations etc.).
One transaction per i2c_msg
buffer is IMO the way to go. Explicitly as some drivers are requiring it already.
One transaction per
i2c_msg
buffer is IMO the way to go. Explicitly as some drivers are requiring it already.
Would not that make it impossible to send RESTART
between messages, since each transaction will end with a STOP
condition?
One transaction per
i2c_msg
buffer is IMO the way to go. Explicitly as some drivers are requiring it already.Would not that make it impossible to send
RESTART
between messages, since each transaction will end with aSTOP
condition?
That’s not what I meant, A write followed by a restart and a read would be two transaction. Everything thats between a (Re-) Start and a Stop or Restart should be one i2c_msg
buffer. No splitting one write into two buffers
That’s not what I meant, A write followed by a restart and a read would be two transaction. Everything thats between a (Re-) Start and a Stop or Restart should be one
i2c_msg
buffer. No splitting one write into two buffers
Allowing reads and writes to be split between multiple i2c_msg
buffers can greatly simplify writing driver software. Imagine receving a pointer to buffer containing the date to be transferred, but having to prepend these data bytes with e.g. a command and address byte.
If forced to do this in on i2c_msg
buffer, one would have to allocate enough memory to hold both the command, address, and data bytes and copy the data into the buffer. Being able to do with in two buffers allows allocating a buffer just for the static command and address bytes and use a separate buffer for the data without any copying.
Allowing reads and writes to be split between multiple
i2c_msg
buffers can greatly simplify writing driver software. Imagine receving a pointer to buffer containing the date to be transferred, but having to prepend these data bytes with e.g. a command and address byte.If forced to do this in on
i2c_msg
buffer, one would have to allocate enough memory to hold both the command, address, and data bytes and copy the data into the buffer. Being able to do with in two buffers allows allocating a buffer just for the static command and address bytes and use a separate buffer for the data without any copying.
This is convenient for users of the API, but it is devastating for the I2C controller drivers, especially those supporting DMAs and/or not supporting leaving the bus in an idle state after a transfer.
If the i2c_msg
is complete, the DMA can point to the buffer, there will be only one .flags
config to read (RX, TX, RESTART), and the transfer can be started. It is even simple to chain together i2c_msg
for TXRX with intermittent restart for I2C controllers which support that. It enforces efficiency and proper transfer formatting, ensuring a STOP at the end after the last i2c_msg
as well (it is not possible to have a transfer which leaves the bus in the busy state)
If the i2c_msg
can be split, drivers need to iterate through all of them, figure out which can be bundled into a single transfer, if using a DMA, the TX i2c_msg
messages must be copied to a RAM buffer the DMA can point to. For receiving, the number of bytes to receive must be determined again by iterating over the i2c_msg
to figure out how much to receive, which if using a DMA means again using a RAM buffer for receiving. After receiving, the data must then be copied back into the "chained together" i2c_msg
messages.
IMO, the API should match the actual hardware capabilities, if we wanted to allow stitching together i2c_msg
, there should be a higher level utility library for doing this, which handles allocating RAM buffers, before passing it to the drivers.
That’s not what I meant, A write followed by a restart and a read would be two transaction. Everything thats between a (Re-) Start and a Stop or Restart should be one
i2c_msg
buffer. No splitting one write into two buffersAllowing reads and writes to be split between multiple
i2c_msg
buffers can greatly simplify writing driver software. Imagine receving a pointer to buffer containing the date to be transferred, but having to prepend these data bytes with e.g. a command and address byte.If forced to do this in on
i2c_msg
buffer, one would have to allocate enough memory to hold both the command, address, and data bytes and copy the data into the buffer. Being able to do with in two buffers allows allocating a buffer just for the static command and address bytes and use a separate buffer for the data without any copying.
Currently i2c controller drivers don't have to support this. So we have to ether have all i2c controller drivers support this, or remove that, because keeping it the way it is, is no option, as I would mean that some peripheral drivers can't be used with i2c controllers that don't support it.
If the i2c_msg can be split, drivers need to iterate through all of them, figure out which can be bundled into a single transfer, if using a DMA, the TX i2c_msg messages must be copied to a RAM buffer the DMA can point to. For receiving, the number of bytes to receive must be determined again by iterating over the i2c_msg to figure out how much to receive, which if using a DMA means again using a RAM buffer for receiving. After receiving, the data must then be copied back into the "chained together" i2c_msg messages.
So you are arguing against handling this complexity in a central place (the I2C controller driver) and arguing for leaving this complexity to be handled by every I2C peripheral driver instead?
@henrikbrixandersen the peripheral driver should be responseble . It can easily provide a longer buffer that's long enough. At later points we hat to limit the length to a fixed value or use malloc. Which both have its disadvantages
So you are arguing against handling this complexity in a central place (the I2C controller driver) and arguing for leaving this complexity to be handled by every I2C peripheral driver instead?
This complexity does not exist in the I2C peripheral driver :) They just work with simple uint8_t buffers, those are way simpler to format and allocate correctly :)
Looked through the tree to scope out the impact:
Not used
The in-tree changes are quite easily addressed. After they are addressed, I2C device drivers can be optimized to adhere to the harder requirements without any breaking changes in-tree :)
Scope
Looked through the tree to scope out the impact:
In tree usage of i2c_transfer() not in I2C or I3C subsystem
* mb85rcxx_i2c_write() -> essentially i2c_burst_write() * smbus_stm32_block_write() -> essentially i2c_burst_write(), but really malformed (no flags?)
Usage of i2c_burst_write() not in I2C or I3C subsystem
Not used
Overall impact
The in-tree changes are quite easily addressed. After they are addressed, I2C device drivers can be optimized to adhere to the harder requirements without any breaking changes in-tree :)
you should probably include i2c_burst_write_dt()
and i2c_transfer_dt()
as they are just wrappers.
Looked through the tree to scope out the impact:
Summary:
i2c_burst_write()
which must be updated to i2c_write()i2c_transfer()
used like i2c_burst_write() which must be updatedi2c_write_read()
i2c_write()
i2c_read()
Adapting entire tree to changes requires changing about 40 calls of i2c_burst_write() into a single call to i2c_write(), out of 250 calls to the I2C transfer API. The ds2477 is a an edge case which requires more investigation.
As an intermediate step, i2c_burst_write() could be extended to allocate a linear buffer, prepend the address, pass it to i2c_write(), unalloc after transfer.
Scope (updated to include _dt variants) Looked through the tree to scope out the impact:
Summary:
36 instances of
i2c_burst_write()
which must be updated to i2c_write()6 instances of
i2c_transfer()
used like i2c_burst_write() which must be updated~62 instances of
i2c_write_read()
~130 instances of
i2c_write()
25 instances
i2c_read()
Adapting entire tree to changes requires changing about 40 calls of i2c_burst_write() into a single call to i2c_write(), out of > 250 calls to the I2C transfer API. The ds2477 is a an edge case which requires more investigation.As an intermediate step, i2c_burst_write() could be extended to allocate a linear buffer, prepend the address, pass it to > > i2c_write(), unalloc after transfer.
i2c_transfer() and _dt() which needs fixing
mb85rcxx_i2c_write() -> i2c_burst_write()
smbus_stm32_block_write() -> i2c_burst_write()
i2c_burst_write16_dt() -> i2c_burst_write()
ov5640_write_reg() -> i2c_burst_write()
ov7725_write_reg() -> i2c_burst_write()
ds2477_85_write_block -> i2c_burst_write()
ds2477_85_read_block() -> doing some strange unsupported reads
ds2485_w1_script_cmd() -> read split into two messages, replace with i2c_read()
How many i2c drivers today have a problem with i2c_burst_write? My recollection is that it's predominantly a problem for Nordic and all others work fine with it, but I could very well be remembering incorrectly.
Quick grep of memcpy() in the i2c drivers...
target/eeprom_target.c
44: memcpy(data->buffer, eeprom_data, length);
184: memcpy(&data->buffer[data->buffer_idx], ptr + 1, len - 1);
i2c_handlers.c
38: memcpy(copy, msgs, num_msgs * sizeof(*msgs));
i2c_ene_kb1200.c
42: memcpy((void *)&config->fsmbm->FSMBMDAT[0],
73: memcpy((void *)&data->msg_buf[data->index], (void *)&config->fsmbm->FSMBMDAT[0],
132: memcpy((void *)&config->fsmbm->FSMBMDAT[0], (void *)&data->msg_buf[data->index],
i2c_nrfx_twim.c
107: memcpy(msg_buf + msg_buf_used,
190: memcpy(msgs[j].buf,
i2c_ite_enhance.c
1043: memcpy(target_buffer->out_buffer, rdata, len);
So ITE, Nordic, and ENE are doing some memcpying, perhaps to deal with burst write incompatibility or other reasons.
Pretty limited in which drivers have to deal with this sort of burst_write incompatibility at a very brief glance.
How many i2c drivers today have a problem with i2c_burst_write? My recollection is that it's predominantly a problem for Nordic and all others work fine with it, but I could very well be remembering incorrectly.
The Nordic TWIM does "support it", it just requires copying it into a linear buffer which can be used with a DMA, just like any other I2C controller which wishes to utilize a DMA for transfers. The changes in this RFC simply optimizes the transfers to be linear buffers of same direction (TX, RX), just like the I2C protocol, which are directly usable with DMAs.
How many i2c drivers today have a problem with i2c_burst_write? My recollection is that it's predominantly a problem for Nordic and all others work fine with it, but I could very well be remembering incorrectly.
The Nordic TWIM does "support it", it just requires copying it into a linear buffer which can be used with a DMA, just like any other I2C controller which wishes to utilize a DMA for transfers. The changes in this RFC simply optimizes the transfers to be linear buffers of same direction (TX, RX), just like the I2C protocol, which are directly usable with DMAs.
I don't believe that's the case but do please carefully review the i2c controller drivers. Many DMAs are perfectly fine supplying scatter gather requests (sourcing data from flash or wherever) with lists or chained together interrupts (dma transfer completion, software starts the next one). Nordic is sort of the odd duck out in these cases. The Nordic SPI driver is a pretty keen example of that. The only one I saw doing memcpy to deal with the DMA from Flash issue.
Edit: esp32 spim driver also does a memcpy into dma friendly memory.
How many i2c drivers today have a problem with i2c_burst_write? My recollection is that it's predominantly a problem for Nordic and all others work fine with it, but I could very well be remembering incorrectly. The Nordic TWIM does "support it", it just requires copying it into a linear buffer which can be used with a DMA, just like any other I2C controller which wishes to utilize a DMA for transfers. The changes in this RFC simply optimizes the transfers to be linear buffers of same direction (TX, RX), just like the I2C protocol, which are directly usable with DMAs.
I don't believe that's the case but do please carefully review the i2c controller drivers. Many DMAs are perfectly fine supplying scatter gather requests (from flash or wherever) with lists or chained together interrupts (dma transfer completion, software starts the next one). Nordic is sort of the odd duck out in these cases. The Nordic SPI driver is a pretty keen example of that. The only one I saw doing memcpy to deal with the DMA from Flash issue.
Edit: esp32 spim driver also does a memcpy into dma friendly memory.
I do not mean to distract from the main topic but is there a better way to handle DMA from Flash cases?
Architecture WG:
How many i2c drivers today have a problem with i2c_burst_write? My recollection is that it's predominantly a problem for Nordic and all others work fine with it, but I could very well be remembering incorrectly. The Nordic TWIM does "support it", it just requires copying it into a linear buffer which can be used with a DMA, just like any other I2C controller which wishes to utilize a DMA for transfers. The changes in this RFC simply optimizes the transfers to be linear buffers of same direction (TX, RX), just like the I2C protocol, which are directly usable with DMAs.
I don't believe that's the case but do please carefully review the i2c controller drivers. Many DMAs are perfectly fine supplying scatter gather requests (from flash or wherever) with lists or chained together interrupts (dma transfer completion, software starts the next one). Nordic is sort of the odd duck out in these cases. The Nordic SPI driver is a pretty keen example of that. The only one I saw doing memcpy to deal with the DMA from Flash issue. Edit: esp32 spim driver also does a memcpy into dma friendly memory.
I do not mean to distract from the main topic but is there a better way to handle DMA from Flash cases?
Transmitting any const value, those are typically stashed in flash
Hey, from the arch wg notes, the operation I bumped into issue with the auto-stop is an smbus read_block implementation. I know there's an smbus subsystem but that does not help at all, for the record smbus_stm32.c
implements smbus_stm32_block_write
using i2c_transfer
, and conveniently does not implement i2c_read
, but if it would it would be impacted by this problem. Anyway, I had this specific issue with reading smart battery strings, which does an smbus block read (https://docs.kernel.org/i2c/smbus-protocol.html#smbus-block-read) which implies variable length read depending one of the value that are read.
In my specific case I think I can split the read into two transaction, although that works by the assumption that the data being read does not change from a transaction to another. I think it's the case for my device but it may not be the case for all devices out there. If that's the case then the auto-stop thing would break those devices.
Found this in the wild as well https://forums.raspberrypi.com/viewtopic.php?t=236153 where someone referring to a https://www.nxp.com/docs/en/data-sheet/A71CH.pdf implies that they need to do the transfer in a single transaction as well, @henrikbrixandersen is that something similar to what you bumped into?
I'm holding https://github.com/zephyrproject-rtos/zephyr/pull/76654 for now in the off case that the original PR gets reverted.
@henrikbrixandersen and @ubieda suggests adding a new flag "do not add a stop condition"
funny that was my original proposed workaround :-)
As for the locking, how about having a "locked" bit in a common i2c driver data structure and track it in the transfer
callback based on the START
/STOP
flag on the messages? Then it would not be up to the device driver to do it.
Hey, from the arch wg notes, the operation I bumped into issue with the auto-stop is an smbus read_block implementation. I know there's an smbus subsystem but that does not help at all, for the record
smbus_stm32.c
implementssmbus_stm32_block_write
usingi2c_transfer
, and conveniently does not implementi2c_read
, but if it would it would be impacted by this problem. Anyway, I had this specific issue with reading smart battery strings, which does an smbus block read (https://docs.kernel.org/i2c/smbus-protocol.html#smbus-block-read) which implies variable length read depending one of the value that are read.In my specific case I think I can split the read into two transaction, although that works by the assumption that the data being read does not change from a transaction to another. I think it's the case for my device but it may not be the case for all devices out there. If that's the case then the auto-stop thing would break those devices.
If length is one of the values being read... and you know the maximum read length possible...
you could perhaps read the length directly into the next messages len value.
E.g.
uint8_t rx_buf[RX_MAX];
struct i2c_msg msgs[3] = {
{
.buf = reg_addr,
.len = sizeof(reg_addr),
.flags = I2C_MSG_WRITE,
},
{
.buf = &msgs[2].len,
.len = sizeof(msgs[2].len),
.flags = I2C_MSG_READ,
},
{
.buf = rx_buf,
.len = 0,
.flags = I2C_MSG_READ,
}
};
you could perhaps read the length directly into the next messages len value.
Mind blown :-) yeah looks brittle but it may work. That said, yes I know the maximum size but I'd still like to validate it before asking the i2c controller to write on the buffer.
Hey, from the arch wg notes, the operation I bumped into issue with the auto-stop is an smbus read_block implementation. I know there's an smbus subsystem but that does not help at all, for the record
smbus_stm32.c
implementssmbus_stm32_block_write
usingi2c_transfer
, and conveniently does not implementi2c_read
, but if it would it would be impacted by this problem. Anyway, I had this specific issue with reading smart battery strings, which does an smbus block read (https://docs.kernel.org/i2c/smbus-protocol.html#smbus-block-read) which implies variable length read depending one of the value that are read.In my specific case I think I can split the read into two transaction, although that works by the assumption that the data being read does not change from a transaction to another. I think it's the case for my device but it may not be the case for all devices out there. If that's the case then the auto-stop thing would break those devices.
Found this in the wild as well https://forums.raspberrypi.com/viewtopic.php?t=236153 where someone referring to a https://www.nxp.com/docs/en/data-sheet/A71CH.pdf implies that they need to do the transfer in a single transaction as well, @henrikbrixandersen is that something similar to what you bumped into?
I'm holding #76654 for now in the off case that the original PR gets reverted.
@henrikbrixandersen and @ubieda suggests adding a new flag "do not add a stop condition"
funny that was my original proposed workaround :-)
Yes but your workaround leaves the bus unlocked and available to be reused by any thread/execution context that pre-empts the current one trying to use the i2c controller. This leaves every user of zephyr open for a race condition on the i2c bus where undefined behavior would be very easy to hit.
To do this correctly the thread wishing to do logic between i2c_transfer calls needs to hold a lock and release a lock on the bus only when its done, in some manner.
Yes but your workaround leaves the bus unlocked and available to be reused by any thread/execution context that pre-empts the current one trying to use the i2c controller. This leaves every user of zephyr open for a race condition on the i2c bus where undefined behavior would be very easy to hit.
Yes, sorry wasn't meaning to imply that that was "correct", just that it seems like the problem is more complex than initially thought.
Yes but your workaround leaves the bus unlocked and available to be reused by any thread/execution context that pre-empts the current one trying to use the i2c controller. This leaves every user of zephyr open for a race condition on the i2c bus where undefined behavior would be very easy to hit.
Yes, sorry wasn't meaning to imply that that was "correct", just that it seems like the problem is more complex than initially thought.
Well if you want to do this portably, the only portable option would be a bitbanged i2c driver. Not all hardware (Nordic) supports this sort of thing.
We likely will need some Kconfigs signaling to the application what is/isn't supported by each driver with the change you and @henrikbrixandersen are looking for.
Well if you want to do this portably, the only portable option would be a bitbanged i2c driver. Not all hardware (Nordic) supports this sort of thing.
And I took that personally :D
Well if you want to do this portably, the only portable option would be a bitbanged i2c driver. Not all hardware (Nordic) supports this sort of thing.
And I took that personally :D
Right, an abstraction is only useful here if the the behavior is uniform. Otherwise whats the point, may as well #ifdef around hal APIs.
Yeah the portability issue is the issue I suppose, Linux has a "functionality" thing https://elixir.bootlin.com/linux/v6.10.3/source/include/uapi/linux/i2c.h#L91, sort of cover this by putting the read block functionality there.
If length is one of the values being read... and you know the maximum read length possible...
you could perhaps read the length directly into the next messages len value.
E.g.
uint8_t rx_buf[RX_MAX];
struct i2c_msg msgs[3] = { { .buf = reg_addr, .len = sizeof(reg_addr), .flags = I2C_MSG_WRITE, }, { .buf = &msgs[2].len, .len = sizeof(msgs[2].len), .flags = I2C_MSG_READ, }, { .buf = rx_buf, .len = 0, .flags = I2C_MSG_READ, } };
But the proposal in this RFC is to disallow splitting sequential reads into multiple Rx buffers, which would render the above non-compliant as well?
If length is one of the values being read... and you know the maximum read length possible... you could perhaps read the length directly into the next messages len value. E.g. uint8_t rx_buf[RX_MAX]; struct i2c_msg msgs[3] = { { .buf = reg_addr, .len = sizeof(reg_addr), .flags = I2C_MSG_WRITE, }, { .buf = &msgs[2].len, .len = sizeof(msgs[2].len), .flags = I2C_MSG_READ, }, { .buf = rx_buf, .len = 0, .flags = I2C_MSG_READ, } };
But the proposal in this RFC is to disallow splitting sequential reads into multiple Rx buffers, which would render the above non-compliant as well?
That's right. It would. You'd need to read the len, then in a separate transfer read the data to be fully portable. I don't think you could do this sort of thing on a nordic twim controller
This RFC is definately not the solution it seems :) but hopefully it spawns a better solution :)
One more interesting datapoint, my use case was really an implementation of smbus block read, I just realized that the STM32 driver we have under smbus is lacking that implementation: https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/smbus/smbus_stm32.c#L269
Wondering if that's because it was impossible to implement it with a single transaction since the stm32 i2c driver always implemented the "last message stop feature".
Introduction
The I2C API's
i2c_transfer()
and async variants are currently loosely specified, leading to inconsistent and inefficient device driver behavior and usage. This RFC aims to update thei2c_transfer()
to be unambiguous, portable and consistent. This will ensure applications/subsystems/drivers using the I2C API work with all in-tree I2C controllers.Proposed
i2c_transfer()
specification enhancementWe will go through the existing specification step by step and modying it. For completeness, here is the existing specification:
The following allows calling the transfer API with
num_msgs = 0
, while still enforcingmsgs != NULL
.This has no utility, and should be considered invalid, thus shall be removed.
The following allows splitting single transfers into multiple i2c_msg structs. For example, a TXRX transfer with 4 bytes to transmit, followed by a restart, followed by receiving 50 bytes, with a stop at the end, can be "validly" split into 54 separate
struct i2c_msg
.This is not reflective of the I2C protocol, which consists of blocks of single direction transfers which are started by a start condition, and ended with a repeated start or stop condition, with the last transfer enforcing a stop condition See I2C spec from NXP page 13 to 14.
An argument for doing it this way is giving the application the flexibility of using
struct i2c_msg
arrays as simpleuint8_t buf[]
arrays, which the device driver then has to identify, validate, and stitch together into an actual complete transfer. An issue with this argument is clearly specified: "_(multiple consecutivei2c_msg
buffers all configured forI2C_MSG_WRITE
) may be packed into a single transaction by some drivers, but others may emit each fragment as a distinct write transaction, which will not produce the same behavior._". We should not allow such ambiguity when there indeed is a specification enforcing a consistent way of formatting I2C transfers.We should enforce the formatting of the transfer to match the protocol exactly, which will greatly reduce the complexity of device drivers, and improve I2C transfer efficiency.
This will enforce complete, single, compliant transfers with every call to
i2c_transfer()
, leaving the bus in the idle state, allowing for proper bus arbitration.The following allows a transfer to end with the bus in the busy state, which is not conformant to the spec and likely for that reason not possible with all I2C controllers.
This shall be removed, as it is replaced by the explicit requirement to set the flag.
The
struct i2c_msg *msgs
are currently modifiable by the device drivers, which may alter/correct them before transmission.The
struct i2c_msg *msgs
should be made const, and validated by thei2c_transfer()
wrapper. With strong requirements, we can ensure the entire transfer is valid before being passed to the device drivers.For completeness, the following is the enhanced I2C API spec:
Consequent changes
i2c_transfer_cb()
andi2c_transfer_signal()
must be updated with aconst struct i2c_msg *msgs
.i2c_burst_write()
must be removed as it is not compliant with thei2c_transfer()
API.i2c_write()
must be used instead.struct i2c_msg
shall have its now inaccurate@note
removed.Alternate solutions
With the
struct i2c_msg *msgs
ofi2c_transfer()
being mutable, the wrapper could modify transfer before passing it to the device driver, which at least ensured the transfer would end with the bus in the idle state (see #75801). This however revealed that some downstream users where relying on the non conformant behavior of some device drivers to not follow this part of the specification (see #76654).Additional context
This enhancement is a result of work on updating in-tree i2c device drivers and adding support for new I2C controller silicon (like the nRF TWIM) which are designed to conform to the I2C protocol specification. The in-tree inconsistent driver behavior and loose I2C API specification allows for operations which don't conform to the I2C protocol specification, or map quite poorly to it, leading to, at best, really complex hacks having to be implemented in device drivers to match what users expect to be possible, at worst, making the hardware incompatible with zephyr.