i2c:ocores: fixes and polling mechanism

i2c:ocores: fixes and polling mechanism

[Federico Vaga]

The first two patches fix what I believe are bugs.

The third patch add a polling mechanism for those systems where interrupts
are not available.

All these patches have been tested on a system without interrupt, this
means that I used my third patch to validate also the other two.
I would be nice if someone can run verify this also on other system,
perhaps with interrupts. If you consider it a useful information, I'm not
using devicetree for this installation.

i2c:ocores: fixes and polling mechanism

[Federico Vaga]

The first two patches fix what I believe are bugs.

The third patch add a polling mechanism for those systems where interrupts
are not available.

All these patches have been tested on a system without interrupt, this
means that I used my third patch to validate also the other two.
I would be nice if someone can run verify this also on other system,
perhaps with interrupts. If you consider it a useful information, I'm not
using devicetree for this installation.

[PATCH 1/3] i2c:ocores: stop transfer on timeout

[Federico Vaga]

Detecting a timeout is ok, but we also need to assert a STOP command on
the bus in order to prevent it from generating interrupts when there are
no on going transfers.

Example: very long transmission.

1. ocores_xfer: START a transfer
2. ocores_isr : handle byte by byte the transfer
3. ocores_xfer: goes in timeout [[bugfix here]]
4. ocores_xfer: return to I2C subsystem and to the I2C driver
5. I2C driver : it may clean up the i2c_msg memory
6. ocores_isr : receives another interrupt (pending bytes to be
                transferred) but the i2c_msg memory is invalid now

So, since the transfer was too long, we have to detect the timeout and
STOP the transfer.

Another point is that we have a critical region here. When handling the
timeout condition we may have a running IRQ handler. For this reason I
introduce a spinlock. In the IRQ handler we can just use trylock because
if the lock is taken is because we are in timeout, so there is no need to
process the IRQ.

Signed-off-by: Federico Vaga <federico.vaga@cern.ch>
---
 drivers/i2c/busses/i2c-ocores.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index 88444ef74943..98c0ef74882b 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -25,6 +25,7 @@
 #include <linux/slab.h>
 #include <linux/io.h>
 #include <linux/log2.h>
+#include <linux/spinlock.h>
 
 struct ocores_i2c {
 	void __iomem *base;
@@ -36,6 +37,7 @@ struct ocores_i2c {
 	int pos;
 	int nmsgs;
 	int state; /* see STATE_ */
+	spinlock_t xfer_lock;
 	struct clk *clk;
 	int ip_clock_khz;
 	int bus_clock_khz;
@@ -207,15 +209,30 @@ static void ocores_process(struct ocores_i2c *i2c)
 static irqreturn_t ocores_isr(int irq, void *dev_id)
 {
 	struct ocores_i2c *i2c = dev_id;
+	unsigned long flags;
+	int ret;
+
+	/*
+	 * We need to protect i2c against a timeout event (see ocores_xfer())
+	 * If we cannot take this lock, it means that we are already in
+	 * timeout, so it's pointless to handle this interrupt because we
+	 * are going to abort the current transfer.
+	 */
+	ret = spin_trylock_irqsave(&i2c->xfer_lock, flags);
+	if (!ret)
+		return IRQ_HANDLED;
 
 	ocores_process(i2c);
 
+	spin_unlock_irqrestore(&i2c->xfer_lock, flags);
+
 	return IRQ_HANDLED;
 }
 
 static int ocores_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 {
 	struct ocores_i2c *i2c = i2c_get_adapdata(adap);
+	unsigned long flags;
 
 	i2c->msg = msgs;
 	i2c->pos = 0;
@@ -226,10 +243,15 @@ static int ocores_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 	oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_START);
 
 	if (wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
-			       (i2c->state == STATE_DONE), HZ))
+			       (i2c->state == STATE_DONE), HZ)) {
 		return (i2c->state == STATE_DONE) ? num : -EIO;
-	else
+	} else {
+		spin_lock_irqsave(&i2c->xfer_lock, flags);
+		i2c->state = STATE_ERROR;
+		oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP);
+		spin_unlock_irqrestore(&i2c->xfer_lock, flags);
 		return -ETIMEDOUT;
+	}
 }
 
 static int ocores_init(struct device *dev, struct ocores_i2c *i2c)
@@ -422,6 +444,8 @@ static int ocores_i2c_probe(struct platform_device *pdev)
 	if (!i2c)
 		return -ENOMEM;
 
+	spin_lock_init(&i2c->xfer_lock);
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	i2c->base = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(i2c->base))
-- 
2.15.0

[PATCH 1/3] i2c:ocores: stop transfer on timeout

[Federico Vaga]

Detecting a timeout is ok, but we also need to assert a STOP command on
the bus in order to prevent it from generating interrupts when there are
no on going transfers.

Example: very long transmission.

1. ocores_xfer: START a transfer
2. ocores_isr : handle byte by byte the transfer
3. ocores_xfer: goes in timeout [[bugfix here]]
4. ocores_xfer: return to I2C subsystem and to the I2C driver
5. I2C driver : it may clean up the i2c_msg memory
6. ocores_isr : receives another interrupt (pending bytes to be
                transferred) but the i2c_msg memory is invalid now

So, since the transfer was too long, we have to detect the timeout and
STOP the transfer.

Another point is that we have a critical region here. When handling the
timeout condition we may have a running IRQ handler. For this reason I
introduce a spinlock. In the IRQ handler we can just use trylock because
if the lock is taken is because we are in timeout, so there is no need to
process the IRQ.

Signed-off-by: Federico Vaga <federico.vaga@cern.ch>
---
 drivers/i2c/busses/i2c-ocores.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index 88444ef74943..98c0ef74882b 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -25,6 +25,7 @@
 #include <linux/slab.h>
 #include <linux/io.h>
 #include <linux/log2.h>
+#include <linux/spinlock.h>
 
 struct ocores_i2c {
 	void __iomem *base;
@@ -36,6 +37,7 @@ struct ocores_i2c {
 	int pos;
 	int nmsgs;
 	int state; /* see STATE_ */
+	spinlock_t xfer_lock;
 	struct clk *clk;
 	int ip_clock_khz;
 	int bus_clock_khz;
@@ -207,15 +209,30 @@ static void ocores_process(struct ocores_i2c *i2c)
 static irqreturn_t ocores_isr(int irq, void *dev_id)
 {
 	struct ocores_i2c *i2c = dev_id;
+	unsigned long flags;
+	int ret;
+
+	/*
+	 * We need to protect i2c against a timeout event (see ocores_xfer())
+	 * If we cannot take this lock, it means that we are already in
+	 * timeout, so it's pointless to handle this interrupt because we
+	 * are going to abort the current transfer.
+	 */
+	ret = spin_trylock_irqsave(&i2c->xfer_lock, flags);
+	if (!ret)
+		return IRQ_HANDLED;
 
 	ocores_process(i2c);
 
+	spin_unlock_irqrestore(&i2c->xfer_lock, flags);
+
 	return IRQ_HANDLED;
 }
 
 static int ocores_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 {
 	struct ocores_i2c *i2c = i2c_get_adapdata(adap);
+	unsigned long flags;
 
 	i2c->msg = msgs;
 	i2c->pos = 0;
@@ -226,10 +243,15 @@ static int ocores_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 	oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_START);
 
 	if (wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
-			       (i2c->state == STATE_DONE), HZ))
+			       (i2c->state == STATE_DONE), HZ)) {
 		return (i2c->state == STATE_DONE) ? num : -EIO;
-	else
+	} else {
+		spin_lock_irqsave(&i2c->xfer_lock, flags);
+		i2c->state = STATE_ERROR;
+		oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP);
+		spin_unlock_irqrestore(&i2c->xfer_lock, flags);
 		return -ETIMEDOUT;
+	}
 }
 
 static int ocores_init(struct device *dev, struct ocores_i2c *i2c)
@@ -422,6 +444,8 @@ static int ocores_i2c_probe(struct platform_device *pdev)
 	if (!i2c)
 		return -ENOMEM;
 
+	spin_lock_init(&i2c->xfer_lock);
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	i2c->base = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(i2c->base))
-- 
2.15.0

[PATCH 2/3] i2c:ocores: do not handle IRQ if IF is not set

[Federico Vaga]

If the Interrupt Flag (IF) is not set, we should not handle the IRQ:
- the line can be shared with other devices
- it can be a spurious interrupt

To avoid reading twice the status register, the ocores_process() function
expects it to be read by the caller.

Signed-off-by: Federico Vaga <federico.vaga@cern.ch>
---
 drivers/i2c/busses/i2c-ocores.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index 98c0ef74882b..274d6eb22a2c 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -139,10 +139,9 @@ static inline u8 oc_getreg(struct ocores_i2c *i2c, int reg)
 	return i2c->getreg(i2c, reg);
 }
 
-static void ocores_process(struct ocores_i2c *i2c)
+static void ocores_process(struct ocores_i2c *i2c, u8 stat)
 {
 	struct i2c_msg *msg = i2c->msg;
-	u8 stat = oc_getreg(i2c, OCI2C_STATUS);
 
 	if ((i2c->state == STATE_DONE) || (i2c->state == STATE_ERROR)) {
 		/* stop has been sent */
@@ -209,9 +208,13 @@ static void ocores_process(struct ocores_i2c *i2c)
 static irqreturn_t ocores_isr(int irq, void *dev_id)
 {
 	struct ocores_i2c *i2c = dev_id;
+	u8 stat = oc_getreg(i2c, OCI2C_STATUS);
 	unsigned long flags;
 	int ret;
 
+	if (!(stat & OCI2C_STAT_IF))
+		return IRQ_NONE;
+
 	/*
 	 * We need to protect i2c against a timeout event (see ocores_xfer())
 	 * If we cannot take this lock, it means that we are already in
@@ -222,7 +225,7 @@ static irqreturn_t ocores_isr(int irq, void *dev_id)
 	if (!ret)
 		return IRQ_HANDLED;
 
-	ocores_process(i2c);
+	ocores_process(i2c, stat);
 
 	spin_unlock_irqrestore(&i2c->xfer_lock, flags);
 
-- 
2.15.0

[PATCH 2/3] i2c:ocores: do not handle IRQ if IF is not set

[Federico Vaga]

If the Interrupt Flag (IF) is not set, we should not handle the IRQ:
- the line can be shared with other devices
- it can be a spurious interrupt

To avoid reading twice the status register, the ocores_process() function
expects it to be read by the caller.

Signed-off-by: Federico Vaga <federico.vaga@cern.ch>
---
 drivers/i2c/busses/i2c-ocores.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index 98c0ef74882b..274d6eb22a2c 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -139,10 +139,9 @@ static inline u8 oc_getreg(struct ocores_i2c *i2c, int reg)
 	return i2c->getreg(i2c, reg);
 }
 
-static void ocores_process(struct ocores_i2c *i2c)
+static void ocores_process(struct ocores_i2c *i2c, u8 stat)
 {
 	struct i2c_msg *msg = i2c->msg;
-	u8 stat = oc_getreg(i2c, OCI2C_STATUS);
 
 	if ((i2c->state == STATE_DONE) || (i2c->state == STATE_ERROR)) {
 		/* stop has been sent */
@@ -209,9 +208,13 @@ static void ocores_process(struct ocores_i2c *i2c)
 static irqreturn_t ocores_isr(int irq, void *dev_id)
 {
 	struct ocores_i2c *i2c = dev_id;
+	u8 stat = oc_getreg(i2c, OCI2C_STATUS);
 	unsigned long flags;
 	int ret;
 
+	if (!(stat & OCI2C_STAT_IF))
+		return IRQ_NONE;
+
 	/*
 	 * We need to protect i2c against a timeout event (see ocores_xfer())
 	 * If we cannot take this lock, it means that we are already in
@@ -222,7 +225,7 @@ static irqreturn_t ocores_isr(int irq, void *dev_id)
 	if (!ret)
 		return IRQ_HANDLED;
 
-	ocores_process(i2c);
+	ocores_process(i2c, stat);
 
 	spin_unlock_irqrestore(&i2c->xfer_lock, flags);
 
-- 
2.15.0

[PATCH 3/3] i2c:ocores: add polling interface

[Federico Vaga]

This driver assumes that an interrupt line is always available for
the I2C master. This is not always the case and this patch adds support
for a polling version based on workqueue.

Signed-off-by: Federico Vaga <federico.vaga@cern.ch>
---
 drivers/i2c/busses/i2c-ocores.c | 94 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 79 insertions(+), 15 deletions(-)

diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index 274d6eb22a2c..0dad1a512ef5 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -13,6 +13,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -26,14 +27,19 @@
 #include <linux/io.h>
 #include <linux/log2.h>
 #include <linux/spinlock.h>
+#include <linux/workqueue.h>
+
+#define OCORES_FLAG_POLL BIT(0)
 
 struct ocores_i2c {
 	void __iomem *base;
 	u32 reg_shift;
 	u32 reg_io_width;
+	unsigned long flags;
 	wait_queue_head_t wait;
 	struct i2c_adapter adap;
 	struct i2c_msg *msg;
+	struct work_struct xfer_work;
 	int pos;
 	int nmsgs;
 	int state; /* see STATE_ */
@@ -166,8 +172,9 @@ static void ocores_process(struct ocores_i2c *i2c, u8 stat)
 			oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP);
 			return;
 		}
-	} else
+	} else {
 		msg->buf[i2c->pos++] = oc_getreg(i2c, OCI2C_DATA);
+	}
 
 	/* end of msg? */
 	if (i2c->pos == msg->len) {
@@ -232,6 +239,50 @@ static irqreturn_t ocores_isr(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+
+/**
+ * It waits until is possible to process some data
+ * @i2c: ocores I2C device instance
+ *
+ * This is used when the device is in polling mode (interrupts disabled).
+ * It sleeps for the time necessary to send 8bits (one transfer over
+ * the I2C bus), then it permanently ping the ip-core until is possible
+ * to process data. The idea is that we sleep for most of the time at the
+ * beginning because we are sure that the ip-core is not ready yet.
+ */
+static void ocores_poll_wait(struct ocores_i2c *i2c)
+{
+	int sleep_min = (8/i2c->bus_clock_khz) * 1000; /* us for 8bits */
+	u8 loop_on;
+
+	usleep_range(sleep_min, sleep_min + 10);
+	if (i2c->state == STATE_DONE || i2c->state == STATE_ERROR)
+		loop_on = OCI2C_STAT_BUSY;
+	else
+		loop_on = OCI2C_STAT_TIP;
+	while (oc_getreg(i2c, OCI2C_STATUS) & loop_on)
+		;
+}
+
+
+/**
+ * It implements the polling logic
+ * @work: work instance descriptor
+ *
+ * Here we try to re-use as much as possible from the IRQ logic
+ */
+static void ocores_work(struct work_struct *work)
+{
+	struct ocores_i2c *i2c = container_of(work,
+					      struct ocores_i2c, xfer_work);
+	irqreturn_t ret;
+
+	do {
+		ocores_poll_wait(i2c);
+		ret = ocores_isr(-1, i2c);
+	} while (ret != IRQ_NONE);
+}
+
 static int ocores_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 {
 	struct ocores_i2c *i2c = i2c_get_adapdata(adap);
@@ -245,6 +296,9 @@ static int ocores_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 	oc_setreg(i2c, OCI2C_DATA, i2c_8bit_addr_from_msg(i2c->msg));
 	oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_START);
 
+	if (i2c->flags & OCORES_FLAG_POLL)
+		schedule_work(&i2c->xfer_work);
+
 	if (wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
 			       (i2c->state == STATE_DONE), HZ)) {
 		return (i2c->state == STATE_DONE) ? num : -EIO;
@@ -264,7 +318,8 @@ static int ocores_init(struct device *dev, struct ocores_i2c *i2c)
 	u8 ctrl = oc_getreg(i2c, OCI2C_CONTROL);
 
 	/* make sure the device is disabled */
-	oc_setreg(i2c, OCI2C_CONTROL, ctrl & ~(OCI2C_CTRL_EN|OCI2C_CTRL_IEN));
+	ctrl &= ~(OCI2C_CTRL_EN|OCI2C_CTRL_IEN);
+	oc_setreg(i2c, OCI2C_CONTROL, ctrl);
 
 	prescale = (i2c->ip_clock_khz / (5 * i2c->bus_clock_khz)) - 1;
 	prescale = clamp(prescale, 0, 0xffff);
@@ -277,12 +332,16 @@ static int ocores_init(struct device *dev, struct ocores_i2c *i2c)
 		return -EINVAL;
 	}
 
+
 	oc_setreg(i2c, OCI2C_PRELOW, prescale & 0xff);
 	oc_setreg(i2c, OCI2C_PREHIGH, prescale >> 8);
 
 	/* Init the device */
 	oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_IACK);
-	oc_setreg(i2c, OCI2C_CONTROL, ctrl | OCI2C_CTRL_IEN | OCI2C_CTRL_EN);
+	ctrl |= OCI2C_CTRL_EN;
+	if (i2c->flags != OCORES_FLAG_POLL)
+		ctrl |= OCI2C_CTRL_IEN;
+	oc_setreg(i2c, OCI2C_CONTROL, ctrl);
 
 	return 0;
 }
@@ -439,10 +498,6 @@ static int ocores_i2c_probe(struct platform_device *pdev)
 	int ret;
 	int i;
 
-	irq = platform_get_irq(pdev, 0);
-	if (irq < 0)
-		return irq;
-
 	i2c = devm_kzalloc(&pdev->dev, sizeof(*i2c), GFP_KERNEL);
 	if (!i2c)
 		return -ENOMEM;
@@ -497,18 +552,25 @@ static int ocores_i2c_probe(struct platform_device *pdev)
 		}
 	}
 
+	init_waitqueue_head(&i2c->wait);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq == -ENXIO) {
+		i2c->flags |= OCORES_FLAG_POLL;
+		INIT_WORK(&i2c->xfer_work, ocores_work);
+	} else {
+		ret = devm_request_irq(&pdev->dev, irq, ocores_isr, 0,
+				       pdev->name, i2c);
+		if (ret) {
+			dev_err(&pdev->dev, "Cannot claim IRQ\n");
+			goto err_clk;
+		}
+	}
+
 	ret = ocores_init(&pdev->dev, i2c);
 	if (ret)
 		goto err_clk;
 
-	init_waitqueue_head(&i2c->wait);
-	ret = devm_request_irq(&pdev->dev, irq, ocores_isr, 0,
-			       pdev->name, i2c);
-	if (ret) {
-		dev_err(&pdev->dev, "Cannot claim IRQ\n");
-		goto err_clk;
-	}
-
 	/* hook up driver to tree */
 	platform_set_drvdata(pdev, i2c);
 	i2c->adap = ocores_adapter;
@@ -538,6 +600,8 @@ static int ocores_i2c_remove(struct platform_device *pdev)
 {
 	struct ocores_i2c *i2c = platform_get_drvdata(pdev);
 
+	flush_scheduled_work();
+
 	/* disable i2c logic */
 	oc_setreg(i2c, OCI2C_CONTROL, oc_getreg(i2c, OCI2C_CONTROL)
 		  & ~(OCI2C_CTRL_EN|OCI2C_CTRL_IEN));
-- 
2.15.0

[PATCH 3/3] i2c:ocores: add polling interface

[Federico Vaga]

This driver assumes that an interrupt line is always available for
the I2C master. This is not always the case and this patch adds support
for a polling version based on workqueue.

Signed-off-by: Federico Vaga <federico.vaga@cern.ch>
---
 drivers/i2c/busses/i2c-ocores.c | 94 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 79 insertions(+), 15 deletions(-)

diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index 274d6eb22a2c..0dad1a512ef5 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -13,6 +13,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -26,14 +27,19 @@
 #include <linux/io.h>
 #include <linux/log2.h>
 #include <linux/spinlock.h>
+#include <linux/workqueue.h>
+
+#define OCORES_FLAG_POLL BIT(0)
 
 struct ocores_i2c {
 	void __iomem *base;
 	u32 reg_shift;
 	u32 reg_io_width;
+	unsigned long flags;
 	wait_queue_head_t wait;
 	struct i2c_adapter adap;
 	struct i2c_msg *msg;
+	struct work_struct xfer_work;
 	int pos;
 	int nmsgs;
 	int state; /* see STATE_ */
@@ -166,8 +172,9 @@ static void ocores_process(struct ocores_i2c *i2c, u8 stat)
 			oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP);
 			return;
 		}
-	} else
+	} else {
 		msg->buf[i2c->pos++] = oc_getreg(i2c, OCI2C_DATA);
+	}
 
 	/* end of msg? */
 	if (i2c->pos == msg->len) {
@@ -232,6 +239,50 @@ static irqreturn_t ocores_isr(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+
+/**
+ * It waits until is possible to process some data
+ * @i2c: ocores I2C device instance
+ *
+ * This is used when the device is in polling mode (interrupts disabled).
+ * It sleeps for the time necessary to send 8bits (one transfer over
+ * the I2C bus), then it permanently ping the ip-core until is possible
+ * to process data. The idea is that we sleep for most of the time at the
+ * beginning because we are sure that the ip-core is not ready yet.
+ */
+static void ocores_poll_wait(struct ocores_i2c *i2c)
+{
+	int sleep_min = (8/i2c->bus_clock_khz) * 1000; /* us for 8bits */
+	u8 loop_on;
+
+	usleep_range(sleep_min, sleep_min + 10);
+	if (i2c->state == STATE_DONE || i2c->state == STATE_ERROR)
+		loop_on = OCI2C_STAT_BUSY;
+	else
+		loop_on = OCI2C_STAT_TIP;
+	while (oc_getreg(i2c, OCI2C_STATUS) & loop_on)
+		;
+}
+
+
+/**
+ * It implements the polling logic
+ * @work: work instance descriptor
+ *
+ * Here we try to re-use as much as possible from the IRQ logic
+ */
+static void ocores_work(struct work_struct *work)
+{
+	struct ocores_i2c *i2c = container_of(work,
+					      struct ocores_i2c, xfer_work);
+	irqreturn_t ret;
+
+	do {
+		ocores_poll_wait(i2c);
+		ret = ocores_isr(-1, i2c);
+	} while (ret != IRQ_NONE);
+}
+
 static int ocores_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 {
 	struct ocores_i2c *i2c = i2c_get_adapdata(adap);
@@ -245,6 +296,9 @@ static int ocores_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 	oc_setreg(i2c, OCI2C_DATA, i2c_8bit_addr_from_msg(i2c->msg));
 	oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_START);
 
+	if (i2c->flags & OCORES_FLAG_POLL)
+		schedule_work(&i2c->xfer_work);
+
 	if (wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
 			       (i2c->state == STATE_DONE), HZ)) {
 		return (i2c->state == STATE_DONE) ? num : -EIO;
@@ -264,7 +318,8 @@ static int ocores_init(struct device *dev, struct ocores_i2c *i2c)
 	u8 ctrl = oc_getreg(i2c, OCI2C_CONTROL);
 
 	/* make sure the device is disabled */
-	oc_setreg(i2c, OCI2C_CONTROL, ctrl & ~(OCI2C_CTRL_EN|OCI2C_CTRL_IEN));
+	ctrl &= ~(OCI2C_CTRL_EN|OCI2C_CTRL_IEN);
+	oc_setreg(i2c, OCI2C_CONTROL, ctrl);
 
 	prescale = (i2c->ip_clock_khz / (5 * i2c->bus_clock_khz)) - 1;
 	prescale = clamp(prescale, 0, 0xffff);
@@ -277,12 +332,16 @@ static int ocores_init(struct device *dev, struct ocores_i2c *i2c)
 		return -EINVAL;
 	}
 
+
 	oc_setreg(i2c, OCI2C_PRELOW, prescale & 0xff);
 	oc_setreg(i2c, OCI2C_PREHIGH, prescale >> 8);
 
 	/* Init the device */
 	oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_IACK);
-	oc_setreg(i2c, OCI2C_CONTROL, ctrl | OCI2C_CTRL_IEN | OCI2C_CTRL_EN);
+	ctrl |= OCI2C_CTRL_EN;
+	if (i2c->flags != OCORES_FLAG_POLL)
+		ctrl |= OCI2C_CTRL_IEN;
+	oc_setreg(i2c, OCI2C_CONTROL, ctrl);
 
 	return 0;
 }
@@ -439,10 +498,6 @@ static int ocores_i2c_probe(struct platform_device *pdev)
 	int ret;
 	int i;
 
-	irq = platform_get_irq(pdev, 0);
-	if (irq < 0)
-		return irq;
-
 	i2c = devm_kzalloc(&pdev->dev, sizeof(*i2c), GFP_KERNEL);
 	if (!i2c)
 		return -ENOMEM;
@@ -497,18 +552,25 @@ static int ocores_i2c_probe(struct platform_device *pdev)
 		}
 	}
 
+	init_waitqueue_head(&i2c->wait);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq == -ENXIO) {
+		i2c->flags |= OCORES_FLAG_POLL;
+		INIT_WORK(&i2c->xfer_work, ocores_work);
+	} else {
+		ret = devm_request_irq(&pdev->dev, irq, ocores_isr, 0,
+				       pdev->name, i2c);
+		if (ret) {
+			dev_err(&pdev->dev, "Cannot claim IRQ\n");
+			goto err_clk;
+		}
+	}
+
 	ret = ocores_init(&pdev->dev, i2c);
 	if (ret)
 		goto err_clk;
 
-	init_waitqueue_head(&i2c->wait);
-	ret = devm_request_irq(&pdev->dev, irq, ocores_isr, 0,
-			       pdev->name, i2c);
-	if (ret) {
-		dev_err(&pdev->dev, "Cannot claim IRQ\n");
-		goto err_clk;
-	}
-
 	/* hook up driver to tree */
 	platform_set_drvdata(pdev, i2c);
 	i2c->adap = ocores_adapter;
@@ -538,6 +600,8 @@ static int ocores_i2c_remove(struct platform_device *pdev)
 {
 	struct ocores_i2c *i2c = platform_get_drvdata(pdev);
 
+	flush_scheduled_work();
+
 	/* disable i2c logic */
 	oc_setreg(i2c, OCI2C_CONTROL, oc_getreg(i2c, OCI2C_CONTROL)
 		  & ~(OCI2C_CTRL_EN|OCI2C_CTRL_IEN));
-- 
2.15.0

Re: i2c:ocores: fixes and polling mechanism

[Federico Vaga]

Hello,

sorry to disturb you all but after one month and a half I never received 
any comment about this patch set and I fear it ended up in a forgotten 
corner. I would like to know if someone is considering it or not.

Thanks :)

On Monday, June 25, 2018 6:13:00 PM CEST Federico Vaga wrote:
> The first two patches fix what I believe are bugs.
> 
> The third patch add a polling mechanism for those systems where
> interrupts are not available.
> 
> All these patches have been tested on a system without interrupt, this
> means that I used my third patch to validate also the other two.
> I would be nice if someone can run verify this also on other system,
> perhaps with interrupts. If you consider it a useful information, I'm not
> using devicetree for this installation.


-- 
Federico Vaga
[BE-CO-HT]

Re: i2c:ocores: fixes and polling mechanism

[Federico Vaga]

Hello,

sorry to disturb you all but after one month and a half I never received 
any comment about this patch set and I fear it ended up in a forgotten 
corner. I would like to know if someone is considering it or not.

Thanks :)

On Monday, June 25, 2018 6:13:00 PM CEST Federico Vaga wrote:
> The first two patches fix what I believe are bugs.
> 
> The third patch add a polling mechanism for those systems where
> interrupts are not available.
> 
> All these patches have been tested on a system without interrupt, this
> means that I used my third patch to validate also the other two.
> I would be nice if someone can run verify this also on other system,
> perhaps with interrupts. If you consider it a useful information, I'm not
> using devicetree for this installation.


-- 
Federico Vaga
[BE-CO-HT]

Re: i2c:ocores: fixes and polling mechanism

[Wolfram Sang]

> sorry to disturb you all but after one month and a half I never received 
> any comment about this patch set and I fear it ended up in a forgotten 
> corner. I would like to know if someone is considering it or not.

Adding Peter to CC using his latest EMail address.

Peter, you said you wanted to update MAINTAINERs with the new address?

Re: i2c:ocores: fixes and polling mechanism

[Peter Korsgaard]

>>>>> "Wolfram" == Wolfram Sang <wsa@the-dreams.de> writes:

 >> sorry to disturb you all but after one month and a half I never received 
 >> any comment about this patch set and I fear it ended up in a forgotten 
 >> corner. I would like to know if someone is considering it or not.

 > Adding Peter to CC using his latest EMail address.

Thanks! I'll take a look at the patches now.

> Peter, you said you wanted to update MAINTAINERs with the new address?

Sorry, I forgot about it when I left on holidays. Will send now.

-- 
Bye, Peter Korsgaard

Re: i2c:ocores: fixes and polling mechanism

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 58 bytes --]


> Thanks! I'll take a look at the patches now.

Ping :)


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: i2c:ocores: fixes and polling mechanism

[Peter Korsgaard]

>>>>> "Wolfram" == Wolfram Sang <wsa@the-dreams.de> writes:

 >> Thanks! I'll take a look at the patches now.

 > Ping :)

I'm terribly sorry. I didn't manage to review before leaving on
travel. I'm back next week and then I'll review, OK?

-- 
Bye, Peter Korsgaard

Re: i2c:ocores: fixes and polling mechanism

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 161 bytes --]


> I'm terribly sorry. I didn't manage to review before leaving on
> travel. I'm back next week and then I'll review, OK?

Sure thing, thanks for the heads up!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/3] i2c:ocores: stop transfer on timeout

[Peter Korsgaard]

On Mon, Jun 25, 2018 at 6:14 PM Federico Vaga <federico.vaga@cern.ch> wrote:

Hi, and sorry for the slow response.

> Detecting a timeout is ok, but we also need to assert a STOP command on
> the bus in order to prevent it from generating interrupts when there are
> no on going transfers.
>
> Example: very long transmission.
>
> 1. ocores_xfer: START a transfer
> 2. ocores_isr : handle byte by byte the transfer
> 3. ocores_xfer: goes in timeout [[bugfix here]]
> 4. ocores_xfer: return to I2C subsystem and to the I2C driver
> 5. I2C driver : it may clean up the i2c_msg memory
> 6. ocores_isr : receives another interrupt (pending bytes to be
>                 transferred) but the i2c_msg memory is invalid now
>
> So, since the transfer was too long, we have to detect the timeout and
> STOP the transfer.
>
> Another point is that we have a critical region here. When handling the
> timeout condition we may have a running IRQ handler. For this reason I
> introduce a spinlock. In the IRQ handler we can just use trylock because
> if the lock is taken is because we are in timeout, so there is no need to
> process the IRQ.
>
> Signed-off-by: Federico Vaga <federico.vaga@cern.ch>
> ---
>  drivers/i2c/busses/i2c-ocores.c | 28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
> index 88444ef74943..98c0ef74882b 100644
> --- a/drivers/i2c/busses/i2c-ocores.c
> +++ b/drivers/i2c/busses/i2c-ocores.c
> @@ -25,6 +25,7 @@
>  #include <linux/slab.h>
>  #include <linux/io.h>
>  #include <linux/log2.h>
> +#include <linux/spinlock.h>
>
>  struct ocores_i2c {
>         void __iomem *base;
> @@ -36,6 +37,7 @@ struct ocores_i2c {
>         int pos;
>         int nmsgs;
>         int state; /* see STATE_ */
> +       spinlock_t xfer_lock;
>         struct clk *clk;
>         int ip_clock_khz;
>         int bus_clock_khz;
> @@ -207,15 +209,30 @@ static void ocores_process(struct ocores_i2c *i2c)
>  static irqreturn_t ocores_isr(int irq, void *dev_id)
>  {
>         struct ocores_i2c *i2c = dev_id;
> +       unsigned long flags;
> +       int ret;
> +
> +       /*
> +        * We need to protect i2c against a timeout event (see ocores_xfer())
> +        * If we cannot take this lock, it means that we are already in
> +        * timeout, so it's pointless to handle this interrupt because we
> +        * are going to abort the current transfer.
> +        */
> +       ret = spin_trylock_irqsave(&i2c->xfer_lock, flags);

This is very old code, so I might be missing something - But I still
don't quite understand this trylock logic. If we end up here with the
lock taken, then that must mean that we are on SMP system. We still
need to ack the interrupt, so just spinning until the other CPU
releases the lock seems more sensible?

-- 
Bye, Peter Korsgaard

Re: [PATCH 2/3] i2c:ocores: do not handle IRQ if IF is not set

[Peter Korsgaard]

On Mon, Jun 25, 2018 at 6:14 PM Federico Vaga <federico.vaga@cern.ch> wrote:
>
> If the Interrupt Flag (IF) is not set, we should not handle the IRQ:
> - the line can be shared with other devices
> - it can be a spurious interrupt
>
> To avoid reading twice the status register, the ocores_process() function
> expects it to be read by the caller.
>
> Signed-off-by: Federico Vaga <federico.vaga@cern.ch>

Looks good.

Acked-by: Peter Korsgaard <peter@korsgaard.com>

-- 
Bye, Peter Korsgaard

Re: [PATCH 3/3] i2c:ocores: add polling interface

[Peter Korsgaard]

On Mon, Jun 25, 2018 at 6:14 PM Federico Vaga <federico.vaga@cern.ch> wrote:
>
> This driver assumes that an interrupt line is always available for
> the I2C master. This is not always the case and this patch adds support
> for a polling version based on workqueue.

It probably makes sense to make it the switch between irq/irqless mode
dynamic to support the upcoming master_xfer_irqless logic.

> Signed-off-by: Federico Vaga <federico.vaga@cern.ch>
> ---
>  drivers/i2c/busses/i2c-ocores.c | 94 ++++++++++++++++++++++++++++++++++-------
>  1 file changed, 79 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
> index 274d6eb22a2c..0dad1a512ef5 100644
> --- a/drivers/i2c/busses/i2c-ocores.c
> +++ b/drivers/i2c/busses/i2c-ocores.c
> @@ -13,6 +13,7 @@
>   */
>
>  #include <linux/clk.h>
> +#include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> @@ -26,14 +27,19 @@
>  #include <linux/io.h>
>  #include <linux/log2.h>
>  #include <linux/spinlock.h>
> +#include <linux/workqueue.h>
> +
> +#define OCORES_FLAG_POLL BIT(0)
>
>  struct ocores_i2c {
>         void __iomem *base;
>         u32 reg_shift;
>         u32 reg_io_width;
> +       unsigned long flags;
>         wait_queue_head_t wait;
>         struct i2c_adapter adap;
>         struct i2c_msg *msg;
> +       struct work_struct xfer_work;
>         int pos;
>         int nmsgs;
>         int state; /* see STATE_ */
> @@ -166,8 +172,9 @@ static void ocores_process(struct ocores_i2c *i2c, u8 stat)
>                         oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP);
>                         return;
>                 }
> -       } else
> +       } else {
>                 msg->buf[i2c->pos++] = oc_getreg(i2c, OCI2C_DATA);
> +       }

This looks unrelated to $SUBJECT.

>
>         /* end of msg? */
>         if (i2c->pos == msg->len) {
> @@ -232,6 +239,50 @@ static irqreturn_t ocores_isr(int irq, void *dev_id)
>         return IRQ_HANDLED;
>  }
>
> +
> +/**
> + * It waits until is possible to process some data

Please don't use "It waits ..", but rather "wait until ..". Same for
the other function comments.


> + * @i2c: ocores I2C device instance
> + *
> + * This is used when the device is in polling mode (interrupts disabled).
> + * It sleeps for the time necessary to send 8bits (one transfer over
> + * the I2C bus), then it permanently ping the ip-core until is possible
> + * to process data. The idea is that we sleep for most of the time at the
> + * beginning because we are sure that the ip-core is not ready yet.
> + */
> +static void ocores_poll_wait(struct ocores_i2c *i2c)
> +{
> +       int sleep_min = (8/i2c->bus_clock_khz) * 1000; /* us for 8bits */
> +       u8 loop_on;
> +
> +       usleep_range(sleep_min, sleep_min + 10);

Where does this 10 come from?

> +       if (i2c->state == STATE_DONE || i2c->state == STATE_ERROR)
> +               loop_on = OCI2C_STAT_BUSY;
> +       else
> +               loop_on = OCI2C_STAT_TIP;
> +       while (oc_getreg(i2c, OCI2C_STATUS) & loop_on)
> +               ;

How would an I2C transmission timeout be handled here?

> +}
> +
> +
> +/**
> + * It implements the polling logic
> + * @work: work instance descriptor
> + *
> + * Here we try to re-use as much as possible from the IRQ logic
> + */
> +static void ocores_work(struct work_struct *work)
> +{
> +       struct ocores_i2c *i2c = container_of(work,
> +                                             struct ocores_i2c, xfer_work);
> +       irqreturn_t ret;
> +
> +       do {
> +               ocores_poll_wait(i2c);
> +               ret = ocores_isr(-1, i2c);
> +       } while (ret != IRQ_NONE);

Might as well drop the negation, E.G. while (ret == IRQ_HANDLED);

> +}
> +
>  static int ocores_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>  {
>         struct ocores_i2c *i2c = i2c_get_adapdata(adap);
> @@ -245,6 +296,9 @@ static int ocores_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>         oc_setreg(i2c, OCI2C_DATA, i2c_8bit_addr_from_msg(i2c->msg));
>         oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_START);
>
> +       if (i2c->flags & OCORES_FLAG_POLL)
> +               schedule_work(&i2c->xfer_work);
> +
>         if (wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
>                                (i2c->state == STATE_DONE), HZ)) {
>                 return (i2c->state == STATE_DONE) ? num : -EIO;
> @@ -264,7 +318,8 @@ static int ocores_init(struct device *dev, struct ocores_i2c *i2c)
>         u8 ctrl = oc_getreg(i2c, OCI2C_CONTROL);
>
>         /* make sure the device is disabled */
> -       oc_setreg(i2c, OCI2C_CONTROL, ctrl & ~(OCI2C_CTRL_EN|OCI2C_CTRL_IEN));
> +       ctrl &= ~(OCI2C_CTRL_EN|OCI2C_CTRL_IEN);
> +       oc_setreg(i2c, OCI2C_CONTROL, ctrl);

This looks unrelated to $SUBJECT

>
>         prescale = (i2c->ip_clock_khz / (5 * i2c->bus_clock_khz)) - 1;
>         prescale = clamp(prescale, 0, 0xffff);
> @@ -277,12 +332,16 @@ static int ocores_init(struct device *dev, struct ocores_i2c *i2c)
>                 return -EINVAL;
>         }
>
> +

Here as well.

> @@ -538,6 +600,8 @@ static int ocores_i2c_remove(struct platform_device *pdev)
>  {
>         struct ocores_i2c *i2c = platform_get_drvdata(pdev);
>
> +       flush_scheduled_work();
> +

Why not cancel_work_sync(&i2c->xfer_work)?

-- 
Bye, Peter Korsgaard

Re: [PATCH 3/3] i2c:ocores: add polling interface

[Federico Vaga]

On Sunday, October 21, 2018 4:39:07 PM CEST Peter Korsgaard wrote:
> On Mon, Jun 25, 2018 at 6:14 PM Federico Vaga <federico.vaga@cern.ch> wrote:
> > This driver assumes that an interrupt line is always available for
> > the I2C master. This is not always the case and this patch adds support
> > for a polling version based on workqueue.
> 
> It probably makes sense to make it the switch between irq/irqless mode
> dynamic to support the upcoming master_xfer_irqless logic.
> 
> > Signed-off-by: Federico Vaga <federico.vaga@cern.ch>
> > ---
> > 
> >  drivers/i2c/busses/i2c-ocores.c | 94
> >  ++++++++++++++++++++++++++++++++++------- 1 file changed, 79
> >  insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-ocores.c
> > b/drivers/i2c/busses/i2c-ocores.c index 274d6eb22a2c..0dad1a512ef5 100644
> > --- a/drivers/i2c/busses/i2c-ocores.c
> > +++ b/drivers/i2c/busses/i2c-ocores.c
> > @@ -13,6 +13,7 @@
> > 
> >   */
> >  
> >  #include <linux/clk.h>
> > 
> > +#include <linux/delay.h>
> > 
> >  #include <linux/err.h>
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> > 
> > @@ -26,14 +27,19 @@
> > 
> >  #include <linux/io.h>
> >  #include <linux/log2.h>
> >  #include <linux/spinlock.h>
> > 
> > +#include <linux/workqueue.h>
> > +
> > +#define OCORES_FLAG_POLL BIT(0)
> > 
> >  struct ocores_i2c {
> >  
> >         void __iomem *base;
> >         u32 reg_shift;
> >         u32 reg_io_width;
> > 
> > +       unsigned long flags;
> > 
> >         wait_queue_head_t wait;
> >         struct i2c_adapter adap;
> >         struct i2c_msg *msg;
> > 
> > +       struct work_struct xfer_work;
> > 
> >         int pos;
> >         int nmsgs;
> >         int state; /* see STATE_ */
> > 
> > @@ -166,8 +172,9 @@ static void ocores_process(struct ocores_i2c *i2c, u8
> > stat)> 
> >                         oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP);
> >                         return;
> >                 
> >                 }
> > 
> > -       } else
> > +       } else {
> > 
> >                 msg->buf[i2c->pos++] = oc_getreg(i2c, OCI2C_DATA);
> > 
> > +       }
> 
> This looks unrelated to $SUBJECT.

Do you prefer a different patch just for styling?

> 
> >         /* end of msg? */
> >         if (i2c->pos == msg->len) {
> > 
> > @@ -232,6 +239,50 @@ static irqreturn_t ocores_isr(int irq, void *dev_id)
> > 
> >         return IRQ_HANDLED;
> >  
> >  }
> > 
> > +
> > +/**
> > + * It waits until is possible to process some data
> 
> Please don't use "It waits ..", but rather "wait until ..". Same for
> the other function comments.

ok

> > + * @i2c: ocores I2C device instance
> > + *
> > + * This is used when the device is in polling mode (interrupts disabled).
> > + * It sleeps for the time necessary to send 8bits (one transfer over
> > + * the I2C bus), then it permanently ping the ip-core until is possible
> > + * to process data. The idea is that we sleep for most of the time at the
> > + * beginning because we are sure that the ip-core is not ready yet.
> > + */
> > +static void ocores_poll_wait(struct ocores_i2c *i2c)
> > +{
> > +       int sleep_min = (8/i2c->bus_clock_khz) * 1000; /* us for 8bits */
> > +       u8 loop_on;
> > +
> > +       usleep_range(sleep_min, sleep_min + 10);
> 
> Where does this 10 come from?

It's true, it's just a random number. It can be zero as well, and we ask the 
system to just sleep for that amount of time. 

(1) usleep_range(sleep_min, sleep_min);

I noticed that it is a common practice to just put numbers that sounds 
correct, indeed there are many random numbers (not commented at least, so they 
are random numbers for the reader) in drivers/i2c/busses when they use this 
function.

This magic number can be also something like:
 
(2) usleep_range(sleep_min, sleep_min * 1.10);

to give a 10% (again random choice) extra margin before starting to actively 
poll.

But I agree that random numbers are not good. So I'm ok with option (1). I did 
not try it, but I think is fine to give a zero delta (delta=max-min=0).

> 
> > +       if (i2c->state == STATE_DONE || i2c->state == STATE_ERROR)
> > +               loop_on = OCI2C_STAT_BUSY;
> > +       else
> > +               loop_on = OCI2C_STAT_TIP;
> > +       while (oc_getreg(i2c, OCI2C_STATUS) & loop_on)
> > +               ;
> 
> How would an I2C transmission timeout be handled here?

There is the assumption that the hardware is alive and what we read from 
oc_getreg() is correct. With this assumption, when there is a timeout this 
will happen:
1. STOP command (previous patch)
2. both TIP and BUSY will become zero at some point and we get out from the 
loop

I can see now that there are cases when it may loop forever: for example if 
the device is broken and it does answer always with 0xFFFF: we should not 
break the host as well :)

I can fix this.
 
> > +}
> > +
> > +
> > +/**
> > + * It implements the polling logic
> > + * @work: work instance descriptor
> > + *
> > + * Here we try to re-use as much as possible from the IRQ logic
> > + */
> > +static void ocores_work(struct work_struct *work)
> > +{
> > +       struct ocores_i2c *i2c = container_of(work,
> > +                                             struct ocores_i2c,
> > xfer_work); +       irqreturn_t ret;
> > +
> > +       do {
> > +               ocores_poll_wait(i2c);
> > +               ret = ocores_isr(-1, i2c);
> > +       } while (ret != IRQ_NONE);
> 
> Might as well drop the negation, E.G. while (ret == IRQ_HANDLED);

ok

> > +}
> > +
> > 
> >  static int ocores_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> >  int num) {
> >  
> >         struct ocores_i2c *i2c = i2c_get_adapdata(adap);
> > 
> > @@ -245,6 +296,9 @@ static int ocores_xfer(struct i2c_adapter *adap,
> > struct i2c_msg *msgs, int num)> 
> >         oc_setreg(i2c, OCI2C_DATA, i2c_8bit_addr_from_msg(i2c->msg));
> >         oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_START);
> > 
> > +       if (i2c->flags & OCORES_FLAG_POLL)
> > +               schedule_work(&i2c->xfer_work);
> > +
> > 
> >         if (wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
> >         
> >                                (i2c->state == STATE_DONE), HZ)) {
> >                 
> >                 return (i2c->state == STATE_DONE) ? num : -EIO;
> > 
> > @@ -264,7 +318,8 @@ static int ocores_init(struct device *dev, struct
> > ocores_i2c *i2c)> 
> >         u8 ctrl = oc_getreg(i2c, OCI2C_CONTROL);
> >         
> >         /* make sure the device is disabled */
> > 
> > -       oc_setreg(i2c, OCI2C_CONTROL, ctrl &
> > ~(OCI2C_CTRL_EN|OCI2C_CTRL_IEN));
> > +       ctrl &= ~(OCI2C_CTRL_EN|OCI2C_CTRL_IEN);
> > +       oc_setreg(i2c, OCI2C_CONTROL, ctrl);
> 
> This looks unrelated to $SUBJECT
>
> 
> >         prescale = (i2c->ip_clock_khz / (5 * i2c->bus_clock_khz)) - 1;
> >         prescale = clamp(prescale, 0, 0xffff);
> > 
> > @@ -277,12 +332,16 @@ static int ocores_init(struct device *dev, struct
> > ocores_i2c *i2c)> 
> >                 return -EINVAL;
> >         
> >         }
> > 
> > +
> 
> Here as well.
>
> 
> > @@ -538,6 +600,8 @@ static int ocores_i2c_remove(struct platform_device
> > *pdev)> 
> >  {
> >  
> >         struct ocores_i2c *i2c = platform_get_drvdata(pdev);
> > 
> > +       flush_scheduled_work();
> > +
> 
> Why not cancel_work_sync(&i2c->xfer_work)?

you are right!

Re: [PATCH 3/3] i2c:ocores: add polling interface

[Federico Vaga]

On Sunday, October 21, 2018 4:39:07 PM CEST Peter Korsgaard wrote:
> On Mon, Jun 25, 2018 at 6:14 PM Federico Vaga <federico.vaga@cern.ch> wrote:
> > This driver assumes that an interrupt line is always available for
> > the I2C master. This is not always the case and this patch adds support
> > for a polling version based on workqueue.
> 
> It probably makes sense to make it the switch between irq/irqless mode
> dynamic to support the upcoming master_xfer_irqless logic.
> 
> > Signed-off-by: Federico Vaga <federico.vaga@cern.ch>
> > ---
> > 
> >  drivers/i2c/busses/i2c-ocores.c | 94
> >  ++++++++++++++++++++++++++++++++++------- 1 file changed, 79
> >  insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-ocores.c
> > b/drivers/i2c/busses/i2c-ocores.c index 274d6eb22a2c..0dad1a512ef5 100644
> > --- a/drivers/i2c/busses/i2c-ocores.c
> > +++ b/drivers/i2c/busses/i2c-ocores.c
> > @@ -13,6 +13,7 @@
> > 
> >   */
> >  
> >  #include <linux/clk.h>
> > 
> > +#include <linux/delay.h>
> > 
> >  #include <linux/err.h>
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> > 
> > @@ -26,14 +27,19 @@
> > 
> >  #include <linux/io.h>
> >  #include <linux/log2.h>
> >  #include <linux/spinlock.h>
> > 
> > +#include <linux/workqueue.h>
> > +
> > +#define OCORES_FLAG_POLL BIT(0)
> > 
> >  struct ocores_i2c {
> >  
> >         void __iomem *base;
> >         u32 reg_shift;
> >         u32 reg_io_width;
> > 
> > +       unsigned long flags;
> > 
> >         wait_queue_head_t wait;
> >         struct i2c_adapter adap;
> >         struct i2c_msg *msg;
> > 
> > +       struct work_struct xfer_work;
> > 
> >         int pos;
> >         int nmsgs;
> >         int state; /* see STATE_ */
> > 
> > @@ -166,8 +172,9 @@ static void ocores_process(struct ocores_i2c *i2c, u8
> > stat)> 
> >                         oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP);
> >                         return;
> >                 
> >                 }
> > 
> > -       } else
> > +       } else {
> > 
> >                 msg->buf[i2c->pos++] = oc_getreg(i2c, OCI2C_DATA);
> > 
> > +       }
> 
> This looks unrelated to $SUBJECT.

Do you prefer a different patch just for styling?

> 
> >         /* end of msg? */
> >         if (i2c->pos == msg->len) {
> > 
> > @@ -232,6 +239,50 @@ static irqreturn_t ocores_isr(int irq, void *dev_id)
> > 
> >         return IRQ_HANDLED;
> >  
> >  }
> > 
> > +
> > +/**
> > + * It waits until is possible to process some data
> 
> Please don't use "It waits ..", but rather "wait until ..". Same for
> the other function comments.

ok

> > + * @i2c: ocores I2C device instance
> > + *
> > + * This is used when the device is in polling mode (interrupts disabled).
> > + * It sleeps for the time necessary to send 8bits (one transfer over
> > + * the I2C bus), then it permanently ping the ip-core until is possible
> > + * to process data. The idea is that we sleep for most of the time at the
> > + * beginning because we are sure that the ip-core is not ready yet.
> > + */
> > +static void ocores_poll_wait(struct ocores_i2c *i2c)
> > +{
> > +       int sleep_min = (8/i2c->bus_clock_khz) * 1000; /* us for 8bits */
> > +       u8 loop_on;
> > +
> > +       usleep_range(sleep_min, sleep_min + 10);
> 
> Where does this 10 come from?

It's true, it's just a random number. It can be zero as well, and we ask the 
system to just sleep for that amount of time. 

(1) usleep_range(sleep_min, sleep_min);

I noticed that it is a common practice to just put numbers that sounds 
correct, indeed there are many random numbers (not commented at least, so they 
are random numbers for the reader) in drivers/i2c/busses when they use this 
function.

This magic number can be also something like:
 
(2) usleep_range(sleep_min, sleep_min * 1.10);

to give a 10% (again random choice) extra margin before starting to actively 
poll.

But I agree that random numbers are not good. So I'm ok with option (1). I did 
not try it, but I think is fine to give a zero delta (delta=max-min=0).

> 
> > +       if (i2c->state == STATE_DONE || i2c->state == STATE_ERROR)
> > +               loop_on = OCI2C_STAT_BUSY;
> > +       else
> > +               loop_on = OCI2C_STAT_TIP;
> > +       while (oc_getreg(i2c, OCI2C_STATUS) & loop_on)
> > +               ;
> 
> How would an I2C transmission timeout be handled here?

There is the assumption that the hardware is alive and what we read from 
oc_getreg() is correct. With this assumption, when there is a timeout this 
will happen:
1. STOP command (previous patch)
2. both TIP and BUSY will become zero at some point and we get out from the 
loop

I can see now that there are cases when it may loop forever: for example if 
the device is broken and it does answer always with 0xFFFF: we should not 
break the host as well :)

I can fix this.
 
> > +}
> > +
> > +
> > +/**
> > + * It implements the polling logic
> > + * @work: work instance descriptor
> > + *
> > + * Here we try to re-use as much as possible from the IRQ logic
> > + */
> > +static void ocores_work(struct work_struct *work)
> > +{
> > +       struct ocores_i2c *i2c = container_of(work,
> > +                                             struct ocores_i2c,
> > xfer_work); +       irqreturn_t ret;
> > +
> > +       do {
> > +               ocores_poll_wait(i2c);
> > +               ret = ocores_isr(-1, i2c);
> > +       } while (ret != IRQ_NONE);
> 
> Might as well drop the negation, E.G. while (ret == IRQ_HANDLED);

ok

> > +}
> > +
> > 
> >  static int ocores_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> >  int num) {
> >  
> >         struct ocores_i2c *i2c = i2c_get_adapdata(adap);
> > 
> > @@ -245,6 +296,9 @@ static int ocores_xfer(struct i2c_adapter *adap,
> > struct i2c_msg *msgs, int num)> 
> >         oc_setreg(i2c, OCI2C_DATA, i2c_8bit_addr_from_msg(i2c->msg));
> >         oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_START);
> > 
> > +       if (i2c->flags & OCORES_FLAG_POLL)
> > +               schedule_work(&i2c->xfer_work);
> > +
> > 
> >         if (wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
> >         
> >                                (i2c->state == STATE_DONE), HZ)) {
> >                 
> >                 return (i2c->state == STATE_DONE) ? num : -EIO;
> > 
> > @@ -264,7 +318,8 @@ static int ocores_init(struct device *dev, struct
> > ocores_i2c *i2c)> 
> >         u8 ctrl = oc_getreg(i2c, OCI2C_CONTROL);
> >         
> >         /* make sure the device is disabled */
> > 
> > -       oc_setreg(i2c, OCI2C_CONTROL, ctrl &
> > ~(OCI2C_CTRL_EN|OCI2C_CTRL_IEN));
> > +       ctrl &= ~(OCI2C_CTRL_EN|OCI2C_CTRL_IEN);
> > +       oc_setreg(i2c, OCI2C_CONTROL, ctrl);
> 
> This looks unrelated to $SUBJECT
>
> 
> >         prescale = (i2c->ip_clock_khz / (5 * i2c->bus_clock_khz)) - 1;
> >         prescale = clamp(prescale, 0, 0xffff);
> > 
> > @@ -277,12 +332,16 @@ static int ocores_init(struct device *dev, struct
> > ocores_i2c *i2c)> 
> >                 return -EINVAL;
> >         
> >         }
> > 
> > +
> 
> Here as well.
>
> 
> > @@ -538,6 +600,8 @@ static int ocores_i2c_remove(struct platform_device
> > *pdev)> 
> >  {
> >  
> >         struct ocores_i2c *i2c = platform_get_drvdata(pdev);
> > 
> > +       flush_scheduled_work();
> > +
> 
> Why not cancel_work_sync(&i2c->xfer_work)?

you are right!

Re: [PATCH 1/3] i2c:ocores: stop transfer on timeout

[Federico Vaga]

On Sunday, October 21, 2018 4:10:30 PM CEST Peter Korsgaard wrote:
> On Mon, Jun 25, 2018 at 6:14 PM Federico Vaga <federico.vaga@cern.ch> wrote:
> 
> Hi, and sorry for the slow response.
> 
> > Detecting a timeout is ok, but we also need to assert a STOP command on
> > the bus in order to prevent it from generating interrupts when there are
> > no on going transfers.
> > 
> > Example: very long transmission.
> > 
> > 1. ocores_xfer: START a transfer
> > 2. ocores_isr : handle byte by byte the transfer
> > 3. ocores_xfer: goes in timeout [[bugfix here]]
> > 4. ocores_xfer: return to I2C subsystem and to the I2C driver
> > 5. I2C driver : it may clean up the i2c_msg memory
> > 6. ocores_isr : receives another interrupt (pending bytes to be
> > 
> >                 transferred) but the i2c_msg memory is invalid now
> > 
> > So, since the transfer was too long, we have to detect the timeout and
> > STOP the transfer.
> > 
> > Another point is that we have a critical region here. When handling the
> > timeout condition we may have a running IRQ handler. For this reason I
> > introduce a spinlock. In the IRQ handler we can just use trylock because
> > if the lock is taken is because we are in timeout, so there is no need to
> > process the IRQ.
> > 
> > Signed-off-by: Federico Vaga <federico.vaga@cern.ch>
> > ---
> > 
> >  drivers/i2c/busses/i2c-ocores.c | 28 ++++++++++++++++++++++++++--
> >  1 file changed, 26 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-ocores.c
> > b/drivers/i2c/busses/i2c-ocores.c index 88444ef74943..98c0ef74882b 100644
> > --- a/drivers/i2c/busses/i2c-ocores.c
> > +++ b/drivers/i2c/busses/i2c-ocores.c
> > @@ -25,6 +25,7 @@
> > 
> >  #include <linux/slab.h>
> >  #include <linux/io.h>
> >  #include <linux/log2.h>
> > 
> > +#include <linux/spinlock.h>
> > 
> >  struct ocores_i2c {
> >  
> >         void __iomem *base;
> > 
> > @@ -36,6 +37,7 @@ struct ocores_i2c {
> > 
> >         int pos;
> >         int nmsgs;
> >         int state; /* see STATE_ */
> > 
> > +       spinlock_t xfer_lock;
> > 
> >         struct clk *clk;
> >         int ip_clock_khz;
> >         int bus_clock_khz;
> > 
> > @@ -207,15 +209,30 @@ static void ocores_process(struct ocores_i2c *i2c)
> > 
> >  static irqreturn_t ocores_isr(int irq, void *dev_id)
> >  {
> >  
> >         struct ocores_i2c *i2c = dev_id;
> > 
> > +       unsigned long flags;
> > +       int ret;
> > +
> > +       /*
> > +        * We need to protect i2c against a timeout event (see
> > ocores_xfer()) +        * If we cannot take this lock, it means that we
> > are already in +        * timeout, so it's pointless to handle this
> > interrupt because we +        * are going to abort the current transfer.
> > +        */
> > +       ret = spin_trylock_irqsave(&i2c->xfer_lock, flags);
> 
> This is very old code, so I might be missing something - But I still
> don't quite understand this trylock logic. If we end up here with the
> lock taken, then that must mean that we are on SMP system. We still
> need to ack the interrupt, so just spinning until the other CPU
> releases the lock seems more sensible?

I think you are right.

When I wrote that, I had the idea the STOP command stops the ongoing I2C 
transfer and clear IACK automatically (I do not remember why I had this idea 
in mind, unfortunately I did not take notes about this). So in that case, 
having done STOP on time out, makes IACK useless: that's why "try".

I had another look at the HDL code and apparently my assumption was wrong, and 
STOP just do stop, and IACK is still necessary.

So, yes, without try is better because we save an extra, useless, IRQ call

Re: [PATCH 1/3] i2c:ocores: stop transfer on timeout

[Federico Vaga]

On Sunday, October 21, 2018 4:10:30 PM CEST Peter Korsgaard wrote:
> On Mon, Jun 25, 2018 at 6:14 PM Federico Vaga <federico.vaga@cern.ch> wrote:
> 
> Hi, and sorry for the slow response.
> 
> > Detecting a timeout is ok, but we also need to assert a STOP command on
> > the bus in order to prevent it from generating interrupts when there are
> > no on going transfers.
> > 
> > Example: very long transmission.
> > 
> > 1. ocores_xfer: START a transfer
> > 2. ocores_isr : handle byte by byte the transfer
> > 3. ocores_xfer: goes in timeout [[bugfix here]]
> > 4. ocores_xfer: return to I2C subsystem and to the I2C driver
> > 5. I2C driver : it may clean up the i2c_msg memory
> > 6. ocores_isr : receives another interrupt (pending bytes to be
> > 
> >                 transferred) but the i2c_msg memory is invalid now
> > 
> > So, since the transfer was too long, we have to detect the timeout and
> > STOP the transfer.
> > 
> > Another point is that we have a critical region here. When handling the
> > timeout condition we may have a running IRQ handler. For this reason I
> > introduce a spinlock. In the IRQ handler we can just use trylock because
> > if the lock is taken is because we are in timeout, so there is no need to
> > process the IRQ.
> > 
> > Signed-off-by: Federico Vaga <federico.vaga@cern.ch>
> > ---
> > 
> >  drivers/i2c/busses/i2c-ocores.c | 28 ++++++++++++++++++++++++++--
> >  1 file changed, 26 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-ocores.c
> > b/drivers/i2c/busses/i2c-ocores.c index 88444ef74943..98c0ef74882b 100644
> > --- a/drivers/i2c/busses/i2c-ocores.c
> > +++ b/drivers/i2c/busses/i2c-ocores.c
> > @@ -25,6 +25,7 @@
> > 
> >  #include <linux/slab.h>
> >  #include <linux/io.h>
> >  #include <linux/log2.h>
> > 
> > +#include <linux/spinlock.h>
> > 
> >  struct ocores_i2c {
> >  
> >         void __iomem *base;
> > 
> > @@ -36,6 +37,7 @@ struct ocores_i2c {
> > 
> >         int pos;
> >         int nmsgs;
> >         int state; /* see STATE_ */
> > 
> > +       spinlock_t xfer_lock;
> > 
> >         struct clk *clk;
> >         int ip_clock_khz;
> >         int bus_clock_khz;
> > 
> > @@ -207,15 +209,30 @@ static void ocores_process(struct ocores_i2c *i2c)
> > 
> >  static irqreturn_t ocores_isr(int irq, void *dev_id)
> >  {
> >  
> >         struct ocores_i2c *i2c = dev_id;
> > 
> > +       unsigned long flags;
> > +       int ret;
> > +
> > +       /*
> > +        * We need to protect i2c against a timeout event (see
> > ocores_xfer()) +        * If we cannot take this lock, it means that we
> > are already in +        * timeout, so it's pointless to handle this
> > interrupt because we +        * are going to abort the current transfer.
> > +        */
> > +       ret = spin_trylock_irqsave(&i2c->xfer_lock, flags);
> 
> This is very old code, so I might be missing something - But I still
> don't quite understand this trylock logic. If we end up here with the
> lock taken, then that must mean that we are on SMP system. We still
> need to ack the interrupt, so just spinning until the other CPU
> releases the lock seems more sensible?

I think you are right.

When I wrote that, I had the idea the STOP command stops the ongoing I2C 
transfer and clear IACK automatically (I do not remember why I had this idea 
in mind, unfortunately I did not take notes about this). So in that case, 
having done STOP on time out, makes IACK useless: that's why "try".

I had another look at the HDL code and apparently my assumption was wrong, and 
STOP just do stop, and IACK is still necessary.

So, yes, without try is better because we save an extra, useless, IRQ call

Re: [PATCH 1/3] i2c:ocores: stop transfer on timeout

[Federico Vaga]

(sorry for the noise, peter's email I had does not exist, so I'm resending 
this email with the correct address)

On Sunday, October 21, 2018 4:10:30 PM CEST Peter Korsgaard wrote:
> On Mon, Jun 25, 2018 at 6:14 PM Federico Vaga <federico.vaga@cern.ch> wrote:
> 
> Hi, and sorry for the slow response.
> 
> > Detecting a timeout is ok, but we also need to assert a STOP command on
> > the bus in order to prevent it from generating interrupts when there are
> > no on going transfers.
> > 
> > Example: very long transmission.
> > 
> > 1. ocores_xfer: START a transfer
> > 2. ocores_isr : handle byte by byte the transfer
> > 3. ocores_xfer: goes in timeout [[bugfix here]]
> > 4. ocores_xfer: return to I2C subsystem and to the I2C driver
> > 5. I2C driver : it may clean up the i2c_msg memory
> > 6. ocores_isr : receives another interrupt (pending bytes to be
> > 
> >                 transferred) but the i2c_msg memory is invalid now
> > 
> > So, since the transfer was too long, we have to detect the timeout and
> > STOP the transfer.
> > 
> > Another point is that we have a critical region here. When handling the
> > timeout condition we may have a running IRQ handler. For this reason I
> > introduce a spinlock. In the IRQ handler we can just use trylock because
> > if the lock is taken is because we are in timeout, so there is no need to
> > process the IRQ.
> > 
> > Signed-off-by: Federico Vaga <federico.vaga@cern.ch>
> > ---
> > 
> >  drivers/i2c/busses/i2c-ocores.c | 28 ++++++++++++++++++++++++++--
> >  1 file changed, 26 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-ocores.c
> > b/drivers/i2c/busses/i2c-ocores.c index 88444ef74943..98c0ef74882b 100644
> > --- a/drivers/i2c/busses/i2c-ocores.c
> > +++ b/drivers/i2c/busses/i2c-ocores.c
> > @@ -25,6 +25,7 @@
> > 
> >  #include <linux/slab.h>
> >  #include <linux/io.h>
> >  #include <linux/log2.h>
> > 
> > +#include <linux/spinlock.h>
> > 
> >  struct ocores_i2c {
> >  
> >         void __iomem *base;
> > 
> > @@ -36,6 +37,7 @@ struct ocores_i2c {
> > 
> >         int pos;
> >         int nmsgs;
> >         int state; /* see STATE_ */
> > 
> > +       spinlock_t xfer_lock;
> > 
> >         struct clk *clk;
> >         int ip_clock_khz;
> >         int bus_clock_khz;
> > 
> > @@ -207,15 +209,30 @@ static void ocores_process(struct ocores_i2c *i2c)
> > 
> >  static irqreturn_t ocores_isr(int irq, void *dev_id)
> >  {
> >  
> >         struct ocores_i2c *i2c = dev_id;
> > 
> > +       unsigned long flags;
> > +       int ret;
> > +
> > +       /*
> > +        * We need to protect i2c against a timeout event (see
> > ocores_xfer()) +        * If we cannot take this lock, it means that we
> > are already in +        * timeout, so it's pointless to handle this
> > interrupt because we +        * are going to abort the current transfer.
> > +        */
> > +       ret = spin_trylock_irqsave(&i2c->xfer_lock, flags);
> 
> This is very old code, so I might be missing something - But I still
> don't quite understand this trylock logic. If we end up here with the
> lock taken, then that must mean that we are on SMP system. We still
> need to ack the interrupt, so just spinning until the other CPU
> releases the lock seems more sensible?

I think you are right.

When I wrote that, I had the idea the STOP command stops the ongoing I2C 
transfer and clear IACK automatically (I do not remember why I had this idea 
in mind, unfortunately I did not take notes about this). So in that case, 
having done STOP on time out, makes IACK useless: that's why "try".

I had another look at the HDL code and apparently my assumption was wrong, and 
STOP just do stop, and IACK is still necessary.

So, yes, without try is better because we save an extra, useless, IRQ call

Re: [PATCH 1/3] i2c:ocores: stop transfer on timeout

[Federico Vaga]

(sorry for the noise, peter's email I had does not exist, so I'm resending 
this email with the correct address)

On Sunday, October 21, 2018 4:10:30 PM CEST Peter Korsgaard wrote:
> On Mon, Jun 25, 2018 at 6:14 PM Federico Vaga <federico.vaga@cern.ch> wrote:
> 
> Hi, and sorry for the slow response.
> 
> > Detecting a timeout is ok, but we also need to assert a STOP command on
> > the bus in order to prevent it from generating interrupts when there are
> > no on going transfers.
> > 
> > Example: very long transmission.
> > 
> > 1. ocores_xfer: START a transfer
> > 2. ocores_isr : handle byte by byte the transfer
> > 3. ocores_xfer: goes in timeout [[bugfix here]]
> > 4. ocores_xfer: return to I2C subsystem and to the I2C driver
> > 5. I2C driver : it may clean up the i2c_msg memory
> > 6. ocores_isr : receives another interrupt (pending bytes to be
> > 
> >                 transferred) but the i2c_msg memory is invalid now
> > 
> > So, since the transfer was too long, we have to detect the timeout and
> > STOP the transfer.
> > 
> > Another point is that we have a critical region here. When handling the
> > timeout condition we may have a running IRQ handler. For this reason I
> > introduce a spinlock. In the IRQ handler we can just use trylock because
> > if the lock is taken is because we are in timeout, so there is no need to
> > process the IRQ.
> > 
> > Signed-off-by: Federico Vaga <federico.vaga@cern.ch>
> > ---
> > 
> >  drivers/i2c/busses/i2c-ocores.c | 28 ++++++++++++++++++++++++++--
> >  1 file changed, 26 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-ocores.c
> > b/drivers/i2c/busses/i2c-ocores.c index 88444ef74943..98c0ef74882b 100644
> > --- a/drivers/i2c/busses/i2c-ocores.c
> > +++ b/drivers/i2c/busses/i2c-ocores.c
> > @@ -25,6 +25,7 @@
> > 
> >  #include <linux/slab.h>
> >  #include <linux/io.h>
> >  #include <linux/log2.h>
> > 
> > +#include <linux/spinlock.h>
> > 
> >  struct ocores_i2c {
> >  
> >         void __iomem *base;
> > 
> > @@ -36,6 +37,7 @@ struct ocores_i2c {
> > 
> >         int pos;
> >         int nmsgs;
> >         int state; /* see STATE_ */
> > 
> > +       spinlock_t xfer_lock;
> > 
> >         struct clk *clk;
> >         int ip_clock_khz;
> >         int bus_clock_khz;
> > 
> > @@ -207,15 +209,30 @@ static void ocores_process(struct ocores_i2c *i2c)
> > 
> >  static irqreturn_t ocores_isr(int irq, void *dev_id)
> >  {
> >  
> >         struct ocores_i2c *i2c = dev_id;
> > 
> > +       unsigned long flags;
> > +       int ret;
> > +
> > +       /*
> > +        * We need to protect i2c against a timeout event (see
> > ocores_xfer()) +        * If we cannot take this lock, it means that we
> > are already in +        * timeout, so it's pointless to handle this
> > interrupt because we +        * are going to abort the current transfer.
> > +        */
> > +       ret = spin_trylock_irqsave(&i2c->xfer_lock, flags);
> 
> This is very old code, so I might be missing something - But I still
> don't quite understand this trylock logic. If we end up here with the
> lock taken, then that must mean that we are on SMP system. We still
> need to ack the interrupt, so just spinning until the other CPU
> releases the lock seems more sensible?

I think you are right.

When I wrote that, I had the idea the STOP command stops the ongoing I2C 
transfer and clear IACK automatically (I do not remember why I had this idea 
in mind, unfortunately I did not take notes about this). So in that case, 
having done STOP on time out, makes IACK useless: that's why "try".

I had another look at the HDL code and apparently my assumption was wrong, and 
STOP just do stop, and IACK is still necessary.

So, yes, without try is better because we save an extra, useless, IRQ call

Re: [PATCH 3/3] i2c:ocores: add polling interface

[Federico Vaga]

(sorry for the noise, peter's email I had does not exist, so I'm resending 
this email with the correct address)

On Sunday, October 21, 2018 4:39:07 PM CEST Peter Korsgaard wrote:
> On Mon, Jun 25, 2018 at 6:14 PM Federico Vaga <federico.vaga@cern.ch> wrote:
> > This driver assumes that an interrupt line is always available for
> > the I2C master. This is not always the case and this patch adds support
> > for a polling version based on workqueue.
>
> It probably makes sense to make it the switch between irq/irqless mode
> dynamic to support the upcoming master_xfer_irqless logic.
>
> > Signed-off-by: Federico Vaga <federico.vaga@cern.ch>
> > ---
> >
> > drivers/i2c/busses/i2c-ocores.c | 94
> > ++++++++++++++++++++++++++++++++++------- 1 file changed, 79
> > insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-ocores.c
> > b/drivers/i2c/busses/i2c-ocores.c index 274d6eb22a2c..0dad1a512ef5 100644
> > --- a/drivers/i2c/busses/i2c-ocores.c
> > +++ b/drivers/i2c/busses/i2c-ocores.c
> > @@ -13,6 +13,7 @@
> >
> > */
> >
> > #include <linux/clk.h>
> >
> > +#include <linux/delay.h>
> >
> > #include <linux/err.h>
> > #include <linux/kernel.h>
> > #include <linux/module.h>
> >
> > @@ -26,14 +27,19 @@
> >
> > #include <linux/io.h>
> > #include <linux/log2.h>
> > #include <linux/spinlock.h>
> >
> > +#include <linux/workqueue.h>
> > +
> > +#define OCORES_FLAG_POLL BIT(0)
> >
> > struct ocores_i2c {
> >
> > void __iomem *base;
> > u32 reg_shift;
> > u32 reg_io_width;
> >
> > +       unsigned long flags;
> >
> > wait_queue_head_t wait;
> > struct i2c_adapter adap;
> > struct i2c_msg *msg;
> >
> > +       struct work_struct xfer_work;
> >
> > int pos;
> > int nmsgs;
> > int state; /* see STATE_ */
> >
> > @@ -166,8 +172,9 @@ static void ocores_process(struct ocores_i2c *i2c, u8
> > stat)> 
> > oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP);
> > return;
> >
> > }
> >
> > -       } else
> > +       } else {
> >
> > msg->buf[i2c->pos++] = oc_getreg(i2c, OCI2C_DATA);
> >
> > +       }
>
> This looks unrelated to $SUBJECT.

Do you prefer a different patch just for styling?

>
> > /* end of msg? */
> > if (i2c->pos == msg->len) {
> >
> > @@ -232,6 +239,50 @@ static irqreturn_t ocores_isr(int irq, void *dev_id)
> >
> > return IRQ_HANDLED;
> >
> > }
> >
> > +
> > +/**
> > + * It waits until is possible to process some data
>
> Please don't use "It waits ..", but rather "wait until ..". Same for
> the other function comments.

ok

> > + * @i2c: ocores I2C device instance
> > + *
> > + * This is used when the device is in polling mode (interrupts disabled).
> > + * It sleeps for the time necessary to send 8bits (one transfer over
> > + * the I2C bus), then it permanently ping the ip-core until is possible
> > + * to process data. The idea is that we sleep for most of the time at the
> > + * beginning because we are sure that the ip-core is not ready yet.
> > + */
> > +static void ocores_poll_wait(struct ocores_i2c *i2c)
> > +{
> > +       int sleep_min = (8/i2c->bus_clock_khz) * 1000; /* us for 8bits */
> > +       u8 loop_on;
> > +
> > +       usleep_range(sleep_min, sleep_min + 10);
>
> Where does this 10 come from?

It's true, it's just a random number. It can be zero as well, and we ask the 
system to just sleep for that amount of time. 

(1) usleep_range(sleep_min, sleep_min);

I noticed that it is a common practice to just put numbers that sounds 
correct, indeed there are many random numbers (not commented at least, so they 
are random numbers for the reader) in drivers/i2c/busses when they use this 
function.

This magic number can be also something like:
 
(2) usleep_range(sleep_min, sleep_min * 1.10);

to give a 10% (again random choice) extra margin before starting to actively 
poll.

But I agree that random numbers are not good. So I'm ok with option (1). I did 
not try it, but I think is fine to give a zero delta (delta=max-min=0).

>
> > +       if (i2c->state == STATE_DONE || i2c->state == STATE_ERROR)
> > +               loop_on = OCI2C_STAT_BUSY;
> > +       else
> > +               loop_on = OCI2C_STAT_TIP;
> > +       while (oc_getreg(i2c, OCI2C_STATUS) & loop_on)
> > +               ;
>
> How would an I2C transmission timeout be handled here?

There is the assumption that the hardware is alive and what we read from 
oc_getreg() is correct. With this assumption, when there is a timeout this 
will happen:
1. STOP command (previous patch)
2. both TIP and BUSY will become zero at some point and we get out from the 
loop

I can see now that there are cases when it may loop forever: for example if 
the device is broken and it does answer always with 0xFFFF: we should not 
break the host as well 

I can fix this.
 
> > +}
> > +
> > +
> > +/**
> > + * It implements the polling logic
> > + * @work: work instance descriptor
> > + *
> > + * Here we try to re-use as much as possible from the IRQ logic
> > + */
> > +static void ocores_work(struct work_struct *work)
> > +{
> > +       struct ocores_i2c *i2c = container_of(work,
> > +                                             struct ocores_i2c,
> > xfer_work); +       irqreturn_t ret;
> > +
> > +       do {
> > +               ocores_poll_wait(i2c);
> > +               ret = ocores_isr(-1, i2c);
> > +       } while (ret != IRQ_NONE);
>
> Might as well drop the negation, E.G. while (ret == IRQ_HANDLED);

ok

> > +}
> > +
> >
> > static int ocores_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> > int num) {
> >
> > struct ocores_i2c *i2c = i2c_get_adapdata(adap);
> >
> > @@ -245,6 +296,9 @@ static int ocores_xfer(struct i2c_adapter *adap,
> > struct i2c_msg *msgs, int num)> 
> > oc_setreg(i2c, OCI2C_DATA, i2c_8bit_addr_from_msg(i2c->msg));
> > oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_START);
> >
> > +       if (i2c->flags & OCORES_FLAG_POLL)
> > +               schedule_work(&i2c->xfer_work);
> > +
> >
> > if (wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
> >
> > (i2c->state == STATE_DONE), HZ)) {
> >
> > return (i2c->state == STATE_DONE) ? num : -EIO;
> >
> > @@ -264,7 +318,8 @@ static int ocores_init(struct device *dev, struct
> > ocores_i2c *i2c)> 
> > u8 ctrl = oc_getreg(i2c, OCI2C_CONTROL);
> >
> > /* make sure the device is disabled */
> >
> > -       oc_setreg(i2c, OCI2C_CONTROL, ctrl &
> > ~(OCI2C_CTRL_EN|OCI2C_CTRL_IEN));
> > +       ctrl &= ~(OCI2C_CTRL_EN|OCI2C_CTRL_IEN);
> > +       oc_setreg(i2c, OCI2C_CONTROL, ctrl);
>
> This looks unrelated to $SUBJECT
>
>
> > prescale = (i2c->ip_clock_khz / (5 * i2c->bus_clock_khz)) - 1;
> > prescale = clamp(prescale, 0, 0xffff);
> >
> > @@ -277,12 +332,16 @@ static int ocores_init(struct device *dev, struct
> > ocores_i2c *i2c)> 
> > return -EINVAL;
> >
> > }
> >
> > +
>
> Here as well.
>
>
> > @@ -538,6 +600,8 @@ static int ocores_i2c_remove(struct platform_device
> > *pdev)> 
> > {
> >
> > struct ocores_i2c *i2c = platform_get_drvdata(pdev);
> >
> > +       flush_scheduled_work();
> > +
>
> Why not cancel_work_sync(&i2c->xfer_work)?

you are right!

Re: [PATCH 3/3] i2c:ocores: add polling interface

[Federico Vaga]

(sorry for the noise, peter's email I had does not exist, so I'm resending 
this email with the correct address)

On Sunday, October 21, 2018 4:39:07 PM CEST Peter Korsgaard wrote:
> On Mon, Jun 25, 2018 at 6:14 PM Federico Vaga <federico.vaga@cern.ch> wrote:
> > This driver assumes that an interrupt line is always available for
> > the I2C master. This is not always the case and this patch adds support
> > for a polling version based on workqueue.
>
> It probably makes sense to make it the switch between irq/irqless mode
> dynamic to support the upcoming master_xfer_irqless logic.
>
> > Signed-off-by: Federico Vaga <federico.vaga@cern.ch>
> > ---
> >
> > drivers/i2c/busses/i2c-ocores.c | 94
> > ++++++++++++++++++++++++++++++++++------- 1 file changed, 79
> > insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-ocores.c
> > b/drivers/i2c/busses/i2c-ocores.c index 274d6eb22a2c..0dad1a512ef5 100644
> > --- a/drivers/i2c/busses/i2c-ocores.c
> > +++ b/drivers/i2c/busses/i2c-ocores.c
> > @@ -13,6 +13,7 @@
> >
> > */
> >
> > #include <linux/clk.h>
> >
> > +#include <linux/delay.h>
> >
> > #include <linux/err.h>
> > #include <linux/kernel.h>
> > #include <linux/module.h>
> >
> > @@ -26,14 +27,19 @@
> >
> > #include <linux/io.h>
> > #include <linux/log2.h>
> > #include <linux/spinlock.h>
> >
> > +#include <linux/workqueue.h>
> > +
> > +#define OCORES_FLAG_POLL BIT(0)
> >
> > struct ocores_i2c {
> >
> > void __iomem *base;
> > u32 reg_shift;
> > u32 reg_io_width;
> >
> > +       unsigned long flags;
> >
> > wait_queue_head_t wait;
> > struct i2c_adapter adap;
> > struct i2c_msg *msg;
> >
> > +       struct work_struct xfer_work;
> >
> > int pos;
> > int nmsgs;
> > int state; /* see STATE_ */
> >
> > @@ -166,8 +172,9 @@ static void ocores_process(struct ocores_i2c *i2c, u8
> > stat)> 
> > oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP);
> > return;
> >
> > }
> >
> > -       } else
> > +       } else {
> >
> > msg->buf[i2c->pos++] = oc_getreg(i2c, OCI2C_DATA);
> >
> > +       }
>
> This looks unrelated to $SUBJECT.

Do you prefer a different patch just for styling?

>
> > /* end of msg? */
> > if (i2c->pos == msg->len) {
> >
> > @@ -232,6 +239,50 @@ static irqreturn_t ocores_isr(int irq, void *dev_id)
> >
> > return IRQ_HANDLED;
> >
> > }
> >
> > +
> > +/**
> > + * It waits until is possible to process some data
>
> Please don't use "It waits ..", but rather "wait until ..". Same for
> the other function comments.

ok

> > + * @i2c: ocores I2C device instance
> > + *
> > + * This is used when the device is in polling mode (interrupts disabled).
> > + * It sleeps for the time necessary to send 8bits (one transfer over
> > + * the I2C bus), then it permanently ping the ip-core until is possible
> > + * to process data. The idea is that we sleep for most of the time at the
> > + * beginning because we are sure that the ip-core is not ready yet.
> > + */
> > +static void ocores_poll_wait(struct ocores_i2c *i2c)
> > +{
> > +       int sleep_min = (8/i2c->bus_clock_khz) * 1000; /* us for 8bits */
> > +       u8 loop_on;
> > +
> > +       usleep_range(sleep_min, sleep_min + 10);
>
> Where does this 10 come from?

It's true, it's just a random number. It can be zero as well, and we ask the 
system to just sleep for that amount of time. 

(1) usleep_range(sleep_min, sleep_min);

I noticed that it is a common practice to just put numbers that sounds 
correct, indeed there are many random numbers (not commented at least, so they 
are random numbers for the reader) in drivers/i2c/busses when they use this 
function.

This magic number can be also something like:
 
(2) usleep_range(sleep_min, sleep_min * 1.10);

to give a 10% (again random choice) extra margin before starting to actively 
poll.

But I agree that random numbers are not good. So I'm ok with option (1). I did 
not try it, but I think is fine to give a zero delta (delta=max-min=0).

>
> > +       if (i2c->state == STATE_DONE || i2c->state == STATE_ERROR)
> > +               loop_on = OCI2C_STAT_BUSY;
> > +       else
> > +               loop_on = OCI2C_STAT_TIP;
> > +       while (oc_getreg(i2c, OCI2C_STATUS) & loop_on)
> > +               ;
>
> How would an I2C transmission timeout be handled here?

There is the assumption that the hardware is alive and what we read from 
oc_getreg() is correct. With this assumption, when there is a timeout this 
will happen:
1. STOP command (previous patch)
2. both TIP and BUSY will become zero at some point and we get out from the 
loop

I can see now that there are cases when it may loop forever: for example if 
the device is broken and it does answer always with 0xFFFF: we should not 
break the host as well 

I can fix this.
 
> > +}
> > +
> > +
> > +/**
> > + * It implements the polling logic
> > + * @work: work instance descriptor
> > + *
> > + * Here we try to re-use as much as possible from the IRQ logic
> > + */
> > +static void ocores_work(struct work_struct *work)
> > +{
> > +       struct ocores_i2c *i2c = container_of(work,
> > +                                             struct ocores_i2c,
> > xfer_work); +       irqreturn_t ret;
> > +
> > +       do {
> > +               ocores_poll_wait(i2c);
> > +               ret = ocores_isr(-1, i2c);
> > +       } while (ret != IRQ_NONE);
>
> Might as well drop the negation, E.G. while (ret == IRQ_HANDLED);

ok

> > +}
> > +
> >
> > static int ocores_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> > int num) {
> >
> > struct ocores_i2c *i2c = i2c_get_adapdata(adap);
> >
> > @@ -245,6 +296,9 @@ static int ocores_xfer(struct i2c_adapter *adap,
> > struct i2c_msg *msgs, int num)> 
> > oc_setreg(i2c, OCI2C_DATA, i2c_8bit_addr_from_msg(i2c->msg));
> > oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_START);
> >
> > +       if (i2c->flags & OCORES_FLAG_POLL)
> > +               schedule_work(&i2c->xfer_work);
> > +
> >
> > if (wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
> >
> > (i2c->state == STATE_DONE), HZ)) {
> >
> > return (i2c->state == STATE_DONE) ? num : -EIO;
> >
> > @@ -264,7 +318,8 @@ static int ocores_init(struct device *dev, struct
> > ocores_i2c *i2c)> 
> > u8 ctrl = oc_getreg(i2c, OCI2C_CONTROL);
> >
> > /* make sure the device is disabled */
> >
> > -       oc_setreg(i2c, OCI2C_CONTROL, ctrl &
> > ~(OCI2C_CTRL_EN|OCI2C_CTRL_IEN));
> > +       ctrl &= ~(OCI2C_CTRL_EN|OCI2C_CTRL_IEN);
> > +       oc_setreg(i2c, OCI2C_CONTROL, ctrl);
>
> This looks unrelated to $SUBJECT
>
>
> > prescale = (i2c->ip_clock_khz / (5 * i2c->bus_clock_khz)) - 1;
> > prescale = clamp(prescale, 0, 0xffff);
> >
> > @@ -277,12 +332,16 @@ static int ocores_init(struct device *dev, struct
> > ocores_i2c *i2c)> 
> > return -EINVAL;
> >
> > }
> >
> > +
>
> Here as well.
>
>
> > @@ -538,6 +600,8 @@ static int ocores_i2c_remove(struct platform_device
> > *pdev)> 
> > {
> >
> > struct ocores_i2c *i2c = platform_get_drvdata(pdev);
> >
> > +       flush_scheduled_work();
> > +
>
> Why not cancel_work_sync(&i2c->xfer_work)?

you are right!

Re: [PATCH 3/3] i2c:ocores: add polling interface

[Peter Korsgaard]

>>>>> "Federico" == Federico Vaga <federico.vaga@cern.ch> writes:

Hi,

 >> > -       } else
 >> > +       } else {
 >> > 
 >> >                 msg->buf[i2c->pos++] = oc_getreg(i2c, OCI2C_DATA);
 >> > 
 >> > +       }
 >> 
 >> This looks unrelated to $SUBJECT.

 > Do you prefer a different patch just for styling?

Yes please, it is a lot nicer to keep functional changes from pure style
changes.

 >> > +static void ocores_poll_wait(struct ocores_i2c *i2c)
 >> > +{
 >> > +       int sleep_min = (8/i2c->bus_clock_khz) * 1000; /* us for 8bits */
 >> > +       u8 loop_on;
 >> > +
 >> > +       usleep_range(sleep_min, sleep_min + 10);
 >> 
 >> Where does this 10 come from?

 > It's true, it's just a random number. It can be zero as well, and we ask the 
 > system to just sleep for that amount of time. 

 > (1) usleep_range(sleep_min, sleep_min);

Or just usleep(sleep_min);

 >> 
 >> > +       if (i2c->state == STATE_DONE || i2c->state == STATE_ERROR)
 >> > +               loop_on = OCI2C_STAT_BUSY;
 >> > +       else
 >> > +               loop_on = OCI2C_STAT_TIP;
 >> > +       while (oc_getreg(i2c, OCI2C_STATUS) & loop_on)
 >> > +               ;
 >> 
 >> How would an I2C transmission timeout be handled here?

 > There is the assumption that the hardware is alive and what we read from 
 > oc_getreg() is correct. With this assumption, when there is a timeout this 
 > will happen:
 > 1. STOP command (previous patch)
 > 2. both TIP and BUSY will become zero at some point and we get out from the 
 > loop

 > I can see now that there are cases when it may loop forever: for example if 
 > the device is broken and it does answer always with 0xFFFF: we should not 
 > break the host as well :)

 > I can fix this.

Thanks!

-- 
Bye, Peter Korsgaard

Re: [PATCH 3/3] i2c:ocores: add polling interface

[Peter Korsgaard]

>>>>> "Federico" == Federico Vaga <federico.vaga@cern.ch> writes:

Hi,

 >> > -       } else
 >> > +       } else {
 >> > 
 >> >                 msg->buf[i2c->pos++] = oc_getreg(i2c, OCI2C_DATA);
 >> > 
 >> > +       }
 >> 
 >> This looks unrelated to $SUBJECT.

 > Do you prefer a different patch just for styling?

Yes please, it is a lot nicer to keep functional changes from pure style
changes.

 >> > +static void ocores_poll_wait(struct ocores_i2c *i2c)
 >> > +{
 >> > +       int sleep_min = (8/i2c->bus_clock_khz) * 1000; /* us for 8bits */
 >> > +       u8 loop_on;
 >> > +
 >> > +       usleep_range(sleep_min, sleep_min + 10);
 >> 
 >> Where does this 10 come from?

 > It's true, it's just a random number. It can be zero as well, and we ask the 
 > system to just sleep for that amount of time. 

 > (1) usleep_range(sleep_min, sleep_min);

Or just usleep(sleep_min);

 >> 
 >> > +       if (i2c->state == STATE_DONE || i2c->state == STATE_ERROR)
 >> > +               loop_on = OCI2C_STAT_BUSY;
 >> > +       else
 >> > +               loop_on = OCI2C_STAT_TIP;
 >> > +       while (oc_getreg(i2c, OCI2C_STATUS) & loop_on)
 >> > +               ;
 >> 
 >> How would an I2C transmission timeout be handled here?

 > There is the assumption that the hardware is alive and what we read from 
 > oc_getreg() is correct. With this assumption, when there is a timeout this 
 > will happen:
 > 1. STOP command (previous patch)
 > 2. both TIP and BUSY will become zero at some point and we get out from the 
 > loop

 > I can see now that there are cases when it may loop forever: for example if 
 > the device is broken and it does answer always with 0xFFFF: we should not 
 > break the host as well :)

 > I can fix this.

Thanks!

-- 
Bye, Peter Korsgaard

Re: [PATCH 1/3] i2c:ocores: stop transfer on timeout

[Peter Korsgaard]

>>>>> "Federico" == Federico Vaga <federico.vaga@cern.ch> writes:

Hi,

 > I had another look at the HDL code and apparently my assumption was wrong, and 
 > STOP just do stop, and IACK is still necessary.

 > So, yes, without try is better because we save an extra, useless, IRQ call

Ok, great!

-- 
Bye, Peter Korsgaard

Re: [PATCH 1/3] i2c:ocores: stop transfer on timeout

[Peter Korsgaard]

>>>>> "Federico" == Federico Vaga <federico.vaga@cern.ch> writes:

Hi,

 > I had another look at the HDL code and apparently my assumption was wrong, and 
 > STOP just do stop, and IACK is still necessary.

 > So, yes, without try is better because we save an extra, useless, IRQ call

Ok, great!

-- 
Bye, Peter Korsgaard

Re: [PATCH 3/3] i2c:ocores: add polling interface

[Federico Vaga]

Hi Peter,

On Friday, October 26, 2018 7:45:29 PM CET Peter Korsgaard wrote:
> >>>>> "Federico" == Federico Vaga <federico.vaga@cern.ch> writes:
> Hi,
> 
>  >> > -       } else
>  >> > +       } else {
>  >> > 
>  >> >                 msg->buf[i2c->pos++] = oc_getreg(i2c, OCI2C_DATA);
>  >> > 
>  >> > +       }
>  >> 
>  >> This looks unrelated to $SUBJECT.
>  > 
>  > Do you prefer a different patch just for styling?
> 
> Yes please, it is a lot nicer to keep functional changes from pure style
> changes.

Ok

>  >> > +static void ocores_poll_wait(struct ocores_i2c *i2c)
>  >> > +{
>  >> > +       int sleep_min = (8/i2c->bus_clock_khz) * 1000; /* us for 8bits
>  >> > */
>  >> > +       u8 loop_on;
>  >> > +
>  >> > +       usleep_range(sleep_min, sleep_min + 10);
>  >> 
>  >> Where does this 10 come from?
>  > 
>  > It's true, it's just a random number. It can be zero as well, and we ask
>  > the system to just sleep for that amount of time.
>  > 
>  > (1) usleep_range(sleep_min, sleep_min);
> 
> Or just usleep(sleep_min);

This does not exist as far as I know; the alternative is an active wait with 
udelay. But then, it is not that different from just start polling TIP or BUSY 
flags.

I think that something like this could be better

(2) usleep_range(sleep_min, sleep_min * XXX);

But.
Since it is better to make this patch ready for xfer_irqless, then I will 
definitively go for udelay(). The reason is that, xfer_irqless may run in 
atomic context where we can't sleep at all.

Re: [PATCH 3/3] i2c:ocores: add polling interface

[Federico Vaga]

Hi Peter,

On Friday, October 26, 2018 7:45:29 PM CET Peter Korsgaard wrote:
> >>>>> "Federico" == Federico Vaga <federico.vaga@cern.ch> writes:
> Hi,
> 
>  >> > -       } else
>  >> > +       } else {
>  >> > 
>  >> >                 msg->buf[i2c->pos++] = oc_getreg(i2c, OCI2C_DATA);
>  >> > 
>  >> > +       }
>  >> 
>  >> This looks unrelated to $SUBJECT.
>  > 
>  > Do you prefer a different patch just for styling?
> 
> Yes please, it is a lot nicer to keep functional changes from pure style
> changes.

Ok

>  >> > +static void ocores_poll_wait(struct ocores_i2c *i2c)
>  >> > +{
>  >> > +       int sleep_min = (8/i2c->bus_clock_khz) * 1000; /* us for 8bits
>  >> > */
>  >> > +       u8 loop_on;
>  >> > +
>  >> > +       usleep_range(sleep_min, sleep_min + 10);
>  >> 
>  >> Where does this 10 come from?
>  > 
>  > It's true, it's just a random number. It can be zero as well, and we ask
>  > the system to just sleep for that amount of time.
>  > 
>  > (1) usleep_range(sleep_min, sleep_min);
> 
> Or just usleep(sleep_min);

This does not exist as far as I know; the alternative is an active wait with 
udelay. But then, it is not that different from just start polling TIP or BUSY 
flags.

I think that something like this could be better

(2) usleep_range(sleep_min, sleep_min * XXX);

But.
Since it is better to make this patch ready for xfer_irqless, then I will 
definitively go for udelay(). The reason is that, xfer_irqless may run in 
atomic context where we can't sleep at all.

Re: [PATCH 2/3] i2c:ocores: do not handle IRQ if IF is not set

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 706 bytes --]

On Sun, Oct 21, 2018 at 04:12:10PM +0200, Peter Korsgaard wrote:
> On Mon, Jun 25, 2018 at 6:14 PM Federico Vaga <federico.vaga@cern.ch> wrote:
> >
> > If the Interrupt Flag (IF) is not set, we should not handle the IRQ:
> > - the line can be shared with other devices
> > - it can be a spurious interrupt
> >
> > To avoid reading twice the status register, the ocores_process() function
> > expects it to be read by the caller.
> >
> > Signed-off-by: Federico Vaga <federico.vaga@cern.ch>
> 
> Looks good.
> 
> Acked-by: Peter Korsgaard <peter@korsgaard.com>

I assume this patch will be resent when the other patches get updated?
Or shall I pick this one independently of the others?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/3] i2c:ocores: add polling interface

[Peter Korsgaard]

>>>>> "Federico" == Federico Vaga <federico.vaga@cern.ch> writes:

Hi,

 >> >> Where does this 10 come from?
 >> > 
 >> > It's true, it's just a random number. It can be zero as well, and we ask
 >> > the system to just sleep for that amount of time.
 >> > 
 >> > (1) usleep_range(sleep_min, sleep_min);
 >> 
 >> Or just usleep(sleep_min);

 > This does not exist as far as I know; the alternative is an active wait with 
 > udelay. But then, it is not that different from just start polling TIP or BUSY 
 > flags.

Ahh yes.

 > I think that something like this could be better

 > (2) usleep_range(sleep_min, sleep_min * XXX);

 > But.
 > Since it is better to make this patch ready for xfer_irqless, then I will 
 > definitively go for udelay(). The reason is that, xfer_irqless may run in 
 > atomic context where we can't sleep at all.

Great! BTW I noticed that your sleep_min calculation looked odd:

int sleep_min = (8/i2c->bus_clock_khz) * 1000; /* us for 8bits

bus_clock_khz almost certainly will be bigger than 8 (E.G. likely
100KHz), so the above set sleep_min to 0. Please move the * 1000 before
the division.

-- 
Bye, Peter Korsgaard

Re: [PATCH 3/3] i2c:ocores: add polling interface

[Peter Korsgaard]

>>>>> "Federico" == Federico Vaga <federico.vaga@cern.ch> writes:

Hi,

 >> >> Where does this 10 come from?
 >> > 
 >> > It's true, it's just a random number. It can be zero as well, and we ask
 >> > the system to just sleep for that amount of time.
 >> > 
 >> > (1) usleep_range(sleep_min, sleep_min);
 >> 
 >> Or just usleep(sleep_min);

 > This does not exist as far as I know; the alternative is an active wait with 
 > udelay. But then, it is not that different from just start polling TIP or BUSY 
 > flags.

Ahh yes.

 > I think that something like this could be better

 > (2) usleep_range(sleep_min, sleep_min * XXX);

 > But.
 > Since it is better to make this patch ready for xfer_irqless, then I will 
 > definitively go for udelay(). The reason is that, xfer_irqless may run in 
 > atomic context where we can't sleep at all.

Great! BTW I noticed that your sleep_min calculation looked odd:

int sleep_min = (8/i2c->bus_clock_khz) * 1000; /* us for 8bits

bus_clock_khz almost certainly will be bigger than 8 (E.G. likely
100KHz), so the above set sleep_min to 0. Please move the * 1000 before
the division.

-- 
Bye, Peter Korsgaard

Re: [PATCH 3/3] i2c:ocores: add polling interface

[Federico Vaga]

On Monday, October 29, 2018 2:04:13 PM CET Peter Korsgaard wrote:
>  > I think that something like this could be better
>  > 
>  > (2) usleep_range(sleep_min, sleep_min * XXX);
>  > 
>  > But.
>  > Since it is better to make this patch ready for xfer_irqless, then I will
>  > definitively go for udelay(). The reason is that, xfer_irqless may run in
>  > atomic context where we can't sleep at all.
> 
> Great! BTW I noticed that your sleep_min calculation looked odd:
> 
> int sleep_min = (8/i2c->bus_clock_khz) * 1000; /* us for 8bits
> 
> bus_clock_khz almost certainly will be bigger than 8 (E.G. likely
> 100KHz), so the above set sleep_min to 0. Please move the * 1000 before
> the division.

True, oops

Re: [PATCH 3/3] i2c:ocores: add polling interface

[Federico Vaga]

On Monday, October 29, 2018 2:04:13 PM CET Peter Korsgaard wrote:
>  > I think that something like this could be better
>  > 
>  > (2) usleep_range(sleep_min, sleep_min * XXX);
>  > 
>  > But.
>  > Since it is better to make this patch ready for xfer_irqless, then I will
>  > definitively go for udelay(). The reason is that, xfer_irqless may run in
>  > atomic context where we can't sleep at all.
> 
> Great! BTW I noticed that your sleep_min calculation looked odd:
> 
> int sleep_min = (8/i2c->bus_clock_khz) * 1000; /* us for 8bits
> 
> bus_clock_khz almost certainly will be bigger than 8 (E.G. likely
> 100KHz), so the above set sleep_min to 0. Please move the * 1000 before
> the division.

True, oops

Re: [PATCH 2/3] i2c:ocores: do not handle IRQ if IF is not set

[Federico Vaga]

On Monday, October 29, 2018 9:53:01 AM CET Wolfram Sang wrote:
> On Sun, Oct 21, 2018 at 04:12:10PM +0200, Peter Korsgaard wrote:
> > On Mon, Jun 25, 2018 at 6:14 PM Federico Vaga <federico.vaga@cern.ch> 
wrote:
> > > If the Interrupt Flag (IF) is not set, we should not handle the IRQ:
> > > - the line can be shared with other devices
> > > - it can be a spurious interrupt
> > > 
> > > To avoid reading twice the status register, the ocores_process()
> > > function
> > > expects it to be read by the caller.
> > > 
> > > Signed-off-by: Federico Vaga <federico.vaga@cern.ch>
> > 
> > Looks good.
> > 
> > Acked-by: Peter Korsgaard <peter@korsgaard.com>
> 
> I assume this patch will be resent when the other patches get updated?
> Or shall I pick this one independently of the others?

Since Peter did not answer yet, I would say to wait because I'm going to re-
send the full patch-set soon.

Re: [PATCH 2/3] i2c:ocores: do not handle IRQ if IF is not set

[Federico Vaga]

On Monday, October 29, 2018 9:53:01 AM CET Wolfram Sang wrote:
> On Sun, Oct 21, 2018 at 04:12:10PM +0200, Peter Korsgaard wrote:
> > On Mon, Jun 25, 2018 at 6:14 PM Federico Vaga <federico.vaga@cern.ch> 
wrote:
> > > If the Interrupt Flag (IF) is not set, we should not handle the IRQ:
> > > - the line can be shared with other devices
> > > - it can be a spurious interrupt
> > > 
> > > To avoid reading twice the status register, the ocores_process()
> > > function
> > > expects it to be read by the caller.
> > > 
> > > Signed-off-by: Federico Vaga <federico.vaga@cern.ch>
> > 
> > Looks good.
> > 
> > Acked-by: Peter Korsgaard <peter@korsgaard.com>
> 
> I assume this patch will be resent when the other patches get updated?
> Or shall I pick this one independently of the others?

Since Peter did not answer yet, I would say to wait because I'm going to re-
send the full patch-set soon.