lkml.org 
[lkml]   [2021]   [May]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v3 4/7] i2c: aspeed: Acknowledge Tx done w/wo ACK irq late
    From
    Date
    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
    >>

    \
     
     \ /
      Last update: 2021-05-20 15:53    [W:3.079 / U:0.316 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site