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>,
	Ryan Chen <ryan_chen@aspeedtech.com>
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 3/7] i2c: aspeed: Fix unhandled Tx done with NAK
Date: Wed, 19 May 2021 23:28:37 +0000	[thread overview]
Message-ID: <CACPK8XeFsuEXeCvG9DC0z+tiri6ptjOFOXe3x+COEZTVqUbVFg@mail.gmail.com> (raw)
In-Reply-To: <20210519074934.20712-4-quan@os.amperecomputing.com>

Ryan, can you please review this change?

On Wed, 19 May 2021 at 07:50, Quan Nguyen <quan@os.amperecomputing.com> wrote:
>
> It is observed that in normal condition, when the last byte sent by
> slave, the Tx Done with NAK irq will raise.
> But it is also observed that sometimes master issues next transaction
> too quick while the slave irq handler is not yet invoked and Tx Done
> with NAK irq of last byte of previous READ PROCESSED was not ack'ed.
> This Tx Done with NAK irq is raised together with the Slave Match and
> Rx Done irq of the next coming transaction from master.
> Unfortunately, the current slave irq handler handles the Slave Match and
> Rx Done only in higher priority and ignore the Tx Done with NAK, causing
> the complain as below:
> "aspeed-i2c-bus 1e78a040.i2c-bus: irq handled != irq. expected
> 0x00000086, but was 0x00000084"
>
> This commit handles this case by emitting a Slave Stop event for the
> Tx Done with NAK before processing Slave Match and Rx Done for the
> coming transaction from master.

It sounds like this patch is independent of the rest of the series,
and can go in on it's own. Please send it separately to the i2c
maintainers and add a suitable Fixes line, such as:

  Fixes: f9eb91350bb2 ("i2c: aspeed: added slave support for Aspeed I2C driver")

>
> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
> ---
> v3:
>   + First introduce in v3 [Quan]
>
>  drivers/i2c/busses/i2c-aspeed.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index 724bf30600d6..3fb37c3f23d4 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -254,6 +254,11 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
>
>         /* Slave was requested, restart state machine. */
>         if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {

Can you explain why you need to do this handing inside the SLAVE_MATCH case?

Could you instead move the TX_NAK handling to be above the SLAVE_MATCH case?

> +               if (irq_status & ASPEED_I2CD_INTR_TX_NAK &&
> +                   bus->slave_state == ASPEED_I2C_SLAVE_READ_PROCESSED) {

Either way, this needs a comment to explain what we're working around.

> +                       irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
> +                       i2c_slave_event(slave, I2C_SLAVE_STOP, &value);
> +               }
>                 irq_handled |= ASPEED_I2CD_INTR_SLAVE_MATCH;
>                 bus->slave_state = ASPEED_I2C_SLAVE_START;
>         }
> --
> 2.28.0
>

  reply	other threads:[~2021-05-19 23:28 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 [this message]
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
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=CACPK8XeFsuEXeCvG9DC0z+tiri6ptjOFOXe3x+COEZTVqUbVFg@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=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=ryan_chen@aspeedtech.com \
    --cc=thang@os.amperecomputing.com \
    --cc=wsa@kernel.org \
    --subject='Re: [PATCH v3 3/7] i2c: aspeed: Fix unhandled Tx done with NAK' \
    /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).