Re: [PATCH] i2c: imx: increase retries on arbitration loss

[Oleksij Rempel]

On Fri, Dec 30, 2022 at 05:47:21PM +0100, Francesco Dolcini wrote:
> On Fri, Dec 30, 2022 at 05:12:09PM +0100, Oleksij Rempel wrote:
> > On Fri, Dec 30, 2022 at 03:40:58PM +0100, Francesco Dolcini wrote:
> > > +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,
> > 
> > Well, this is persistent confusion in this monolog. It will not make it
> > correctly.
> > 
> > > and at a
> > > minimum setting the retry count from user space is not going to solve
> > > potential issues during initial driver probe.
> > 
> > I assume it is not clear from programmer point of view. Lets try other way:
> > 
> > - The I2C slave could not correctly interpret the data on SDA because the SDA
> >   high or low-level voltages do not reach its appropriate input
> >   thresholds.
> > 
> > This means:
> > 
> > You have this:
> > 
> >     /-\    /-\ ----- 2.5Vcc
> > ___/   \__/   \___
> > 
> > Instead of this:
> > 
> >      /-\     /-\ ----- 3.3Vcc
> >     /  \    /   \
> > ___/    \__/     \___
> > 
> > This is bad, because master or slave will not be able to interpret the pick level
> > correctly. It may see some times 0 instead of 1. This means, what ever we are
> > writing we are to the slave or reading from the slave is potentially corrupt
> > and only __sometimes__ the master was able to detect it. 
> > 
> > - The I2C slave missed an SCL cycle because the SCL high or low-level voltages
> >   do not reach its appropriate input thresholds.
> > 
> > This means, the bus frequency is too high for current configured or physical PCB
> > designed. So, you will have different kind of corruptions and some times they
> > will be detected. 
> > 
> > - The I2C slave accidently interpreted a spike etc. as an SCL cycle.
> > 
> > This means the noise level is to high. The driver strange should be increased
> > or PCB redesign should be made. May be there are more options. If not done,
> > data corruption can be expected.
> > 
> > None of this issue can be "fixed" by retries or made more "robust".
> > Doing more retries means: we do what ever we do until the system was not able to
> > detect the error.
> 
> Hello Oleksij,
> thanks for the detailed explanation, appreciated.
> 
> Given that is it correct that the i2c imx driver return EAGAIN in such a
> case (arbitration error)? You made it crystal clear that there is no
> such thing as try again for this error, I would be inclined to prepare a
> patch to fix this.
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index cf5bacf3a488..a2a581c8ae07 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -492,7 +492,7 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy, bool a
>                 /* check for arbitration lost */
>                 if (temp & I2SR_IAL) {
>                         i2c_imx_clear_irq(i2c_imx, I2SR_IAL);
> -                       return -EAGAIN;
> +                       return -EIO;
>                 }
> 
>                 if (for_busy && (temp & I2SR_IBB)) {
> 

Hm, good question.

> In addition to that is there any valid use case of the i2c retry
> mechanism?
> Is possible for an I2C controller to report anything that can
> be recovered with a retry?

In case of multimaster bus, except of noise and signal level issues, we
would have a simple conflict between masters. In this case, we should
retry. Potentially, every master should use randomized pause before
retrying (at last it is done by some other protocol using shared
medium).

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |