LKML Archive on
help / color / mirror / Atom feed
From: Wolfram Sang <>
To: Eddie James <>
Subject: Re: [PATCH v10 4/7] i2c: fsi: Add abort and hardware reset procedures
Date: Mon, 2 Jul 2018 20:15:11 +0200	[thread overview]
Message-ID: <20180702181511.mv2fuiwjjdhyp43v@ninjato> (raw)
In-Reply-To: <>

[-- Attachment #1: Type: text/plain, Size: 3605 bytes --]

Hi Eddie,

> > I think this is a way too aggressive recovery. Your are doing the 9
> > pulse toggles basically on any error while this is only when the device
> > keeps SDA low and you want to recover from that. If SDA is not stuck
> > low, sending a STOP should do. Or do you have a known case where this is
> > not going to work?
> It is aggressive, but I don't see the harm in doing this on every error.

Well, as it happens, I just fixed such a case. Please check these patch
series and elinux wiki pages:


(new fault injector)
[PATCH v2 0/2] i2c: gpio: fault-injector: add new injector

(actual recovery fix)
[PATCH 0/2] i2c: recovery: make sure pulses are not misinterpreted


And here is the new elinux wiki page to describe my findings:

Also, the previous pages have been updated to reflect the latest status:

To sum it up: This is a proven case where uncontrolled bus recovery can
result into a bogus write!

> There are some other error conditions with this hardware which may require
> the clock toggling, such as "bus arbitration lost." I think this is the

Why is that? In my understanding, recovery is *only* needed when SDA is
stuck low. If SDA is high, sending STOP should do. If not, it needs to
be researched why.

> safest option for this hardware, and this routine has been tested for many
> years.

I remember having a similar argument with Joakim Tjernlund a while ago.
I recently re-read our argument, yet I still keep my position: I don't
want to do $random things to recover, just a tested and well understood
procedure. And in that thread, I was never given a test case.

> > 
> > Also, you implement the pulse toggling manually. Can't you just populate
> > {get|set}_{scl|sda} and use the generic routine we have in the core?
> I see that the generic implementation breaks the loop if it sees the clock
> isn't high after setting it, or if SDA goes high. I think it's safer to
> finish the reset for our hardware. Plus, we actually have different

Why do you think it is safer? What is the test case for that? I think
one really should do check SDA! See above, you might trigger a write
otherwise. If this breaks something for you, I am looking forward to
discuss it.

> registers for setting 0 or 1 to the clock/data, so we save some cpu cycles
> by doing it directly instead of implementing set_scl/sda and having to check
> val every time :)

Correctness comes above all here. And I am afraid your implementation is
not correct.

> If you feel very strongly that this recovery procedure needs to be reduced,
> then I will work on that and have to do some extensive testing.

I am open for discussion, yet I also feel strong about it. The reason
why the recovery procedure is moved into the core is to have one working
and understood bit-banging algorithm which all drivers can rely on. If
all drivers implement their custom version, they might miss gory details
like the above write_byte fix.

I do understand this might cause testing effort for you, I am sorry for
the delay it causes. However, my goal as a maintainer is to have a
reliable recovery mechanism, for your driver as well as for all drivers.

I hope this is understandable. BTW if you want this driver upstream
soon, then it may be an idea to resend it without any bus recovery and
then we can work on it incrementally.

Kind regards and thanks,


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

  reply	other threads:[~2018-07-02 18:15 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-13 19:36 [PATCH v10 0/7] i2c: Add FSI-attached I2C master algorithm Eddie James
2018-06-13 19:36 ` [PATCH v10 1/7] dt-bindings: i2c: Add FSI-attached I2C master dt binding documentation Eddie James
2018-06-26  2:39   ` Wolfram Sang
2018-06-13 19:36 ` [PATCH v10 2/7] i2c: Add FSI-attached I2C master algorithm Eddie James
2018-06-13 19:36 ` [PATCH v10 3/7] i2c: fsi: Add port structures Eddie James
2018-06-20  3:34   ` Benjamin Herrenschmidt
2018-06-20  3:59     ` Joel Stanley
2018-06-13 19:36 ` [PATCH v10 4/7] i2c: fsi: Add abort and hardware reset procedures Eddie James
2018-06-26  2:38   ` Wolfram Sang
2018-06-27 13:48     ` Eddie James
2018-07-02 18:15       ` Wolfram Sang [this message]
2018-07-05 18:50         ` Eddie James
2018-07-05 22:06           ` Wolfram Sang
2018-06-13 19:36 ` [PATCH v10 5/7] i2c: fsi: Add transfer implementation Eddie James
2018-06-26  2:38   ` Wolfram Sang
2018-06-27 13:21     ` Eddie James
2018-07-02 18:24       ` Wolfram Sang
2018-07-05 18:52         ` Eddie James
2018-07-05 21:59           ` Wolfram Sang
2018-06-13 19:36 ` [PATCH v10 6/7] i2c: fsi: Add I2C master locking Eddie James
2018-06-13 19:36 ` [PATCH v10 7/7] i2c: fsi: Add bus recovery Eddie James
2018-06-26  2:38   ` Wolfram Sang
2018-06-27 13:32     ` Eddie James
2018-07-02 18:16       ` Wolfram Sang
2018-06-14  9:05 ` [PATCH v10 0/7] i2c: Add FSI-attached I2C master algorithm Andy Shevchenko
2018-06-18  4:53 ` Joel Stanley
2018-06-26  2:39 ` Wolfram Sang
2018-06-27 13:53   ` Eddie James

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180702181511.mv2fuiwjjdhyp43v@ninjato \ \ \ \ \ \ \ \ \ \ \ \ \ \
    --subject='Re: [PATCH v10 4/7] i2c: fsi: Add abort and hardware reset procedures' \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).