Hi David,
In fact, in current octeon_i2c_probe(), there has the bus reset unconditionally,
It is reset by octeon_i2c_init_lowlevel() unconditionally.
In my patch, I replace it by octeon_i2c_recovery(), which will be executed
under three conditions.
+ /*
+ * Try to recover bus in three conditions: TWSI core status
+ * not IDLE, SDA stuck low or TWSI_CTL_IFLG not cleared
+ */
+ if ((octeon_i2c_stat_read(i2c) != STAT_IDLE) ||
+ !(octeon_i2c_read_int(i2c) & TWSI_INT_SDA_OVR) ||
+ (octeon_i2c_ctl_read(i2c) & TWSI_CTL_IFLG)) {
+ result = octeon_i2c_recovery(i2c);
+ if (result) {
+ dev_err(i2c->dev, "octeon i2c recovery failed\n");
+ goto out;
+ }
+ }
+
Attached improved patch and C file for your review. Thanks in advance.
BR,
Sean Zhang
-----Original Message-----
From: David Daney [mailto:ddaney@xxxxxxxxxxxxxxxxxx]
Sent: Wednesday, December 06, 2017 12:43 AM
To: Zhang, Sean C. (NSB - CN/Hangzhou) <sean.c.zhang@xxxxxxxxxxxxxxx>; Jan Glauber <jan.glauber@xxxxxxxxxxxxxxxxxx>; david.daney@xxxxxxxxxx
Cc: wsa@xxxxxxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
Subject: Re: [Bug fix] octeon-i2c driver updates
On 12/04/2017 10:44 PM, Zhang, Sean C. (NSB - CN/Hangzhou) wrote:
> Hi Jan,
>
> Thanks for your comments, I get your point for the second point (retry of START after recovery).
>
> Hi David,
> For the issue as the first one, would you give your further comments? Thanks in advance.
>
> I have an environment with CN6780 (TWSI core has property: compatible
> = "cavium,octeon-3860-twsi"), And encounter below problem:
> During i2c-octeon driver probing, this TWSI core original status is
> 0x20 (this may induced by uboot), And octeon_i2c_init_lowlevel()
> function in octeon_i2c_probe() is not enough to recover the I2C bus,
> If go without full recovery of octeon_i2c_recovery(), the following
> octeon_i2c_hlc_write(), octeon_i2c_hlc_read(), octeon_i2c_hlc_comp_read() and octeon_i2c_hlc_comp_write() will goes error, because these functions has no bus recovery step.
> While after replace octeon_i2c_init_lowlevel() with
> octeon_i2c_recovery() in octeon_i2c_probe(), the problem has gone.
>
> Once more, this octeon_i2c_recovery() can also recover dead lock (I2C
> slave device stuck low SCL) issue, so I think use octeon_i2c_recovery() instead will be stronger.
I don't want to do a bus reset unconditionally, as it is currently working well on many systems.
Perhaps you could add a module parameter to enable the recovery mode on probe as an option. Would that work or be acceptable?
Thanks,
David Daney
>
> BR,
> Sean Zhang
>
>
> -----Original Message-----
> From: Jan Glauber [mailto:jan.glauber@xxxxxxxxxxxxxxxxxx]
> Sent: Friday, December 01, 2017 6:07 PM
> To: Zhang, Sean C. (NSB - CN/Hangzhou) <sean.c.zhang@xxxxxxxxxxxxxxx>
> Cc: david.daney@xxxxxxxxxx; wsa@xxxxxxxxxxxxx;
> linux-i2c@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [Bug fix] octeon-i2c driver updates
>
> Hi Sean,
>
> as you try to solve two different issues I suggest that you create one
> patch per issue.
>
> For the second point (retry of START after recovery) I would still
> like to hear Wolfram's opinion. I would assume that any i2c user
> should be well aware of -EAGAIN, so I wonder if it is worth the
> additional complexity of the retry logic.
>
> Also, the first issue changes Octeon MIPS which I'm not able to test,
> so David needs to be involved here.
>
> thanks,
> Jan
>
> On Thu, Nov 30, 2017 at 01:56:09AM +0000, Zhang, Sean C. (NSB - CN/Hangzhou) wrote:
>> Hi Jan,
>>
>> Any other comments for this patch?
>>
>> BR,
>> Sean Zhang
>>
>> -----Original Message-----
>> From: Zhang, Sean C. (NSB - CN/Hangzhou)
>> Sent: Monday, November 27, 2017 4:38 PM
>> To: 'Jan Glauber' <jan.glauber@xxxxxxxxxxxxxxxxxx>
>> Cc: david.daney@xxxxxxxxxx; wsa@xxxxxxxxxxxxx;
>> linux-i2c@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
>> Subject: RE: [Bug fix] octeon-i2c driver updates
>>
>> Hi Jan,
>>
>> There are two points in this patch.
>>
>> Point 1. As you see, replaced octeon_i2c_init_lowlevel() by recover bus status if TWSI controller is not IDLE.
>> Please take a scenario like this: when system soft reset without I2C
>> slave reset, maybe make this I2C bus dead lock occurred (I2C slave
>> device stuck low SCL) in chance. Then during system goes up and I2C
>> slave device creating process, if this I2C slave device has a register with less than 8 bytes to read, but I2C bus was still stuck low SCL by last system reset, then the read will failed and this I2C slave device cannot be created.
>> If bus recovered before the reading process, this failure can be fixed.
>>
>> Function flow explanation shown as below:
>>
>> a. System reset without I2C slave device reset --make SCL stuck low
>> by I2C slave device ......
>> b. octeon_i2c_probe（）
>> -- octeon_i2c_init_lowlevel //reset TWSI core, but SCL still stuck low by.
>> ......
>>
>> c. Another I2C slave device creating process
>> octeon_i2c_xfer()
>> -- octeon_i2c_hlc_comp_read() //failed due to SCL stuck low.
>>
>> If full recovery executed in octeon_i2c_probe（), above failure can be avoided.
>>
>>
>> Point 2. octeon_i2c_recovery() is used in octeon_i2c_start() error
>> branch, in the case of octeon_i2c_recovery() successful,
>> octeon_i2c_start() will return -EAGAIN, and then octeon_i2c_xfer()
>> return with error. I understand this like
>> this: if octeon_i2c_recovery() successful, then i2c START signal can
>> be sent again, and all following step can be continue,
>> octeon_i2c_xfer() should not return error from this condition.
>>
>> BR,
>> Sean Zhang
>>
>> -----Original Message-----
>> From: Jan Glauber [mailto:jan.glauber@xxxxxxxxxxxxxxxxxx]
>> Sent: Friday, November 24, 2017 9:10 PM
>> To: Zhang, Sean C. (NSB - CN/Hangzhou) <sean.c.zhang@xxxxxxxxxxxxxxx>
>> Cc: david.daney@xxxxxxxxxx; wsa@xxxxxxxxxxxxx;
>> linux-i2c@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
>> Subject: Re: [Bug fix] octeon-i2c driver updates
>>
>> On Thu, Nov 23, 2017 at 11:42:36AM +0000, Zhang, Sean C. (NSB - CN/Hangzhou) wrote:
>>> Dear Maintainer,
>>>
>>> For octeon TWSI controller, I found below two cases, maybe can be improved.
>>
>> Hi Sean,
>>
>> form the description below this looks like you're fixing a bug. Can
>> you elaborate on when the I2C bus dead lock occurs. Is it always happening?
>>
>> What I don't like about the patch is that you're removing
>> octeon_i2c_init_lowlevel() from the probe and replacing it by
>> _always_ going through a full recovery. Why is this neccessary?
>>
>> Regards,
>> Jan
>>
>>>
>>> >From 09d9f0ce658d7f6a50d1af352dde619c29bc8bcf Mon Sep 17 00:00:00
>>> >2001
>>> From: hgt463 <sean.c.zhang@xxxxxxxxx>
>>> Date: Thu, 23 Nov 2017 18:46:09 +0800
>>> Subject: [PATCH] Driver updates:
>>> - In the case of I2C bus dead lock occurred during driver probing,
>>> it is better try to recovery it. so added bus recovery step in
>>> octeon_i2c_probe();
>>> - The purpose of function octeon_i2c_start() is to send START, so after
>>> bus recovery, also need try to send START again.
>>>
>>> Signed-off-by: hgt463 <sean.c.zhang@xxxxxxxxx>
>>> ---
>>> drivers/i2c/busses/i2c-octeon-core.c | 31 ++++++++++++++++++++++++++++++-
>>> drivers/i2c/busses/i2c-octeon-platdrv.c | 15 +++++++++------
>>> 2 files changed, 39 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-octeon-core.c
>>> b/drivers/i2c/busses/i2c-octeon-core.c
>>> index 1d87757..3ae1e03 100644
>>> --- a/drivers/i2c/busses/i2c-octeon-core.c
>>> +++ b/drivers/i2c/busses/i2c-octeon-core.c
>>> @@ -255,6 +255,35 @@ static int octeon_i2c_recovery(struct octeon_i2c *i2c)
>>> return ret;
>>> }
>>>
>>> +/*
>>> + * octeon_i2c_start_retry - send START to the bus after bus recovery.
>>> + * @i2c: The struct octeon_i2c
>>> + *
>>> + * Returns 0 on success, otherwise a negative errno.
>>> + */
>>> +static int octeon_i2c_start_retry(struct octeon_i2c *i2c) {
>>> + int ret;
>>> + u8 stat;
>>> +
>>> + ret = octeon_i2c_recovery(i2c);
>>> + if (ret)
>>> + goto error;
>>> +
>>> + octeon_i2c_ctl_write(i2c, TWSI_CTL_ENAB | TWSI_CTL_STA);
>>> + ret = octeon_i2c_wait(i2c);
>>> + if (ret)
>>> + goto error;
>>> +
>>> + stat = octeon_i2c_stat_read(i2c);
>>> + if (stat == STAT_START || stat == STAT_REP_START)
>>> + /* START successful, bail out */
>>> + return 0;
>>> +
>>> +error:
>>> + return (ret) ? ret : -EAGAIN; }
>>> +
>>> /**
>>> * octeon_i2c_start - send START to the bus
>>> * @i2c: The struct octeon_i2c
>>> @@ -280,7 +309,7 @@ static int octeon_i2c_start(struct octeon_i2c
>>> *i2c)
>>>
>>> error:
>>> /* START failed, try to recover */
>>> - ret = octeon_i2c_recovery(i2c);
>>> + ret = octeon_i2c_start_retry(i2c);
>>> return (ret) ? ret : -EAGAIN;
>>> }
>>>
>>> diff --git a/drivers/i2c/busses/i2c-octeon-platdrv.c
>>> b/drivers/i2c/busses/i2c-octeon-platdrv.c
>>> index 64bda83..98063af 100644
>>> --- a/drivers/i2c/busses/i2c-octeon-platdrv.c
>>> +++ b/drivers/i2c/busses/i2c-octeon-platdrv.c
>>> @@ -228,12 +228,6 @@ static int octeon_i2c_probe(struct platform_device *pdev)
>>> if (OCTEON_IS_MODEL(OCTEON_CN38XX))
>>> i2c->broken_irq_check = true;
>>>
>>> - result = octeon_i2c_init_lowlevel(i2c);
>>> - if (result) {
>>> - dev_err(i2c->dev, "init low level failed\n");
>>> - goto out;
>>> - }
>>> -
>>> octeon_i2c_set_clock(i2c);
>>>
>>> i2c->adap = octeon_i2c_ops; @@ -245,6 +239,15 @@ static int
>>> octeon_i2c_probe(struct platform_device *pdev)
>>> i2c_set_adapdata(&i2c->adap, i2c);
>>> platform_set_drvdata(pdev, i2c);
>>>
>>> + stat = octeon_i2c_stat_read(i2c);
>>> + if (stat != STAT_IDLE) {
>>> + result = octeon_i2c_recovery(i2c);
>>> + if (result) {
>>> + dev_err(i2c->dev, "octeon i2c recovery failed\n");
>>> + goto out;
>>> + }
>>> + }
>>> +
>>> result = i2c_add_adapter(&i2c->adap);
>>> if (result < 0)
>>> goto out;
>>>
>>>
>>> Attached patch for you review. Thanks in advance.
>>>
>>> BR,
>>> Sean Zhang
>>
>>
Attachment:
0001-Driver-updates.patch
Description: 0001-Driver-updates.patch
/*
* (C) Copyright 2009-2010
* Nokia Siemens Networks, michael.lawnick.ext@xxxxxxx
*
* Portions Copyright (C) 2010 - 2016 Cavium, Inc.
*
* This is a driver for the i2c adapter in Cavium Networks' OCTEON processors.
*
* This file is licensed under the terms of the GNU General Public
* License version 2. This program is licensed "as is" without any
* warranty of any kind, whether express or implied.
*/
#include <linux/atomic.h>
#include <linux/delay.h>
#include <linux/i2c.h>
#include <linux/interrupt.h>
#include <linux/io.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/of.h>
#include <linux/platform_device.h>
#include <linux/sched.h>
#include <linux/slab.h>
#include <asm/octeon/octeon.h>
#include "i2c-octeon-core.h"
#define DRV_NAME "i2c-octeon"
/**
* octeon_i2c_int_enable - enable the CORE interrupt
* @i2c: The struct octeon_i2c
*
* The interrupt will be asserted when there is non-STAT_IDLE state in
* the SW_TWSI_EOP_TWSI_STAT register.
*/
static void octeon_i2c_int_enable(struct octeon_i2c *i2c)
{
octeon_i2c_write_int(i2c, TWSI_INT_CORE_EN);
}
/* disable the CORE interrupt */
static void octeon_i2c_int_disable(struct octeon_i2c *i2c)
{
/* clear TS/ST/IFLG events */
octeon_i2c_write_int(i2c, 0);
}
/**
* octeon_i2c_int_enable78 - enable the CORE interrupt
* @i2c: The struct octeon_i2c
*
* The interrupt will be asserted when there is non-STAT_IDLE state in the
* SW_TWSI_EOP_TWSI_STAT register.
*/
static void octeon_i2c_int_enable78(struct octeon_i2c *i2c)
{
atomic_inc_return(&i2c->int_enable_cnt);
enable_irq(i2c->irq);
}
static void __octeon_i2c_irq_disable(atomic_t *cnt, int irq)
{
int count;
/*
* The interrupt can be disabled in two places, but we only
* want to make the disable_irq_nosync() call once, so keep
* track with the atomic variable.
*/
count = atomic_dec_if_positive(cnt);
if (count >= 0)
disable_irq_nosync(irq);
}
/* disable the CORE interrupt */
static void octeon_i2c_int_disable78(struct octeon_i2c *i2c)
{
__octeon_i2c_irq_disable(&i2c->int_enable_cnt, i2c->irq);
}
/**
* octeon_i2c_hlc_int_enable78 - enable the ST interrupt
* @i2c: The struct octeon_i2c
*
* The interrupt will be asserted when there is non-STAT_IDLE state in
* the SW_TWSI_EOP_TWSI_STAT register.
*/
static void octeon_i2c_hlc_int_enable78(struct octeon_i2c *i2c)
{
atomic_inc_return(&i2c->hlc_int_enable_cnt);
enable_irq(i2c->hlc_irq);
}
/* disable the ST interrupt */
static void octeon_i2c_hlc_int_disable78(struct octeon_i2c *i2c)
{
__octeon_i2c_irq_disable(&i2c->hlc_int_enable_cnt, i2c->hlc_irq);
}
/* HLC interrupt service routine */
static irqreturn_t octeon_i2c_hlc_isr78(int irq, void *dev_id)
{
struct octeon_i2c *i2c = dev_id;
i2c->hlc_int_disable(i2c);
wake_up(&i2c->queue);
return IRQ_HANDLED;
}
static void octeon_i2c_hlc_int_enable(struct octeon_i2c *i2c)
{
octeon_i2c_write_int(i2c, TWSI_INT_ST_EN);
}
static u32 octeon_i2c_functionality(struct i2c_adapter *adap)
{
return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK) |
I2C_FUNC_SMBUS_READ_BLOCK_DATA | I2C_SMBUS_BLOCK_PROC_CALL;
}
static const struct i2c_algorithm octeon_i2c_algo = {
.master_xfer = octeon_i2c_xfer,
.functionality = octeon_i2c_functionality,
};
static const struct i2c_adapter octeon_i2c_ops = {
.owner = THIS_MODULE,
.name = "OCTEON adapter",
.algo = &octeon_i2c_algo,
};
static int octeon_i2c_probe(struct platform_device *pdev)
{
struct device_node *node = pdev->dev.of_node;
int irq, result = 0, hlc_irq = 0;
struct resource *res_mem;
struct octeon_i2c *i2c;
bool cn78xx_style;
cn78xx_style = of_device_is_compatible(node, "cavium,octeon-7890-twsi");
if (cn78xx_style) {
hlc_irq = platform_get_irq(pdev, 0);
if (hlc_irq < 0)
return hlc_irq;
irq = platform_get_irq(pdev, 2);
if (irq < 0)
return irq;
} else {
/* All adaptors have an irq. */
irq = platform_get_irq(pdev, 0);
if (irq < 0)
return irq;
}
i2c = devm_kzalloc(&pdev->dev, sizeof(*i2c), GFP_KERNEL);
if (!i2c) {
result = -ENOMEM;
goto out;
}
i2c->dev = &pdev->dev;
i2c->roff.sw_twsi = 0x00;
i2c->roff.twsi_int = 0x10;
i2c->roff.sw_twsi_ext = 0x18;
res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
i2c->twsi_base = devm_ioremap_resource(&pdev->dev, res_mem);
if (IS_ERR(i2c->twsi_base)) {
result = PTR_ERR(i2c->twsi_base);
goto out;
}
/*
* "clock-rate" is a legacy binding, the official binding is
* "clock-frequency". Try the official one first and then
* fall back if it doesn't exist.
*/
if (of_property_read_u32(node, "clock-frequency", &i2c->twsi_freq) &&
of_property_read_u32(node, "clock-rate", &i2c->twsi_freq)) {
dev_err(i2c->dev,
"no I2C 'clock-rate' or 'clock-frequency' property\n");
result = -ENXIO;
goto out;
}
i2c->sys_freq = octeon_get_io_clock_rate();
init_waitqueue_head(&i2c->queue);
i2c->irq = irq;
if (cn78xx_style) {
i2c->hlc_irq = hlc_irq;
i2c->int_enable = octeon_i2c_int_enable78;
i2c->int_disable = octeon_i2c_int_disable78;
i2c->hlc_int_enable = octeon_i2c_hlc_int_enable78;
i2c->hlc_int_disable = octeon_i2c_hlc_int_disable78;
irq_set_status_flags(i2c->irq, IRQ_NOAUTOEN);
irq_set_status_flags(i2c->hlc_irq, IRQ_NOAUTOEN);
result = devm_request_irq(&pdev->dev, i2c->hlc_irq,
octeon_i2c_hlc_isr78, 0,
DRV_NAME, i2c);
if (result < 0) {
dev_err(i2c->dev, "failed to attach interrupt\n");
goto out;
}
} else {
i2c->int_enable = octeon_i2c_int_enable;
i2c->int_disable = octeon_i2c_int_disable;
i2c->hlc_int_enable = octeon_i2c_hlc_int_enable;
i2c->hlc_int_disable = octeon_i2c_int_disable;
}
result = devm_request_irq(&pdev->dev, i2c->irq,
octeon_i2c_isr, 0, DRV_NAME, i2c);
if (result < 0) {
dev_err(i2c->dev, "failed to attach interrupt\n");
goto out;
}
if (OCTEON_IS_MODEL(OCTEON_CN38XX))
i2c->broken_irq_check = true;
octeon_i2c_set_clock(i2c);
i2c->adap = octeon_i2c_ops;
i2c->adap.timeout = msecs_to_jiffies(2);
i2c->adap.retries = 5;
i2c->adap.bus_recovery_info = &octeon_i2c_recovery_info;
i2c->adap.dev.parent = &pdev->dev;
i2c->adap.dev.of_node = node;
i2c_set_adapdata(&i2c->adap, i2c);
platform_set_drvdata(pdev, i2c);
/*
* Try to recover bus in three conditions: TWSI core status
* not IDLE, SDA stucked low or TWSI_CTL_IFLG not cleared
*/
if ((octeon_i2c_stat_read(i2c) != STAT_IDLE) ||
!(octeon_i2c_read_int(i2c) & TWSI_INT_SDA_OVR) ||
(octeon_i2c_ctl_read(i2c) & TWSI_CTL_IFLG)) {
result = octeon_i2c_recovery(i2c);
if (result) {
dev_err(i2c->dev, "octeon i2c recovery failed\n");
goto out;
}
}
result = i2c_add_adapter(&i2c->adap);
if (result < 0)
goto out;
dev_info(i2c->dev, "probed\n");
return 0;
out:
return result;
};
static int octeon_i2c_remove(struct platform_device *pdev)
{
struct octeon_i2c *i2c = platform_get_drvdata(pdev);
i2c_del_adapter(&i2c->adap);
return 0;
};
static const struct of_device_id octeon_i2c_match[] = {
{ .compatible = "cavium,octeon-3860-twsi", },
{ .compatible = "cavium,octeon-7890-twsi", },
{},
};
MODULE_DEVICE_TABLE(of, octeon_i2c_match);
static struct platform_driver octeon_i2c_driver = {
.probe = octeon_i2c_probe,
.remove = octeon_i2c_remove,
.driver = {
.name = DRV_NAME,
.of_match_table = octeon_i2c_match,
},
};
module_platform_driver(octeon_i2c_driver);
MODULE_AUTHOR("Michael Lawnick <michael.lawnick.ext@xxxxxxx>");
MODULE_DESCRIPTION("I2C-Bus adapter for Cavium OCTEON processors");
MODULE_LICENSE("GPL");
