diff mbox series

[v2,06/11] i2c: nomadik: support short xfer timeouts using waitqueue & hrtimer

Message ID 20240229-mbly-i2c-v2-6-b32ed18c098c@bootlin.com
State New
Headers
Series Add Mobileye EyeQ5 support to the Nomadik I2C controller & use hrtimers for timeouts |

Commit Message

Théo Lebrun Feb. 29, 2024, 6:10 p.m. UTC 
  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>
---
 drivers/i2c/busses/i2c-nomadik.c | 70 +++++++++++++++++++++++++++-------------
 1 file changed, 48 insertions(+), 22 deletions(-)

Comments

Wolfram Sang March 4, 2024, 9:18 a.m. UTC | #1 
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?
Théo Lebrun March 4, 2024, 10:14 a.m. UTC | #2 
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
Wolfram Sang March 4, 2024, 11:37 a.m. UTC | #3 
> > > -	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.
Andi Shyti March 4, 2024, 1:54 p.m. UTC | #4 
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
Théo Lebrun March 4, 2024, 2:32 p.m. UTC | #5 
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
Andi Shyti March 4, 2024, 3:09 p.m. UTC | #6 
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 mbox series

Patch

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);