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 0CD87C4167B for ; Wed, 29 Nov 2023 00:34:16 +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=Fc7ucy14V45DoGxGDfTO65F3XSmqrWEH9dRVfsjN5MA=; b=Em7Zs81e3Y6UJQ uLgszcdDGmtArzESlWqCiPIPO9kGPCS5ydu1Y6Xjcd+sadxSuCgJ8YhR6PYLn+8EXqpLPIXVgaSy1 WXUYH8CRUU6mIaMBJjwsWxD+/lT88up5xwMN+jJYbeA1bWXfq/06Qf+OmgYQkrR+vSRFkRhGN0Yog T84JUQlG2Wblut0KWkd5zPcTk8SpNhniirZTdTogGIJXpNFaxK4YWWSc+I0rlSxIt6Ys8XEmrbWc7 p/IX2HjDA7ucOU3kDgJp/FAOIkBcdJD2pn1fn6vaALmH0gHzT0CWZnvMIxPkoCB0jM8FCrxySMm6X Ra5gI593NU4dV3qJiNBw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1r88WR-006gcu-35; Wed, 29 Nov 2023 00:33:47 +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 1r88WN-006gcX-2d for linux-arm-kernel@lists.infradead.org; Wed, 29 Nov 2023 00:33:45 +0000 Received: from [192.168.68.112] (ppp118-210-131-38.adl-adc-lon-bras33.tpg.internode.on.net [118.210.131.38]) by mail.codeconstruct.com.au (Postfix) with ESMTPSA id 6FE6820173; Wed, 29 Nov 2023 08:33:40 +0800 (AWST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=codeconstruct.com.au; s=2022a; t=1701218021; bh=+WS89WTcKCHrEbe/yOsqOJI9VuSu2zv7xlF8oc8yqX8=; h=Subject:From:To:Cc:Date:In-Reply-To:References; b=CdESjDWDMLB0K5t8mldNMDgWHYXdJNIbYBo3NdZ1sS/jPkBR89TnoURcGR2EYdlR7 wVNeuiHIGYL2kLxm/iCtC9vqiCl+b1xiVZT3SH4drgvnGWPIju/Za6bgwGaR/csEXw /ixxbHNVHCMAkWj2GoqKqRqgbG55eUJeZTlKfppfdvYwEuNpSTKSfQ3icgM3LCecD4 A/I2d2gN1K6PwUaDKuHC9kRAS0ZBq39GCdAG2364XZ8FqLqhpWSHZCzLXvbI9YeIqK Y8YJAWcf8oAVAZXN3ujiXJdqs5+EiXXMihjpEfKV2E0za7c0n+GTpjo6gCmMx9iGOr rwNhDHXWrs4UQ== Message-ID: <2186c3b9ac92f03c68e8a2dd9fda871f80a6d664.camel@codeconstruct.com.au> Subject: Re: [PATCH v2 RESEND 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: Wed, 29 Nov 2023 11:03:39 +1030 In-Reply-To: <20231128075236.2724038-3-quan@os.amperecomputing.com> References: <20231128075236.2724038-1-quan@os.amperecomputing.com> <20231128075236.2724038-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-20231128_163344_280602_940A4FAB X-CRM114-Status: GOOD ( 26.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, 2023-11-28 at 14:52 +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 is 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, resulting in "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 this issue by acknowledging > this interrupt for both ACK and NACK cases late in the interrupt handler > also. > > Fixes: 2be6b47211e1 ("i2c: aspeed: Acknowledge most interrupts early in interrupt handler") > Signed-off-by: Quan Nguyen > --- > 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 | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c > index 79476b46285b..3231f430e335 100644 > --- a/drivers/i2c/busses/i2c-aspeed.c > +++ b/drivers/i2c/busses/i2c-aspeed.c > @@ -611,8 +611,9 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) > > spin_lock(&bus->lock); > irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG); > - /* Ack all interrupts except for Rx done */ > - writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE, > + /* Ack all interrupts except for Rx done and Tx done with/without ACK */ I'm not a huge fan of this comment, it just says what the code does. It would be much better to explain *why* the code does what it does. I realise describing what the code does was already the gist of the comment and that you're just updating it to match the change to the code, but that's my entire problem with it. We'd be better off deleting it if we're not going to explain why the masking is necessary. > + writel(irq_received & > + ~(ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_TX_ACK | ASPEED_I2CD_INTR_TX_NAK), > bus->base + ASPEED_I2C_INTR_STS_REG); > readl(bus->base + ASPEED_I2C_INTR_STS_REG); > irq_received &= ASPEED_I2CD_INTR_RECV_MASK; > @@ -657,12 +658,12 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) > "irq handled != irq. expected 0x%08x, but was 0x%08x\n", > irq_received, irq_handled); > > - /* Ack Rx done */ > - if (irq_received & ASPEED_I2CD_INTR_RX_DONE) { > - writel(ASPEED_I2CD_INTR_RX_DONE, > - bus->base + ASPEED_I2C_INTR_STS_REG); > - readl(bus->base + ASPEED_I2C_INTR_STS_REG); > - } > + /* Ack Rx done and Tx done with/without ACK */ > + writel(irq_received & > + (ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_TX_ACK | ASPEED_I2CD_INTR_TX_NAK), > + bus->base + ASPEED_I2C_INTR_STS_REG); > + readl(bus->base + ASPEED_I2C_INTR_STS_REG); I'm not sure why the write was conditional, but I'm not sure that making it unconditional is valid either? Why the change? Why not add the extra interrupt bits to the condition in addition to the value mask for the write? Andrew _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel