From patchwork Wed Dec 18 12:07:40 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Conor Dooley X-Patchwork-Id: 13913565 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 6F379E7718A for ; Wed, 18 Dec 2024 12:08:18 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Message-ID:Date:Subject:Cc:To:From:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=diCwuCQNCca3vmBt1YQ03yOTzmdBzEWXfOc1yhAf5YU=; b=2hRzxya3vLtUJe F20eHtkPvBN9w/3VUteaMsN+ztto3I9VKs3LjF2DhqwuZ66Yb61vw+AnZORX8Zy3evz/eoNO9abOY iSb4lQY4zeKOOBmQl5jqBjPSjtEM/0Naz7NaBpNlQfLhHbKn7xONzRGbbbeao0FbMwbJvvvZi7SMW 2w5Mu//8Q7DKXSpsBjmE6yhTPKJg1YJ50KKE1bbp7L1Us53F15EtxwSuGK868G4ZEh44JV8IhE+uN mSao4uU8Ag3bF2pHnuN18ilLbHR07TlDsznTuxIG+lu79v3uTN/hu9M72740BbKYoXQhoM1+Xbs5n 8fZoABxbloxf9NqrcHmA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98 #2 (Red Hat Linux)) id 1tNsqY-0000000GXse-2Xgl; Wed, 18 Dec 2024 12:08:10 +0000 Received: from nyc.source.kernel.org ([2604:1380:45d1:ec00::3]) by bombadil.infradead.org with esmtps (Exim 4.98 #2 (Red Hat Linux)) id 1tNsqW-0000000GXru-0Zf3 for linux-riscv@lists.infradead.org; Wed, 18 Dec 2024 12:08:09 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by nyc.source.kernel.org (Postfix) with ESMTP id CFB4CA42627; Wed, 18 Dec 2024 12:06:16 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 689AFC4CED4; Wed, 18 Dec 2024 12:08:05 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1734523687; bh=AQLU0Q8cFQjLvr25vZUEK5lZ20NqxcXHXm5DN+eiw5M=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=BSxU3b7iAjpSsButUhn0fiEoV3VJlwsuI1jywq7fGH562E0Gb7wdKhHq62A2AFHSH TNmiW8sHrn9Kd0lkVOm3cYsFG7wTr4WoCoCgI4SISnBtgME05Vm+enRZibQXcXprIo Q3hSL0p7R7VRjEzrUZyq3l4NgkTcsQdwuXu7t/SO8ws2kiQXolSZcZhnv/biiqCvAT 12WxUCoAqSFRGb1w+JksES3q+64SZdy7dP0fviJ2U9WIXsVusPQUvVhpT4dpJlmC0j /f+yFEVU+v5fwql1ItGq+c7yPAanah8ZKifwIOOAAtlLSONmzKdR5bZ/kUUIFcPGP3 jhIo54GhtPNpQ== From: Conor Dooley To: linux-i2c@vger.kernel.org Cc: conor@kernel.org, Conor Dooley , Daire McNamara , Andi Shyti , Wolfram Sang , linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org Subject: [PATCH v2 1/2] i2c: microchip-core: actually use repeated sends Date: Wed, 18 Dec 2024 12:07:40 +0000 Message-ID: <20241218-football-composure-e56df2461461@spud> X-Mailer: git-send-email 2.45.2 In-Reply-To: <20241218-steadier-corridor-0c0a0ce58ca2@spud> References: <20241218-steadier-corridor-0c0a0ce58ca2@spud> MIME-Version: 1.0 X-Developer-Signature: v=1; a=openpgp-sha256; l=6229; i=conor.dooley@microchip.com; h=from:subject:message-id; bh=fyLCcxFql6reCLgGItzI26EB/OD8RBiql59BUhKOfsc=; b=owGbwMvMwCFWscWwfUFT0iXG02pJDOlJu7njs/TF3RRnu3Iax3K1rLhotO9xTJigxvaaC9cuf VFk5fHtKGVhEONgkBVTZEm83dcitf6Pyw7nnrcwc1iZQIYwcHEKwETO2zAynJ1yI1vwSVWEjsf8 jvLLv8ydYi53sfUGcvu4f27lepL8npFhkk3HkXNNlTqdu3n7Xuy25DOcUnJFacP/KULdslXfdBm ZAQ== X-Developer-Key: i=conor.dooley@microchip.com; a=openpgp; fpr=F9ECA03CF54F12CD01F1655722E2C55B37CF380C X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20241218_040808_301695_EBC92548 X-CRM114-Status: GOOD ( 23.41 ) X-BeenThere: linux-riscv@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-riscv" Errors-To: linux-riscv-bounces+linux-riscv=archiver.kernel.org@lists.infradead.org From: Conor Dooley At present, where repeated sends are intended to be used, the i2c-microchip-core driver sends a stop followed by a start. Lots of i2c devices must not malfunction in the face of this behaviour, because the driver has operated like this for years! Try to keep track of whether or not a repeated send is required, and suppress sending a stop in these cases. CC: stable@vger.kernel.org Fixes: 64a6f1c4987e ("i2c: add support for microchip fpga i2c controllers") Signed-off-by: Conor Dooley --- drivers/i2c/busses/i2c-microchip-corei2c.c | 124 ++++++++++++++++----- 1 file changed, 96 insertions(+), 28 deletions(-) diff --git a/drivers/i2c/busses/i2c-microchip-corei2c.c b/drivers/i2c/busses/i2c-microchip-corei2c.c index 0b0a1c4d17ca..e5af38dfaa81 100644 --- a/drivers/i2c/busses/i2c-microchip-corei2c.c +++ b/drivers/i2c/busses/i2c-microchip-corei2c.c @@ -93,27 +93,35 @@ * @base: pointer to register struct * @dev: device reference * @i2c_clk: clock reference for i2c input clock + * @msg_queue: pointer to the messages requiring sending * @buf: pointer to msg buffer for easier use * @msg_complete: xfer completion object * @adapter: core i2c abstraction * @msg_err: error code for completed message * @bus_clk_rate: current i2c bus clock rate * @isr_status: cached copy of local ISR status + * @total_num: total number of messages to be sent/received + * @current_num: index of the current message being sent/received * @msg_len: number of bytes transferred in msg * @addr: address of the current slave + * @restart_needed: whether or not a repeated start is required after current message */ struct mchp_corei2c_dev { void __iomem *base; struct device *dev; struct clk *i2c_clk; + struct i2c_msg *msg_queue; u8 *buf; struct completion msg_complete; struct i2c_adapter adapter; int msg_err; + int total_num; + int current_num; u32 bus_clk_rate; u32 isr_status; u16 msg_len; u8 addr; + bool restart_needed; }; static void mchp_corei2c_core_disable(struct mchp_corei2c_dev *idev) @@ -222,6 +230,47 @@ static int mchp_corei2c_fill_tx(struct mchp_corei2c_dev *idev) return 0; } +static void mchp_corei2c_next_msg(struct mchp_corei2c_dev *idev) +{ + struct i2c_msg *this_msg; + u8 ctrl; + + if (idev->current_num >= idev->total_num) { + complete(&idev->msg_complete); + return; + } + + /* + * If there's been an error, the isr needs to return control + * to the "main" part of the driver, so as not to keep sending + * messages once it completes and clears the SI bit. + */ + if (idev->msg_err) { + complete(&idev->msg_complete); + return; + } + + this_msg = idev->msg_queue++; + + if (idev->current_num < (idev->total_num - 1)) { + struct i2c_msg *next_msg = idev->msg_queue; + + idev->restart_needed = next_msg->flags & I2C_M_RD; + } else { + idev->restart_needed = false; + } + + idev->addr = i2c_8bit_addr_from_msg(this_msg); + idev->msg_len = this_msg->len; + idev->buf = this_msg->buf; + + ctrl = readb(idev->base + CORE_I2C_CTRL); + ctrl |= CTRL_STA; + writeb(ctrl, idev->base + CORE_I2C_CTRL); + + idev->current_num++; +} + static irqreturn_t mchp_corei2c_handle_isr(struct mchp_corei2c_dev *idev) { u32 status = idev->isr_status; @@ -247,10 +296,14 @@ static irqreturn_t mchp_corei2c_handle_isr(struct mchp_corei2c_dev *idev) break; case STATUS_M_SLAW_ACK: case STATUS_M_TX_DATA_ACK: - if (idev->msg_len > 0) + if (idev->msg_len > 0) { mchp_corei2c_fill_tx(idev); - else - last_byte = true; + } else { + if (idev->restart_needed) + finished = true; + else + last_byte = true; + } break; case STATUS_M_TX_DATA_NACK: case STATUS_M_SLAR_NACK: @@ -287,7 +340,7 @@ static irqreturn_t mchp_corei2c_handle_isr(struct mchp_corei2c_dev *idev) mchp_corei2c_stop(idev); if (last_byte || finished) - complete(&idev->msg_complete); + mchp_corei2c_next_msg(idev); return IRQ_HANDLED; } @@ -311,21 +364,48 @@ static irqreturn_t mchp_corei2c_isr(int irq, void *_dev) return ret; } -static int mchp_corei2c_xfer_msg(struct mchp_corei2c_dev *idev, - struct i2c_msg *msg) +static int mchp_corei2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, + int num) { - u8 ctrl; + struct mchp_corei2c_dev *idev = i2c_get_adapdata(adap); + struct i2c_msg *this_msg = msgs; unsigned long time_left; - - idev->addr = i2c_8bit_addr_from_msg(msg); - idev->msg_len = msg->len; - idev->buf = msg->buf; - idev->msg_err = 0; - - reinit_completion(&idev->msg_complete); + u8 ctrl; mchp_corei2c_core_enable(idev); + /* + * The isr controls the flow of a transfer, this info needs to be saved + * to a location that it can access the queue information from. + */ + idev->restart_needed = false; + idev->msg_queue = msgs; + idev->total_num = num; + idev->current_num = 0; + + /* + * But the first entry to the isr is triggered by the start in this + * function, so the first message needs to be "dequeued". + */ + idev->addr = i2c_8bit_addr_from_msg(this_msg); + idev->msg_len = this_msg->len; + idev->buf = this_msg->buf; + idev->msg_err = 0; + + if (idev->total_num > 1) { + struct i2c_msg *next_msg = msgs + 1; + + idev->restart_needed = next_msg->flags & I2C_M_RD; + } + + idev->current_num++; + idev->msg_queue++; + + reinit_completion(&idev->msg_complete); + + /* + * Send the first start to pass control to the isr + */ ctrl = readb(idev->base + CORE_I2C_CTRL); ctrl |= CTRL_STA; writeb(ctrl, idev->base + CORE_I2C_CTRL); @@ -335,20 +415,8 @@ static int mchp_corei2c_xfer_msg(struct mchp_corei2c_dev *idev, if (!time_left) return -ETIMEDOUT; - return idev->msg_err; -} - -static int mchp_corei2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, - int num) -{ - struct mchp_corei2c_dev *idev = i2c_get_adapdata(adap); - int i, ret; - - for (i = 0; i < num; i++) { - ret = mchp_corei2c_xfer_msg(idev, msgs++); - if (ret) - return ret; - } + if (idev->msg_err) + return idev->msg_err; return num; } From patchwork Wed Dec 18 12:07:42 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Conor Dooley X-Patchwork-Id: 13913566 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id B1E18E77187 for ; Wed, 18 Dec 2024 12:08:39 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Message-ID:Date:Subject:Cc:To:From:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=lWWEdM115UyeImuWLgaWj2Bi2SCM6iW+fa/5UWVxYaM=; b=XGhTlxhwTTnw1U xwaDK/pXaNoS3Yw534X61naQgFHb+R7CV5fcCOmvwaPRnu4IsW1Kf3oDAfbuDB4bV2eUleIsW0F5f g1r1EiCh7LHDHWNHJp1zAMZBwTNxmYHeotrDjeZ+/Y31wHclJcW8rW24qE/xY7GiRyDQz2E47PHLc wuW4lmx9twX9CFxcw1mATolRbXpwqHQVpMeACltrg7ZiPtMTRy2+AJ2m2L46jV3vxHglAOyRryxAH CDP7WaxamE9aDb2AZmx74T8u/wDA2kU1JQFK+1mi0Zihg03LvNUJ5YuhxSiFZCVaBBb7Gprc7JpaP 0n9Ta1XFotQkhOIriNzA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98 #2 (Red Hat Linux)) id 1tNsqx-0000000GY0D-0pj4; Wed, 18 Dec 2024 12:08:35 +0000 Received: from nyc.source.kernel.org ([147.75.193.91]) by bombadil.infradead.org with esmtps (Exim 4.98 #2 (Red Hat Linux)) id 1tNsqu-0000000GXzH-2Knm for linux-riscv@lists.infradead.org; Wed, 18 Dec 2024 12:08:33 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by nyc.source.kernel.org (Postfix) with ESMTP id 34962A4261C; Wed, 18 Dec 2024 12:06:41 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id C02D4C4CED7; Wed, 18 Dec 2024 12:08:29 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1734523711; bh=Aki9cR5hgIlgmZZzFSZjatM8ttFH9PdwQjT6VXm94Xo=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=aUINo1ISnQ7uJtkW9Z3SEBYlPgd/cSynL7tilEY/JlgoQ0jkgPUhUc8F/Uk++7Wfn S2eQMOr81PSoeWg3qnJnWQmQ8+PfiFXfyEGkD0ZswbRYtWuBzSDAYnuIam/ZHyPup5 6iu59KbOihwTo7onx89nLjFY6kxP+RlwFsI6sqGV47NdMxwf9Ty7HJY6SKSX50mUxf ZhLeR0yeqofZMjR8OFTjzPXhdtJp2iVvCYEKqKN/Z1I9/mZikIjCpKArgQROSRQOhu 2poZcIFMPYoHoAHscdPnIVc6gaiG3p3nj3KV2qNPWpIAKI3CBXrtVkqaOTeLyDjK+1 cqVqWEzqHhzzw== From: Conor Dooley To: linux-i2c@vger.kernel.org Cc: conor@kernel.org, Conor Dooley , Daire McNamara , Andi Shyti , Wolfram Sang , linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org Subject: [PATCH v2 2/2] i2c: microchip-core: fix "ghost" detections Date: Wed, 18 Dec 2024 12:07:42 +0000 Message-ID: <20241218-outbid-encounter-b2e78b1cc707@spud> X-Mailer: git-send-email 2.45.2 In-Reply-To: <20241218-steadier-corridor-0c0a0ce58ca2@spud> References: <20241218-steadier-corridor-0c0a0ce58ca2@spud> MIME-Version: 1.0 X-Developer-Signature: v=1; a=openpgp-sha256; l=2438; i=conor.dooley@microchip.com; h=from:subject:message-id; bh=+vHaPH1a1woczZ/LXoIUBZOD/s4yRKT5BR6VDjl0RiM=; b=owGbwMvMwCFWscWwfUFT0iXG02pJDOlJu7lvLYmacUjr4G2Drd0Xi1/fSprJ4Lep2PKAXOzF+ bqP7P2lOkpZGMQ4GGTFFFkSb/e1SK3/47LDuectzBxWJpAhDFycAjARPXZGhu8FjIt9p7x5+1D3 1u4jnI8Duh9uuCO6LXa5pbe9/doM88mMDFt/ffCdH/jSs4fJva3U5f3/SR3WhUXnP3PzPV6r8ea 8JQsA X-Developer-Key: i=conor.dooley@microchip.com; a=openpgp; fpr=F9ECA03CF54F12CD01F1655722E2C55B37CF380C X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20241218_040832_729123_2C96317A X-CRM114-Status: GOOD ( 15.10 ) X-BeenThere: linux-riscv@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-riscv" Errors-To: linux-riscv-bounces+linux-riscv=archiver.kernel.org@lists.infradead.org From: Conor Dooley Running i2c-detect currently produces an output akin to: 0 1 2 3 4 5 6 7 8 9 a b c d e f 00: 08 -- 0a -- 0c -- 0e -- 10: 10 -- 12 -- 14 -- 16 -- UU 19 -- 1b -- 1d -- 1f 20: -- 21 -- 23 -- 25 -- 27 -- 29 -- 2b -- 2d -- 2f 30: -- -- -- -- -- -- -- -- 38 -- 3a -- 3c -- 3e -- 40: 40 -- 42 -- 44 -- 46 -- 48 -- 4a -- 4c -- 4e -- 50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 60: 60 -- 62 -- 64 -- 66 -- 68 -- 6a -- 6c -- 6e -- 70: 70 -- 72 -- 74 -- 76 -- This happens because for an i2c_msg with a len of 0 the driver will mark the transmission of the message as a success once the START has been sent, without waiting for the devices on the bus to respond with an ACK/NAK. Since i2cdetect seems to run in a tight loop over all addresses the NAK is treated as part of the next test for the next address. Delete the fast path that marks a message as complete when idev->msg_len is zero after sending a START/RESTART since this isn't a valid scenario. CC: stable@vger.kernel.org Fixes: 64a6f1c4987e ("i2c: add support for microchip fpga i2c controllers") Signed-off-by: Conor Dooley --- My original tests with KASAN/UBSAN/PREEMPT_RT enabled saw far fewer of these "ghost" detections and the skip caused by the occupied address at 0x18 on this bus is part of my attribution of the problem. Unless I'm mistaken there's no scenario that you consider a message complete after sending a START/RESTART without waiting for the ACK/NAK and this code path I deleted is useless? Looking out of tree, it predates my involvement with the code so I don't know where it came from, nor is there anything like it in the bare-metal driver the linux one was based on. --- drivers/i2c/busses/i2c-microchip-corei2c.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/i2c/busses/i2c-microchip-corei2c.c b/drivers/i2c/busses/i2c-microchip-corei2c.c index e5af38dfaa81..b0a51695138a 100644 --- a/drivers/i2c/busses/i2c-microchip-corei2c.c +++ b/drivers/i2c/busses/i2c-microchip-corei2c.c @@ -287,8 +287,6 @@ static irqreturn_t mchp_corei2c_handle_isr(struct mchp_corei2c_dev *idev) ctrl &= ~CTRL_STA; writeb(idev->addr, idev->base + CORE_I2C_DATA); writeb(ctrl, idev->base + CORE_I2C_CTRL); - if (idev->msg_len == 0) - finished = true; break; case STATUS_M_ARB_LOST: idev->msg_err = -EAGAIN;