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=-14.1 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,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED 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 0B41FC433B4 for ; Wed, 19 May 2021 23:45:23 +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 79E96611AB for ; Wed, 19 May 2021 23:45:22 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 79E96611AB Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=jms.id.au 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-Transfer-Encoding :Content-Type:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:Cc:To:Subject:Message-ID:Date:From:In-Reply-To: References:MIME-Version:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=jye00slZKXpi6iceCCgYTdLwI3b0n/v868/VEZSCtXs=; b=iaeFPYgNnQfcxTIVV4XubI7iE +yoHUjAk/E+QpZuXd3v5OXXtshMHv6XYYdlRn27KtK+S8fS4dEIpSu3ZbjEXN9DpmrKaaczpxE+i2 /hlCbfP7mND79BNJ7/VDbD/4mcvcCHmt00YsXXDFwD0CYXYnM4ar15wzWZyt+uslz4VuMcuoZ/oX+ vu4A+WReKQ7HkIkk8XZ0p447mo2hp1VafReoAtlqcEvDBWtVrxoZeyfeqDp4jWiPyS54vdDf1uUYs SNO8dKuMVonPQpx22rmaE4nHb1P1h4aGdvtxaiY3zN5zQKBxYTQxteNtHKlT73CrJQyJCOCic/CNs FiGrlNxGA==; Received: from localhost ([::1] helo=desiato.infradead.org) by desiato.infradead.org with esmtp (Exim 4.94 #2 (Red Hat Linux)) id 1ljVqZ-005N3D-K6; Wed, 19 May 2021 23:43:28 +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 1ljVqW-005N2m-7Z for linux-arm-kernel@desiato.infradead.org; Wed, 19 May 2021 23:43:25 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=bombadil.20210309; h=Content-Type:Cc:To:Subject:Message-ID :Date:From:In-Reply-To:References:MIME-Version:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=jZj61H+nZldX9VF8Rk7SoLRZEnhdWQ15nrHYMIdlXL8=; b=MAW5Qkx01YFAMcVDmaA72WeU8W SL7EZaQDSB59uMpsPinnJFLAMnJsnWwcfL/kSmI54wI2JoC+3bJQIFdT/cDT7r7Rybk3RetyeCWEl r9LsPigdFJMbr8slsvLjybBgnfljPTvOPT6u+Bb5RKP3PEkNlQp3UpjCFRWRgKd5/iERoY2ynwZ81 njXvpnnBvm7DPEL1rR0C6f2aLKyUUYhYa3e3bIxb8plCTq3gxioiUJvrmee5GZF90aP9j67rdSmSw XmSPGFVCO3qFqexMZaXHscOZu75sDkA9z4X1l6yOAn4id3KIDPKaNBjmr11VZgMzEIKHDsqL8VWVB de5/umfA==; Received: from mail-qk1-x72c.google.com ([2607:f8b0:4864:20::72c]) by bombadil.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1ljVqT-00Fqho-Bo for linux-arm-kernel@lists.infradead.org; Wed, 19 May 2021 23:43:22 +0000 Received: by mail-qk1-x72c.google.com with SMTP id v8so14573474qkv.1 for ; Wed, 19 May 2021 16:43:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=jms.id.au; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=jZj61H+nZldX9VF8Rk7SoLRZEnhdWQ15nrHYMIdlXL8=; b=hV289Pn1fzt0l5vYzw9Zco9XFbeqr0tKlwl117wL+ReIbVLezHs/xFwIcMKafGIL/r Ceq9AJkQHisWMOwWZVoe+E3BdzypGyKrbGtP5GVg+wjxmevjIE5q30D1Z3rQCTBmbzyl JopSkzzu13aCRFAkE3C3pYN93lb/AFgpuiOcI= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=jZj61H+nZldX9VF8Rk7SoLRZEnhdWQ15nrHYMIdlXL8=; b=tuJ2R7U4q5FQcbAJ8m9MeDRJqW2sBcoFp7uZlxFiksmNAos1RYpaDLTYikuI1G64qs SDS8onKrg/UjxP4PbSPp3kb+WdChE7aP2ybx66IrAcB1jMM9cg6G+J+dW3LiqYPX+PT1 4cj4Oc6irFzhxjWJg+iuOTOOS4ApVp2/U5iKW26U1VLibaIhL7Fbkyy+uPtoDoQ0MKqH kQNLF1C8FWAHG8qDroSXDZtS1H6qHSYJi1kvMQoWWL+IxPMhI3FUAPObO/Wp9Qoxhh/6 IMp0wAlf3a40WlmGG8Lr0j2iKGpfX3zKjBvFIXltGTBUckS6oSjr7cSetwsfo6Yh+yFo Brkg== X-Gm-Message-State: AOAM530cVn58B5Jey+AhY3ib3OiSMhQ6DwzEMTSdvZyfsMcKDuryNjmq 1AF6OlopEGVBUqCfV8DirsTRSSIWTwfuUA99dmA= X-Google-Smtp-Source: ABdhPJyUszWiuobnzOy5L9VplXiMBG9qLzJjbpbR0WfzIFw8cspGiZ3PnAV6tV3oDvUEgCvPn/rTmcBK9mwiyYTq9ws= X-Received: by 2002:a05:620a:704:: with SMTP id 4mr1331345qkc.66.1621467800517; Wed, 19 May 2021 16:43:20 -0700 (PDT) MIME-Version: 1.0 References: <20210519074934.20712-1-quan@os.amperecomputing.com> <20210519074934.20712-5-quan@os.amperecomputing.com> In-Reply-To: <20210519074934.20712-5-quan@os.amperecomputing.com> From: Joel Stanley Date: Wed, 19 May 2021 23:43:08 +0000 Message-ID: Subject: Re: [PATCH v3 4/7] i2c: aspeed: Acknowledge Tx done w/wo ACK irq late To: Quan Nguyen , Guenter Roeck 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 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210519_164321_433963_D2E90D2E X-CRM114-Status: GOOD ( 29.10 ) 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-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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. 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); 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 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel