Re: [PATCH V9 1/2] i2c/adapter: Add bus recovery infrastructure
On 25 January 2013 14:45, Wolfram Sang wrote:
> On Thu, Jan 24, 2013 at 04:06:37PM +0530, Viresh Kumar wrote:
>> +static int i2c_generic_recovery(struct i2c_adapter *adap)
>> +{
>> + struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
>> + int i = 0, val = 1, ret = 0;
>> +
>> + if (bri->prepare_recovery)
>> + bri->prepare_recovery(bri);
>> +
>> + /*
>> + * By this time SCL is high, as we need to give 9 falling-rising edges
>> + */
>> + while (i++ < RECOVERY_CLK_CNT * 2) {
>> + /* SCL shouldn't be low here */
>> + if (val && !bri->get_scl(adap)) {
>> + dev_err(&adap->dev, "SCL is stuck Low exit
>> recovery\n");
>> + ret = -EBUSY;
>> + goto unprepare;
>> + }
>> +
>> + val = !val;
>> + bri->set_scl(adap, val);
>> + ndelay(clk_delay);
>> +
>> + /* break if sda got high, check only when scl line is high */
>> + if (bri->get_sda && val)
>> + if (bri->get_sda(adap))
>> + break;
>
> Didn't we agree to move the SDA check before the SCL setting?
Yes. There was a check for SCL (separately) before this loop. I got that into
this loop to save on redundant code. And now, after sending a complete clock,
we first check sda, then go to next iteration of loop and check scl.
Over that, this routine is a modified a bit again, due to Uwe's comment.
Wait for v10. You would get that in 2 mins :)
BTW, all other comments are accepted.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V9 1/2] i2c/adapter: Add bus recovery infrastructure
On Thu, Jan 24, 2013 at 04:06:37PM +0530, Viresh Kumar wrote:
> Add i2c bus recovery infrastructure to i2c adapters as specified in the i2c
> protocol Rev. 03 section 3.1.16 titled "Bus clear".
>
> http://www.nxp.com/documents/user_manual/UM10204.pdf
>
> Sometimes during operation i2c bus hangs and we need to give dummy clocks to
> slave device to start the transfer again. Now we may have capability in the
> bus
> controller to generate these clocks or platform may have gpio pins which can
> be
> toggled to generate dummy clocks. This patch supports both.
>
> This patch also adds in generic bus recovery routines gpio or scl line based
> which can be used by bus controller. In addition controller driver may provide
> its own version of the bus recovery routine.
>
> This doesn't support multi-master recovery for now.
>
> Reviewed-by: Paul Carpenter
> Signed-off-by: Viresh Kumar
> ---
> Most number of versions for any patchset i submitted :)
>
> V8->V9:
> - removed skip_sda_polling variable
> - simplified/fixed i2c_generic_recovery() routine
> - added new routine i2c_recover_bus()
> - added gpio_is_valid() checks over scl and sda gpios
> - code is rearranged at places (based on Wolfram's comments)
>
> drivers/i2c/i2c-core.c | 155
> +
> include/linux/i2c.h| 39 +
> 2 files changed, 194 insertions(+)
>
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index e388590..8076e52 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -27,7 +27,9 @@
>
> #include
> #include
> +#include
> #include
> +#include
> #include
> #include
> #include
> @@ -109,6 +111,127 @@ static int i2c_device_uevent(struct device *dev, struct
> kobj_uevent_env *env)
> #define i2c_device_ueventNULL
> #endif /* CONFIG_HOTPLUG */
>
> +/* i2c bus recovery routines */
> +#define RECOVERY_CLK_CNT 9
> +#define DEFAULT_CLK_RATE 100 /* KHz */
RECOVERY_CLK_RATE
> +/* 10^6/KHz for delay in ns */
> +unsigned long clk_delay = DIV_ROUND_UP(100, DEFAULT_CLK_RATE * 2);
Since all inputs are constant values, the result will be a constant as
well. We might even get rid of this variable altogether.
> +
> +static int get_scl_gpio_value(struct i2c_adapter *adap)
> +{
> + return gpio_get_value(adap->bus_recovery_info->scl_gpio);
> +}
> +
> +static void set_scl_gpio_value(struct i2c_adapter *adap, int val)
> +{
> + gpio_set_value(adap->bus_recovery_info->scl_gpio, val);
> +}
> +
> +static int get_sda_gpio_value(struct i2c_adapter *adap)
> +{
> + return gpio_get_value(adap->bus_recovery_info->sda_gpio);
> +}
> +
> +static int i2c_get_gpios_for_recovery(struct i2c_adapter *adap)
> +{
> + struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
> + struct device *dev = &adap->dev;
> + int ret = 0;
> +
> + ret = gpio_request_one(bri->scl_gpio, GPIOF_OPEN_DRAIN |
> + GPIOF_OUT_INIT_HIGH, "i2c-scl");
> + if (ret) {
> + dev_warn(dev, "gpio request fail: %d\n", bri->scl_gpio);
> + return ret;
> + }
> +
> + if (bri->get_sda) {
> + if (gpio_request_one(bri->sda_gpio, GPIOF_IN, "i2c-sda")) {
> + /* work without sda polling */
> + dev_warn(dev, "can't get sda: %d. Skip sda polling\n",
> + bri->sda_gpio);
> + bri->get_sda = NULL;
> + }
> + }
> +
> + return ret;
> +}
> +
> +static void i2c_put_gpios_for_recovery(struct i2c_adapter *adap)
> +{
> + struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
> +
> + if (bri->get_sda)
> + gpio_free(bri->sda_gpio);
> +
> + gpio_free(bri->scl_gpio);
> +}
> +
> +static int i2c_generic_recovery(struct i2c_adapter *adap)
> +{
> + struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
> + int i = 0, val = 1, ret = 0;
> +
> + if (bri->prepare_recovery)
> + bri->prepare_recovery(bri);
> +
> + /*
> + * By this time SCL is high, as we need to give 9 falling-rising edges
> + */
> + while (i++ < RECOVERY_CLK_CNT * 2) {
> + /* SCL shouldn't be low here */
> + if (val && !bri->get_scl(adap)) {
> + dev_err(&adap->dev, "SCL is stuck Low exit recovery\n");
s/Low/low/
> + ret = -EBUSY;
> + goto unprepare;
> + }
> +
> + val = !val;
> + bri->set_scl(adap, val);
> + ndelay(clk_delay);
> +
> + /* break if sda got high, check only when scl line is high */
> + if (bri->get_sda && val)
> + if (bri->get_sda(adap))
> + break;
if (val && bri->get_sda && bri->get_sda(adap)
break;
Didn't we agree to move the SDA check before the SCL setting?
> + }
> +
> +unprepare:
> + if (bri->unprepare_recovery)
Re: [PATCH V9 1/2] i2c/adapter: Add bus recovery infrastructure
On 24 January 2013 17:13, Uwe Kleine-König
wrote:
> On Thu, Jan 24, 2013 at 04:47:53PM +0530, Viresh Kumar wrote:
>> So, the whole loop is for 9 pulses at max and it runs 18 times. Twice per
>> clock. With my code, i only check for sda after supplying full clock pulse,
>> and you are asking me to check that in middle of clock. Isn't it wrong?
> Actually in the data phase of a transfer sda only changes when scl is
> low. (Otherwise it's a stop condition.) So it should make sense to check
> after pulling scl low, doesn't it? Hmm, it trades one ndelay for
> (possibly several) get_sda. hmm, *shrug*.
In any case, we have to make scl high again. So, why not check it at
that stage only?
>> >> +int i2c_generic_scl_recovery(struct i2c_adapter *adap)
>> >> +{
>> >> + adap->bus_recovery_info->set_scl(adap, 1);
>> > Why this?
>>
>> This came out of some earlier discussion we had. We should start
>> with high and then do low-high 9 times.
> Ah, I missed the first val = !val and so thought you start with setting
> to 1 anyhow. So yes, I agree. Maybe document this assumption in a
> comment?
Some sort of comment is present in the routine which toggles the line,
which says "by this time scl must be high and we should apply 9 pulses
now"
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V9 1/2] i2c/adapter: Add bus recovery infrastructure
Hello,
On Thu, Jan 24, 2013 at 04:47:53PM +0530, Viresh Kumar wrote:
> On 24 January 2013 16:36, Uwe Kleine-König
> wrote:
> > On Thu, Jan 24, 2013 at 04:06:37PM +0530, Viresh Kumar wrote:
> >> Most number of versions for any patchset i submitted :)
> > So let's see if you do a v10 ... :-)
>
> Haha..
>
> >> +static int i2c_generic_recovery(struct i2c_adapter *adap)
> >> +{
> >> + struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
> >> + int i = 0, val = 1, ret = 0;
> >> +
> >> + if (bri->prepare_recovery)
> >> + bri->prepare_recovery(bri);
> >> +
> > Do we want to break out here if sda is high?
>
> Good point actually. It may happen we are asked to recover already working
> system :)
>
> >> + /*
> >> + * By this time SCL is high, as we need to give 9 falling-rising
> >> edges
> >> + */
> >> + while (i++ < RECOVERY_CLK_CNT * 2) {
> >> + /* SCL shouldn't be low here */
> >> + if (val && !bri->get_scl(adap)) {
> >> + dev_err(&adap->dev, "SCL is stuck Low exit
> >> recovery\n");
> >> + ret = -EBUSY;
> >> + goto unprepare;
> >> + }
> >> +
> >> + val = !val;
> >> + bri->set_scl(adap, val);
> >> + ndelay(clk_delay);
> >> +
> >> + /* break if sda got high, check only when scl line is high */
> > Above you wrote "SCL", here "scl". I suggest to use one of them
> > consistently and use the same capitalisation for sda.
>
> Sure.
>
> >> + if (bri->get_sda && val)
> >> + if (bri->get_sda(adap))
> >> + break;
> > Maybe better:
> > /* break if sda got high */
> > if (bri->get_sda && bri->get_sda(adap)) {
> > /* don't leave with scl low */
> > if (!val)
> > bri->set_scl(adap, 1);
> > break;
> > }
>
> So, the whole loop is for 9 pulses at max and it runs 18 times. Twice per
> clock. With my code, i only check for sda after supplying full clock pulse,
> and you are asking me to check that in middle of clock. Isn't it wrong?
Actually in the data phase of a transfer sda only changes when scl is
low. (Otherwise it's a stop condition.) So it should make sense to check
after pulling scl low, doesn't it? Hmm, it trades one ndelay for
(possibly several) get_sda. hmm, *shrug*.
> >> +int i2c_generic_scl_recovery(struct i2c_adapter *adap)
> >> +{
> >> + adap->bus_recovery_info->set_scl(adap, 1);
> > Why this?
>
> This came out of some earlier discussion we had. We should start
> with high and then do low-high 9 times.
Ah, I missed the first val = !val and so thought you start with setting
to 1 anyhow. So yes, I agree. Maybe document this assumption in a
comment?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V9 1/2] i2c/adapter: Add bus recovery infrastructure
On 24 January 2013 16:36, Uwe Kleine-König
wrote:
> On Thu, Jan 24, 2013 at 04:06:37PM +0530, Viresh Kumar wrote:
>> Most number of versions for any patchset i submitted :)
> So let's see if you do a v10 ... :-)
Haha..
>> +static int i2c_generic_recovery(struct i2c_adapter *adap)
>> +{
>> + struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
>> + int i = 0, val = 1, ret = 0;
>> +
>> + if (bri->prepare_recovery)
>> + bri->prepare_recovery(bri);
>> +
> Do we want to break out here if sda is high?
Good point actually. It may happen we are asked to recover already working
system :)
>> + /*
>> + * By this time SCL is high, as we need to give 9 falling-rising edges
>> + */
>> + while (i++ < RECOVERY_CLK_CNT * 2) {
>> + /* SCL shouldn't be low here */
>> + if (val && !bri->get_scl(adap)) {
>> + dev_err(&adap->dev, "SCL is stuck Low exit
>> recovery\n");
>> + ret = -EBUSY;
>> + goto unprepare;
>> + }
>> +
>> + val = !val;
>> + bri->set_scl(adap, val);
>> + ndelay(clk_delay);
>> +
>> + /* break if sda got high, check only when scl line is high */
> Above you wrote "SCL", here "scl". I suggest to use one of them
> consistently and use the same capitalisation for sda.
Sure.
>> + if (bri->get_sda && val)
>> + if (bri->get_sda(adap))
>> + break;
> Maybe better:
> /* break if sda got high */
> if (bri->get_sda && bri->get_sda(adap)) {
> /* don't leave with scl low */
> if (!val)
> bri->set_scl(adap, 1);
> break;
> }
So, the whole loop is for 9 pulses at max and it runs 18 times. Twice per
clock. With my code, i only check for sda after supplying full clock pulse,
and you are asking me to check that in middle of clock. Isn't it wrong?
>> +int i2c_generic_scl_recovery(struct i2c_adapter *adap)
>> +{
>> + adap->bus_recovery_info->set_scl(adap, 1);
> Why this?
This came out of some earlier discussion we had. We should start
with high and then do low-high 9 times.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V9 1/2] i2c/adapter: Add bus recovery infrastructure
Hello,
On Thu, Jan 24, 2013 at 04:06:37PM +0530, Viresh Kumar wrote:
> Most number of versions for any patchset i submitted :)
So let's see if you do a v10 ... :-)
> +static int i2c_generic_recovery(struct i2c_adapter *adap)
> +{
> + struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
> + int i = 0, val = 1, ret = 0;
> +
> + if (bri->prepare_recovery)
> + bri->prepare_recovery(bri);
> +
Do we want to break out here if sda is high?
> + /*
> + * By this time SCL is high, as we need to give 9 falling-rising edges
> + */
> + while (i++ < RECOVERY_CLK_CNT * 2) {
> + /* SCL shouldn't be low here */
> + if (val && !bri->get_scl(adap)) {
> + dev_err(&adap->dev, "SCL is stuck Low exit recovery\n");
> + ret = -EBUSY;
> + goto unprepare;
> + }
> +
> + val = !val;
> + bri->set_scl(adap, val);
> + ndelay(clk_delay);
> +
> + /* break if sda got high, check only when scl line is high */
Above you wrote "SCL", here "scl". I suggest to use one of them
consistently and use the same capitalisation for sda.
> + if (bri->get_sda && val)
> + if (bri->get_sda(adap))
> + break;
Maybe better:
/* break if sda got high */
if (bri->get_sda && bri->get_sda(adap)) {
/* don't leave with scl low */
if (!val)
bri->set_scl(adap, 1);
break;
}
> + }
> +
> +unprepare:
> + if (bri->unprepare_recovery)
> + bri->unprepare_recovery(bri);
> +
> + return ret;
> +}
> +
> +int i2c_generic_scl_recovery(struct i2c_adapter *adap)
> +{
> + adap->bus_recovery_info->set_scl(adap, 1);
Why this?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V9 1/2] i2c/adapter: Add bus recovery infrastructure
On 24 January 2013 16:06, Viresh Kumar wrote: > Add i2c bus recovery infrastructure to i2c adapters as specified in the i2c > protocol Rev. 03 section 3.1.16 titled "Bus clear". > > http://www.nxp.com/documents/user_manual/UM10204.pdf > > Sometimes during operation i2c bus hangs and we need to give dummy clocks to > slave device to start the transfer again. Now we may have capability in the > bus > controller to generate these clocks or platform may have gpio pins which can > be > toggled to generate dummy clocks. This patch supports both. > > This patch also adds in generic bus recovery routines gpio or scl line based > which can be used by bus controller. In addition controller driver may provide > its own version of the bus recovery routine. > > This doesn't support multi-master recovery for now. > > Reviewed-by: Paul Carpenter > Signed-off-by: Viresh Kumar ARM mail servers are broken now a days, so these patches may not apply from mail. Find them attached. 0001-i2c-adapter-Add-bus-recovery-infrastructure.patch Description: Binary data 0002-i2c-designware-Provide-i2c-bus-recovery-support.patch Description: Binary data
[PATCH V9 1/2] i2c/adapter: Add bus recovery infrastructure
Add i2c bus recovery infrastructure to i2c adapters as specified in the i2c
protocol Rev. 03 section 3.1.16 titled "Bus clear".
http://www.nxp.com/documents/user_manual/UM10204.pdf
Sometimes during operation i2c bus hangs and we need to give dummy clocks to
slave device to start the transfer again. Now we may have capability in the bus
controller to generate these clocks or platform may have gpio pins which can be
toggled to generate dummy clocks. This patch supports both.
This patch also adds in generic bus recovery routines gpio or scl line based
which can be used by bus controller. In addition controller driver may provide
its own version of the bus recovery routine.
This doesn't support multi-master recovery for now.
Reviewed-by: Paul Carpenter
Signed-off-by: Viresh Kumar
---
Most number of versions for any patchset i submitted :)
V8->V9:
- removed skip_sda_polling variable
- simplified/fixed i2c_generic_recovery() routine
- added new routine i2c_recover_bus()
- added gpio_is_valid() checks over scl and sda gpios
- code is rearranged at places (based on Wolfram's comments)
drivers/i2c/i2c-core.c | 155 +
include/linux/i2c.h| 39 +
2 files changed, 194 insertions(+)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index e388590..8076e52 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -27,7 +27,9 @@
#include
#include
+#include
#include
+#include
#include
#include
#include
@@ -109,6 +111,127 @@ static int i2c_device_uevent(struct device *dev, struct
kobj_uevent_env *env)
#define i2c_device_uevent NULL
#endif /* CONFIG_HOTPLUG */
+/* i2c bus recovery routines */
+#define RECOVERY_CLK_CNT 9
+#define DEFAULT_CLK_RATE 100 /* KHz */
+/* 10^6/KHz for delay in ns */
+unsigned long clk_delay = DIV_ROUND_UP(100, DEFAULT_CLK_RATE * 2);
+
+static int get_scl_gpio_value(struct i2c_adapter *adap)
+{
+ return gpio_get_value(adap->bus_recovery_info->scl_gpio);
+}
+
+static void set_scl_gpio_value(struct i2c_adapter *adap, int val)
+{
+ gpio_set_value(adap->bus_recovery_info->scl_gpio, val);
+}
+
+static int get_sda_gpio_value(struct i2c_adapter *adap)
+{
+ return gpio_get_value(adap->bus_recovery_info->sda_gpio);
+}
+
+static int i2c_get_gpios_for_recovery(struct i2c_adapter *adap)
+{
+ struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
+ struct device *dev = &adap->dev;
+ int ret = 0;
+
+ ret = gpio_request_one(bri->scl_gpio, GPIOF_OPEN_DRAIN |
+ GPIOF_OUT_INIT_HIGH, "i2c-scl");
+ if (ret) {
+ dev_warn(dev, "gpio request fail: %d\n", bri->scl_gpio);
+ return ret;
+ }
+
+ if (bri->get_sda) {
+ if (gpio_request_one(bri->sda_gpio, GPIOF_IN, "i2c-sda")) {
+ /* work without sda polling */
+ dev_warn(dev, "can't get sda: %d. Skip sda polling\n",
+ bri->sda_gpio);
+ bri->get_sda = NULL;
+ }
+ }
+
+ return ret;
+}
+
+static void i2c_put_gpios_for_recovery(struct i2c_adapter *adap)
+{
+ struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
+
+ if (bri->get_sda)
+ gpio_free(bri->sda_gpio);
+
+ gpio_free(bri->scl_gpio);
+}
+
+static int i2c_generic_recovery(struct i2c_adapter *adap)
+{
+ struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
+ int i = 0, val = 1, ret = 0;
+
+ if (bri->prepare_recovery)
+ bri->prepare_recovery(bri);
+
+ /*
+* By this time SCL is high, as we need to give 9 falling-rising edges
+*/
+ while (i++ < RECOVERY_CLK_CNT * 2) {
+ /* SCL shouldn't be low here */
+ if (val && !bri->get_scl(adap)) {
+ dev_err(&adap->dev, "SCL is stuck Low exit recovery\n");
+ ret = -EBUSY;
+ goto unprepare;
+ }
+
+ val = !val;
+ bri->set_scl(adap, val);
+ ndelay(clk_delay);
+
+ /* break if sda got high, check only when scl line is high */
+ if (bri->get_sda && val)
+ if (bri->get_sda(adap))
+ break;
+ }
+
+unprepare:
+ if (bri->unprepare_recovery)
+ bri->unprepare_recovery(bri);
+
+ return ret;
+}
+
+int i2c_generic_scl_recovery(struct i2c_adapter *adap)
+{
+ adap->bus_recovery_info->set_scl(adap, 1);
+ return i2c_generic_recovery(adap);
+}
+
+int i2c_generic_gpio_recovery(struct i2c_adapter *adap)
+{
+ int ret;
+
+ ret = i2c_get_gpios_for_recovery(adap);
+ if (ret)
+ return ret;
+
+ ret = i2c_generic_recovery(adap);
+ i2c_put_gpios_for_recovery(adap);
+
+ return ret;
+}
+
+int i2c_recover_bus(st
