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 X-Spam-Level: X-Spam-Status: No, score=-3.8 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 14A4FC4363D for ; Fri, 2 Oct 2020 08:03:53 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id ABC05206B7 for ; Fri, 2 Oct 2020 08:03:52 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="cFd1jKd0" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org ABC05206B7 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arri.de Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To:Message-ID:Date: Subject:To:From:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=5cd2OiR4x6hCd99lKcFTlupyuQiw/tx2AMdxbpFKSNE=; b=cFd1jKd0nuWJRBWvahAgxGndC U7oIkvLzGzHlrRRkwqVHxfoS/GbO0Lqv8mrmRDMVD+5RgOr3YnfMDTx4yjMb+U9Z3uNS6mjkrM/GN MHwWGKs1sgNqU2CldyaVFgjaj/LL89s16TNX4H2qKPhkDZ+Erlk5dQU2kCim7H48TlD2iSXWVWfa/ 6Pe7/srGtJ5M6vdbK0F8ne7ajSsMp4sB8Zx4CmvwrVQCVv45qn6IOhsibOcqbnslNCSf87zqwiCuu I+VezVapqGpgeeO/MkHafkZc7lGARw+wkfZaVQvdiqeT+z1Ryb1hnu7eaVGXQ0RxshiV0qUJUMOMG wBzKepYKg==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kOG1R-0003Lq-CS; Fri, 02 Oct 2020 08:02:33 +0000 Received: from mailout04.rmx.de ([94.199.90.94]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kOG1N-0003Jd-RM for linux-arm-kernel@lists.infradead.org; Fri, 02 Oct 2020 08:02:31 +0000 Received: from kdin02.retarus.com (kdin02.dmz1.retloc [172.19.17.49]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mailout04.rmx.de (Postfix) with ESMTPS id 4C2jBv40lMz3qvQv; Fri, 2 Oct 2020 10:02:15 +0200 (CEST) Received: from mta.arri.de (unknown [217.111.95.66]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by kdin02.retarus.com (Postfix) with ESMTPS id 4C2jBV62Vyz2TRjV; Fri, 2 Oct 2020 10:01:54 +0200 (CEST) Received: from n95hx1g2.localnet (192.168.54.43) by mta.arri.de (192.168.100.104) with Microsoft SMTP Server (TLS) id 14.3.408.0; Fri, 2 Oct 2020 10:01:31 +0200 From: Christian Eggers To: Uwe =?ISO-8859-1?Q?Kleine=2DK=F6nig?= Subject: Re: [PATCH 1/3] i2c: imx: Fix reset of I2SR_IAL flag Date: Fri, 2 Oct 2020 10:01:30 +0200 Message-ID: <3615207.ozermb6hxN@n95hx1g2> Organization: Arnold & Richter Cine Technik GmbH & Co. Betriebs KG In-Reply-To: <20200925081101.dthsj4hokqz2vsgp@pengutronix.de> References: <20200917122029.11121-1-ceggers@arri.de> <16013235.tl8pWZfNaG@n95hx1g2> <20200925081101.dthsj4hokqz2vsgp@pengutronix.de> MIME-Version: 1.0 X-Originating-IP: [192.168.54.43] X-RMX-ID: 20201002-100156-4C2jBV62Vyz2TRjV-0@kdin02 X-RMX-SOURCE: 217.111.95.66 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201002_040230_093143_8ABCF55D X-CRM114-Status: GOOD ( 30.73 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Fabio Estevam , Sascha Hauer , linux-kernel@vger.kernel.org, stable@vger.kernel.org, Oleksij Rempel , NXP Linux Team , Pengutronix Kernel Team , Shawn Guo , linux-arm-kernel@lists.infradead.org, linux-i2c@vger.kernel.org Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Uwe, On Friday, 25 September 2020, 10:11:01 CEST, Uwe Kleine-K=F6nig wrote: > On Thu, Sep 17, 2020 at 04:13:50PM +0200, Christian Eggers wrote: > > On Thursday, 17 September 2020, 16:02:35 CEST, Uwe Kleine-K=F6nig wrote: > > > On Thu, Sep 17, 2020 at 02:20:27PM +0200, Christian Eggers wrote: > > > ... > > > = > > > > /* check for arbitration lost */ > > > > if (temp & I2SR_IAL) { > > > > = > > > > temp &=3D ~I2SR_IAL; > > > > = > > > > + temp |=3D (i2c_imx->hwdata->i2sr_clr_opcode & I= 2SR_IAL); > > > > = > > > > imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2SR); > > > > return -EAGAIN; > > > = > > > ... > > > = > > > This looks strange. First the flag is cleared and then it is (in some > > > cases) set again. > > = > > i.MX controllers require writing a 0 to clear these bits. Vybrid > > controllers need writing a 1 for the same. > = > Yes, I understood that. > = > > > If I2SR_IIF is set in temp you ack this irq without handling it. (Whi= ch > > > might happen if atomic is set and irqs are off?!) > > = > > This patch is only about using the correct processor specific value for > > acknowledging an IRQ... But I think that returning EAGAIN (which aborts > > the > > transfer) should be handling enough. At the next transfer, the controll= er > > will be set back to master mode. > = > Either you didn't understand what I meant, or I don't understand why you > consider your patch right anyhow. = Probably both. > So I try to explain in other and more words. > = > IMHO the intention here (and also what happens on i.MX) is that exactly > the AL interrupt pending bit should be cleared and the IF irq is > supposed to be untouched. Yes. > Given there are only two irq flags in the I2SR register (which is called > IBSR on Vybrid) ... Vybrid: =3D=3D=3D=3D=3D=3D=3D +-------+-----+------+-----+------+-----+-----+------+------+ | BIT | 7 | 6 | 5 | 4 | 3 | 2 | 1 | 0 | +-------+-----+------+-----+------+-----+-----+------+------+ | READ | TCF | IAAS | IBB | IBAL | 0 | SRW | IBIF | RXAK | +-------+-----+------+-----+------+-----+-----+------+------+ | WRITE | - | - | - | W1C | - | - | W1C | - | +-------+-----+------+-----+-^^^--+-----+-----+-^^^--+------+ i.MX: =3D=3D=3D=3D=3D=3D=3D +-------+-----+------+-----+------+-----+-----+------+------+ | BIT | 7 | 6 | 5 | 4 | 3 | 2 | 1 | 0 | +-------+-----+------+-----+------+-----+-----+------+------+ | READ | ICF | IAAS | IBB | IAL | 0 | SRW | IIF | RXAK | +-------+-----+------+-----+------+-----+-----+------+------+ | WRITE | - | - | - | W0C | - | - | W0C | - | +-------+-----+------+-----+-^^^--+-----+-----+-^^^--+------+ > ... the status quo (i.e. without your patch) is: > = > On i.MX IAL is cleared Yes > On Vybrid IIF (which is called IBIF there) is cleared. If IBIF is set, then it's cleared (probably by accident). But in the "if (temp & I2SR_IAL)" condition, I focus on = the IBAL flag, not IBIF. > With your patch we get: > = > On i.MX IAL is cleared Yes > On Vybrid both IIF (aka IBIF) and IAL (aka IBAL) are cleared. Agree. IBAL is cleared by intention, IBIF by accident (if set). Do you see any problem if IBIF is also cleared? > To get it right for both SoC types you have to do (e.g.): > = > temp =3D ~i2c_imx->hwdata->i2sr_clr_opcode ^ I2SR_IAL; Sorry, even if this is correct, it looks hard to understand for me. > (and in i2c_imx_isr() the same using I2SR_IIF instead of I2SR_IAL > because there currently IAL might be cleared by mistake on Vybrid). > = > I considered creating a patch, but as I don't have a Vybrid on my desk > and on i.MX there is no change, I let you do this. I also don't own a Vybrid system. I consider my patch correct in terms of clearing the IBAL flag (which was wrong before). Additional work may be useful for NOT clearing the other status flag. I also would like to keep this task for somebody who owns a Vybrid system. But the other patches in this series fixes some more important problems, so maybe you could give your acknowledge anyhow. Best regards Christian _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel