[PATCH 1/5] i2c: xiic: Fix broken locking on tx_msg

[PATCH 1/5] i2c: xiic: Fix broken locking on tx_msg

[Marek Vasut]

The tx_msg is set from multiple places, sometimes without locking,
which fall apart on any SMP system. Only ever access tx_msg inside
the driver mutex.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Michal Simek <michal.simek@xilinx.com>
Cc: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
Cc: Wolfram Sang <wsa@kernel.org>
---
 drivers/i2c/busses/i2c-xiic.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 90c1c362394d0..0777e577720db 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -168,7 +168,7 @@ struct xiic_i2c {
 #define xiic_tx_space(i2c) ((i2c)->tx_msg->len - (i2c)->tx_pos)
 #define xiic_rx_space(i2c) ((i2c)->rx_msg->len - (i2c)->rx_pos)
 
-static int xiic_start_xfer(struct xiic_i2c *i2c);
+static int xiic_start_xfer(struct xiic_i2c *i2c, struct i2c_msg *msgs, int num);
 static void __xiic_start_xfer(struct xiic_i2c *i2c);
 
 /*
@@ -673,15 +673,24 @@ static void __xiic_start_xfer(struct xiic_i2c *i2c)
 
 }
 
-static int xiic_start_xfer(struct xiic_i2c *i2c)
+static int xiic_start_xfer(struct xiic_i2c *i2c, struct i2c_msg *msgs, int num)
 {
 	int ret;
+
 	mutex_lock(&i2c->lock);
 
+	ret = xiic_busy(i2c);
+	if (ret)
+		goto out;
+
+	i2c->tx_msg = msgs;
+	i2c->nmsgs = num;
+
 	ret = xiic_reinit(i2c);
 	if (!ret)
 		__xiic_start_xfer(i2c);
 
+out:
 	mutex_unlock(&i2c->lock);
 
 	return ret;
@@ -699,14 +708,7 @@ static int xiic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 	if (err < 0)
 		return err;
 
-	err = xiic_busy(i2c);
-	if (err)
-		goto out;
-
-	i2c->tx_msg = msgs;
-	i2c->nmsgs = num;
-
-	err = xiic_start_xfer(i2c);
+	err = xiic_start_xfer(i2c, msgs, num);
 	if (err < 0) {
 		dev_err(adap->dev.parent, "Error xiic_start_xfer\n");
 		goto out;
@@ -714,9 +716,11 @@ static int xiic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 
 	if (wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
 		(i2c->state == STATE_DONE), HZ)) {
+		mutex_lock(&i2c->lock);
 		err = (i2c->state == STATE_DONE) ? num : -EIO;
 		goto out;
 	} else {
+		mutex_lock(&i2c->lock);
 		i2c->tx_msg = NULL;
 		i2c->rx_msg = NULL;
 		i2c->nmsgs = 0;
@@ -724,6 +728,7 @@ static int xiic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 		goto out;
 	}
 out:
+	mutex_unlock(&i2c->lock);
 	pm_runtime_mark_last_busy(i2c->dev);
 	pm_runtime_put_autosuspend(i2c->dev);
 	return err;
-- 
2.26.2

[PATCH 2/5] i2c: xiic: Drop broken interrupt handler

[Marek Vasut]

The interrupt handler is missing locking when reading out registers
and is racing with other threads which might access the driver. Drop
it altogether, so that the threaded interrupt is always executed, as
that one is already serialized by the driver mutex. This also allows
dropping local_irq_save()/local_irq_restore() in xiic_start_recv().

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Michal Simek <michal.simek@xilinx.com>
Cc: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
Cc: Wolfram Sang <wsa@kernel.org>
---
 drivers/i2c/busses/i2c-xiic.c | 25 +------------------------
 1 file changed, 1 insertion(+), 24 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 0777e577720db..6db71c0fb6583 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -543,7 +543,6 @@ static void xiic_start_recv(struct xiic_i2c *i2c)
 {
 	u8 rx_watermark;
 	struct i2c_msg *msg = i2c->rx_msg = i2c->tx_msg;
-	unsigned long flags;
 
 	/* Clear and enable Rx full interrupt. */
 	xiic_irq_clr_en(i2c, XIIC_INTR_RX_FULL_MASK | XIIC_INTR_TX_ERROR_MASK);
@@ -559,7 +558,6 @@ static void xiic_start_recv(struct xiic_i2c *i2c)
 		rx_watermark = IIC_RX_FIFO_DEPTH;
 	xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET, rx_watermark - 1);
 
-	local_irq_save(flags);
 	if (!(msg->flags & I2C_M_NOSTART))
 		/* write the address */
 		xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET,
@@ -569,7 +567,6 @@ static void xiic_start_recv(struct xiic_i2c *i2c)
 
 	xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET,
 		msg->len | ((i2c->nmsgs == 1) ? XIIC_TX_DYN_STOP_MASK : 0));
-	local_irq_restore(flags);
 
 	if (i2c->nmsgs == 1)
 		/* very last, enable bus not busy as well */
@@ -609,26 +606,6 @@ static void xiic_start_send(struct xiic_i2c *i2c)
 		XIIC_INTR_BNB_MASK);
 }
 
-static irqreturn_t xiic_isr(int irq, void *dev_id)
-{
-	struct xiic_i2c *i2c = dev_id;
-	u32 pend, isr, ier;
-	irqreturn_t ret = IRQ_NONE;
-	/* Do not processes a devices interrupts if the device has no
-	 * interrupts pending
-	 */
-
-	dev_dbg(i2c->adap.dev.parent, "%s entry\n", __func__);
-
-	isr = xiic_getreg32(i2c, XIIC_IISR_OFFSET);
-	ier = xiic_getreg32(i2c, XIIC_IIER_OFFSET);
-	pend = isr & ier;
-	if (pend)
-		ret = IRQ_WAKE_THREAD;
-
-	return ret;
-}
-
 static void __xiic_start_xfer(struct xiic_i2c *i2c)
 {
 	int first = 1;
@@ -807,7 +784,7 @@ static int xiic_i2c_probe(struct platform_device *pdev)
 	pm_runtime_use_autosuspend(i2c->dev);
 	pm_runtime_set_active(i2c->dev);
 	pm_runtime_enable(i2c->dev);
-	ret = devm_request_threaded_irq(&pdev->dev, irq, xiic_isr,
+	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
 					xiic_process, IRQF_ONESHOT,
 					pdev->name, i2c);
 
-- 
2.26.2

[PATCH 3/5] i2c: xiic: Defer xiic_wakeup() and __xiic_start_xfer() in xiic_process()

[Marek Vasut]

The __xiic_start_xfer() manipulates the interrupt flags, xiic_wakeup()
may result in return from xiic_xfer() early. Defer both to the end of
the xiic_process() interrupt thread, so that they are executed after
all the other interrupt bits handling completed and once it completely
safe to perform changes to the interrupt bits in the hardware.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Michal Simek <michal.simek@xilinx.com>
Cc: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
Cc: Wolfram Sang <wsa@kernel.org>
---
 drivers/i2c/busses/i2c-xiic.c | 37 ++++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 6db71c0fb6583..87eca9d46afd9 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -373,6 +373,9 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
 	struct xiic_i2c *i2c = dev_id;
 	u32 pend, isr, ier;
 	u32 clr = 0;
+	int xfer_more = 0;
+	int wakeup_req = 0;
+	int wakeup_code = 0;
 
 	/* Get the interrupt Status from the IPIF. There is no clearing of
 	 * interrupts in the IPIF. Interrupts must be cleared at the source.
@@ -409,10 +412,14 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
 		 */
 		xiic_reinit(i2c);
 
-		if (i2c->rx_msg)
-			xiic_wakeup(i2c, STATE_ERROR);
-		if (i2c->tx_msg)
-			xiic_wakeup(i2c, STATE_ERROR);
+		if (i2c->rx_msg) {
+			wakeup_req = 1;
+			wakeup_code = STATE_ERROR;
+		}
+		if (i2c->tx_msg) {
+			wakeup_req = 1;
+			wakeup_code = STATE_ERROR;
+		}
 	}
 	if (pend & XIIC_INTR_RX_FULL_MASK) {
 		/* Receive register/FIFO is full */
@@ -446,8 +453,7 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
 				i2c->tx_msg++;
 				dev_dbg(i2c->adap.dev.parent,
 					"%s will start next...\n", __func__);
-
-				__xiic_start_xfer(i2c);
+				xfer_more = 1;
 			}
 		}
 	}
@@ -461,11 +467,13 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
 		if (!i2c->tx_msg)
 			goto out;
 
-		if ((i2c->nmsgs == 1) && !i2c->rx_msg &&
-			xiic_tx_space(i2c) == 0)
-			xiic_wakeup(i2c, STATE_DONE);
+		wakeup_req = 1;
+
+		if (i2c->nmsgs == 1 && !i2c->rx_msg &&
+		    xiic_tx_space(i2c) == 0)
+			wakeup_code = STATE_DONE;
 		else
-			xiic_wakeup(i2c, STATE_ERROR);
+			wakeup_code = STATE_ERROR;
 	}
 	if (pend & (XIIC_INTR_TX_EMPTY_MASK | XIIC_INTR_TX_HALF_MASK)) {
 		/* Transmit register/FIFO is empty or ½ empty */
@@ -489,7 +497,7 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
 			if (i2c->nmsgs > 1) {
 				i2c->nmsgs--;
 				i2c->tx_msg++;
-				__xiic_start_xfer(i2c);
+				xfer_more = 1;
 			} else {
 				xiic_irq_dis(i2c, XIIC_INTR_TX_HALF_MASK);
 
@@ -507,6 +515,13 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
 	dev_dbg(i2c->adap.dev.parent, "%s clr: 0x%x\n", __func__, clr);
 
 	xiic_setreg32(i2c, XIIC_IISR_OFFSET, clr);
+	if (xfer_more)
+		__xiic_start_xfer(i2c);
+	if (wakeup_req)
+		xiic_wakeup(i2c, wakeup_code);
+
+	WARN_ON(xfer_more && wakeup_req);
+
 	mutex_unlock(&i2c->lock);
 	return IRQ_HANDLED;
 }
-- 
2.26.2

[PATCH 4/5] i2c: xiic: Switch from waitqueue to completion

[Marek Vasut]

There will never be threads queueing up in the xiic_xmit(), use
completion synchronization primitive to wait for the interrupt
handler thread to complete instead as it is much better fit and
there is no need to overload it for this purpose.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Michal Simek <michal.simek@xilinx.com>
Cc: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
Cc: Wolfram Sang <wsa@kernel.org>
---
 drivers/i2c/busses/i2c-xiic.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 87eca9d46afd9..e4c3427b2f3f5 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -23,7 +23,7 @@
 #include <linux/platform_device.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
-#include <linux/wait.h>
+#include <linux/completion.h>
 #include <linux/platform_data/i2c-xiic.h>
 #include <linux/io.h>
 #include <linux/slab.h>
@@ -48,7 +48,7 @@ enum xiic_endian {
  * struct xiic_i2c - Internal representation of the XIIC I2C bus
  * @dev:	Pointer to device structure
  * @base:	Memory base of the HW registers
- * @wait:	Wait queue for callers
+ * @completion:	Completion for callers
  * @adap:	Kernel adapter representation
  * @tx_msg:	Messages from above to be sent
  * @lock:	Mutual exclusion
@@ -63,7 +63,7 @@ enum xiic_endian {
 struct xiic_i2c {
 	struct device		*dev;
 	void __iomem		*base;
-	wait_queue_head_t	wait;
+	struct completion	completion;
 	struct i2c_adapter	adap;
 	struct i2c_msg		*tx_msg;
 	struct mutex		lock;
@@ -365,7 +365,7 @@ static void xiic_wakeup(struct xiic_i2c *i2c, int code)
 	i2c->rx_msg = NULL;
 	i2c->nmsgs = 0;
 	i2c->state = code;
-	wake_up(&i2c->wait);
+	complete(&i2c->completion);
 }
 
 static irqreturn_t xiic_process(int irq, void *dev_id)
@@ -677,6 +677,7 @@ static int xiic_start_xfer(struct xiic_i2c *i2c, struct i2c_msg *msgs, int num)
 
 	i2c->tx_msg = msgs;
 	i2c->nmsgs = num;
+	init_completion(&i2c->completion);
 
 	ret = xiic_reinit(i2c);
 	if (!ret)
@@ -703,23 +704,24 @@ static int xiic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 	err = xiic_start_xfer(i2c, msgs, num);
 	if (err < 0) {
 		dev_err(adap->dev.parent, "Error xiic_start_xfer\n");
-		goto out;
+		return err;
 	}
 
-	if (wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
-		(i2c->state == STATE_DONE), HZ)) {
-		mutex_lock(&i2c->lock);
-		err = (i2c->state == STATE_DONE) ? num : -EIO;
-		goto out;
-	} else {
-		mutex_lock(&i2c->lock);
+	err = wait_for_completion_interruptible_timeout(&i2c->completion,
+							XIIC_I2C_TIMEOUT);
+	mutex_lock(&i2c->lock);
+	if (err == 0) {	/* Timeout */
 		i2c->tx_msg = NULL;
 		i2c->rx_msg = NULL;
 		i2c->nmsgs = 0;
 		err = -ETIMEDOUT;
-		goto out;
+	} else if (err < 0) {	/* Completion error */
+		i2c->tx_msg = NULL;
+		i2c->rx_msg = NULL;
+		i2c->nmsgs = 0;
+	} else {
+		err = (i2c->state == STATE_DONE) ? num : -EIO;
 	}
-out:
 	mutex_unlock(&i2c->lock);
 	pm_runtime_mark_last_busy(i2c->dev);
 	pm_runtime_put_autosuspend(i2c->dev);
@@ -781,7 +783,6 @@ static int xiic_i2c_probe(struct platform_device *pdev)
 	i2c->adap.dev.of_node = pdev->dev.of_node;
 
 	mutex_init(&i2c->lock);
-	init_waitqueue_head(&i2c->wait);
 
 	i2c->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(i2c->clk)) {
-- 
2.26.2

[PATCH 5/5] i2c: xiic: Only ever transfer single message

[Marek Vasut]

Transferring multiple messages via XIIC suffers from strange interaction
between the interrupt status/enable register flags. These flags are being
reused in the hardware to indicate different things for read and write
transfer, and doing multiple transactions becomes horribly complex. Just
send a single transaction and reload the controller with another message
once the transaction is done in the interrupt handler thread.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Michal Simek <michal.simek@xilinx.com>
Cc: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
Cc: Wolfram Sang <wsa@kernel.org>
---
 drivers/i2c/busses/i2c-xiic.c | 43 ++++++++---------------------------
 1 file changed, 10 insertions(+), 33 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index e4c3427b2f3f5..fad0b84a273d1 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -595,8 +595,6 @@ static void xiic_start_send(struct xiic_i2c *i2c)
 {
 	struct i2c_msg *msg = i2c->tx_msg;
 
-	xiic_irq_clr(i2c, XIIC_INTR_TX_ERROR_MASK);
-
 	dev_dbg(i2c->adap.dev.parent, "%s entry, msg: %p, len: %d",
 		__func__, msg, msg->len);
 	dev_dbg(i2c->adap.dev.parent, "%s entry, ISR: 0x%x, CR: 0x%x\n",
@@ -614,11 +612,13 @@ static void xiic_start_send(struct xiic_i2c *i2c)
 		xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, data);
 	}
 
-	xiic_fill_tx_fifo(i2c);
-
 	/* Clear any pending Tx empty, Tx Error and then enable them. */
 	xiic_irq_clr_en(i2c, XIIC_INTR_TX_EMPTY_MASK | XIIC_INTR_TX_ERROR_MASK |
-		XIIC_INTR_BNB_MASK);
+		XIIC_INTR_BNB_MASK |
+		((i2c->nmsgs > 1 || xiic_tx_space(i2c)) ?
+			XIIC_INTR_TX_HALF_MASK : 0));
+
+	xiic_fill_tx_fifo(i2c);
 }
 
 static void __xiic_start_xfer(struct xiic_i2c *i2c)
@@ -634,35 +634,12 @@ static void __xiic_start_xfer(struct xiic_i2c *i2c)
 	i2c->rx_pos = 0;
 	i2c->tx_pos = 0;
 	i2c->state = STATE_START;
-	while ((fifo_space >= 2) && (first || (i2c->nmsgs > 1))) {
-		if (!first) {
-			i2c->nmsgs--;
-			i2c->tx_msg++;
-			i2c->tx_pos = 0;
-		} else
-			first = 0;
-
-		if (i2c->tx_msg->flags & I2C_M_RD) {
-			/* we dont date putting several reads in the FIFO */
-			xiic_start_recv(i2c);
-			return;
-		} else {
-			xiic_start_send(i2c);
-			if (xiic_tx_space(i2c) != 0) {
-				/* the message could not be completely sent */
-				break;
-			}
-		}
-
-		fifo_space = xiic_tx_fifo_space(i2c);
+	if (i2c->tx_msg->flags & I2C_M_RD) {
+		/* we dont date putting several reads in the FIFO */
+		xiic_start_recv(i2c);
+	} else {
+		xiic_start_send(i2c);
 	}
-
-	/* there are more messages or the current one could not be completely
-	 * put into the FIFO, also enable the half empty interrupt
-	 */
-	if (i2c->nmsgs > 1 || xiic_tx_space(i2c))
-		xiic_irq_clr_en(i2c, XIIC_INTR_TX_HALF_MASK);
-
 }
 
 static int xiic_start_xfer(struct xiic_i2c *i2c, struct i2c_msg *msgs, int num)
-- 
2.26.2

Re: [PATCH 5/5] i2c: xiic: Only ever transfer single message

[Wolfram Sang]

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

Hi Marek,

On Sat, Jun 13, 2020 at 05:07:51PM +0200, Marek Vasut wrote:
> Transferring multiple messages via XIIC suffers from strange interaction
> between the interrupt status/enable register flags. These flags are being
> reused in the hardware to indicate different things for read and write
> transfer, and doing multiple transactions becomes horribly complex. Just
> send a single transaction and reload the controller with another message
> once the transaction is done in the interrupt handler thread.

Do we still get a repeated start between messages of a transfer? Or will
it be a STOP/START combination?

Happy hacking,

   Wolfram


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

Re: [PATCH 5/5] i2c: xiic: Only ever transfer single message

[Marek Vasut]

On 6/13/20 9:33 PM, Wolfram Sang wrote:
> Hi Marek,

Hi,

> On Sat, Jun 13, 2020 at 05:07:51PM +0200, Marek Vasut wrote:
>> Transferring multiple messages via XIIC suffers from strange interaction
>> between the interrupt status/enable register flags. These flags are being
>> reused in the hardware to indicate different things for read and write
>> transfer, and doing multiple transactions becomes horribly complex. Just
>> send a single transaction and reload the controller with another message
>> once the transaction is done in the interrupt handler thread.
> 
> Do we still get a repeated start between messages of a transfer? Or will
> it be a STOP/START combination?

Repeated start.

Re: [PATCH 5/5] i2c: xiic: Only ever transfer single message

[Wolfram Sang]

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


> >> Transferring multiple messages via XIIC suffers from strange interaction
> >> between the interrupt status/enable register flags. These flags are being
> >> reused in the hardware to indicate different things for read and write
> >> transfer, and doing multiple transactions becomes horribly complex. Just
> >> send a single transaction and reload the controller with another message
> >> once the transaction is done in the interrupt handler thread.
> > 
> > Do we still get a repeated start between messages of a transfer? Or will
> > it be a STOP/START combination?
> 
> Repeated start.

Good. That was my high level question. I'll leave the rest of the review
then for Michal.


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

Re: [PATCH 2/5] i2c: xiic: Drop broken interrupt handler

[Shubhrajyoti Datta]

Hi Marek,

On Sat, Jun 13, 2020 at 8:39 PM Marek Vasut <marex@denx.de> wrote:
>
> The interrupt handler is missing locking when reading out registers
> and is racing with other threads which might access the driver. Drop
> it altogether, so that the threaded interrupt is always executed, as
> that one is already serialized by the driver mutex. This also allows
> dropping local_irq_save()/local_irq_restore() in xiic_start_recv().
>
The idea of the local_irq_save / restore was to make it atomic in case
there are a lot
of non i2c interrupts.

so it should still be needed right?

Re: [PATCH 2/5] i2c: xiic: Drop broken interrupt handler

[Marek Vasut]

On 6/17/20 2:25 PM, Shubhrajyoti Datta wrote:
> Hi Marek,

Hi,

> On Sat, Jun 13, 2020 at 8:39 PM Marek Vasut <marex@denx.de> wrote:
>>
>> The interrupt handler is missing locking when reading out registers
>> and is racing with other threads which might access the driver. Drop
>> it altogether, so that the threaded interrupt is always executed, as
>> that one is already serialized by the driver mutex. This also allows
>> dropping local_irq_save()/local_irq_restore() in xiic_start_recv().
>>
> The idea of the local_irq_save / restore was to make it atomic in case
> there are a lot
> of non i2c interrupts.

Make what atomic ? Two consecutive register writes cannot be atomic
unless there is some hardware way to do that. The XIIC has no such way.

> so it should still be needed right?

No, if there is a mutex around both the threaded interrupt handler and
this function, the register accesses will not be interleaved between the
two functions. I think that is what this local_irq_*() was trying to
prevent, right ?

RE: [PATCH 1/5] i2c: xiic: Fix broken locking on tx_msg

[Raviteja Narayanam]

Hi Marek,

Thanks for the patches. I have tested them. Please see comments inline.

> -----Original Message-----
> From: linux-i2c-owner@vger.kernel.org <linux-i2c-owner@vger.kernel.org> On
> Behalf Of Marek Vasut
> Sent: Saturday, June 13, 2020 8:38 PM
> To: linux-i2c@vger.kernel.org
> Cc: Marek Vasut <marex@denx.de>; Michal Simek <michals@xilinx.com>;
> Shubhrajyoti Datta <shubhraj@xilinx.com>; Wolfram Sang <wsa@kernel.org>
> Subject: [PATCH 1/5] i2c: xiic: Fix broken locking on tx_msg
> 
> The tx_msg is set from multiple places, sometimes without locking, which fall
> apart on any SMP system. Only ever access tx_msg inside the driver mutex.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Michal Simek <michal.simek@xilinx.com>
> Cc: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> Cc: Wolfram Sang <wsa@kernel.org>
> ---
>  drivers/i2c/busses/i2c-xiic.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c index
> 90c1c362394d0..0777e577720db 100644
> --- a/drivers/i2c/busses/i2c-xiic.c
> +++ b/drivers/i2c/busses/i2c-xiic.c
> @@ -168,7 +168,7 @@ struct xiic_i2c {
>  #define xiic_tx_space(i2c) ((i2c)->tx_msg->len - (i2c)->tx_pos)  #define
> xiic_rx_space(i2c) ((i2c)->rx_msg->len - (i2c)->rx_pos)
> 
> -static int xiic_start_xfer(struct xiic_i2c *i2c);
> +static int xiic_start_xfer(struct xiic_i2c *i2c, struct i2c_msg *msgs,
> +int num);
>  static void __xiic_start_xfer(struct xiic_i2c *i2c);
> 
>  /*
> @@ -673,15 +673,24 @@ static void __xiic_start_xfer(struct xiic_i2c *i2c)
> 
>  }
> 
> -static int xiic_start_xfer(struct xiic_i2c *i2c)
> +static int xiic_start_xfer(struct xiic_i2c *i2c, struct i2c_msg *msgs,
> +int num)
>  {
>  	int ret;
> +
>  	mutex_lock(&i2c->lock);
> 
> +	ret = xiic_busy(i2c);
> +	if (ret)
> +		goto out;
> +
> +	i2c->tx_msg = msgs;
> +	i2c->nmsgs = num;
> +
>  	ret = xiic_reinit(i2c);
>  	if (!ret)
>  		__xiic_start_xfer(i2c);
> 
> +out:
>  	mutex_unlock(&i2c->lock);
> 
>  	return ret;
> @@ -699,14 +708,7 @@ static int xiic_xfer(struct i2c_adapter *adap, struct
> i2c_msg *msgs, int num)
>  	if (err < 0)
>  		return err;
> 
> -	err = xiic_busy(i2c);
> -	if (err)
> -		goto out;
> -
> -	i2c->tx_msg = msgs;
> -	i2c->nmsgs = num;

On an SMP system with multiple i2c-transfer command scripts running, the above critical section is expected to cause serious trouble overwriting the previous msg pointers.
But that's not happening as the i2c-core is having a lock at adapter level inside i2c-core-base.c (rt_mutex_lock_nested).
So, the race condition between different threads is not possible. They are all serialized by the locking in i2c-core.

Although no issues are seen in the tests, the contention within the driver is still possible (isr vs xiic_xfer), if there is a spurious interrupt. And this patch is needed in that case.

> -
> -	err = xiic_start_xfer(i2c);
> +	err = xiic_start_xfer(i2c, msgs, num);
>  	if (err < 0) {
>  		dev_err(adap->dev.parent, "Error xiic_start_xfer\n");
>  		goto out;
> @@ -714,9 +716,11 @@ static int xiic_xfer(struct i2c_adapter *adap, struct
> i2c_msg *msgs, int num)
> 
>  	if (wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
>  		(i2c->state == STATE_DONE), HZ)) {
> +		mutex_lock(&i2c->lock);
>  		err = (i2c->state == STATE_DONE) ? num : -EIO;
>  		goto out;
>  	} else {
> +		mutex_lock(&i2c->lock);
>  		i2c->tx_msg = NULL;
>  		i2c->rx_msg = NULL;
>  		i2c->nmsgs = 0;
> @@ -724,6 +728,7 @@ static int xiic_xfer(struct i2c_adapter *adap, struct
> i2c_msg *msgs, int num)
>  		goto out;
>  	}
>  out:
> +	mutex_unlock(&i2c->lock);
>  	pm_runtime_mark_last_busy(i2c->dev);
>  	pm_runtime_put_autosuspend(i2c->dev);
>  	return err;

Raviteja N

RE: [PATCH 2/5] i2c: xiic: Drop broken interrupt handler

[Raviteja Narayanam]

> -----Original Message-----
> From: linux-i2c-owner@vger.kernel.org <linux-i2c-owner@vger.kernel.org> On
> Behalf Of Marek Vasut
> Sent: Saturday, June 13, 2020 8:38 PM
> To: linux-i2c@vger.kernel.org
> Cc: Marek Vasut <marex@denx.de>; Michal Simek <michals@xilinx.com>;
> Shubhrajyoti Datta <shubhraj@xilinx.com>; Wolfram Sang <wsa@kernel.org>
> Subject: [PATCH 2/5] i2c: xiic: Drop broken interrupt handler
> 
> The interrupt handler is missing locking when reading out registers and is
> racing with other threads which might access the driver. Drop it altogether, so
> that the threaded interrupt is always executed, as that one is already serialized
> by the driver mutex. This also allows dropping
> local_irq_save()/local_irq_restore() in xiic_start_recv().
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Michal Simek <michal.simek@xilinx.com>
> Cc: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> Cc: Wolfram Sang <wsa@kernel.org>
> ---
>  drivers/i2c/busses/i2c-xiic.c | 25 +------------------------
>  1 file changed, 1 insertion(+), 24 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c index
> 0777e577720db..6db71c0fb6583 100644
> --- a/drivers/i2c/busses/i2c-xiic.c
> +++ b/drivers/i2c/busses/i2c-xiic.c
> @@ -543,7 +543,6 @@ static void xiic_start_recv(struct xiic_i2c *i2c)  {
>  	u8 rx_watermark;
>  	struct i2c_msg *msg = i2c->rx_msg = i2c->tx_msg;
> -	unsigned long flags;
> 
>  	/* Clear and enable Rx full interrupt. */
>  	xiic_irq_clr_en(i2c, XIIC_INTR_RX_FULL_MASK |
> XIIC_INTR_TX_ERROR_MASK); @@ -559,7 +558,6 @@ static void
> xiic_start_recv(struct xiic_i2c *i2c)
>  		rx_watermark = IIC_RX_FIFO_DEPTH;
>  	xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET, rx_watermark - 1);
> 
> -	local_irq_save(flags);

It is added as part of (i2c: xiic: Make the start and the byte count write atomic - ae7304c3ea28a3ba47a7a8312c76c654ef24967e) commit
to make the below 2 register writes atomic so that the controller doesn't produce a wrong transaction. (If there is a delay between the 2 register
writes, controller is messing up with the transaction). It is not intended for locking between this function and isr. So, this can't be removed. 

>  	if (!(msg->flags & I2C_M_NOSTART))
>  		/* write the address */
>  		xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, @@ -569,7 +567,6
> @@ static void xiic_start_recv(struct xiic_i2c *i2c)
> 
>  	xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET,
>  		msg->len | ((i2c->nmsgs == 1) ? XIIC_TX_DYN_STOP_MASK :
> 0));
> -	local_irq_restore(flags);
> 
>  	if (i2c->nmsgs == 1)
>  		/* very last, enable bus not busy as well */ @@ -609,26 +606,6
> @@ static void xiic_start_send(struct xiic_i2c *i2c)
>  		XIIC_INTR_BNB_MASK);
>  }
> 
> -static irqreturn_t xiic_isr(int irq, void *dev_id) -{
> -	struct xiic_i2c *i2c = dev_id;
> -	u32 pend, isr, ier;
> -	irqreturn_t ret = IRQ_NONE;
> -	/* Do not processes a devices interrupts if the device has no
> -	 * interrupts pending
> -	 */
> -
> -	dev_dbg(i2c->adap.dev.parent, "%s entry\n", __func__);
> -
> -	isr = xiic_getreg32(i2c, XIIC_IISR_OFFSET);
> -	ier = xiic_getreg32(i2c, XIIC_IIER_OFFSET);
> -	pend = isr & ier;
> -	if (pend)
> -		ret = IRQ_WAKE_THREAD;
> -
> -	return ret;
> -}
> -
>  static void __xiic_start_xfer(struct xiic_i2c *i2c)  {
>  	int first = 1;
> @@ -807,7 +784,7 @@ static int xiic_i2c_probe(struct platform_device *pdev)
>  	pm_runtime_use_autosuspend(i2c->dev);
>  	pm_runtime_set_active(i2c->dev);
>  	pm_runtime_enable(i2c->dev);
> -	ret = devm_request_threaded_irq(&pdev->dev, irq, xiic_isr,
> +	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
>  					xiic_process, IRQF_ONESHOT,
>  					pdev->name, i2c);
> 
Removal of isr is fine.

Raviteja N

RE: [PATCH 3/5] i2c: xiic: Defer xiic_wakeup() and __xiic_start_xfer() in xiic_process()

[Raviteja Narayanam]

> -----Original Message-----
> From: linux-i2c-owner@vger.kernel.org <linux-i2c-owner@vger.kernel.org> On
> Behalf Of Marek Vasut
> Sent: Saturday, June 13, 2020 8:38 PM
> To: linux-i2c@vger.kernel.org
> Cc: Marek Vasut <marex@denx.de>; Michal Simek <michals@xilinx.com>;
> Shubhrajyoti Datta <shubhraj@xilinx.com>; Wolfram Sang <wsa@kernel.org>
> Subject: [PATCH 3/5] i2c: xiic: Defer xiic_wakeup() and __xiic_start_xfer() in
> xiic_process()
> 
> The __xiic_start_xfer() manipulates the interrupt flags, xiic_wakeup() may
> result in return from xiic_xfer() early. Defer both to the end of the
> xiic_process() interrupt thread, so that they are executed after all the other
> interrupt bits handling completed and once it completely safe to perform
> changes to the interrupt bits in the hardware.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Michal Simek <michal.simek@xilinx.com>
> Cc: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> Cc: Wolfram Sang <wsa@kernel.org>
> ---
>  drivers/i2c/busses/i2c-xiic.c | 37 ++++++++++++++++++++++++-----------
>  1 file changed, 26 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c index
> 6db71c0fb6583..87eca9d46afd9 100644
> --- a/drivers/i2c/busses/i2c-xiic.c
> +++ b/drivers/i2c/busses/i2c-xiic.c
> @@ -373,6 +373,9 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
>  	struct xiic_i2c *i2c = dev_id;
>  	u32 pend, isr, ier;
>  	u32 clr = 0;
> +	int xfer_more = 0;
> +	int wakeup_req = 0;
> +	int wakeup_code = 0;
> 
>  	/* Get the interrupt Status from the IPIF. There is no clearing of
>  	 * interrupts in the IPIF. Interrupts must be cleared at the source.
> @@ -409,10 +412,14 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
>  		 */
>  		xiic_reinit(i2c);
> 
> -		if (i2c->rx_msg)
> -			xiic_wakeup(i2c, STATE_ERROR);
> -		if (i2c->tx_msg)
> -			xiic_wakeup(i2c, STATE_ERROR);
> +		if (i2c->rx_msg) {
> +			wakeup_req = 1;
> +			wakeup_code = STATE_ERROR;
> +		}
> +		if (i2c->tx_msg) {
> +			wakeup_req = 1;
> +			wakeup_code = STATE_ERROR;
> +		}
>  	}
>  	if (pend & XIIC_INTR_RX_FULL_MASK) {
>  		/* Receive register/FIFO is full */
> @@ -446,8 +453,7 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
>  				i2c->tx_msg++;
>  				dev_dbg(i2c->adap.dev.parent,
>  					"%s will start next...\n", __func__);
> -
> -				__xiic_start_xfer(i2c);
> +				xfer_more = 1;
>  			}
>  		}
>  	}
> @@ -461,11 +467,13 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
>  		if (!i2c->tx_msg)
>  			goto out;
> 
> -		if ((i2c->nmsgs == 1) && !i2c->rx_msg &&
> -			xiic_tx_space(i2c) == 0)
> -			xiic_wakeup(i2c, STATE_DONE);
> +		wakeup_req = 1;
> +
> +		if (i2c->nmsgs == 1 && !i2c->rx_msg &&
> +		    xiic_tx_space(i2c) == 0)
> +			wakeup_code = STATE_DONE;
>  		else
> -			xiic_wakeup(i2c, STATE_ERROR);
> +			wakeup_code = STATE_ERROR;
>  	}
>  	if (pend & (XIIC_INTR_TX_EMPTY_MASK | XIIC_INTR_TX_HALF_MASK))
> {
>  		/* Transmit register/FIFO is empty or ½ empty */ @@ -489,7
> +497,7 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
>  			if (i2c->nmsgs > 1) {
>  				i2c->nmsgs--;
>  				i2c->tx_msg++;
> -				__xiic_start_xfer(i2c);
> +				xfer_more = 1;
>  			} else {
>  				xiic_irq_dis(i2c, XIIC_INTR_TX_HALF_MASK);
> 
> @@ -507,6 +515,13 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
>  	dev_dbg(i2c->adap.dev.parent, "%s clr: 0x%x\n", __func__, clr);
> 
>  	xiic_setreg32(i2c, XIIC_IISR_OFFSET, clr);
> +	if (xfer_more)
> +		__xiic_start_xfer(i2c);
> +	if (wakeup_req)
> +		xiic_wakeup(i2c, wakeup_code);
> +
> +	WARN_ON(xfer_more && wakeup_req);
> +
>  	mutex_unlock(&i2c->lock);
>  	return IRQ_HANDLED;
>  }

This is tested and working fine.

Raviteja N

RE: [PATCH 4/5] i2c: xiic: Switch from waitqueue to completion

[Raviteja Narayanam]

> -----Original Message-----
> From: linux-i2c-owner@vger.kernel.org <linux-i2c-owner@vger.kernel.org> On
> Behalf Of Marek Vasut
> Sent: Saturday, June 13, 2020 8:38 PM
> To: linux-i2c@vger.kernel.org
> Cc: Marek Vasut <marex@denx.de>; Michal Simek <michals@xilinx.com>;
> Shubhrajyoti Datta <shubhraj@xilinx.com>; Wolfram Sang <wsa@kernel.org>
> Subject: [PATCH 4/5] i2c: xiic: Switch from waitqueue to completion
> 
> There will never be threads queueing up in the xiic_xmit(), use completion
> synchronization primitive to wait for the interrupt handler thread to complete
> instead as it is much better fit and there is no need to overload it for this
> purpose.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Michal Simek <michal.simek@xilinx.com>
> Cc: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> Cc: Wolfram Sang <wsa@kernel.org>
> ---
>  drivers/i2c/busses/i2c-xiic.c | 31 ++++++++++++++++---------------
>  1 file changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c index
> 87eca9d46afd9..e4c3427b2f3f5 100644
> --- a/drivers/i2c/busses/i2c-xiic.c
> +++ b/drivers/i2c/busses/i2c-xiic.c
> @@ -23,7 +23,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/i2c.h>
>  #include <linux/interrupt.h>
> -#include <linux/wait.h>
> +#include <linux/completion.h>
>  #include <linux/platform_data/i2c-xiic.h>  #include <linux/io.h>  #include
> <linux/slab.h> @@ -48,7 +48,7 @@ enum xiic_endian {
>   * struct xiic_i2c - Internal representation of the XIIC I2C bus
>   * @dev:	Pointer to device structure
>   * @base:	Memory base of the HW registers
> - * @wait:	Wait queue for callers
> + * @completion:	Completion for callers
>   * @adap:	Kernel adapter representation
>   * @tx_msg:	Messages from above to be sent
>   * @lock:	Mutual exclusion
> @@ -63,7 +63,7 @@ enum xiic_endian {
>  struct xiic_i2c {
>  	struct device		*dev;
>  	void __iomem		*base;
> -	wait_queue_head_t	wait;
> +	struct completion	completion;
>  	struct i2c_adapter	adap;
>  	struct i2c_msg		*tx_msg;
>  	struct mutex		lock;
> @@ -365,7 +365,7 @@ static void xiic_wakeup(struct xiic_i2c *i2c, int code)
>  	i2c->rx_msg = NULL;
>  	i2c->nmsgs = 0;
>  	i2c->state = code;
> -	wake_up(&i2c->wait);
> +	complete(&i2c->completion);
>  }
> 
>  static irqreturn_t xiic_process(int irq, void *dev_id) @@ -677,6 +677,7 @@
> static int xiic_start_xfer(struct xiic_i2c *i2c, struct i2c_msg *msgs, int num)
> 
>  	i2c->tx_msg = msgs;
>  	i2c->nmsgs = num;
> +	init_completion(&i2c->completion);
> 
>  	ret = xiic_reinit(i2c);
>  	if (!ret)
> @@ -703,23 +704,24 @@ static int xiic_xfer(struct i2c_adapter *adap, struct
> i2c_msg *msgs, int num)
>  	err = xiic_start_xfer(i2c, msgs, num);
>  	if (err < 0) {
>  		dev_err(adap->dev.parent, "Error xiic_start_xfer\n");
> -		goto out;
> +		return err;
>  	}
> 
> -	if (wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
> -		(i2c->state == STATE_DONE), HZ)) {
> -		mutex_lock(&i2c->lock);
> -		err = (i2c->state == STATE_DONE) ? num : -EIO;
> -		goto out;
> -	} else {
> -		mutex_lock(&i2c->lock);
> +	err = wait_for_completion_interruptible_timeout(&i2c->completion,
> +							XIIC_I2C_TIMEOUT);

This is causing random lock up of bus. Since it is "interruptible" API,  once we enter Ctrl+C, 
It is coming out of wait state, the message pointers are made NULL and the ongoing transaction is not completed. 
Can use " wait_for_completion_timeout" API in place of this.

> +	mutex_lock(&i2c->lock);
> +	if (err == 0) {	/* Timeout */
>  		i2c->tx_msg = NULL;
>  		i2c->rx_msg = NULL;
>  		i2c->nmsgs = 0;
>  		err = -ETIMEDOUT;
> -		goto out;
> +	} else if (err < 0) {	/* Completion error */
> +		i2c->tx_msg = NULL;
> +		i2c->rx_msg = NULL;
> +		i2c->nmsgs = 0;
> +	} else {
> +		err = (i2c->state == STATE_DONE) ? num : -EIO;
>  	}
> -out:
>  	mutex_unlock(&i2c->lock);
>  	pm_runtime_mark_last_busy(i2c->dev);
>  	pm_runtime_put_autosuspend(i2c->dev);
> @@ -781,7 +783,6 @@ static int xiic_i2c_probe(struct platform_device *pdev)
>  	i2c->adap.dev.of_node = pdev->dev.of_node;
> 
>  	mutex_init(&i2c->lock);
> -	init_waitqueue_head(&i2c->wait);
> 
>  	i2c->clk = devm_clk_get(&pdev->dev, NULL);
>  	if (IS_ERR(i2c->clk)) {

Raviteja N

RE: [PATCH 5/5] i2c: xiic: Only ever transfer single message

[Raviteja Narayanam]

> -----Original Message-----
> From: linux-i2c-owner@vger.kernel.org <linux-i2c-owner@vger.kernel.org> On
> Behalf Of Marek Vasut
> Sent: Saturday, June 13, 2020 8:38 PM
> To: linux-i2c@vger.kernel.org
> Cc: Marek Vasut <marex@denx.de>; Michal Simek <michals@xilinx.com>;
> Shubhrajyoti Datta <shubhraj@xilinx.com>; Wolfram Sang <wsa@kernel.org>
> Subject: [PATCH 5/5] i2c: xiic: Only ever transfer single message
> 
> Transferring multiple messages via XIIC suffers from strange interaction
> between the interrupt status/enable register flags. These flags are being reused
> in the hardware to indicate different things for read and write transfer, and
> doing multiple transactions becomes horribly complex. Just send a single
> transaction and reload the controller with another message once the
> transaction is done in the interrupt handler thread.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Michal Simek <michal.simek@xilinx.com>
> Cc: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> Cc: Wolfram Sang <wsa@kernel.org>
> ---
>  drivers/i2c/busses/i2c-xiic.c | 43 ++++++++---------------------------
>  1 file changed, 10 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c index
> e4c3427b2f3f5..fad0b84a273d1 100644
> --- a/drivers/i2c/busses/i2c-xiic.c
> +++ b/drivers/i2c/busses/i2c-xiic.c
> @@ -595,8 +595,6 @@ static void xiic_start_send(struct xiic_i2c *i2c)  {
>  	struct i2c_msg *msg = i2c->tx_msg;
> 
> -	xiic_irq_clr(i2c, XIIC_INTR_TX_ERROR_MASK);
> -
>  	dev_dbg(i2c->adap.dev.parent, "%s entry, msg: %p, len: %d",
>  		__func__, msg, msg->len);
>  	dev_dbg(i2c->adap.dev.parent, "%s entry, ISR: 0x%x, CR: 0x%x\n", @@
> -614,11 +612,13 @@ static void xiic_start_send(struct xiic_i2c *i2c)
>  		xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, data);
>  	}
> 
> -	xiic_fill_tx_fifo(i2c);
> -
>  	/* Clear any pending Tx empty, Tx Error and then enable them. */
>  	xiic_irq_clr_en(i2c, XIIC_INTR_TX_EMPTY_MASK |
> XIIC_INTR_TX_ERROR_MASK |
> -		XIIC_INTR_BNB_MASK);
> +		XIIC_INTR_BNB_MASK |
> +		((i2c->nmsgs > 1 || xiic_tx_space(i2c)) ?
> +			XIIC_INTR_TX_HALF_MASK : 0));
> +
> +	xiic_fill_tx_fifo(i2c);
>  }
> 
>  static void __xiic_start_xfer(struct xiic_i2c *i2c) @@ -634,35 +634,12 @@
> static void __xiic_start_xfer(struct xiic_i2c *i2c)

Can remove the definition of unused variable ("first").

>  	i2c->rx_pos = 0;
>  	i2c->tx_pos = 0;
>  	i2c->state = STATE_START;
> -	while ((fifo_space >= 2) && (first || (i2c->nmsgs > 1))) {
> -		if (!first) {
> -			i2c->nmsgs--;
> -			i2c->tx_msg++;
> -			i2c->tx_pos = 0;
> -		} else
> -			first = 0;
> -
> -		if (i2c->tx_msg->flags & I2C_M_RD) {
> -			/* we dont date putting several reads in the FIFO */
> -			xiic_start_recv(i2c);
> -			return;
> -		} else {
> -			xiic_start_send(i2c);
> -			if (xiic_tx_space(i2c) != 0) {
> -				/* the message could not be completely sent
> */
> -				break;
> -			}
> -		}
> -
> -		fifo_space = xiic_tx_fifo_space(i2c);
> +	if (i2c->tx_msg->flags & I2C_M_RD) {
> +		/* we dont date putting several reads in the FIFO */
> +		xiic_start_recv(i2c);
> +	} else {
> +		xiic_start_send(i2c);
>  	}
> -
> -	/* there are more messages or the current one could not be completely
> -	 * put into the FIFO, also enable the half empty interrupt
> -	 */
> -	if (i2c->nmsgs > 1 || xiic_tx_space(i2c))
> -		xiic_irq_clr_en(i2c, XIIC_INTR_TX_HALF_MASK);
> -
>  }
> 
>  static int xiic_start_xfer(struct xiic_i2c *i2c, struct i2c_msg *msgs, int num)

This patch is much needed to simplify the logic. Tested, working fine.

Raviteja N

Re: [PATCH 1/5] i2c: xiic: Fix broken locking on tx_msg

[Marek Vasut]

On 6/26/20 2:11 PM, Raviteja Narayanam wrote:

Hi,

[...]

>> @@ -699,14 +708,7 @@ static int xiic_xfer(struct i2c_adapter *adap, struct
>> i2c_msg *msgs, int num)
>>  	if (err < 0)
>>  		return err;
>>
>> -	err = xiic_busy(i2c);
>> -	if (err)
>> -		goto out;
>> -
>> -	i2c->tx_msg = msgs;
>> -	i2c->nmsgs = num;
> 
> On an SMP system with multiple i2c-transfer command scripts running, the above critical section is expected to cause serious trouble overwriting the previous msg pointers.
> But that's not happening as the i2c-core is having a lock at adapter level inside i2c-core-base.c (rt_mutex_lock_nested).
> So, the race condition between different threads is not possible. They are all serialized by the locking in i2c-core.
> 
> Although no issues are seen in the tests, the contention within the driver is still possible (isr vs xiic_xfer), if there is a spurious interrupt. And this patch is needed in that case.
The contention happens between the threaded interrupt handler
xiic_process() and this xiic_xfer() function. While you can not have
xiic_xfer() running on two CPU cores at the same time, you can have
xiic_xfer() running on one CPU core and xiic_process() on another CPU
core, and that will lead to problems.

[...]

Re: [PATCH 2/5] i2c: xiic: Drop broken interrupt handler

[Marek Vasut]

On 6/26/20 2:13 PM, Raviteja Narayanam wrote:

Hi,

[...]

>> diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c index
>> 0777e577720db..6db71c0fb6583 100644
>> --- a/drivers/i2c/busses/i2c-xiic.c
>> +++ b/drivers/i2c/busses/i2c-xiic.c
>> @@ -543,7 +543,6 @@ static void xiic_start_recv(struct xiic_i2c *i2c)  {
>>  	u8 rx_watermark;
>>  	struct i2c_msg *msg = i2c->rx_msg = i2c->tx_msg;
>> -	unsigned long flags;
>>
>>  	/* Clear and enable Rx full interrupt. */
>>  	xiic_irq_clr_en(i2c, XIIC_INTR_RX_FULL_MASK |
>> XIIC_INTR_TX_ERROR_MASK); @@ -559,7 +558,6 @@ static void
>> xiic_start_recv(struct xiic_i2c *i2c)
>>  		rx_watermark = IIC_RX_FIFO_DEPTH;
>>  	xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET, rx_watermark - 1);
>>
>> -	local_irq_save(flags);
> 
> It is added as part of (i2c: xiic: Make the start and the byte count write atomic - ae7304c3ea28a3ba47a7a8312c76c654ef24967e) commit
> to make the below 2 register writes atomic so that the controller doesn't produce a wrong transaction.

Two consecutive register writes are not atomic, they are posted as two
consecutive writes on the bus and will reach the hardware IP as two
separate writes with arbitrary delay between them.

I think the intention of the patch ae7304c3ea28 ("i2c: xiic: Make the
start and the byte count write atomic") is to make sure that there will
be no read/write register access to the XIIC IP while those registers in
local_irq_save()/local_irq_restore() block are written, and that makes
sense.

However, local_irq_save()/local_irq_restore() is local to one CPU core,
it does not prevent another CPU core from posting register read/write
access to the XIIC IP (for example from xiic_process() threaded
interrupt handler, which might just be running on that other CPU core).

The &i2c->lock mutex is locked by both the xiic_start() and
xiic_process(), and therefore guarantees that no other thread can post
read/write register access to the XIIC IP while xiic_start() (and thus
xiic_recv_start(), which is called from it) is running.

And so, I think this local_irq_save()/local_irq_restore() can be removed.

> (If there is a delay between the 2 register
> writes, controller is messing up with the transaction). It is not intended for locking between this function and isr. So, this can't be removed. 

But the bus can insert arbitrary delay between two consecutive register
writes to the XIIC IP, so if the timing between the two register writes
is really critical, then I don't see how to guarantee the timing
requirement. Do you happen to have more details on what really happens
on the hardware level here , maybe some errata document for the XIIC IP?

Thanks

>>  	if (!(msg->flags & I2C_M_NOSTART))
>>  		/* write the address */
>>  		xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, @@ -569,7 +567,6

[...]

Re: [PATCH 4/5] i2c: xiic: Switch from waitqueue to completion

[Marek Vasut]

On 6/26/20 2:13 PM, Raviteja Narayanam wrote:

Hi,

[...]

>> @@ -703,23 +704,24 @@ static int xiic_xfer(struct i2c_adapter *adap, struct
>> i2c_msg *msgs, int num)
>>  	err = xiic_start_xfer(i2c, msgs, num);
>>  	if (err < 0) {
>>  		dev_err(adap->dev.parent, "Error xiic_start_xfer\n");
>> -		goto out;
>> +		return err;
>>  	}
>>
>> -	if (wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
>> -		(i2c->state == STATE_DONE), HZ)) {
>> -		mutex_lock(&i2c->lock);
>> -		err = (i2c->state == STATE_DONE) ? num : -EIO;
>> -		goto out;
>> -	} else {
>> -		mutex_lock(&i2c->lock);
>> +	err = wait_for_completion_interruptible_timeout(&i2c->completion,
>> +							XIIC_I2C_TIMEOUT);
> 
> This is causing random lock up of bus. Since it is "interruptible" API,  once we enter Ctrl+C, 
> It is coming out of wait state, the message pointers are made NULL and the ongoing transaction is not completed. 
> Can use " wait_for_completion_timeout" API in place of this.
> 
>> +	mutex_lock(&i2c->lock);

So in case this is interrupted, we would have to somehow reset the
controller ? What sort of lockups do you see exactly ? Can you share
some sort of test case, so I can replicate it ?

Thanks

[...]

RE: [PATCH 1/5] i2c: xiic: Fix broken locking on tx_msg

[Raviteja Narayanam]

> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Monday, June 29, 2020 4:49 AM
> To: Raviteja Narayanam <rna@xilinx.com>; linux-i2c@vger.kernel.org
> Cc: Michal Simek <michals@xilinx.com>; Shubhrajyoti Datta
> <shubhraj@xilinx.com>; Wolfram Sang <wsa@kernel.org>
> Subject: Re: [PATCH 1/5] i2c: xiic: Fix broken locking on tx_msg
> 
> On 6/26/20 2:11 PM, Raviteja Narayanam wrote:
> 
> Hi,
> 
> [...]
> 
> >> @@ -699,14 +708,7 @@ static int xiic_xfer(struct i2c_adapter *adap,
> >> struct i2c_msg *msgs, int num)
> >>  	if (err < 0)
> >>  		return err;
> >>
> >> -	err = xiic_busy(i2c);
> >> -	if (err)
> >> -		goto out;
> >> -
> >> -	i2c->tx_msg = msgs;
> >> -	i2c->nmsgs = num;
> >
> > On an SMP system with multiple i2c-transfer command scripts running, the
> above critical section is expected to cause serious trouble overwriting the
> previous msg pointers.
> > But that's not happening as the i2c-core is having a lock at adapter level
> inside i2c-core-base.c (rt_mutex_lock_nested).
> > So, the race condition between different threads is not possible. They are all
> serialized by the locking in i2c-core.
> >
> > Although no issues are seen in the tests, the contention within the driver is
> still possible (isr vs xiic_xfer), if there is a spurious interrupt. And this patch is
> needed in that case.
> The contention happens between the threaded interrupt handler
> xiic_process() and this xiic_xfer() function. While you can not have
> xiic_xfer() running on two CPU cores at the same time, you can have
> xiic_xfer() running on one CPU core and xiic_process() on another CPU core,
> and that will lead to problems.

Yes, this patch is needed for that case.

> 
> [...]

RE: [PATCH 2/5] i2c: xiic: Drop broken interrupt handler

[Raviteja Narayanam]

> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Monday, June 29, 2020 5:11 AM
> To: Raviteja Narayanam <rna@xilinx.com>; linux-i2c@vger.kernel.org
> Cc: Michal Simek <michals@xilinx.com>; Shubhrajyoti Datta
> <shubhraj@xilinx.com>; Wolfram Sang <wsa@kernel.org>
> Subject: Re: [PATCH 2/5] i2c: xiic: Drop broken interrupt handler
> 
> On 6/26/20 2:13 PM, Raviteja Narayanam wrote:
> 
> Hi,
> 
> [...]
> 
> >> diff --git a/drivers/i2c/busses/i2c-xiic.c
> >> b/drivers/i2c/busses/i2c-xiic.c index
> >> 0777e577720db..6db71c0fb6583 100644
> >> --- a/drivers/i2c/busses/i2c-xiic.c
> >> +++ b/drivers/i2c/busses/i2c-xiic.c
> >> @@ -543,7 +543,6 @@ static void xiic_start_recv(struct xiic_i2c *i2c)  {
> >>  	u8 rx_watermark;
> >>  	struct i2c_msg *msg = i2c->rx_msg = i2c->tx_msg;
> >> -	unsigned long flags;
> >>
> >>  	/* Clear and enable Rx full interrupt. */
> >>  	xiic_irq_clr_en(i2c, XIIC_INTR_RX_FULL_MASK |
> >> XIIC_INTR_TX_ERROR_MASK); @@ -559,7 +558,6 @@ static void
> >> xiic_start_recv(struct xiic_i2c *i2c)
> >>  		rx_watermark = IIC_RX_FIFO_DEPTH;
> >>  	xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET, rx_watermark - 1);
> >>
> >> -	local_irq_save(flags);
> >
> > It is added as part of (i2c: xiic: Make the start and the byte count
> > write atomic - ae7304c3ea28a3ba47a7a8312c76c654ef24967e) commit to
> make the below 2 register writes atomic so that the controller doesn't produce
> a wrong transaction.
> 
> Two consecutive register writes are not atomic, they are posted as two
> consecutive writes on the bus and will reach the hardware IP as two separate
> writes with arbitrary delay between them.
> 
> I think the intention of the patch ae7304c3ea28 ("i2c: xiic: Make the start and
> the byte count write atomic") is to make sure that there will be no read/write
> register access to the XIIC IP while those registers in
> local_irq_save()/local_irq_restore() block are written, and that makes sense.
> 
> However, local_irq_save()/local_irq_restore() is local to one CPU core, it does
> not prevent another CPU core from posting register read/write access to the
> XIIC IP (for example from xiic_process() threaded interrupt handler, which
> might just be running on that other CPU core).
> 
> The &i2c->lock mutex is locked by both the xiic_start() and xiic_process(), and
> therefore guarantees that no other thread can post read/write register access
> to the XIIC IP while xiic_start() (and thus xiic_recv_start(), which is called from
> it) is running.
> 
> And so, I think this local_irq_save()/local_irq_restore() can be removed.
> 
> > (If there is a delay between the 2 register writes, controller is
> > messing up with the transaction). It is not intended for locking between this
> function and isr. So, this can't be removed.
> 
> But the bus can insert arbitrary delay between two consecutive register writes
> to the XIIC IP, so if the timing between the two register writes is really critical,
> then I don't see how to guarantee the timing requirement. Do you happen to
> have more details on what really happens on the hardware level here , maybe
> some errata document for the XIIC IP?

From the commit description of "i2c: xiic: Make the start and the byte count write atomic"(ae7304c3ea28a3ba47a7a8312c76c654ef24967e),

[Disable interrupts while configuring the transfer and enable them back.

We have below as the programming sequence
1. start and slave address
2. byte count and stop

In some customer platform there was a lot of interrupts between 1 and 2
and after slave address (around 7 clock cyles) if 2 is not executed
then the transaction is nacked.

To fix this case make the 2 writes atomic.]

So, this local_irq_save()/local_irq_restore() is not related to exclusive access in the driver (xiic_process vs xiic_start),
but to supply the byte count to controller before it completes transmitting START and slave address.

> 
> Thanks
> 
> >>  	if (!(msg->flags & I2C_M_NOSTART))
> >>  		/* write the address */
> >>  		xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, @@ -569,7 +567,6
> 
> [...]

RE: [PATCH 4/5] i2c: xiic: Switch from waitqueue to completion

[Raviteja Narayanam]

> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Monday, June 29, 2020 5:11 AM
> To: Raviteja Narayanam <rna@xilinx.com>; linux-i2c@vger.kernel.org
> Cc: Michal Simek <michals@xilinx.com>; Shubhrajyoti Datta
> <shubhraj@xilinx.com>; Wolfram Sang <wsa@kernel.org>
> Subject: Re: [PATCH 4/5] i2c: xiic: Switch from waitqueue to completion
> 
> On 6/26/20 2:13 PM, Raviteja Narayanam wrote:
> 
> Hi,
> 
> [...]
> 
> >> @@ -703,23 +704,24 @@ static int xiic_xfer(struct i2c_adapter *adap,
> >> struct i2c_msg *msgs, int num)
> >>  	err = xiic_start_xfer(i2c, msgs, num);
> >>  	if (err < 0) {
> >>  		dev_err(adap->dev.parent, "Error xiic_start_xfer\n");
> >> -		goto out;
> >> +		return err;
> >>  	}
> >>
> >> -	if (wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
> >> -		(i2c->state == STATE_DONE), HZ)) {
> >> -		mutex_lock(&i2c->lock);
> >> -		err = (i2c->state == STATE_DONE) ? num : -EIO;
> >> -		goto out;
> >> -	} else {
> >> -		mutex_lock(&i2c->lock);
> >> +	err = wait_for_completion_interruptible_timeout(&i2c->completion,
> >> +							XIIC_I2C_TIMEOUT);
> >
> > This is causing random lock up of bus. Since it is "interruptible"
> > API,  once we enter Ctrl+C, It is coming out of wait state, the message
> pointers are made NULL and the ongoing transaction is not completed.
> > Can use " wait_for_completion_timeout" API in place of this.
> >
> >> +	mutex_lock(&i2c->lock);
> 
> So in case this is interrupted, we would have to somehow reset the controller ?

Yes, but the cleanup is not straight forward because we do not know the exact state
Of controller and the I2C bus. That’s why preferring a non-interruptible API.

> What sort of lockups do you see exactly ? 

There is an i2ctransfer going on (let's say a read of 255 bytes), we get interrupt everytime the Rx FIFO is full.
While the controller is receiving data, if there is a signal, this is exiting abruptly and there is no STOP condition
on the bus. So, no master can communicate on that bus further. 

Can you share some sort of test case,
> so I can replicate it ?

I have 3 scripts running with commands like below, and I am randomly giving Ctrl+C.

i=0
while [ 1 ]
do
		i2ctransfer -y -f 0 w1@0X54 0X00 r255@0X54
		i=$(expr $i + 1)
        echo "$i"
done

> 
> Thanks
> 
> [...]

Raviteja N

Re: [PATCH 2/5] i2c: xiic: Drop broken interrupt handler

[Marek Vasut]

On 6/29/20 2:52 PM, Raviteja Narayanam wrote:

Hi,

[...]

>>>> diff --git a/drivers/i2c/busses/i2c-xiic.c
>>>> b/drivers/i2c/busses/i2c-xiic.c index
>>>> 0777e577720db..6db71c0fb6583 100644
>>>> --- a/drivers/i2c/busses/i2c-xiic.c
>>>> +++ b/drivers/i2c/busses/i2c-xiic.c
>>>> @@ -543,7 +543,6 @@ static void xiic_start_recv(struct xiic_i2c *i2c)  {
>>>>  	u8 rx_watermark;
>>>>  	struct i2c_msg *msg = i2c->rx_msg = i2c->tx_msg;
>>>> -	unsigned long flags;
>>>>
>>>>  	/* Clear and enable Rx full interrupt. */
>>>>  	xiic_irq_clr_en(i2c, XIIC_INTR_RX_FULL_MASK |
>>>> XIIC_INTR_TX_ERROR_MASK); @@ -559,7 +558,6 @@ static void
>>>> xiic_start_recv(struct xiic_i2c *i2c)
>>>>  		rx_watermark = IIC_RX_FIFO_DEPTH;
>>>>  	xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET, rx_watermark - 1);
>>>>
>>>> -	local_irq_save(flags);
>>>
>>> It is added as part of (i2c: xiic: Make the start and the byte count
>>> write atomic - ae7304c3ea28a3ba47a7a8312c76c654ef24967e) commit to
>> make the below 2 register writes atomic so that the controller doesn't produce
>> a wrong transaction.
>>
>> Two consecutive register writes are not atomic, they are posted as two
>> consecutive writes on the bus and will reach the hardware IP as two separate
>> writes with arbitrary delay between them.
>>
>> I think the intention of the patch ae7304c3ea28 ("i2c: xiic: Make the start and
>> the byte count write atomic") is to make sure that there will be no read/write
>> register access to the XIIC IP while those registers in
>> local_irq_save()/local_irq_restore() block are written, and that makes sense.
>>
>> However, local_irq_save()/local_irq_restore() is local to one CPU core, it does
>> not prevent another CPU core from posting register read/write access to the
>> XIIC IP (for example from xiic_process() threaded interrupt handler, which
>> might just be running on that other CPU core).
>>
>> The &i2c->lock mutex is locked by both the xiic_start() and xiic_process(), and
>> therefore guarantees that no other thread can post read/write register access
>> to the XIIC IP while xiic_start() (and thus xiic_recv_start(), which is called from
>> it) is running.
>>
>> And so, I think this local_irq_save()/local_irq_restore() can be removed.
>>
>>> (If there is a delay between the 2 register writes, controller is
>>> messing up with the transaction). It is not intended for locking between this
>> function and isr. So, this can't be removed.
>>
>> But the bus can insert arbitrary delay between two consecutive register writes
>> to the XIIC IP, so if the timing between the two register writes is really critical,
>> then I don't see how to guarantee the timing requirement. Do you happen to
>> have more details on what really happens on the hardware level here , maybe
>> some errata document for the XIIC IP?
> 
> From the commit description of "i2c: xiic: Make the start and the byte count write atomic"(ae7304c3ea28a3ba47a7a8312c76c654ef24967e),
> 
> [Disable interrupts while configuring the transfer and enable them back.
> 
> We have below as the programming sequence
> 1. start and slave address
> 2. byte count and stop
> 
> In some customer platform there was a lot of interrupts between 1 and 2
> and after slave address (around 7 clock cyles) if 2 is not executed
> then the transaction is nacked.

Is that a hardware property of the XIIC IP, that the transaction is
NACKed after 7 clock cycles of the XIIC IP ?

> To fix this case make the 2 writes atomic.]

But using local_irq_save()/local_irq_restore() will not make two
separate register writes atomic, they will still be two separate
consecutive register writes.

Also note that another CPU core can very well be generating a lot of
traffic on the bus between current CPU core and the XIIC IP, so the
first write from current CPU core to the XIIC IP would arrive at the
XIIC IP, then the bus will be busy for a long time with other requiests
(more than 7 cycles), and then the second write to the XIIC IP will
arrive at the XIIC IP. The local_irq_save()/local_irq_restore() can not
prevent this scenario, but this is very real on a system under load.

> So, this local_irq_save()/local_irq_restore() is not related to exclusive access in the driver (xiic_process vs xiic_start),
> but to supply the byte count to controller before it completes transmitting START and slave address.

But then shouldn't the XIIC IP be fixed / extended with support for such
an atomic update instead ?

>> Thanks
>>
>>>>  	if (!(msg->flags & I2C_M_NOSTART))
>>>>  		/* write the address */
>>>>  		xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, @@ -569,7 +567,6
>>
>> [...]

Re: [PATCH 4/5] i2c: xiic: Switch from waitqueue to completion

[Marek Vasut]

On 6/29/20 2:53 PM, Raviteja Narayanam wrote:

Hi,

[...]

>>>> @@ -703,23 +704,24 @@ static int xiic_xfer(struct i2c_adapter *adap,
>>>> struct i2c_msg *msgs, int num)
>>>>  	err = xiic_start_xfer(i2c, msgs, num);
>>>>  	if (err < 0) {
>>>>  		dev_err(adap->dev.parent, "Error xiic_start_xfer\n");
>>>> -		goto out;
>>>> +		return err;
>>>>  	}
>>>>
>>>> -	if (wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
>>>> -		(i2c->state == STATE_DONE), HZ)) {
>>>> -		mutex_lock(&i2c->lock);
>>>> -		err = (i2c->state == STATE_DONE) ? num : -EIO;
>>>> -		goto out;
>>>> -	} else {
>>>> -		mutex_lock(&i2c->lock);
>>>> +	err = wait_for_completion_interruptible_timeout(&i2c->completion,
>>>> +							XIIC_I2C_TIMEOUT);
>>>
>>> This is causing random lock up of bus. Since it is "interruptible"
>>> API,  once we enter Ctrl+C, It is coming out of wait state, the message
>> pointers are made NULL and the ongoing transaction is not completed.
>>> Can use " wait_for_completion_timeout" API in place of this.
>>>
>>>> +	mutex_lock(&i2c->lock);
>>
>> So in case this is interrupted, we would have to somehow reset the controller ?
> 
> Yes, but the cleanup is not straight forward because we do not know the exact state
> Of controller and the I2C bus. That’s why preferring a non-interruptible API.

Ah, right, that makes sense, thanks.

>> What sort of lockups do you see exactly ? 
> 
> There is an i2ctransfer going on (let's say a read of 255 bytes), we get interrupt everytime the Rx FIFO is full.
> While the controller is receiving data, if there is a signal, this is exiting abruptly and there is no STOP condition
> on the bus. So, no master can communicate on that bus further. 
> 
> Can you share some sort of test case,
>> so I can replicate it ?
> 
> I have 3 scripts running with commands like below, and I am randomly giving Ctrl+C.

OK, thanks.

RE: [PATCH 1/5] i2c: xiic: Fix broken locking on tx_msg

[Raviteja Narayanam]

Tested-by: Raviteja Narayanam <raviteja.narayanam@xilinx.com>

Thanks
Raviteja N

> -----Original Message-----
> From: linux-i2c-owner@vger.kernel.org <linux-i2c-owner@vger.kernel.org> On
> Behalf Of Raviteja Narayanam
> Sent: Monday, June 29, 2020 6:22 PM
> To: Marek Vasut <marex@denx.de>; linux-i2c@vger.kernel.org
> Cc: Michal Simek <michals@xilinx.com>; Shubhrajyoti Datta
> <shubhraj@xilinx.com>; Wolfram Sang <wsa@kernel.org>
> Subject: RE: [PATCH 1/5] i2c: xiic: Fix broken locking on tx_msg
> 
> 
> 
> > -----Original Message-----
> > From: Marek Vasut <marex@denx.de>
> > Sent: Monday, June 29, 2020 4:49 AM
> > To: Raviteja Narayanam <rna@xilinx.com>; linux-i2c@vger.kernel.org
> > Cc: Michal Simek <michals@xilinx.com>; Shubhrajyoti Datta
> > <shubhraj@xilinx.com>; Wolfram Sang <wsa@kernel.org>
> > Subject: Re: [PATCH 1/5] i2c: xiic: Fix broken locking on tx_msg
> >
> > On 6/26/20 2:11 PM, Raviteja Narayanam wrote:
> >
> > Hi,
> >
> > [...]
> >
> > >> @@ -699,14 +708,7 @@ static int xiic_xfer(struct i2c_adapter *adap,
> > >> struct i2c_msg *msgs, int num)
> > >>  	if (err < 0)
> > >>  		return err;
> > >>
> > >> -	err = xiic_busy(i2c);
> > >> -	if (err)
> > >> -		goto out;
> > >> -
> > >> -	i2c->tx_msg = msgs;
> > >> -	i2c->nmsgs = num;
> > >
> > > On an SMP system with multiple i2c-transfer command scripts running,
> > > the
> > above critical section is expected to cause serious trouble
> > overwriting the previous msg pointers.
> > > But that's not happening as the i2c-core is having a lock at adapter
> > > level
> > inside i2c-core-base.c (rt_mutex_lock_nested).
> > > So, the race condition between different threads is not possible.
> > > They are all
> > serialized by the locking in i2c-core.
> > >
> > > Although no issues are seen in the tests, the contention within the
> > > driver is
> > still possible (isr vs xiic_xfer), if there is a spurious interrupt.
> > And this patch is needed in that case.
> > The contention happens between the threaded interrupt handler
> > xiic_process() and this xiic_xfer() function. While you can not have
> > xiic_xfer() running on two CPU cores at the same time, you can have
> > xiic_xfer() running on one CPU core and xiic_process() on another CPU
> > core, and that will lead to problems.
> 
> Yes, this patch is needed for that case.
> 
> >
> > [...]

RE: [PATCH 2/5] i2c: xiic: Drop broken interrupt handler

[Raviteja Narayanam]

Hi Marek,

> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Tuesday, June 30, 2020 3:20 AM
> To: Raviteja Narayanam <rna@xilinx.com>; linux-i2c@vger.kernel.org
> Cc: Michal Simek <michals@xilinx.com>; Shubhrajyoti Datta
> <shubhraj@xilinx.com>; Wolfram Sang <wsa@kernel.org>
> Subject: Re: [PATCH 2/5] i2c: xiic: Drop broken interrupt handler
> 
> On 6/29/20 2:52 PM, Raviteja Narayanam wrote:
> 
> Hi,
> 
> [...]
> 
> >>>> diff --git a/drivers/i2c/busses/i2c-xiic.c
> >>>> b/drivers/i2c/busses/i2c-xiic.c index
> >>>> 0777e577720db..6db71c0fb6583 100644
> >>>> --- a/drivers/i2c/busses/i2c-xiic.c
> >>>> +++ b/drivers/i2c/busses/i2c-xiic.c
> >>>> @@ -543,7 +543,6 @@ static void xiic_start_recv(struct xiic_i2c *i2c)  {
> >>>>  	u8 rx_watermark;
> >>>>  	struct i2c_msg *msg = i2c->rx_msg = i2c->tx_msg;
> >>>> -	unsigned long flags;
> >>>>
> >>>>  	/* Clear and enable Rx full interrupt. */
> >>>>  	xiic_irq_clr_en(i2c, XIIC_INTR_RX_FULL_MASK |
> >>>> XIIC_INTR_TX_ERROR_MASK); @@ -559,7 +558,6 @@ static void
> >>>> xiic_start_recv(struct xiic_i2c *i2c)
> >>>>  		rx_watermark = IIC_RX_FIFO_DEPTH;
> >>>>  	xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET, rx_watermark - 1);
> >>>>
> >>>> -	local_irq_save(flags);
> >>>
> >>> It is added as part of (i2c: xiic: Make the start and the byte count
> >>> write atomic - ae7304c3ea28a3ba47a7a8312c76c654ef24967e) commit to
> >> make the below 2 register writes atomic so that the controller
> >> doesn't produce a wrong transaction.
> >>
> >> Two consecutive register writes are not atomic, they are posted as
> >> two consecutive writes on the bus and will reach the hardware IP as
> >> two separate writes with arbitrary delay between them.
> >>
> >> I think the intention of the patch ae7304c3ea28 ("i2c: xiic: Make the
> >> start and the byte count write atomic") is to make sure that there
> >> will be no read/write register access to the XIIC IP while those
> >> registers in
> >> local_irq_save()/local_irq_restore() block are written, and that makes sense.
> >>
> >> However, local_irq_save()/local_irq_restore() is local to one CPU
> >> core, it does not prevent another CPU core from posting register
> >> read/write access to the XIIC IP (for example from xiic_process()
> >> threaded interrupt handler, which might just be running on that other CPU
> core).
> >>
> >> The &i2c->lock mutex is locked by both the xiic_start() and
> >> xiic_process(), and therefore guarantees that no other thread can
> >> post read/write register access to the XIIC IP while xiic_start()
> >> (and thus xiic_recv_start(), which is called from
> >> it) is running.
> >>
> >> And so, I think this local_irq_save()/local_irq_restore() can be removed.
> >>
> >>> (If there is a delay between the 2 register writes, controller is
> >>> messing up with the transaction). It is not intended for locking
> >>> between this
> >> function and isr. So, this can't be removed.
> >>
> >> But the bus can insert arbitrary delay between two consecutive
> >> register writes to the XIIC IP, so if the timing between the two
> >> register writes is really critical, then I don't see how to guarantee
> >> the timing requirement. Do you happen to have more details on what
> >> really happens on the hardware level here , maybe some errata document
> for the XIIC IP?
> >
> > From the commit description of "i2c: xiic: Make the start and the byte
> > count write atomic"(ae7304c3ea28a3ba47a7a8312c76c654ef24967e),
> >
> > [Disable interrupts while configuring the transfer and enable them back.
> >
> > We have below as the programming sequence 1. start and slave address
> > 2. byte count and stop
> >
> > In some customer platform there was a lot of interrupts between 1 and
> > 2 and after slave address (around 7 clock cyles) if 2 is not executed
> > then the transaction is nacked.
> 
> Is that a hardware property of the XIIC IP, that the transaction is NACKed after
> 7 clock cycles of the XIIC IP ?

Yes, it is from IP. 

> 
> > To fix this case make the 2 writes atomic.]
> 
> But using local_irq_save()/local_irq_restore() will not make two separate
> register writes atomic, they will still be two separate consecutive register
> writes.
> 
> Also note that another CPU core can very well be generating a lot of traffic on
> the bus between current CPU core and the XIIC IP, so the first write from
> current CPU core to the XIIC IP would arrive at the XIIC IP, then the bus will be
> busy for a long time with other requiests (more than 7 cycles), and then the
> second write to the XIIC IP will arrive at the XIIC IP. The
> local_irq_save()/local_irq_restore() can not prevent this scenario, but this is
> very real on a system under load.

Yeah, such possibility is there. local_irq_save()/local_irq_restore() can't be the right solution
as you said.

> 
> > So, this local_irq_save()/local_irq_restore() is not related to
> > exclusive access in the driver (xiic_process vs xiic_start), but to supply the
> byte count to controller before it completes transmitting START and slave
> address.
> 
> But then shouldn't the XIIC IP be fixed / extended with support for such an
> atomic update instead ?

I have started communicating with the hardware team on why the IP behavior is like this. 
But, that will take more time to get to a conclusion. We can continue this discussion here.
So, how about we remove this patch from the current series and go ahead with the rest of patches?
In that case please send a v2 with the minimal changes suggested in other patches (4 and 5)?

Thanks
Raviteja N
> 
> >> Thanks
> >>
> >>>>  	if (!(msg->flags & I2C_M_NOSTART))
> >>>>  		/* write the address */
> >>>>  		xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, @@ -569,7 +567,6
> >>
> >> [...]

RE: [PATCH 3/5] i2c: xiic: Defer xiic_wakeup() and __xiic_start_xfer() in xiic_process()

[Raviteja Narayanam]

Tested-by: Raviteja Narayanam <raviteja.narayanam@xilinx.com>

Thanks
Raviteja N

> -----Original Message-----
> From: linux-i2c-owner@vger.kernel.org <linux-i2c-owner@vger.kernel.org> On
> Behalf Of Raviteja Narayanam
> Sent: Friday, June 26, 2020 5:44 PM
> To: Marek Vasut <marex@denx.de>; linux-i2c@vger.kernel.org
> Cc: Michal Simek <michals@xilinx.com>; Shubhrajyoti Datta
> <shubhraj@xilinx.com>; Wolfram Sang <wsa@kernel.org>
> Subject: RE: [PATCH 3/5] i2c: xiic: Defer xiic_wakeup() and __xiic_start_xfer() in
> xiic_process()
> 
> 
> 
> > -----Original Message-----
> > From: linux-i2c-owner@vger.kernel.org
> > <linux-i2c-owner@vger.kernel.org> On Behalf Of Marek Vasut
> > Sent: Saturday, June 13, 2020 8:38 PM
> > To: linux-i2c@vger.kernel.org
> > Cc: Marek Vasut <marex@denx.de>; Michal Simek <michals@xilinx.com>;
> > Shubhrajyoti Datta <shubhraj@xilinx.com>; Wolfram Sang
> > <wsa@kernel.org>
> > Subject: [PATCH 3/5] i2c: xiic: Defer xiic_wakeup() and
> > __xiic_start_xfer() in
> > xiic_process()
> >
> > The __xiic_start_xfer() manipulates the interrupt flags, xiic_wakeup()
> > may result in return from xiic_xfer() early. Defer both to the end of
> > the
> > xiic_process() interrupt thread, so that they are executed after all
> > the other interrupt bits handling completed and once it completely
> > safe to perform changes to the interrupt bits in the hardware.
> >
> > Signed-off-by: Marek Vasut <marex@denx.de>
> > Cc: Michal Simek <michal.simek@xilinx.com>
> > Cc: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> > Cc: Wolfram Sang <wsa@kernel.org>
> > ---
> >  drivers/i2c/busses/i2c-xiic.c | 37
> > ++++++++++++++++++++++++-----------
> >  1 file changed, 26 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-xiic.c
> > b/drivers/i2c/busses/i2c-xiic.c index
> > 6db71c0fb6583..87eca9d46afd9 100644
> > --- a/drivers/i2c/busses/i2c-xiic.c
> > +++ b/drivers/i2c/busses/i2c-xiic.c
> > @@ -373,6 +373,9 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
> >  	struct xiic_i2c *i2c = dev_id;
> >  	u32 pend, isr, ier;
> >  	u32 clr = 0;
> > +	int xfer_more = 0;
> > +	int wakeup_req = 0;
> > +	int wakeup_code = 0;
> >
> >  	/* Get the interrupt Status from the IPIF. There is no clearing of
> >  	 * interrupts in the IPIF. Interrupts must be cleared at the source.
> > @@ -409,10 +412,14 @@ static irqreturn_t xiic_process(int irq, void
> *dev_id)
> >  		 */
> >  		xiic_reinit(i2c);
> >
> > -		if (i2c->rx_msg)
> > -			xiic_wakeup(i2c, STATE_ERROR);
> > -		if (i2c->tx_msg)
> > -			xiic_wakeup(i2c, STATE_ERROR);
> > +		if (i2c->rx_msg) {
> > +			wakeup_req = 1;
> > +			wakeup_code = STATE_ERROR;
> > +		}
> > +		if (i2c->tx_msg) {
> > +			wakeup_req = 1;
> > +			wakeup_code = STATE_ERROR;
> > +		}
> >  	}
> >  	if (pend & XIIC_INTR_RX_FULL_MASK) {
> >  		/* Receive register/FIFO is full */ @@ -446,8 +453,7 @@ static
> > irqreturn_t xiic_process(int irq, void *dev_id)
> >  				i2c->tx_msg++;
> >  				dev_dbg(i2c->adap.dev.parent,
> >  					"%s will start next...\n", __func__);
> > -
> > -				__xiic_start_xfer(i2c);
> > +				xfer_more = 1;
> >  			}
> >  		}
> >  	}
> > @@ -461,11 +467,13 @@ static irqreturn_t xiic_process(int irq, void
> *dev_id)
> >  		if (!i2c->tx_msg)
> >  			goto out;
> >
> > -		if ((i2c->nmsgs == 1) && !i2c->rx_msg &&
> > -			xiic_tx_space(i2c) == 0)
> > -			xiic_wakeup(i2c, STATE_DONE);
> > +		wakeup_req = 1;
> > +
> > +		if (i2c->nmsgs == 1 && !i2c->rx_msg &&
> > +		    xiic_tx_space(i2c) == 0)
> > +			wakeup_code = STATE_DONE;
> >  		else
> > -			xiic_wakeup(i2c, STATE_ERROR);
> > +			wakeup_code = STATE_ERROR;
> >  	}
> >  	if (pend & (XIIC_INTR_TX_EMPTY_MASK | XIIC_INTR_TX_HALF_MASK))
> {
> >  		/* Transmit register/FIFO is empty or ½ empty */ @@ -489,7
> > +497,7 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
> >  			if (i2c->nmsgs > 1) {
> >  				i2c->nmsgs--;
> >  				i2c->tx_msg++;
> > -				__xiic_start_xfer(i2c);
> > +				xfer_more = 1;
> >  			} else {
> >  				xiic_irq_dis(i2c, XIIC_INTR_TX_HALF_MASK);
> >
> > @@ -507,6 +515,13 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
> >  	dev_dbg(i2c->adap.dev.parent, "%s clr: 0x%x\n", __func__, clr);
> >
> >  	xiic_setreg32(i2c, XIIC_IISR_OFFSET, clr);
> > +	if (xfer_more)
> > +		__xiic_start_xfer(i2c);
> > +	if (wakeup_req)
> > +		xiic_wakeup(i2c, wakeup_code);
> > +
> > +	WARN_ON(xfer_more && wakeup_req);
> > +
> >  	mutex_unlock(&i2c->lock);
> >  	return IRQ_HANDLED;
> >  }
> 
> This is tested and working fine.
> 
> Raviteja N

Re: [PATCH 5/5] i2c: xiic: Only ever transfer single message

[Michal Simek]

[-- Attachment #1.1: Type: text/plain, Size: 985 bytes --]



On 13. 06. 20 21:42, Wolfram Sang wrote:
> 
>>>> Transferring multiple messages via XIIC suffers from strange interaction
>>>> between the interrupt status/enable register flags. These flags are being
>>>> reused in the hardware to indicate different things for read and write
>>>> transfer, and doing multiple transactions becomes horribly complex. Just
>>>> send a single transaction and reload the controller with another message
>>>> once the transaction is done in the interrupt handler thread.
>>>
>>> Do we still get a repeated start between messages of a transfer? Or will
>>> it be a STOP/START combination?
>>
>> Repeated start.
> 
> Good. That was my high level question. I'll leave the rest of the review
> then for Michal.
> 

I have read all reviews in this thread and there should be update on 4/5
and discussion around 2/5 is not done.
That's why please send 1/3/4/5 as v2 and 2/5 separately to deal with
this separately.

Thanks,
Michal


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 2/5] i2c: xiic: Drop broken interrupt handler

[Marek Vasut]

On 7/8/20 5:23 PM, Raviteja Narayanam wrote:
> Hi Marek,

Hi,

[...]

>>> So, this local_irq_save()/local_irq_restore() is not related to
>>> exclusive access in the driver (xiic_process vs xiic_start), but to supply the
>> byte count to controller before it completes transmitting START and slave
>> address.
>>
>> But then shouldn't the XIIC IP be fixed / extended with support for such an
>> atomic update instead ?
> 
> I have started communicating with the hardware team on why the IP behavior is like this.

Any news from the hardware team ?

[...]

RE: [PATCH 2/5] i2c: xiic: Drop broken interrupt handler

[Raviteja Narayanam]

> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Wednesday, August 19, 2020 11:12 PM
> To: Raviteja Narayanam <rna@xilinx.com>; linux-i2c@vger.kernel.org
> Cc: Michal Simek <michals@xilinx.com>; Shubhrajyoti Datta
> <shubhraj@xilinx.com>; Wolfram Sang <wsa@kernel.org>
> Subject: Re: [PATCH 2/5] i2c: xiic: Drop broken interrupt handler
> 
> On 7/8/20 5:23 PM, Raviteja Narayanam wrote:
> > Hi Marek,
> 
> Hi,
> 
> [...]
> 
> >>> So, this local_irq_save()/local_irq_restore() is not related to
> >>> exclusive access in the driver (xiic_process vs xiic_start), but to
> >>> supply the
> >> byte count to controller before it completes transmitting START and
> >> slave address.
> >>
> >> But then shouldn't the XIIC IP be fixed / extended with support for
> >> such an atomic update instead ?
> >
> > I have started communicating with the hardware team on why the IP
> behavior is like this.
> 
> Any news from the hardware team ?

" The expectation from the IP is un interrupted read i.e the read command should be un interrupted and max delay expected is not more than 35-40 us provided IIC frequency is 100 Khz. Please check if we can manage this in Software. If delay is not managed enable iic control only after stop related data is received"
That was the response as is. 
The workaround suggested above is to enable the AXI I2C only after second register write(STOP bit info with read count) is completed. This is not generic, so we couldn't move forward.
Our typical application scenario is like this "START WRITE, REPEATED START READ, STOP". So, we must enable AXI I2C before read count is sent.

> 
> [...]

Re: [PATCH 2/5] i2c: xiic: Drop broken interrupt handler

[Marek Vasut]

On 8/24/20 10:27 AM, Raviteja Narayanam wrote:

[...]

>>>>> So, this local_irq_save()/local_irq_restore() is not related to
>>>>> exclusive access in the driver (xiic_process vs xiic_start), but to
>>>>> supply the
>>>> byte count to controller before it completes transmitting START and
>>>> slave address.
>>>>
>>>> But then shouldn't the XIIC IP be fixed / extended with support for
>>>> such an atomic update instead ?
>>>
>>> I have started communicating with the hardware team on why the IP
>> behavior is like this.
>>
>> Any news from the hardware team ?
> 
> " The expectation from the IP is un interrupted read i.e the read command should be un interrupted and max delay expected is not more than 35-40 us provided IIC frequency is 100 Khz. Please check if we can manage this in Software. If delay is not managed enable iic control only after stop related data is received"
> That was the response as is. 
> The workaround suggested above is to enable the AXI I2C only after second register write(STOP bit info with read count) is completed. This is not generic, so we couldn't move forward.
> Our typical application scenario is like this "START WRITE, REPEATED START READ, STOP". So, we must enable AXI I2C before read count is sent.

So, if I understand it correctly, the XIIC IP is broken and cannot be
fixed in software reliably ? How shall we proceed then ?

RE: [PATCH 2/5] i2c: xiic: Drop broken interrupt handler

[Raviteja Narayanam]

> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Monday, August 24, 2020 5:28 PM
> To: Raviteja Narayanam <rna@xilinx.com>; linux-i2c@vger.kernel.org
> Cc: Michal Simek <michals@xilinx.com>; Shubhrajyoti Datta
> <shubhraj@xilinx.com>; Wolfram Sang <wsa@kernel.org>; Srinivas Goud
> <sgoud@xilinx.com>
> Subject: Re: [PATCH 2/5] i2c: xiic: Drop broken interrupt handler
> 
> On 8/24/20 10:27 AM, Raviteja Narayanam wrote:
> 
> [...]
> 
> >>>>> So, this local_irq_save()/local_irq_restore() is not related to
> >>>>> exclusive access in the driver (xiic_process vs xiic_start), but
> >>>>> to supply the
> >>>> byte count to controller before it completes transmitting START and
> >>>> slave address.
> >>>>
> >>>> But then shouldn't the XIIC IP be fixed / extended with support for
> >>>> such an atomic update instead ?
> >>>
> >>> I have started communicating with the hardware team on why the IP
> >> behavior is like this.
> >>
> >> Any news from the hardware team ?
> >
> > " The expectation from the IP is un interrupted read i.e the read command
> should be un interrupted and max delay expected is not more than 35-40 us
> provided IIC frequency is 100 Khz. Please check if we can manage this in
> Software. If delay is not managed enable iic control only after stop related
> data is received"
> > That was the response as is.
> > The workaround suggested above is to enable the AXI I2C only after
> second register write(STOP bit info with read count) is completed. This is not
> generic, so we couldn't move forward.
> > Our typical application scenario is like this "START WRITE, REPEATED START
> READ, STOP". So, we must enable AXI I2C before read count is sent.
> 
> So, if I understand it correctly, the XIIC IP is broken and cannot be fixed in
> software reliably ? How shall we proceed then ?

We are thinking of two options.
1. Trying for a SW fix to workaround this problem. Waiting on the HW IP team for any other suggestions.
2. XIIC IP has two modes of operation as stated in the user guide - Dynamic and Standard modes.
Currently, the linux driver is using only dynamic mode and this bug appears here.
The SW programming for standard mode is different and we are testing it for all possible use cases. Once
That comes out successful, we will send out the patches.

Re: [PATCH 2/5] i2c: xiic: Drop broken interrupt handler

[Marek Vasut]

On 8/25/20 11:44 AM, Raviteja Narayanam wrote:

Hi,

[...]

>>>>>>> So, this local_irq_save()/local_irq_restore() is not related to
>>>>>>> exclusive access in the driver (xiic_process vs xiic_start), but
>>>>>>> to supply the
>>>>>> byte count to controller before it completes transmitting START and
>>>>>> slave address.
>>>>>>
>>>>>> But then shouldn't the XIIC IP be fixed / extended with support for
>>>>>> such an atomic update instead ?
>>>>>
>>>>> I have started communicating with the hardware team on why the IP
>>>> behavior is like this.
>>>>
>>>> Any news from the hardware team ?
>>>
>>> " The expectation from the IP is un interrupted read i.e the read command
>> should be un interrupted and max delay expected is not more than 35-40 us
>> provided IIC frequency is 100 Khz. Please check if we can manage this in
>> Software. If delay is not managed enable iic control only after stop related
>> data is received"
>>> That was the response as is.
>>> The workaround suggested above is to enable the AXI I2C only after
>> second register write(STOP bit info with read count) is completed. This is not
>> generic, so we couldn't move forward.
>>> Our typical application scenario is like this "START WRITE, REPEATED START
>> READ, STOP". So, we must enable AXI I2C before read count is sent.
>>
>> So, if I understand it correctly, the XIIC IP is broken and cannot be fixed in
>> software reliably ? How shall we proceed then ?
> 
> We are thinking of two options.
> 1. Trying for a SW fix to workaround this problem. Waiting on the HW IP team for any other suggestions.
> 2. XIIC IP has two modes of operation as stated in the user guide - Dynamic and Standard modes.
> Currently, the linux driver is using only dynamic mode and this bug appears here.
> The SW programming for standard mode is different and we are testing it for all possible use cases. Once
> That comes out successful, we will send out the patches.

Is this dynamic/standard mode switching what is already in the
downstream xilinx kernel tree ?

I think we should fix what is already upstream first, and only then add
more complexity.

[...]

RE: [PATCH 2/5] i2c: xiic: Drop broken interrupt handler

[Raviteja Narayanam]

> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Wednesday, August 26, 2020 2:21 AM
> To: Raviteja Narayanam <rna@xilinx.com>; linux-i2c@vger.kernel.org
> Cc: Michal Simek <michals@xilinx.com>; Shubhrajyoti Datta
> <shubhraj@xilinx.com>; Wolfram Sang <wsa@kernel.org>; Srinivas Goud
> <sgoud@xilinx.com>
> Subject: Re: [PATCH 2/5] i2c: xiic: Drop broken interrupt handler
> 
> On 8/25/20 11:44 AM, Raviteja Narayanam wrote:
> 
> Hi,
> 
> [...]
> 
> >>>>>>> So, this local_irq_save()/local_irq_restore() is not related to
> >>>>>>> exclusive access in the driver (xiic_process vs xiic_start), but
> >>>>>>> to supply the
> >>>>>> byte count to controller before it completes transmitting START
> >>>>>> and slave address.
> >>>>>>
> >>>>>> But then shouldn't the XIIC IP be fixed / extended with support
> >>>>>> for such an atomic update instead ?
> >>>>>
> >>>>> I have started communicating with the hardware team on why the IP
> >>>> behavior is like this.
> >>>>
> >>>> Any news from the hardware team ?
> >>>
> >>> " The expectation from the IP is un interrupted read i.e the read
> >>> command
> >> should be un interrupted and max delay expected is not more than
> >> 35-40 us provided IIC frequency is 100 Khz. Please check if we can
> >> manage this in Software. If delay is not managed enable iic control
> >> only after stop related data is received"
> >>> That was the response as is.
> >>> The workaround suggested above is to enable the AXI I2C only after
> >> second register write(STOP bit info with read count) is completed.
> >> This is not generic, so we couldn't move forward.
> >>> Our typical application scenario is like this "START WRITE, REPEATED
> >>> START
> >> READ, STOP". So, we must enable AXI I2C before read count is sent.
> >>
> >> So, if I understand it correctly, the XIIC IP is broken and cannot be
> >> fixed in software reliably ? How shall we proceed then ?
> >
> > We are thinking of two options.
> > 1. Trying for a SW fix to workaround this problem. Waiting on the HW IP
> team for any other suggestions.
> > 2. XIIC IP has two modes of operation as stated in the user guide - Dynamic
> and Standard modes.
> > Currently, the linux driver is using only dynamic mode and this bug appears
> here.
> > The SW programming for standard mode is different and we are testing
> > it for all possible use cases. Once That comes out successful, we will send
> out the patches.
> 
> Is this dynamic/standard mode switching what is already in the downstream
> xilinx kernel tree ?
> 
> I think we should fix what is already upstream first, and only then add more
> complexity.

Yes, agreed. But, that would be a hardware fix in the future IP release. Since we have
to support existing IP versions, we are planning to upstream the standard mode
and based on the DT property (IP version), we will switch.

Thanks,
Raviteja N

> 
> [...]

Re: [PATCH 2/5] i2c: xiic: Drop broken interrupt handler

[Marek Vasut]

On 9/7/20 10:51 AM, Raviteja Narayanam wrote:

[...]

>> Is this dynamic/standard mode switching what is already in the downstream
>> xilinx kernel tree ?
>>
>> I think we should fix what is already upstream first, and only then add more
>> complexity.
> 
> Yes, agreed. But, that would be a hardware fix in the future IP release. Since we have
> to support existing IP versions, we are planning to upstream the standard mode
> and based on the DT property (IP version), we will switch.

Is the dynamic mode addition be backported to stable as well ?
It is a lot of code, so I have a feeling that would be difficult.

Maybe it would be better to mark the xiic driver as broken until the
hardware is fixed ?

Re: [PATCH 2/5] i2c: xiic: Drop broken interrupt handler

[Michal Simek]

On 08. 09. 20 16:04, Marek Vasut wrote:
> On 9/7/20 10:51 AM, Raviteja Narayanam wrote:
> 
> [...]
> 
>>> Is this dynamic/standard mode switching what is already in the downstream
>>> xilinx kernel tree ?
>>>
>>> I think we should fix what is already upstream first, and only then add more
>>> complexity.
>>
>> Yes, agreed. But, that would be a hardware fix in the future IP release. Since we have
>> to support existing IP versions, we are planning to upstream the standard mode
>> and based on the DT property (IP version), we will switch.
> 
> Is the dynamic mode addition be backported to stable as well ?
> It is a lot of code, so I have a feeling that would be difficult.

Let's see when that changes are available.

> Maybe it would be better to mark the xiic driver as broken until the
> hardware is fixed ?

I don't have a preference here.
Wolfram: What do you think?

Thanks,
Michal