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 94CF1C4167B for ; Fri, 8 Dec 2023 04:00:58 +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:MIME-Version:References:In-Reply-To: Date:Cc:To:From:Subject:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=PcIg36FtnTEVcAvPj98l8wCCNQdemLNUvxQnDraYu8c=; b=WrX832wJa7wJLf cMhh0p5zEcSv87SziTqmmV83l/fqXVmhp+nbVcoNb280jig+IG5Mu7xhcmxGS07Hcp+xFGHkxnr9F ND74fi8r3/Clh9onaTFGccD7Vv+woxbgo0wMOatBjDaBQERzztXbnjKF9dYmRARDGVlKC3N/HrfJN PPtOlliWx7vgx2rxtPb4HrWdL++xI3JuMV0F1NNznGZ1lsU5QMcrNFcmtWxwwKqDhMEucQpZNlk+U mWy3WmN2LE70ckdYe7Pm3YQAsdmyk0E7ckM7O87G/mh5yUTgCscr2+vTB2N6wJjSYVqAxYd09LnsC xNxXti3sXjOBpauJe7tQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1rBS2K-00EeEn-1f; Fri, 08 Dec 2023 04:00:24 +0000 Received: from pi.codeconstruct.com.au ([203.29.241.158] helo=codeconstruct.com.au) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1rBS2H-00EeEL-1Z for linux-arm-kernel@lists.infradead.org; Fri, 08 Dec 2023 04:00:23 +0000 Received: from [192.168.68.112] (ppp118-210-181-59.adl-adc-lon-bras34.tpg.internode.on.net [118.210.181.59]) by mail.codeconstruct.com.au (Postfix) with ESMTPSA id A9E6D20016; Fri, 8 Dec 2023 12:00:18 +0800 (AWST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=codeconstruct.com.au; s=2022a; t=1702008019; bh=6g1gg4ODYQHLBaloxEIVH+B88/pEH5RLfLJX0MQ0iPk=; h=Subject:From:To:Cc:Date:In-Reply-To:References; b=kuPvuuYQxgat5rpGJsEfXAPhzGJuzhJ2gy1NNdBDywTsALSlK+y5+LJJrfM67Cf/N pWYRPPWFtmBhxmcueYYwlJgeflZkijiXUyk57+6+ccSB9MpLhmEUFob3tvyTWOp2qY cfe1lzES+DvX3b8f3u0kq84I/X4t6z/R4g+IFy0GJoBGzEbe5ogR/jAVyzbz42UPsc XMdVxIGceJzbhow79nOdKbD1F87BbNHz+VSuPg/Ox/zfaSp9UgWK/cpYUj19stXq8Y sh5ezvwsWrbuJg8B9kHzOIiHqTwRMbqKMbx2WF3RZVNsDcjS3FAB6ZCIPMyvqARxGh HgJy677d6fiYA== Message-ID: Subject: Re: [PATCH v3 2/2] i2c: aspeed: Acknowledge Tx done with and without ACK irq late From: Andrew Jeffery To: Quan Nguyen , Brendan Higgins , Benjamin Herrenschmidt , Joel Stanley , Andi Shyti , Wolfram Sang , Jae Hyun Yoo , Guenter Roeck , 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 , Open Source Submission , Phong Vo , "Thang Q . Nguyen" Date: Fri, 08 Dec 2023 14:30:18 +1030 In-Reply-To: <20231208033142.1673232-3-quan@os.amperecomputing.com> References: <20231208033142.1673232-1-quan@os.amperecomputing.com> <20231208033142.1673232-3-quan@os.amperecomputing.com> User-Agent: Evolution 3.46.4-2 MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20231207_200021_750534_020EC528 X-CRM114-Status: GOOD ( 20.07 ) 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 Fri, 2023-12-08 at 10:31 +0700, Quan Nguyen wrote: > Commit 2be6b47211e1 ("i2c: aspeed: Acknowledge most interrupts early in > interrupt handler") acknowledges most interrupts early before the slave > irq handler is executed, except for the "Receive Done Interrupt status" > which is acknowledged late in the interrupt. > However, it has been observed that the early acknowledgment of "Transmit > Done Interrupt Status" (with ACK or NACK) often causes the interrupt to > be raised in READ REQUEST state, that shows the > "Unexpected ACK on read request." complaint messages. > > Assuming that the "Transmit Done" interrupt should only be acknowledged > once it is truly processed, this commit fixes that issue by acknowledging > interrupts for both ACK and NACK cases late in the interrupt handler. > > Fixes: 2be6b47211e1 ("i2c: aspeed: Acknowledge most interrupts early in interrupt handler") > Signed-off-by: Quan Nguyen > --- > v3: > + Fix the unconditinal write when ack the irqs [Andrew] > + Refactor the code to enhance code readability [Quan] > + Fix grammar in commit message [Quan] > > v2: > + Split to separate series [Joel] > + Added the Fixes line [Joel] > + Fixed multiline comment [Joel] > + Refactor irq clearing code [Joel, Guenter] > + Revised commit message [Joel] > + Revised commit message [Quan] > + About a note to remind why the readl() should immediately follow the > writel() to fix the race condition when clearing irq status from commit > c926c87b8e36 ("i2c: aspeed: Avoid i2c interrupt status clear race > condition"), I think it looks straight forward in this patch and decided > not to add that note. [Joel] > > v1: > + First introduced in > https://lore.kernel.org/all/20210519074934.20712-1-quan@os.amperecomputing.com/ > --- > drivers/i2c/busses/i2c-aspeed.c | 21 +++++++++++++-------- > 1 file changed, 13 insertions(+), 8 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c > index 1c2a4f4c4e1b..967a26dd4ffa 100644 > --- a/drivers/i2c/busses/i2c-aspeed.c > +++ b/drivers/i2c/busses/i2c-aspeed.c > @@ -617,13 +617,19 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status) > static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) > { > struct aspeed_i2c_bus *bus = dev_id; > - u32 irq_received, irq_remaining, irq_handled; > + u32 irq_received, irq_remaining, irq_handled, irq_ack_last; `irq_ack_last` might be better as a macro, but you're probably saved by the optimiser anyway. If there's another reason to do a v4 or others are unhappy with it then consider fixing it, otherwise: Reviewed-by: Andrew Jeffery Thanks. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel