From: Quan Nguyen <quan@os.amperecomputing.com> To: Joel Stanley <joel@jms.id.au>, Guenter Roeck <linux@roeck-us.net> Cc: Corey Minyard <minyard@acm.org>, Rob Herring <robh+dt@kernel.org>, Andrew Jeffery <andrew@aj.id.au>, Brendan Higgins <brendanhiggins@google.com>, Benjamin Herrenschmidt <benh@kernel.crashing.org>, Wolfram Sang <wsa@kernel.org>, Philipp Zabel <p.zabel@pengutronix.de>, openipmi-developer@lists.sourceforge.net, devicetree <devicetree@vger.kernel.org>, Linux ARM <linux-arm-kernel@lists.infradead.org>, linux-aspeed <linux-aspeed@lists.ozlabs.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, linux-i2c@vger.kernel.org, Open Source Submission <patches@amperecomputing.com>, Phong Vo <phong@os.amperecomputing.com>, "Thang Q . Nguyen" <thang@os.amperecomputing.com>, OpenBMC Maillist <openbmc@lists.ozlabs.org> Subject: Re: [PATCH v3 4/7] i2c: aspeed: Acknowledge Tx done w/wo ACK irq late Date: Thu, 20 May 2021 20:52:15 +0700 [thread overview] Message-ID: <b082d8a8-cc96-7bc1-6ca4-589ab5ac677e@os.amperecomputing.com> (raw) In-Reply-To: <CACPK8XdyQT=cuSr9KBqC0PBkOLgBUBpyz3kZEA3JuOuZsQN_Rw@mail.gmail.com> On 20/05/2021 06:43, Joel Stanley wrote: > On Wed, 19 May 2021 at 07:50, Quan Nguyen <quan@os.amperecomputing.com> wrote: >> >> With Tx done w/wo ACK are ack'ed early at beginning of irq handler, > > Is w/wo a typo? If not, please write the full words ("with and without") > It is "with and without", will fix in next version >> it is observed that, usually, the Tx done with Ack irq raises in the >> READ REQUESTED state. This is unexpected and complaint as below appear: >> "Unexpected Ack on read request" >> >> Assumed that Tx done should only be ack'ed once it was truly processed, >> switch to late ack'ed this two irqs and seen this issue go away through >> test with AST2500.. > > Please read Guneter's commit message > 2be6b47211e17e6c90ead40d24d2a5cc815f2d5c to confirm that your changes > do not invalidate the fix that they made. Add them to CC for review. > > Again, this is a fix that is independent of the ssif work. Please send > it separately with a Fixes line. > Will do and separate this patch into other series in next version. >> >> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com> >> --- >> v3: >> + First introduce in v3 [Quan] >> >> drivers/i2c/busses/i2c-aspeed.c | 26 ++++++++++++++++++-------- >> 1 file changed, 18 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c >> index 3fb37c3f23d4..b2e9c8f0ddf7 100644 >> --- a/drivers/i2c/busses/i2c-aspeed.c >> +++ b/drivers/i2c/busses/i2c-aspeed.c >> @@ -606,8 +606,12 @@ 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 > > Nit: this comment can be on one line. > Thanks, will fix. > >> + */ >> + 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; >> @@ -652,12 +656,18 @@ 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 */ >> + /* Note: Re-use irq_handled variable */ > > I'm not sure what this note means. > >> + irq_handled = 0; >> + if (irq_received & ASPEED_I2CD_INTR_RX_DONE) >> + irq_handled |= ASPEED_I2CD_INTR_RX_DONE; >> + if (irq_received & ASPEED_I2CD_INTR_TX_ACK) >> + irq_handled |= ASPEED_I2CD_INTR_TX_ACK; >> + if (irq_received & ASPEED_I2CD_INTR_TX_NAK) >> + irq_handled |= ASPEED_I2CD_INTR_TX_NAK; >> + writel(irq_handled, bus->base + ASPEED_I2C_INTR_STS_REG); > > Are you intentionally only acking the bits that are set when we read > from STS_REG at the start of the handler? If not, we could write this > instead: > > writel(ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_TX_ACK | > ASPEED_I2CD_INTR_TX_NAK, > bus->base + ASPEED_I2C_INTR_STS_REG); > > If you only want to ack the bits that are set, then do this: > > 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); > > That way, you can avoid all of the tests. > Thanks, will fix this in next version. >> + readl(bus->base + ASPEED_I2C_INTR_STS_REG); > > When you move this, please add a comment that reminds us why we do a > write-then-read (see commit c926c87b8e36dcc0ea5c2a0a0227ed4f32d0516a). > Will fix in next version. >> + >> spin_unlock(&bus->lock); >> return irq_remaining ? IRQ_NONE : IRQ_HANDLED; >> } >> -- >> 2.28.0 >>
next prev parent reply other threads:[~2021-05-20 13:52 UTC|newest] Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-05-19 7:49 [PATCH v3 0/7] Add Aspeed SSIF BMC driver Quan Nguyen 2021-05-19 7:49 ` [PATCH v3 1/7] i2c: i2c-core-smbus: Expose PEC calculate function for generic use Quan Nguyen 2021-06-25 15:02 ` Wolfram Sang 2021-05-19 7:49 ` [PATCH v3 2/7] ipmi: ssif_bmc: Add SSIF BMC driver Quan Nguyen 2021-05-19 12:30 ` Corey Minyard 2021-05-20 14:19 ` Quan Nguyen 2021-05-19 7:49 ` [PATCH v3 3/7] i2c: aspeed: Fix unhandled Tx done with NAK Quan Nguyen 2021-05-19 23:28 ` Joel Stanley 2021-05-20 11:28 ` Ryan Chen 2021-05-20 14:15 ` Quan Nguyen 2021-05-20 13:48 ` Quan Nguyen 2021-05-19 7:49 ` [PATCH v3 4/7] i2c: aspeed: Acknowledge Tx done w/wo ACK irq late Quan Nguyen 2021-05-19 23:43 ` Joel Stanley 2021-05-20 1:19 ` Guenter Roeck 2021-05-20 14:03 ` Quan Nguyen 2021-05-20 13:52 ` Quan Nguyen [this message] 2021-05-19 7:49 ` [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy() Quan Nguyen 2021-05-20 11:06 ` Ryan Chen 2021-05-20 14:10 ` Quan Nguyen 2021-05-21 6:09 ` Ryan Chen 2021-05-28 1:00 ` Quan Nguyen 2021-05-24 10:06 ` Ryan Chen 2021-05-24 10:20 ` Quan Nguyen 2021-05-24 10:36 ` Ryan Chen 2021-05-24 10:48 ` Quan Nguyen 2021-05-25 10:30 ` Ryan Chen 2021-05-28 0:53 ` Quan Nguyen 2021-05-28 2:57 ` Ryan Chen 2021-06-07 14:57 ` Graeme Gregory 2021-05-19 7:49 ` [PATCH v3 6/7] ipmi: ssif_bmc: Add Aspeed SSIF BMC driver Quan Nguyen 2021-05-19 7:49 ` [PATCH v3 7/7] bindings: ipmi: Add binding for " Quan Nguyen 2021-05-19 15:29 ` Rob Herring 2021-05-20 14:24 ` Quan Nguyen 2021-05-19 12:34 ` [PATCH v3 0/7] Add " Corey Minyard 2021-05-20 14:23 ` Quan Nguyen
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=b082d8a8-cc96-7bc1-6ca4-589ab5ac677e@os.amperecomputing.com \ --to=quan@os.amperecomputing.com \ --cc=andrew@aj.id.au \ --cc=benh@kernel.crashing.org \ --cc=brendanhiggins@google.com \ --cc=devicetree@vger.kernel.org \ --cc=joel@jms.id.au \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-aspeed@lists.ozlabs.org \ --cc=linux-i2c@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux@roeck-us.net \ --cc=minyard@acm.org \ --cc=openbmc@lists.ozlabs.org \ --cc=openipmi-developer@lists.sourceforge.net \ --cc=p.zabel@pengutronix.de \ --cc=patches@amperecomputing.com \ --cc=phong@os.amperecomputing.com \ --cc=robh+dt@kernel.org \ --cc=thang@os.amperecomputing.com \ --cc=wsa@kernel.org \ --subject='Re: [PATCH v3 4/7] i2c: aspeed: Acknowledge Tx done w/wo ACK irq late' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).