diff mbox

[v6,07/19] i2c: octeon: Use i2c recovery framework

Message ID ab24627b5928f23967633243639b431d2113dbe1.1460387640.git.jglauber@cavium.com
State Superseded
Headers show

Commit Message

Jan Glauber April 11, 2016, 3:28 p.m. UTC 
Switch to the i2c bus recovery framework using generic SCL recovery.
If this fails try to reset the hardware. The recovery is triggered
during START on timeout of the interrupt or failure to reach
the START / repeated-START condition.

The START function is moved to xfer and while at it:
- removed xfer debug message (i2c core already provides debugging)
- removed length is zero check

Signed-off-by: Jan Glauber <jglauber@cavium.com>
---
 drivers/i2c/busses/i2c-octeon.c | 178 +++++++++++++++++++++++++---------------
 1 file changed, 111 insertions(+), 67 deletions(-)

Comments

Wolfram Sang April 20, 2016, 9:31 p.m. UTC | #1 
On Mon, Apr 11, 2016 at 05:28:38PM +0200, Jan Glauber wrote:
> Switch to the i2c bus recovery framework using generic SCL recovery.
> If this fails try to reset the hardware. The recovery is triggered
> during START on timeout of the interrupt or failure to reach
> the START / repeated-START condition.
> 
> The START function is moved to xfer and while at it:
> - removed xfer debug message (i2c core already provides debugging)
> - removed length is zero check
> 
> Signed-off-by: Jan Glauber <jglauber@cavium.com>
> ---
>  drivers/i2c/busses/i2c-octeon.c | 178 +++++++++++++++++++++++++---------------
>  1 file changed, 111 insertions(+), 67 deletions(-)

Interesting, it got larger...

> 
> +/**
> + * octeon_i2c_write_int - read the TWSI_INT register

read_int

> +	int ret, retries = 2;

I don't think 'retries' makes sense here. On failure, you return
-EAGAIN, so the core will retry 'adapter->retries' times anyhow.

> -	if (length < 1)
> -		return -EINVAL;

So, the adapter support 0-length messages now? Or why was it there? I
have the feeling this is a seperate patch.

> +static void octeon_i2c_prepare_recovery(struct i2c_adapter *adap)
> +{
> +	struct octeon_i2c *i2c = i2c_get_adapdata(adap);
> +
> +	/*
> +	 * The stop resets the state machine, does not _transmit_ STOP unless
> +	 * engine was active.
> +	 */
> +	octeon_i2c_stop(i2c);
> +
> +	octeon_i2c_write_int(i2c, 0);

Maybe a comment why the delay?

> +	udelay(5);
> +}
Jan Glauber April 21, 2016, 1:08 p.m. UTC | #2 
On Wed, Apr 20, 2016 at 11:31:21PM +0200, Wolfram Sang wrote:
> On Mon, Apr 11, 2016 at 05:28:38PM +0200, Jan Glauber wrote:
> > Switch to the i2c bus recovery framework using generic SCL recovery.
> > If this fails try to reset the hardware. The recovery is triggered
> > during START on timeout of the interrupt or failure to reach
> > the START / repeated-START condition.
> > 
> > The START function is moved to xfer and while at it:
> > - removed xfer debug message (i2c core already provides debugging)
> > - removed length is zero check
> > 
> > Signed-off-by: Jan Glauber <jglauber@cavium.com>
> > ---
> >  drivers/i2c/busses/i2c-octeon.c | 178 +++++++++++++++++++++++++---------------
> >  1 file changed, 111 insertions(+), 67 deletions(-)
> 
> Interesting, it got larger...
> 
> > 
> > +/**
> > + * octeon_i2c_write_int - read the TWSI_INT register
> 
> read_int

OK

> > +	int ret, retries = 2;
> 
> I don't think 'retries' makes sense here. On failure, you return
> -EAGAIN, so the core will retry 'adapter->retries' times anyhow.

OK. I'll remove the retry looping and call recovery once
if the START condition is not reached. If recovery succeeds
I'll return -EAGAIN, otherwise the error code.

> > -	if (length < 1)
> > -		return -EINVAL;
> 
> So, the adapter support 0-length messages now? Or why was it there? I
> have the feeling this is a seperate patch.

I assumed this check was bogus and there are no valid 0-length
messages...
If 0-length messages can indeed happen I'll keep this check (but why
would the i2c core not check for that in a central place and return
an error before calling xfer() ?).

> > +static void octeon_i2c_prepare_recovery(struct i2c_adapter *adap)
> > +{
> > +	struct octeon_i2c *i2c = i2c_get_adapdata(adap);
> > +
> > +	/*
> > +	 * The stop resets the state machine, does not _transmit_ STOP unless
> > +	 * engine was active.
> > +	 */
> > +	octeon_i2c_stop(i2c);
> > +
> > +	octeon_i2c_write_int(i2c, 0);
> 
> Maybe a comment why the delay?

Later patch "Flush TWSI writes with readback" adds synchronization after
octeon_i2c_write_int(), so the delays in prepare/unprepare_recovery can
go away.

> > +	udelay(5);
> > +}


--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wolfram Sang April 21, 2016, 1:54 p.m. UTC | #3 
> I assumed this check was bogus and there are no valid 0-length
> messages...

They are valid (check SMBUS_QUICK), but not every controller can handle
them correctly. Your driver has SMBUS_QUICK enabled, so this is a
contradiction to the check above where it rejects it.

So, it looks like it needs to be tested again (and documented this
time). If the HW can't do it, the FUNC bit for QUICK needs to be masked
out. If it can do SMBUS_QUICK, the check can probably go away.
Jan Glauber April 21, 2016, 5:51 p.m. UTC | #4 
On Thu, Apr 21, 2016 at 03:54:50PM +0200, Wolfram Sang wrote:
> 
> > I assumed this check was bogus and there are no valid 0-length
> > messages...
> 
> They are valid (check SMBUS_QUICK), but not every controller can handle
> them correctly. Your driver has SMBUS_QUICK enabled, so this is a
> contradiction to the check above where it rejects it.

Oops, this mismatch dates back to the inital driver code. From the
documentation I would say SMBUS_QUICK is not supported, although nothing
terrible happens in the write case.

> So, it looks like it needs to be tested again (and documented this
> time). If the HW can't do it, the FUNC bit for QUICK needs to be masked
> out. If it can do SMBUS_QUICK, the check can probably go away.
> 

I would like to disable SMBUS_QUICK. It never worked for the read case.
Could we break something by disabling the quick-write case or is
the quick-write emulated by a larger write if the feature bit is not
set?
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wolfram Sang April 21, 2016, 9:33 p.m. UTC | #5 
> I would like to disable SMBUS_QUICK. It never worked for the read case.

Fine with me.

> Could we break something by disabling the quick-write case or is
> the quick-write emulated by a larger write if the feature bit is not
> set?

No emulation. It is simply not supported then. It is actually quite
dangerous to simulate it with regular write.
diff mbox

Patch

diff --git a/drivers/i2c/busses/i2c-octeon.c b/drivers/i2c/busses/i2c-octeon.c
index 483c8a3..88c9686 100644
--- a/drivers/i2c/busses/i2c-octeon.c
+++ b/drivers/i2c/busses/i2c-octeon.c
@@ -90,6 +90,8 @@ 
 #define TWSI_INT_CORE_EN	BIT_ULL(6)
 #define TWSI_INT_SDA_OVR	BIT_ULL(8)
 #define TWSI_INT_SCL_OVR	BIT_ULL(9)
+#define TWSI_INT_SDA		BIT_ULL(10)
+#define TWSI_INT_SCL		BIT_ULL(11)
 
 struct octeon_i2c {
 	wait_queue_head_t queue;
@@ -152,6 +154,18 @@  static u8 octeon_i2c_reg_read(struct octeon_i2c *i2c, u64 eop_reg)
 #define octeon_i2c_stat_read(i2c)					\
 	octeon_i2c_reg_read(i2c, SW_TWSI_EOP_TWSI_STAT)
 
+
+/**
+ * octeon_i2c_write_int - read the TWSI_INT register
+ * @i2c: The struct octeon_i2c
+ *
+ * Returns the value of the register.
+ */
+static u64 octeon_i2c_read_int(struct octeon_i2c *i2c)
+{
+	return __raw_readq(i2c->twsi_base + TWSI_INT);
+}
+
 /**
  * octeon_i2c_write_int - write the TWSI_INT register
  * @i2c: The struct octeon_i2c
@@ -182,33 +196,6 @@  static void octeon_i2c_int_disable(struct octeon_i2c *i2c)
 	octeon_i2c_write_int(i2c, 0);
 }
 
-/**
- * octeon_i2c_unblock - unblock the bus
- * @i2c: The struct octeon_i2c
- *
- * If there was a reset while a device was driving 0 to bus, bus is blocked.
- * We toggle it free manually by some clock cycles and send a stop.
- */
-static void octeon_i2c_unblock(struct octeon_i2c *i2c)
-{
-	int i;
-
-	dev_dbg(i2c->dev, "%s\n", __func__);
-
-	for (i = 0; i < 9; i++) {
-		octeon_i2c_write_int(i2c, 0);
-		udelay(5);
-		octeon_i2c_write_int(i2c, TWSI_INT_SCL_OVR);
-		udelay(5);
-	}
-	/* hand-crank a STOP */
-	octeon_i2c_write_int(i2c, TWSI_INT_SDA_OVR | TWSI_INT_SCL_OVR);
-	udelay(5);
-	octeon_i2c_write_int(i2c, TWSI_INT_SDA_OVR);
-	udelay(5);
-	octeon_i2c_write_int(i2c, 0);
-}
-
 /* interrupt service routine */
 static irqreturn_t octeon_i2c_isr(int irq, void *dev_id)
 {
@@ -369,6 +356,17 @@  static int octeon_i2c_init_lowlevel(struct octeon_i2c *i2c)
 	return -EIO;
 }
 
+static int octeon_i2c_recovery(struct octeon_i2c *i2c)
+{
+	int ret;
+
+	ret = i2c_recover_bus(&i2c->adap);
+	if (ret)
+		/* recover failed, try hardware re-init */
+		ret = octeon_i2c_init_lowlevel(i2c);
+	return ret;
+}
+
 /**
  * octeon_i2c_start - send START to the bus
  * @i2c: The struct octeon_i2c
@@ -377,34 +375,34 @@  static int octeon_i2c_init_lowlevel(struct octeon_i2c *i2c)
  */
 static int octeon_i2c_start(struct octeon_i2c *i2c)
 {
-	int result;
-	u8 data;
-
-	octeon_i2c_ctl_write(i2c, TWSI_CTL_ENAB | TWSI_CTL_STA);
-
-	result = octeon_i2c_wait(i2c);
-	if (result) {
-		if (octeon_i2c_stat_read(i2c) == STAT_IDLE) {
-			/*
-			 * Controller refused to send start flag May
-			 * be a client is holding SDA low - let's try
-			 * to free it.
-			 */
-			octeon_i2c_unblock(i2c);
-			octeon_i2c_ctl_write(i2c, TWSI_CTL_ENAB | TWSI_CTL_STA);
-			result = octeon_i2c_wait(i2c);
+	int ret, retries = 2;
+	u8 stat;
+
+retry:
+	if (retries > 0) {
+		octeon_i2c_ctl_write(i2c, TWSI_CTL_ENAB | TWSI_CTL_STA);
+		ret = octeon_i2c_wait(i2c);
+		if (ret) {
+			/* timeout error, try to recover */
+			ret = octeon_i2c_recovery(i2c);
+			if (ret)
+				return ret;
+			retries--;
+			goto retry;
 		}
-		if (result)
-			return result;
-	}
+	} else
+		return -EAGAIN;
 
-	data = octeon_i2c_stat_read(i2c);
-	if ((data != STAT_START) && (data != STAT_RSTART)) {
-		dev_err(i2c->dev, "%s: bad status (0x%x)\n", __func__, data);
-		return -EIO;
-	}
+	stat = octeon_i2c_stat_read(i2c);
+	if (stat == STAT_START || stat == STAT_REP_START)
+		/* START successful, bail out */
+		return 0;
 
-	return 0;
+	/* START failed, try to recover */
+	ret = octeon_i2c_recovery(i2c);
+	if (!ret && retries--)
+		goto retry;
+	return ret;
 }
 
 /* send STOP to the bus */
@@ -429,10 +427,6 @@  static int octeon_i2c_write(struct octeon_i2c *i2c, int target,
 {
 	int i, result;
 
-	result = octeon_i2c_start(i2c);
-	if (result)
-		return result;
-
 	octeon_i2c_data_write(i2c, target << 1);
 	octeon_i2c_ctl_write(i2c, TWSI_CTL_ENAB);
 
@@ -474,13 +468,6 @@  static int octeon_i2c_read(struct octeon_i2c *i2c, int target,
 	int i, result, length = *rlength;
 	bool final_read = false;
 
-	if (length < 1)
-		return -EINVAL;
-
-	result = octeon_i2c_start(i2c);
-	if (result)
-		return result;
-
 	octeon_i2c_data_write(i2c, (target << 1) | 1);
 	octeon_i2c_ctl_write(i2c, TWSI_CTL_ENAB);
 
@@ -544,10 +531,10 @@  static int octeon_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 	for (i = 0; ret == 0 && i < num; i++) {
 		struct i2c_msg *pmsg = &msgs[i];
 
-		dev_dbg(i2c->dev,
-			"Doing %s %d byte(s) to/from 0x%02x - %d of %d messages\n",
-			 pmsg->flags & I2C_M_RD ? "read" : "write",
-			 pmsg->len, pmsg->addr, i + 1, num);
+		ret = octeon_i2c_start(i2c);
+		if (ret)
+			return ret;
+
 		if (pmsg->flags & I2C_M_RD)
 			ret = octeon_i2c_read(i2c, pmsg->addr, pmsg->buf,
 					      &pmsg->len, pmsg->flags & I2C_M_RECV_LEN);
@@ -560,6 +547,62 @@  static int octeon_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 	return (ret != 0) ? ret : num;
 }
 
+static int octeon_i2c_get_scl(struct i2c_adapter *adap)
+{
+	struct octeon_i2c *i2c = i2c_get_adapdata(adap);
+	u64 state;
+
+	state = octeon_i2c_read_int(i2c);
+	return state & TWSI_INT_SCL;
+}
+
+static void octeon_i2c_set_scl(struct i2c_adapter *adap, int val)
+{
+	struct octeon_i2c *i2c = i2c_get_adapdata(adap);
+
+	octeon_i2c_write_int(i2c, TWSI_INT_SCL_OVR);
+}
+
+static int octeon_i2c_get_sda(struct i2c_adapter *adap)
+{
+	struct octeon_i2c *i2c = i2c_get_adapdata(adap);
+	u64 state;
+
+	state = octeon_i2c_read_int(i2c);
+	return state & TWSI_INT_SDA;
+}
+
+static void octeon_i2c_prepare_recovery(struct i2c_adapter *adap)
+{
+	struct octeon_i2c *i2c = i2c_get_adapdata(adap);
+
+	/*
+	 * The stop resets the state machine, does not _transmit_ STOP unless
+	 * engine was active.
+	 */
+	octeon_i2c_stop(i2c);
+
+	octeon_i2c_write_int(i2c, 0);
+	udelay(5);
+}
+
+static void octeon_i2c_unprepare_recovery(struct i2c_adapter *adap)
+{
+	struct octeon_i2c *i2c = i2c_get_adapdata(adap);
+
+	octeon_i2c_write_int(i2c, 0);
+	udelay(5);
+}
+
+static struct i2c_bus_recovery_info octeon_i2c_recovery_info = {
+	.recover_bus = i2c_generic_scl_recovery,
+	.get_scl = octeon_i2c_get_scl,
+	.set_scl = octeon_i2c_set_scl,
+	.get_sda = octeon_i2c_get_sda,
+	.prepare_recovery = octeon_i2c_prepare_recovery,
+	.unprepare_recovery = octeon_i2c_unprepare_recovery,
+};
+
 static u32 octeon_i2c_functionality(struct i2c_adapter *adap)
 {
 	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL |
@@ -640,6 +683,7 @@  static int octeon_i2c_probe(struct platform_device *pdev)
 	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);