Re: [PATCH RFC 2/3] hw/i2c: add mctp core

[Jeremy Kerr <jk@codeconstruct.com.au>]

Hi Klaus,

> With those changes, I can get control protocol going, and multi-
> packet messages work.

Ah, I also needed a change to the aspeed I2C driver, as I'm seeing
the interrupt handler being invoked with both a stop and a start event
pending.

Patch below; if this seems sensible I will propose upstream.

Cheers,


Jeremy

---
From 6331cf2499c182606979029d2aaed93ee3e544fa Mon Sep 17 00:00:00 2001
From: Jeremy Kerr <jk@codeconstruct.com.au>
Date: Fri, 18 Nov 2022 14:04:50 +0800
Subject: [PATCH] i2c: aspeed: Allow combined STOP & START irqs

Currently, if we enter our interrupt handler with both a stop and start
condition pending, the stop event handler will override the current
state, so we'll then lose the start condition.

This change handles the stop condition before anything else, which means
we can then restart the state machine on any pending start state.

Because of this, we no longer need the ASPEED_I2C_SLAVE_STOP state,
because we're just reverting directly to INACTIVE.

I have only seen this condition on emulation; it may be impossible to
hit on real HW.

Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
---
 drivers/i2c/busses/i2c-aspeed.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index 185dedfebbac..45f2766b2b66 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -135,7 +135,6 @@ enum aspeed_i2c_slave_state {
 	ASPEED_I2C_SLAVE_READ_PROCESSED,
 	ASPEED_I2C_SLAVE_WRITE_REQUESTED,
 	ASPEED_I2C_SLAVE_WRITE_RECEIVED,
-	ASPEED_I2C_SLAVE_STOP,
 };
 
 struct aspeed_i2c_bus {
@@ -250,6 +249,14 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
 
 	command = readl(bus->base + ASPEED_I2C_CMD_REG);
 
+	/* handle a stop before starting any new events */
+	if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE &&
+	    irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
+		irq_handled |= ASPEED_I2CD_INTR_NORMAL_STOP;
+		i2c_slave_event(slave, I2C_SLAVE_STOP, &value);
+		bus->slave_state = ASPEED_I2C_SLAVE_INACTIVE;
+	}
+
 	/* Slave was requested, restart state machine. */
 	if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
 		irq_handled |= ASPEED_I2CD_INTR_SLAVE_MATCH;
@@ -278,15 +285,11 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
 		irq_handled |= ASPEED_I2CD_INTR_RX_DONE;
 	}
 
-	/* Slave was asked to stop. */
-	if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
-		irq_handled |= ASPEED_I2CD_INTR_NORMAL_STOP;
-		bus->slave_state = ASPEED_I2C_SLAVE_STOP;
-	}
 	if (irq_status & ASPEED_I2CD_INTR_TX_NAK &&
 	    bus->slave_state == ASPEED_I2C_SLAVE_READ_PROCESSED) {
 		irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
-		bus->slave_state = ASPEED_I2C_SLAVE_STOP;
+		i2c_slave_event(slave, I2C_SLAVE_STOP, &value);
+		bus->slave_state = ASPEED_I2C_SLAVE_INACTIVE;
 	}
 
 	switch (bus->slave_state) {
@@ -316,13 +319,11 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
 	case ASPEED_I2C_SLAVE_WRITE_RECEIVED:
 		i2c_slave_event(slave, I2C_SLAVE_WRITE_RECEIVED, &value);
 		break;
-	case ASPEED_I2C_SLAVE_STOP:
-		i2c_slave_event(slave, I2C_SLAVE_STOP, &value);
-		bus->slave_state = ASPEED_I2C_SLAVE_INACTIVE;
-		break;
 	case ASPEED_I2C_SLAVE_START:
 		/* Slave was just started. Waiting for the next event. */;
 		break;
+	case ASPEED_I2C_SLAVE_INACTIVE:
+		break;
 	default:
 		dev_err(bus->dev, "unknown slave_state: %d\n",
 			bus->slave_state);
-- 
2.35.1