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 83B91C4167B for ; Wed, 29 Nov 2023 22:45:17 +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=A4P3Vnc9d2rB05QZ4b1K2mYbjp0IjsWK40oZCLlY88w=; b=PgUC8uhCYSaz2M ur4ZvI691wEiaICSv8mKiEnyjNN2lAyk9KoF+KuEFSwmHKVZGe+gzS7IOpms0VBhLsOz0iBI85q5/ z+0D9bljTKbBsWzWV1FFUBrWKeizmtwyqFnINSIPllVbgBVagBd7sM8CywNu10UKZSJ3V/h76Btpt sWmujsAud30Qwfp7bquDoYG/EQ7SqO9pwvOwKLCRj9EiucRaZIeYiAwfjM77fVUva9Israv11kBt7 Lj/VmetpsWDd+Q2b6pPgARi1KvQeMF4c+gc53cVNjAjsbAnF5egpPrUKBI8dqHahY4wPE90ZruSdr 2c1CRfePkK3aSOf43LmQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1r8TIb-009Sk5-12; Wed, 29 Nov 2023 22:44:53 +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 1r8TIY-009Sja-2L for linux-arm-kernel@lists.infradead.org; Wed, 29 Nov 2023 22:44:52 +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 2A2642014A; Thu, 30 Nov 2023 06:44:38 +0800 (AWST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=codeconstruct.com.au; s=2022a; t=1701297884; bh=3/RdWbl1/Y7109L7deJTqux8rPtQXt1q7C4U6mcb2AM=; h=Subject:From:To:Cc:Date:In-Reply-To:References; b=HKhhAUwUL/rIv0NFaMEyE2NscNboxbaKpc7q+jumnKREr/4dyPS0lTIGAoUtjzfqI +ZJn2JIvPHsZz3k7RVK6CUo9GZENgHOgcOzuUx0XI0hQoafGnNX/xgV265OxHK35Ir SQi66MjFYqk027UQfs8101rerH89LCnVaUhBb+qM6yr6n0RZp5Xqkebh4Nc7oxpyJX G5xKKjNRd9fVJgifEa1f2Tc3PxXfHtWCmoGpnBbOtowN/T/Ia0sZSkFnn28f3bkOlP sP5IWPBvU3wOQ+wjR/7Jt0CmAIBwT0bpO7DypAUHs/f0z2F6k3adv4wQTGKzhrr1ah ck2dkrKQPNUQQ== Message-ID: 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: Thu, 30 Nov 2023 09:14:37 +1030 In-Reply-To: <66dcea57-db0b-4686-9eaf-746db637f31c@os.amperecomputing.com> References: <20231128075236.2724038-1-quan@os.amperecomputing.com> <20231128075236.2724038-3-quan@os.amperecomputing.com> <2186c3b9ac92f03c68e8a2dd9fda871f80a6d664.camel@codeconstruct.com.au> <66dcea57-db0b-4686-9eaf-746db637f31c@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-20231129_144451_177560_98EF60CB X-CRM114-Status: GOOD ( 39.24 ) 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 Wed, 2023-11-29 at 16:02 +0700, Quan Nguyen wrote: > > On 29/11/2023 07:33, Andrew Jeffery wrote: > > On Tue, 2023-11-28 at 14:52 +0700, Quan Nguyen wrote: > > > 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. > > > > Thanks for the comment Andrew. > > I would prefer to delete it. > > But if to put some comment, how about: > > /* Early ack INTR_RX_DONE, INTR_TX_[ACK|NAK] would indicate HW to start > receiving/sending new data and may cause a race condition as irq handler > not yet to handle these irqs but being acked. Let ack them late in the > end of irq handler when those are truly processed */ Please update the patch with this comment. It at least goes some way to explain why. > > > > + 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? > > > > In original code, only INTR_RX_DONE was acked late. So the check > (irq_received & ASPEED_I2CD_INTR_RX_DONE) is need and that help to save > one write() then read() if there was no such irq. > > In the new code, there is no such check and the drawback is that there > always be one write() and one read() for all cases, include the case > where there is no irq at all, ie writing 0 into ASPEED_I2C_INTR_STS_REG. > > And yes, your concern maybe right, we can not say of writing 0 into > ASPEED_I2C_INTR_STS_REG is good or not. > > I checked back my debug log and seeing that irq always come with at > least one of INTR_RX_DONE BIT(2), INTR_TX_ACK BIT(0), INTR_TX_NAK BIT(1) > raised. So it seems like the case of writing 0 into > ASPEED_I2C_INTR_STS_REG is indeed rarely to happen. > > Do you think we should change it to: > > if (irq_received & (INTR_RX_DONE | INTR_TX_ACK | INTR_TX_NAK)) { > writel( irq_received & (INTR_RX_DONE| INTR_TX_ACK| INTR_TX_NAK), > bus->base + ASPEED_I2C_INTR_STS_REG); > readl(bus->base + ASPEED_I2C_INTR_STS_REG); > } This is less different from the existing strategy and doesn't require any explanation beyond what you're already trying to achieve in the patch, so I think you should switch to this approach. If someone wants to work out why it was done conditionally and argue for its removal then they can do that separately. Cheers, Andrew _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel