Message ID | 20231208033142.1673232-3-quan@os.amperecomputing.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:bcd1:0:b0:403:3b70:6f57 with SMTP id r17csp5220624vqy; Thu, 7 Dec 2023 19:32:41 -0800 (PST) X-Google-Smtp-Source: AGHT+IG3Lw0hJPxrqT5v3eekBWXPPqiXhSnYeeZFyYx6LSxDyBygrW+fnMk23z/Dijv+DU74+1+L X-Received: by 2002:a05:6871:4414:b0:1fb:d89:4879 with SMTP id nd20-20020a056871441400b001fb0d894879mr4021054oab.26.1702006361219; Thu, 07 Dec 2023 19:32:41 -0800 (PST) ARC-Seal: i=2; a=rsa-sha256; t=1702006361; cv=pass; d=google.com; s=arc-20160816; b=mSP6EQYGmVaD6WsMltQhmXnlc3No53g1tyK6pl4TNi+//F3FAuhNd9OWjc72/v7TBJ 2S5KqLnSbGu3y8xVMhQPim5/ZKOz1Sj1r+HlQpuDcuHwE+p45b3erWuxVq5UnqPaVMcP iELWNfbxf0CSYzXDbO+8Rx7mV1ZLXrr6DoLQSW3UGgQqumJexmzXCOgaAvtMfXNMFUYn ho+rTUyp8qr9mq8KrhL3kLujs4TMJ2Q/idROE6kXbDMsTRxFivmZyBUEHH+r9PABQ8wP uybBFpDBXewsZ8SAk5mmwejwK7+wdjBWaTBP4Nb5DuArrZfLMxwiAO5sDhrMQVk2PK9p L1Yw== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:mime-version:content-transfer-encoding :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=YlMP3M3fABHaRpRNGuvf9NO+rSFvhyPtOwBNy1e3lk4=; fh=WFcqSubssMIwC0Dm7Plh6RRvLP+WIFWeZgWd743wkLo=; b=czIZ4s+he9RMJbdoicdROgc+gy9bL/VPh+zycwSxumhSdRKXhiFa2eJvbyLt55obLj TZkn5z6JYlDI8VTX8NlO8ZdEe0/fxW4g5IYcnBr8sR3lQmfgLq231QKoMvVsgwI8kv9i aY5G1PE9OAIGANCYTBUhjo7CG25ZXB8I7WoG3YLnaKsdI43HWLX/Bg0rTr6VW6YEgW/X 0C6sXC0/wjB4J6ONX+ikur3i15IcakK+lQNH/UtLDDgyi61WVqhz8cGVxW3zdqqdEWg9 hOH/5q65V5inzldtHlpM/VaKZ0Wm4Clf/JjNPf7JTAKdlOtpXgWNZ6B12afZ9ZhgsQLz CBDA== ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@os.amperecomputing.com header.s=selector2 header.b=g8fz7HTK; arc=pass (i=1 spf=pass spfdomain=os.amperecomputing.com dkim=pass dkdomain=os.amperecomputing.com dmarc=pass fromdomain=os.amperecomputing.com); spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=amperecomputing.com Received: from morse.vger.email (morse.vger.email. [23.128.96.31]) by mx.google.com with ESMTPS id o62-20020a634141000000b00565ead2509asi752297pga.199.2023.12.07.19.32.40 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 07 Dec 2023 19:32:41 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31; Authentication-Results: mx.google.com; dkim=pass header.i=@os.amperecomputing.com header.s=selector2 header.b=g8fz7HTK; arc=pass (i=1 spf=pass spfdomain=os.amperecomputing.com dkim=pass dkdomain=os.amperecomputing.com dmarc=pass fromdomain=os.amperecomputing.com); spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=amperecomputing.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by morse.vger.email (Postfix) with ESMTP id 73E15836CE46; Thu, 7 Dec 2023 19:32:38 -0800 (PST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at morse.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1573090AbjLHDcQ (ORCPT <rfc822;chrisfriedt@gmail.com> + 99 others); Thu, 7 Dec 2023 22:32:16 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54062 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1573078AbjLHDcN (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 7 Dec 2023 22:32:13 -0500 Received: from NAM04-BN8-obe.outbound.protection.outlook.com (mail-bn8nam04on2134.outbound.protection.outlook.com [40.107.100.134]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0EA451727; Thu, 7 Dec 2023 19:32:18 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Lbt3JChCNqavGZdEg0/bbTeXhCFjQmg4IygDkZdLCKwI78BzCwaKw523mc9bf3xVodAkWxi2r0+sePZC0VFOObefUezraMEg6WBIYwkWSSf41V7As9ITBW2HLZX9bsoZFzITbtPXk+vx79S01SFjIFQC2hlBJqC0rvDBEfDK91YCyAGSftbRFm/pUOoEWTEJkfyrmhGFsA/Uv8UmT9pcsqG/CAMYb2JQJ7v5Is9auTk6c1fSD3aBcXfx1eSW8gUJPLBZqZgaW72G3GUqV9fbLzRw8QomWv0M1J+nzJDhjvPk/UTz39XkoCg4ceZNUai/2cZzI9XCqiOtK7sqmWq90A== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=YlMP3M3fABHaRpRNGuvf9NO+rSFvhyPtOwBNy1e3lk4=; b=UrK9W3+7BS0mn3Me70j67aW7uL5bk901c6npHQuUevoy3I3xGjYLaY1YwPmWhseh8zwi2KrjHez5p14gz6MsFTZxA/14TvszrqSJ5mZXb9X+2w/fSOldrgBkLXmkaxi/eRrQhCdFqoPzxK2i6oJMMQ98ExAhHxd/2ejxUu8FKaMLjQe4c8dGMm2BR1nygFn4E0Ery9B5EHgpU/jIOpUdjHbkV7dzTyoMneYA6eNy9Y7M82V8VczZlJOgy1eJhbvhB8a1RncMpFO0i8ba5DIAadN+D9OTljR6xM3gNySUhOXGHtn4Ok0O8IqpBN0AYX+w+OucOa+nOUhQQ4PPykXCLQ== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=os.amperecomputing.com; dmarc=pass action=none header.from=os.amperecomputing.com; dkim=pass header.d=os.amperecomputing.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=os.amperecomputing.com; s=selector2; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=YlMP3M3fABHaRpRNGuvf9NO+rSFvhyPtOwBNy1e3lk4=; b=g8fz7HTKarJoVFqPCz6+S6iYs6VwOXHQ1vI0wJ37tDXCdto187vLgthfHpuWfFZo3CnZpy9QpnPHDJbUh1j2+Db9VVs+Agordubk//kq0soZrkIUk9nGWQwpIPmb1ARRj2Vb5+t4IJHH1BAEZG8sVbGHDg0XNQPb9w6QX6Vmtpg= Authentication-Results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=os.amperecomputing.com; Received: from SN4PR01MB7455.prod.exchangelabs.com (2603:10b6:806:202::11) by DS7PR01MB7784.prod.exchangelabs.com (2603:10b6:8:7f::22) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7068.27; Fri, 8 Dec 2023 03:32:15 +0000 Received: from SN4PR01MB7455.prod.exchangelabs.com ([fe80::5682:1d84:171a:1d68]) by SN4PR01MB7455.prod.exchangelabs.com ([fe80::5682:1d84:171a:1d68%3]) with mapi id 15.20.7068.028; Fri, 8 Dec 2023 03:32:15 +0000 From: Quan Nguyen <quan@os.amperecomputing.com> To: Brendan Higgins <brendan.higgins@linux.dev>, Benjamin Herrenschmidt <benh@kernel.crashing.org>, Joel Stanley <joel@jms.id.au>, Andi Shyti <andi.shyti@kernel.org>, Andrew Jeffery <andrew@codeconstruct.com.au>, Wolfram Sang <wsa@kernel.org>, Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>, Guenter Roeck <linux@roeck-us.net>, linux-i2c@vger.kernel.org, openbmc@lists.ozlabs.org, linux-arm-kernel@lists.infradead.org, linux-aspeed@lists.ozlabs.org, linux-kernel@vger.kernel.org Cc: Cosmo Chou <chou.cosmo@gmail.com>, Open Source Submission <patches@amperecomputing.com>, Phong Vo <phong@os.amperecomputing.com>, "Thang Q . Nguyen" <thang@os.amperecomputing.com>, Quan Nguyen <quan@os.amperecomputing.com> Subject: [PATCH v3 2/2] i2c: aspeed: Acknowledge Tx done with and without ACK irq late Date: Fri, 8 Dec 2023 10:31:42 +0700 Message-Id: <20231208033142.1673232-3-quan@os.amperecomputing.com> X-Mailer: git-send-email 2.35.1 In-Reply-To: <20231208033142.1673232-1-quan@os.amperecomputing.com> References: <20231208033142.1673232-1-quan@os.amperecomputing.com> Content-Transfer-Encoding: 8bit Content-Type: text/plain X-ClientProxiedBy: SGAP274CA0014.SGPP274.PROD.OUTLOOK.COM (2603:1096:4:b6::26) To SN4PR01MB7455.prod.exchangelabs.com (2603:10b6:806:202::11) MIME-Version: 1.0 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: SN4PR01MB7455:EE_|DS7PR01MB7784:EE_ X-MS-Office365-Filtering-Correlation-Id: a89b4120-66dc-4f5f-b8e2-08dbf79e45b0 X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: 7VwckWnszuPjfJ1FGKoQ5sg+NEvgZhZeL5J2L3U39EwA8VtUI211Idd9wPpZyWsjM5zRCvXzVhXl9kOm8cNkl2AVBSq/9yqM2PRcfaJ8RcVFLLXbHPbN8QrSsPASdXNbFqgO2rbB9wStkZnkw56gEPJgIFwgIP5hquhiqM97olRp2x4Hc4ccnNM1JuqCPz/dowxj47sprQlJON/4R9fJk7QkQgakwEzJlmTZsAEXeIdXJd1hLMN9J6g0q0freZwl1dO8CVlshbeLQvVw0dfWJH4r47OFGeL1c5QhO0tZZ0gIQCYhu7QQZ37WqYd1ZA4ohLK1HYiNP6dNJ/UDSOia53j0FBpOYjy0FydPkDAm+BxyNChjqgoLH6pLbtlKEbAZilqvtiEjI9meocp2htbQVQjc9NwdrU/kEPf327Uwg17lLrAnRZjekDtjTzbDQpBuprJwLeUI8ccWb6FteoIfX3o6ZR0Fu3GgxxpKe8aTU3Nyp1IsW+vUPyNp6CUnJsZWaHCPaichJFvXGHWSJwbxP0ixsLP/G5kNg5tV2WZ+WSlEoyuc1xyKqORTrpcA+urtX+tJjOCK47gkdSOqHqWambgYScDPAAgTNloQYk6Dv/1dG3GzRKJ3AZ+NtcxCoYn1 X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:SN4PR01MB7455.prod.exchangelabs.com;PTR:;CAT:NONE;SFS:(13230031)(346002)(376002)(366004)(39850400004)(136003)(396003)(230922051799003)(1800799012)(64100799003)(451199024)(186009)(38100700002)(41300700001)(7416002)(5660300002)(2906002)(8676002)(38350700005)(316002)(4326008)(921008)(1076003)(110136005)(478600001)(6512007)(6666004)(6506007)(52116002)(2616005)(66476007)(86362001)(66556008)(54906003)(26005)(66946007)(107886003)(8936002)(83380400001)(966005)(6486002);DIR:OUT;SFP:1102; X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: yJP4exEKch3CkzPFSPfMms+t1cRdqzQsozSyuSBYjKGMlyiwOKdr7N9HOE9OBqHIZXrqGyEy6UGsvPE11Sb8O8WkZURJzpNU0HDID6ll7R3n0gFbL+RkjpN6FR58fapIFQu0oMrmxUwG5Hgh+V83x2uILmsNTRofiCPrQoCfs/zuyZY2v/pP6rmLaqFrKj99LLVP95NbmcgXceT6DD34uMheNLGPjNVq6GyhpE/3QnyriZz0zX9OVqBURkmilSPZ1tXb7AzYUncpJzujd4AYcUso27Bj5l9Gc87JGYSGcSa5SdO+7N9hoUCLFjSsOl/nrlb75tUHe7wuqTsdG0+6m30QVQeliJcYb2XH955Qz/xjRbr097IVAkH0guelYKCJMvYa82jJAsiFqVgBG4E0ifzkyrdqHQ+izzjN9iNutMydRk7HMApBa9XHCzY6wDiEbn+SM84xMB18fxyHY0PElBtiNdlAu8DmPB9e3vSp7ipI/ysJZvmfSI3MB1MpBlMd+mE+5gnG1TzSAx5BaHO/A9db2C2pHi1biIOXDs8/f5ZF3xHA7EgG2IXy1QbbX5r8xzcbo3YEu5JR6sUlucfi6o0PErYOHgc/oOfqfq49MRQ0ruIGJM5kIoKiyhSPL2ZM3XcSzll8RG1MHFQ9Rkh1xfh9u2S8Mc3kAIcW7FrbP73idb9TtTuAu4mYRE1yjmwHwr7u8pBnzTopWVY3EgMTsAoVnQ3Y8Xq7VibrWV1glrnRshlih+/0TtnN0HDCdi5coMX97kGqyLCQaxDl39oQpyEnQjV/aQSdv32YMDiGbdd0477HNr662XZX9WdiH06TYzJi35dL+EQTTyN8YcM6aBq/qwOhUVOUNOokdbwVm31Hyhdjhw/Nn28PQWl5eE0msqe3rF7zFptcNgqjnJ7F+1/+9rsugMM2xPtE1yCQiMRuQJSCKrJxzlxUGsmaiqhKI3/hORiReqcbi3gdKlrVUeuUscYOMMk/icW6ySV1fKxGYYunixz52gDcNCecuKL3on9+Xntsc1BkSHnc9Jb6T2InHoeaF7nklCgHuPAfs4QfZi6NPEWx99+uJ2BG6vtE1GF3CXPYbVFC4kXMFS7YgyLHpfMC782aMXK5OG3S3DmmeBwdqQLOvL+GMJj//aX+m16dPzGDWB2RlK3y4yEHYLB9c/BrxaJPZCwpb0Pf0YOZhavLWlLDHzBKK+ehPoMNnnO6DBz1SydVPw0Yi19WyKN8fSPLG/ztTaxwgS727jM+dstsyqaa1J5vorWH9xvIaexx0775dbATNpbc8Zs55i6gGrOyJPWZwtyJhQAqwie6XutnoqmeXUEcRjhjOgeZ9MoFGfLBRrdaFnjB8UiPQcjOjr2xU7Lhu+KsGuL0cItVtL0GLToCEDdAevmUGsX7pnJJh1a2TcVP12Lq9uUJd4LdUctO5J/rYyr1Be62iAyH1gGxVEBBg7mDTB/FLBVL+qwHM07QqwHNZQsHh4DFLUNekj7BDURZzybma3jOxb08jlCHF9dd9ljGrpr8d4CZSfqu2wG6nKH7jn5FER1YyJvKGD8pEaHaFzctMbU5Gx/qaYC8NUks8uFeqB1qLE5Gq7GQKEG3EtGunKS/loWQqY58MzlnFW9nnGepG79sOQU= X-OriginatorOrg: os.amperecomputing.com X-MS-Exchange-CrossTenant-Network-Message-Id: a89b4120-66dc-4f5f-b8e2-08dbf79e45b0 X-MS-Exchange-CrossTenant-AuthSource: SN4PR01MB7455.prod.exchangelabs.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Dec 2023 03:32:15.3008 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 3bc2b170-fd94-476d-b0ce-4229bdc904a7 X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: uz7HCNU0kVg1jpTNGtYD8qORLzFZwnOWOWoo/LIoYqY6xFPqeQBwVLQS0ch0Ht73ZgvIJgNsolVPnv5SeTiW8m27jG+A0h7DAFeHQfLcH3o= X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR01MB7784 X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on morse.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (morse.vger.email [0.0.0.0]); Thu, 07 Dec 2023 19:32:38 -0800 (PST) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1784683022442280304 X-GMAIL-MSGID: 1784683022442280304 |
Series |
i2c: aspeed: Late ack Tx done irqs and handle coalesced start with stop conditions
|
|
Commit Message
Quan Nguyen
Dec. 8, 2023, 3:31 a.m. UTC
Commit 2be6b47211e1 ("i2c: aspeed: Acknowledge most interrupts early in
interrupt handler") acknowledges most interrupts early before the slave
irq handler is executed, except for the "Receive Done Interrupt status"
which is acknowledged late in the interrupt.
However, it has been observed that the early acknowledgment of "Transmit
Done Interrupt Status" (with ACK or NACK) often causes the interrupt to
be raised in READ REQUEST state, that shows the
"Unexpected ACK on read request." complaint messages.
Assuming that the "Transmit Done" interrupt should only be acknowledged
once it is truly processed, this commit fixes that issue by acknowledging
interrupts for both ACK and NACK cases late in the interrupt handler.
Fixes: 2be6b47211e1 ("i2c: aspeed: Acknowledge most interrupts early in interrupt handler")
Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
---
v3:
+ Fix the unconditinal write when ack the irqs [Andrew]
+ Refactor the code to enhance code readability [Quan]
+ Fix grammar in commit message [Quan]
v2:
+ Split to separate series [Joel]
+ Added the Fixes line [Joel]
+ Fixed multiline comment [Joel]
+ Refactor irq clearing code [Joel, Guenter]
+ Revised commit message [Joel]
+ Revised commit message [Quan]
+ About a note to remind why the readl() should immediately follow the
writel() to fix the race condition when clearing irq status from commit
c926c87b8e36 ("i2c: aspeed: Avoid i2c interrupt status clear race
condition"), I think it looks straight forward in this patch and decided
not to add that note. [Joel]
v1:
+ First introduced in
https://lore.kernel.org/all/20210519074934.20712-1-quan@os.amperecomputing.com/
---
drivers/i2c/busses/i2c-aspeed.c | 21 +++++++++++++--------
1 file changed, 13 insertions(+), 8 deletions(-)
Comments
On Fri, 2023-12-08 at 10:31 +0700, Quan Nguyen wrote: > Commit 2be6b47211e1 ("i2c: aspeed: Acknowledge most interrupts early in > interrupt handler") acknowledges most interrupts early before the slave > irq handler is executed, except for the "Receive Done Interrupt status" > which is acknowledged late in the interrupt. > However, it has been observed that the early acknowledgment of "Transmit > Done Interrupt Status" (with ACK or NACK) often causes the interrupt to > be raised in READ REQUEST state, that shows the > "Unexpected ACK on read request." complaint messages. > > Assuming that the "Transmit Done" interrupt should only be acknowledged > once it is truly processed, this commit fixes that issue by acknowledging > interrupts for both ACK and NACK cases late in the interrupt handler. > > Fixes: 2be6b47211e1 ("i2c: aspeed: Acknowledge most interrupts early in interrupt handler") > Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com> > --- > v3: > + Fix the unconditinal write when ack the irqs [Andrew] > + Refactor the code to enhance code readability [Quan] > + Fix grammar in commit message [Quan] > > v2: > + Split to separate series [Joel] > + Added the Fixes line [Joel] > + Fixed multiline comment [Joel] > + Refactor irq clearing code [Joel, Guenter] > + Revised commit message [Joel] > + Revised commit message [Quan] > + About a note to remind why the readl() should immediately follow the > writel() to fix the race condition when clearing irq status from commit > c926c87b8e36 ("i2c: aspeed: Avoid i2c interrupt status clear race > condition"), I think it looks straight forward in this patch and decided > not to add that note. [Joel] > > v1: > + First introduced in > https://lore.kernel.org/all/20210519074934.20712-1-quan@os.amperecomputing.com/ > --- > drivers/i2c/busses/i2c-aspeed.c | 21 +++++++++++++-------- > 1 file changed, 13 insertions(+), 8 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c > index 1c2a4f4c4e1b..967a26dd4ffa 100644 > --- a/drivers/i2c/busses/i2c-aspeed.c > +++ b/drivers/i2c/busses/i2c-aspeed.c > @@ -617,13 +617,19 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status) > static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) > { > struct aspeed_i2c_bus *bus = dev_id; > - u32 irq_received, irq_remaining, irq_handled; > + u32 irq_received, irq_remaining, irq_handled, irq_ack_last; `irq_ack_last` might be better as a macro, but you're probably saved by the optimiser anyway. If there's another reason to do a v4 or others are unhappy with it then consider fixing it, otherwise: Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au> Thanks.
Hi Quan, [...] > - /* Ack all interrupts except for Rx done */ > - writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE, > - bus->base + ASPEED_I2C_INTR_STS_REG); > + > + /* > + * Early acking of INTR_RX_DONE and INTR_TX_[ACK|NAK] would indicate HW to > + * start receiving or sending new data, and this may cause a race condition > + * as the irq handler has not yet handled these irqs but is being acked. > + * Let's ack them late at the end of the irq handler when those are truly processed. > + */ > + irq_ack_last = ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_TX_ACK | ASPEED_I2CD_INTR_TX_NAK; > + writel(irq_received & ~irq_ack_last, bus->base + ASPEED_I2C_INTR_STS_REG); I like Andrews suggestion of having irq_ack_last as a define that is already negated, instead of negating it in the writel, which makes it a bit difficult to read. Besides, ack_last, as a name is not very meaningful, I'd rather call it irq_ack_rx_tx (or something similar). But I'm not going to block it for this, up to you if you want to send a new version. Reviewed-by: Andi Shyti <andi.shyti@kernel.org> Thanks, Andi
On 08/12/2023 11:00, Andrew Jeffery wrote: > On Fri, 2023-12-08 at 10:31 +0700, Quan Nguyen wrote: >> Commit 2be6b47211e1 ("i2c: aspeed: Acknowledge most interrupts early in >> interrupt handler") acknowledges most interrupts early before the slave >> irq handler is executed, except for the "Receive Done Interrupt status" >> which is acknowledged late in the interrupt. >> However, it has been observed that the early acknowledgment of "Transmit >> Done Interrupt Status" (with ACK or NACK) often causes the interrupt to >> be raised in READ REQUEST state, that shows the >> "Unexpected ACK on read request." complaint messages. >> >> Assuming that the "Transmit Done" interrupt should only be acknowledged >> once it is truly processed, this commit fixes that issue by acknowledging >> interrupts for both ACK and NACK cases late in the interrupt handler. >> >> Fixes: 2be6b47211e1 ("i2c: aspeed: Acknowledge most interrupts early in interrupt handler") >> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com> >> --- >> v3: >> + Fix the unconditinal write when ack the irqs [Andrew] >> + Refactor the code to enhance code readability [Quan] >> + Fix grammar in commit message [Quan] >> >> v2: >> + Split to separate series [Joel] >> + Added the Fixes line [Joel] >> + Fixed multiline comment [Joel] >> + Refactor irq clearing code [Joel, Guenter] >> + Revised commit message [Joel] >> + Revised commit message [Quan] >> + About a note to remind why the readl() should immediately follow the >> writel() to fix the race condition when clearing irq status from commit >> c926c87b8e36 ("i2c: aspeed: Avoid i2c interrupt status clear race >> condition"), I think it looks straight forward in this patch and decided >> not to add that note. [Joel] >> >> v1: >> + First introduced in >> https://lore.kernel.org/all/20210519074934.20712-1-quan@os.amperecomputing.com/ >> --- >> drivers/i2c/busses/i2c-aspeed.c | 21 +++++++++++++-------- >> 1 file changed, 13 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c >> index 1c2a4f4c4e1b..967a26dd4ffa 100644 >> --- a/drivers/i2c/busses/i2c-aspeed.c >> +++ b/drivers/i2c/busses/i2c-aspeed.c >> @@ -617,13 +617,19 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status) >> static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) >> { >> struct aspeed_i2c_bus *bus = dev_id; >> - u32 irq_received, irq_remaining, irq_handled; >> + u32 irq_received, irq_remaining, irq_handled, irq_ack_last; > > `irq_ack_last` might be better as a macro, but you're probably saved by > the optimiser anyway. If there's another reason to do a v4 or others > are unhappy with it then consider fixing it, otherwise: > > Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au> > I'll send out the v4 to use the defined macro instead as below: Thanks Andi, for the suggestion on the macro name. diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c index 5511fd46a65e..0f67218cf04a 100644 --- a/drivers/i2c/busses/i2c-aspeed.c +++ b/drivers/i2c/busses/i2c-aspeed.c @@ -93,6 +93,10 @@ ASPEED_I2CD_INTR_RX_DONE | \ ASPEED_I2CD_INTR_TX_NAK | \ ASPEED_I2CD_INTR_TX_ACK) +#define ASPEED_I2CD_INTR_ACK_RX_TX \ + (ASPEED_I2CD_INTR_RX_DONE | \ + ASPEED_I2CD_INTR_TX_ACK | \ + ASPEED_I2CD_INTR_TX_NAK) Thanks for the review, - Quan
On 08/12/2023 10:31, Quan Nguyen wrote: > Commit 2be6b47211e1 ("i2c: aspeed: Acknowledge most interrupts early in > interrupt handler") acknowledges most interrupts early before the slave > irq handler is executed, except for the "Receive Done Interrupt status" > which is acknowledged late in the interrupt. > However, it has been observed that the early acknowledgment of "Transmit > Done Interrupt Status" (with ACK or NACK) often causes the interrupt to > be raised in READ REQUEST state, that shows the > "Unexpected ACK on read request." complaint messages. > > Assuming that the "Transmit Done" interrupt should only be acknowledged > once it is truly processed, this commit fixes that issue by acknowledging > interrupts for both ACK and NACK cases late in the interrupt handler. > > Fixes: 2be6b47211e1 ("i2c: aspeed: Acknowledge most interrupts early in interrupt handler") > Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com> > --- > v3: > + Fix the unconditinal write when ack the irqs [Andrew] > + Refactor the code to enhance code readability [Quan] > + Fix grammar in commit message [Quan] > > v2: > + Split to separate series [Joel] > + Added the Fixes line [Joel] > + Fixed multiline comment [Joel] > + Refactor irq clearing code [Joel, Guenter] > + Revised commit message [Joel] > + Revised commit message [Quan] > + About a note to remind why the readl() should immediately follow the > writel() to fix the race condition when clearing irq status from commit > c926c87b8e36 ("i2c: aspeed: Avoid i2c interrupt status clear race > condition"), I think it looks straight forward in this patch and decided > not to add that note. [Joel] > > v1: > + First introduced in > https://lore.kernel.org/all/20210519074934.20712-1-quan@os.amperecomputing.com/ > --- > drivers/i2c/busses/i2c-aspeed.c | 21 +++++++++++++-------- > 1 file changed, 13 insertions(+), 8 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c > index 1c2a4f4c4e1b..967a26dd4ffa 100644 > --- a/drivers/i2c/busses/i2c-aspeed.c > +++ b/drivers/i2c/busses/i2c-aspeed.c > @@ -617,13 +617,19 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status) > static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) > { > struct aspeed_i2c_bus *bus = dev_id; > - u32 irq_received, irq_remaining, irq_handled; > + u32 irq_received, irq_remaining, irq_handled, irq_ack_last; > > 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, > - bus->base + ASPEED_I2C_INTR_STS_REG); > + > + /* > + * Early acking of INTR_RX_DONE and INTR_TX_[ACK|NAK] would indicate HW to > + * start receiving or sending new data, and this may cause a race condition > + * as the irq handler has not yet handled these irqs but is being acked. > + * Let's ack them late at the end of the irq handler when those are truly processed. > + */ > + irq_ack_last = ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_TX_ACK | ASPEED_I2CD_INTR_TX_NAK; > + writel(irq_received & ~irq_ack_last, bus->base + ASPEED_I2C_INTR_STS_REG); As there will be v4, I'd like to improve this line of code a bit. During debug, there are significant number of times when the irq_received has the value of 0x00000004 (INTR_RX_DONE) or 0x00000001 (INTR_TX_ACK). This makes the "irq_received & ~irq_ack_last" turn out to be 0. AFAIK, it does not make sense to keep repeatedly writing the 0 to the HW then try to re-read it with readl() every time. I'd like to change this HW access conditionally to avoid those unnecessary access. The change would looks like below: diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c index 5511fd46a65e..0f67218cf04a 100644 --- a/drivers/i2c/busses/i2c-aspeed.c +++ b/drivers/i2c/busses/i2c-aspeed.c @@ -93,6 +93,10 @@ ASPEED_I2CD_INTR_RX_DONE | \ ASPEED_I2CD_INTR_TX_NAK | \ ASPEED_I2CD_INTR_TX_ACK) +#define ASPEED_I2CD_INTR_ACK_RX_TX \ + (ASPEED_I2CD_INTR_RX_DONE | \ + ASPEED_I2CD_INTR_TX_ACK | \ + ASPEED_I2CD_INTR_TX_NAK) /* 0x14 : I2CD Command/Status Register */ #define ASPEED_I2CD_SCL_LINE_STS BIT(18) @@ -622,10 +626,19 @@ 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, - bus->base + ASPEED_I2C_INTR_STS_REG); - readl(bus->base + ASPEED_I2C_INTR_STS_REG); + + /* + * Early acking of INTR_RX_DONE and INTR_TX_[ACK|NAK] would + ... + */ + if (irq_received & ~ASPEED_I2CD_INTR_ACK_RX_TX) { + writel(irq_received & ~ASPEED_I2CD_INTR_ACK_RX_TX, + bus->base + ASPEED_I2C_INTR_STS_REG); + readl(bus->base + ASPEED_I2C_INTR_STS_REG); + } + irq_received &= ASPEED_I2CD_INTR_RECV_MASK; irq_remaining = irq_received; Thanks, - Quan
On 10/12/2023 03:44, Andi Shyti wrote: > Hi Quan, > > [...] > >> - /* Ack all interrupts except for Rx done */ >> - writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE, >> - bus->base + ASPEED_I2C_INTR_STS_REG); >> + >> + /* >> + * Early acking of INTR_RX_DONE and INTR_TX_[ACK|NAK] would indicate HW to >> + * start receiving or sending new data, and this may cause a race condition >> + * as the irq handler has not yet handled these irqs but is being acked. >> + * Let's ack them late at the end of the irq handler when those are truly processed. >> + */ >> + irq_ack_last = ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_TX_ACK | ASPEED_I2CD_INTR_TX_NAK; >> + writel(irq_received & ~irq_ack_last, bus->base + ASPEED_I2C_INTR_STS_REG); > > I like Andrews suggestion of having irq_ack_last as a define that > is already negated, instead of negating it in the writel, which > makes it a bit difficult to read. > Yes, but the it still need to negate again when do the write to late ack them later in the end of irq handler. So I'll keep the define as below in my v4: +#define ASPEED_I2CD_INTR_ACK_RX_TX \ + (ASPEED_I2CD_INTR_RX_DONE | \ + ASPEED_I2CD_INTR_TX_ACK | \ + ASPEED_I2CD_INTR_TX_NAK) The early ack will look like this: + writel(irq_received & ~ASPEED_I2CD_INTR_ACK_RX_TX, + bus->base + ASPEED_I2C_INTR_STS_REG); + readl(bus->base + ASPEED_I2C_INTR_STS_REG); And the late ack: - /* Ack Rx done */ - if (irq_received & ASPEED_I2CD_INTR_RX_DONE) { - writel(ASPEED_I2CD_INTR_RX_DONE, + if (irq_received & ASPEED_I2CD_INTR_ACK_RX_TX) { + writel(irq_received & ASPEED_I2CD_INTR_ACK_RX_TX, bus->base + ASPEED_I2C_INTR_STS_REG); readl(bus->base + ASPEED_I2C_INTR_STS_REG); } > Besides, ack_last, as a name is not very meaningful, I'd rather > call it irq_ack_rx_tx (or something similar). > > But I'm not going to block it for this, up to you if you want to > send a new version. > > Reviewed-by: Andi Shyti <andi.shyti@kernel.org> > Thanks, Andi for the comments. I will send out v4 to address those. - Quan
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c index 1c2a4f4c4e1b..967a26dd4ffa 100644 --- a/drivers/i2c/busses/i2c-aspeed.c +++ b/drivers/i2c/busses/i2c-aspeed.c @@ -617,13 +617,19 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status) static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) { struct aspeed_i2c_bus *bus = dev_id; - u32 irq_received, irq_remaining, irq_handled; + u32 irq_received, irq_remaining, irq_handled, irq_ack_last; 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, - bus->base + ASPEED_I2C_INTR_STS_REG); + + /* + * Early acking of INTR_RX_DONE and INTR_TX_[ACK|NAK] would indicate HW to + * start receiving or sending new data, and this may cause a race condition + * as the irq handler has not yet handled these irqs but is being acked. + * Let's ack them late at the end of the irq handler when those are truly processed. + */ + irq_ack_last = ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_TX_ACK | ASPEED_I2CD_INTR_TX_NAK; + writel(irq_received & ~irq_ack_last, bus->base + ASPEED_I2C_INTR_STS_REG); readl(bus->base + ASPEED_I2C_INTR_STS_REG); irq_received &= ASPEED_I2CD_INTR_RECV_MASK; irq_remaining = irq_received; @@ -667,12 +673,11 @@ 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); + if (irq_received & irq_ack_last) { + writel(irq_received & irq_ack_last, bus->base + ASPEED_I2C_INTR_STS_REG); readl(bus->base + ASPEED_I2C_INTR_STS_REG); } + spin_unlock(&bus->lock); return irq_remaining ? IRQ_NONE : IRQ_HANDLED; }