linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Joel Stanley <joel@jms.id.au>
To: Quan Nguyen <quan@os.amperecomputing.com>,
	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: Wed, 19 May 2021 23:43:08 +0000	[thread overview]
Message-ID: <CACPK8XdyQT=cuSr9KBqC0PBkOLgBUBpyz3kZEA3JuOuZsQN_Rw@mail.gmail.com> (raw)
In-Reply-To: <20210519074934.20712-5-quan@os.amperecomputing.com>

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 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.

>
> 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.


> +        */
> +       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.

> +       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).

> +
>         spin_unlock(&bus->lock);
>         return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
>  }
> --
> 2.28.0
>

  reply	other threads:[~2021-05-19 23:43 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 [this message]
2021-05-20  1:19     ` Guenter Roeck
2021-05-20 14:03       ` Quan Nguyen
2021-05-20 13:52     ` Quan Nguyen
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='CACPK8XdyQT=cuSr9KBqC0PBkOLgBUBpyz3kZEA3JuOuZsQN_Rw@mail.gmail.com' \
    --to=joel@jms.id.au \
    --cc=andrew@aj.id.au \
    --cc=benh@kernel.crashing.org \
    --cc=brendanhiggins@google.com \
    --cc=devicetree@vger.kernel.org \
    --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=quan@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).