Hello Sam,
On 10.06.23 08:15, Sam Edwards wrote:
Hi I²C maintainers,
My target has the following devices sharing one bus:
- 24C02 EEPROM
- Realtek 8370 Ethernet switch
- Allwinner T113-s3 (running U-Boot, interfacing via MVTWSI)
The RTL8370 is configured in "EEPROM autoload" mode, so on reset it will load the full contents of the EEPROM. During this sequence, it does an odd move where it sends a re-start, stop, pulses scl low, and then a fresh start.
Something about this sequence (I'm betting the scl pulse after stop) upsets the MVTWSI controller, causing it to retreat into state 0x00, which the documentation for my chip names "bus error." I'd guess this is a feature for slave operation: in slave mode, the controller FSM is completely at the mercy of the bus, and so a misbehaving/glitching bus can result in essentially a random walk through the states. Rather than allow that (and risk a potentially dangerous accidental write), the controller goes to a failsafe "bus error" state and remains there, effectively shutting down the whole controller.
However, in master mode (which U-Boot uses exclusively), this feature is a nuisance. We do not care what another master was doing on the bus previously, as long as it is in the proper idle state when we want to use it. We also don't care if the bus error was our fault in a previous transaction, since the error would have been reported at that time. I reckon that it makes sense to check for this "bus error" state at the beginning of each new read/write and clear it if detected.
Unfortunately, I couldn't find any way to coax the FSM out of the error state just by poking at the control register. It's possible I didn't look hard enough (I'm willing to try other things), but I'm otherwise left with only the obvious way out: a reset. Since that also resets the baud and address registers, I have to save and restore those too.
Attached here is my RFC patch (which DOES resolve my problem), for feedback and suggestions on what I might try differently, as I'm not sure whether or not I like this approach:
- I would be happier if the code did a fresh init instead of saving and restoring register state, but this driver is plumbed in a way that the config struct isn't readily accessible at the low level.
- I don't really like having to duplicate the state check in the read and write functions; is there anything I can do that's more DRY?
- Avoiding a reset would be nice, ideally avoiding the "bus error" state altogether by disabling the error detection somehow.
Thoughts?
I have not the deep knowledge of this specific i2c driver, but may also an option is to set
int (*deblock)(struct udevice *bus);
in
static const struct dm_i2c_ops mvtwsi_i2c_ops = {
for this driver and do there the stuff needed to deblock the bus, as you have done in twsi_i2c_reinit() in your patch?
See as an example:
drivers/i2c/ast_i2c.c
Currently this deblock function is only used from i2c core in i2c command, but may it makes sense to add in dm_i2c_xfer function in
drivers/i2c/i2c-uclass.c
a call to i2c_deblock() in case bus is blocked?
bye, Heiko
Cheers, Sam
drivers/i2c/mvtwsi.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+)
diff --git a/drivers/i2c/mvtwsi.c b/drivers/i2c/mvtwsi.c index d088ea75b9..38a3bdade0 100644 --- a/drivers/i2c/mvtwsi.c +++ b/drivers/i2c/mvtwsi.c @@ -142,6 +142,8 @@ enum mvtwsi_ctrl_register_fields {
- code.
*/ enum mvstwsi_status_values {
- /* Protocol violation on bus; this is a terminal state */
- MVTWSI_BUS_ERROR = 0x00, /* START condition transmitted */ MVTWSI_STATUS_START = 0x08, /* Repeated START condition transmitted */
@@ -525,6 +527,36 @@ static void __twsi_i2c_init(struct mvtwsi_registers *twsi, int speed, #endif }
+/*
- __twsi_i2c_reinit() - Reset and reinitialize the I2C controller.
- This function should be called to get the MVTWSI controller out of the
- "bus error" state. It saves and restores the baud and address registers.
- @twsi: The MVTWSI register structure to use.
- @tick: The duration of a clock cycle at the current I2C speed.
- */
+static void __twsi_i2c_reinit(struct mvtwsi_registers *twsi, uint tick) +{
- uint baud;
- uint slaveadd;
- /* Save baud, address registers */
- baud = readl(&twsi->baudrate);
- slaveadd = readl(&twsi->slave_address);
- /* Reset controller */
- twsi_reset(twsi);
- /* Restore baud, address registers */
- writel(baud, &twsi->baudrate);
- writel(slaveadd, &twsi->slave_address);
- writel(0, &twsi->xtnd_slave_addr);
- /* Assert STOP, but don't care for the result */
- (void) twsi_stop(twsi, tick);
+}
/*
- i2c_begin() - Start a I2C transaction.
@@ -621,6 +653,11 @@ static int __twsi_i2c_read(struct mvtwsi_registers *twsi, uchar chip, int stop_status; int expected_start = MVTWSI_STATUS_START;
- /* Check for (and clear) a bus error from a previous failed transaction
* or another master on the same bus */- if (readl(&twsi->status) == MVTWSI_BUS_ERROR)
__twsi_i2c_reinit(twsi, tick);- if (alen > 0) { /* Begin i2c write to send the address bytes */ status = i2c_begin(twsi, expected_start, (chip << 1), tick);
@@ -668,6 +705,11 @@ static int __twsi_i2c_write(struct mvtwsi_registers *twsi, uchar chip, { int status, stop_status;
- /* Check for (and clear) a bus error from a previous failed transaction
* or another master on the same bus */- if (readl(&twsi->status) == MVTWSI_BUS_ERROR)
__twsi_i2c_reinit(twsi, tick);- /* Begin i2c write to send first the address bytes, then the
status = i2c_begin(twsi, MVTWSI_STATUS_START, (chip << 1), tick);
- data bytes */