Message ID | 20231211102217.2436294-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 r17csp6946860vqy; Mon, 11 Dec 2023 02:23:03 -0800 (PST) X-Google-Smtp-Source: AGHT+IH04Pj1yBTiOV8neCwMlWgbwKf0hKKwTerzsusJoY7DHq05oM8LcHojQRmYrtNXkFu94xzT X-Received: by 2002:a05:6a00:1993:b0:6cb:b818:c7fc with SMTP id d19-20020a056a00199300b006cbb818c7fcmr2220679pfl.23.1702290183216; Mon, 11 Dec 2023 02:23:03 -0800 (PST) ARC-Seal: i=2; a=rsa-sha256; t=1702290183; cv=pass; d=google.com; s=arc-20160816; b=FPTupskzCVgrS1PSNXmLjiyKI8KuQJ6vHG/FBKpRsVNpmJSVNCSv8KnQaqZ+UV/Z8n woqlEEQckyiCGlzXj03Ubb+wV+xa5XH6kvtgbZWNLojNn179XF6OhztF0EHcGfN8Y1jd 7PNIEqsQvdwHY897k8wJJEQ9xGPmSP9dX95G2bCYlD3EnyrKGB/sTvyyu5E5K0mPri/x 8C5oeS8rNg9fI1vAu6GxDRUKlQlcDv1U3RJUxeuplQmlMz2pqH6JNDoePWHRTVb5tEsv qZ1iAIAE3DeBp96s2hXRpQIHNGHz8V5yEYLNTeMup/HgdxVdYBPxNCTHuSkEWsWFnPy4 GhbA== 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=AL8ucwBp7iSZS1mOKdfvDd6/FN0LMGhmG9Mje9C0kSA=; fh=WFcqSubssMIwC0Dm7Plh6RRvLP+WIFWeZgWd743wkLo=; b=HoEpyXACUWewk0EyG0LI1zc7teLxwPPKd4aYRbQrmcUOIq0Hj3/T3+aWxK2WAuwLYw q4mQw1NCGl/MSabWuOuVAAvFrcFJ+1NOZ9J8mJPElG2cAvAw+hX04u0QDv4sgrA1+/0o JeN4X2Q97vJIyaTsjJHvcUaseZIWWLxC5IYUrR9Baql6Rgcnx/mYwzR7I5gRvsEwER80 TkkofFSnh/a2ghomCJt0xoX7EGZkOpi0um3IGlIbJCZ1orHzRt7RTqP0UZUaEjiXjbOE khptvtRSF6qsfb/4cBRMMHetq19v8zX9c+KduKiNyLq1ay0jEImF8GVNznibQ2hK1/Zc p1hA== ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@os.amperecomputing.com header.s=selector2 header.b=n9JsUr30; 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 2620:137:e000::3:7 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 snail.vger.email (snail.vger.email. [2620:137:e000::3:7]) by mx.google.com with ESMTPS id j27-20020a63231b000000b005be327e08afsi5833545pgj.224.2023.12.11.02.23.01 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 11 Dec 2023 02:23:03 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7; Authentication-Results: mx.google.com; dkim=pass header.i=@os.amperecomputing.com header.s=selector2 header.b=n9JsUr30; 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 2620:137:e000::3:7 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 snail.vger.email (Postfix) with ESMTP id 3144D809D3CC; Mon, 11 Dec 2023 02:23:01 -0800 (PST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at snail.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234918AbjLKKWs (ORCPT <rfc822;dexuan.linux@gmail.com> + 99 others); Mon, 11 Dec 2023 05:22:48 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35332 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234878AbjLKKWq (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 11 Dec 2023 05:22:46 -0500 Received: from NAM11-BN8-obe.outbound.protection.outlook.com (mail-bn8nam11on2100.outbound.protection.outlook.com [40.107.236.100]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id DDD6CE5; Mon, 11 Dec 2023 02:22:51 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=WDidnJPACR/yJ0Xsc1RdAqqO7RjX8fZwHDwQMxjo7CC1xfL81Ti/61MeplPhpra0YxcA4sw/1HDzx5Y2k3Cc+X4I1F0fAlsbCMMm0IjT7mkBgeJZgS+KUQm4L8T9aVUvnZADViXIQ9/szQfAxAZReRPCezCHLe2tGP11U6srcp0/V4t5216SIP9RGJNlh/Yp9DAFXn3lODGpCAC0d4AozrWUFS9DRLiqwCnpVoVt0uJ+AsLT4VUVka21igSg5zerqgCq38BRNBomVWDtAyp3iIHbgykOOMhvSdA4EnheGGqlfWl1bgnFIXviGahR2CAXIi/U4pGRvUQ/fH0Q9kLmkw== 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=AL8ucwBp7iSZS1mOKdfvDd6/FN0LMGhmG9Mje9C0kSA=; b=S/Jgydl1F99LJwmnMpoPMMfZ23emQgjLWhYhzYfu2TqV4kADXQLRXcZVpYCIQLqiw4yNHEeEyLm23avYoST5RoqVS3US/nrEWMsHGj6xlTdhYRRLVmKEcBW/vOO6fOeisaBNMPb06t+pnD/ybh8z04Q/i5hcbeVRC+vG1spfb0GuP7SJ6i7SZxt0HPdWfAqDBZ8wStmG3Bn2qjOZnpuM9m1H6HvyQVD6pRVHlMgu8uXVOQE6TxXWF+9piejVpR3FFq6b2GWzAUf2N7CHzt+ZbdYLd5iJYCzwauSkLHuMmgPARWgwkFWBk/7IzXH/QiYw0S4rPY135fTikA/GvTtyuQ== 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=AL8ucwBp7iSZS1mOKdfvDd6/FN0LMGhmG9Mje9C0kSA=; b=n9JsUr30Xzb3j/SbO45wKRo/uEs21FVacYgyyaYA/3nGcZSFTbG3txYtveVH7BneyZRjzbsbblGzFXKGfGG8Hh8z455aPYX/uhkR08XyiDlBG+pVsfdrEtyRJxBkghpEBVWmMtc/Ap0rUrFdK2bmV68XS7qP1kEyMy6Rfbk+24E= 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 CO1PR01MB6568.prod.exchangelabs.com (2603:10b6:303:f9::19) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7068.33; Mon, 11 Dec 2023 10:22:49 +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.031; Mon, 11 Dec 2023 10:22:49 +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 v4 2/2] i2c: aspeed: Acknowledge Tx done with and without ACK irq late Date: Mon, 11 Dec 2023 17:22:17 +0700 Message-Id: <20231211102217.2436294-3-quan@os.amperecomputing.com> X-Mailer: git-send-email 2.35.1 In-Reply-To: <20231211102217.2436294-1-quan@os.amperecomputing.com> References: <20231211102217.2436294-1-quan@os.amperecomputing.com> Content-Transfer-Encoding: 8bit Content-Type: text/plain X-ClientProxiedBy: SG2P153CA0007.APCP153.PROD.OUTLOOK.COM (2603:1096::17) To SN4PR01MB7455.prod.exchangelabs.com (2603:10b6:806:202::11) MIME-Version: 1.0 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: SN4PR01MB7455:EE_|CO1PR01MB6568:EE_ X-MS-Office365-Filtering-Correlation-Id: a303e0ec-eb89-417a-744d-08dbfa331fd5 X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: U9mw8C6ILK3csf8axLoZ2t6tMXF7AO5KSaNrw6FKIUmMZSFIXwD+EtS+t2s1sLNHq21eDoFTc0xpvRdh150sWIiED5njgrQOoKCIAijWOhYS3tG1VIvT5B95goL4d1Uj1rXqBj3byYDVaySjSl2ItTjKpkwFMJDNFJFLvScISpKyEEB6FJnu+zceahim+nu/z/DBwFIaD9GfU10LzeK6VxkVPyjzcrnoYxKIBa52cu+YqhEklgoMvPSjtD/hyle3YYofk5v+AmYuPqk0aEYpugl2vzICAbJFqsoTKk662BriWxFpOiKA0A5h28fLiXsQjLPPeJ8lHdGaBMdX4NcQlP0+xSJp2Eog6PeWH+tZu2Q874jvDf2c6nBGz/Fg4oYab10yxdZBgnOO9g6EI2f1zBrSQMUQSqEzGZCfU/qflYyW3EHzQTI1bbQ9bB7KQIJU2ILwaq93RT4eiFywlTOFUJFBtZlUHiBsQL7GM0aU/1rebaS3vn6r9m7WgC890PoHPkeMm7x7VNcuiyFzPL73xJREuCM6HQd+6mnof0VRkIOJ5pOseN9Gy+Ehj7wYTwu1HOS3FZiFRec4fAdsioRLDogyLJMdO7p2oHRbbqd7LPL4PA+3MBsqWOxfnOJUMMC7 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)(376002)(39850400004)(136003)(396003)(346002)(366004)(230922051799003)(451199024)(1800799012)(186009)(64100799003)(1076003)(26005)(107886003)(2616005)(6506007)(6512007)(6666004)(52116002)(83380400001)(5660300002)(7416002)(41300700001)(2906002)(4326008)(478600001)(966005)(6486002)(8676002)(8936002)(66946007)(110136005)(66556008)(316002)(66476007)(54906003)(86362001)(38100700002)(38350700005)(921008);DIR:OUT;SFP:1102; X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: lHduaNS8DVTh9rEkMjoGrFTJdjb3/AG2NOLmhLfENOAT/NMI4E0Be5DpsA3OKoLVzlQyMs9IQGzEvOrZ3qz3ukLTcOnaxvmVYzTJte9aO5Std4Fga4UIDmETRYypn+7lvkLTRZIE6d8BNvyA+UBuLNvcQ0JP+D5ntL7mcFwTfnMmdRjvmy4V9x7qQ3bvV8qyPkCMD9GTHbZpJDvuskAMzeB1l06260QX9SOkso6AyxfTFa0DamAajX/+vVDNZNZdb1euUHay8q6Ckx49grcfgTkq57oD0Z0wQLwYkab6aZwZjgZ/+/ZjGfbUXthXYqiD6GtcReOLpH1TjOxzFxsP2ZdSeks12E6ib/bpiI3EqIWgAdZ75AcVhpwI4nsbu4X2Ojh3HFAxIGHMB6xyKj3gh9Y5QjtWaTDQZsZMdKAANbL5fOlS6ZKTEQFrjYvHCoJta7vh5JQ2U9OzZJlQPiMP079LOVTlxNl4xj6BqBnpLc1t7FNHZEkqN4DQIFUjyQ9/NQ29UylTTVd/FeO8FIHP1DMkWnIeZ2XoB/cBG6uRF4J4Jq2rqImwaURLo0QPPsdyqXMhYflv3+GqcmRNJR8n459fW5e7VUM/cWugCQTHVt3SHYgBtPUw8mTemXd5kNP3/z5oLAAhSnAK3esCXk0weu9tuikIatKFlCOYl0LkMsUm2KKMqJZTIt/d9ukzOf1shW7U7H9pqpbGa1rHq2Jq2uz16qRicuqOd20sA+VrIeXrdAgAZqQE5Q0k8kptmJ/lM10PDWcRK86sAJfZxvWiWrnaHhDSNaCROWWK3OaNScD1I11DuRG9lzaiolnU2LTpuOZTGHo9U+5pwg/bPFZiYhuG6Z70M2K8mQYNBmb+xs9Rdv3irRBJh2+hzqRhDCyjFlYFpmITgi+fJt5siORHX6v4iEno/XINZENB55S3WFt48t129DHzlkH46o0BlrAk1k+1Gjirq1NSgvCqi8BUjepP/pQbfpxS+DKV35bCW4v43XBp/hB7t5Ox+Hktwrp3+MuUouVs0NRFn/GcARXGTN6xSTG5a4MHND5cORLoHnA2Pmn2IzLA8ugDv/7vWGbWHpa7dFjkTsHTDELFliOYAbnGdpuh3rLvvoJG06bGd+7EofLpk91uEBgIdBVsphStGbsJputrMdvjWSUgwiePRvDGfWvKNIpr9sOFLjSs7ecIaKq86H7pfBahPyaSckhXoBLa+kgp44hScV91kNjpX9kd4264wRFjdZghArsqvCwuLR70OXRIwU+EIZHasM/AgveGeUztzI1x1EffXAIqaGoHgT5nMsXkbkGkqaQDz/xyeRNVM5WzQ57RKRV/OON/ovPqQKVKWN0pUvPkLUquJecmjOsaEqIfeAhcoRHIj5tdfzf6hvezt/NwisMCWAmoe+bFBv806f4i/9s1kirhlxfY3iuRz4vvKZ3PUptRBuoHRunZPxD2sdtCdo8TSMGDTPrY+K4XhOL/MByV30bGnKAhowolzJBiFrCBVeotRLkKkUkItn+7eO/bEXg5BHYNZXsRr/lEKh69X9CQLfLXkow8N/G4TPd7Jt6x7UA2M6NVGEs7/hTvr+jbWsUBXG2cfJTtC2OttmH2tiBLO8+i+fE5kvhlyyDJ8dwk2sQmUDE= X-OriginatorOrg: os.amperecomputing.com X-MS-Exchange-CrossTenant-Network-Message-Id: a303e0ec-eb89-417a-744d-08dbfa331fd5 X-MS-Exchange-CrossTenant-AuthSource: SN4PR01MB7455.prod.exchangelabs.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Dec 2023 10:22:49.1549 (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: PItBE7hQJPUk1izhwECgYdVAJft+7UDK0p7N2wmh4omF1x975WbDKas1MPK9DhOef3sW9bJsTMWYQG6MVpLVByfIORcLa6ukgJqjVLG2GmE= X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR01MB6568 X-Spam-Status: No, score=-2.0 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2, SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net 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 (snail.vger.email [0.0.0.0]); Mon, 11 Dec 2023 02:23:01 -0800 (PST) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1784980631125704527 X-GMAIL-MSGID: 1784980631125704527 |
Series |
i2c: aspeed: Late ack Tx done irqs and handle coalesced start with stop conditions
|
|
Commit Message
Quan Nguyen
Dec. 11, 2023, 10:22 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>
---
v4:
+ Switch to use define macro instead of variable [Andrew]
+ Make the early ack conditionally to avoid unnecessary
writel()/readl() [Quan]
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 | 27 ++++++++++++++++++++-------
1 file changed, 20 insertions(+), 7 deletions(-)
Comments
On Mon, 2023-12-11 at 17:22 +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> Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au> Thanks Quan!
> On Mon, 2023-12-11 at 17:22 +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> > > Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au> So I just booted this series on v6.7-rc5 under qemu v8.2.0-rc4 and found this: ``` $ qemu-system-arm \ -M ast2600-evb \ -kernel build.aspeed_g5/arch/arm/boot/zImage \ -dtb build.aspeed_g5/arch/arm/boot/dts/aspeed/aspeed-ast2600-evb.dtb \ -initrd ~/src/buildroot.org/buildroot/output/images/rootfs.cpio.xz \ -nographic 2>&1 \ | ts -s ... 00:00:03 [ 1.089187] Freeing initrd memory: 3308K 00:00:05 smbus: error: Unexpected send start condition in state 1 00:00:05 smbus: error: Unexpected write in state -1 00:00:06 [ 3.685731] aspeed-i2c-bus 1e78a400.i2c-bus: i2c bus 7 registered, irq 48 00:00:06 [ 3.688918] aspeed-i2c-bus 1e78a480.i2c-bus: i2c bus 8 registered, irq 49 00:00:06 [ 3.692326] aspeed-i2c-bus 1e78a500.i2c-bus: i2c bus 9 registered, irq 50 00:00:06 [ 3.693757] aspeed-i2c-bus 1e78a680.i2c-bus: i2c bus 12 registered, irq 51 00:00:06 [ 3.695070] aspeed-i2c-bus 1e78a700.i2c-bus: i2c bus 13 registered, irq 52 00:00:06 [ 3.696184] aspeed-i2c-bus 1e78a780.i2c-bus: i2c bus 14 registered, irq 53 00:00:06 [ 3.697144] aspeed-i2c-bus 1e78a800.i2c-bus: i2c bus 15 registered, irq 54 00:00:06 [ 3.699061] aspeed-video 1e700000.video: irq 55 00:00:06 [ 3.699254] aspeed-video 1e700000.video: assigned reserved memory node video 00:00:06 [ 3.702755] aspeed-video 1e700000.video: alloc mem size(24576) at 0xbc000000 for jpeg header 00:00:06 [ 3.706139] Driver for 1-wire Dallas network protocol. 00:00:07 smbus: error: Unexpected send start condition in state -1 00:00:07 smbus: error: Unexpected write in state -1 00:00:10 smbus: error: Unexpected send start condition in state -1 00:00:10 smbus: error: Unexpected write in state -1 00:00:12 smbus: error: Unexpected send start condition in state -1 00:00:12 smbus: error: Unexpected write in state -1 00:00:14 smbus: error: Unexpected send start condition in state -1 00:00:14 smbus: error: Unexpected write in state -1 00:00:17 smbus: error: Unexpected send start condition in state -1 00:00:17 smbus: error: Unexpected write in state -1 00:00:18 [ 14.080141] adt7475 7-002e: Error configuring attenuator bypass 00:00:19 smbus: error: Unexpected send start condition in state -1 00:00:19 smbus: error: Unexpected write in state -1 00:00:21 smbus: error: Unexpected send start condition in state -1 00:00:21 smbus: error: Unexpected write in state -1 00:00:24 smbus: error: Unexpected send start condition in state -1 00:00:24 smbus: error: Unexpected write in state -1 ``` The smbus errors do not occur if I revert this patch. Can you look into qemu to see if it's a bug in the aspeed i2c controller model's state machine? Cheers, Andrew
Hi Quan, On Mon, Dec 11, 2023 at 05:22:17PM +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> Reviewed-by: Andi Shyti <andi.shyti@kernel.org> Thanks, Andi
On 15/12/2023 05:21, Andrew Jeffery wrote: >> On Mon, 2023-12-11 at 17:22 +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> >> >> Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au> > > So I just booted this series on v6.7-rc5 under qemu v8.2.0-rc4 and > found this: > > ``` > $ qemu-system-arm \ > -M ast2600-evb \ > -kernel build.aspeed_g5/arch/arm/boot/zImage \ > -dtb build.aspeed_g5/arch/arm/boot/dts/aspeed/aspeed-ast2600-evb.dtb \ > -initrd ~/src/buildroot.org/buildroot/output/images/rootfs.cpio.xz \ > -nographic 2>&1 \ > | ts -s > ... > 00:00:03 [ 1.089187] Freeing initrd memory: 3308K > 00:00:05 smbus: error: Unexpected send start condition in state 1 > 00:00:05 smbus: error: Unexpected write in state -1 > 00:00:06 [ 3.685731] aspeed-i2c-bus 1e78a400.i2c-bus: i2c bus 7 registered, irq 48 > 00:00:06 [ 3.688918] aspeed-i2c-bus 1e78a480.i2c-bus: i2c bus 8 registered, irq 49 > 00:00:06 [ 3.692326] aspeed-i2c-bus 1e78a500.i2c-bus: i2c bus 9 registered, irq 50 > 00:00:06 [ 3.693757] aspeed-i2c-bus 1e78a680.i2c-bus: i2c bus 12 registered, irq 51 > 00:00:06 [ 3.695070] aspeed-i2c-bus 1e78a700.i2c-bus: i2c bus 13 registered, irq 52 > 00:00:06 [ 3.696184] aspeed-i2c-bus 1e78a780.i2c-bus: i2c bus 14 registered, irq 53 > 00:00:06 [ 3.697144] aspeed-i2c-bus 1e78a800.i2c-bus: i2c bus 15 registered, irq 54 > 00:00:06 [ 3.699061] aspeed-video 1e700000.video: irq 55 > 00:00:06 [ 3.699254] aspeed-video 1e700000.video: assigned reserved memory node video > 00:00:06 [ 3.702755] aspeed-video 1e700000.video: alloc mem size(24576) at 0xbc000000 for jpeg header > 00:00:06 [ 3.706139] Driver for 1-wire Dallas network protocol. > 00:00:07 smbus: error: Unexpected send start condition in state -1 > 00:00:07 smbus: error: Unexpected write in state -1 > 00:00:10 smbus: error: Unexpected send start condition in state -1 > 00:00:10 smbus: error: Unexpected write in state -1 > 00:00:12 smbus: error: Unexpected send start condition in state -1 > 00:00:12 smbus: error: Unexpected write in state -1 > 00:00:14 smbus: error: Unexpected send start condition in state -1 > 00:00:14 smbus: error: Unexpected write in state -1 > 00:00:17 smbus: error: Unexpected send start condition in state -1 > 00:00:17 smbus: error: Unexpected write in state -1 > 00:00:18 [ 14.080141] adt7475 7-002e: Error configuring attenuator bypass > 00:00:19 smbus: error: Unexpected send start condition in state -1 > 00:00:19 smbus: error: Unexpected write in state -1 > 00:00:21 smbus: error: Unexpected send start condition in state -1 > 00:00:21 smbus: error: Unexpected write in state -1 > 00:00:24 smbus: error: Unexpected send start condition in state -1 > 00:00:24 smbus: error: Unexpected write in state -1 > ``` > > The smbus errors do not occur if I revert this patch. > > Can you look into qemu to see if it's a bug in the aspeed i2c > controller model's state machine? > Thanks, Andrew, for testing these patches on qemu. I'll try to look into it to see if anything can be improved, but I have to admit that I'm not so familiar with it. This is my first time trying it on qemu. Just did these tests on real HW with waveform captured sometimes. So far I could be able to reproduce the issue and start playing around trying to understand the model. Thanks, - Quan
On Mon, 2023-12-18 at 15:45 +0700, Quan Nguyen wrote: > > On 15/12/2023 05:21, Andrew Jeffery wrote: > > > > ``` > > $ qemu-system-arm \ > > -M ast2600-evb \ > > -kernel build.aspeed_g5/arch/arm/boot/zImage \ > > -dtb build.aspeed_g5/arch/arm/boot/dts/aspeed/aspeed-ast2600-evb.dtb \ > > -initrd ~/src/buildroot.org/buildroot/output/images/rootfs.cpio.xz \ > > -nographic 2>&1 \ > > | ts -s > > ... > > 00:00:03 [ 1.089187] Freeing initrd memory: 3308K > > 00:00:05 smbus: error: Unexpected send start condition in state 1 > > 00:00:05 smbus: error: Unexpected write in state -1 > > 00:00:06 [ 3.685731] aspeed-i2c-bus 1e78a400.i2c-bus: i2c bus 7 registered, irq 48 > > 00:00:06 [ 3.688918] aspeed-i2c-bus 1e78a480.i2c-bus: i2c bus 8 registered, irq 49 > > 00:00:06 [ 3.692326] aspeed-i2c-bus 1e78a500.i2c-bus: i2c bus 9 registered, irq 50 > > 00:00:06 [ 3.693757] aspeed-i2c-bus 1e78a680.i2c-bus: i2c bus 12 registered, irq 51 > > 00:00:06 [ 3.695070] aspeed-i2c-bus 1e78a700.i2c-bus: i2c bus 13 registered, irq 52 > > 00:00:06 [ 3.696184] aspeed-i2c-bus 1e78a780.i2c-bus: i2c bus 14 registered, irq 53 > > 00:00:06 [ 3.697144] aspeed-i2c-bus 1e78a800.i2c-bus: i2c bus 15 registered, irq 54 > > 00:00:06 [ 3.699061] aspeed-video 1e700000.video: irq 55 > > 00:00:06 [ 3.699254] aspeed-video 1e700000.video: assigned reserved memory node video > > 00:00:06 [ 3.702755] aspeed-video 1e700000.video: alloc mem size(24576) at 0xbc000000 for jpeg header > > 00:00:06 [ 3.706139] Driver for 1-wire Dallas network protocol. > > 00:00:07 smbus: error: Unexpected send start condition in state -1 > > 00:00:07 smbus: error: Unexpected write in state -1 > > 00:00:10 smbus: error: Unexpected send start condition in state -1 > > 00:00:10 smbus: error: Unexpected write in state -1 > > 00:00:12 smbus: error: Unexpected send start condition in state -1 > > 00:00:12 smbus: error: Unexpected write in state -1 > > 00:00:14 smbus: error: Unexpected send start condition in state -1 > > 00:00:14 smbus: error: Unexpected write in state -1 > > 00:00:17 smbus: error: Unexpected send start condition in state -1 > > 00:00:17 smbus: error: Unexpected write in state -1 > > 00:00:18 [ 14.080141] adt7475 7-002e: Error configuring attenuator bypass > > 00:00:19 smbus: error: Unexpected send start condition in state -1 > > 00:00:19 smbus: error: Unexpected write in state -1 > > 00:00:21 smbus: error: Unexpected send start condition in state -1 > > 00:00:21 smbus: error: Unexpected write in state -1 > > 00:00:24 smbus: error: Unexpected send start condition in state -1 > > 00:00:24 smbus: error: Unexpected write in state -1 > > ``` > > > > The smbus errors do not occur if I revert this patch. > > > > Can you look into qemu to see if it's a bug in the aspeed i2c > > controller model's state machine? > > > > Thanks, Andrew, for testing these patches on qemu. > > I'll try to look into it to see if anything can be improved, but I have > to admit that I'm not so familiar with it. This is my first time trying > it on qemu. Just did these tests on real HW with waveform captured > sometimes. > > So far I could be able to reproduce the issue and start playing around > trying to understand the model. > So `$ git grep -lF 'Unexpected write in state'` leads us to hw/i2c/smbus_slave.c:193. From the switch statement there and the log output above dev->mode must be SMBUS_CONFUSED: https://gitlab.com/qemu-project/qemu/-/blob/039afc5ef7367fbc8fb475580c291c2655e856cb/hw/i2c/smbus_slave.c#L35-L41 The prior log message was: ``` 00:00:05 smbus: error: Unexpected send start condition in state 1 ``` So we entered SMBUS_CONFUSED from SMBUS_WRITE_DATA. Given the log output above it suggests the master model is failing to send an I2C_FINISH prior to I2C_START_SEND, as that log message is emitted from `dev->mode != SMBUS_IDLE` when the slave sees an `I2C_START_SEND`. Perhaps the M_STOP_CMD handling needs to go above the M_START_CMD handling in aspeed_i2c_bus_handle_cmd()? https://gitlab.com/qemu-project/qemu/-/blob/039afc5ef7367fbc8fb475580c291c2655e856cb/hw/i2c/aspeed_i2c.c#L450 Andrew
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 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. + */ + 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; @@ -668,12 +681,12 @@ 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, + 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); } + spin_unlock(&bus->lock); return irq_remaining ? IRQ_NONE : IRQ_HANDLED; }