RE: [Bug fix] octeon-i2c driver improvement [2/2]

[Zhang, Sean C. (NSB - CN/Hangzhou)]

Hi Jan and David,

Any other comment for these three patches? I sent them separately.
1. [Bug fix] octeon-i2c driver updates
2. [Bug fix] octeon-i2c driver improvement [1/2]
3. [Bug fix] octeon-i2c driver improvement [2/2]


BR,
Sean Zhang

-Original Message-
From: Zhang, Sean C. (NSB - CN/Hangzhou) 
Sent: Wednesday, December 06, 2017 5:51 PM
To: 'David Daney' <dda...@caviumnetworks.com>; 'Jan Glauber' 
<jan.glau...@caviumnetworks.com>; 'david.da...@cavium.com' 
<david.da...@cavium.com>
Cc: 'w...@the-dreams.de' <w...@the-dreams.de>; 'linux-...@vger.kernel.org' 
<linux-...@vger.kernel.org>; 'linux-kernel@vger.kernel.org' 
<linux-kernel@vger.kernel.org>
Subject: [Bug fix] octeon-i2c driver improvement [2/2]

Hi Jan and David,

For octeon-i2c driver, there has duplicated interrupt disable in 
octeon_i2c_isr(),
octeon_i2c_hlc_wait() and octeon_i2c_wait(), since octeon_i2c_hlc_wait() and
octeon_i2c_wait() has pair of interrupt enable and disable, so the interrupt 
disable in octeon_i2c_isr() is not necessary. attached patch removed this 
unnecessary interrupt disable step.

Please have time to review it. Thanks.

BR
Sean Zhang

RE: [Bug fix] octeon-i2c driver improvement [2/2]

[Zhang, Sean C. (NSB - CN/Hangzhou)]

Hi Jan and David,

Any other comment for these three patches? I sent them separately.
1. [Bug fix] octeon-i2c driver updates
2. [Bug fix] octeon-i2c driver improvement [1/2]
3. [Bug fix] octeon-i2c driver improvement [2/2]


BR,
Sean Zhang

-Original Message-
From: Zhang, Sean C. (NSB - CN/Hangzhou) 
Sent: Wednesday, December 06, 2017 5:51 PM
To: 'David Daney' ; 'Jan Glauber' 
; 'david.da...@cavium.com' 

Cc: 'w...@the-dreams.de' ; 'linux-...@vger.kernel.org' 
; 'linux-kernel@vger.kernel.org' 

Subject: [Bug fix] octeon-i2c driver improvement [2/2]

Hi Jan and David,

For octeon-i2c driver, there has duplicated interrupt disable in 
octeon_i2c_isr(),
octeon_i2c_hlc_wait() and octeon_i2c_wait(), since octeon_i2c_hlc_wait() and
octeon_i2c_wait() has pair of interrupt enable and disable, so the interrupt 
disable in octeon_i2c_isr() is not necessary. attached patch removed this 
unnecessary interrupt disable step.

Please have time to review it. Thanks.

BR
Sean Zhang

[Bug fix] octeon-i2c driver improvement [2/2]

[Zhang, Sean C. (NSB - CN/Hangzhou)]

Hi Jan and David,

For octeon-i2c driver, there has duplicated interrupt disable in 
octeon_i2c_isr(),
octeon_i2c_hlc_wait() and octeon_i2c_wait(), since octeon_i2c_hlc_wait() and
octeon_i2c_wait() has pair of interrupt enable and disable, so the interrupt
disable in octeon_i2c_isr() is not necessary. attached patch removed this
unnecessary interrupt disable step.

Please have time to review it. Thanks.

BR
Sean Zhang


0001-i2c-octeon-driver-improvement.patch
Description: 0001-i2c-octeon-driver-improvement.patch
/*
 * (C) Copyright 2009-2010
 * Nokia Siemens Networks, michael.lawnick@nsn.com
 *
 * Portions Copyright (C) 2010 - 2016 Cavium, Inc.
 *
 * This file contains the shared part of the driver for the i2c adapter in
 * Cavium Networks' OCTEON processors and ThunderX SOCs.
 *
 * 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 
#include 
#include 
#include 
#include 

#include "i2c-octeon-core.h"

/* interrupt service routine */
irqreturn_t octeon_i2c_isr(int irq, void *dev_id)
{
struct octeon_i2c *i2c = dev_id;

wake_up(>queue);

return IRQ_HANDLED;
}

static bool octeon_i2c_test_iflg(struct octeon_i2c *i2c)
{
return (octeon_i2c_ctl_read(i2c) & TWSI_CTL_IFLG);
}

/**
 * octeon_i2c_wait - wait for the IFLG to be set
 * @i2c: The struct octeon_i2c
 *
 * Returns 0 on success, otherwise a negative errno.
 */
static int octeon_i2c_wait(struct octeon_i2c *i2c)
{
long time_left;

/*
 * Some chip revisions don't assert the irq in the interrupt
 * controller. So we must poll for the IFLG change.
 */
if (i2c->broken_irq_mode) {
u64 end = get_jiffies_64() + i2c->adap.timeout;

while (!octeon_i2c_test_iflg(i2c) &&
   time_before64(get_jiffies_64(), end))
usleep_range(I2C_OCTEON_EVENT_WAIT / 2, 
I2C_OCTEON_EVENT_WAIT);

return octeon_i2c_test_iflg(i2c) ? 0 : -ETIMEDOUT;
}

i2c->int_enable(i2c);
time_left = wait_event_timeout(i2c->queue, octeon_i2c_test_iflg(i2c),
   i2c->adap.timeout);
i2c->int_disable(i2c);

if (i2c->broken_irq_check && !time_left &&
octeon_i2c_test_iflg(i2c)) {
dev_err(i2c->dev, "broken irq connection detected, switching to 
polling mode.\n");
i2c->broken_irq_mode = true;
return 0;
}

if (!time_left)
return -ETIMEDOUT;

return 0;
}

static bool octeon_i2c_hlc_test_valid(struct octeon_i2c *i2c)
{
return (__raw_readq(i2c->twsi_base + SW_TWSI(i2c)) & SW_TWSI_V) == 0;
}

static void octeon_i2c_hlc_int_clear(struct octeon_i2c *i2c)
{
/* clear ST/TS events, listen for neither */
octeon_i2c_write_int(i2c, TWSI_INT_ST_INT | TWSI_INT_TS_INT);
}

/*
 * Cleanup low-level state & enable high-level controller.
 */
static void octeon_i2c_hlc_enable(struct octeon_i2c *i2c)
{
int try = 0;
u64 val;

if (i2c->hlc_enabled)
return;
i2c->hlc_enabled = true;

while (1) {
val = octeon_i2c_ctl_read(i2c);
if (!(val & (TWSI_CTL_STA | TWSI_CTL_STP)))
break;

/* clear IFLG event */
if (val & TWSI_CTL_IFLG)
octeon_i2c_ctl_write(i2c, TWSI_CTL_ENAB);

if (try++ > 100) {
pr_err("%s: giving up\n", __func__);
break;
}

/* spin until any start/stop has finished */
udelay(10);
}
octeon_i2c_ctl_write(i2c, TWSI_CTL_CE | TWSI_CTL_AAK | TWSI_CTL_ENAB);
}

static void octeon_i2c_hlc_disable(struct octeon_i2c *i2c)
{
if (!i2c->hlc_enabled)
return;

i2c->hlc_enabled = false;
octeon_i2c_ctl_write(i2c, TWSI_CTL_ENAB);
}

/**
 * octeon_i2c_hlc_wait - wait for an HLC operation to complete
 * @i2c: The struct octeon_i2c
 *
 * Returns 0 on success, otherwise -ETIMEDOUT.
 */
static int octeon_i2c_hlc_wait(struct octeon_i2c *i2c)
{
int time_left;

/*
 * Some cn38xx boards don't assert the irq in the interrupt
 * controller. So we must poll for the valid bit change.
 */
if (i2c->broken_irq_mode) {
u64 end = get_jiffies_64() + i2c->adap.timeout;

while (!octeon_i2c_hlc_test_valid(i2c) &&
   time_before64(get_jiffies_64(), end))
usleep_range(I2C_OCTEON_EVENT_WAIT / 2, 
I2C_OCTEON_EVENT_WAIT);

return octeon_i2c_hlc_test_valid(i2c) ? 0 : -ETIMEDOUT;
}

i2c->hlc_int_enable(i2c);
time_left = wait_event_timeout(i2c->queue,
   

[Bug fix] octeon-i2c driver improvement [2/2]

[Zhang, Sean C. (NSB - CN/Hangzhou)]

Hi Jan and David,

For octeon-i2c driver, there has duplicated interrupt disable in 
octeon_i2c_isr(),
octeon_i2c_hlc_wait() and octeon_i2c_wait(), since octeon_i2c_hlc_wait() and
octeon_i2c_wait() has pair of interrupt enable and disable, so the interrupt
disable in octeon_i2c_isr() is not necessary. attached patch removed this
unnecessary interrupt disable step.

Please have time to review it. Thanks.

BR
Sean Zhang


0001-i2c-octeon-driver-improvement.patch
Description: 0001-i2c-octeon-driver-improvement.patch
/*
 * (C) Copyright 2009-2010
 * Nokia Siemens Networks, michael.lawnick@nsn.com
 *
 * Portions Copyright (C) 2010 - 2016 Cavium, Inc.
 *
 * This file contains the shared part of the driver for the i2c adapter in
 * Cavium Networks' OCTEON processors and ThunderX SOCs.
 *
 * 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 
#include 
#include 
#include 
#include 

#include "i2c-octeon-core.h"

/* interrupt service routine */
irqreturn_t octeon_i2c_isr(int irq, void *dev_id)
{
struct octeon_i2c *i2c = dev_id;

wake_up(>queue);

return IRQ_HANDLED;
}

static bool octeon_i2c_test_iflg(struct octeon_i2c *i2c)
{
return (octeon_i2c_ctl_read(i2c) & TWSI_CTL_IFLG);
}

/**
 * octeon_i2c_wait - wait for the IFLG to be set
 * @i2c: The struct octeon_i2c
 *
 * Returns 0 on success, otherwise a negative errno.
 */
static int octeon_i2c_wait(struct octeon_i2c *i2c)
{
long time_left;

/*
 * Some chip revisions don't assert the irq in the interrupt
 * controller. So we must poll for the IFLG change.
 */
if (i2c->broken_irq_mode) {
u64 end = get_jiffies_64() + i2c->adap.timeout;

while (!octeon_i2c_test_iflg(i2c) &&
   time_before64(get_jiffies_64(), end))
usleep_range(I2C_OCTEON_EVENT_WAIT / 2, 
I2C_OCTEON_EVENT_WAIT);

return octeon_i2c_test_iflg(i2c) ? 0 : -ETIMEDOUT;
}

i2c->int_enable(i2c);
time_left = wait_event_timeout(i2c->queue, octeon_i2c_test_iflg(i2c),
   i2c->adap.timeout);
i2c->int_disable(i2c);

if (i2c->broken_irq_check && !time_left &&
octeon_i2c_test_iflg(i2c)) {
dev_err(i2c->dev, "broken irq connection detected, switching to 
polling mode.\n");
i2c->broken_irq_mode = true;
return 0;
}

if (!time_left)
return -ETIMEDOUT;

return 0;
}

static bool octeon_i2c_hlc_test_valid(struct octeon_i2c *i2c)
{
return (__raw_readq(i2c->twsi_base + SW_TWSI(i2c)) & SW_TWSI_V) == 0;
}

static void octeon_i2c_hlc_int_clear(struct octeon_i2c *i2c)
{
/* clear ST/TS events, listen for neither */
octeon_i2c_write_int(i2c, TWSI_INT_ST_INT | TWSI_INT_TS_INT);
}

/*
 * Cleanup low-level state & enable high-level controller.
 */
static void octeon_i2c_hlc_enable(struct octeon_i2c *i2c)
{
int try = 0;
u64 val;

if (i2c->hlc_enabled)
return;
i2c->hlc_enabled = true;

while (1) {
val = octeon_i2c_ctl_read(i2c);
if (!(val & (TWSI_CTL_STA | TWSI_CTL_STP)))
break;

/* clear IFLG event */
if (val & TWSI_CTL_IFLG)
octeon_i2c_ctl_write(i2c, TWSI_CTL_ENAB);

if (try++ > 100) {
pr_err("%s: giving up\n", __func__);
break;
}

/* spin until any start/stop has finished */
udelay(10);
}
octeon_i2c_ctl_write(i2c, TWSI_CTL_CE | TWSI_CTL_AAK | TWSI_CTL_ENAB);
}

static void octeon_i2c_hlc_disable(struct octeon_i2c *i2c)
{
if (!i2c->hlc_enabled)
return;

i2c->hlc_enabled = false;
octeon_i2c_ctl_write(i2c, TWSI_CTL_ENAB);
}

/**
 * octeon_i2c_hlc_wait - wait for an HLC operation to complete
 * @i2c: The struct octeon_i2c
 *
 * Returns 0 on success, otherwise -ETIMEDOUT.
 */
static int octeon_i2c_hlc_wait(struct octeon_i2c *i2c)
{
int time_left;

/*
 * Some cn38xx boards don't assert the irq in the interrupt
 * controller. So we must poll for the valid bit change.
 */
if (i2c->broken_irq_mode) {
u64 end = get_jiffies_64() + i2c->adap.timeout;

while (!octeon_i2c_hlc_test_valid(i2c) &&
   time_before64(get_jiffies_64(), end))
usleep_range(I2C_OCTEON_EVENT_WAIT / 2, 
I2C_OCTEON_EVENT_WAIT);

return octeon_i2c_hlc_test_valid(i2c) ? 0 : -ETIMEDOUT;
}

i2c->hlc_int_enable(i2c);
time_left = wait_event_timeout(i2c->queue,
   

[Bug fix] octeon-i2c driver improvement [1/2]

[Zhang, Sean C. (NSB - CN/Hangzhou)]

Hi Jan and David,

For octeon-i2c driver, there only has bus recovery for octeon TWSI low level 
control mode,
no recovery for high level control mode, attached patch is used to extend the 
recovery step
from octeon_i2c_start() to octeon_i2c_xfer() for some case octeon TWSI high 
level control
mode get failure.

Please have time to review it. Thanks.

BR
Sean Zhang


0001-i2c-octeon-driver-improvement.patch
Description: 0001-i2c-octeon-driver-improvement.patch
/*
 * (C) Copyright 2009-2010
 * Nokia Siemens Networks, michael.lawnick@nsn.com
 *
 * Portions Copyright (C) 2010 - 2016 Cavium, Inc.
 *
 * This file contains the shared part of the driver for the i2c adapter in
 * Cavium Networks' OCTEON processors and ThunderX SOCs.
 *
 * 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 
#include 
#include 
#include 
#include 

#include "i2c-octeon-core.h"

/* interrupt service routine */
irqreturn_t octeon_i2c_isr(int irq, void *dev_id)
{
struct octeon_i2c *i2c = dev_id;

i2c->int_disable(i2c);
wake_up(>queue);

return IRQ_HANDLED;
}

static bool octeon_i2c_test_iflg(struct octeon_i2c *i2c)
{
return (octeon_i2c_ctl_read(i2c) & TWSI_CTL_IFLG);
}

/**
 * octeon_i2c_wait - wait for the IFLG to be set
 * @i2c: The struct octeon_i2c
 *
 * Returns 0 on success, otherwise a negative errno.
 */
static int octeon_i2c_wait(struct octeon_i2c *i2c)
{
long time_left;

/*
 * Some chip revisions don't assert the irq in the interrupt
 * controller. So we must poll for the IFLG change.
 */
if (i2c->broken_irq_mode) {
u64 end = get_jiffies_64() + i2c->adap.timeout;

while (!octeon_i2c_test_iflg(i2c) &&
   time_before64(get_jiffies_64(), end))
usleep_range(I2C_OCTEON_EVENT_WAIT / 2, 
I2C_OCTEON_EVENT_WAIT);

return octeon_i2c_test_iflg(i2c) ? 0 : -ETIMEDOUT;
}

i2c->int_enable(i2c);
time_left = wait_event_timeout(i2c->queue, octeon_i2c_test_iflg(i2c),
   i2c->adap.timeout);
i2c->int_disable(i2c);

if (i2c->broken_irq_check && !time_left &&
octeon_i2c_test_iflg(i2c)) {
dev_err(i2c->dev, "broken irq connection detected, switching to 
polling mode.\n");
i2c->broken_irq_mode = true;
return 0;
}

if (!time_left)
return -ETIMEDOUT;

return 0;
}

static bool octeon_i2c_hlc_test_valid(struct octeon_i2c *i2c)
{
return (__raw_readq(i2c->twsi_base + SW_TWSI(i2c)) & SW_TWSI_V) == 0;
}

static void octeon_i2c_hlc_int_clear(struct octeon_i2c *i2c)
{
/* clear ST/TS events, listen for neither */
octeon_i2c_write_int(i2c, TWSI_INT_ST_INT | TWSI_INT_TS_INT);
}

/*
 * Cleanup low-level state & enable high-level controller.
 */
static void octeon_i2c_hlc_enable(struct octeon_i2c *i2c)
{
int try = 0;
u64 val;

if (i2c->hlc_enabled)
return;
i2c->hlc_enabled = true;

while (1) {
val = octeon_i2c_ctl_read(i2c);
if (!(val & (TWSI_CTL_STA | TWSI_CTL_STP)))
break;

/* clear IFLG event */
if (val & TWSI_CTL_IFLG)
octeon_i2c_ctl_write(i2c, TWSI_CTL_ENAB);

if (try++ > 100) {
pr_err("%s: giving up\n", __func__);
break;
}

/* spin until any start/stop has finished */
udelay(10);
}
octeon_i2c_ctl_write(i2c, TWSI_CTL_CE | TWSI_CTL_AAK | TWSI_CTL_ENAB);
}

static void octeon_i2c_hlc_disable(struct octeon_i2c *i2c)
{
if (!i2c->hlc_enabled)
return;

i2c->hlc_enabled = false;
octeon_i2c_ctl_write(i2c, TWSI_CTL_ENAB);
}

/**
 * octeon_i2c_hlc_wait - wait for an HLC operation to complete
 * @i2c: The struct octeon_i2c
 *
 * Returns 0 on success, otherwise -ETIMEDOUT.
 */
static int octeon_i2c_hlc_wait(struct octeon_i2c *i2c)
{
int time_left;

/*
 * Some cn38xx boards don't assert the irq in the interrupt
 * controller. So we must poll for the valid bit change.
 */
if (i2c->broken_irq_mode) {
u64 end = get_jiffies_64() + i2c->adap.timeout;

while (!octeon_i2c_hlc_test_valid(i2c) &&
   time_before64(get_jiffies_64(), end))
usleep_range(I2C_OCTEON_EVENT_WAIT / 2, 
I2C_OCTEON_EVENT_WAIT);

return octeon_i2c_hlc_test_valid(i2c) ? 0 : -ETIMEDOUT;
}

i2c->hlc_int_enable(i2c);
time_left = wait_event_timeout(i2c->queue,
   

[Bug fix] octeon-i2c driver improvement [1/2]

[Zhang, Sean C. (NSB - CN/Hangzhou)]

Hi Jan and David,

For octeon-i2c driver, there only has bus recovery for octeon TWSI low level 
control mode,
no recovery for high level control mode, attached patch is used to extend the 
recovery step
from octeon_i2c_start() to octeon_i2c_xfer() for some case octeon TWSI high 
level control
mode get failure.

Please have time to review it. Thanks.

BR
Sean Zhang


0001-i2c-octeon-driver-improvement.patch
Description: 0001-i2c-octeon-driver-improvement.patch
/*
 * (C) Copyright 2009-2010
 * Nokia Siemens Networks, michael.lawnick@nsn.com
 *
 * Portions Copyright (C) 2010 - 2016 Cavium, Inc.
 *
 * This file contains the shared part of the driver for the i2c adapter in
 * Cavium Networks' OCTEON processors and ThunderX SOCs.
 *
 * 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 
#include 
#include 
#include 
#include 

#include "i2c-octeon-core.h"

/* interrupt service routine */
irqreturn_t octeon_i2c_isr(int irq, void *dev_id)
{
struct octeon_i2c *i2c = dev_id;

i2c->int_disable(i2c);
wake_up(>queue);

return IRQ_HANDLED;
}

static bool octeon_i2c_test_iflg(struct octeon_i2c *i2c)
{
return (octeon_i2c_ctl_read(i2c) & TWSI_CTL_IFLG);
}

/**
 * octeon_i2c_wait - wait for the IFLG to be set
 * @i2c: The struct octeon_i2c
 *
 * Returns 0 on success, otherwise a negative errno.
 */
static int octeon_i2c_wait(struct octeon_i2c *i2c)
{
long time_left;

/*
 * Some chip revisions don't assert the irq in the interrupt
 * controller. So we must poll for the IFLG change.
 */
if (i2c->broken_irq_mode) {
u64 end = get_jiffies_64() + i2c->adap.timeout;

while (!octeon_i2c_test_iflg(i2c) &&
   time_before64(get_jiffies_64(), end))
usleep_range(I2C_OCTEON_EVENT_WAIT / 2, 
I2C_OCTEON_EVENT_WAIT);

return octeon_i2c_test_iflg(i2c) ? 0 : -ETIMEDOUT;
}

i2c->int_enable(i2c);
time_left = wait_event_timeout(i2c->queue, octeon_i2c_test_iflg(i2c),
   i2c->adap.timeout);
i2c->int_disable(i2c);

if (i2c->broken_irq_check && !time_left &&
octeon_i2c_test_iflg(i2c)) {
dev_err(i2c->dev, "broken irq connection detected, switching to 
polling mode.\n");
i2c->broken_irq_mode = true;
return 0;
}

if (!time_left)
return -ETIMEDOUT;

return 0;
}

static bool octeon_i2c_hlc_test_valid(struct octeon_i2c *i2c)
{
return (__raw_readq(i2c->twsi_base + SW_TWSI(i2c)) & SW_TWSI_V) == 0;
}

static void octeon_i2c_hlc_int_clear(struct octeon_i2c *i2c)
{
/* clear ST/TS events, listen for neither */
octeon_i2c_write_int(i2c, TWSI_INT_ST_INT | TWSI_INT_TS_INT);
}

/*
 * Cleanup low-level state & enable high-level controller.
 */
static void octeon_i2c_hlc_enable(struct octeon_i2c *i2c)
{
int try = 0;
u64 val;

if (i2c->hlc_enabled)
return;
i2c->hlc_enabled = true;

while (1) {
val = octeon_i2c_ctl_read(i2c);
if (!(val & (TWSI_CTL_STA | TWSI_CTL_STP)))
break;

/* clear IFLG event */
if (val & TWSI_CTL_IFLG)
octeon_i2c_ctl_write(i2c, TWSI_CTL_ENAB);

if (try++ > 100) {
pr_err("%s: giving up\n", __func__);
break;
}

/* spin until any start/stop has finished */
udelay(10);
}
octeon_i2c_ctl_write(i2c, TWSI_CTL_CE | TWSI_CTL_AAK | TWSI_CTL_ENAB);
}

static void octeon_i2c_hlc_disable(struct octeon_i2c *i2c)
{
if (!i2c->hlc_enabled)
return;

i2c->hlc_enabled = false;
octeon_i2c_ctl_write(i2c, TWSI_CTL_ENAB);
}

/**
 * octeon_i2c_hlc_wait - wait for an HLC operation to complete
 * @i2c: The struct octeon_i2c
 *
 * Returns 0 on success, otherwise -ETIMEDOUT.
 */
static int octeon_i2c_hlc_wait(struct octeon_i2c *i2c)
{
int time_left;

/*
 * Some cn38xx boards don't assert the irq in the interrupt
 * controller. So we must poll for the valid bit change.
 */
if (i2c->broken_irq_mode) {
u64 end = get_jiffies_64() + i2c->adap.timeout;

while (!octeon_i2c_hlc_test_valid(i2c) &&
   time_before64(get_jiffies_64(), end))
usleep_range(I2C_OCTEON_EVENT_WAIT / 2, 
I2C_OCTEON_EVENT_WAIT);

return octeon_i2c_hlc_test_valid(i2c) ? 0 : -ETIMEDOUT;
}

i2c->hlc_int_enable(i2c);
time_left = wait_event_timeout(i2c->queue,
   

RE: [Bug fix] octeon-i2c driver updates

[Zhang, Sean C. (NSB - CN/Hangzhou)]

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:dda...@caviumnetworks.com] 
Sent: Wednesday, December 06, 2017 12:43 AM
To: Zhang, Sean C. (NSB - CN/Hangzhou) <sean.c.zh...@nokia-sbell.com>; Jan 
Glauber <jan.glau...@caviumnetworks.com>; david.da...@cavium.com
Cc: w...@the-dreams.de; linux-...@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.glau...@caviumnetworks.com]
> Sent: Friday, December 01, 2017 6:07 PM
> To: Zhang, Sean C. (NSB - CN/Hangzhou) <sean.c.zh...@nokia-sbell.com>
> Cc: david.da...@cavium.com; w...@the-dreams.de; 
> linux-...@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 +, 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.glau...@caviumnetworks.com>
>> Cc: david.da...@cavium.com; w...@the-dreams.de; 
>> linux-...@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 tha

RE: [Bug fix] octeon-i2c driver updates

[Zhang, Sean C. (NSB - CN/Hangzhou)]

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:dda...@caviumnetworks.com] 
Sent: Wednesday, December 06, 2017 12:43 AM
To: Zhang, Sean C. (NSB - CN/Hangzhou) ; Jan 
Glauber ; david.da...@cavium.com
Cc: w...@the-dreams.de; linux-...@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.glau...@caviumnetworks.com]
> Sent: Friday, December 01, 2017 6:07 PM
> To: Zhang, Sean C. (NSB - CN/Hangzhou) 
> Cc: david.da...@cavium.com; w...@the-dreams.de; 
> linux-...@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' 
>> Cc: david.da...@cavium.com; w...@the-dreams.de; 
>> linux-...@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 
>

RE: [Bug fix] octeon-i2c driver updates

[Zhang, Sean C. (NSB - CN/Hangzhou)]

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.

BR,
Sean Zhang


-Original Message-
From: Jan Glauber [mailto:jan.glau...@caviumnetworks.com] 
Sent: Friday, December 01, 2017 6:07 PM
To: Zhang, Sean C. (NSB - CN/Hangzhou) <sean.c.zh...@nokia-sbell.com>
Cc: david.da...@cavium.com; w...@the-dreams.de; linux-...@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.glau...@caviumnetworks.com>
> Cc: david.da...@cavium.com; w...@the-dreams.de; linux-...@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.glau...@caviumnetworks.com] 
> Sent: Friday, November 24, 2017 9:10 PM
> To: Zhang, Sean C. (NSB - CN/Hangzhou) <sean.c.zh...@nokia-sbell.com>
> Cc: david.da...@cavium.com; w...@the-dreams.de; linux-...@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 +, 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 t

RE: [Bug fix] octeon-i2c driver updates

[Zhang, Sean C. (NSB - CN/Hangzhou)]

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.

BR,
Sean Zhang


-Original Message-
From: Jan Glauber [mailto:jan.glau...@caviumnetworks.com] 
Sent: Friday, December 01, 2017 6:07 PM
To: Zhang, Sean C. (NSB - CN/Hangzhou) 
Cc: david.da...@cavium.com; w...@the-dreams.de; linux-...@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' 
> Cc: david.da...@cavium.com; w...@the-dreams.de; linux-...@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.glau...@caviumnetworks.com] 
> Sent: Friday, November 24, 2017 9:10 PM
> To: Zhang, Sean C. (NSB - CN/Hangzhou) 
> Cc: david.da...@cavium.com; w...@the-dreams.de; linux-...@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 +, 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 removi

RE: [Bug fix] octeon-i2c driver updates

[Zhang, Sean C. (NSB - CN/Hangzhou)]

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.glau...@caviumnetworks.com>
Cc: david.da...@cavium.com; w...@the-dreams.de; linux-...@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.glau...@caviumnetworks.com] 
Sent: Friday, November 24, 2017 9:10 PM
To: Zhang, Sean C. (NSB - CN/Hangzhou) <sean.c.zh...@nokia-sbell.com>
Cc: david.da...@cavium.com; w...@the-dreams.de; linux-...@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.zh...@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.zh...@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
>

RE: [Bug fix] octeon-i2c driver updates

[Zhang, Sean C. (NSB - CN/Hangzhou)]

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' 
Cc: david.da...@cavium.com; w...@the-dreams.de; linux-...@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.glau...@caviumnetworks.com] 
Sent: Friday, November 24, 2017 9:10 PM
To: Zhang, Sean C. (NSB - CN/Hangzhou) 
Cc: david.da...@cavium.com; w...@the-dreams.de; linux-...@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 +, 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 
> 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 
> ---
>  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 recov

RE: [Bug fix] octeon-i2c driver updates

[Zhang, Sean C. (NSB - CN/Hangzhou)]

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.glau...@caviumnetworks.com] 
Sent: Friday, November 24, 2017 9:10 PM
To: Zhang, Sean C. (NSB - CN/Hangzhou) <sean.c.zh...@nokia-sbell.com>
Cc: david.da...@cavium.com; w...@the-dreams.de; linux-...@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 +, 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.zh...@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.zh...@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

RE: [Bug fix] octeon-i2c driver updates

[Zhang, Sean C. (NSB - CN/Hangzhou)]

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.glau...@caviumnetworks.com] 
Sent: Friday, November 24, 2017 9:10 PM
To: Zhang, Sean C. (NSB - CN/Hangzhou) 
Cc: david.da...@cavium.com; w...@the-dreams.de; linux-...@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 +, 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 
> 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 
> ---
>  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-pla

[Bug fix] octeon-i2c driver updates

[Zhang, Sean C. (NSB - CN/Hangzhou)]

Dear Maintainer,

For octeon TWSI controller, I found below two cases, maybe can be improved.


>From 09d9f0ce658d7f6a50d1af352dde619c29bc8bcf Mon Sep 17 00:00:00 2001
From: hgt463 
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 
---
 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(>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(>adap);
if (result < 0)
goto out;


Attached patch for you review. Thanks in advance.

BR,
Sean Zhang


0001-Driver-updates.patch
Description: 0001-Driver-updates.patch

[Bug fix] octeon-i2c driver updates

[Zhang, Sean C. (NSB - CN/Hangzhou)]

Dear Maintainer,

For octeon TWSI controller, I found below two cases, maybe can be improved.


>From 09d9f0ce658d7f6a50d1af352dde619c29bc8bcf Mon Sep 17 00:00:00 2001
From: hgt463 
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 
---
 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(>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(>adap);
if (result < 0)
goto out;


Attached patch for you review. Thanks in advance.

BR,
Sean Zhang


0001-Driver-updates.patch
Description: 0001-Driver-updates.patch