All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Grygorii Strashko <grygorii.strashko@ti.com>
Cc: Wolfram Sang <wsa@the-dreams.de>,
	linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org,
	Sekhar Nori <nsekhar@ti.com>,
	Kevin Hilman <khilman@deeprootsystems.com>,
	Santosh Shilimkar <ssantosh@kernel.org>,
	Murali Karicheri <m-karicheri2@ti.com>
Subject: Re: [4/5] i2c: davinci: use bus recovery infrastructure
Date: Mon, 24 Nov 2014 21:07:54 +0100	[thread overview]
Message-ID: <20141124200754.GW4431@pengutronix.de> (raw)
In-Reply-To: <547331F2.3000006@ti.com>

Hi Grygorii,

On Mon, Nov 24, 2014 at 03:26:10PM +0200, Grygorii Strashko wrote:
> On 11/23/2014 10:36 PM, Uwe Kleine-König wrote:
> > On Fri, Nov 21, 2014 at 09:33:22PM +0200, Grygorii Strashko wrote:
> >> On 11/21/2014 09:07 PM, Uwe Kleine-König wrote:
> >>> On Thu, Nov 20, 2014 at 12:03:07PM +0200, Grygorii Strashko wrote:
> >>> Just another general comment about the driver that doesn't influence the
> >>> correctness of this patch: The i2c-davinci driver is quite quick to
> >>> reset the bus. I wonder how often this reset triggers. Is the bus in
> >>> question less "stable" than others?
> >>
> >> In comparison to ..? :)
> > In comparison to other bus drivers in other SoCs. I know this might be
> > hard to answer. I just wonder where the reason for this has to be
> > located. Strange hardware? Software bug? Or is this SoC just operating
> > with strange slaves more often than others?
> 
> Davinci driver does reset in two cases:
> - when I2C transaction isn't completed due to timeout (no irq received)
> - when BB is detected
> both cases are reasonable, because in 1st case HW state is undefined
> in 2d case - Davinci I2C supports only master mode and if BB detected
> we need perform some recovery procedure.
note that for multi-master scenarios bus recovery might be bad. So that
has not necessarily something to do with "master mode only".

> Also, this patch doesn't introduce functional changes - it's just code
> reworking intended to reuse I2C bus recovery infrastructure
Yeah this is fine, and my concern shouldn't stop your patch from getting
in as it is an improvement for sure. Still I only had to handle BB once
in my linux lifetime, and that was for a different reason. (Machine
doing massive i2c transfers and so a client was often holding the bus
busy when the watchdog resetted the cpu.) So I just wondered how often
the need for recovery arises.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

WARNING: multiple messages have this Message-ID (diff)
From: "Uwe Kleine-König" <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
To: Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org>
Cc: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org>,
	Kevin Hilman
	<khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>,
	Santosh Shilimkar
	<ssantosh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>,
	Murali Karicheri <m-karicheri2-l0cyMroinI0@public.gmane.org>
Subject: Re: [4/5] i2c: davinci: use bus recovery infrastructure
Date: Mon, 24 Nov 2014 21:07:54 +0100	[thread overview]
Message-ID: <20141124200754.GW4431@pengutronix.de> (raw)
In-Reply-To: <547331F2.3000006-l0cyMroinI0@public.gmane.org>

Hi Grygorii,

On Mon, Nov 24, 2014 at 03:26:10PM +0200, Grygorii Strashko wrote:
> On 11/23/2014 10:36 PM, Uwe Kleine-König wrote:
> > On Fri, Nov 21, 2014 at 09:33:22PM +0200, Grygorii Strashko wrote:
> >> On 11/21/2014 09:07 PM, Uwe Kleine-König wrote:
> >>> On Thu, Nov 20, 2014 at 12:03:07PM +0200, Grygorii Strashko wrote:
> >>> Just another general comment about the driver that doesn't influence the
> >>> correctness of this patch: The i2c-davinci driver is quite quick to
> >>> reset the bus. I wonder how often this reset triggers. Is the bus in
> >>> question less "stable" than others?
> >>
> >> In comparison to ..? :)
> > In comparison to other bus drivers in other SoCs. I know this might be
> > hard to answer. I just wonder where the reason for this has to be
> > located. Strange hardware? Software bug? Or is this SoC just operating
> > with strange slaves more often than others?
> 
> Davinci driver does reset in two cases:
> - when I2C transaction isn't completed due to timeout (no irq received)
> - when BB is detected
> both cases are reasonable, because in 1st case HW state is undefined
> in 2d case - Davinci I2C supports only master mode and if BB detected
> we need perform some recovery procedure.
note that for multi-master scenarios bus recovery might be bad. So that
has not necessarily something to do with "master mode only".

> Also, this patch doesn't introduce functional changes - it's just code
> reworking intended to reuse I2C bus recovery infrastructure
Yeah this is fine, and my concern shouldn't stop your patch from getting
in as it is an improvement for sure. Still I only had to handle BB once
in my linux lifetime, and that was for a different reason. (Machine
doing massive i2c transfers and so a client was often holding the bus
busy when the watchdog resetted the cpu.) So I just wondered how often
the need for recovery arises.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

  reply	other threads:[~2014-11-24 20:07 UTC|newest]

Thread overview: 56+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-11-20 10:03 [PATCH 0/5] i2c: davinci improvements and fixes Grygorii Strashko
2014-11-20 10:03 ` Grygorii Strashko
2014-11-20 10:03 ` [PATCH 1/5] i2c: i2c-davinci: switch to use platform_get_irq Grygorii Strashko
2014-11-20 10:03   ` Grygorii Strashko
2014-11-20 21:48   ` [1/5] " Uwe Kleine-König
2014-11-21 11:01     ` Grygorii Strashko
2014-11-21 11:01       ` Grygorii Strashko
2014-11-21 14:03       ` Rob Herring
2014-11-21 14:03         ` Rob Herring
2014-11-21 14:59         ` Grygorii Strashko
2014-11-20 10:03 ` [PATCH 2/5] i2c: davinci: query STP always when NACK is received Grygorii Strashko
2014-11-20 10:03   ` Grygorii Strashko
2014-11-20 22:19   ` [2/5] " Uwe Kleine-König
2014-11-20 22:19     ` Uwe Kleine-König
2014-11-21 12:48     ` Grygorii Strashko
2014-11-21 12:48       ` Grygorii Strashko
2014-11-21 13:10       ` Uwe Kleine-König
2014-11-21 13:10         ` Uwe Kleine-König
2014-11-21 15:33         ` Grygorii Strashko
2014-11-21 15:33           ` Grygorii Strashko
2014-11-23 20:33           ` Uwe Kleine-König
2014-11-23 20:33             ` Uwe Kleine-König
2014-11-24 13:34             ` Grygorii Strashko
2014-11-24 13:34               ` Grygorii Strashko
2014-11-24 20:02               ` Uwe Kleine-König
2014-11-24 20:02                 ` Uwe Kleine-König
2014-11-20 10:03 ` [PATCH 3/5] i2c: recovery: change input parameter to i2c_adapter for prepare/unprepare_recovery Grygorii Strashko
2014-11-20 10:03   ` Grygorii Strashko
2014-11-21 18:49   ` [3/5] " Uwe Kleine-König
2014-11-21 18:49     ` Uwe Kleine-König
2014-11-20 10:03 ` [PATCH 4/5] i2c: davinci: use bus recovery infrastructure Grygorii Strashko
2014-11-20 10:03   ` Grygorii Strashko
2014-11-21 19:07   ` [4/5] " Uwe Kleine-König
2014-11-21 19:07     ` Uwe Kleine-König
2014-11-21 19:33     ` Grygorii Strashko
2014-11-21 19:33       ` Grygorii Strashko
2014-11-23 20:36       ` Uwe Kleine-König
2014-11-23 20:36         ` Uwe Kleine-König
2014-11-24 13:26         ` Grygorii Strashko
2014-11-24 13:26           ` Grygorii Strashko
2014-11-24 20:07           ` Uwe Kleine-König [this message]
2014-11-24 20:07             ` Uwe Kleine-König
2014-11-20 10:03 ` [PATCH 5/5] i2c: davinci: use ICPFUNC to toggle I2C as gpio for bus recovery Grygorii Strashko
2014-11-20 10:03   ` Grygorii Strashko
2014-11-23 17:04   ` [5/5] " Uwe Kleine-König
2014-11-23 17:04     ` Uwe Kleine-König
2014-11-24 13:15     ` Grygorii Strashko
2014-11-24 13:15       ` Grygorii Strashko
2014-11-24 18:13       ` Mike Looijmans
2014-11-24 18:13         ` Mike Looijmans
2014-11-24 19:22         ` Grygorii Strashko
2014-11-24 19:22           ` Grygorii Strashko
2014-11-24 19:45       ` Uwe Kleine-König
2014-11-24 19:45         ` Uwe Kleine-König
2014-11-25 13:04         ` Grygorii Strashko
2014-11-25 13:04           ` Grygorii Strashko

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:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

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

  git send-email \
    --in-reply-to=20141124200754.GW4431@pengutronix.de \
    --to=u.kleine-koenig@pengutronix.de \
    --cc=grygorii.strashko@ti.com \
    --cc=khilman@deeprootsystems.com \
    --cc=linux-i2c@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=m-karicheri2@ti.com \
    --cc=nsekhar@ti.com \
    --cc=ssantosh@kernel.org \
    --cc=wsa@the-dreams.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.