[RFC PATCH] i2c: mvtwsi: reinitialize controller to clear bus errors

[Sam Edwards]

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?

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
 	 * data bytes */
 	status = i2c_begin(twsi, MVTWSI_STATUS_START, (chip << 1), tick);
-- 
2.39.2