| Message ID | 20180625161303.7991-4-federico.vaga@cern.ch |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/3] i2c:ocores: stop transfer on timeout | expand |
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)?
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!
(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!
>>>>> "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!
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.
>>>>> "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.
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
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));
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(-)