From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.6 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 56B15C433ED for ; Thu, 20 May 2021 01:22:03 +0000 (UTC) Received: from desiato.infradead.org (desiato.infradead.org [90.155.92.199]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id CC94E61028 for ; Thu, 20 May 2021 01:22:02 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org CC94E61028 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=roeck-us.net Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=desiato.20200630; h=Sender:Content-Type: Content-Transfer-Encoding:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date:Message-ID:Subject: From:References:Cc:To:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=vgyiqoIk88pcXkWXSfRnUPVcTkf3oH5uGjs991p1uU4=; b=eVOo/xTwkbPN3U3eezxGFR1dA fyX827svVeXmIBwhwqBdX95TNe5eZQVv/Si+MxADLg+U2op8WzLtvPVOvyUDYYX1KWkjCwFHVUWnU QiOhQX61aXNRtmC1tYQqIHkdaVLTn5GNv3Ks9wOXwZQqufPlSltqLxkxOxaG6GmafPnC7qKC35LjY a7sMp5kpqItuLiYTwU7cSCoGEWILgDHrxAFXhLV/j82p2N6dbBvqI7v15WtIQ8ajNAqwLNsqc5FYF JLHj4+G6tQGXugXYrSMabrLzv+OksqYktdP3vO+3Hu7llcHt2zmOWGudY33x+AXOaHQi8W2Vjlls9 5uNUoyUYQ==; Received: from localhost ([::1] helo=desiato.infradead.org) by desiato.infradead.org with esmtp (Exim 4.94 #2 (Red Hat Linux)) id 1ljXLu-005Xaf-FV; Thu, 20 May 2021 01:19:54 +0000 Received: from bombadil.infradead.org ([2607:7c80:54:e::133]) by desiato.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1ljXLm-005Xa5-Df for linux-arm-kernel@desiato.infradead.org; Thu, 20 May 2021 01:19:46 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=bombadil.20210309; h=Content-Transfer-Encoding: Content-Type:In-Reply-To:MIME-Version:Date:Message-ID:Subject:From:References :Cc:To:Sender:Reply-To:Content-ID:Content-Description; bh=1D3O+JRxyNjN7AfoZ/LrzEWFZxa2jRcbF0cobpLTwBI=; b=az/UY2fjKsEsgGuOfhKzCFDkJ1 FIXogUYdrRaYtqG0k9G6djd8orEBqd32EQVAezmBwmIYy7CoWaqN24iTukMpkiOe4fBHeM/Mljak2 RRWAWmZXL6reO2GUjfMiV/+jWynb4sYdvuSuiL92NBsxWyG8dSuE8l1Y0zn8NLRPbrEkr+tamnTzy xkhWRjtq3qBzn+1QHR08ogP6driZZe1gWqvQZN6HiBkrc9TSvIx+ZYAGQp0oLSIdbR/rnvzXueciK 0lmzLi3a+G7nA0YhZcrbVOAdpOC/qFU+ap+lNcGSGvaeqvpM6V5sTgWsMLSNfLxEYhPPWDtDQPRes xLLP+e4g==; Received: from mail-qk1-x72e.google.com ([2607:f8b0:4864:20::72e]) by bombadil.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1ljXLj-00Fsaw-9G for linux-arm-kernel@lists.infradead.org; Thu, 20 May 2021 01:19:45 +0000 Received: by mail-qk1-x72e.google.com with SMTP id q10so14706316qkc.5 for ; Wed, 19 May 2021 18:19:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=sender:to:cc:references:from:subject:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=1D3O+JRxyNjN7AfoZ/LrzEWFZxa2jRcbF0cobpLTwBI=; b=eU7zw9r8Zd+1y+GyrlwfK44OtLvUENYjnu3QHCykZLnrIK1EN7Jd8yURoCh4abpebN nXffVmA/P4XqKMZfZrpr7T1+Mmuk8i8PSzF4Cb83TFFmSc5OHyzLSo5Ss9zMgIXCFJIS M4OMkrTj3I76GCuMk7ahIAtCJDLiY307C2iQihHpsIUhm7tYX+55tYEtD7P+Ezw9TNLh TyounvSXxOTukIGHmFD9aU0uAXRo0LdKXYLUf6o5P1PO/wG2Li/5dFsHXjRf4TcdKziN T3KUgTFP8G0AJmwUfsb1SCqALHzb1Da224QzqT7nGd2WB2UTOWV3leq8x6jdmO3evhnd zo1Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:sender:to:cc:references:from:subject:message-id :date:user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=1D3O+JRxyNjN7AfoZ/LrzEWFZxa2jRcbF0cobpLTwBI=; b=sO5/NAOH7+WpsX4xWtHsCPq/dPH0rIQ9qhKlyFLM2h4XOyh5buO1vwQZMEVzAs2QfR awIoKqZvrbkopaatZTzOA7aNkVZ26qx84KGkWcrVQKP+K/eI+s4xoa9KgXEzbQdndhkO 6ngKSpBpLsQeUC3OsDrEyRMD9HNwInWgRbm3A8ZOUjGvDaPsmbKTcsgzWf6PkSr1C8xK CucNMRNXq6SZP5Yd63pszWEmmesKBaFY9WSGntte5YYAegJEdFpiqJzjZgUjolN+MXpU Xn5e6wdN4q6tq1zMJwPCk1A6vK0DqtSMlQmKEO2qVagT+31BHWse0lK7Lbkcyb2kE+9w vT7w== X-Gm-Message-State: AOAM530PDTO0gZOPaYnyIw6xqgLpxkNXI5+MpUpZklgeShRNHmHq3kEO F8SLJCgGC7XNNXCIRNVW/Tc= X-Google-Smtp-Source: ABdhPJyFOg1bxkiIgR5UtafC5GzENVkl+n00XgA3bXG4idwEtbP07zX0Un68QH28aZ9fnkkoC860yg== X-Received: by 2002:ae9:e706:: with SMTP id m6mr2534629qka.74.1621473581138; Wed, 19 May 2021 18:19:41 -0700 (PDT) Received: from server.roeck-us.net ([2600:1700:e321:62f0:329c:23ff:fee3:9d7c]) by smtp.gmail.com with ESMTPSA id u126sm1075725qkd.80.2021.05.19.18.19.38 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 19 May 2021 18:19:40 -0700 (PDT) To: Joel Stanley , Quan Nguyen Cc: Corey Minyard , Rob Herring , Andrew Jeffery , Brendan Higgins , Benjamin Herrenschmidt , Wolfram Sang , Philipp Zabel , openipmi-developer@lists.sourceforge.net, devicetree , Linux ARM , linux-aspeed , Linux Kernel Mailing List , linux-i2c@vger.kernel.org, Open Source Submission , Phong Vo , "Thang Q . Nguyen" , OpenBMC Maillist References: <20210519074934.20712-1-quan@os.amperecomputing.com> <20210519074934.20712-5-quan@os.amperecomputing.com> From: Guenter Roeck Subject: Re: [PATCH v3 4/7] i2c: aspeed: Acknowledge Tx done w/wo ACK irq late Message-ID: <414a51e8-0973-0007-9ffc-2949f3c7b0f8@roeck-us.net> Date: Wed, 19 May 2021 18:19:37 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210519_181943_369968_F2ED3EB1 X-CRM114-Status: GOOD ( 39.12 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 5/19/21 4:43 PM, Joel Stanley wrote: > On Wed, 19 May 2021 at 07:50, Quan Nguyen 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. > This might re-introduce a race condition if the code that is handling Tx done sends another byte without acknowledging the original interrupt, and another Tx done (or Tx nack) interrupt is received before the interrupt handler returns. If that happens, the second Tx done interrupt would be acknowledged but not be handled, and transmit would stall. That may well be what I had observed at the time but it is too long ago to remember, sorry. > 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 >> --- >> 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); > This would clear those bits unconditionally even if they were not handled. > 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. > Or irq_handled = irq_received & (ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_TX_ACK | ASPEED_I2CD_INTR_TX_NAK); writel(irq_handled, bus->base + ASPEED_I2C_INTR_STS_REG); if the idea was to avoid the long statement. Guenter >> + 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 >> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel