Linux-i2c Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2 1/1] i2c: busses: Add support for atomic transfers in Actions Semi Owl driver
@ 2020-09-08 10:52 Cristian Ciocaltea
  2020-09-09 15:17 ` Manivannan Sadhasivam
  0 siblings, 1 reply; 8+ messages in thread
From: Cristian Ciocaltea @ 2020-09-08 10:52 UTC (permalink / raw)
  To: Manivannan Sadhasivam, Andreas Färber
  Cc: linux-i2c, linux-arm-kernel, linux-kernel, linux-actions

Atomic transfers are required to properly power off a machine through
an I2C controlled PMIC, such as the Actions Semi ATC260x series.

System shutdown may happen with interrupts being disabled and, as a
consequence, the kernel may hang if the driver does not support atomic
transfers.

This functionality is essentially implemented by polling the FIFO
Status register until either Command Execute Completed or NACK Error
bits are set.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
---
Changes in v2:
 - Dropped unused return codes from owl_i2c_xfer_data(), per Mani's review
 - Rebased patch on v5.9-rc4

 drivers/i2c/busses/i2c-owl.c | 78 ++++++++++++++++++++++++++----------
 1 file changed, 57 insertions(+), 21 deletions(-)

diff --git a/drivers/i2c/busses/i2c-owl.c b/drivers/i2c/busses/i2c-owl.c
index 672f1f239bd6..29605257831f 100644
--- a/drivers/i2c/busses/i2c-owl.c
+++ b/drivers/i2c/busses/i2c-owl.c
@@ -14,6 +14,7 @@
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
+#include <linux/iopoll.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
 
@@ -76,6 +77,7 @@
 #define OWL_I2C_FIFOCTL_TFR	BIT(2)
 
 /* I2Cc_FIFOSTAT Bit Mask */
+#define OWL_I2C_FIFOSTAT_CECB	BIT(0)
 #define OWL_I2C_FIFOSTAT_RNB	BIT(1)
 #define OWL_I2C_FIFOSTAT_RFE	BIT(2)
 #define OWL_I2C_FIFOSTAT_TFF	BIT(5)
@@ -83,7 +85,8 @@
 #define OWL_I2C_FIFOSTAT_RFD	GENMASK(15, 8)
 
 /* I2C bus timeout */
-#define OWL_I2C_TIMEOUT		msecs_to_jiffies(4 * 1000)
+#define OWL_I2C_TIMEOUT_MS	(4 * 1000)
+#define OWL_I2C_TIMEOUT		msecs_to_jiffies(OWL_I2C_TIMEOUT_MS)
 
 #define OWL_I2C_MAX_RETRIES	50
 
@@ -161,29 +164,25 @@ static void owl_i2c_set_freq(struct owl_i2c_dev *i2c_dev)
 	writel(OWL_I2C_DIV_FACTOR(val), i2c_dev->base + OWL_I2C_REG_CLKDIV);
 }
 
-static irqreturn_t owl_i2c_interrupt(int irq, void *_dev)
+static void owl_i2c_xfer_data(struct owl_i2c_dev *i2c_dev)
 {
-	struct owl_i2c_dev *i2c_dev = _dev;
 	struct i2c_msg *msg = i2c_dev->msg;
-	unsigned long flags;
 	unsigned int stat, fifostat;
 
-	spin_lock_irqsave(&i2c_dev->lock, flags);
-
 	i2c_dev->err = 0;
 
 	/* Handle NACK from slave */
 	fifostat = readl(i2c_dev->base + OWL_I2C_REG_FIFOSTAT);
 	if (fifostat & OWL_I2C_FIFOSTAT_RNB) {
 		i2c_dev->err = -ENXIO;
-		goto stop;
+		return;
 	}
 
 	/* Handle bus error */
 	stat = readl(i2c_dev->base + OWL_I2C_REG_STAT);
 	if (stat & OWL_I2C_STAT_BEB) {
 		i2c_dev->err = -EIO;
-		goto stop;
+		return;
 	}
 
 	/* Handle FIFO read */
@@ -196,18 +195,28 @@ static irqreturn_t owl_i2c_interrupt(int irq, void *_dev)
 	} else {
 		/* Handle the remaining bytes which were not sent */
 		while (!(readl(i2c_dev->base + OWL_I2C_REG_FIFOSTAT) &
-			 OWL_I2C_FIFOSTAT_TFF) && i2c_dev->msg_ptr < msg->len) {
+			OWL_I2C_FIFOSTAT_TFF) && i2c_dev->msg_ptr < msg->len) {
 			writel(msg->buf[i2c_dev->msg_ptr++],
 			       i2c_dev->base + OWL_I2C_REG_TXDAT);
 		}
 	}
+}
+
+static irqreturn_t owl_i2c_interrupt(int irq, void *_dev)
+{
+	struct owl_i2c_dev *i2c_dev = _dev;
+	unsigned long flags;
+
+	spin_lock_irqsave(&i2c_dev->lock, flags);
+
+	owl_i2c_xfer_data(i2c_dev);
 
-stop:
 	/* Clear pending interrupts */
 	owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_STAT,
 			   OWL_I2C_STAT_IRQP, true);
 
 	complete_all(&i2c_dev->msg_complete);
+
 	spin_unlock_irqrestore(&i2c_dev->lock, flags);
 
 	return IRQ_HANDLED;
@@ -235,8 +244,8 @@ static int owl_i2c_check_bus_busy(struct i2c_adapter *adap)
 	return 0;
 }
 
-static int owl_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
-			       int num)
+static int owl_i2c_xfer_common(struct i2c_adapter *adap, struct i2c_msg *msgs,
+			       int num, bool atomic)
 {
 	struct owl_i2c_dev *i2c_dev = i2c_get_adapdata(adap);
 	struct i2c_msg *msg;
@@ -280,11 +289,12 @@ static int owl_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 		goto err_exit;
 	}
 
-	reinit_completion(&i2c_dev->msg_complete);
+	if (!atomic)
+		reinit_completion(&i2c_dev->msg_complete);
 
-	/* Enable I2C controller interrupt */
+	/* Enable/disable I2C controller interrupt */
 	owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_CTL,
-			   OWL_I2C_CTL_IRQE, true);
+			   OWL_I2C_CTL_IRQE, !atomic);
 
 	/*
 	 * Select: FIFO enable, Master mode, Stop enable, Data count enable,
@@ -352,20 +362,33 @@ static int owl_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 
 	spin_unlock_irqrestore(&i2c_dev->lock, flags);
 
-	time_left = wait_for_completion_timeout(&i2c_dev->msg_complete,
-						adap->timeout);
+	if (atomic) {
+		/* Wait for Command Execute Completed or NACK Error bits */
+		ret = readl_poll_timeout_atomic(i2c_dev->base + OWL_I2C_REG_FIFOSTAT,
+						val, val & (OWL_I2C_FIFOSTAT_CECB |
+							    OWL_I2C_FIFOSTAT_RNB),
+						10, OWL_I2C_TIMEOUT_MS * 1000);
+	} else {
+		time_left = wait_for_completion_timeout(&i2c_dev->msg_complete,
+							adap->timeout);
+		if (!time_left)
+			ret = -ETIMEDOUT;
+	}
 
 	spin_lock_irqsave(&i2c_dev->lock, flags);
-	if (time_left == 0) {
+
+	if (ret) {
 		dev_err(&adap->dev, "Transaction timed out\n");
 		/* Send stop condition and release the bus */
 		owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_CTL,
 				   OWL_I2C_CTL_GBCC_STOP | OWL_I2C_CTL_RB,
 				   true);
-		ret = -ETIMEDOUT;
 		goto err_exit;
 	}
 
+	if (atomic)
+		owl_i2c_xfer_data(i2c_dev);
+
 	ret = i2c_dev->err < 0 ? i2c_dev->err : num;
 
 err_exit:
@@ -379,9 +402,22 @@ static int owl_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 	return ret;
 }
 
+static int owl_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
+			int num)
+{
+	return owl_i2c_xfer_common(adap, msgs, num, false);
+}
+
+static int owl_i2c_xfer_atomic(struct i2c_adapter *adap,
+			       struct i2c_msg *msgs, int num)
+{
+	return owl_i2c_xfer_common(adap, msgs, num, true);
+}
+
 static const struct i2c_algorithm owl_i2c_algorithm = {
-	.master_xfer    = owl_i2c_master_xfer,
-	.functionality  = owl_i2c_func,
+	.master_xfer	     = owl_i2c_xfer,
+	.master_xfer_atomic  = owl_i2c_xfer_atomic,
+	.functionality	     = owl_i2c_func,
 };
 
 static const struct i2c_adapter_quirks owl_i2c_quirks = {
-- 
2.28.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/1] i2c: busses: Add support for atomic transfers in Actions Semi Owl driver
  2020-09-08 10:52 [PATCH v2 1/1] i2c: busses: Add support for atomic transfers in Actions Semi Owl driver Cristian Ciocaltea
@ 2020-09-09 15:17 ` Manivannan Sadhasivam
  2020-09-09 16:59   ` Cristian Ciocaltea
  0 siblings, 1 reply; 8+ messages in thread
From: Manivannan Sadhasivam @ 2020-09-09 15:17 UTC (permalink / raw)
  To: Cristian Ciocaltea
  Cc: Andreas Färber, linux-i2c, linux-arm-kernel, linux-kernel,
	linux-actions

Hi Cristi,

On 0908, Cristian Ciocaltea wrote:
> Atomic transfers are required to properly power off a machine through
> an I2C controlled PMIC, such as the Actions Semi ATC260x series.
> 
> System shutdown may happen with interrupts being disabled and, as a
> consequence, the kernel may hang if the driver does not support atomic
> transfers.
> 
> This functionality is essentially implemented by polling the FIFO
> Status register until either Command Execute Completed or NACK Error
> bits are set.
> 

Thanks for the patch! I just have couple of minor comments and other
than that it looks good to me.

> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
> ---
> Changes in v2:
>  - Dropped unused return codes from owl_i2c_xfer_data(), per Mani's review
>  - Rebased patch on v5.9-rc4
> 
>  drivers/i2c/busses/i2c-owl.c | 78 ++++++++++++++++++++++++++----------
>  1 file changed, 57 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-owl.c b/drivers/i2c/busses/i2c-owl.c
> index 672f1f239bd6..29605257831f 100644
> --- a/drivers/i2c/busses/i2c-owl.c
> +++ b/drivers/i2c/busses/i2c-owl.c
> @@ -14,6 +14,7 @@
>  #include <linux/i2c.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
> +#include <linux/iopoll.h>
>  #include <linux/module.h>
>  #include <linux/of_device.h>
>  
> @@ -76,6 +77,7 @@
>  #define OWL_I2C_FIFOCTL_TFR	BIT(2)
>  
>  /* I2Cc_FIFOSTAT Bit Mask */
> +#define OWL_I2C_FIFOSTAT_CECB	BIT(0)
>  #define OWL_I2C_FIFOSTAT_RNB	BIT(1)
>  #define OWL_I2C_FIFOSTAT_RFE	BIT(2)
>  #define OWL_I2C_FIFOSTAT_TFF	BIT(5)
> @@ -83,7 +85,8 @@
>  #define OWL_I2C_FIFOSTAT_RFD	GENMASK(15, 8)
>  
>  /* I2C bus timeout */
> -#define OWL_I2C_TIMEOUT		msecs_to_jiffies(4 * 1000)
> +#define OWL_I2C_TIMEOUT_MS	(4 * 1000)
> +#define OWL_I2C_TIMEOUT		msecs_to_jiffies(OWL_I2C_TIMEOUT_MS)
>  
>  #define OWL_I2C_MAX_RETRIES	50
>  
> @@ -161,29 +164,25 @@ static void owl_i2c_set_freq(struct owl_i2c_dev *i2c_dev)
>  	writel(OWL_I2C_DIV_FACTOR(val), i2c_dev->base + OWL_I2C_REG_CLKDIV);
>  }
>  
> -static irqreturn_t owl_i2c_interrupt(int irq, void *_dev)
> +static void owl_i2c_xfer_data(struct owl_i2c_dev *i2c_dev)
>  {
> -	struct owl_i2c_dev *i2c_dev = _dev;
>  	struct i2c_msg *msg = i2c_dev->msg;
> -	unsigned long flags;
>  	unsigned int stat, fifostat;
>  
> -	spin_lock_irqsave(&i2c_dev->lock, flags);
> -
>  	i2c_dev->err = 0;
>  
>  	/* Handle NACK from slave */
>  	fifostat = readl(i2c_dev->base + OWL_I2C_REG_FIFOSTAT);
>  	if (fifostat & OWL_I2C_FIFOSTAT_RNB) {
>  		i2c_dev->err = -ENXIO;
> -		goto stop;
> +		return;
>  	}
>  
>  	/* Handle bus error */
>  	stat = readl(i2c_dev->base + OWL_I2C_REG_STAT);
>  	if (stat & OWL_I2C_STAT_BEB) {
>  		i2c_dev->err = -EIO;
> -		goto stop;
> +		return;
>  	}
>  
>  	/* Handle FIFO read */
> @@ -196,18 +195,28 @@ static irqreturn_t owl_i2c_interrupt(int irq, void *_dev)
>  	} else {
>  		/* Handle the remaining bytes which were not sent */
>  		while (!(readl(i2c_dev->base + OWL_I2C_REG_FIFOSTAT) &
> -			 OWL_I2C_FIFOSTAT_TFF) && i2c_dev->msg_ptr < msg->len) {
> +			OWL_I2C_FIFOSTAT_TFF) && i2c_dev->msg_ptr < msg->len) {

Spurious change?

>  			writel(msg->buf[i2c_dev->msg_ptr++],
>  			       i2c_dev->base + OWL_I2C_REG_TXDAT);
>  		}
>  	}
> +}
> +
> +static irqreturn_t owl_i2c_interrupt(int irq, void *_dev)
> +{
> +	struct owl_i2c_dev *i2c_dev = _dev;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&i2c_dev->lock, flags);
> +
> +	owl_i2c_xfer_data(i2c_dev);
>  
> -stop:
>  	/* Clear pending interrupts */
>  	owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_STAT,
>  			   OWL_I2C_STAT_IRQP, true);
>  
>  	complete_all(&i2c_dev->msg_complete);
> +
>  	spin_unlock_irqrestore(&i2c_dev->lock, flags);
>  
>  	return IRQ_HANDLED;
> @@ -235,8 +244,8 @@ static int owl_i2c_check_bus_busy(struct i2c_adapter *adap)
>  	return 0;
>  }
>  
> -static int owl_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> -			       int num)
> +static int owl_i2c_xfer_common(struct i2c_adapter *adap, struct i2c_msg *msgs,
> +			       int num, bool atomic)
>  {
>  	struct owl_i2c_dev *i2c_dev = i2c_get_adapdata(adap);
>  	struct i2c_msg *msg;
> @@ -280,11 +289,12 @@ static int owl_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
>  		goto err_exit;
>  	}
>  
> -	reinit_completion(&i2c_dev->msg_complete);
> +	if (!atomic)
> +		reinit_completion(&i2c_dev->msg_complete);
>  
> -	/* Enable I2C controller interrupt */
> +	/* Enable/disable I2C controller interrupt */
>  	owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_CTL,
> -			   OWL_I2C_CTL_IRQE, true);
> +			   OWL_I2C_CTL_IRQE, !atomic);
>  
>  	/*
>  	 * Select: FIFO enable, Master mode, Stop enable, Data count enable,
> @@ -352,20 +362,33 @@ static int owl_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
>  
>  	spin_unlock_irqrestore(&i2c_dev->lock, flags);
>  
> -	time_left = wait_for_completion_timeout(&i2c_dev->msg_complete,
> -						adap->timeout);
> +	if (atomic) {
> +		/* Wait for Command Execute Completed or NACK Error bits */
> +		ret = readl_poll_timeout_atomic(i2c_dev->base + OWL_I2C_REG_FIFOSTAT,
> +						val, val & (OWL_I2C_FIFOSTAT_CECB |
> +							    OWL_I2C_FIFOSTAT_RNB),
> +						10, OWL_I2C_TIMEOUT_MS * 1000);
> +	} else {
> +		time_left = wait_for_completion_timeout(&i2c_dev->msg_complete,
> +							adap->timeout);
> +		if (!time_left)
> +			ret = -ETIMEDOUT;
> +	}
>  
>  	spin_lock_irqsave(&i2c_dev->lock, flags);
> -	if (time_left == 0) {
> +
> +	if (ret) {
>  		dev_err(&adap->dev, "Transaction timed out\n");
>  		/* Send stop condition and release the bus */
>  		owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_CTL,
>  				   OWL_I2C_CTL_GBCC_STOP | OWL_I2C_CTL_RB,
>  				   true);
> -		ret = -ETIMEDOUT;
>  		goto err_exit;
>  	}
>  
> +	if (atomic)
> +		owl_i2c_xfer_data(i2c_dev);

You are not clearing the pending interrupts here.

Thanks,
Mani

> +
>  	ret = i2c_dev->err < 0 ? i2c_dev->err : num;
>  
>  err_exit:
> @@ -379,9 +402,22 @@ static int owl_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
>  	return ret;
>  }
>  
> +static int owl_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> +			int num)
> +{
> +	return owl_i2c_xfer_common(adap, msgs, num, false);
> +}
> +
> +static int owl_i2c_xfer_atomic(struct i2c_adapter *adap,
> +			       struct i2c_msg *msgs, int num)
> +{
> +	return owl_i2c_xfer_common(adap, msgs, num, true);
> +}
> +
>  static const struct i2c_algorithm owl_i2c_algorithm = {
> -	.master_xfer    = owl_i2c_master_xfer,
> -	.functionality  = owl_i2c_func,
> +	.master_xfer	     = owl_i2c_xfer,
> +	.master_xfer_atomic  = owl_i2c_xfer_atomic,
> +	.functionality	     = owl_i2c_func,
>  };
>  
>  static const struct i2c_adapter_quirks owl_i2c_quirks = {
> -- 
> 2.28.0
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/1] i2c: busses: Add support for atomic transfers in Actions Semi Owl driver
  2020-09-09 15:17 ` Manivannan Sadhasivam
@ 2020-09-09 16:59   ` Cristian Ciocaltea
  2020-09-10  3:02     ` Manivannan Sadhasivam
  0 siblings, 1 reply; 8+ messages in thread
From: Cristian Ciocaltea @ 2020-09-09 16:59 UTC (permalink / raw)
  To: Manivannan Sadhasivam
  Cc: Andreas Färber, linux-i2c, linux-arm-kernel, linux-kernel,
	linux-actions

Hi Mani,

Thanks for the review!

On Wed, Sep 09, 2020 at 08:47:48PM +0530, Manivannan Sadhasivam wrote:
> Hi Cristi,
> 
> On 0908, Cristian Ciocaltea wrote:
> > Atomic transfers are required to properly power off a machine through
> > an I2C controlled PMIC, such as the Actions Semi ATC260x series.
> > 
> > System shutdown may happen with interrupts being disabled and, as a
> > consequence, the kernel may hang if the driver does not support atomic
> > transfers.
> > 
> > This functionality is essentially implemented by polling the FIFO
> > Status register until either Command Execute Completed or NACK Error
> > bits are set.
> > 
> 
> Thanks for the patch! I just have couple of minor comments and other
> than that it looks good to me.
> 
> > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
> > ---
> > Changes in v2:
> >  - Dropped unused return codes from owl_i2c_xfer_data(), per Mani's review
> >  - Rebased patch on v5.9-rc4
> > 
> >  drivers/i2c/busses/i2c-owl.c | 78 ++++++++++++++++++++++++++----------
> >  1 file changed, 57 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-owl.c b/drivers/i2c/busses/i2c-owl.c
> > index 672f1f239bd6..29605257831f 100644
> > --- a/drivers/i2c/busses/i2c-owl.c
> > +++ b/drivers/i2c/busses/i2c-owl.c
> > @@ -14,6 +14,7 @@
> >  #include <linux/i2c.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/io.h>
> > +#include <linux/iopoll.h>
> >  #include <linux/module.h>
> >  #include <linux/of_device.h>
> >  
> > @@ -76,6 +77,7 @@
> >  #define OWL_I2C_FIFOCTL_TFR	BIT(2)
> >  
> >  /* I2Cc_FIFOSTAT Bit Mask */
> > +#define OWL_I2C_FIFOSTAT_CECB	BIT(0)
> >  #define OWL_I2C_FIFOSTAT_RNB	BIT(1)
> >  #define OWL_I2C_FIFOSTAT_RFE	BIT(2)
> >  #define OWL_I2C_FIFOSTAT_TFF	BIT(5)
> > @@ -83,7 +85,8 @@
> >  #define OWL_I2C_FIFOSTAT_RFD	GENMASK(15, 8)
> >  
> >  /* I2C bus timeout */
> > -#define OWL_I2C_TIMEOUT		msecs_to_jiffies(4 * 1000)
> > +#define OWL_I2C_TIMEOUT_MS	(4 * 1000)
> > +#define OWL_I2C_TIMEOUT		msecs_to_jiffies(OWL_I2C_TIMEOUT_MS)
> >  
> >  #define OWL_I2C_MAX_RETRIES	50
> >  
> > @@ -161,29 +164,25 @@ static void owl_i2c_set_freq(struct owl_i2c_dev *i2c_dev)
> >  	writel(OWL_I2C_DIV_FACTOR(val), i2c_dev->base + OWL_I2C_REG_CLKDIV);
> >  }
> >  
> > -static irqreturn_t owl_i2c_interrupt(int irq, void *_dev)
> > +static void owl_i2c_xfer_data(struct owl_i2c_dev *i2c_dev)
> >  {
> > -	struct owl_i2c_dev *i2c_dev = _dev;
> >  	struct i2c_msg *msg = i2c_dev->msg;
> > -	unsigned long flags;
> >  	unsigned int stat, fifostat;
> >  
> > -	spin_lock_irqsave(&i2c_dev->lock, flags);
> > -
> >  	i2c_dev->err = 0;
> >  
> >  	/* Handle NACK from slave */
> >  	fifostat = readl(i2c_dev->base + OWL_I2C_REG_FIFOSTAT);
> >  	if (fifostat & OWL_I2C_FIFOSTAT_RNB) {
> >  		i2c_dev->err = -ENXIO;
> > -		goto stop;
> > +		return;
> >  	}
> >  
> >  	/* Handle bus error */
> >  	stat = readl(i2c_dev->base + OWL_I2C_REG_STAT);
> >  	if (stat & OWL_I2C_STAT_BEB) {
> >  		i2c_dev->err = -EIO;
> > -		goto stop;
> > +		return;
> >  	}
> >  
> >  	/* Handle FIFO read */
> > @@ -196,18 +195,28 @@ static irqreturn_t owl_i2c_interrupt(int irq, void *_dev)
> >  	} else {
> >  		/* Handle the remaining bytes which were not sent */
> >  		while (!(readl(i2c_dev->base + OWL_I2C_REG_FIFOSTAT) &
> > -			 OWL_I2C_FIFOSTAT_TFF) && i2c_dev->msg_ptr < msg->len) {
> > +			OWL_I2C_FIFOSTAT_TFF) && i2c_dev->msg_ptr < msg->len) {
> 
> Spurious change?

I have just fixed a small indentation issue (removed extra space char
in front of OWL_I2C...). I will revert it if you think it's not the right
context for doing this (located ~10 lines bellow the previous edit).

> >  			writel(msg->buf[i2c_dev->msg_ptr++],
> >  			       i2c_dev->base + OWL_I2C_REG_TXDAT);
> >  		}
> >  	}
> > +}
> > +
> > +static irqreturn_t owl_i2c_interrupt(int irq, void *_dev)
> > +{
> > +	struct owl_i2c_dev *i2c_dev = _dev;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&i2c_dev->lock, flags);
> > +
> > +	owl_i2c_xfer_data(i2c_dev);
> >  
> > -stop:
> >  	/* Clear pending interrupts */
> >  	owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_STAT,
> >  			   OWL_I2C_STAT_IRQP, true);
> >  
> >  	complete_all(&i2c_dev->msg_complete);
> > +
> >  	spin_unlock_irqrestore(&i2c_dev->lock, flags);
> >  
> >  	return IRQ_HANDLED;
> > @@ -235,8 +244,8 @@ static int owl_i2c_check_bus_busy(struct i2c_adapter *adap)
> >  	return 0;
> >  }
> >  
> > -static int owl_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> > -			       int num)
> > +static int owl_i2c_xfer_common(struct i2c_adapter *adap, struct i2c_msg *msgs,
> > +			       int num, bool atomic)
> >  {
> >  	struct owl_i2c_dev *i2c_dev = i2c_get_adapdata(adap);
> >  	struct i2c_msg *msg;
> > @@ -280,11 +289,12 @@ static int owl_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> >  		goto err_exit;
> >  	}
> >  
> > -	reinit_completion(&i2c_dev->msg_complete);
> > +	if (!atomic)
> > +		reinit_completion(&i2c_dev->msg_complete);
> >  
> > -	/* Enable I2C controller interrupt */
> > +	/* Enable/disable I2C controller interrupt */
> >  	owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_CTL,
> > -			   OWL_I2C_CTL_IRQE, true);
> > +			   OWL_I2C_CTL_IRQE, !atomic);
> >  
> >  	/*
> >  	 * Select: FIFO enable, Master mode, Stop enable, Data count enable,
> > @@ -352,20 +362,33 @@ static int owl_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> >  
> >  	spin_unlock_irqrestore(&i2c_dev->lock, flags);
> >  
> > -	time_left = wait_for_completion_timeout(&i2c_dev->msg_complete,
> > -						adap->timeout);
> > +	if (atomic) {
> > +		/* Wait for Command Execute Completed or NACK Error bits */
> > +		ret = readl_poll_timeout_atomic(i2c_dev->base + OWL_I2C_REG_FIFOSTAT,
> > +						val, val & (OWL_I2C_FIFOSTAT_CECB |
> > +							    OWL_I2C_FIFOSTAT_RNB),
> > +						10, OWL_I2C_TIMEOUT_MS * 1000);
> > +	} else {
> > +		time_left = wait_for_completion_timeout(&i2c_dev->msg_complete,
> > +							adap->timeout);
> > +		if (!time_left)
> > +			ret = -ETIMEDOUT;
> > +	}
> >  
> >  	spin_lock_irqsave(&i2c_dev->lock, flags);
> > -	if (time_left == 0) {
> > +
> > +	if (ret) {
> >  		dev_err(&adap->dev, "Transaction timed out\n");
> >  		/* Send stop condition and release the bus */
> >  		owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_CTL,
> >  				   OWL_I2C_CTL_GBCC_STOP | OWL_I2C_CTL_RB,
> >  				   true);
> > -		ret = -ETIMEDOUT;
> >  		goto err_exit;
> >  	}
> >  
> > +	if (atomic)
> > +		owl_i2c_xfer_data(i2c_dev);
> 
> You are not clearing the pending interrupts here.

I assumed this is not needed for atomic contexts since the controller
interrupt is disabled (please see the comment above: Enable/disable I2C
controller interrupt).

Otherwise I will simply move the clear pending interrupts code from 
owl_i2c_interrupt() to owl_i2c_xfer_data().

> Thanks,
> Mani
> 
> > +
> >  	ret = i2c_dev->err < 0 ? i2c_dev->err : num;
> >  
> >  err_exit:
> > @@ -379,9 +402,22 @@ static int owl_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> >  	return ret;
> >  }
> >  
> > +static int owl_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> > +			int num)
> > +{
> > +	return owl_i2c_xfer_common(adap, msgs, num, false);
> > +}
> > +
> > +static int owl_i2c_xfer_atomic(struct i2c_adapter *adap,
> > +			       struct i2c_msg *msgs, int num)
> > +{
> > +	return owl_i2c_xfer_common(adap, msgs, num, true);
> > +}
> > +
> >  static const struct i2c_algorithm owl_i2c_algorithm = {
> > -	.master_xfer    = owl_i2c_master_xfer,
> > -	.functionality  = owl_i2c_func,
> > +	.master_xfer	     = owl_i2c_xfer,
> > +	.master_xfer_atomic  = owl_i2c_xfer_atomic,
> > +	.functionality	     = owl_i2c_func,
> >  };
> >  
> >  static const struct i2c_adapter_quirks owl_i2c_quirks = {
> > -- 
> > 2.28.0
> > 

Kind regards,
Cristi

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/1] i2c: busses: Add support for atomic transfers in Actions Semi Owl driver
  2020-09-09 16:59   ` Cristian Ciocaltea
@ 2020-09-10  3:02     ` Manivannan Sadhasivam
  2020-09-10 14:12       ` Cristian Ciocaltea
  0 siblings, 1 reply; 8+ messages in thread
From: Manivannan Sadhasivam @ 2020-09-10  3:02 UTC (permalink / raw)
  To: Cristian Ciocaltea
  Cc: Andreas Färber, linux-i2c, linux-arm-kernel, linux-kernel,
	linux-actions

On 0909, Cristian Ciocaltea wrote:
> Hi Mani,
> 
> Thanks for the review!
> 
> On Wed, Sep 09, 2020 at 08:47:48PM +0530, Manivannan Sadhasivam wrote:
> > Hi Cristi,
> > 
> > On 0908, Cristian Ciocaltea wrote:
> > > Atomic transfers are required to properly power off a machine through
> > > an I2C controlled PMIC, such as the Actions Semi ATC260x series.
> > > 
> > > System shutdown may happen with interrupts being disabled and, as a
> > > consequence, the kernel may hang if the driver does not support atomic
> > > transfers.
> > > 
> > > This functionality is essentially implemented by polling the FIFO
> > > Status register until either Command Execute Completed or NACK Error
> > > bits are set.
> > > 
> > 
> > Thanks for the patch! I just have couple of minor comments and other
> > than that it looks good to me.
> > 
> > > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
> > > ---
> > > Changes in v2:
> > >  - Dropped unused return codes from owl_i2c_xfer_data(), per Mani's review
> > >  - Rebased patch on v5.9-rc4
> > > 
> > >  drivers/i2c/busses/i2c-owl.c | 78 ++++++++++++++++++++++++++----------
> > >  1 file changed, 57 insertions(+), 21 deletions(-)
> > > 
> > > diff --git a/drivers/i2c/busses/i2c-owl.c b/drivers/i2c/busses/i2c-owl.c
> > > index 672f1f239bd6..29605257831f 100644
> > > --- a/drivers/i2c/busses/i2c-owl.c
> > > +++ b/drivers/i2c/busses/i2c-owl.c
> > > @@ -14,6 +14,7 @@
> > >  #include <linux/i2c.h>
> > >  #include <linux/interrupt.h>
> > >  #include <linux/io.h>
> > > +#include <linux/iopoll.h>
> > >  #include <linux/module.h>
> > >  #include <linux/of_device.h>
> > >  
> > > @@ -76,6 +77,7 @@
> > >  #define OWL_I2C_FIFOCTL_TFR	BIT(2)
> > >  
> > >  /* I2Cc_FIFOSTAT Bit Mask */
> > > +#define OWL_I2C_FIFOSTAT_CECB	BIT(0)
> > >  #define OWL_I2C_FIFOSTAT_RNB	BIT(1)
> > >  #define OWL_I2C_FIFOSTAT_RFE	BIT(2)
> > >  #define OWL_I2C_FIFOSTAT_TFF	BIT(5)
> > > @@ -83,7 +85,8 @@
> > >  #define OWL_I2C_FIFOSTAT_RFD	GENMASK(15, 8)
> > >  
> > >  /* I2C bus timeout */
> > > -#define OWL_I2C_TIMEOUT		msecs_to_jiffies(4 * 1000)
> > > +#define OWL_I2C_TIMEOUT_MS	(4 * 1000)
> > > +#define OWL_I2C_TIMEOUT		msecs_to_jiffies(OWL_I2C_TIMEOUT_MS)
> > >  
> > >  #define OWL_I2C_MAX_RETRIES	50
> > >  
> > > @@ -161,29 +164,25 @@ static void owl_i2c_set_freq(struct owl_i2c_dev *i2c_dev)
> > >  	writel(OWL_I2C_DIV_FACTOR(val), i2c_dev->base + OWL_I2C_REG_CLKDIV);
> > >  }
> > >  
> > > -static irqreturn_t owl_i2c_interrupt(int irq, void *_dev)
> > > +static void owl_i2c_xfer_data(struct owl_i2c_dev *i2c_dev)
> > >  {
> > > -	struct owl_i2c_dev *i2c_dev = _dev;
> > >  	struct i2c_msg *msg = i2c_dev->msg;
> > > -	unsigned long flags;
> > >  	unsigned int stat, fifostat;
> > >  
> > > -	spin_lock_irqsave(&i2c_dev->lock, flags);
> > > -
> > >  	i2c_dev->err = 0;
> > >  
> > >  	/* Handle NACK from slave */
> > >  	fifostat = readl(i2c_dev->base + OWL_I2C_REG_FIFOSTAT);
> > >  	if (fifostat & OWL_I2C_FIFOSTAT_RNB) {
> > >  		i2c_dev->err = -ENXIO;
> > > -		goto stop;
> > > +		return;
> > >  	}
> > >  
> > >  	/* Handle bus error */
> > >  	stat = readl(i2c_dev->base + OWL_I2C_REG_STAT);
> > >  	if (stat & OWL_I2C_STAT_BEB) {
> > >  		i2c_dev->err = -EIO;
> > > -		goto stop;
> > > +		return;
> > >  	}
> > >  
> > >  	/* Handle FIFO read */
> > > @@ -196,18 +195,28 @@ static irqreturn_t owl_i2c_interrupt(int irq, void *_dev)
> > >  	} else {
> > >  		/* Handle the remaining bytes which were not sent */
> > >  		while (!(readl(i2c_dev->base + OWL_I2C_REG_FIFOSTAT) &
> > > -			 OWL_I2C_FIFOSTAT_TFF) && i2c_dev->msg_ptr < msg->len) {
> > > +			OWL_I2C_FIFOSTAT_TFF) && i2c_dev->msg_ptr < msg->len) {
> > 
> > Spurious change?
> 
> I have just fixed a small indentation issue (removed extra space char
> in front of OWL_I2C...). I will revert it if you think it's not the right
> context for doing this (located ~10 lines bellow the previous edit).
> 

The extra space was there to align with the starting of 'readl', so please keep
it as it is.

> > >  			writel(msg->buf[i2c_dev->msg_ptr++],
> > >  			       i2c_dev->base + OWL_I2C_REG_TXDAT);
> > >  		}
> > >  	}
> > > +}
> > > +
> > > +static irqreturn_t owl_i2c_interrupt(int irq, void *_dev)
> > > +{
> > > +	struct owl_i2c_dev *i2c_dev = _dev;
> > > +	unsigned long flags;
> > > +
> > > +	spin_lock_irqsave(&i2c_dev->lock, flags);
> > > +
> > > +	owl_i2c_xfer_data(i2c_dev);
> > >  
> > > -stop:
> > >  	/* Clear pending interrupts */
> > >  	owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_STAT,
> > >  			   OWL_I2C_STAT_IRQP, true);
> > >  
> > >  	complete_all(&i2c_dev->msg_complete);
> > > +
> > >  	spin_unlock_irqrestore(&i2c_dev->lock, flags);
> > >  
> > >  	return IRQ_HANDLED;
> > > @@ -235,8 +244,8 @@ static int owl_i2c_check_bus_busy(struct i2c_adapter *adap)
> > >  	return 0;
> > >  }
> > >  
> > > -static int owl_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> > > -			       int num)
> > > +static int owl_i2c_xfer_common(struct i2c_adapter *adap, struct i2c_msg *msgs,
> > > +			       int num, bool atomic)
> > >  {
> > >  	struct owl_i2c_dev *i2c_dev = i2c_get_adapdata(adap);
> > >  	struct i2c_msg *msg;
> > > @@ -280,11 +289,12 @@ static int owl_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> > >  		goto err_exit;
> > >  	}
> > >  
> > > -	reinit_completion(&i2c_dev->msg_complete);
> > > +	if (!atomic)
> > > +		reinit_completion(&i2c_dev->msg_complete);
> > >  
> > > -	/* Enable I2C controller interrupt */
> > > +	/* Enable/disable I2C controller interrupt */
> > >  	owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_CTL,
> > > -			   OWL_I2C_CTL_IRQE, true);
> > > +			   OWL_I2C_CTL_IRQE, !atomic);
> > >  
> > >  	/*
> > >  	 * Select: FIFO enable, Master mode, Stop enable, Data count enable,
> > > @@ -352,20 +362,33 @@ static int owl_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> > >  
> > >  	spin_unlock_irqrestore(&i2c_dev->lock, flags);
> > >  
> > > -	time_left = wait_for_completion_timeout(&i2c_dev->msg_complete,
> > > -						adap->timeout);
> > > +	if (atomic) {
> > > +		/* Wait for Command Execute Completed or NACK Error bits */
> > > +		ret = readl_poll_timeout_atomic(i2c_dev->base + OWL_I2C_REG_FIFOSTAT,
> > > +						val, val & (OWL_I2C_FIFOSTAT_CECB |
> > > +							    OWL_I2C_FIFOSTAT_RNB),
> > > +						10, OWL_I2C_TIMEOUT_MS * 1000);
> > > +	} else {
> > > +		time_left = wait_for_completion_timeout(&i2c_dev->msg_complete,
> > > +							adap->timeout);
> > > +		if (!time_left)
> > > +			ret = -ETIMEDOUT;
> > > +	}
> > >  
> > >  	spin_lock_irqsave(&i2c_dev->lock, flags);
> > > -	if (time_left == 0) {
> > > +
> > > +	if (ret) {
> > >  		dev_err(&adap->dev, "Transaction timed out\n");
> > >  		/* Send stop condition and release the bus */
> > >  		owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_CTL,
> > >  				   OWL_I2C_CTL_GBCC_STOP | OWL_I2C_CTL_RB,
> > >  				   true);
> > > -		ret = -ETIMEDOUT;
> > >  		goto err_exit;
> > >  	}
> > >  
> > > +	if (atomic)
> > > +		owl_i2c_xfer_data(i2c_dev);
> > 
> > You are not clearing the pending interrupts here.
> 
> I assumed this is not needed for atomic contexts since the controller
> interrupt is disabled (please see the comment above: Enable/disable I2C
> controller interrupt).
> 
> Otherwise I will simply move the clear pending interrupts code from 
> owl_i2c_interrupt() to owl_i2c_xfer_data().
> 

Hmm. My intention was to say that we are not clearing the error bits when they
happen but I misinterpreted that setting IRQP will take care of that but it
doesn't.

So I think you should submit a patch which writes to RNB and BEB fields when
they are set (writing 1 will clear the interrupts) and ignore my previous
comment.

Thanks,
Mani

> > Thanks,
> > Mani
> > 
> > > +
> > >  	ret = i2c_dev->err < 0 ? i2c_dev->err : num;
> > >  
> > >  err_exit:
> > > @@ -379,9 +402,22 @@ static int owl_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> > >  	return ret;
> > >  }
> > >  
> > > +static int owl_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> > > +			int num)
> > > +{
> > > +	return owl_i2c_xfer_common(adap, msgs, num, false);
> > > +}
> > > +
> > > +static int owl_i2c_xfer_atomic(struct i2c_adapter *adap,
> > > +			       struct i2c_msg *msgs, int num)
> > > +{
> > > +	return owl_i2c_xfer_common(adap, msgs, num, true);
> > > +}
> > > +
> > >  static const struct i2c_algorithm owl_i2c_algorithm = {
> > > -	.master_xfer    = owl_i2c_master_xfer,
> > > -	.functionality  = owl_i2c_func,
> > > +	.master_xfer	     = owl_i2c_xfer,
> > > +	.master_xfer_atomic  = owl_i2c_xfer_atomic,
> > > +	.functionality	     = owl_i2c_func,
> > >  };
> > >  
> > >  static const struct i2c_adapter_quirks owl_i2c_quirks = {
> > > -- 
> > > 2.28.0
> > > 
> 
> Kind regards,
> Cristi

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/1] i2c: busses: Add support for atomic transfers in Actions Semi Owl driver
  2020-09-10  3:02     ` Manivannan Sadhasivam
@ 2020-09-10 14:12       ` Cristian Ciocaltea
  2020-09-29 19:45         ` Wolfram Sang
  0 siblings, 1 reply; 8+ messages in thread
From: Cristian Ciocaltea @ 2020-09-10 14:12 UTC (permalink / raw)
  To: Manivannan Sadhasivam
  Cc: Andreas Färber, linux-i2c, linux-arm-kernel, linux-kernel,
	linux-actions

On Thu, Sep 10, 2020 at 08:32:25AM +0530, Manivannan Sadhasivam wrote:
> On 0909, Cristian Ciocaltea wrote:
> > Hi Mani,
> > 
> > Thanks for the review!
> > 
> > On Wed, Sep 09, 2020 at 08:47:48PM +0530, Manivannan Sadhasivam wrote:
> > > Hi Cristi,
> > > 
> > > On 0908, Cristian Ciocaltea wrote:
> > > > Atomic transfers are required to properly power off a machine through
> > > > an I2C controlled PMIC, such as the Actions Semi ATC260x series.
> > > > 
> > > > System shutdown may happen with interrupts being disabled and, as a
> > > > consequence, the kernel may hang if the driver does not support atomic
> > > > transfers.
> > > > 
> > > > This functionality is essentially implemented by polling the FIFO
> > > > Status register until either Command Execute Completed or NACK Error
> > > > bits are set.
> > > > 
> > > 
> > > Thanks for the patch! I just have couple of minor comments and other
> > > than that it looks good to me.
> > > 
> > > > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
> > > > ---
> > > > Changes in v2:
> > > >  - Dropped unused return codes from owl_i2c_xfer_data(), per Mani's review
> > > >  - Rebased patch on v5.9-rc4
> > > > 
> > > >  drivers/i2c/busses/i2c-owl.c | 78 ++++++++++++++++++++++++++----------
> > > >  1 file changed, 57 insertions(+), 21 deletions(-)
> > > > 
> > > > diff --git a/drivers/i2c/busses/i2c-owl.c b/drivers/i2c/busses/i2c-owl.c
> > > > index 672f1f239bd6..29605257831f 100644
> > > > --- a/drivers/i2c/busses/i2c-owl.c
> > > > +++ b/drivers/i2c/busses/i2c-owl.c
> > > > @@ -14,6 +14,7 @@
> > > >  #include <linux/i2c.h>
> > > >  #include <linux/interrupt.h>
> > > >  #include <linux/io.h>
> > > > +#include <linux/iopoll.h>
> > > >  #include <linux/module.h>
> > > >  #include <linux/of_device.h>
> > > >  
> > > > @@ -76,6 +77,7 @@
> > > >  #define OWL_I2C_FIFOCTL_TFR	BIT(2)
> > > >  
> > > >  /* I2Cc_FIFOSTAT Bit Mask */
> > > > +#define OWL_I2C_FIFOSTAT_CECB	BIT(0)
> > > >  #define OWL_I2C_FIFOSTAT_RNB	BIT(1)
> > > >  #define OWL_I2C_FIFOSTAT_RFE	BIT(2)
> > > >  #define OWL_I2C_FIFOSTAT_TFF	BIT(5)
> > > > @@ -83,7 +85,8 @@
> > > >  #define OWL_I2C_FIFOSTAT_RFD	GENMASK(15, 8)
> > > >  
> > > >  /* I2C bus timeout */
> > > > -#define OWL_I2C_TIMEOUT		msecs_to_jiffies(4 * 1000)
> > > > +#define OWL_I2C_TIMEOUT_MS	(4 * 1000)
> > > > +#define OWL_I2C_TIMEOUT		msecs_to_jiffies(OWL_I2C_TIMEOUT_MS)
> > > >  
> > > >  #define OWL_I2C_MAX_RETRIES	50
> > > >  
> > > > @@ -161,29 +164,25 @@ static void owl_i2c_set_freq(struct owl_i2c_dev *i2c_dev)
> > > >  	writel(OWL_I2C_DIV_FACTOR(val), i2c_dev->base + OWL_I2C_REG_CLKDIV);
> > > >  }
> > > >  
> > > > -static irqreturn_t owl_i2c_interrupt(int irq, void *_dev)
> > > > +static void owl_i2c_xfer_data(struct owl_i2c_dev *i2c_dev)
> > > >  {
> > > > -	struct owl_i2c_dev *i2c_dev = _dev;
> > > >  	struct i2c_msg *msg = i2c_dev->msg;
> > > > -	unsigned long flags;
> > > >  	unsigned int stat, fifostat;
> > > >  
> > > > -	spin_lock_irqsave(&i2c_dev->lock, flags);
> > > > -
> > > >  	i2c_dev->err = 0;
> > > >  
> > > >  	/* Handle NACK from slave */
> > > >  	fifostat = readl(i2c_dev->base + OWL_I2C_REG_FIFOSTAT);
> > > >  	if (fifostat & OWL_I2C_FIFOSTAT_RNB) {
> > > >  		i2c_dev->err = -ENXIO;
> > > > -		goto stop;
> > > > +		return;
> > > >  	}
> > > >  
> > > >  	/* Handle bus error */
> > > >  	stat = readl(i2c_dev->base + OWL_I2C_REG_STAT);
> > > >  	if (stat & OWL_I2C_STAT_BEB) {
> > > >  		i2c_dev->err = -EIO;
> > > > -		goto stop;
> > > > +		return;
> > > >  	}
> > > >  
> > > >  	/* Handle FIFO read */
> > > > @@ -196,18 +195,28 @@ static irqreturn_t owl_i2c_interrupt(int irq, void *_dev)
> > > >  	} else {
> > > >  		/* Handle the remaining bytes which were not sent */
> > > >  		while (!(readl(i2c_dev->base + OWL_I2C_REG_FIFOSTAT) &
> > > > -			 OWL_I2C_FIFOSTAT_TFF) && i2c_dev->msg_ptr < msg->len) {
> > > > +			OWL_I2C_FIFOSTAT_TFF) && i2c_dev->msg_ptr < msg->len) {
> > > 
> > > Spurious change?
> > 
> > I have just fixed a small indentation issue (removed extra space char
> > in front of OWL_I2C...). I will revert it if you think it's not the right
> > context for doing this (located ~10 lines bellow the previous edit).
> > 
> 
> The extra space was there to align with the starting of 'readl', so please keep
> it as it is.

Right, sorry! I aligned it with the other 'readl' a few lines above and
I missed the extra offset caused by the '!' operator.

> > > >  			writel(msg->buf[i2c_dev->msg_ptr++],
> > > >  			       i2c_dev->base + OWL_I2C_REG_TXDAT);
> > > >  		}
> > > >  	}
> > > > +}
> > > > +
> > > > +static irqreturn_t owl_i2c_interrupt(int irq, void *_dev)
> > > > +{
> > > > +	struct owl_i2c_dev *i2c_dev = _dev;
> > > > +	unsigned long flags;
> > > > +
> > > > +	spin_lock_irqsave(&i2c_dev->lock, flags);
> > > > +
> > > > +	owl_i2c_xfer_data(i2c_dev);
> > > >  
> > > > -stop:
> > > >  	/* Clear pending interrupts */
> > > >  	owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_STAT,
> > > >  			   OWL_I2C_STAT_IRQP, true);
> > > >  
> > > >  	complete_all(&i2c_dev->msg_complete);
> > > > +
> > > >  	spin_unlock_irqrestore(&i2c_dev->lock, flags);
> > > >  
> > > >  	return IRQ_HANDLED;
> > > > @@ -235,8 +244,8 @@ static int owl_i2c_check_bus_busy(struct i2c_adapter *adap)
> > > >  	return 0;
> > > >  }
> > > >  
> > > > -static int owl_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> > > > -			       int num)
> > > > +static int owl_i2c_xfer_common(struct i2c_adapter *adap, struct i2c_msg *msgs,
> > > > +			       int num, bool atomic)
> > > >  {
> > > >  	struct owl_i2c_dev *i2c_dev = i2c_get_adapdata(adap);
> > > >  	struct i2c_msg *msg;
> > > > @@ -280,11 +289,12 @@ static int owl_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> > > >  		goto err_exit;
> > > >  	}
> > > >  
> > > > -	reinit_completion(&i2c_dev->msg_complete);
> > > > +	if (!atomic)
> > > > +		reinit_completion(&i2c_dev->msg_complete);
> > > >  
> > > > -	/* Enable I2C controller interrupt */
> > > > +	/* Enable/disable I2C controller interrupt */
> > > >  	owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_CTL,
> > > > -			   OWL_I2C_CTL_IRQE, true);
> > > > +			   OWL_I2C_CTL_IRQE, !atomic);
> > > >  
> > > >  	/*
> > > >  	 * Select: FIFO enable, Master mode, Stop enable, Data count enable,
> > > > @@ -352,20 +362,33 @@ static int owl_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> > > >  
> > > >  	spin_unlock_irqrestore(&i2c_dev->lock, flags);
> > > >  
> > > > -	time_left = wait_for_completion_timeout(&i2c_dev->msg_complete,
> > > > -						adap->timeout);
> > > > +	if (atomic) {
> > > > +		/* Wait for Command Execute Completed or NACK Error bits */
> > > > +		ret = readl_poll_timeout_atomic(i2c_dev->base + OWL_I2C_REG_FIFOSTAT,
> > > > +						val, val & (OWL_I2C_FIFOSTAT_CECB |
> > > > +							    OWL_I2C_FIFOSTAT_RNB),
> > > > +						10, OWL_I2C_TIMEOUT_MS * 1000);
> > > > +	} else {
> > > > +		time_left = wait_for_completion_timeout(&i2c_dev->msg_complete,
> > > > +							adap->timeout);
> > > > +		if (!time_left)
> > > > +			ret = -ETIMEDOUT;
> > > > +	}
> > > >  
> > > >  	spin_lock_irqsave(&i2c_dev->lock, flags);
> > > > -	if (time_left == 0) {
> > > > +
> > > > +	if (ret) {
> > > >  		dev_err(&adap->dev, "Transaction timed out\n");
> > > >  		/* Send stop condition and release the bus */
> > > >  		owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_CTL,
> > > >  				   OWL_I2C_CTL_GBCC_STOP | OWL_I2C_CTL_RB,
> > > >  				   true);
> > > > -		ret = -ETIMEDOUT;
> > > >  		goto err_exit;
> > > >  	}
> > > >  
> > > > +	if (atomic)
> > > > +		owl_i2c_xfer_data(i2c_dev);
> > > 
> > > You are not clearing the pending interrupts here.
> > 
> > I assumed this is not needed for atomic contexts since the controller
> > interrupt is disabled (please see the comment above: Enable/disable I2C
> > controller interrupt).
> > 
> > Otherwise I will simply move the clear pending interrupts code from 
> > owl_i2c_interrupt() to owl_i2c_xfer_data().
> > 
> 
> Hmm. My intention was to say that we are not clearing the error bits when they
> happen but I misinterpreted that setting IRQP will take care of that but it
> doesn't.
> 
> So I think you should submit a patch which writes to RNB and BEB fields when
> they are set (writing 1 will clear the interrupts) and ignore my previous
> comment.

Sure, I can handle this. I assume this should be a separate patch, to
be applied before the current patch. Should I submit a patch series
instead?

Thanks,
Cristi

> Thanks,
> Mani
> 
> > > Thanks,
> > > Mani
> > > 
> > > > +
> > > >  	ret = i2c_dev->err < 0 ? i2c_dev->err : num;
> > > >  
> > > >  err_exit:
> > > > @@ -379,9 +402,22 @@ static int owl_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> > > >  	return ret;
> > > >  }
> > > >  
> > > > +static int owl_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> > > > +			int num)
> > > > +{
> > > > +	return owl_i2c_xfer_common(adap, msgs, num, false);
> > > > +}
> > > > +
> > > > +static int owl_i2c_xfer_atomic(struct i2c_adapter *adap,
> > > > +			       struct i2c_msg *msgs, int num)
> > > > +{
> > > > +	return owl_i2c_xfer_common(adap, msgs, num, true);
> > > > +}
> > > > +
> > > >  static const struct i2c_algorithm owl_i2c_algorithm = {
> > > > -	.master_xfer    = owl_i2c_master_xfer,
> > > > -	.functionality  = owl_i2c_func,
> > > > +	.master_xfer	     = owl_i2c_xfer,
> > > > +	.master_xfer_atomic  = owl_i2c_xfer_atomic,
> > > > +	.functionality	     = owl_i2c_func,
> > > >  };
> > > >  
> > > >  static const struct i2c_adapter_quirks owl_i2c_quirks = {
> > > > -- 
> > > > 2.28.0
> > > > 
> > 
> > Kind regards,
> > Cristi

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/1] i2c: busses: Add support for atomic transfers in Actions Semi Owl driver
  2020-09-10 14:12       ` Cristian Ciocaltea
@ 2020-09-29 19:45         ` Wolfram Sang
  2020-09-29 21:00           ` Cristian Ciocaltea
  0 siblings, 1 reply; 8+ messages in thread
From: Wolfram Sang @ 2020-09-29 19:45 UTC (permalink / raw)
  To: Cristian Ciocaltea
  Cc: Manivannan Sadhasivam, Andreas Färber, linux-i2c,
	linux-arm-kernel, linux-kernel, linux-actions


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


> Sure, I can handle this. I assume this should be a separate patch, to
> be applied before the current patch. Should I submit a patch series
> instead?

Yes, just do it. And please remove irrelevant parts of the mail when
replying. Thanks!


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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/1] i2c: busses: Add support for atomic transfers in Actions Semi Owl driver
  2020-09-29 19:45         ` Wolfram Sang
@ 2020-09-29 21:00           ` Cristian Ciocaltea
  2020-10-08 22:17             ` Cristian Ciocaltea
  0 siblings, 1 reply; 8+ messages in thread
From: Cristian Ciocaltea @ 2020-09-29 21:00 UTC (permalink / raw)
  To: Wolfram Sang, Manivannan Sadhasivam, Andreas Färber,
	linux-i2c, linux-arm-kernel, linux-kernel, linux-actions

Hi Wolfram,

On Tue, Sep 29, 2020 at 09:45:23PM +0200, Wolfram Sang wrote:
> 
> > Sure, I can handle this. I assume this should be a separate patch, to
> > be applied before the current patch. Should I submit a patch series
> > instead?
> 
> Yes, just do it. 

Sure, I have already clarified this directly with Mani and I'm going
to handle it soon.

> And please remove irrelevant parts of the mail when
> replying. Thanks!

Ok, I'll pay attention to this.

Thanks,
Cristi



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/1] i2c: busses: Add support for atomic transfers in Actions Semi Owl driver
  2020-09-29 21:00           ` Cristian Ciocaltea
@ 2020-10-08 22:17             ` Cristian Ciocaltea
  0 siblings, 0 replies; 8+ messages in thread
From: Cristian Ciocaltea @ 2020-10-08 22:17 UTC (permalink / raw)
  To: Manivannan Sadhasivam, Wolfram Sang
  Cc: Andreas Färber, linux-i2c, linux-arm-kernel, linux-kernel,
	linux-actions

Hi Mani, Wolfram,

On Wed, Sep 30, 2020 at 12:00:10AM +0300, Cristian Ciocaltea wrote:
> Hi Wolfram,
> 
> On Tue, Sep 29, 2020 at 09:45:23PM +0200, Wolfram Sang wrote:
> > 
> > > Sure, I can handle this. I assume this should be a separate patch, to
> > > be applied before the current patch. Should I submit a patch series
> > > instead?
> > 
> > Yes, just do it. 
> 
> Sure, I have already clarified this directly with Mani and I'm going
> to handle it soon.

I have just submitted a patch series that incorporates the current
feedback (patches 1 & 2) and an additional improvement (patch 3):

  i2c: owl: Clear NACK and BUS error bits
  i2c: owl: Add support for atomic transfers
  i2c: owl: Enable asynchronous probing

https://lore.kernel.org/lkml/cover.1602190168.git.cristian.ciocaltea@gmail.com/

Thanks,
Cristi

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-08 10:52 [PATCH v2 1/1] i2c: busses: Add support for atomic transfers in Actions Semi Owl driver Cristian Ciocaltea
2020-09-09 15:17 ` Manivannan Sadhasivam
2020-09-09 16:59   ` Cristian Ciocaltea
2020-09-10  3:02     ` Manivannan Sadhasivam
2020-09-10 14:12       ` Cristian Ciocaltea
2020-09-29 19:45         ` Wolfram Sang
2020-09-29 21:00           ` Cristian Ciocaltea
2020-10-08 22:17             ` Cristian Ciocaltea

Linux-i2c Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-i2c/0 linux-i2c/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-i2c linux-i2c/ https://lore.kernel.org/linux-i2c \
		linux-i2c@vger.kernel.org
	public-inbox-index linux-i2c

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-i2c


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git