Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 30 Dec 2022 15:40:58 +0100
From:   Francesco Dolcini <francesco@dolcini.it>
To:     Primoz Fiser <primoz.fiser@norik.com>
Cc:     Francesco Dolcini <francesco@dolcini.it>,
        Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= 
        <u.kleine-koenig@pengutronix.de>,
        Oleksij Rempel <o.rempel@pengutronix.de>,
        linux-kernel@vger.kernel.org, Shawn Guo <shawnguo@kernel.org>,
        Sascha Hauer <s.hauer@pengutronix.de>,
        upstream@lists.phytec.de, Marco Felsch <m.felsch@pengutronix.de>,
        Oleksij Rempel <linux@rempel-privat.de>,
        NXP Linux Team <linux-imx@nxp.com>,
        Pengutronix Kernel Team <kernel@pengutronix.de>,
        Fabio Estevam <festevam@gmail.com>,
        linux-arm-kernel@lists.infradead.org, linux-i2c@vger.kernel.org,
        francesco.dolcini@toradex.com, wsa@kernel.org
Subject: Re: [PATCH] i2c: imx: increase retries on arbitration loss
Message-ID: <Y674eoNsHtAeG7IP@francesco-nb.int.toradex.com>
References: <20221216084511.2576786-1-primoz.fiser@norik.com>
 <20221216094518.bevkg5buzu7iybfh@pengutronix.de>
 <bb4882a9-8be6-5255-6256-aa1253362e59@norik.com>
 <20221216110227.GA12327@pengutronix.de>
 <20221216111308.wckibotr5d3q6ree@pengutronix.de>
 <5c2e0531-e7c3-1b37-35ed-c8e9795a0d18@norik.com>
 <Y5xpt6J01Boec6Xr@francesco-nb.int.toradex.com>
 <41991ce2-3e88-5afc-6def-6e718d624768@norik.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <41991ce2-3e88-5afc-6def-6e718d624768@norik.com>
Precedence: bulk

+Wolfram

On Wed, Dec 28, 2022 at 09:01:46AM +0100, Primoz Fiser wrote:
> On 16. 12. 22 13:51, Francesco Dolcini wrote:
> > On Fri, Dec 16, 2022 at 01:23:29PM +0100, Primoz Fiser wrote:
> > > The only solid point in the thread seems to be that in that case we are not
> > > covering up the potential i2c hardware issues?
> > 
> > I believe that in this case we should just have a warning in the kernel.
> > The retry potentially work-around a transient issue and we do not hide any hardware
> > issue at the same time. It seems an easy win-win solution.
> 
> I would agree about throwing a warning message in retry case.
> 
> Not sure how would it affect other i2c bus drivers using retries > 0.
> Retries might be pretty rare with i2c-imx but some other drivers set this to
> 5 for example. At least using _ratelimited printk is a must using this
> approach.

Wolfram, Uwe, Oleksij

Would it be acceptable to have a warning when we have I2C retries, and
with that in place enabling retries on the imx driver?

It exists hardware that requires this to work correctly, and at a
minimum setting the retry count from user space is not going to solve
potential issues during initial driver probe.

To me the only reasonable solution is to have the retry enabled with a
sensible number (3? 5?), however there is a concern that this might
hide real hardware issues.

> > > Yeah fair point but on the other hand, goal of this patch would be to
> > > improve robustness in case of otherwise good performing hardware. From user
> > > perspective I just want it to work despite it retrying under the hood from
> > > time to time. I think Francesco had the same idea.
> > 
> > Unfortunately I was missing the exact background that made us do this
> > change, we just had it sitting in our fork for too long :-/
> > This is one of the reason I gave up on it.
> > 
> > Quoting Uwe [1]
> > > sometimes there is no practical way around such work arounds. I happens
> > > from time to time that the reason for problem is known, but fixing the
> > > hardware is no option, then you need such workrounds. (This applies to
> > > both, retrying the transfers and resetting the bus.)
> 
> I wouldn't say this is exactly a workaround if "retries mechanism" is
> standard part of the i2c core. Other drivers use it as well. it is just a
> setting to improve bus robustness.
> 
> But OK, I guess we can live with this one-liner in the downstream kernel.
To me this is not a good idea.

Francesco