p8-i2c: Further timeout reworks

This patch reworks the way timeouts are set so that rather than imposing
a hard deadline based on the transaction length it uses a
kick-the-can-down-the-road approach where the timeout will be reset each
time data is written to or received from the master. This fits better
with the actual failure modes that timeouts are designed to handle, such
as unusually slow or broken devices.

Additionally this patch moves all the special case detection out of the
timeout handler. This is help to improve the robustness of the driver and
prepare for a more substantial rework of the driver as a whole later on.

Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
Signed-off-by: Stewart Smith <stewart@linux.vnet.ibm.com>

1 files changed, 75 insertions, 91 deletions

diff --git a/hw/p8-i2c.c b/hw/p8-i2c.c
index 0defe7b..ce56b26 100644
--- a/hw/p8-i2c.c
+++ b/hw/p8-i2c.c
@@ -190,6 +190,7 @@ struct p8_i2c_master {
 	struct lock		lock;		/* Lock to guard the members */
 	enum p8_i2c_master_type	type;		/* P8 vs. Centaur */
 	uint64_t		start_time;	/* Request start time */
+	uint64_t		last_update;
 	uint64_t		poll_interval;	/* Polling interval  */
 	uint64_t		byte_timeout;	/* Timeout per byte */
 	uint64_t		xscom_base;	/* xscom base of i2cm */
@@ -235,6 +236,17 @@ struct p8_i2c_request {
 
 static int occ_i2c_unlock(struct p8_i2c_master *master);
 
+
+static int64_t i2cm_read_reg(struct p8_i2c_master *m, int reg, uint64_t *val)
+{
+	return xscom_read(m->chip_id, m->xscom_base + reg, val);
+}
+
+static int64_t i2cm_write_reg(struct p8_i2c_master *m, int reg, uint64_t val)
+{
+	return xscom_write(m->chip_id, m->xscom_base + reg, val);
+}
+
 static void p8_i2c_print_debug_info(struct p8_i2c_master_port *port,
 				    struct i2c_request *req, uint64_t end_time)
 {
@@ -352,6 +364,17 @@ static int p8_i2c_enable_irqs(struct p8_i2c_master *master)
 	return rc;
 }
 
+static void p8_i2c_reset_timeout(struct p8_i2c_master *master,
+		struct i2c_request *req)
+{
+	struct p8_i2c_request *request;
+	uint64_t now = mftb();
+
+	master->last_update = now;
+	request = container_of(req, struct p8_i2c_request, req);
+	schedule_timer_at(&master->timeout, now + request->timeout);
+}
+
 static int p8_i2c_prog_watermark(struct p8_i2c_master *master)
 {
 	uint64_t watermark;
@@ -795,10 +818,13 @@ static void p8_i2c_status_data_request(struct p8_i2c_master *master,
 				 "state %d in data req !\n", master->state);
 		rc = OPAL_WRONG_STATE;
 	}
-	if (rc)
+
+	if (rc) {
 		p8_i2c_complete_request(master, req, rc);
-	else
+	} else {
 		p8_i2c_enable_irqs(master);
+		p8_i2c_reset_timeout(master, req);
+	}
 }
 
 static void p8_i2c_complete_offset(struct p8_i2c_master *master,
@@ -848,6 +874,7 @@ static void p8_i2c_complete_offset(struct p8_i2c_master *master,
 
 	/* Enable the interrupts */
 	p8_i2c_enable_irqs(master);
+	p8_i2c_reset_timeout(master, req);
 	return;
 
  complete:
@@ -886,72 +913,61 @@ static void p8_i2c_status_cmd_completion(struct p8_i2c_master *master,
 	p8_i2c_complete_request(master, req, rc);
 }
 
-static void  p8_i2c_check_status(struct p8_i2c_master *master, bool timeout)
+static void p8_i2c_check_status(struct p8_i2c_master *master)
 {
 	struct p8_i2c_master_port *port;
+	struct p8_i2c_request *request;
+	uint64_t status, deadline, now;
 	struct i2c_request *req;
-	uint64_t status, now = mftb();
 	int rc;
 
-	/* If we are idle, just return, we'll catch error conditions
-	 * when we next try to enqueue a request
+	/*
+	 * When idle or waiting for the occ to release the bus there's
+	 * nothing to check. Error states are handled when starting
+	 * a new request.
 	 */
 	if (master->state == state_idle || master->state == state_occache_dis)
 		return;
 
-	/* Read status register */
-	rc = xscom_read(master->chip_id, master->xscom_base + I2C_STAT_REG,
-			&status);
-	if (rc) {
-		log_simple_error(&e_info(OPAL_RC_I2C_TRANSFER), "I2C: Failed "
-				 "to read the STAT_REG\n");
+	/* A non-idle master should always have a pending request */
+	req = list_top(&master->req_list, struct i2c_request, link);
+	if (!req) {
+		prerror("I2C: Master is not idle and has no pending request\n");
 		return;
 	}
 
-	/* Nothing happened ? Go back */
-	if (!timeout && !(status & (I2C_STAT_ANY_ERR | I2C_STAT_DATA_REQ |
-			I2C_STAT_CMD_COMP)))
-		return;
-
-	DBG("Non-0 status: %016llx\n", status);
-
-	/* Mask the interrupts for this engine */
-	rc = xscom_write(master->chip_id, master->xscom_base + I2C_INTR_REG,
-			 ~I2C_INTR_ALL);
+	rc = i2cm_read_reg(master, I2C_STAT_REG, &status);
 	if (rc) {
-		log_simple_error(&e_info(OPAL_RC_I2C_TRANSFER), "I2C: Failed "
-				 "to disable the interrupts\n");
+		log_simple_error(&e_info(OPAL_RC_I2C_TRANSFER),
+			"I2C: Failed to read the STAT_REG\n");
 		return;
 	}
 
-	/* No request ? That's not normal ! Bail out without re-enabling
-	 * the interrupt
-	 */
-	req = list_top(&master->req_list, struct i2c_request, link);
-	if (req == NULL) {
+	/* Mask the interrupts for this engine */
+	rc = i2cm_write_reg(master, I2C_INTR_REG, ~I2C_INTR_ALL);
+	if (rc) {
 		log_simple_error(&e_info(OPAL_RC_I2C_TRANSFER),
-				 "I2C: Interrupt with no request"
-				 ", status=0x%016llx\n", status);
+			"I2C: Failed to disable the interrupts\n");
 		return;
 	}
 
 	/* Get port for current request */
 	port = container_of(req->bus, struct p8_i2c_master_port, bus);
+	now = mftb();
 
-	/* Handle the status in that order: errors, data requests and
-	 * command completion.
-	 */
-	if (status & I2C_STAT_ANY_ERR) {
-		/* Mask status to avoid some unrelated bit overwriting
-		 * our pseudo-status "timeout" bit 63
-		 */
+	request = container_of(req, struct p8_i2c_request, req);
+	deadline = master->last_update + request->timeout;
+
+	if (status & I2C_STAT_ANY_ERR)
 		p8_i2c_status_error(port, req, status & I2C_STAT_ANY_ERR, now);
-	} else if (status & I2C_STAT_DATA_REQ)
+	else if (status & I2C_STAT_DATA_REQ)
 		p8_i2c_status_data_request(master, req, status);
 	else if (status & I2C_STAT_CMD_COMP)
 		p8_i2c_status_cmd_completion(master, req, now);
-	else if (timeout)
+	else if (tb_compare(now, deadline) == TB_AAFTERB)
 		p8_i2c_status_error(port, req, I2C_STAT_PSEUDO_TIMEOUT, now);
+	else
+		p8_i2c_enable_irqs(master);
 }
 
 static int p8_i2c_check_initial_status(struct p8_i2c_master_port *port)
@@ -1129,8 +1145,8 @@ static int p8_i2c_start_request(struct p8_i2c_master *master,
 	struct p8_i2c_master_port *port;
 	struct p8_i2c_request *request =
 		container_of(req, struct p8_i2c_request, req);
-	uint64_t cmd, now, poll_interval;
-	int64_t rc, tbytes;
+	uint64_t cmd, poll_interval;
+	int64_t rc;
 
 	DBG("Starting req %d len=%d addr=%02x (offset=%x)\n",
 	    req->op, req->rw_len, req->dev_addr, req->offset);
@@ -1259,18 +1275,14 @@ static int p8_i2c_start_request(struct p8_i2c_master *master,
 		poll_interval = TIMER_POLL;
 	else
 		poll_interval = master->poll_interval;
-	now = schedule_timer(&master->poller, poll_interval);
+	schedule_timer(&master->poller, poll_interval);
 
-	/* Calculate and start timeout */
-	if (request->timeout) {
-		request->timeout += now;
-	} else {
-		tbytes = req->rw_len + req->offset_bytes + 2;
-		request->timeout = now + tbytes * master->byte_timeout;
-	}
+	/* If we don't have a user-set timeout then use the master's default */
+	if (!request->timeout)
+		request->timeout = master->byte_timeout;
 
 	/* Start the timeout */
-	schedule_timer_at(&master->timeout, request->timeout);
+	p8_i2c_reset_timeout(master, req);
 
 	return OPAL_SUCCESS;
 }
@@ -1313,7 +1325,7 @@ void p9_i2c_bus_owner_change(u32 chip_id)
 		master->state = state_idle;
 
 		p8_i2c_check_work(master);
-		p8_i2c_check_status(master, false);
+		p8_i2c_check_status(master);
 done:
 		unlock(&master->lock);
 	}
@@ -1388,7 +1400,7 @@ static uint64_t p8_i2c_run_request(struct i2c_request *req)
 	uint64_t poll_interval = 0;
 
 	lock(&master->lock);
-	p8_i2c_check_status(master, false);
+	p8_i2c_check_status(master);
 	p8_i2c_check_work(master);
 	poll_interval = master->poll_interval;
 	unlock(&master->lock);
@@ -1414,51 +1426,23 @@ static inline uint64_t p8_i2c_get_poll_interval(uint32_t bus_speed)
 	return usecs_to_tb(usec);
 }
 
-static void p8_i2c_timeout(struct timer *t __unused, void *data, uint64_t now)
+static void p8_i2c_timeout(struct timer *t __unused, void *data,
+		uint64_t __unused now)
 {
-	struct p8_i2c_master_port *port;
 	struct p8_i2c_master *master = data;
-	struct p8_i2c_request *request;
-	struct i2c_request *req;
 
 	lock(&master->lock);
 
-	/* This could be spurrious ... */
-	if (master->state == state_idle) {
-		DBG("I2C: Timeout in idle state\n");
-		goto exit;
-	}
-
-	/* We might still be spurrious timer, we need to ensure that the
-	 * head request is indeed old enough to be the one timing out
-	 */
-	req = list_top(&master->req_list, struct i2c_request, link);
-	if (req == NULL) {
-		DBG("I2C: Timeout with no"
-		    " pending request state=%d\n", master->state);
-		goto exit;
-	}
-	request = container_of(req, struct p8_i2c_request, req);
-	if (tb_compare(now, request->timeout) == TB_ABEFOREB) {
-		DBG("I2C: Timeout with request not expired\n");
-		goto exit;
-	}
-
-	request->timeout = 0ul;
-	port = container_of(req->bus, struct p8_i2c_master_port, bus);
-
-	DBG("timeout on c%de%d\n",
-		master->chip_id, master->engine_id);
+	DBG("timeout on c%de%d\n", master->chip_id, master->engine_id);
 
 	/*
-	 * Run through the usual path with timeout checking. The command might
-	 * have been completed successfully and we just lost an interrupt
-	 * somewhere.
+	 * Run through the usual status checks. It's possible to get spurious
+	 * timeouts due to races between the interrupt/poller paths and the
+	 * timeout handler. So we do all the checking, all the time.
 	 */
-	p8_i2c_check_status(port->master, true);
-	p8_i2c_check_work(port->master);
+	p8_i2c_check_status(master);
+	p8_i2c_check_work(master);
 
- exit:
 	unlock(&master->lock);
 }
 
@@ -1531,7 +1515,7 @@ static void p8_i2c_poll(struct timer *t __unused, void *data, uint64_t now)
 		return;
 
 	lock(&master->lock);
-	p8_i2c_check_status(master, false);
+	p8_i2c_check_status(master);
 	if (master->state != state_idle)
 		schedule_timer_at(&master->poller, now + master->poll_interval);
 	p8_i2c_check_work(master);
@@ -1553,7 +1537,7 @@ void p8_i2c_interrupt(uint32_t chip_id)
 		lock(&master->lock);
 
 		/* Run the state machine */
-		p8_i2c_check_status(master, false);
+		p8_i2c_check_status(master);
 
 		/* Check for new work */
 		p8_i2c_check_work(master);