Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
From:   Quan Nguyen <quan@os.amperecomputing.com>
To:     Brendan Higgins <brendan.higgins@linux.dev>,
        Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        Joel Stanley <joel@jms.id.au>,
        Andi Shyti <andi.shyti@kernel.org>,
        Andrew Jeffery <andrew@codeconstruct.com.au>,
        Wolfram Sang <wsa@kernel.org>,
        Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>,
        Guenter Roeck <linux@roeck-us.net>, linux-i2c@vger.kernel.org,
        openbmc@lists.ozlabs.org, linux-arm-kernel@lists.infradead.org,
        linux-aspeed@lists.ozlabs.org, linux-kernel@vger.kernel.org
Cc:     Cosmo Chou <chou.cosmo@gmail.com>,
        Open Source Submission <patches@amperecomputing.com>,
        Phong Vo <phong@os.amperecomputing.com>,
        "Thang Q . Nguyen" <thang@os.amperecomputing.com>,
        Quan Nguyen <quan@os.amperecomputing.com>
Subject: [PATCH v4 1/2] i2c: aspeed: Handle the coalesced stop conditions with the start conditions.
Date:   Mon, 11 Dec 2023 17:22:16 +0700
Message-Id: <20231211102217.2436294-2-quan@os.amperecomputing.com>
In-Reply-To: <20231211102217.2436294-1-quan@os.amperecomputing.com>
References: <20231211102217.2436294-1-quan@os.amperecomputing.com>
Content-Transfer-Encoding: 8bit
Content-Type: text/plain
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?x1oTkao/5yE5qMO2xfF8BCazMJJBkD0JYbdd1Jvoc5Nv9PHAWFMJB6r4Vszf?=
 =?us-ascii?Q?F7/kdXIlKmP4HUmN5guoDlkB6734m5MRg3SAsbRLeylpKKNNv0Y3iKLsD1oh?=
 =?us-ascii?Q?HpDeZZ7nM0XuS7Ndsk3Qi1Tvu8m9+NgzaDed2K8QEt7u2vE45xdaEa6T5KTW?=
 =?us-ascii?Q?YWtk45zLK9YqnLq6rLnHZoiHFF8qQI4hTOY4Lu9chmRmuCN17m1mjeTqIILd?=
 =?us-ascii?Q?u+c4JJeDOjlRPS16cuRVVJD5v/5y6bmBwuBcXgcAT9C2ciaTFfjPzp8vbsIb?=
 =?us-ascii?Q?op9boNcDGNsJ0hqgRir0htgxy0H7h/BClp+P8LG0Urt5u3Hn/dqBvDEn19Nq?=
 =?us-ascii?Q?qpCArfQO5bQY2468bZ9O/0j9mbDw3u23lP/ybjqbemsfE6q9z9rj7JpejjeH?=
 =?us-ascii?Q?8RmJaZ20K/EfgksmSIQ0qPREbAnwDRFgqYH6SgaYZbggumXO68CgqQJTnhKh?=
 =?us-ascii?Q?D0MdmwYr3I5NS9CXyYOKkVseuGXcXq8ZEeZamXHKM8KIOLDjRZL0rlh5fhSc?=
 =?us-ascii?Q?Z9weHmAy0TLJXx4SEJdDBgf+iCL2rv2yHJ1MOloh3FRHu7pmd95/e1lcyQXQ?=
 =?us-ascii?Q?Jb/hJ3uq7hM+++rs2PO9KhAvXQZbLwetS3L/LEE2ogfrk98xk35UT+PxVPNs?=
 =?us-ascii?Q?Fw4bWTRGxNqhUDtlTU+AUj6mBxzOc56JYIJ+HD0pEDkzoLn/6k66kHXPE3cf?=
 =?us-ascii?Q?ZnbtNwQ/eHK5iTjqxZn1yYL27kh09EKJqg27gLk8ZULEQrJP5WCPrOf4KJNi?=
 =?us-ascii?Q?Z9lFMumI6euA/Isuy62gYjNsrJ87o0Tsi7h8u/+9/4ory1K7/7uzGIFU17dA?=
 =?us-ascii?Q?mSB5hlgMdd+S217BAlUVb5oaN3mTw1bwq6mK86+zSWRvrCFMjGx6ycQnQt9C?=
 =?us-ascii?Q?CixrxTKe4hioJfT72gB/l+gtgJAxwb7cepz/sbrT0zaca5SnDnSF3MpDAIS6?=
 =?us-ascii?Q?f7imolJEH15fVyqq9Hn9TXNhu8cZPLkgd1EJy5FrJJ3tOd5Id/RR6nKJhKeA?=
 =?us-ascii?Q?RmHsAA7MSU7mFkCwLPkjPUa2zf20T8h0scJXlr+BHDVPkyJ/h9Pqnw1oTUFl?=
 =?us-ascii?Q?2X0RmAT28xBOC3ljxIooGq+e479eTXIpsEJhUis2xG4MHdEw+3yilWQDujUs?=
 =?us-ascii?Q?WI8Ks0Fi1J5pR2I1mBRKLteHkA4G7VDrbEpZIrAM+cqNX/cX6DCHVDDQChyv?=
 =?us-ascii?Q?79ZigOz5ZULgWmwZt6wyC/bVw+nJET/atF3xcFs9DDVay0Iupq74A8KhIvzn?=
 =?us-ascii?Q?AGVuY6R68jj+NuDxZr0TCcLfrHHdtMKXb310SaORife7LbBkbhNVcqCEmbFK?=
 =?us-ascii?Q?+Z1SavyofZmMTGsm7DxP/4evfLht0xSY6t/OBpBKRmQc5kBrZJyHvV1jrDyI?=
 =?us-ascii?Q?624IpchxDeBFVHWyjmcsGISWV+9mYkbRyyMoTevGc2aG7xHlrgWhRJTzA51l?=
 =?us-ascii?Q?CNIDKLlVz0NzjJgjIcTwGGrEapSNEYb8J3elHdK/TNOguRarA+cKVMhVR8uX?=
 =?us-ascii?Q?n/6kdMPPBONsXjM+s+2NzYhHxyibSd93xDXJgRWe3wkYb53uNcG+NP4GrlCH?=
 =?us-ascii?Q?M++kYwalRfNonX75Kh6zXQ/6TCrLDWTTMR4+540x5JEmYzUfCcnVEnFt+G4I?=
 =?us-ascii?Q?YDIfvErzao+zuC5fik2w6G8=3D?=
X-OriginatorOrg: os.amperecomputing.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 191937b4-53ef-4954-1d5f-08dbfa331cd8
X-MS-Exchange-CrossTenant-AuthSource: SN4PR01MB7455.prod.exchangelabs.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Dec 2023 10:22:43.9396
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3bc2b170-fd94-476d-b0ce-4229bdc904a7
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: IX2yGEacm58I0fwfDHy35K31N15yTecyJC47tebN7DsFXONPkrsc7w2UK7dK+WQP/qntc6g069VVNPAgjSuzsxEg5flu7gMK5Wdipkn8sYs=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR01MB6568
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,
	SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lipwig.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (lipwig.vger.email [0.0.0.0]); Mon, 11 Dec 2023 02:23:12 -0800 (PST)

Some masters may drive the transfers with low enough latency between
the nak/stop phase of the current command and the start/address phase
of the following command that the interrupts are coalesced by the
time we process them.
Handle the stop conditions before processing SLAVE_MATCH to fix the
complaints that sometimes occur below.

"aspeed-i2c-bus 1e78a040.i2c-bus: irq handled != irq. Expected
0x00000086, but was 0x00000084"

Fixes: f9eb91350bb2 ("i2c: aspeed: added slave support for Aspeed I2C driver")
Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au>
Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
---
v4:
  + Add an extra empty line before the comment                        [Andrew]

Note: I'd like to keep adding Reviewed-by Andrew and Andi for this patch
eventhough there is an extra empty line added before the comment as mentioned
above. Thanks for the review.

v3:
  + Change to handle the coalesced stop condition with the start
conditions                                                            [Andrew]
  + Revised commit message                                              [Quan]

v2:
  + Split to separate series                                            [Joel]
  + Added the Fixes line                                                [Joel]
  + Revised commit message                                              [Quan]

v1:
  + First introduced in
https://lore.kernel.org/all/20210519074934.20712-1-quan@os.amperecomputing.com/
---
 drivers/i2c/busses/i2c-aspeed.c | 48 ++++++++++++++++++++++-----------
 1 file changed, 32 insertions(+), 16 deletions(-)

diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index 28e2a5fc4528..5511fd46a65e 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -249,18 +249,46 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
 	if (!slave)
 		return 0;
 
-	command = readl(bus->base + ASPEED_I2C_CMD_REG);
+	/*
+	 * Handle stop conditions early, prior to SLAVE_MATCH. Some masters may drive
+	 * transfers with low enough latency between the nak/stop phase of the current
+	 * command and the start/address phase of the following command that the
+	 * interrupts are coalesced by the time we process them.
+	 */
+	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;
+	}
+
+	/* Propagate any stop conditions to the slave implementation. */
+	if (bus->slave_state == ASPEED_I2C_SLAVE_STOP) {
+		i2c_slave_event(slave, I2C_SLAVE_STOP, &value);
+		bus->slave_state = ASPEED_I2C_SLAVE_INACTIVE;
+	}
 
-	/* Slave was requested, restart state machine. */
+	/*
+	 * Now that we've dealt with any potentially coalesced stop conditions,
+	 * address any start conditions.
+	 */
 	if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
 		irq_handled |= ASPEED_I2CD_INTR_SLAVE_MATCH;
 		bus->slave_state = ASPEED_I2C_SLAVE_START;
 	}
 
-	/* Slave is not currently active, irq was for someone else. */
+	/*
+	 * If the slave has been stopped and not started then slave interrupt
+	 * handling is complete.
+	 */
 	if (bus->slave_state == ASPEED_I2C_SLAVE_INACTIVE)
 		return irq_handled;
 
+	command = readl(bus->base + ASPEED_I2C_CMD_REG);
 	dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
 		irq_status, command);
 
@@ -279,17 +307,6 @@ 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;
-	}
-
 	switch (bus->slave_state) {
 	case ASPEED_I2C_SLAVE_READ_REQUESTED:
 		if (unlikely(irq_status & ASPEED_I2CD_INTR_TX_ACK))
@@ -324,8 +341,7 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
 		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;
+		/* Stop event handling is done early. Unreachable. */
 		break;
 	case ASPEED_I2C_SLAVE_START:
 		/* Slave was just started. Waiting for the next event. */;
-- 
2.35.1