Messages in this thread Patch in this message | | | From | "Zhang, Sean C. (NSB - CN/Hangzhou)" <> | Subject | RE: [Bug fix] octeon-i2c driver updates | Date | Wed, 6 Dec 2017 07:11:28 +0000 |
| |
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@caviumnetworks.com] Sent: Wednesday, December 06, 2017 12:43 AM To: Zhang, Sean C. (NSB - CN/Hangzhou) <sean.c.zhang@nokia-sbell.com>; Jan Glauber <jan.glauber@caviumnetworks.com>; david.daney@cavium.com Cc: wsa@the-dreams.de; linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org 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@caviumnetworks.com] > Sent: Friday, December 01, 2017 6:07 PM > To: Zhang, Sean C. (NSB - CN/Hangzhou) <sean.c.zhang@nokia-sbell.com> > Cc: david.daney@cavium.com; wsa@the-dreams.de; > linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org > 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@caviumnetworks.com> >> Cc: david.daney@cavium.com; wsa@the-dreams.de; >> linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org >> 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@caviumnetworks.com] >> Sent: Friday, November 24, 2017 9:10 PM >> To: Zhang, Sean C. (NSB - CN/Hangzhou) <sean.c.zhang@nokia-sbell.com> >> Cc: david.daney@cavium.com; wsa@the-dreams.de; >> linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org >> 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@nokia.com> >>> 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@nokia.com> >>> --- >>> 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 >> >>
[unhandled content-type:application/octet-stream]/* * (C) Copyright 2009-2010 * Nokia Siemens Networks, michael.lawnick.ext@nsn.com * * 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@nsn.com>"); MODULE_DESCRIPTION("I2C-Bus adapter for Cavium OCTEON processors"); MODULE_LICENSE("GPL");
| |