From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 458EEC43334 for ; Fri, 22 Jul 2022 03:09:41 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=y9Vkg3sRhE/HbRLYz0w4YKSqFKTQZvwiZgoiztk3KD4=; b=AtoadGeCwNxUM7 hp+1gvLdCAZey0gnLljF/Eqq9zM8X0i+YyUqHxVh5jy8fKd2gw0/4a49PHazthlH5YelLDZ8qKppC tgiOTh6cJQI9lylPGYq1pU12upw5idwk8D7kWEozJXGuQkEf3nAA9TdX9SbMjLjaaHOTI6k766miN QmqgXQZPgmPoUNxE1/1a/DcMubuh3Dogi0JT4WLZ7lTF0B3NOIHK2Km4eeaH9GmxEzKXVovVK9jxP MM2yA6qodutKm+wuRTZqmsRNEzSo0vggD/bMel+uEu90gfesx+al53UEI01VKT0SFZmZyWi3Loc5I loxHp/1S6gUdd23wrq4Q==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oEj1m-00H6Z4-RI; Fri, 22 Jul 2022 03:08:34 +0000 Received: from new3-smtp.messagingengine.com ([66.111.4.229]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1oEj1j-00H6So-RF for linux-arm-kernel@lists.infradead.org; Fri, 22 Jul 2022 03:08:33 +0000 Received: from compute5.internal (compute5.nyi.internal [10.202.2.45]) by mailnew.nyi.internal (Postfix) with ESMTP id 412B2580E19; Thu, 21 Jul 2022 23:08:28 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute5.internal (MEProxy); Thu, 21 Jul 2022 23:08:28 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pjd.dev; h=cc:cc :content-type:date:date:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:sender:subject :subject:to:to; s=fm1; t=1658459308; x=1658462908; bh=uG2Rb1C0Ih Z7yNKCi2bEM3jWfHG2vQqfIOk05YbeD9M=; b=ogDLl2oDCCPHk3XNtlhynR8Zjk TDPmvhYleBdslcmUW/nckL9llYKjnYFGMPaWypTc5FNV6yurPxvTAEvqdC//bmtL z39ubYj+NoYOqP5Li6Xz8CTOP852Rf/8vVpMgHJEskQMpKxDb54vA4KeiqRoZXE4 HI0ShzQYBdYlhxEzWdq3lsOToCBL0SI0OTnJCdhLm7uwaJzZWAmM8ltySw6eQH+5 1CVy+YK29/M+u2plbMR8irXREM6sNgjYcZyQQsGl8uvoP0qtAyOHg0jZAmb3U4QW Kf4rbd5b+CAQ+ksp1pDEatCFLC51H7wibEdJLDkvz0povmJYV2rXV6FiEFqQ== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-type:date:date:feedback-id :feedback-id:from:from:in-reply-to:in-reply-to:message-id :mime-version:references:reply-to:sender:subject:subject:to:to :x-me-proxy:x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s= fm3; t=1658459308; x=1658462908; bh=uG2Rb1C0IhZ7yNKCi2bEM3jWfHG2 vQqfIOk05YbeD9M=; b=MolbHctsNv9+0mlUjQfoD9ynCHqeDnP1/IY1PIt31Rb4 VNqFSWAqbmldv57MoU2na54NhgE0Ci4GsUUBGxZBZN602VYdcJCKKYDNOyZgDZP+ ZSQ2D7IRtw6/JaBZxMd1nLV0eUdW6wMminPPBvAeC210pIKQFuWCi1Rv5qTMsjt7 DSrnaNkrHlsWpDWfxKr5ZF3T5gqVihuJ9xJMInQyjgzVDngdwRbGYCAD6WPXJm4U 2UyvTWVzTVX/YAqXPCtRtE9G1ENOwYnnk0WW8mdK11/iMHVDe/b84VE3io/l7oU+ t1TkIAdqSTJXxDEGRNkwvzjECfLr1p/xSrOtUh48AQ== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvfedrvddtuddgjedtucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucenucfjughrpeffhffvvefukfhfgggtuggjsehttd ertddttddvnecuhfhrohhmpefrvghtvghrucffvghlvghvohhrhigrshcuoehpvghtvghr sehpjhgurdguvghvqeenucggtffrrghtthgvrhhnpeduteeihfffleeuveekgedugfeffe ehtdeguefffffhleehgfduueejjeekfeeukeenucevlhhushhtvghrufhiiigvpedtnecu rfgrrhgrmhepmhgrihhlfhhrohhmpehpvghtvghrsehpjhgurdguvghv X-ME-Proxy: Feedback-ID: i9e814621:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Thu, 21 Jul 2022 23:08:26 -0400 (EDT) Date: Thu, 21 Jul 2022 20:08:24 -0700 From: Peter Delevoryas To: ryan_chen Cc: brendanhiggins@google.com, benh@kernel.crashing.org, joel@jms.id.au, andrew@aj.id.au, 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 Subject: Re: [PATCH] i2c: aspeed: Fix slave mode unexpected irq handler Message-ID: References: <20220531093140.28770-1-ryan_chen@aspeedtech.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20220531093140.28770-1-ryan_chen@aspeedtech.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220721_200832_017594_BDF98B92 X-CRM114-Status: GOOD ( 22.99 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, May 31, 2022 at 05:31:40PM +0800, ryan_chen wrote: > When i2c master send the new i2c transfer immediately > after stop. the i2c slave will see the stop and new > address match stage together. And it needs handle the > stop first. otherwise will occur unexpected handle > isr. I think it would be helpful if you could include some driver trace messages to indicate the sequence of events that lead you to make this change, like an example of the problem happening. > > Fixes: f327c686d3ba ("i2c: aspeed: added drover for Aspeed I2C) Typo on "drover"? Actually, slave support wasn't included in this commit. I think it should be: Fixes: f9eb91350bb2 ("i2c: aspeed: added slave support for Aspeed I2C driver") > Signed-off-by: ryan_chen > --- > drivers/i2c/busses/i2c-aspeed.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c > index 771e53d3d197..9f21e090ce47 100644 > --- a/drivers/i2c/busses/i2c-aspeed.c > +++ b/drivers/i2c/busses/i2c-aspeed.c > @@ -252,6 +252,12 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status) > > /* Slave was requested, restart state machine. */ > if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) { > + if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP && > + bus->slave_state == ASPEED_I2C_SLAVE_WRITE_RECEIVED) { > + irq_handled |= ASPEED_I2CD_INTR_NORMAL_STOP; > + irq_status &= ~ASPEED_I2CD_INTR_NORMAL_STOP; > + i2c_slave_event(slave, I2C_SLAVE_STOP, &value); > + } Ok, we might receive the STOP and START signals at the same time. So, we need to make sure that we handle the STOP first. [1] Why is this within the START case then? Can't we untangle this from the START handling? irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH means SLAVE START, right? We know we're in slave mode already because this is the slave IRQ handler. And we can access the state of the bus through bus->slave_state, to see if it's a spurious STOP or an expected one. It would also be nice to unify the STOP handling code too, to make sure this matches the normal STOP path that already exists below here. [1] Although if that's the case, it seems like we might need to handle the START first sometimes, depending on the current state, right? Can we have the reverse case, where we see a START and the matching STOP simultaneously? Perhaps that's already handled properly by the code since it's structured chronologically. > irq_handled |= ASPEED_I2CD_INTR_SLAVE_MATCH; > bus->slave_state = ASPEED_I2C_SLAVE_START; > } > -- > 2.17.1 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel