[I2C, hal, SW] Restructured and refactored the functions#535
[I2C, hal, SW] Restructured and refactored the functions#535KinzaQamar wants to merge 5 commits into
Conversation
KinzaQamar
force-pushed
the
i2c_sw_modif
branch
8 times, most recently
from
c431f00 to
a0269e1
Compare
KinzaQamar
force-pushed
the
i2c_sw_modif
branch
8 times, most recently
from
49e9862 to
ab7f1dc
Compare
|
Tested 
|
KinzaQamar
force-pushed
the
i2c_sw_modif
branch
2 times, most recently
from
f9b5397 to
ff319ad
Compare
KinzaQamar
force-pushed
the
i2c_sw_modif
branch
2 times, most recently
from
26efa97 to
6224074
Compare
|
Hi @engdoreis, I've addresses your comment. Meanwhile, testing the change I found out that a loop back test can't happen when the bytes we want to read are greater than FIFO_DEPTH |
KinzaQamar
force-pushed
the
i2c_sw_modif
branch
2 times, most recently
from
2cf67af to
00dc426
Compare
| // Transmits a single byte to the target | ||
| void i2c_write_byte(i2c_t i2c, uint8_t addr, uint8_t data); | ||
| // Transmits multiple bytes to the target | ||
| void i2c_write_n_bytes(i2c_t i2c, uint8_t addr, const uint8_t *data, uint8_t num_wr_bytes); | ||
|
|
||
| // Receive a single byte from the target | ||
| void i2c_read_byte(i2c_t i2c, uint8_t addr); | ||
| // Receive n bytes from the target | ||
| void i2c_read_n_bytes(i2c_t i2c, uint8_t addr, uint8_t num_rd_bytes); |
There was a problem hiding this comment.
Would prefer these to be called i2c_write_bytes and i2c_read_bytes, and the length parameter should just be called len.
There was a problem hiding this comment.
I wanted an n in b/w to indicate that the function can read / write one byte as well.
There was a problem hiding this comment.
Hmm, I think I'd go with your suggestion. As n_bytes and only bytes are same meaning
There was a problem hiding this comment.
and the length parameter should just be called len
I wanted to call it with a _bytes suffix to be explicit that these are the number of bytes to read and write. I can change it to num_bytes or len_bytes. Does that look sensible to you?
| // Check the overflow condition on every byte which is a multiple of 64. | ||
| if (overflow) { | ||
| i2c_status i2c_status_reg = VOLATILE_READ(i2c->status); | ||
|
|
||
| // Wait until FMT FIFO has some space | ||
| while (i2c_status_reg & i2c_status_fmtfull) { | ||
| i2c_status_reg = VOLATILE_READ(i2c->status); | ||
| } | ||
| } |
There was a problem hiding this comment.
I think there is a bug here - it could be that after writing 64 bytes, the FIFO isn't full but is also not empty, so another 64 bytes would be written before checking the FIFO status again, which would overflow it. It's much more robust and simple to check whether the FIFO is full before writing each byte.
There was a problem hiding this comment.
Hmm, that's interesting. I wanted to only check an overflow when there is a chance of overflow. I2C controller is fast enough to drain the FIFO so we will never get it full. But your comment is right that when the SW is fast and controller is slow, this check will lead to data loss. Thanks @ziuziakowska
KinzaQamar
force-pushed
the
i2c_sw_modif
branch
4 times, most recently
from
05b27ab to
0f8b757
Compare
| // Reset the FMT FIFO as a precautionary step in case something goes wrong when controller's FSM | ||
| // is halted and the SW didn't manage to clear the FIFO during that scenario. | ||
| i2c_fifo_ctrl fifo_ctrl_reg = { .fmtrst = 1u }; | ||
| VOLATILE_WRITE(i2c->fifo_ctrl, fifo_ctrl_reg); |
There was a problem hiding this comment.
@elliotb-lowrisc Do we still need to keep this precautionary step before the start of every read and write no that we are clearing on errors?
|
Comments addressed ! |
Taking the transfer checking part out from the i2c_read_byte() and i2c_write_byte() functions. It is needed when you want to controller wants to read / write multiple bytes. Signed-off-by: Kinza Qamar <kqzaman@lowrisc.org>
This is suggested in programmer's guide. As a precautionary step, reset the FMT fifo before sending any transfer Signed-off-by: Kinza Qamar <kqzaman@lowrisc.org>
Signed-off-by: Kinza Qamar <kqzaman@lowrisc.org>
Signed-off-by: Kinza Qamar <kqzaman@lowrisc.org>
…depth Signed-off-by: Kinza Qamar <kqzaman@lowrisc.org>
4 participants
Taking the transfer checking part out from the i2c_read_byte() and i2c_write_byte() functions. It is needed when you want to controller wants to read / write multiple bytes.
Merge #545 first as a dependency on this PR