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 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 9F3DDC2BA1A for ; Fri, 21 Jun 2024 15:23:07 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=rkGR5EJentvUL1VShfMi+kg50D4pwOAkzJOrsMjz8qk=; b=URBLdNw43UJOMtkHcMnvJNY74q BtGI2kaL1qSrIaHyHvz0WVaGQSVrsULSJGALM4wtbayfFCYY3CbiRSkOpdRTSwiOv6uPgEaQFW21d GbgLNhT14BpEwtMbu+V8/ojniWl6IgzZVP6jp8OmtDuQSZEZOC2l4fxOBmA7IYl86+BpwR0cDZRuD +jeyWzPKUUggfGVT79ooPLCcYNCL3gLf6NE1yUn9OpkwUMYB+hvCPkvbSBmRe1RAMQO8dMkcWAuhR dlkYSFJDWTTqtJvMLsLvhl2sGuMZJmQF+lbCY68PpLdHZdYkg4x4ByU1TZn8tQdtXl86PVGABvVLo 6HvfMiCw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sKg6J-00000009isY-0R8X; Fri, 21 Jun 2024 15:22:55 +0000 Received: from mail-wr1-x42b.google.com ([2a00:1450:4864:20::42b]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sKg6E-00000009iri-3hLF for linux-arm-kernel@lists.infradead.org; Fri, 21 Jun 2024 15:22:52 +0000 Received: by mail-wr1-x42b.google.com with SMTP id ffacd0b85a97d-363bbd51050so1542774f8f.0 for ; Fri, 21 Jun 2024 08:22:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1718983369; x=1719588169; darn=lists.infradead.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=rkGR5EJentvUL1VShfMi+kg50D4pwOAkzJOrsMjz8qk=; b=AiKGL2/snWr3RLG24pXhdEdHkn3lxf6v/MGLYkWXvX7nq7M/QeuMoM2flMyg+66AqS 2FnMCo9N9a2uagvVa8fDw4tOBd0nLrH23Biu01ec7d1b5cAnKCyaFLHHeBqkPvE/m3cw XRPmlfKE9LeESf1mezBpCgKvOUr8vaPZ7k1mDF0QeqBoB4ZXJ7DZnfs7Fk+ZlgKnz6UI x39C3Mg9uozF+k+LTh9IPm18AEedlzRsxpalNcYdaE6GqAMSevnTwZPHU+1tCy5BVMP1 EtxaFJYRmsLfc19hiV13qTG7oSaVteirPfkuRG+8OmacMf8+yxCJjGRKfHKHXCelUuK+ ITqA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1718983369; x=1719588169; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=rkGR5EJentvUL1VShfMi+kg50D4pwOAkzJOrsMjz8qk=; b=UpkEbb1n96FvMEyWMchfhBN3+4hGuGlODBtnS7K0qVGHy0n8hqDP5DvKQXYiamxq6E U7SUvBNUS/eyHOXFfja/xB6CixAZEn3HPCcAJks5D5BXoXlIj4Qi/dD2KrLsz7/HTwk3 MoOHXzjMn+ZtBp3Q6pvfCn6KMq3PK7hQVRe52IxvrX0qPYDzZKbmdkx/1B79ZfqRkT14 Io/nFEWxsAFdPsER6PUQeOdmkh2FbQbsZFHG66uGwBXDA1jO/3gGu2zmqk0XxISNXXuB UnQs1LNxd0S2m44KtjrYRaeDHayTk8vLCatcB3rkJRdgfY35YRXZHVBApLRtibgEYy5r aSlQ== X-Forwarded-Encrypted: i=1; AJvYcCWnV2anufA28nyGTkc7HAhQZHxkUWAfCP8NMr8o+Y7ikA8enzn8tX0CKRhX5XdOgNNXXpYk1Dwxa8lRmwPcbCauxnes6287WGoTZm2sRqk6wRWYwUU= X-Gm-Message-State: AOJu0Yz/W9d8ikE4+I3zuCcYn1jm6orgB7V4vCrlnJZaJgcUutdUDFNP GOcHO4eFy8546vcUTXT2U7/Yd1CzfGA3jlM0xGhQcNYrnz1oon0L X-Google-Smtp-Source: AGHT+IF9NG8saDwxeiICFI/hTyWoeWVUie5mWBddjO6sIoRMdxV0gFAvfVhnV74lANeVsONmdrpDxg== X-Received: by 2002:adf:eac1:0:b0:362:3526:4ebb with SMTP id ffacd0b85a97d-36317d737cbmr6808799f8f.37.1718983368601; Fri, 21 Jun 2024 08:22:48 -0700 (PDT) Received: from eichest-laptop ([2a02:168:af72:0:b162:502a:9bd1:4c8b]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-36638d9c1aasm2020326f8f.55.2024.06.21.08.22.47 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 21 Jun 2024 08:22:48 -0700 (PDT) Date: Fri, 21 Jun 2024 17:22:46 +0200 From: Stefan Eichenberger To: o.rempel@pengutronix.de, kernel@pengutronix.de, andi.shyti@kernel.org, shawnguo@kernel.org, s.hauer@pengutronix.de, festevam@gmail.com, jic23@kernel.org, lars@metafoo.de, nuno.sa@analog.com, andriy.shevchenko@linux.intel.com, marcelo.schmitt@analog.com, gnstark@salutedevices.com, francesco.dolcini@toradex.com, wsa+renesas@sang-engineering.com, andrew@lunn.ch Cc: linux-i2c@vger.kernel.org, imx@lists.linux.dev, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org, Stefan Eichenberger Subject: Re: [RFC PATCH] i2c: imx: avoid rescheduling when waiting for bus not busy Message-ID: References: <20240531142437.74831-1-eichest@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240531142437.74831-1-eichest@gmail.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240621_082251_016504_47707373 X-CRM114-Status: GOOD ( 46.73 ) 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: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Andi, Andrew, Wolfram, Oleksij, After some internal discussion we still have some questions which are blocking us from solving the issue. On Fri, May 31, 2024 at 04:24:37PM +0200, Stefan Eichenberger wrote: > From: Stefan Eichenberger > > On our i.MX8M Mini based module we have an ADS1015 I2C ADC connected to > the I2C bus. The ADS1015 I2C ADC will timeout after 25ms when the I2C > bus is idle. The imx i2c driver will call schedule when waiting for the > bus to become idle after switching to master mode. When the i2c > controller switches to master mode it pulls SCL and SDA low, if the > ADS1015 I2C ADC sees this for more than 25 ms without seeing SCL > clocking, it will timeout and ignore all signals until the next start > condition occurs (SCL and SDA low). This can occur when the system load > is high and schedule returns after more than 25 ms. > > This rfc tries to solve the problem by using a udelay for the first 10 > ms before calling schedule. This reduces the chance that we will > reschedule. However, it is still theoretically possible for the problem > to occur. To properly solve the problem, we would also need to disable > interrupts during the transfer. > > After some internal discussion, we see three possible solutions: > 1. Use udelay as shown in this rfc and also disable the interrupts > during the transfer. This would solve the problem but disable the > interrupts. Also, we would have to re-enable the interrupts if the > timeout is longer than 1ms (TBD). > 2. We use a retry mechanism in the ti-ads1015 driver. When we see a > timeout, we try again. > 3. We use the suggested solution and accept that there is an edge case > where the timeout can happen. > > There may be a better way to do this, which is why this is an RFC. > > Signed-off-by: Stefan Eichenberger > --- > drivers/i2c/busses/i2c-imx.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c > index 3842e527116b7..179f8367490a5 100644 > --- a/drivers/i2c/busses/i2c-imx.c > +++ b/drivers/i2c/busses/i2c-imx.c > @@ -503,10 +503,18 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy, bool a > "<%s> I2C bus is busy\n", __func__); > return -ETIMEDOUT; > } > - if (atomic) > + if (atomic) { > udelay(100); > - else > - schedule(); > + } else { > + /* > + * Avoid rescheduling in the first 10 ms to avoid > + * timeouts for SMBus like devices > + */ > + if (time_before(jiffies, orig_jiffies + msecs_to_jiffies(10))) > + udelay(10); > + else > + schedule(); > + } > } > > return 0; > -- > 2.40.1 If we want to be sure that the ADS1015 I2C ADC will never timeout, we would have to add a patch to disable preemption during transmission. This would look like this: @@ -1244,6 +1248,12 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter, bool is_lastmsg = false; struct imx_i2c_struct *i2c_imx = i2c_get_adapdata(adapter); + /* If we are in SMBus mode we need to do the transfer atomically */ + if (i2c_imx->smbus_mode) { + preempt_disable(); + atomic = true; + } + /* Start I2C transfer */ result = i2c_imx_start(i2c_imx, atomic); if (result) { @@ -1320,6 +1330,9 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter, if (i2c_imx->slave) i2c_imx_slave_init(i2c_imx); + if (i2c_imx->smbus_mode) + preempt_enable(); + return (result < 0) ? result : num; } However, we are aware that disabling preemption is not a good idea. So we were discussing how this is normally handled with SMBus devices? Is it just expected that SMBus devices will timeout in rare cases? For our use case, the problem would be solved if we could get rid of the schedule call and replace it with a udelay. It seems that the i.MX8M Mini I2C controller needs a few ms to clear the IBB flag. In the reference manual, they write: > I2C bus busy bit. Indicates the status of the bus. NOTE: When I2C is > enabled (I2C_I2CR[IEN] = 1), it continuously polls the bus data (SDA) > and clock (SCL) signals to determine a Start or Stop condition. Bus is > idle. If a Stop signal is detected, IBB is cleared. Bus is busy. When > Start is detected, IBB is set. Unfortunately, it is not clear how often they poll. In our tests the issue disappeard when we used udelay instead of usleep or schedule for the first 10 ms. Since we know we don't have a multi-master configuration, another way would be to not test for IBB and just start the transfer. We saw that other drivers use the multi-master device tree property to determine if multi-master is supported. Here another quote from the reference manual: > On a multimaster bus system, the busy bus (I2C_I2SR[IBB]) must be > tested to determine whether the serial bus is free. We assume this means it is not necessary to test for IBB if we know we are in a single-master configuration. Would either of these workarounds be acceptable, and which would be preferred? Thanks, Stefan