| Message ID | 20240229-mbly-i2c-v2-6-b32ed18c098c@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add Mobileye EyeQ5 support to the Nomadik I2C controller & use hrtimers for timeouts | expand |
On Thu, Feb 29, 2024 at 07:10:54PM +0100, Théo Lebrun wrote: > Replace the completion by a waitqueue for synchronization from IRQ > handler to task. For short timeouts, use hrtimers, else use timers. > Usecase: avoid blocking the I2C bus for too long when an issue occurs. > > The threshold picked is one jiffy: if timeout is below that, use > hrtimers. This threshold is NOT configurable. > > Implement behavior but do NOT change fetching of timeout. This means the > timeout is unchanged (200ms) and the hrtimer case will never trigger. > > A waitqueue is used because it supports both desired timeout approaches. > See wait_event_timeout() and wait_event_hrtimeout(). An atomic boolean > serves as synchronization condition. > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> Largely: Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Nit: > - int timeout; > + int timeout_usecs; I think 'unsigned' makes a lot of sense here. Maybe u32 even?
Hello, On Mon Mar 4, 2024 at 10:18 AM CET, Wolfram Sang wrote: > On Thu, Feb 29, 2024 at 07:10:54PM +0100, Théo Lebrun wrote: > > Replace the completion by a waitqueue for synchronization from IRQ > > handler to task. For short timeouts, use hrtimers, else use timers. > > Usecase: avoid blocking the I2C bus for too long when an issue occurs. > > > > The threshold picked is one jiffy: if timeout is below that, use > > hrtimers. This threshold is NOT configurable. > > > > Implement behavior but do NOT change fetching of timeout. This means the > > timeout is unchanged (200ms) and the hrtimer case will never trigger. > > > > A waitqueue is used because it supports both desired timeout approaches. > > See wait_event_timeout() and wait_event_hrtimeout(). An atomic boolean > > serves as synchronization condition. > > > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > > Largely: > > Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Thanks for the reviews Wolfram. > Nit: > > > - int timeout; > > + int timeout_usecs; > > I think 'unsigned' makes a lot of sense here. Maybe u32 even? Yes unsigned would make sense. unsigned int or u32, I wouldn't know which to pick. Regards, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
> > > - int timeout; > > > + int timeout_usecs; > > > > I think 'unsigned' makes a lot of sense here. Maybe u32 even? > > Yes unsigned would make sense. unsigned int or u32, I wouldn't know > which to pick. It gets (later) fed by of_property_read_u32(), so I tend to suggest u32. Sounds like least conversions then but please double check.
Hi Theo, ... > +static bool nmk_i2c_wait_xfer_done(struct nmk_i2c_dev *priv) > +{ > + if (priv->timeout_usecs < jiffies_to_usecs(1)) { > + unsigned long timeout_usecs = priv->timeout_usecs; > + ktime_t timeout = ktime_set(0, timeout_usecs * NSEC_PER_USEC); > + > + wait_event_hrtimeout(priv->xfer_wq, priv->xfer_done, timeout); > + } else { > + unsigned long timeout = usecs_to_jiffies(priv->timeout_usecs); > + > + wait_event_timeout(priv->xfer_wq, priv->xfer_done, timeout); > + } > + > + return priv->xfer_done; You could eventually write this as static bool nmk_i2c_wait_xfer_done(struct nmk_i2c_dev *priv) { if (priv->timeout_usecs < jiffies_to_usecs(1)) { ... return !wait_event_hrtimeout(...); } ... return wait_event_timeout(...); } It looks a bit cleaner to me... your choice. Rest looks good. Reviewed-by: Andi Shyti <andi.shyti@kernel.org> Andi
Hello, On Mon Mar 4, 2024 at 2:54 PM CET, Andi Shyti wrote: > Hi Theo, > > ... > > > +static bool nmk_i2c_wait_xfer_done(struct nmk_i2c_dev *priv) > > +{ > > + if (priv->timeout_usecs < jiffies_to_usecs(1)) { > > + unsigned long timeout_usecs = priv->timeout_usecs; > > + ktime_t timeout = ktime_set(0, timeout_usecs * NSEC_PER_USEC); > > + > > + wait_event_hrtimeout(priv->xfer_wq, priv->xfer_done, timeout); > > + } else { > > + unsigned long timeout = usecs_to_jiffies(priv->timeout_usecs); > > + > > + wait_event_timeout(priv->xfer_wq, priv->xfer_done, timeout); > > + } > > + > > + return priv->xfer_done; > > You could eventually write this as > > static bool nmk_i2c_wait_xfer_done(struct nmk_i2c_dev *priv) > { > if (priv->timeout_usecs < jiffies_to_usecs(1)) { > ... > > return !wait_event_hrtimeout(...); > } > > ... > return wait_event_timeout(...); > } > > It looks a bit cleaner to me... your choice. The full block would become: static bool nmk_i2c_wait_xfer_done(struct nmk_i2c_dev *priv) { if (priv->timeout_usecs < jiffies_to_usecs(1)) { unsigned long timeout_usecs = priv->timeout_usecs; ktime_t timeout = ktime_set(0, timeout_usecs * NSEC_PER_USEC); return !wait_event_hrtimeout(priv->xfer_wq, priv->xfer_done, timeout); } return wait_event_timeout(priv->xfer_wq, priv->xfer_done, usecs_to_jiffies(priv->timeout_usecs)); } Three things: - Deindenting the jiffy timeout case means no variable declaration after the if-block. This is fine from my point-of-view. - It means we depend on the half-mess that are return values from wait_event_*timeout() macros. I wanted to avoid that because it looks like an error when you read the above code and see one is negated while the other is not. - Also, I'm not confident in casting either return value to bool; what happens if either macro returns an error? This is a theoretical case that shouldn't happen, but behavior might change at some point or bugs could occur. We know priv->xfer_done will give us the right answer. My preference still goes to the original version, but I'm happy we are having a discussion about this code block. > Reviewed-by: Andi Shyti <andi.shyti@kernel.org> Thanks for your review Andi! -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hi Theo, On Mon, Mar 04, 2024 at 03:32:38PM +0100, Théo Lebrun wrote: > On Mon Mar 4, 2024 at 2:54 PM CET, Andi Shyti wrote: > > > +static bool nmk_i2c_wait_xfer_done(struct nmk_i2c_dev *priv) > > > +{ > > > + if (priv->timeout_usecs < jiffies_to_usecs(1)) { > > > + unsigned long timeout_usecs = priv->timeout_usecs; > > > + ktime_t timeout = ktime_set(0, timeout_usecs * NSEC_PER_USEC); > > > + > > > + wait_event_hrtimeout(priv->xfer_wq, priv->xfer_done, timeout); > > > + } else { > > > + unsigned long timeout = usecs_to_jiffies(priv->timeout_usecs); > > > + > > > + wait_event_timeout(priv->xfer_wq, priv->xfer_done, timeout); > > > + } > > > + > > > + return priv->xfer_done; > > > > You could eventually write this as > > > > static bool nmk_i2c_wait_xfer_done(struct nmk_i2c_dev *priv) > > { > > if (priv->timeout_usecs < jiffies_to_usecs(1)) { > > ... > > > > return !wait_event_hrtimeout(...); > > } > > > > ... > > return wait_event_timeout(...); > > } > > > > It looks a bit cleaner to me... your choice. > > The full block would become: > > static bool nmk_i2c_wait_xfer_done(struct nmk_i2c_dev *priv) > { > if (priv->timeout_usecs < jiffies_to_usecs(1)) { > unsigned long timeout_usecs = priv->timeout_usecs; > ktime_t timeout = ktime_set(0, timeout_usecs * NSEC_PER_USEC); > > return !wait_event_hrtimeout(priv->xfer_wq, priv->xfer_done, > timeout); > } > > return wait_event_timeout(priv->xfer_wq, priv->xfer_done, > usecs_to_jiffies(priv->timeout_usecs)); > } > > Three things: > > - Deindenting the jiffy timeout case means no variable declaration > after the if-block. This is fine from my point-of-view. > > - It means we depend on the half-mess that are return values from > wait_event_*timeout() macros. I wanted to avoid that because it > looks like an error when you read the above code and see one is > negated while the other is not. > > - Also, I'm not confident in casting either return value to bool; what > happens if either macro returns an error? This is a theoretical case > that shouldn't happen, but behavior might change at some point or > bugs could occur. We know priv->xfer_done will give us the right > answer. > > My preference still goes to the original version, but I'm happy we are > having a discussion about this code block. sure... it's not a binding comment. Andi
diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c index aa68ab402b10..e68b8e0d7919 100644 --- a/drivers/i2c/busses/i2c-nomadik.c +++ b/drivers/i2c/busses/i2c-nomadik.c @@ -162,10 +162,11 @@ struct i2c_nmk_client { * @clk_freq: clock frequency for the operation mode * @tft: Tx FIFO Threshold in bytes * @rft: Rx FIFO Threshold in bytes - * @timeout: Slave response timeout (ms) + * @timeout_usecs: Slave response timeout * @sm: speed mode * @stop: stop condition. - * @xfer_complete: acknowledge completion for a I2C message. + * @xfer_wq: xfer done wait queue. + * @xfer_done: xfer done boolean. * @result: controller propogated result. */ struct nmk_i2c_dev { @@ -179,10 +180,11 @@ struct nmk_i2c_dev { u32 clk_freq; unsigned char tft; unsigned char rft; - int timeout; + int timeout_usecs; enum i2c_freq_mode sm; int stop; - struct completion xfer_complete; + struct wait_queue_head xfer_wq; + bool xfer_done; int result; }; @@ -434,6 +436,22 @@ static void setup_i2c_controller(struct nmk_i2c_dev *priv) writel(priv->rft, priv->virtbase + I2C_RFTR); } +static bool nmk_i2c_wait_xfer_done(struct nmk_i2c_dev *priv) +{ + if (priv->timeout_usecs < jiffies_to_usecs(1)) { + unsigned long timeout_usecs = priv->timeout_usecs; + ktime_t timeout = ktime_set(0, timeout_usecs * NSEC_PER_USEC); + + wait_event_hrtimeout(priv->xfer_wq, priv->xfer_done, timeout); + } else { + unsigned long timeout = usecs_to_jiffies(priv->timeout_usecs); + + wait_event_timeout(priv->xfer_wq, priv->xfer_done, timeout); + } + + return priv->xfer_done; +} + /** * read_i2c() - Read from I2C client device * @priv: private data of I2C Driver @@ -445,9 +463,9 @@ static void setup_i2c_controller(struct nmk_i2c_dev *priv) */ static int read_i2c(struct nmk_i2c_dev *priv, u16 flags) { - int status = 0; u32 mcr, irq_mask; - unsigned long timeout; + int status = 0; + bool xfer_done; mcr = load_i2c_mcr_reg(priv, flags); writel(mcr, priv->virtbase + I2C_MCR); @@ -459,7 +477,8 @@ static int read_i2c(struct nmk_i2c_dev *priv, u16 flags) /* enable the controller */ i2c_set_bit(priv->virtbase + I2C_CR, I2C_CR_PE); - init_completion(&priv->xfer_complete); + init_waitqueue_head(&priv->xfer_wq); + priv->xfer_done = false; /* enable interrupts by setting the mask */ irq_mask = (I2C_IT_RXFNF | I2C_IT_RXFF | @@ -475,10 +494,9 @@ static int read_i2c(struct nmk_i2c_dev *priv, u16 flags) writel(readl(priv->virtbase + I2C_IMSCR) | irq_mask, priv->virtbase + I2C_IMSCR); - timeout = wait_for_completion_timeout( - &priv->xfer_complete, priv->adap.timeout); + xfer_done = nmk_i2c_wait_xfer_done(priv); - if (timeout == 0) { + if (!xfer_done) { /* Controller timed out */ dev_err(&priv->adev->dev, "read from slave 0x%x timed out\n", priv->cli.slave_adr); @@ -513,9 +531,9 @@ static void fill_tx_fifo(struct nmk_i2c_dev *priv, int no_bytes) */ static int write_i2c(struct nmk_i2c_dev *priv, u16 flags) { - u32 status = 0; u32 mcr, irq_mask; - unsigned long timeout; + u32 status = 0; + bool xfer_done; mcr = load_i2c_mcr_reg(priv, flags); @@ -528,7 +546,8 @@ static int write_i2c(struct nmk_i2c_dev *priv, u16 flags) /* enable the controller */ i2c_set_bit(priv->virtbase + I2C_CR, I2C_CR_PE); - init_completion(&priv->xfer_complete); + init_waitqueue_head(&priv->xfer_wq); + priv->xfer_done = false; /* enable interrupts by settings the masks */ irq_mask = (I2C_IT_TXFOVR | I2C_IT_MAL | I2C_IT_BERR); @@ -554,10 +573,9 @@ static int write_i2c(struct nmk_i2c_dev *priv, u16 flags) writel(readl(priv->virtbase + I2C_IMSCR) | irq_mask, priv->virtbase + I2C_IMSCR); - timeout = wait_for_completion_timeout( - &priv->xfer_complete, priv->adap.timeout); + xfer_done = nmk_i2c_wait_xfer_done(priv); - if (timeout == 0) { + if (!xfer_done) { /* Controller timed out */ dev_err(&priv->adev->dev, "write to slave 0x%x timed out\n", priv->cli.slave_adr); @@ -807,7 +825,9 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg) priv->cli.count); init_hw(priv); } - complete(&priv->xfer_complete); + priv->xfer_done = true; + wake_up(&priv->xfer_wq); + break; @@ -817,7 +837,9 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg) init_hw(priv); i2c_set_bit(priv->virtbase + I2C_ICR, I2C_IT_MAL); - complete(&priv->xfer_complete); + priv->xfer_done = true; + wake_up(&priv->xfer_wq); + break; @@ -834,7 +856,9 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg) init_hw(priv); i2c_set_bit(priv->virtbase + I2C_ICR, I2C_IT_BERR); - complete(&priv->xfer_complete); + priv->xfer_done = true; + wake_up(&priv->xfer_wq); + } break; @@ -848,7 +872,9 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg) init_hw(priv); dev_err(dev, "Tx Fifo Over run\n"); - complete(&priv->xfer_complete); + priv->xfer_done = true; + wake_up(&priv->xfer_wq); + break; @@ -949,7 +975,7 @@ static void nmk_i2c_of_probe(struct device_node *np, priv->sm = I2C_FREQ_MODE_FAST; priv->tft = 1; /* Tx FIFO threshold */ priv->rft = 8; /* Rx FIFO threshold */ - priv->timeout = 200; /* Slave response timeout(ms) */ + priv->timeout_usecs = 200 * USEC_PER_MSEC; /* Slave response timeout */ } static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id) @@ -1009,7 +1035,7 @@ static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id) adap->owner = THIS_MODULE; adap->class = I2C_CLASS_DEPRECATED; adap->algo = &nmk_i2c_algo; - adap->timeout = msecs_to_jiffies(priv->timeout); + adap->timeout = usecs_to_jiffies(priv->timeout_usecs); snprintf(adap->name, sizeof(adap->name), "Nomadik I2C at %pR", &adev->res);