Messages in this thread | | | Subject | Re: [PATCH v3 4/7] i2c: aspeed: Acknowledge Tx done w/wo ACK irq late | From | Quan Nguyen <> | Date | Thu, 20 May 2021 20:52:15 +0700 |
| |
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 >>
| |