This patch set provides improvements to the i2c-ocore driver.
[V1 -> V2]
- replaced usleep_range() with udelay() so that the polling version can be
used in atomic context.
- added dedicated patch for minor style issues
- fixed delay computation
- use spin_lock_irqsave(), instead of spin_trylock_irqsave(). IACK is always
necessary and a trylock would generate an extra interrupt for nothing
- make the driver ready for an eventual master_xfer_irqless()
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 order to make easier to understan locking I have:
- added a new function to handle timeout
- modified the current ocores_process() function in order to be protected
by the new spinlock
Like this it is obvious at first sight that this locking serializes
the execution of ocores_process() and ocores_process_timeout()
Signed-off-by: Federico Vaga <[email protected]>
---
drivers/i2c/busses/i2c-ocores.c | 54 ++++++++++++++++++++++++++++++++++-------
1 file changed, 45 insertions(+), 9 deletions(-)
diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index 87f9caa..aa85202 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -25,7 +25,12 @@
#include <linux/slab.h>
#include <linux/io.h>
#include <linux/log2.h>
+#include <linux/spinlock.h>
+/**
+ * @process_lock: protect I2C transfer process.
+ * ocores_process() and ocores_process_timeout() can't run in parallel.
+ */
struct ocores_i2c {
void __iomem *base;
u32 reg_shift;
@@ -36,6 +41,7 @@ struct ocores_i2c {
int pos;
int nmsgs;
int state; /* see STATE_ */
+ spinlock_t process_lock;
struct clk *clk;
int ip_clock_khz;
int bus_clock_khz;
@@ -141,19 +147,26 @@ static void ocores_process(struct ocores_i2c *i2c)
{
struct i2c_msg *msg = i2c->msg;
u8 stat = oc_getreg(i2c, OCI2C_STATUS);
+ unsigned long flags;
+
+ /*
+ * If we spin here is because we are in timeout, so we are going
+ * to be in STATE_ERROR. See ocores_process_timeout()
+ */
+ spin_lock_irqsave(&i2c->process_lock, flags);
if ((i2c->state == STATE_DONE) || (i2c->state == STATE_ERROR)) {
/* stop has been sent */
oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_IACK);
wake_up(&i2c->wait);
- return;
+ goto out;
}
/* error? */
if (stat & OCI2C_STAT_ARBLOST) {
i2c->state = STATE_ERROR;
oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP);
- return;
+ goto out;
}
if ((i2c->state == STATE_START) || (i2c->state == STATE_WRITE)) {
@@ -163,7 +176,7 @@ static void ocores_process(struct ocores_i2c *i2c)
if (stat & OCI2C_STAT_NACK) {
i2c->state = STATE_ERROR;
oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP);
- return;
+ goto out;
}
} else
msg->buf[i2c->pos++] = oc_getreg(i2c, OCI2C_DATA);
@@ -184,14 +197,14 @@ static void ocores_process(struct ocores_i2c *i2c)
oc_setreg(i2c, OCI2C_DATA, addr);
oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_START);
- return;
+ goto out;
} else
i2c->state = (msg->flags & I2C_M_RD)
? STATE_READ : STATE_WRITE;
} else {
i2c->state = STATE_DONE;
oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP);
- return;
+ goto out;
}
}
@@ -202,6 +215,9 @@ static void ocores_process(struct ocores_i2c *i2c)
oc_setreg(i2c, OCI2C_DATA, msg->buf[i2c->pos++]);
oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_WRITE);
}
+
+out:
+ spin_unlock_irqrestore(&i2c->process_lock, flags);
}
static irqreturn_t ocores_isr(int irq, void *dev_id)
@@ -213,9 +229,24 @@ static irqreturn_t ocores_isr(int irq, void *dev_id)
return IRQ_HANDLED;
}
+/**
+ * Process timeout event
+ * @i2c: ocores I2C device instance
+ */
+static void ocores_process_timeout(struct ocores_i2c *i2c)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&i2c->process_lock, flags);
+ i2c->state = STATE_ERROR;
+ oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP);
+ spin_unlock_irqrestore(&i2c->process_lock, flags);
+}
+
static int ocores_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
{
struct ocores_i2c *i2c = i2c_get_adapdata(adap);
+ int ret;
i2c->msg = msgs;
i2c->pos = 0;
@@ -225,11 +256,14 @@ 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 (wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
- (i2c->state == STATE_DONE), HZ))
- return (i2c->state == STATE_DONE) ? num : -EIO;
- else
+ ret = wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
+ (i2c->state == STATE_DONE), HZ);
+ if (ret == 0) {
+ ocores_process_timeout(i2c);
return -ETIMEDOUT;
+ }
+
+ return (i2c->state == STATE_DONE) ? num : -EIO;
}
static int ocores_init(struct device *dev, struct ocores_i2c *i2c)
@@ -422,6 +456,8 @@ static int ocores_i2c_probe(struct platform_device *pdev)
if (!i2c)
return -ENOMEM;
+ spin_lock_init(&i2c->process_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
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.
Signed-off-by: Federico Vaga <[email protected]>
---
drivers/i2c/busses/i2c-ocores.c | 171 +++++++++++++++++++++++++++++++++++-----
1 file changed, 151 insertions(+), 20 deletions(-)
diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index fcc2558..2d71f11 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,6 +27,9 @@
#include <linux/io.h>
#include <linux/log2.h>
#include <linux/spinlock.h>
+#include <linux/jiffies.h>
+
+#define OCORES_FLAG_POLL BIT(0)
/**
* @process_lock: protect I2C transfer process.
@@ -35,6 +39,7 @@ 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;
@@ -246,10 +251,113 @@ static void ocores_process_timeout(struct ocores_i2c *i2c)
spin_unlock_irqrestore(&i2c->process_lock, flags);
}
-static int ocores_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
+/**
+ * Wait until something change in a given register
+ * @i2c: ocores I2C device instance
+ * @reg: register to query
+ * @mask: bitmask to apply on register value
+ * @val: expected result
+ * @timeout: timeout in jiffies
+ *
+ * Timeout is necessary to avoid to stay here forever when the chip
+ * does not answer correctly.
+ *
+ * Return: 0 on success, -ETIMEDOUT on timeout
+ */
+static int ocores_wait(struct ocores_i2c *i2c,
+ int reg, u8 mask, u8 val,
+ const unsigned long timeout)
+{
+ unsigned long j;
+
+ j = jiffies + timeout;
+ while (1) {
+ u8 status = oc_getreg(i2c, reg);
+
+ if ((status & mask) == val)
+ break;
+
+ if (time_after(jiffies, j))
+ return -ETIMEDOUT;
+ }
+ return 0;
+}
+
+/**
+ * Wait until is possible to process some data
+ * @i2c: ocores I2C device instance
+ *
+ * Used when the device is in polling mode (interrupts disabled).
+ *
+ * Return: 0 on success, -ETIMEDOUT on timeout
+ */
+static int ocores_poll_wait(struct ocores_i2c *i2c)
+{
+ u8 mask;
+ int err;
+
+ if (i2c->state == STATE_DONE || i2c->state == STATE_ERROR) {
+ /* transfer is over */
+ mask = OCI2C_STAT_BUSY;
+ } else {
+ /* on going transfer */
+ mask = OCI2C_STAT_TIP;
+ udelay((8 * 1000) / i2c->bus_clock_khz);
+ }
+
+ /*
+ * once we are here we expect to get the expected result immediately
+ * so if after 1ms we timeout then something is broken.
+ */
+ err = ocores_wait(i2c, OCI2C_STATUS, mask, 0, msecs_to_jiffies(1));
+ if (err)
+ dev_warn(i2c->adap.dev.parent,
+ "%s: STATUS timeout, bit 0x%x did not clear in 1ms\n",
+ __func__, mask);
+ return err;
+}
+
+
+/**
+ * It handles an IRQ-less transfer
+ * @i2c: ocores I2C device instance
+ *
+ * Even if IRQ are disabled, the I2C OpenCore IP behavior is exactly the same
+ * (only that IRQ are not produced). This means that we can re-use entirely
+ * ocores_isr(), we just add our polling code around it.
+ *
+ * It can run in atomic context
+ */
+static void ocores_process_polling(struct ocores_i2c *i2c)
+{
+ while (1) {
+ irqreturn_t ret;
+ int err;
+
+ err = ocores_poll_wait(i2c);
+ if (err) {
+ i2c->state = STATE_ERROR;
+ break; /* timeout */
+ }
+
+ ret = ocores_isr(-1, i2c);
+ if (ret == IRQ_NONE)
+ break; /* all messages have been transfered */
+ }
+}
+
+static int ocores_xfer_core(struct ocores_i2c *i2c,
+ struct i2c_msg *msgs, int num,
+ bool polling)
{
- struct ocores_i2c *i2c = i2c_get_adapdata(adap);
int ret;
+ u8 ctrl;
+
+ ctrl = oc_getreg(i2c, OCI2C_CONTROL);
+ if (polling)
+ oc_setreg(i2c, OCI2C_CONTROL, ctrl & ~OCI2C_CTRL_IEN);
+ else
+ oc_setreg(i2c, OCI2C_CONTROL, ctrl | OCI2C_CTRL_IEN);
i2c->msg = msgs;
i2c->pos = 0;
@@ -259,16 +367,37 @@ 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);
- ret = wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
- (i2c->state == STATE_DONE), HZ);
- if (ret == 0) {
- ocores_process_timeout(i2c);
- return -ETIMEDOUT;
+ if (polling) {
+ ocores_process_polling(i2c);
+ } else {
+ ret = wait_event_timeout(i2c->wait,
+ (i2c->state == STATE_ERROR) ||
+ (i2c->state == STATE_DONE), HZ);
+ if (ret == 0) {
+ ocores_process_timeout(i2c);
+ return -ETIMEDOUT;
+ }
}
return (i2c->state == STATE_DONE) ? num : -EIO;
}
+static int ocores_xfer_polling(struct i2c_adapter *adap,
+ struct i2c_msg *msgs, int num)
+{
+ return ocores_xfer_core(i2c_get_adapdata(adap), msgs, num, true);
+}
+
+static int ocores_xfer(struct i2c_adapter *adap,
+ struct i2c_msg *msgs, int num)
+{
+ struct ocores_i2c *i2c = i2c_get_adapdata(adap);
+
+ if (i2c->flags & OCORES_FLAG_POLL)
+ return ocores_xfer_polling(adap, msgs, num);
+ return ocores_xfer_core(i2c, msgs, num, false);
+}
+
static int ocores_init(struct device *dev, struct ocores_i2c *i2c)
{
int prescale;
@@ -294,7 +423,7 @@ static int ocores_init(struct device *dev, struct ocores_i2c *i2c)
/* Init the device */
oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_IACK);
- oc_setreg(i2c, OCI2C_CONTROL, ctrl | OCI2C_CTRL_IEN | OCI2C_CTRL_EN);
+ oc_setreg(i2c, OCI2C_CONTROL, ctrl | OCI2C_CTRL_EN);
return 0;
}
@@ -451,10 +580,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;
@@ -509,18 +634,24 @@ 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;
+ } 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;
--
2.15.0
Miscellaneous style fixes from checkpatch
Signed-off-by: Federico Vaga <[email protected]>
---
drivers/i2c/busses/i2c-ocores.c | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index e48173b..52c062b 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -182,8 +182,9 @@ static void ocores_process(struct ocores_i2c *i2c, u8 stat)
oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP);
goto out;
}
- } else
+ } else {
msg->buf[i2c->pos++] = oc_getreg(i2c, OCI2C_DATA);
+ }
/* end of msg? */
if (i2c->pos == msg->len) {
@@ -202,9 +203,9 @@ static void ocores_process(struct ocores_i2c *i2c, u8 stat)
oc_setreg(i2c, OCI2C_DATA, addr);
oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_START);
goto out;
- } else
- i2c->state = (msg->flags & I2C_M_RD)
- ? STATE_READ : STATE_WRITE;
+ }
+ i2c->state = (msg->flags & I2C_M_RD)
+ ? STATE_READ : STATE_WRITE;
} else {
i2c->state = STATE_DONE;
oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP);
@@ -405,7 +406,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);
@@ -462,11 +464,13 @@ MODULE_DEVICE_TABLE(of, ocores_i2c_match);
#ifdef CONFIG_OF
/* Read and write functions for the GRLIB port of the controller. Registers are
* 32-bit big endian and the PRELOW and PREHIGH registers are merged into one
- * register. The subsequent registers has their offset decreased accordingly. */
+ * register. The subsequent registers has their offset decreased accordingly.
+ */
static u8 oc_getreg_grlib(struct ocores_i2c *i2c, int reg)
{
u32 rd;
int rreg = reg;
+
if (reg != OCI2C_PRELOW)
rreg--;
rd = ioread32be(i2c->base + (rreg << i2c->reg_shift));
@@ -480,6 +484,7 @@ static void oc_setreg_grlib(struct ocores_i2c *i2c, int reg, u8 value)
{
u32 curr, wr;
int rreg = reg;
+
if (reg != OCI2C_PRELOW)
rreg--;
if (reg == OCI2C_PRELOW || reg == OCI2C_PREHIGH) {
@@ -568,7 +573,7 @@ static int ocores_i2c_of_probe(struct platform_device *pdev,
return 0;
}
#else
-#define ocores_i2c_of_probe(pdev,i2c) -ENODEV
+#define ocores_i2c_of_probe(pdev, i2c) -ENODEV
#endif
static int ocores_i2c_probe(struct platform_device *pdev)
--
2.15.0
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 <[email protected]>
Acked-by: Peter Korsgaard <[email protected]>
---
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 aa85202..fcc2558 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -143,10 +143,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);
unsigned long flags;
/*
@@ -223,8 +222,12 @@ out:
static irqreturn_t ocores_isr(int irq, void *dev_id)
{
struct ocores_i2c *i2c = dev_id;
+ u8 stat = oc_getreg(i2c, OCI2C_STATUS);
+
+ if (!(stat & OCI2C_STAT_IF))
+ return IRQ_NONE;
- ocores_process(i2c);
+ ocores_process(i2c, stat);
return IRQ_HANDLED;
}
--
2.15.0
> This patch set provides improvements to the i2c-ocore driver.
>
> [V1 -> V2]
> - replaced usleep_range() with udelay() so that the polling version can be
> used in atomic context.
> - added dedicated patch for minor style issues
> - fixed delay computation
> - use spin_lock_irqsave(), instead of spin_trylock_irqsave(). IACK is always
> necessary and a trylock would generate an extra interrupt for nothing
> - make the driver ready for an eventual master_xfer_irqless()
Peter, do you have a time slot to review this?
Hi there,
I just want to ask if there has been any update about this patchset that I'm
not aware off. Thanks
On Tuesday, November 27, 2018 12:38:22 PM CET Wolfram Sang wrote:
> > This patch set provides improvements to the i2c-ocore driver.
> >
> > [V1 -> V2]
> > - replaced usleep_range() with udelay() so that the polling version can be
> >
> > used in atomic context.
> >
> > - added dedicated patch for minor style issues
> > - fixed delay computation
> > - use spin_lock_irqsave(), instead of spin_trylock_irqsave(). IACK is
> > always>
> > necessary and a trylock would generate an extra interrupt for nothing
> >
> > - make the driver ready for an eventual master_xfer_irqless()
>
> Peter, do you have a time slot to review this?
On Tue, Jan 15, 2019 at 05:37:07PM +0100, Federico Vaga wrote:
> Hi there,
>
> I just want to ask if there has been any update about this patchset that I'm
> not aware off. Thanks
I don't think so. Peter, what is the status of this series?