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 phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 6817BECAAA1 for ; Fri, 9 Sep 2022 18:02:33 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 11C6684B2D; Fri, 9 Sep 2022 20:02:31 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=foss.st.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=foss.st.com header.i=@foss.st.com header.b="xkNsdd2D"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 6117084133; Fri, 9 Sep 2022 17:17:22 +0200 (CEST) Received: from mx07-00178001.pphosted.com (mx07-00178001.pphosted.com [185.132.182.106]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 7877984928 for ; Fri, 9 Sep 2022 17:17:19 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=foss.st.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=prvs=525199375b=alain.volmat@foss.st.com Received: from pps.filterd (m0288072.ppops.net [127.0.0.1]) by mx07-00178001.pphosted.com (8.17.1.5/8.17.1.5) with ESMTP id 2899xrPB025382; Fri, 9 Sep 2022 17:17:18 +0200 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=foss.st.com; h=date : from : to : cc : subject : message-id : references : mime-version : content-type : content-transfer-encoding : in-reply-to; s=selector1; bh=AG2GSQbg87wJmNT7EnQCVt9jOUOgaLJ6R8x2ghpMyCU=; b=xkNsdd2Dqdse/dPQqh1Ii1uiqDXmyJjb0fK7GR+0Axd+NdjTolNNQuqpmS1KUJJp2Nyq qsKCSSG2xUM+aFr30Fy6pMIFM7qiScGCYVcqp/7D4TUByJQh6PUwmm3BJdt+2n4pCwEm Dwm/mTqCkufQgG6eQ9JbPQ+cqt6A0OZVzE06/MWtheSHrL0rFY3SAmlWaIBVFLG3MLR6 q6KEKlzqcAR1nOiMsP0Oe2r1x5amfd+Qx8u0PV8L1kdvM30adxZti/TPF1y/dBBdWwkj 18FJlaVJLa02QR//3ypZZ0d3A9yBZym9HIfM13JWVFUhI1LyzLYpW1i2DMM84J1foevb OA== Received: from beta.dmz-eu.st.com (beta.dmz-eu.st.com [164.129.1.35]) by mx07-00178001.pphosted.com (PPS) with ESMTPS id 3jg3bnsr5w-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 09 Sep 2022 17:17:18 +0200 Received: from euls16034.sgp.st.com (euls16034.sgp.st.com [10.75.44.20]) by beta.dmz-eu.st.com (STMicroelectronics) with ESMTP id 6B41310002A; Fri, 9 Sep 2022 17:17:16 +0200 (CEST) Received: from Webmail-eu.st.com (shfdag1node1.st.com [10.75.129.69]) by euls16034.sgp.st.com (STMicroelectronics) with ESMTP id 5F05423693D; Fri, 9 Sep 2022 17:17:16 +0200 (CEST) Received: from gnbcxd0016.gnb.st.com (10.75.127.123) by SHFDAG1NODE1.st.com (10.75.129.69) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256) id 15.1.2375.7; Fri, 9 Sep 2022 17:17:16 +0200 Date: Fri, 9 Sep 2022 17:17:11 +0200 From: Alain Volmat To: Patrick DELAUNAY CC: , , , , , Subject: Re: [PATCH v2 3/3] i2c: stm32: only send a STOP upon transfer completion Message-ID: <20220909151711.GA1792417@gnbcxd0016.gnb.st.com> References: <20220908105934.1764482-1-alain.volmat@foss.st.com> <20220908105934.1764482-4-alain.volmat@foss.st.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Disclaimer: ce message est personnel / this message is private X-Originating-IP: [10.75.127.123] X-ClientProxiedBy: GPXDAG2NODE4.st.com (10.75.127.68) To SHFDAG1NODE1.st.com (10.75.129.69) X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.528,FMLib:17.11.122.1 definitions=2022-09-09_08,2022-09-09_01,2022-06-22_01 X-Mailman-Approved-At: Fri, 09 Sep 2022 20:02:29 +0200 X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean Hi Patrick On Fri, Sep 09, 2022 at 02:53:23PM +0200, Patrick DELAUNAY wrote: > Hi Alain > > On 9/8/22 12:59, Alain Volmat wrote: > > Current function stm32_i2c_message_xfer is sending a STOP > > whatever the result of the transaction is. This can cause issues > > such as making the bus busy since the controller itself is already > > sending automatically a STOP when a NACK is generated. This can > > be especially seen when the processing get slower (ex: enabling lots > > of debug messages), ending up send 2 STOP (one automatically by the > > controller and a 2nd one at the end of the stm32_i2c_message_xfer > > function). > > > > Thanks to Jorge Ramirez-Ortiz for diagnosing and proposing a first > > fix for this. [1] > > > > [1] https://lore.kernel.org/u-boot/20220815145211.31342-2-jorge@foundries.io/ > > > > Reported-by: Jorge Ramirez-Ortiz, Foundries > > Signed-off-by: Jorge Ramirez-Ortiz > > Signed-off-by: Alain Volmat > > --- > > drivers/i2c/stm32f7_i2c.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/i2c/stm32f7_i2c.c b/drivers/i2c/stm32f7_i2c.c > > index 0ec67b5c12..8803979d3e 100644 > > --- a/drivers/i2c/stm32f7_i2c.c > > +++ b/drivers/i2c/stm32f7_i2c.c > > @@ -477,16 +477,16 @@ static int stm32_i2c_message_xfer(struct stm32_i2c_priv *i2c_priv, > > if (ret) > > break; > > + /* End of transfer, send stop condition */ > > + mask = STM32_I2C_CR2_STOP; > > + setbits_le32(®s->cr2, mask); > > + > > if (!stop) > > /* Message sent, new message has to be sent */ > > return 0; > > } > > } > > - /* End of transfer, send stop condition */ > > - mask = STM32_I2C_CR2_STOP; > > - setbits_le32(®s->cr2, mask); > > - > > return stm32_i2c_check_end_of_message(i2c_priv); > > } > > > Boot on DK2 failed with the traces: Ouch, I am very sorry about that. I think I might have made a mistake during testing / removing debug traces, leading to this mistake ;-( Very sorry about that, thanks a lot Patrick for the test. > > > U-Boot 2022.10-rc4-00043-g5b118161055 (Sep 09 2022 - 14:19:12 +0200) > > CPU: STM32MP157CAC Rev.B > Model: STMicroelectronics STM32MP157C-DK2 Discovery Board > Board: stm32mp1 in trusted mode (st,stm32mp157c-dk2) > Board: MB1272 Var2.0 Rev.C-01 > DRAM:  512 MiB > Clocks: > - MPU : 650 MHz > - MCU : 208.878 MHz > - AXI : 266.500 MHz > - PER : 24 MHz > - DDR : 533 MHz > stpmic1_pmic stpmic@33: stpmic1_read: failed to read register 0x25 : > -16stpmic1_pmic stpmic@33: stpmic1_write: failed to write register 0x25 > :-16stpmic1_pmic stpmic@33: stpmic1_write: failed to write register 0x25 > :-16stpmic1_pmic stpmic@33: stpmic1_write: failed to write register 0x2a > :-16stpmic1_pmic stpmic@33: stpmic1_write: failed to write register 0x26 > :-16stpmic1_pmic stpmic@33: stpmic1_write: failed to write register 0x21 > :-16stpmic1_pmic stpmic@33: stpmic1_write: failed to write register 0x22 > :-16stpmic1_pmic stpmic@33: stpmic1_write: failed to write register 0x23 > :-16stpmic1_pmic stpmic@33: stpmic1_write: failed to write register 0x25 > :-16stpmic1_pmic stpmic@33: stpmic1_write: failed to write register 0x26 > :-16stpmic1_pmic stpmic@33: stpmic1_write: failed to write register 0x29 > :-16stpmic1_pmic stpmic@33: stpmic1_write: failed to write regi�Core:  275 > devices, 40 uclasses, devicetree: board > WDT:   Started watchdog@5a002000 with servicing (32s timeout) > NAND:  0 MiB > MMC:   STM32 SD/MMC: 0 > Loading Environment from MMC... OK > In:    serial > Out:   serial > Err:   serial > Net:   eth0: ethernet@5800a000 > Hit any key to stop autoboot:  0 > > > I think the code should be inserted AFTER the test "if (!stop)" > > I modify the patch with > > -------------------------- drivers/i2c/stm32f7_i2c.c > -------------------------- index aac592860e1..cd3bcdf8d99 100644 @@ -477,13 > +477,12 @@ static int stm32_i2c_message_xfer(struct stm32_i2c_priv > *i2c_priv, if (ret) break; -/* End of transfer, send stop condition */ -mask > = STM32_I2C_CR2_STOP; -setbits_le32(®s->cr2, mask); - if (!stop) /* > Message sent, new message has to be sent */ return 0; + +/* End of transfer, > send stop condition */ +setbits_le32(®s->cr2, STM32_I2C_CR2_STOP); } } > > > And the boot is OK, I2C read/tested is OK > > test with the 2 available device on the board = STPMIC1 & STUSB1600 > > STM32MP> i2c bus > Bus 4:    i2c@40012000 > Bus 3:    i2c@5c002000  (active 3) >    28: stusb1600@28, offset len 1, flags 0 >    33: stpmic@33, offset len 1, flags 0 > > STM32MP> pmic dev stpmic@33 > > STM32MP> pmic dump > Dump pmic: stpmic@33 registers > > 0x00: 00 10 00 00 00 01 10 00 00 00 00 00 00 00 00 00 > 0x10: 04 00 00 00 00 00 80 00 00 00 00 00 00 00 00 00 > 0x20: 61 79 d9 d9 01 25 61 7d 00 51 0d 00 00 00 00 00 > 0x30: 61 50 d9 d9 00 24 24 24 01 51 04 00 00 00 00 00 > 0x40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0x50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0x60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0x70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0x80: ff ff ff cf 00 00 00 00 00 00 00 00 00 00 00 00 > 0x90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0xa0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0xb0: 00 00 00 00 00 00 00 00 08 00 00 00 00 00 01 02 > 0xc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0xd0: 00 00 00 00 00 00 00 00 00 00 0c 00 00 00 00 00 > 0xe0: aa 55 01 19 f0 81 84 00 31 03 0b 00 00 00 01 08 > 0xf0: 40 10 00 00 20 52 fd 0e ee 92 c0 02 f2 80 02 33 > > STM32MP> regulator status -a > Name                 Enabled            uV         mA Mode > reg11                disabled      1100000          - - > reg18                disabled      1800000          - - > usb33                disabled      3300000          - - > vrefbuf@50025000     enabled       2500000          - - > vddcore              enabled       1200000          - HP > vdd_ddr              enabled       1350000          - HP > vdd                  enabled       3300000          - HP > v3v3                 enabled       3300000          - HP > v1v8_audio           enabled       1800000          - - > v3v3_hdmi            enabled       3300000          - - > vtt_ddr              enabled        675000          - SINK SOURCE > vdd_usb              disabled      3300000          - - > vdda                 enabled       2900000          - - > v1v2_hdmi            enabled       1200000          - - > vref_ddr             enabled        675000          - - > bst_out              disabled            -          - - > vbus_otg             disabled            -          - - > vbus_sw              disabled            -          - - > vin                  enabled       5000000          - > > > I2C write is OK: tested with : > > STM32MP> regulator dev vbus_otg > dev: vbus_otg @ pwr_sw1 > STM32MP> regulator enable > > + USB cable deconnection detection in 'ums command' > > > So I think you need to modify the patch In fact, moving the set STOP at this place right after the TC flag has an issue leading to breaking the i2c probe. As I wrote originaly, we have to take into consideration the first NACK check in the while loop, so finally, the best solution seems to me as making the set STOP conditionnal right at the end of the function (actually as Jorge patch was doing in his patch) but also checking for the NACK or ERROR as well. Basically, as below: + /* End of transfer, send stop condition if appropriate */ + if (!ret && !(status & (STM32_I2C_ISR_NACKF | STM32_I2C_ISR_ERRORS))) + setbits_le32(®s->cr2, STM32_I2C_CR2_STOP); + + return stm32_i2c_check_end_of_message(i2c_priv); Sorry for all the noise with this problem. I tested it again and with that I don't see issues after a NACK and also the probe is still behaving correctly. Let me update the series with a v3. Regards, Alain > > regards > > > Patrick >