[Skiboot] [PATCH v2] hw/p8-i2c: Fix OCC locking

[Oliver O'Halloran]

There's a few issues with the Host<->OCC I2C bus handshaking. First up,
skiboot is currently examining the wrong bit when checking if the OCC
is currently using the bus. Secondly, when we need to wait for the OCC
to release the bus we are scheduling a recovery timer to run zero
timebase ticks after the current moment so the recovery timeout handler
will run immediately after the bus was requested, which will in turn
re-schedule itself, etc, etc. There's also a race between the OCC
interrupt and the recovery handler which can result in an assertion
failure in the recovery thread. All of this is bad.

This patch addresses all these issues and sets the recovery timeout to
10ms.

Signed-off-by: Oliver O'Halloran <oohall at gmail.com>
---
 hw/p8-i2c.c | 58 +++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 37 insertions(+), 21 deletions(-)
---
I've been using this script to test the changes. With this patch it's fairly
stable, but there's still intermittent data corruption issues when
when reading from the bus which I'm still investigating.
---
#!/bin/bash -e

while true;
do
	for f in $(find /sys/devices/platform/ -name '*-????'|grep a3000 )
	do
		bus="$(basename $f | awk -F - -- '{ print $1 }' )"
		dev="$(basename $f | awk -F - -- '{ print $2 }' )"

		i2cdump -y $bus 0x$dev i  > $bus-$dev
		md5sum $bus-$dev
		sleep 0.1
	done
done
---
diff --git a/hw/p8-i2c.c b/hw/p8-i2c.c
index f4666e24953e..a6274171debb 100644
--- a/hw/p8-i2c.c
+++ b/hw/p8-i2c.c
@@ -1063,8 +1063,11 @@ static int occ_i2c_lock(struct p8_i2c_master *master)
 
 	busflag = PPC_BIT(16 + (master->engine_id - 1) * 2);
 
-	DBG("occflags = %llx (locks = %.6llx)\n", (u64)occflags,
-	    GETFIELD(PPC_BITMASK(16, 22), occflags));
+	DBG("I2C: c%de%d: occflags = %llx (locks = %x:%x:%x)\n",
+		master->chip_id, master->engine_id, (u64) occflags,
+		(u32) GETFIELD(PPC_BITMASK(16, 17), occflags),
+		(u32) GETFIELD(PPC_BITMASK(18, 19), occflags),
+		(u32) GETFIELD(PPC_BITMASK(20, 21), occflags));
 
 	rc = xscom_write(master->chip_id, OCCFLG_SET, busflag);
 	if (rc) {
@@ -1073,8 +1076,11 @@ static int occ_i2c_lock(struct p8_i2c_master *master)
 	}
 
 	/* If the OCC also has this bus locked then wait for IRQ */
-	if (occflags & (busflag << 1))
+	if (occflags & (busflag >> 1)) {
+		DBG("I2C: c%de%d: Master in use by OCC\n",
+			master->chip_id, master->engine_id);
 		return 1;
+	}
 
 	master->occ_lock_acquired = true;
 
@@ -1098,8 +1104,8 @@ static int occ_i2c_unlock(struct p8_i2c_master *master)
 	busflag = PPC_BIT(16 + (master->engine_id - 1) * 2);
 
 	if (!(occflags & busflag)) {
-		prerror("I2C: busflag for %d already cleared (flags = %.16llx)",
-			master->engine_id, occflags);
+		DBG("I2C: spurious unlock for c%de%d already cleared (flags = %.16llx)",
+			master->chip_id, master->engine_id, occflags);
 	}
 
 	rc = xscom_write(master->chip_id, OCCFLG_CLEAR, busflag);
@@ -1161,7 +1167,7 @@ static int p8_i2c_start_request(struct p8_i2c_master *master,
 	if (rc > 0) {
 		/* Wait for OCC IRQ */
 		master->state = state_occache_dis;
-		schedule_timer(&master->recovery, rc);
+		schedule_timer(&master->recovery, msecs_to_tb(10));
 		return 0;
 	}
 
@@ -1281,29 +1287,29 @@ void p9_i2c_bus_owner_change(u32 chip_id)
 {
 	struct proc_chip *chip = get_chip(chip_id);
 	struct p8_i2c_master *master = NULL;
-	int rc;
 
 	assert(chip);
 	list_for_each(&chip->i2cms, master, link) {
-		if (master->state == state_idle  ||
-		    master->state != state_occache_dis)
-			continue;
-
 		lock(&master->lock);
 
+		/* spurious */
+		if (master->state != state_occache_dis)
+			goto done;
+
 		/* Can we now lock this master? */
-		rc = occ_i2c_lock(master);
-		if (rc) {
-			unlock(&master->lock);
-			continue;
-		}
+		if (occ_i2c_lock(master))
+			goto done;
 
-		/* Run the state machine */
-		p8_i2c_check_status(master);
+		/* clear the existing wait timer */
+		cancel_timer(&master->recovery);
+
+		/* re-start the request now that we own the master */
+		master->state = state_idle;
 
-		/* Check for new work */
 		p8_i2c_check_work(master);
+		p8_i2c_check_status(master);
 
+done:
 		unlock(&master->lock);
 	}
 }
@@ -1453,8 +1459,18 @@ static void p8_i2c_recover(struct timer *t __unused, void *data,
 	struct p8_i2c_master *master = data;
 
 	lock(&master->lock);
-	assert(master->state == state_recovery ||
-	       master->state == state_occache_dis);
+
+	/*
+	 * The recovery timer can race with the OCC interrupt. If the interrupt
+	 * comes in just before this is called, then we'll get a spurious
+	 * timeout which we need to ignore.
+	 */
+	if (master->state != state_recovery &&
+		master->state != state_occache_dis) {
+		unlock(&master->lock);
+		return;
+	}
+
 	master->state = state_idle;
 
 	/* We may or may not still have work pending, re-enable the sensor cache
-- 
2.9.5