[PATCH v2 00/10] i2c: xiic: Add features, bug fixes.

[PATCH v2 00/10] i2c: xiic: Add features, bug fixes.

[Raviteja Narayanam]

-Add 'standard mode' feature for reads > 255 bytes.
-Add 'smbus block read' functionality.
-Add 'xlnx,axi-iic-2.1' new IP version support.
-Switch to 'AXI I2C standard mode' for i2c reads in affected IP versions.
-Remove 'local_irq_save/restore' calls as discussed here: https://www.spinics.net/lists/linux-i2c/msg46483.html.
-Some trivial fixes.

Changes in v2:
-Grouped the commits as fixes first and then features. 
-The first 4 commits fix the dynamic mode broken feature.
-Corrected the indentation in coding style issues.

Michal Simek (1):
  i2c: xiic: Fix coding style issues

Raviteja Narayanam (7):
  i2c: xiic: Fix Tx Interrupt path for grouped messages
  i2c: xiic: Add standard mode support for > 255 byte read transfers
  i2c: xiic: Switch to Xiic standard mode for i2c-read
  i2c: xiic: Remove interrupt enable/disable in Rx path
  dt-bindings: i2c: xiic: Add 'xlnx,axi-iic-2.1' to compatible
  i2c: xiic: Update compatible with new IP version
  i2c: xiic: Add smbus_block_read functionality

Shubhrajyoti Datta (2):
  i2c: xiic: Return value of xiic_reinit
  i2c: xiic: Fix the type check for xiic_wakeup

 .../bindings/i2c/xlnx,xps-iic-2.00.a.yaml     |   4 +-
 drivers/i2c/busses/i2c-xiic.c                 | 593 ++++++++++++++----
 2 files changed, 487 insertions(+), 110 deletions(-)

-- 
2.25.1

[PATCH v2 01/10] i2c: xiic: Fix Tx Interrupt path for grouped messages

[Raviteja Narayanam]

When a group of messages are sent from user space as a set, if
the last message has less than Tx FIFO DEPTH number of bytes
to transfer, Tx half empty interrupt is triggered continuously
from the hardware. It is due to Bus not busy interrupt coming
along with Tx half empty and tx empty.

Hence, service the Tx interrupts before Bus not busy interrupt
to update the i2c message status correctly.

Signed-off-by: Raviteja Narayanam <raviteja.narayanam@xilinx.com>
---
 drivers/i2c/busses/i2c-xiic.c | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 2a8568b97c14..b0cfd9d15467 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -453,22 +453,6 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
 			}
 		}
 	}
-	if (pend & XIIC_INTR_BNB_MASK) {
-		/* IIC bus has transitioned to not busy */
-		clr |= XIIC_INTR_BNB_MASK;
-
-		/* The bus is not busy, disable BusNotBusy interrupt */
-		xiic_irq_dis(i2c, XIIC_INTR_BNB_MASK);
-
-		if (!i2c->tx_msg)
-			goto out;
-
-		if ((i2c->nmsgs == 1) && !i2c->rx_msg &&
-			xiic_tx_space(i2c) == 0)
-			xiic_wakeup(i2c, STATE_DONE);
-		else
-			xiic_wakeup(i2c, STATE_ERROR);
-	}
 	if (pend & (XIIC_INTR_TX_EMPTY_MASK | XIIC_INTR_TX_HALF_MASK)) {
 		/* Transmit register/FIFO is empty or ½ empty */
 
@@ -505,6 +489,24 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
 			 */
 			xiic_irq_dis(i2c, XIIC_INTR_TX_HALF_MASK);
 	}
+
+	if (pend & XIIC_INTR_BNB_MASK) {
+		/* IIC bus has transitioned to not busy */
+		clr |= XIIC_INTR_BNB_MASK;
+
+		/* The bus is not busy, disable BusNotBusy interrupt */
+		xiic_irq_dis(i2c, XIIC_INTR_BNB_MASK);
+
+		if (!i2c->tx_msg)
+			goto out;
+
+		if (i2c->nmsgs == 1 && !i2c->rx_msg &&
+		    xiic_tx_space(i2c) == 0)
+			xiic_wakeup(i2c, STATE_DONE);
+		else
+			xiic_wakeup(i2c, STATE_ERROR);
+	}
+
 out:
 	dev_dbg(i2c->adap.dev.parent, "%s clr: 0x%x\n", __func__, clr);
 
-- 
2.25.1

[PATCH v2 02/10] i2c: xiic: Add standard mode support for > 255 byte read transfers

[Raviteja Narayanam]

In the current driver implementation, there is a limit of read
transfer size to 255 bytes as it is using AXI I2C dynamic mode.
But the IP supports this transfer through standard mode.

So added AXI I2C standard mode support to enable read transfers
of size more than 255 bytes. The driver scans through the message
request from user space and selects AXI I2C standard mode if there
is a read request of more than 255 bytes. Then the whole message goes
through standard mode Tx and Rx paths.

Signed-off-by: Raviteja Narayanam <raviteja.narayanam@xilinx.com>
---
 drivers/i2c/busses/i2c-xiic.c | 367 +++++++++++++++++++++++++++++-----
 1 file changed, 319 insertions(+), 48 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index b0cfd9d15467..004103267e9c 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -60,6 +60,8 @@ enum xiic_endian {
  * @clk: Pointer to AXI4-lite input clock
  * @state: See STATE_
  * @singlemaster: Indicates bus is single master
+ * @dynamic: Mode of controller
+ * @prev_msg_tx: Previous message is Tx
  */
 struct xiic_i2c {
 	struct device *dev;
@@ -76,6 +78,8 @@ struct xiic_i2c {
 	struct clk *clk;
 	enum xilinx_i2c_state state;
 	bool singlemaster;
+	bool dynamic;
+	bool prev_msg_tx;
 };
 
 
@@ -144,6 +148,9 @@ struct xiic_i2c {
 #define XIIC_TX_DYN_START_MASK            0x0100 /* 1 = Set dynamic start */
 #define XIIC_TX_DYN_STOP_MASK             0x0200 /* 1 = Set dynamic stop */
 
+/* Dynamic mode constants */
+#define MAX_READ_LENGTH_DYNAMIC		255 /* Max length for dynamic read */
+
 /*
  * The following constants define the register offsets for the Interrupt
  * registers. There are some holes in the memory map for reserved addresses
@@ -270,6 +277,24 @@ static int xiic_clear_rx_fifo(struct xiic_i2c *i2c)
 	return 0;
 }
 
+static int xiic_wait_tx_empty(struct xiic_i2c *i2c)
+{
+	u8 isr;
+	unsigned long timeout;
+
+	timeout = jiffies + XIIC_I2C_TIMEOUT;
+	for (isr = xiic_getreg32(i2c, XIIC_IISR_OFFSET);
+			!(isr & XIIC_INTR_TX_EMPTY_MASK);
+			isr = xiic_getreg32(i2c, XIIC_IISR_OFFSET)) {
+		if (time_after(jiffies, timeout)) {
+			dev_err(i2c->dev, "Timeout waiting at Tx empty\n");
+			return -ETIMEDOUT;
+		}
+	}
+
+	return 0;
+}
+
 static int xiic_reinit(struct xiic_i2c *i2c)
 {
 	int ret;
@@ -311,13 +336,14 @@ static void xiic_deinit(struct xiic_i2c *i2c)
 
 static void xiic_read_rx(struct xiic_i2c *i2c)
 {
-	u8 bytes_in_fifo;
+	u8 bytes_in_fifo, cr = 0, bytes_to_read = 0;
+	u32 bytes_rem = 0;
 	int i;
 
 	bytes_in_fifo = xiic_getreg8(i2c, XIIC_RFO_REG_OFFSET) + 1;
 
 	dev_dbg(i2c->adap.dev.parent,
-		"%s entry, bytes in fifo: %d, msg: %d, SR: 0x%x, CR: 0x%x\n",
+		"%s entry, bytes in fifo: %d, rem: %d, SR: 0x%x, CR: 0x%x\n",
 		__func__, bytes_in_fifo, xiic_rx_space(i2c),
 		xiic_getreg8(i2c, XIIC_SR_REG_OFFSET),
 		xiic_getreg8(i2c, XIIC_CR_REG_OFFSET));
@@ -325,13 +351,53 @@ static void xiic_read_rx(struct xiic_i2c *i2c)
 	if (bytes_in_fifo > xiic_rx_space(i2c))
 		bytes_in_fifo = xiic_rx_space(i2c);
 
-	for (i = 0; i < bytes_in_fifo; i++)
+	bytes_to_read = bytes_in_fifo;
+
+	if (!i2c->dynamic) {
+		bytes_rem = xiic_rx_space(i2c) - bytes_in_fifo;
+
+		if (bytes_rem > IIC_RX_FIFO_DEPTH) {
+			bytes_to_read = bytes_in_fifo;
+		} else if (bytes_rem > 1) {
+			bytes_to_read = bytes_rem - 1;
+		} else if (bytes_rem == 1) {
+			bytes_to_read = 1;
+			/* Set NACK in CR to indicate slave transmitter */
+			cr = xiic_getreg8(i2c, XIIC_CR_REG_OFFSET);
+			xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, cr |
+				     XIIC_CR_NO_ACK_MASK);
+		} else if (bytes_rem == 0) {
+			bytes_to_read = bytes_in_fifo;
+
+			/* Generate stop on the bus if it is last message */
+			if (i2c->nmsgs == 1) {
+				cr = xiic_getreg8(i2c, XIIC_CR_REG_OFFSET);
+				xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, cr &
+					     ~XIIC_CR_MSMS_MASK);
+			}
+
+			/* Make TXACK=0, clean up for next transaction */
+			cr = xiic_getreg8(i2c, XIIC_CR_REG_OFFSET);
+			xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, cr &
+				     ~XIIC_CR_NO_ACK_MASK);
+		}
+	}
+
+	/* Read the fifo */
+	for (i = 0; i < bytes_to_read; i++) {
 		i2c->rx_msg->buf[i2c->rx_pos++] =
 			xiic_getreg8(i2c, XIIC_DRR_REG_OFFSET);
+	}
 
-	xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET,
-		(xiic_rx_space(i2c) > IIC_RX_FIFO_DEPTH) ?
-		IIC_RX_FIFO_DEPTH - 1 :  xiic_rx_space(i2c) - 1);
+	if (i2c->dynamic) {
+		u8 bytes;
+
+		/* Receive remaining bytes if less than fifo depth */
+		bytes = min_t(u8, xiic_rx_space(i2c), IIC_RX_FIFO_DEPTH);
+		bytes--;
+
+		xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET, bytes);
+	}
 }
 
 static int xiic_tx_fifo_space(struct xiic_i2c *i2c)
@@ -361,6 +427,62 @@ static void xiic_fill_tx_fifo(struct xiic_i2c *i2c)
 	}
 }
 
+static void xiic_std_fill_tx_fifo(struct xiic_i2c *i2c)
+{
+	u8 fifo_space = xiic_tx_fifo_space(i2c);
+	u16 data = 0;
+	int len = xiic_tx_space(i2c);
+
+	dev_dbg(i2c->adap.dev.parent, "%s entry, len: %d, fifo space: %d\n",
+		__func__, len, fifo_space);
+
+	if (len > fifo_space)
+		len = fifo_space;
+	else if (len && !(i2c->nmsgs > 1))
+		len--;
+
+	while (len--) {
+		data = i2c->tx_msg->buf[i2c->tx_pos++];
+		xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, data);
+	}
+}
+
+static void xiic_send_tx(struct xiic_i2c *i2c)
+{
+	dev_dbg(i2c->adap.dev.parent,
+		"%s entry, rem: %d, SR: 0x%x, CR: 0x%x\n",
+		__func__, xiic_tx_space(i2c),
+		xiic_getreg8(i2c, XIIC_SR_REG_OFFSET),
+		xiic_getreg8(i2c, XIIC_CR_REG_OFFSET));
+
+	if (xiic_tx_space(i2c) > 1) {
+		xiic_std_fill_tx_fifo(i2c);
+		return;
+	}
+
+	if ((xiic_tx_space(i2c) == 1)) {
+		u16 data;
+
+		if (i2c->nmsgs == 1) {
+			u8 cr;
+			int status;
+
+			/* Wait till FIFO is empty so STOP is sent last */
+			status = xiic_wait_tx_empty(i2c);
+			if (status)
+				return;
+
+			/* Write to CR to stop */
+			cr = xiic_getreg8(i2c, XIIC_CR_REG_OFFSET);
+			xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, cr &
+				     ~XIIC_CR_MSMS_MASK);
+		}
+		/* Send last byte */
+		data = i2c->tx_msg->buf[i2c->tx_pos++];
+		xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, data);
+	}
+}
+
 static void xiic_wakeup(struct xiic_i2c *i2c, int code)
 {
 	i2c->tx_msg = NULL;
@@ -391,7 +513,9 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
 	dev_dbg(i2c->adap.dev.parent, "%s: SR: 0x%x, msg: %p, nmsgs: %d\n",
 		__func__, xiic_getreg8(i2c, XIIC_SR_REG_OFFSET),
 		i2c->tx_msg, i2c->nmsgs);
-
+	dev_dbg(i2c->adap.dev.parent, "%s, ISR: 0x%x, CR: 0x%x\n",
+		__func__, xiic_getreg32(i2c, XIIC_IISR_OFFSET),
+		xiic_getreg8(i2c, XIIC_CR_REG_OFFSET));
 
 	/* Service requesting interrupt */
 	if ((pend & XIIC_INTR_ARB_LOST_MASK) ||
@@ -465,7 +589,10 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
 			goto out;
 		}
 
-		xiic_fill_tx_fifo(i2c);
+		if (i2c->dynamic)
+			xiic_fill_tx_fifo(i2c);
+		else
+			xiic_send_tx(i2c);
 
 		/* current message sent and there is space in the fifo */
 		if (!xiic_tx_space(i2c) && xiic_tx_fifo_space(i2c) >= 2) {
@@ -554,35 +681,113 @@ static int xiic_busy(struct xiic_i2c *i2c)
 
 static void xiic_start_recv(struct xiic_i2c *i2c)
 {
-	u8 rx_watermark;
+	u16 rx_watermark;
+	u8 cr = 0, rfd_set = 0;
 	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);
+	dev_dbg(i2c->adap.dev.parent, "%s entry, ISR: 0x%x, CR: 0x%x\n",
+		__func__, xiic_getreg32(i2c, XIIC_IISR_OFFSET),
+		xiic_getreg8(i2c, XIIC_CR_REG_OFFSET));
+
+	/* Disable Tx interrupts */
+	xiic_irq_dis(i2c, XIIC_INTR_TX_HALF_MASK | XIIC_INTR_TX_EMPTY_MASK);
 
-	/* we want to get all but last byte, because the TX_ERROR IRQ is used
-	 * to inidicate error ACK on the address, and negative ack on the last
-	 * received byte, so to not mix them receive all but last.
-	 * In the case where there is only one byte to receive
-	 * we can check if ERROR and RX full is set at the same time
-	 */
-	rx_watermark = msg->len;
-	if (rx_watermark > IIC_RX_FIFO_DEPTH)
-		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 */
+	if (i2c->dynamic) {
+		u8 bytes;
+		u16 val;
+
+		/* Clear and enable Rx full interrupt. */
+		xiic_irq_clr_en(i2c, XIIC_INTR_RX_FULL_MASK |
+				XIIC_INTR_TX_ERROR_MASK);
+
+		/*
+		 * We want to get all but last byte, because the TX_ERROR IRQ
+		 * is used to indicate error ACK on the address, and
+		 * negative ack on the last received byte, so to not mix
+		 * them receive all but last.
+		 * In the case where there is only one byte to receive
+		 * we can check if ERROR and RX full is set at the same time
+		 */
+		rx_watermark = msg->len;
+		bytes = min_t(u8, rx_watermark, IIC_RX_FIFO_DEPTH);
+		bytes--;
+
+		xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET, bytes);
+
+		local_irq_save(flags);
+		if (!(msg->flags & I2C_M_NOSTART))
+			/* write the address */
+			xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET,
+				      i2c_8bit_addr_from_msg(msg) |
+				      XIIC_TX_DYN_START_MASK);
+
+		xiic_irq_clr_en(i2c, XIIC_INTR_BNB_MASK);
+
+		/* If last message, include dynamic stop bit with length */
+		val = (i2c->nmsgs == 1) ? XIIC_TX_DYN_STOP_MASK : 0;
+		val |= msg->len;
+
+		xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, val);
+		local_irq_restore(flags);
+	} else {
+		/*
+		 * If previous message is Tx, make sure that Tx FIFO is empty
+		 * before starting a new transfer as the repeated start in
+		 * standard mode can corrupt the transaction if there are
+		 * still bytes to be transmitted in FIFO
+		 */
+		if (i2c->prev_msg_tx) {
+			int status;
+
+			status = xiic_wait_tx_empty(i2c);
+			if (status)
+				return;
+		}
+
+		cr = xiic_getreg8(i2c, XIIC_CR_REG_OFFSET);
+
+		/* Set Receive fifo depth */
+		rx_watermark = msg->len;
+		if (rx_watermark > IIC_RX_FIFO_DEPTH) {
+			rfd_set = IIC_RX_FIFO_DEPTH - 1;
+		} else if ((rx_watermark == 1) || (rx_watermark == 0)) {
+			rfd_set = rx_watermark - 1;
+			/* Handle single byte transfer separately */
+			cr |= XIIC_CR_NO_ACK_MASK;
+		} else {
+			rfd_set = rx_watermark - 2;
+		}
+
+		/* Check if RSTA should be set */
+		if (cr & XIIC_CR_MSMS_MASK) {
+			/* Already a master, RSTA should be set */
+			xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, (cr |
+				     XIIC_CR_REPEATED_START_MASK) &
+				     ~(XIIC_CR_DIR_IS_TX_MASK));
+		}
+
+		xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET, rfd_set);
+
+		/* Clear and enable Rx full and transmit complete interrupts */
+		xiic_irq_clr_en(i2c, XIIC_INTR_RX_FULL_MASK |
+				XIIC_INTR_TX_ERROR_MASK);
+
+		/* Write the address */
 		xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET,
-			i2c_8bit_addr_from_msg(msg) | XIIC_TX_DYN_START_MASK);
+			      i2c_8bit_addr_from_msg(msg));
 
-	xiic_irq_clr_en(i2c, XIIC_INTR_BNB_MASK);
+		/* Write to Control Register,to start transaction in Rx mode */
+		if ((cr & XIIC_CR_MSMS_MASK) == 0) {
+			xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, (cr |
+				     XIIC_CR_MSMS_MASK)
+				     & ~(XIIC_CR_DIR_IS_TX_MASK));
+		}
 
-	xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET,
-		msg->len | ((i2c->nmsgs == 1) ? XIIC_TX_DYN_STOP_MASK : 0));
-	local_irq_restore(flags);
+		dev_dbg(i2c->adap.dev.parent, "%s end, ISR: 0x%x, CR: 0x%x\n",
+			__func__, xiic_getreg32(i2c, XIIC_IISR_OFFSET),
+			xiic_getreg8(i2c, XIIC_CR_REG_OFFSET));
+	}
 
 	if (i2c->nmsgs == 1)
 		/* very last, enable bus not busy as well */
@@ -590,10 +795,17 @@ static void xiic_start_recv(struct xiic_i2c *i2c)
 
 	/* the message is tx:ed */
 	i2c->tx_pos = msg->len;
+
+	/* Enable interrupts */
+	xiic_setreg32(i2c, XIIC_DGIER_OFFSET, XIIC_GINTR_ENABLE_MASK);
+
+	i2c->prev_msg_tx = false;
 }
 
 static void xiic_start_send(struct xiic_i2c *i2c)
 {
+	u8 cr = 0;
+	u16 data;
 	struct i2c_msg *msg = i2c->tx_msg;
 
 	xiic_irq_clr(i2c, XIIC_INTR_TX_ERROR_MASK);
@@ -604,22 +816,71 @@ static void xiic_start_send(struct xiic_i2c *i2c)
 		__func__, xiic_getreg32(i2c, XIIC_IISR_OFFSET),
 		xiic_getreg8(i2c, XIIC_CR_REG_OFFSET));
 
-	if (!(msg->flags & I2C_M_NOSTART)) {
-		/* write the address */
-		u16 data = i2c_8bit_addr_from_msg(msg) |
-			XIIC_TX_DYN_START_MASK;
-		if ((i2c->nmsgs == 1) && msg->len == 0)
-			/* no data and last message -> add STOP */
-			data |= XIIC_TX_DYN_STOP_MASK;
+	if (i2c->dynamic) {
+		if (!(msg->flags & I2C_M_NOSTART)) {
+			/* write the address */
+			data = i2c_8bit_addr_from_msg(msg) |
+				XIIC_TX_DYN_START_MASK;
+
+			if (i2c->nmsgs == 1 && msg->len == 0)
+				/* no data and last message -> add STOP */
+				data |= XIIC_TX_DYN_STOP_MASK;
+
+			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));
+	} else {
+		/*
+		 * If previous message is Tx, make sure that Tx FIFO is empty
+		 * before starting a new transfer as the repeated start in
+		 * standard mode can corrupt the transaction if there are
+		 * still bytes to be transmitted in FIFO
+		 */
+		if (i2c->prev_msg_tx) {
+			int status;
+
+			status = xiic_wait_tx_empty(i2c);
+			if (status)
+				return;
+		}
 
+		/* Check if RSTA should be set */
+		cr = xiic_getreg8(i2c, XIIC_CR_REG_OFFSET);
+		if (cr & XIIC_CR_MSMS_MASK) {
+			/* Already a master, RSTA should be set */
+			xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, (cr |
+				     XIIC_CR_REPEATED_START_MASK |
+				     XIIC_CR_DIR_IS_TX_MASK) &
+				     ~(XIIC_CR_NO_ACK_MASK));
+		}
+
+		/* Write address to FIFO */
+		data = i2c_8bit_addr_from_msg(msg);
 		xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, data);
-	}
+		/* Fill fifo */
+		xiic_std_fill_tx_fifo(i2c);
 
-	xiic_fill_tx_fifo(i2c);
+		if ((cr & XIIC_CR_MSMS_MASK) == 0) {
 
-	/* 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);
+			/* Start Tx by writing to CR */
+			cr = xiic_getreg8(i2c, XIIC_CR_REG_OFFSET);
+			xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, cr |
+				     XIIC_CR_MSMS_MASK |
+				     XIIC_CR_DIR_IS_TX_MASK);
+		}
+
+		/* 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);
+	}
+	i2c->prev_msg_tx = true;
 }
 
 static irqreturn_t xiic_isr(int irq, void *dev_id)
@@ -703,7 +964,7 @@ static int xiic_start_xfer(struct xiic_i2c *i2c)
 static int xiic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 {
 	struct xiic_i2c *i2c = i2c_get_adapdata(adap);
-	int err;
+	int err, count;
 
 	dev_dbg(adap->dev.parent, "%s entry SR: 0x%x\n", __func__,
 		xiic_getreg8(i2c, XIIC_SR_REG_OFFSET));
@@ -719,6 +980,21 @@ static int xiic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 	i2c->tx_msg = msgs;
 	i2c->nmsgs = num;
 
+	/* Decide standard mode or Dynamic mode */
+	i2c->dynamic = true;
+
+	/* Initialize prev message type */
+	i2c->prev_msg_tx = false;
+
+	/* Enter standard mode only when read length is > 255 bytes */
+	for (count = 0; count < i2c->nmsgs; count++) {
+		if ((i2c->tx_msg[count].flags & I2C_M_RD) &&
+		    i2c->tx_msg[count].len > MAX_READ_LENGTH_DYNAMIC) {
+			i2c->dynamic = false;
+			break;
+		}
+	}
+
 	err = xiic_start_xfer(i2c);
 	if (err < 0) {
 		dev_err(adap->dev.parent, "Error xiic_start_xfer\n");
@@ -752,16 +1028,11 @@ static const struct i2c_algorithm xiic_algorithm = {
 	.functionality = xiic_func,
 };
 
-static const struct i2c_adapter_quirks xiic_quirks = {
-	.max_read_len = 255,
-};
-
 static const struct i2c_adapter xiic_adapter = {
 	.owner = THIS_MODULE,
 	.name = DRIVER_NAME,
 	.class = I2C_CLASS_DEPRECATED,
 	.algo = &xiic_algorithm,
-	.quirks = &xiic_quirks,
 };
 
 
-- 
2.25.1

Re: [PATCH v2 02/10] i2c: xiic: Add standard mode support for > 255 byte read transfers

[Krzysztof Adamski]

Hi,

I know this patch is quite old but we need the smbus_block_read
functionality, which depends on this one, so I would really like this to
be merged. See my comments below.


Dnia Sat, Jun 26, 2021 at 03:57:58PM +0530, Raviteja Narayanam napisał(a):
> In the current driver implementation, there is a limit of read
> transfer size to 255 bytes as it is using AXI I2C dynamic mode.
> But the IP supports this transfer through standard mode.
>
> So added AXI I2C standard mode support to enable read transfers
> of size more than 255 bytes. The driver scans through the message
> request from user space and selects AXI I2C standard mode if there
> is a read request of more than 255 bytes. Then the whole message goes
> through standard mode Tx and Rx paths.
>
> Signed-off-by: Raviteja Narayanam <raviteja.narayanam@xilinx.com>
> ---
> drivers/i2c/busses/i2c-xiic.c | 367 +++++++++++++++++++++++++++++-----
> 1 file changed, 319 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
> index b0cfd9d15467..004103267e9c 100644
> --- a/drivers/i2c/busses/i2c-xiic.c
> +++ b/drivers/i2c/busses/i2c-xiic.c
> @@ -60,6 +60,8 @@ enum xiic_endian {
>   * @clk: Pointer to AXI4-lite input clock
>   * @state: See STATE_
>   * @singlemaster: Indicates bus is single master
> + * @dynamic: Mode of controller
> + * @prev_msg_tx: Previous message is Tx
>   */
> struct xiic_i2c {
> 	struct device *dev;
> @@ -76,6 +78,8 @@ struct xiic_i2c {
> 	struct clk *clk;
> 	enum xilinx_i2c_state state;
> 	bool singlemaster;
> +	bool dynamic;
> +	bool prev_msg_tx;
> };
>
>
> @@ -144,6 +148,9 @@ struct xiic_i2c {
> #define XIIC_TX_DYN_START_MASK            0x0100 /* 1 = Set dynamic start */
> #define XIIC_TX_DYN_STOP_MASK             0x0200 /* 1 = Set dynamic stop */
>
> +/* Dynamic mode constants */
> +#define MAX_READ_LENGTH_DYNAMIC		255 /* Max length for dynamic read */
> +
> /*
>   * The following constants define the register offsets for the Interrupt
>   * registers. There are some holes in the memory map for reserved addresses
> @@ -270,6 +277,24 @@ static int xiic_clear_rx_fifo(struct xiic_i2c *i2c)
> 	return 0;
> }
>
> +static int xiic_wait_tx_empty(struct xiic_i2c *i2c)
> +{
> +	u8 isr;
> +	unsigned long timeout;
> +
> +	timeout = jiffies + XIIC_I2C_TIMEOUT;
> +	for (isr = xiic_getreg32(i2c, XIIC_IISR_OFFSET);
> +			!(isr & XIIC_INTR_TX_EMPTY_MASK);
> +			isr = xiic_getreg32(i2c, XIIC_IISR_OFFSET)) {
> +		if (time_after(jiffies, timeout)) {
> +			dev_err(i2c->dev, "Timeout waiting at Tx empty\n");
> +			return -ETIMEDOUT;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> static int xiic_reinit(struct xiic_i2c *i2c)
> {
> 	int ret;
> @@ -311,13 +336,14 @@ static void xiic_deinit(struct xiic_i2c *i2c)
>
> static void xiic_read_rx(struct xiic_i2c *i2c)
> {
> -	u8 bytes_in_fifo;
> +	u8 bytes_in_fifo, cr = 0, bytes_to_read = 0;
> +	u32 bytes_rem = 0;
> 	int i;
>
> 	bytes_in_fifo = xiic_getreg8(i2c, XIIC_RFO_REG_OFFSET) + 1;
>
> 	dev_dbg(i2c->adap.dev.parent,
> -		"%s entry, bytes in fifo: %d, msg: %d, SR: 0x%x, CR: 0x%x\n",
> +		"%s entry, bytes in fifo: %d, rem: %d, SR: 0x%x, CR: 0x%x\n",
> 		__func__, bytes_in_fifo, xiic_rx_space(i2c),
> 		xiic_getreg8(i2c, XIIC_SR_REG_OFFSET),
> 		xiic_getreg8(i2c, XIIC_CR_REG_OFFSET));
> @@ -325,13 +351,53 @@ static void xiic_read_rx(struct xiic_i2c *i2c)
> 	if (bytes_in_fifo > xiic_rx_space(i2c))
> 		bytes_in_fifo = xiic_rx_space(i2c);
>
> -	for (i = 0; i < bytes_in_fifo; i++)
> +	bytes_to_read = bytes_in_fifo;
> +
> +	if (!i2c->dynamic) {
> +		bytes_rem = xiic_rx_space(i2c) - bytes_in_fifo;
> +
> +		if (bytes_rem > IIC_RX_FIFO_DEPTH) {
> +			bytes_to_read = bytes_in_fifo;
> +		} else if (bytes_rem > 1) {
> +			bytes_to_read = bytes_rem - 1;
> +		} else if (bytes_rem == 1) {
> +			bytes_to_read = 1;
> +			/* Set NACK in CR to indicate slave transmitter */
> +			cr = xiic_getreg8(i2c, XIIC_CR_REG_OFFSET);
> +			xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, cr |
> +				     XIIC_CR_NO_ACK_MASK);
> +		} else if (bytes_rem == 0) {
> +			bytes_to_read = bytes_in_fifo;
> +
> +			/* Generate stop on the bus if it is last message */
> +			if (i2c->nmsgs == 1) {
> +				cr = xiic_getreg8(i2c, XIIC_CR_REG_OFFSET);
> +				xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, cr &
> +					     ~XIIC_CR_MSMS_MASK);
> +			}
> +
> +			/* Make TXACK=0, clean up for next transaction */
> +			cr = xiic_getreg8(i2c, XIIC_CR_REG_OFFSET);
> +			xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, cr &
> +				     ~XIIC_CR_NO_ACK_MASK);
> +		}
> +	}
> +
> +	/* Read the fifo */
> +	for (i = 0; i < bytes_to_read; i++) {
> 		i2c->rx_msg->buf[i2c->rx_pos++] =
> 			xiic_getreg8(i2c, XIIC_DRR_REG_OFFSET);
> +	}
>
> -	xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET,
> -		(xiic_rx_space(i2c) > IIC_RX_FIFO_DEPTH) ?
> -		IIC_RX_FIFO_DEPTH - 1 :  xiic_rx_space(i2c) - 1);
> +	if (i2c->dynamic) {
> +		u8 bytes;
> +
> +		/* Receive remaining bytes if less than fifo depth */
> +		bytes = min_t(u8, xiic_rx_space(i2c), IIC_RX_FIFO_DEPTH);
> +		bytes--;
> +
> +		xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET, bytes);
> +	}
> }
>
> static int xiic_tx_fifo_space(struct xiic_i2c *i2c)
> @@ -361,6 +427,62 @@ static void xiic_fill_tx_fifo(struct xiic_i2c *i2c)
> 	}
> }
>
> +static void xiic_std_fill_tx_fifo(struct xiic_i2c *i2c)
> +{
> +	u8 fifo_space = xiic_tx_fifo_space(i2c);
> +	u16 data = 0;
> +	int len = xiic_tx_space(i2c);
> +
> +	dev_dbg(i2c->adap.dev.parent, "%s entry, len: %d, fifo space: %d\n",
> +		__func__, len, fifo_space);
> +
> +	if (len > fifo_space)
> +		len = fifo_space;
> +	else if (len && !(i2c->nmsgs > 1))
> +		len--;
> +
> +	while (len--) {
> +		data = i2c->tx_msg->buf[i2c->tx_pos++];
> +		xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, data);
> +	}
> +}
This function looks very similar to the original xiic_fill_tx_fifo. The
only difference is that it does not decrease the len in case of
repeated_start (btw, why?), and it does not set the DYN_STOP bit. But
this could be done conditionally based on i2c->dynamic, instead. No need
for this duplication, in my opinion.

> +
> +static void xiic_send_tx(struct xiic_i2c *i2c)
> +{
> +	dev_dbg(i2c->adap.dev.parent,
> +		"%s entry, rem: %d, SR: 0x%x, CR: 0x%x\n",
> +		__func__, xiic_tx_space(i2c),
> +		xiic_getreg8(i2c, XIIC_SR_REG_OFFSET),
> +		xiic_getreg8(i2c, XIIC_CR_REG_OFFSET));
> +
> +	if (xiic_tx_space(i2c) > 1) {
> +		xiic_std_fill_tx_fifo(i2c);
> +		return;
> +	}
> +
> +	if ((xiic_tx_space(i2c) == 1)) {
> +		u16 data;
> +
> +		if (i2c->nmsgs == 1) {
> +			u8 cr;
> +			int status;
> +
> +			/* Wait till FIFO is empty so STOP is sent last */
> +			status = xiic_wait_tx_empty(i2c);
> +			if (status)
> +				return;
> +
> +			/* Write to CR to stop */
> +			cr = xiic_getreg8(i2c, XIIC_CR_REG_OFFSET);
> +			xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, cr &
> +				     ~XIIC_CR_MSMS_MASK);
> +		}
> +		/* Send last byte */
> +		data = i2c->tx_msg->buf[i2c->tx_pos++];
> +		xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, data);
> +	}
> +}
> +
> static void xiic_wakeup(struct xiic_i2c *i2c, int code)
> {
> 	i2c->tx_msg = NULL;
> @@ -391,7 +513,9 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
> 	dev_dbg(i2c->adap.dev.parent, "%s: SR: 0x%x, msg: %p, nmsgs: %d\n",
> 		__func__, xiic_getreg8(i2c, XIIC_SR_REG_OFFSET),
> 		i2c->tx_msg, i2c->nmsgs);
> -
> +	dev_dbg(i2c->adap.dev.parent, "%s, ISR: 0x%x, CR: 0x%x\n",
> +		__func__, xiic_getreg32(i2c, XIIC_IISR_OFFSET),
> +		xiic_getreg8(i2c, XIIC_CR_REG_OFFSET));
>
> 	/* Service requesting interrupt */
> 	if ((pend & XIIC_INTR_ARB_LOST_MASK) ||
> @@ -465,7 +589,10 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
> 			goto out;
> 		}
>
> -		xiic_fill_tx_fifo(i2c);
> +		if (i2c->dynamic)
> +			xiic_fill_tx_fifo(i2c);
> +		else
> +			xiic_send_tx(i2c);
>
> 		/* current message sent and there is space in the fifo */
> 		if (!xiic_tx_space(i2c) && xiic_tx_fifo_space(i2c) >= 2) {
> @@ -554,35 +681,113 @@ static int xiic_busy(struct xiic_i2c *i2c)
>
> static void xiic_start_recv(struct xiic_i2c *i2c)
> {
> -	u8 rx_watermark;
> +	u16 rx_watermark;
> +	u8 cr = 0, rfd_set = 0;
> 	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);
> +	dev_dbg(i2c->adap.dev.parent, "%s entry, ISR: 0x%x, CR: 0x%x\n",
> +		__func__, xiic_getreg32(i2c, XIIC_IISR_OFFSET),
> +		xiic_getreg8(i2c, XIIC_CR_REG_OFFSET));
> +
> +	/* Disable Tx interrupts */
> +	xiic_irq_dis(i2c, XIIC_INTR_TX_HALF_MASK | XIIC_INTR_TX_EMPTY_MASK);
>
> -	/* we want to get all but last byte, because the TX_ERROR IRQ is used
> -	 * to inidicate error ACK on the address, and negative ack on the last
> -	 * received byte, so to not mix them receive all but last.
> -	 * In the case where there is only one byte to receive
> -	 * we can check if ERROR and RX full is set at the same time
> -	 */
> -	rx_watermark = msg->len;
> -	if (rx_watermark > IIC_RX_FIFO_DEPTH)
> -		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 */
> +	if (i2c->dynamic) {
> +		u8 bytes;
> +		u16 val;
> +
> +		/* Clear and enable Rx full interrupt. */
> +		xiic_irq_clr_en(i2c, XIIC_INTR_RX_FULL_MASK |
> +				XIIC_INTR_TX_ERROR_MASK);
> +
> +		/*
> +		 * We want to get all but last byte, because the TX_ERROR IRQ
> +		 * is used to indicate error ACK on the address, and
> +		 * negative ack on the last received byte, so to not mix
> +		 * them receive all but last.
> +		 * In the case where there is only one byte to receive
> +		 * we can check if ERROR and RX full is set at the same time
> +		 */
> +		rx_watermark = msg->len;
> +		bytes = min_t(u8, rx_watermark, IIC_RX_FIFO_DEPTH);
> +		bytes--;

Do we really want to write 255 to RFD if msg->len == 0? That will set
the compare value in the RX_FIFO_PIRQ register to max value (15) but I
don't understand why we would like to do this.
Also, bits 31:4 are reserved so I think we should not try to touch them.

> +
> +		xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET, bytes);
> +
> +		local_irq_save(flags);
> +		if (!(msg->flags & I2C_M_NOSTART))
> +			/* write the address */
> +			xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET,
> +				      i2c_8bit_addr_from_msg(msg) |
> +				      XIIC_TX_DYN_START_MASK);

When reviewing this patch, I tried to understand how the controller
knows if it should work in dynamic or in stanard mode. My understanding
is that in order to start the dynamic mode logic, we have to set the
DYN_START bit in the TX FIFO when we write an address there. Is this
correct? But we don't do that if I2C_M_NOSTART flag is set so how is
this supposed to work with this flag? I mean, does the controller really
supports doing I2C_M_NOSTART in dynamic mode?

Or does it support it at all? After all, when we skip this, we will
still write to the TX_FIFO register 5 lines below. How is the controller
supposed to know that the len that we write there is *not* actually an
address?

That being said, we do not annouce the I2C_FUNC_NOSTART support so maybe
we should not care at all and just remove the code handling the
I2C_M_NOSTART flag?

> +
> +		xiic_irq_clr_en(i2c, XIIC_INTR_BNB_MASK);
> +
> +		/* If last message, include dynamic stop bit with length */
> +		val = (i2c->nmsgs == 1) ? XIIC_TX_DYN_STOP_MASK : 0;
> +		val |= msg->len;
> +
> +		xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, val);
> +		local_irq_restore(flags);
> +	} else {
> +		/*
> +		 * If previous message is Tx, make sure that Tx FIFO is empty
> +		 * before starting a new transfer as the repeated start in
> +		 * standard mode can corrupt the transaction if there are
> +		 * still bytes to be transmitted in FIFO
> +		 */
> +		if (i2c->prev_msg_tx) {
> +			int status;
> +
> +			status = xiic_wait_tx_empty(i2c);
> +			if (status)
> +				return;
> +		}
> +
> +		cr = xiic_getreg8(i2c, XIIC_CR_REG_OFFSET);
> +
> +		/* Set Receive fifo depth */
> +		rx_watermark = msg->len;
> +		if (rx_watermark > IIC_RX_FIFO_DEPTH) {
> +			rfd_set = IIC_RX_FIFO_DEPTH - 1;
> +		} else if ((rx_watermark == 1) || (rx_watermark == 0)) {
> +			rfd_set = rx_watermark - 1;

Again, do we really want to write 255 to RFD if msg->len == 0?

> +			/* Handle single byte transfer separately */
> +			cr |= XIIC_CR_NO_ACK_MASK;
> +		} else {
> +			rfd_set = rx_watermark - 2;
> +		}
> +
> +		/* Check if RSTA should be set */
> +		if (cr & XIIC_CR_MSMS_MASK) {
> +			/* Already a master, RSTA should be set */
> +			xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, (cr |
> +				     XIIC_CR_REPEATED_START_MASK) &
> +				     ~(XIIC_CR_DIR_IS_TX_MASK));
> +		}
> +
> +		xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET, rfd_set);
> +
> +		/* Clear and enable Rx full and transmit complete interrupts */
> +		xiic_irq_clr_en(i2c, XIIC_INTR_RX_FULL_MASK |
> +				XIIC_INTR_TX_ERROR_MASK);
> +
> +		/* Write the address */
> 		xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET,
> -			i2c_8bit_addr_from_msg(msg) | XIIC_TX_DYN_START_MASK);
> +			      i2c_8bit_addr_from_msg(msg));
>
> -	xiic_irq_clr_en(i2c, XIIC_INTR_BNB_MASK);
> +		/* Write to Control Register,to start transaction in Rx mode */
> +		if ((cr & XIIC_CR_MSMS_MASK) == 0) {
> +			xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, (cr |
> +				     XIIC_CR_MSMS_MASK)
> +				     & ~(XIIC_CR_DIR_IS_TX_MASK));
> +		}
>
> -	xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET,
> -		msg->len | ((i2c->nmsgs == 1) ? XIIC_TX_DYN_STOP_MASK : 0));
> -	local_irq_restore(flags);
> +		dev_dbg(i2c->adap.dev.parent, "%s end, ISR: 0x%x, CR: 0x%x\n",
> +			__func__, xiic_getreg32(i2c, XIIC_IISR_OFFSET),
> +			xiic_getreg8(i2c, XIIC_CR_REG_OFFSET));
> +	}
>
> 	if (i2c->nmsgs == 1)
> 		/* very last, enable bus not busy as well */
> @@ -590,10 +795,17 @@ static void xiic_start_recv(struct xiic_i2c *i2c)
>
> 	/* the message is tx:ed */
> 	i2c->tx_pos = msg->len;
> +
> +	/* Enable interrupts */
> +	xiic_setreg32(i2c, XIIC_DGIER_OFFSET, XIIC_GINTR_ENABLE_MASK);
> +
> +	i2c->prev_msg_tx = false;
> }
>
> static void xiic_start_send(struct xiic_i2c *i2c)
> {
> +	u8 cr = 0;
> +	u16 data;
> 	struct i2c_msg *msg = i2c->tx_msg;
>
> 	xiic_irq_clr(i2c, XIIC_INTR_TX_ERROR_MASK);
> @@ -604,22 +816,71 @@ static void xiic_start_send(struct xiic_i2c *i2c)
> 		__func__, xiic_getreg32(i2c, XIIC_IISR_OFFSET),
> 		xiic_getreg8(i2c, XIIC_CR_REG_OFFSET));
>
> -	if (!(msg->flags & I2C_M_NOSTART)) {
> -		/* write the address */
> -		u16 data = i2c_8bit_addr_from_msg(msg) |
> -			XIIC_TX_DYN_START_MASK;
> -		if ((i2c->nmsgs == 1) && msg->len == 0)
> -			/* no data and last message -> add STOP */
> -			data |= XIIC_TX_DYN_STOP_MASK;
> +	if (i2c->dynamic) {
> +		if (!(msg->flags & I2C_M_NOSTART)) {
> +			/* write the address */
> +			data = i2c_8bit_addr_from_msg(msg) |
> +				XIIC_TX_DYN_START_MASK;
> +
> +			if (i2c->nmsgs == 1 && msg->len == 0)
> +				/* no data and last message -> add STOP */
> +				data |= XIIC_TX_DYN_STOP_MASK;
> +
> +			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));

Before this patch, interrupts were cleared before the call to
xiic_fill_tx_fifo(), now it is the other way around. Was this
intentional? If so, why?

> +	} else {
> +		/*
> +		 * If previous message is Tx, make sure that Tx FIFO is empty
> +		 * before starting a new transfer as the repeated start in
> +		 * standard mode can corrupt the transaction if there are
> +		 * still bytes to be transmitted in FIFO
> +		 */
> +		if (i2c->prev_msg_tx) {
> +			int status;
> +
> +			status = xiic_wait_tx_empty(i2c);
> +			if (status)
> +				return;
> +		}
>
> +		/* Check if RSTA should be set */
> +		cr = xiic_getreg8(i2c, XIIC_CR_REG_OFFSET);
> +		if (cr & XIIC_CR_MSMS_MASK) {
> +			/* Already a master, RSTA should be set */
> +			xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, (cr |
> +				     XIIC_CR_REPEATED_START_MASK |
> +				     XIIC_CR_DIR_IS_TX_MASK) &
> +				     ~(XIIC_CR_NO_ACK_MASK));
> +		}
> +
> +		/* Write address to FIFO */
> +		data = i2c_8bit_addr_from_msg(msg);
> 		xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, data);
> -	}
> +		/* Fill fifo */
> +		xiic_std_fill_tx_fifo(i2c);
>
> -	xiic_fill_tx_fifo(i2c);
> +		if ((cr & XIIC_CR_MSMS_MASK) == 0) {
>
> -	/* 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);
> +			/* Start Tx by writing to CR */
> +			cr = xiic_getreg8(i2c, XIIC_CR_REG_OFFSET);
> +			xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, cr |
> +				     XIIC_CR_MSMS_MASK |
> +				     XIIC_CR_DIR_IS_TX_MASK);
> +		}
> +
> +		/* 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);
> +	}
> +	i2c->prev_msg_tx = true;
> }
>
> static irqreturn_t xiic_isr(int irq, void *dev_id)
> @@ -703,7 +964,7 @@ static int xiic_start_xfer(struct xiic_i2c *i2c)
> static int xiic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> {
> 	struct xiic_i2c *i2c = i2c_get_adapdata(adap);
> -	int err;
> +	int err, count;
>
> 	dev_dbg(adap->dev.parent, "%s entry SR: 0x%x\n", __func__,
> 		xiic_getreg8(i2c, XIIC_SR_REG_OFFSET));
> @@ -719,6 +980,21 @@ static int xiic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> 	i2c->tx_msg = msgs;
> 	i2c->nmsgs = num;
>
> +	/* Decide standard mode or Dynamic mode */
> +	i2c->dynamic = true;
> +
> +	/* Initialize prev message type */
> +	i2c->prev_msg_tx = false;
> +
> +	/* Enter standard mode only when read length is > 255 bytes */
> +	for (count = 0; count < i2c->nmsgs; count++) {
> +		if ((i2c->tx_msg[count].flags & I2C_M_RD) &&
> +		    i2c->tx_msg[count].len > MAX_READ_LENGTH_DYNAMIC) {
> +			i2c->dynamic = false;
> +			break;
> +		}
> +	}
> +
> 	err = xiic_start_xfer(i2c);
> 	if (err < 0) {
> 		dev_err(adap->dev.parent, "Error xiic_start_xfer\n");
> @@ -752,16 +1028,11 @@ static const struct i2c_algorithm xiic_algorithm = {
> 	.functionality = xiic_func,
> };
>
> -static const struct i2c_adapter_quirks xiic_quirks = {
> -	.max_read_len = 255,
> -};
> -
> static const struct i2c_adapter xiic_adapter = {
> 	.owner = THIS_MODULE,
> 	.name = DRIVER_NAME,
> 	.class = I2C_CLASS_DEPRECATED,
> 	.algo = &xiic_algorithm,
> -	.quirks = &xiic_quirks,
> };
>
>

Re: [PATCH v2 02/10] i2c: xiic: Add standard mode support for > 255 byte read transfers

[Michal Simek]

Hi,

On 6/29/22 13:02, Krzysztof Adamski wrote:
> 
> Hi,
> 
> I know this patch is quite old but we need the smbus_block_read
> functionality, which depends on this one, so I would really like this to
> be merged. See my comments below.

Please try this series and let us know if it is working for you or not.

https://lore.kernel.org/all/1656072327-13628-1-git-send-email-manikanta.guntupalli@xilinx.com/

Thanks,
Michal

Re: [PATCH v2 02/10] i2c: xiic: Add standard mode support for > 255 byte read transfers

[Krzysztof Adamski]

Hi Michal,

I did not realize there is a v3 to that, sorry. I have tested those 
patches untill the "Add smbus_block_read functionality" patch and it 
seems to be working fine for me. I will move my comments to this thread 
instead.

Krzysztof

W dniu 29.06.2022 o 13:39, Michal Simek pisze:
> Hi,
>
> On 6/29/22 13:02, Krzysztof Adamski wrote:
>>
>> Hi,
>>
>> I know this patch is quite old but we need the smbus_block_read
>> functionality, which depends on this one, so I would really like this to
>> be merged. See my comments below.
>
> Please try this series and let us know if it is working for you or not.
>
> https://lore.kernel.org/all/1656072327-13628-1-git-send-email-manikanta.guntupalli@xilinx.com/ 
>
>
> Thanks,
> Michal

[PATCH v2 03/10] i2c: xiic: Switch to Xiic standard mode for i2c-read

[Raviteja Narayanam]

Xilinx I2C IP has two modes of operation, both of which implement
I2C transactions. The only difference from sw perspective is the
programming sequence for these modes.
Dynamic mode  -> Simple to program, less number of steps in sequence.
Standard mode -> Gives flexibility, more number of steps in sequence.

In dynamic mode, during the i2c-read transactions, if there is a
delay(> 200us) between the register writes (address & byte count),
read transaction fails. On a system with load, this scenario is
occurring frequently.
To avoid this, switch to standard mode if there is a read request.

Added a quirk to identify the IP version effected by this and follow
the standard mode.

Signed-off-by: Raviteja Narayanam <raviteja.narayanam@xilinx.com>
---
 drivers/i2c/busses/i2c-xiic.c | 54 ++++++++++++++++++++++++++++-------
 1 file changed, 43 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 004103267e9c..c31d0d0a8384 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -33,6 +33,8 @@
 
 #define DRIVER_NAME "xiic-i2c"
 
+#define DYNAMIC_MODE_READ_BROKEN_BIT	BIT(0)
+
 enum xilinx_i2c_state {
 	STATE_DONE,
 	STATE_ERROR,
@@ -62,6 +64,7 @@ enum xiic_endian {
  * @singlemaster: Indicates bus is single master
  * @dynamic: Mode of controller
  * @prev_msg_tx: Previous message is Tx
+ * @quirks: To hold platform specific bug info
  */
 struct xiic_i2c {
 	struct device *dev;
@@ -80,8 +83,12 @@ struct xiic_i2c {
 	bool singlemaster;
 	bool dynamic;
 	bool prev_msg_tx;
+	u32 quirks;
 };
 
+struct xiic_version_data {
+	u32 quirks;
+};
 
 #define XIIC_MSB_OFFSET 0
 #define XIIC_REG_OFFSET (0x100+XIIC_MSB_OFFSET)
@@ -963,6 +970,7 @@ static int xiic_start_xfer(struct xiic_i2c *i2c)
 
 static int xiic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 {
+	bool broken_read, max_read_len;
 	struct xiic_i2c *i2c = i2c_get_adapdata(adap);
 	int err, count;
 
@@ -986,10 +994,21 @@ static int xiic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 	/* Initialize prev message type */
 	i2c->prev_msg_tx = false;
 
-	/* Enter standard mode only when read length is > 255 bytes */
+	/*
+	 * Scan through nmsgs, use dynamic mode when none of the below two
+	 * conditions occur. We need standard mode even if one condition holds
+	 * true in the entire array of messages in a single transfer.
+	 * If read transaction as dynamic mode is broken for delayed reads
+	 * in xlnx,axi-iic-2.0 / xlnx,xps-iic-2.00.a IP versions.
+	 * If read length is > 255 bytes.
+	 */
 	for (count = 0; count < i2c->nmsgs; count++) {
-		if ((i2c->tx_msg[count].flags & I2C_M_RD) &&
-		    i2c->tx_msg[count].len > MAX_READ_LENGTH_DYNAMIC) {
+		broken_read = (i2c->quirks & DYNAMIC_MODE_READ_BROKEN_BIT) &&
+			       (i2c->tx_msg[count].flags & I2C_M_RD);
+		max_read_len = (i2c->tx_msg[count].flags & I2C_M_RD) &&
+				(i2c->tx_msg[count].len > MAX_READ_LENGTH_DYNAMIC);
+
+		if (broken_read || max_read_len) {
 			i2c->dynamic = false;
 			break;
 		}
@@ -1035,11 +1054,23 @@ static const struct i2c_adapter xiic_adapter = {
 	.algo = &xiic_algorithm,
 };
 
+static const struct xiic_version_data xiic_2_00 = {
+	.quirks = DYNAMIC_MODE_READ_BROKEN_BIT,
+};
+
+#if defined(CONFIG_OF)
+static const struct of_device_id xiic_of_match[] = {
+	{ .compatible = "xlnx,xps-iic-2.00.a", .data = &xiic_2_00 },
+	{},
+};
+MODULE_DEVICE_TABLE(of, xiic_of_match);
+#endif
 
 static int xiic_i2c_probe(struct platform_device *pdev)
 {
 	struct xiic_i2c *i2c;
 	struct xiic_i2c_platform_data *pdata;
+	const struct of_device_id *match;
 	struct resource *res;
 	int ret, irq;
 	u8 i;
@@ -1049,6 +1080,13 @@ static int xiic_i2c_probe(struct platform_device *pdev)
 	if (!i2c)
 		return -ENOMEM;
 
+	match = of_match_node(xiic_of_match, pdev->dev.of_node);
+	if (match && match->data) {
+		const struct xiic_version_data *data = match->data;
+
+		i2c->quirks = data->quirks;
+	}
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	i2c->base = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(i2c->base))
@@ -1129,6 +1167,8 @@ static int xiic_i2c_probe(struct platform_device *pdev)
 			i2c_new_client_device(&i2c->adap, pdata->devices + i);
 	}
 
+	dev_info(&pdev->dev, "mmio %08lx irq %d\n", (unsigned long)res->start, irq);
+
 	return 0;
 
 err_clk_dis:
@@ -1160,14 +1200,6 @@ static int xiic_i2c_remove(struct platform_device *pdev)
 	return 0;
 }
 
-#if defined(CONFIG_OF)
-static const struct of_device_id xiic_of_match[] = {
-	{ .compatible = "xlnx,xps-iic-2.00.a", },
-	{},
-};
-MODULE_DEVICE_TABLE(of, xiic_of_match);
-#endif
-
 static int __maybe_unused xiic_i2c_runtime_suspend(struct device *dev)
 {
 	struct xiic_i2c *i2c = dev_get_drvdata(dev);
-- 
2.25.1

Re: [PATCH v2 03/10] i2c: xiic: Switch to Xiic standard mode for i2c-read

[Krzysztof Adamski]

W dniu 26.06.2021 o 12:27, Raviteja Narayanam pisze:
> Xilinx I2C IP has two modes of operation, both of which implement
> I2C transactions. The only difference from sw perspective is the
> programming sequence for these modes.
> Dynamic mode  -> Simple to program, less number of steps in sequence.
> Standard mode -> Gives flexibility, more number of steps in sequence.
>
> In dynamic mode, during the i2c-read transactions, if there is a
> delay(> 200us) between the register writes (address & byte count),
> read transaction fails. On a system with load, this scenario is
> occurring frequently.
> To avoid this, switch to standard mode if there is a read request.
>
> Added a quirk to identify the IP version effected by this and follow
> the standard mode.
>
> Signed-off-by: Raviteja Narayanam <raviteja.narayanam@xilinx.com>

[...]

If those two modes only differ in software complexity but we are not
able to support only the simpler one and we have support for the more
complicated (standard mode) anyways, we know that standard mode
can handle or the cases while dynamic mode cannot, we also know that
dynamic mode is broken on some versions of the core, why do we actually
keep support for dynamic mode?

Krzysztof

Re: [PATCH v2 03/10] i2c: xiic: Switch to Xiic standard mode for i2c-read

[Marek Vasut]

On 6/29/22 14:47, Krzysztof Adamski wrote:
> W dniu 26.06.2021 o 12:27, Raviteja Narayanam pisze:
>> Xilinx I2C IP has two modes of operation, both of which implement
>> I2C transactions. The only difference from sw perspective is the
>> programming sequence for these modes.
>> Dynamic mode  -> Simple to program, less number of steps in sequence.
>> Standard mode -> Gives flexibility, more number of steps in sequence.
>>
>> In dynamic mode, during the i2c-read transactions, if there is a
>> delay(> 200us) between the register writes (address & byte count),
>> read transaction fails. On a system with load, this scenario is
>> occurring frequently.
>> To avoid this, switch to standard mode if there is a read request.
>>
>> Added a quirk to identify the IP version effected by this and follow
>> the standard mode.
>>
>> Signed-off-by: Raviteja Narayanam <raviteja.narayanam@xilinx.com>
> 
> [...]
> 
> If those two modes only differ in software complexity but we are not
> able to support only the simpler one and we have support for the more
> complicated (standard mode) anyways, we know that standard mode
> can handle or the cases while dynamic mode cannot, we also know that
> dynamic mode is broken on some versions of the core, why do we actually
> keep support for dynamic mode?

If I recall it right, the dynamic mode was supposed to handle transfers 
longer than 255 Bytes, which the core cannot do in Standard mode. It is 
needed e.g. by Atmel MXT touch controller. I spent a lot of time 
debugging the race conditions in the XIIC, which I ultimately fixed (the 
patches are upstream), but the long transfers I rather fixed in the MXT 
driver instead.

I also recall there was supposed to be some update for the XIIC core 
coming with newer vivado, but I might be wrong about that.

Re: [PATCH v2 03/10] i2c: xiic: Switch to Xiic standard mode for i2c-read

[Krzysztof Adamski]

Hi Marek,

W dniu 29.06.2022 o 16:05, Marek Vasut pisze:
>> [...]
>>
>> If those two modes only differ in software complexity but we are not
>> able to support only the simpler one and we have support for the more
>> complicated (standard mode) anyways, we know that standard mode
>> can handle or the cases while dynamic mode cannot, we also know that
>> dynamic mode is broken on some versions of the core, why do we actually
>> keep support for dynamic mode?
>
> If I recall it right, the dynamic mode was supposed to handle 
> transfers longer than 255 Bytes, which the core cannot do in Standard 
> mode. It is needed e.g. by Atmel MXT touch controller. I spent a lot 
> of time debugging the race conditions in the XIIC, which I ultimately 
> fixed (the patches are upstream), but the long transfers I rather 
> fixed in the MXT driver instead.
>
> I also recall there was supposed to be some update for the XIIC core 
> coming with newer vivado, but I might be wrong about that.

It seems to be the other way around - dynamic mode is limited to 255 
bytes - when you trigger dynamic mode you first write the address of the 
slave to the FIFO, then you write the length as one byte so you can't 
request more than 255 bytes. So *standard* mode is used for those 
messages. In other words - dynamic mode is the one that is more limited 
- everything that you can do in dynamic mode you can also do in standard 
mode. So why don't we use standard mode always for everything?

Krzysztof

Re: [PATCH v2 03/10] i2c: xiic: Switch to Xiic standard mode for i2c-read

[Marek Vasut]

On 6/29/22 16:09, Krzysztof Adamski wrote:
> Hi Marek,
> 
> W dniu 29.06.2022 o 16:05, Marek Vasut pisze:
>>> [...]
>>>
>>> If those two modes only differ in software complexity but we are not
>>> able to support only the simpler one and we have support for the more
>>> complicated (standard mode) anyways, we know that standard mode
>>> can handle or the cases while dynamic mode cannot, we also know that
>>> dynamic mode is broken on some versions of the core, why do we actually
>>> keep support for dynamic mode?
>>
>> If I recall it right, the dynamic mode was supposed to handle 
>> transfers longer than 255 Bytes, which the core cannot do in Standard 
>> mode. It is needed e.g. by Atmel MXT touch controller. I spent a lot 
>> of time debugging the race conditions in the XIIC, which I ultimately 
>> fixed (the patches are upstream), but the long transfers I rather 
>> fixed in the MXT driver instead.
>>
>> I also recall there was supposed to be some update for the XIIC core 
>> coming with newer vivado, but I might be wrong about that.
> 
> It seems to be the other way around - dynamic mode is limited to 255 
> bytes - when you trigger dynamic mode you first write the address of the 
> slave to the FIFO, then you write the length as one byte so you can't 
> request more than 255 bytes. So *standard* mode is used for those 
> messages. In other words - dynamic mode is the one that is more limited 
> - everything that you can do in dynamic mode you can also do in standard 
> mode. So why don't we use standard mode always for everything?

Sigh, it's been a year since I looked into this, sorry.

One of the modes is maybe not supported on all the XIIC core instances ?

RE: [PATCH v2 03/10] i2c: xiic: Switch to Xiic standard mode for i2c-read

[Datta, Shubhrajyoti]

[AMD Official Use Only - General]

Hi Krzysztof, 

> -----Original Message-----
> From: Krzysztof Adamski <krzysztof.adamski@nokia.com>
> Sent: Wednesday, June 29, 2022 7:40 PM
> To: Marek Vasut <marex@denx.de>; Raviteja Narayanam
> <raviteja.narayanam@xilinx.com>; linux-i2c@vger.kernel.org;
> michal.simek@xilinx.com
> Cc: linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> git@xilinx.com; joe@perches.com
> Subject: Re: [PATCH v2 03/10] i2c: xiic: Switch to Xiic standard mode for i2c-
> read
> 
> [CAUTION: External Email]
> 
> CAUTION: This message has originated from an External Source. Please use
> proper judgment and caution when opening attachments, clicking links, or
> responding to this email.
> 
> 
> Hi Marek,
> 
> W dniu 29.06.2022 o 16:05, Marek Vasut pisze:
> >> [...]
> >>
> >> If those two modes only differ in software complexity but we are not
> >> able to support only the simpler one and we have support for the more
> >> complicated (standard mode) anyways, we know that standard mode can
> >> handle or the cases while dynamic mode cannot, we also know that
> >> dynamic mode is broken on some versions of the core, why do we
> >> actually keep support for dynamic mode?
> >
> > If I recall it right, the dynamic mode was supposed to handle
> > transfers longer than 255 Bytes, which the core cannot do in Standard
> > mode. It is needed e.g. by Atmel MXT touch controller. I spent a lot
> > of time debugging the race conditions in the XIIC, which I ultimately
> > fixed (the patches are upstream), but the long transfers I rather
> > fixed in the MXT driver instead.
> >
> > I also recall there was supposed to be some update for the XIIC core
> > coming with newer vivado, but I might be wrong about that.
> 
> It seems to be the other way around - dynamic mode is limited to 255 bytes -
> when you trigger dynamic mode you first write the address of the slave to
> the FIFO, then you write the length as one byte so you can't request more
> than 255 bytes. So *standard* mode is used for those messages. In other
> words - dynamic mode is the one that is more limited
> - everything that you can do in dynamic mode you can also do in standard
> mode. So why don't we use standard mode always for everything?

However  the current mode is dynamic mode so for less than 255 we can use dynamic mode.(the current behavior will not change)
Also the dynamic mode  is  nicer on the processor resources. We set the bytes and the controller takes care of 
transferring.

However do not have any strong views open to suggestions.

> 
> Krzysztof

Re: [PATCH v2 03/10] i2c: xiic: Switch to Xiic standard mode for i2c-read

[Krzysztof Adamski]

W dniu 30.06.2022 o 10:23, Datta, Shubhrajyoti pisze:
> [AMD Official Use Only - General]
>
> Hi Krzysztof,
>
>> -----Original Message-----
>> From: Krzysztof Adamski <krzysztof.adamski@nokia.com>
>> Sent: Wednesday, June 29, 2022 7:40 PM
>> To: Marek Vasut <marex@denx.de>; Raviteja Narayanam
>> <raviteja.narayanam@xilinx.com>; linux-i2c@vger.kernel.org;
>> michal.simek@xilinx.com
>> Cc: linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
>> git@xilinx.com; joe@perches.com
>> Subject: Re: [PATCH v2 03/10] i2c: xiic: Switch to Xiic standard mode for i2c-
>> read
>>
>> [CAUTION: External Email]
>>
>> CAUTION: This message has originated from an External Source. Please use
>> proper judgment and caution when opening attachments, clicking links, or
>> responding to this email.
>>
>>
>> Hi Marek,
>>
>> W dniu 29.06.2022 o 16:05, Marek Vasut pisze:
>>>> [...]
>>>>
>>>> If those two modes only differ in software complexity but we are not
>>>> able to support only the simpler one and we have support for the more
>>>> complicated (standard mode) anyways, we know that standard mode can
>>>> handle or the cases while dynamic mode cannot, we also know that
>>>> dynamic mode is broken on some versions of the core, why do we
>>>> actually keep support for dynamic mode?
>>> If I recall it right, the dynamic mode was supposed to handle
>>> transfers longer than 255 Bytes, which the core cannot do in Standard
>>> mode. It is needed e.g. by Atmel MXT touch controller. I spent a lot
>>> of time debugging the race conditions in the XIIC, which I ultimately
>>> fixed (the patches are upstream), but the long transfers I rather
>>> fixed in the MXT driver instead.
>>>
>>> I also recall there was supposed to be some update for the XIIC core
>>> coming with newer vivado, but I might be wrong about that.
>> It seems to be the other way around - dynamic mode is limited to 255 bytes -
>> when you trigger dynamic mode you first write the address of the slave to
>> the FIFO, then you write the length as one byte so you can't request more
>> than 255 bytes. So *standard* mode is used for those messages. In other
>> words - dynamic mode is the one that is more limited
>> - everything that you can do in dynamic mode you can also do in standard
>> mode. So why don't we use standard mode always for everything?
> However  the current mode is dynamic mode so for less than 255 we can use dynamic mode.(the current behavior will not change)
> Also the dynamic mode  is  nicer on the processor resources. We set the bytes and the controller takes care of
> transferring.
>
> However do not have any strong views open to suggestions.

All I'm saying is that before this patchset, the dynamic mode was used 
in all cases and it made sense - it is easier to work with. But it 
turned out it has its limitations and support for standard mode was 
added with several cases that switch to that mode. The commit message 
suggests that the only difference is in how complicated the code for 
handling them is, does not say why dynamic mode might still be 
preferred. And supporting both of them complicates the code noticeably. 
My understanding now is that the code struggles to use the dynamic mode 
in all cases that it can because that produces less interrupts and so it 
is slightly lighter on resources. So it is a code complication vs 
effectiveness tradeoff. Since this is I2C - a slow bus, I'm not sure it 
is worth it but also don't have strong opinion on that. If nothing else, 
I think it would make sense to update the commit message a little bit to 
better explain why it is worth keeping both modes.

Krzysztof

RE: [PATCH v2 03/10] i2c: xiic: Switch to Xiic standard mode for i2c-read

[Datta, Shubhrajyoti]

[AMD Official Use Only - General]



> -----Original Message-----
> From: Krzysztof Adamski <krzysztof.adamski@nokia.com>
> Sent: Friday, July 1, 2022 12:32 PM
> To: Datta, Shubhrajyoti <shubhrajyoti.datta@amd.com>; Marek Vasut
> <marex@denx.de>; Raviteja Narayanam <raviteja.narayanam@xilinx.com>;
> linux-i2c@vger.kernel.org; michal.simek@xilinx.com
> Cc: linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> git@xilinx.com; joe@perches.com
> Subject: Re: [PATCH v2 03/10] i2c: xiic: Switch to Xiic standard mode for i2c-
> read
> 
> [CAUTION: External Email]
> 
> W dniu 30.06.2022 o 10:23, Datta, Shubhrajyoti pisze:
> > [AMD Official Use Only - General]
> >
> > Hi Krzysztof,
> >
> >> -----Original Message-----
> >> From: Krzysztof Adamski <krzysztof.adamski@nokia.com>
> >> Sent: Wednesday, June 29, 2022 7:40 PM
> >> To: Marek Vasut <marex@denx.de>; Raviteja Narayanam
> >> <raviteja.narayanam@xilinx.com>; linux-i2c@vger.kernel.org;
> >> michal.simek@xilinx.com
> >> Cc: linux-arm-kernel@lists.infradead.org;
> >> linux-kernel@vger.kernel.org; git@xilinx.com; joe@perches.com
> >> Subject: Re: [PATCH v2 03/10] i2c: xiic: Switch to Xiic standard mode
> >> for i2c- read
> >>
> >> [CAUTION: External Email]
> >>
> >> CAUTION: This message has originated from an External Source. Please
> >> use proper judgment and caution when opening attachments, clicking
> >> links, or responding to this email.
> >>
> >>
> >> Hi Marek,
> >>
> >> W dniu 29.06.2022 o 16:05, Marek Vasut pisze:
> >>>> [...]
> >>>>
> >>>> If those two modes only differ in software complexity but we are
> >>>> not able to support only the simpler one and we have support for
> >>>> the more complicated (standard mode) anyways, we know that
> standard
> >>>> mode can handle or the cases while dynamic mode cannot, we also
> >>>> know that dynamic mode is broken on some versions of the core, why
> >>>> do we actually keep support for dynamic mode?
> >>> If I recall it right, the dynamic mode was supposed to handle
> >>> transfers longer than 255 Bytes, which the core cannot do in
> >>> Standard mode. It is needed e.g. by Atmel MXT touch controller. I
> >>> spent a lot of time debugging the race conditions in the XIIC, which
> >>> I ultimately fixed (the patches are upstream), but the long
> >>> transfers I rather fixed in the MXT driver instead.
> >>>
> >>> I also recall there was supposed to be some update for the XIIC core
> >>> coming with newer vivado, but I might be wrong about that.
> >> It seems to be the other way around - dynamic mode is limited to 255
> >> bytes - when you trigger dynamic mode you first write the address of
> >> the slave to the FIFO, then you write the length as one byte so you
> >> can't request more than 255 bytes. So *standard* mode is used for
> >> those messages. In other words - dynamic mode is the one that is more
> >> limited
> >> - everything that you can do in dynamic mode you can also do in
> >> standard mode. So why don't we use standard mode always for
> everything?
> > However  the current mode is dynamic mode so for less than 255 we can
> > use dynamic mode.(the current behavior will not change) Also the
> > dynamic mode  is  nicer on the processor resources. We set the bytes and
> the controller takes care of transferring.
> >
> > However do not have any strong views open to suggestions.
> 
> All I'm saying is that before this patchset, the dynamic mode was used in all
> cases and it made sense - it is easier to work with. But it turned out it has its
> limitations and support for standard mode was added with several cases that
> switch to that mode. The commit message suggests that the only difference is
> in how complicated the code for handling them is, does not say why dynamic
> mode might still be preferred. And supporting both of them complicates the
> code noticeably.
> My understanding now is that the code struggles to use the dynamic mode in
> all cases that it can because that produces less interrupts and so it is slightly
> lighter on resources. So it is a code complication vs effectiveness tradeoff.
> Since this is I2C - a slow bus, I'm not sure it is worth it but also don't have
> strong opinion on that. If nothing else, I think it would make sense to update
> the commit message a little bit to better explain why it is worth keeping both
> modes.

Will update the commit message in the next version.

> 
> Krzysztof

[PATCH v2 04/10] i2c: xiic: Remove interrupt enable/disable in Rx path

[Raviteja Narayanam]

As the 'DYNAMIC_MODE_READ_BROKEN_BIT' quirk is added in the driver,
we no longer enter dynamic mode for the effected IP versions.
So, remove local_irq_save/local_irq_restore APIs from driver.

Signed-off-by: Raviteja Narayanam <raviteja.narayanam@xilinx.com>
---
 drivers/i2c/busses/i2c-xiic.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index c31d0d0a8384..72c46cdfa712 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -691,7 +691,6 @@ static void xiic_start_recv(struct xiic_i2c *i2c)
 	u16 rx_watermark;
 	u8 cr = 0, rfd_set = 0;
 	struct i2c_msg *msg = i2c->rx_msg = i2c->tx_msg;
-	unsigned long flags;
 
 	dev_dbg(i2c->adap.dev.parent, "%s entry, ISR: 0x%x, CR: 0x%x\n",
 		__func__, xiic_getreg32(i2c, XIIC_IISR_OFFSET),
@@ -722,7 +721,6 @@ static void xiic_start_recv(struct xiic_i2c *i2c)
 
 		xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET, bytes);
 
-		local_irq_save(flags);
 		if (!(msg->flags & I2C_M_NOSTART))
 			/* write the address */
 			xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET,
@@ -736,7 +734,6 @@ static void xiic_start_recv(struct xiic_i2c *i2c)
 		val |= msg->len;
 
 		xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, val);
-		local_irq_restore(flags);
 	} else {
 		/*
 		 * If previous message is Tx, make sure that Tx FIFO is empty
-- 
2.25.1

[PATCH v2 05/10] dt-bindings: i2c: xiic: Add 'xlnx,axi-iic-2.1' to compatible

[Raviteja Narayanam]

Added the xilinx I2C new version 'xlnx,axi-iic-2.1' string to compatible

Signed-off-by: Raviteja Narayanam <raviteja.narayanam@xilinx.com>
---
 .../devicetree/bindings/i2c/xlnx,xps-iic-2.00.a.yaml          | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/i2c/xlnx,xps-iic-2.00.a.yaml b/Documentation/devicetree/bindings/i2c/xlnx,xps-iic-2.00.a.yaml
index 715dcfa5a922..e516c1ed358c 100644
--- a/Documentation/devicetree/bindings/i2c/xlnx,xps-iic-2.00.a.yaml
+++ b/Documentation/devicetree/bindings/i2c/xlnx,xps-iic-2.00.a.yaml
@@ -14,7 +14,9 @@ allOf:
 
 properties:
   compatible:
-    const: xlnx,xps-iic-2.00.a
+    enum:
+      - xlnx,xps-iic-2.00.a
+      - xlnx,axi-iic-2.1
 
   reg:
     maxItems: 1
-- 
2.25.1

[PATCH v2 06/10] i2c: xiic: Update compatible with new IP version

[Raviteja Narayanam]

Xilinx AXI I2C IP is updated with a bug fix for dynamic mode reads.
Older IPs are handled with a workaround in which they are using
xiic standard mode for all these effected use cases.
Added the new IP version to compatible.

Signed-off-by: Raviteja Narayanam <raviteja.narayanam@xilinx.com>
---
 drivers/i2c/busses/i2c-xiic.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 72c46cdfa712..007a78215a90 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -1058,6 +1058,7 @@ static const struct xiic_version_data xiic_2_00 = {
 #if defined(CONFIG_OF)
 static const struct of_device_id xiic_of_match[] = {
 	{ .compatible = "xlnx,xps-iic-2.00.a", .data = &xiic_2_00 },
+	{ .compatible = "xlnx,axi-iic-2.1", },
 	{},
 };
 MODULE_DEVICE_TABLE(of, xiic_of_match);
-- 
2.25.1

[PATCH v2 07/10] i2c: xiic: Return value of xiic_reinit

[Raviteja Narayanam]

From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>

Check the return value of xiic_reinit.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
Signed-off-by: Raviteja Narayanam <raviteja.narayanam@xilinx.com>
---
 drivers/i2c/busses/i2c-xiic.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 007a78215a90..0d76261f1841 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -504,6 +504,7 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
 	struct xiic_i2c *i2c = dev_id;
 	u32 pend, isr, ier;
 	u32 clr = 0;
+	int ret;
 
 	/* Get the interrupt Status from the IPIF. There is no clearing of
 	 * interrupts in the IPIF. Interrupts must be cleared at the source.
@@ -540,7 +541,9 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
 		 * fifos and the next message is a TX with len 0 (only addr)
 		 * reset the IP instead of just flush fifos
 		 */
-		xiic_reinit(i2c);
+		ret = xiic_reinit(i2c);
+		if (!ret)
+			dev_dbg(i2c->adap.dev.parent, "reinit failed\n");
 
 		if (i2c->rx_msg)
 			xiic_wakeup(i2c, STATE_ERROR);
-- 
2.25.1

[PATCH v2 08/10] i2c: xiic: Fix the type check for xiic_wakeup

[Raviteja Narayanam]

From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>

Fix the coverity warning
mixed_enum_type: enumerated type mixed with another type

We are passing an enum in the xiic_wakeup lets change
the function parameters to reflect that.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
Signed-off-by: Raviteja Narayanam <raviteja.narayanam@xilinx.com>
---
 drivers/i2c/busses/i2c-xiic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 0d76261f1841..3e81ec08b001 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -490,7 +490,7 @@ static void xiic_send_tx(struct xiic_i2c *i2c)
 	}
 }
 
-static void xiic_wakeup(struct xiic_i2c *i2c, int code)
+static void xiic_wakeup(struct xiic_i2c *i2c, enum xilinx_i2c_state code)
 {
 	i2c->tx_msg = NULL;
 	i2c->rx_msg = NULL;
-- 
2.25.1

[PATCH v2 09/10] i2c: xiic: Fix coding style issues

[Raviteja Narayanam]

From: Michal Simek <michal.simek@xilinx.com>

Most of these stuff are reported by checkpatch.
But fixes are:
- Incorrect indetation
- Missing blank line after variable declaration
- Additional ()
- Missing spaces around +
- Else after if with return
- Missing parenthesis when if has them
- Newlines
- Remove MODULE_ALIAS - none is really using it
- Changing msleep to usleep_range
- Other trivial fixes

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Signed-off-by: Raviteja Narayanam <raviteja.narayanam@xilinx.com>
---
 drivers/i2c/busses/i2c-xiic.c | 67 ++++++++++++++++++-----------------
 1 file changed, 34 insertions(+), 33 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 3e81ec08b001..26fc174b8e95 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -72,7 +72,7 @@ struct xiic_i2c {
 	wait_queue_head_t wait;
 	struct i2c_adapter adap;
 	struct i2c_msg *tx_msg;
-	struct mutex lock;
+	struct mutex lock; /* Locking between isr and new xfer */
 	unsigned int tx_pos;
 	unsigned int nmsgs;
 	struct i2c_msg *rx_msg;
@@ -91,22 +91,22 @@ struct xiic_version_data {
 };
 
 #define XIIC_MSB_OFFSET 0
-#define XIIC_REG_OFFSET (0x100+XIIC_MSB_OFFSET)
+#define XIIC_REG_OFFSET (0x100 + XIIC_MSB_OFFSET)
 
 /*
  * Register offsets in bytes from RegisterBase. Three is added to the
  * base offset to access LSB (IBM style) of the word
  */
-#define XIIC_CR_REG_OFFSET   (0x00+XIIC_REG_OFFSET)	/* Control Register   */
-#define XIIC_SR_REG_OFFSET   (0x04+XIIC_REG_OFFSET)	/* Status Register    */
-#define XIIC_DTR_REG_OFFSET  (0x08+XIIC_REG_OFFSET)	/* Data Tx Register   */
-#define XIIC_DRR_REG_OFFSET  (0x0C+XIIC_REG_OFFSET)	/* Data Rx Register   */
-#define XIIC_ADR_REG_OFFSET  (0x10+XIIC_REG_OFFSET)	/* Address Register   */
-#define XIIC_TFO_REG_OFFSET  (0x14+XIIC_REG_OFFSET)	/* Tx FIFO Occupancy  */
-#define XIIC_RFO_REG_OFFSET  (0x18+XIIC_REG_OFFSET)	/* Rx FIFO Occupancy  */
-#define XIIC_TBA_REG_OFFSET  (0x1C+XIIC_REG_OFFSET)	/* 10 Bit Address reg */
-#define XIIC_RFD_REG_OFFSET  (0x20+XIIC_REG_OFFSET)	/* Rx FIFO Depth reg  */
-#define XIIC_GPO_REG_OFFSET  (0x24+XIIC_REG_OFFSET)	/* Output Register    */
+#define XIIC_CR_REG_OFFSET   (0x00 + XIIC_REG_OFFSET)	/* Control Register   */
+#define XIIC_SR_REG_OFFSET   (0x04 + XIIC_REG_OFFSET)	/* Status Register    */
+#define XIIC_DTR_REG_OFFSET  (0x08 + XIIC_REG_OFFSET)	/* Data Tx Register   */
+#define XIIC_DRR_REG_OFFSET  (0x0C + XIIC_REG_OFFSET)	/* Data Rx Register   */
+#define XIIC_ADR_REG_OFFSET  (0x10 + XIIC_REG_OFFSET)	/* Address Register   */
+#define XIIC_TFO_REG_OFFSET  (0x14 + XIIC_REG_OFFSET)	/* Tx FIFO Occupancy  */
+#define XIIC_RFO_REG_OFFSET  (0x18 + XIIC_REG_OFFSET)	/* Rx FIFO Occupancy  */
+#define XIIC_TBA_REG_OFFSET  (0x1C + XIIC_REG_OFFSET)	/* 10 Bit Address reg */
+#define XIIC_RFD_REG_OFFSET  (0x20 + XIIC_REG_OFFSET)	/* Rx FIFO Depth reg  */
+#define XIIC_GPO_REG_OFFSET  (0x24 + XIIC_REG_OFFSET)	/* Output Register    */
 
 /* Control Register masks */
 #define XIIC_CR_ENABLE_DEVICE_MASK        0x01	/* Device enable = 1      */
@@ -244,18 +244,21 @@ static inline int xiic_getreg32(struct xiic_i2c *i2c, int reg)
 static inline void xiic_irq_dis(struct xiic_i2c *i2c, u32 mask)
 {
 	u32 ier = xiic_getreg32(i2c, XIIC_IIER_OFFSET);
+
 	xiic_setreg32(i2c, XIIC_IIER_OFFSET, ier & ~mask);
 }
 
 static inline void xiic_irq_en(struct xiic_i2c *i2c, u32 mask)
 {
 	u32 ier = xiic_getreg32(i2c, XIIC_IIER_OFFSET);
+
 	xiic_setreg32(i2c, XIIC_IIER_OFFSET, ier | mask);
 }
 
 static inline void xiic_irq_clr(struct xiic_i2c *i2c, u32 mask)
 {
 	u32 isr = xiic_getreg32(i2c, XIIC_IISR_OFFSET);
+
 	xiic_setreg32(i2c, XIIC_IISR_OFFSET, isr & mask);
 }
 
@@ -425,7 +428,8 @@ static void xiic_fill_tx_fifo(struct xiic_i2c *i2c)
 
 	while (len--) {
 		u16 data = i2c->tx_msg->buf[i2c->tx_pos++];
-		if ((xiic_tx_space(i2c) == 0) && (i2c->nmsgs == 1)) {
+
+		if (!xiic_tx_space(i2c) && i2c->nmsgs == 1) {
 			/* last message in transfer -> STOP */
 			data |= XIIC_TX_DYN_STOP_MASK;
 			dev_dbg(i2c->adap.dev.parent, "%s TX STOP\n", __func__);
@@ -527,8 +531,8 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
 
 	/* Service requesting interrupt */
 	if ((pend & XIIC_INTR_ARB_LOST_MASK) ||
-		((pend & XIIC_INTR_TX_ERROR_MASK) &&
-		!(pend & XIIC_INTR_RX_FULL_MASK))) {
+	    ((pend & XIIC_INTR_TX_ERROR_MASK) &&
+	     !(pend & XIIC_INTR_RX_FULL_MASK))) {
 		/* bus arbritration lost, or...
 		 * Transmit error _OR_ RX completed
 		 * if this happens when RX_FULL is not set
@@ -672,9 +676,8 @@ static int xiic_busy(struct xiic_i2c *i2c)
 	 * should ignore it, since bus will never be released and i2c will be
 	 * stuck forever.
 	 */
-	if (i2c->singlemaster) {
+	if (i2c->singlemaster)
 		return 0;
-	}
 
 	/* for instance if previous transfer was terminated due to TX error
 	 * it might be that the bus is on it's way to become available
@@ -682,7 +685,7 @@ static int xiic_busy(struct xiic_i2c *i2c)
 	 */
 	err = xiic_bus_busy(i2c);
 	while (err && tries--) {
-		msleep(1);
+		usleep_range(1000, 2000);
 		err = xiic_bus_busy(i2c);
 	}
 
@@ -874,7 +877,6 @@ static void xiic_start_send(struct xiic_i2c *i2c)
 		xiic_std_fill_tx_fifo(i2c);
 
 		if ((cr & XIIC_CR_MSMS_MASK) == 0) {
-
 			/* Start Tx by writing to CR */
 			cr = xiic_getreg8(i2c, XIIC_CR_REG_OFFSET);
 			xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, cr |
@@ -898,9 +900,6 @@ static irqreturn_t xiic_isr(int irq, void *dev_id)
 	/* 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;
@@ -914,6 +913,7 @@ static void __xiic_start_xfer(struct xiic_i2c *i2c)
 {
 	int first = 1;
 	int fifo_space = xiic_tx_fifo_space(i2c);
+
 	dev_dbg(i2c->adap.dev.parent, "%s entry, msg: %p, fifos space: %d\n",
 		__func__, i2c->tx_msg, fifo_space);
 
@@ -928,19 +928,20 @@ static void __xiic_start_xfer(struct xiic_i2c *i2c)
 			i2c->nmsgs--;
 			i2c->tx_msg++;
 			i2c->tx_pos = 0;
-		} else
+		} 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;
-			}
+		}
+
+		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);
@@ -951,12 +952,12 @@ static void __xiic_start_xfer(struct xiic_i2c *i2c)
 	 */
 	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)
 {
 	int ret;
+
 	mutex_lock(&i2c->lock);
 
 	ret = xiic_reinit(i2c);
@@ -1020,8 +1021,8 @@ static int xiic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 		goto out;
 	}
 
-	if (wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
-		(i2c->state == STATE_DONE), HZ)) {
+	if (wait_event_timeout(i2c->wait, i2c->state == STATE_ERROR ||
+			       i2c->state == STATE_DONE, HZ)) {
 		err = (i2c->state == STATE_DONE) ? num : -EIO;
 		goto out;
 	} else {
@@ -1228,6 +1229,7 @@ static const struct dev_pm_ops xiic_dev_pm_ops = {
 	SET_RUNTIME_PM_OPS(xiic_i2c_runtime_suspend,
 			   xiic_i2c_runtime_resume, NULL)
 };
+
 static struct platform_driver xiic_i2c_driver = {
 	.probe   = xiic_i2c_probe,
 	.remove  = xiic_i2c_remove,
@@ -1243,4 +1245,3 @@ module_platform_driver(xiic_i2c_driver);
 MODULE_AUTHOR("info@mocean-labs.com");
 MODULE_DESCRIPTION("Xilinx I2C bus driver");
 MODULE_LICENSE("GPL v2");
-MODULE_ALIAS("platform:"DRIVER_NAME);
-- 
2.25.1

[PATCH v2 10/10] i2c: xiic: Add smbus_block_read functionality

[Raviteja Narayanam]

smbus_block_read is added to xiic driver to read from few sensors
which support this command. Since the number of bytes to read is not
known prior to transfer, we are using xiic standard mode for low level
control of IP.

Signed-off-by: Raviteja Narayanam <raviteja.narayanam@xilinx.com>
---
 drivers/i2c/busses/i2c-xiic.c | 80 ++++++++++++++++++++++++++++++++---
 1 file changed, 74 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 26fc174b8e95..931b9bce87fe 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -65,6 +65,7 @@ enum xiic_endian {
  * @dynamic: Mode of controller
  * @prev_msg_tx: Previous message is Tx
  * @quirks: To hold platform specific bug info
+ * @smbus_block_read: Flag to handle block read
  */
 struct xiic_i2c {
 	struct device *dev;
@@ -84,6 +85,7 @@ struct xiic_i2c {
 	bool dynamic;
 	bool prev_msg_tx;
 	u32 quirks;
+	bool smbus_block_read;
 };
 
 struct xiic_version_data {
@@ -344,6 +346,54 @@ static void xiic_deinit(struct xiic_i2c *i2c)
 	xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, cr & ~XIIC_CR_ENABLE_DEVICE_MASK);
 }
 
+static void xiic_smbus_block_read_setup(struct xiic_i2c *i2c)
+{
+	u8 rxmsg_len;
+	u8 rfd_set = 0;
+
+	/*
+	 * Clear the I2C_M_RECV_LEN flag to avoid setting
+	 * message length again
+	 */
+	i2c->rx_msg->flags &= ~I2C_M_RECV_LEN;
+
+	/* Set smbus_block_read flag to identify in isr */
+	i2c->smbus_block_read = true;
+
+	/* Read byte from rx fifo and set message length */
+	rxmsg_len  = xiic_getreg8(i2c, XIIC_DRR_REG_OFFSET);
+
+	i2c->rx_msg->buf[i2c->rx_pos++] = rxmsg_len;
+
+	/* Check if received length is valid */
+	if (rxmsg_len <= I2C_SMBUS_BLOCK_MAX) {
+		/* Set Receive fifo depth */
+		if (rxmsg_len > IIC_RX_FIFO_DEPTH) {
+			rfd_set = IIC_RX_FIFO_DEPTH - 1;
+			i2c->rx_msg->len = rxmsg_len + 1;
+		} else if ((rxmsg_len == 1) ||
+			   (rxmsg_len == 0)) {
+			/*
+			 * Minimum of 3 bytes required to exit cleanly. 1 byte
+			 * already received, Second byte is being received. Have
+			 * to set NACK in read_rx before receiving the last byte
+			 */
+			i2c->rx_msg->len = 3;
+		} else {
+			rfd_set = rxmsg_len - 2;
+			i2c->rx_msg->len = rxmsg_len + 1;
+		}
+		xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET, rfd_set);
+
+		return;
+	}
+
+	/* Invalid message length, trigger STATE_ERROR with tx_msg_len in ISR */
+	i2c->tx_msg->len = 3;
+	i2c->smbus_block_read = false;
+	dev_err(i2c->adap.dev.parent, "smbus_block_read Invalid msg length\n");
+}
+
 static void xiic_read_rx(struct xiic_i2c *i2c)
 {
 	u8 bytes_in_fifo, cr = 0, bytes_to_read = 0;
@@ -366,6 +416,12 @@ static void xiic_read_rx(struct xiic_i2c *i2c)
 	if (!i2c->dynamic) {
 		bytes_rem = xiic_rx_space(i2c) - bytes_in_fifo;
 
+		/* Set msg length if smbus_block_read */
+		if (i2c->rx_msg->flags & I2C_M_RECV_LEN) {
+			xiic_smbus_block_read_setup(i2c);
+			return;
+		}
+
 		if (bytes_rem > IIC_RX_FIFO_DEPTH) {
 			bytes_to_read = bytes_in_fifo;
 		} else if (bytes_rem > 1) {
@@ -638,6 +694,12 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
 		/* The bus is not busy, disable BusNotBusy interrupt */
 		xiic_irq_dis(i2c, XIIC_INTR_BNB_MASK);
 
+		if (i2c->tx_msg && i2c->smbus_block_read) {
+			i2c->smbus_block_read = false;
+			/* Set requested message len=1 to indicate STATE_DONE */
+			i2c->tx_msg->len = 1;
+		}
+
 		if (!i2c->tx_msg)
 			goto out;
 
@@ -763,8 +825,12 @@ static void xiic_start_recv(struct xiic_i2c *i2c)
 			rfd_set = IIC_RX_FIFO_DEPTH - 1;
 		} else if ((rx_watermark == 1) || (rx_watermark == 0)) {
 			rfd_set = rx_watermark - 1;
-			/* Handle single byte transfer separately */
-			cr |= XIIC_CR_NO_ACK_MASK;
+
+			/* Set No_ACK, except for smbus_block_read */
+			if (!(i2c->rx_msg->flags & I2C_M_RECV_LEN)) {
+				/* Handle single byte transfer separately */
+				cr |= XIIC_CR_NO_ACK_MASK;
+			}
 		} else {
 			rfd_set = rx_watermark - 2;
 		}
@@ -971,7 +1037,7 @@ static int xiic_start_xfer(struct xiic_i2c *i2c)
 
 static int xiic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 {
-	bool broken_read, max_read_len;
+	bool broken_read, max_read_len, smbus_blk_read;
 	struct xiic_i2c *i2c = i2c_get_adapdata(adap);
 	int err, count;
 
@@ -996,20 +1062,22 @@ static int xiic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 	i2c->prev_msg_tx = false;
 
 	/*
-	 * Scan through nmsgs, use dynamic mode when none of the below two
+	 * Scan through nmsgs, use dynamic mode when none of the below three
 	 * conditions occur. We need standard mode even if one condition holds
 	 * true in the entire array of messages in a single transfer.
 	 * If read transaction as dynamic mode is broken for delayed reads
 	 * in xlnx,axi-iic-2.0 / xlnx,xps-iic-2.00.a IP versions.
 	 * If read length is > 255 bytes.
+	 * If smbus_block_read transaction.
 	 */
 	for (count = 0; count < i2c->nmsgs; count++) {
 		broken_read = (i2c->quirks & DYNAMIC_MODE_READ_BROKEN_BIT) &&
 			       (i2c->tx_msg[count].flags & I2C_M_RD);
 		max_read_len = (i2c->tx_msg[count].flags & I2C_M_RD) &&
 				(i2c->tx_msg[count].len > MAX_READ_LENGTH_DYNAMIC);
+		smbus_blk_read = (i2c->tx_msg[count].flags & I2C_M_RECV_LEN);
 
-		if (broken_read || max_read_len) {
+		if (broken_read || max_read_len || smbus_blk_read) {
 			i2c->dynamic = false;
 			break;
 		}
@@ -1040,7 +1108,7 @@ static int xiic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 
 static u32 xiic_func(struct i2c_adapter *adap)
 {
-	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_SMBUS_BLOCK_DATA;
 }
 
 static const struct i2c_algorithm xiic_algorithm = {
-- 
2.25.1

Re: [PATCH v2 00/10] i2c: xiic: Add features, bug fixes.

[Michal Simek]

On 6/26/21 12:27 PM, Raviteja Narayanam wrote:
> -Add 'standard mode' feature for reads > 255 bytes.
> -Add 'smbus block read' functionality.
> -Add 'xlnx,axi-iic-2.1' new IP version support.
> -Switch to 'AXI I2C standard mode' for i2c reads in affected IP versions.
> -Remove 'local_irq_save/restore' calls as discussed here: https://www.spinics.net/lists/linux-i2c/msg46483.html.
> -Some trivial fixes.
> 
> Changes in v2:
> -Grouped the commits as fixes first and then features. 
> -The first 4 commits fix the dynamic mode broken feature.
> -Corrected the indentation in coding style issues.
> 
> Michal Simek (1):
>   i2c: xiic: Fix coding style issues
> 
> Raviteja Narayanam (7):
>   i2c: xiic: Fix Tx Interrupt path for grouped messages
>   i2c: xiic: Add standard mode support for > 255 byte read transfers
>   i2c: xiic: Switch to Xiic standard mode for i2c-read
>   i2c: xiic: Remove interrupt enable/disable in Rx path
>   dt-bindings: i2c: xiic: Add 'xlnx,axi-iic-2.1' to compatible
>   i2c: xiic: Update compatible with new IP version
>   i2c: xiic: Add smbus_block_read functionality
> 
> Shubhrajyoti Datta (2):
>   i2c: xiic: Return value of xiic_reinit
>   i2c: xiic: Fix the type check for xiic_wakeup
> 
>  .../bindings/i2c/xlnx,xps-iic-2.00.a.yaml     |   4 +-
>  drivers/i2c/busses/i2c-xiic.c                 | 593 ++++++++++++++----
>  2 files changed, 487 insertions(+), 110 deletions(-)
> 

Acked-by: Michal Simek <michal.simek@xilinx.com>

Thanks,
Michal

Re: [PATCH v2 00/10] i2c: xiic: Add features, bug fixes.

[Marek Vasut]

On 6/28/21 9:23 AM, Michal Simek wrote:
> 
> 
> On 6/26/21 12:27 PM, Raviteja Narayanam wrote:
>> -Add 'standard mode' feature for reads > 255 bytes.
>> -Add 'smbus block read' functionality.
>> -Add 'xlnx,axi-iic-2.1' new IP version support.
>> -Switch to 'AXI I2C standard mode' for i2c reads in affected IP versions.
>> -Remove 'local_irq_save/restore' calls as discussed here: https://www.spinics.net/lists/linux-i2c/msg46483.html.
>> -Some trivial fixes.
>>
>> Changes in v2:
>> -Grouped the commits as fixes first and then features.
>> -The first 4 commits fix the dynamic mode broken feature.
>> -Corrected the indentation in coding style issues.
>>
>> Michal Simek (1):
>>    i2c: xiic: Fix coding style issues
>>
>> Raviteja Narayanam (7):
>>    i2c: xiic: Fix Tx Interrupt path for grouped messages
>>    i2c: xiic: Add standard mode support for > 255 byte read transfers
>>    i2c: xiic: Switch to Xiic standard mode for i2c-read
>>    i2c: xiic: Remove interrupt enable/disable in Rx path
>>    dt-bindings: i2c: xiic: Add 'xlnx,axi-iic-2.1' to compatible
>>    i2c: xiic: Update compatible with new IP version
>>    i2c: xiic: Add smbus_block_read functionality
>>
>> Shubhrajyoti Datta (2):
>>    i2c: xiic: Return value of xiic_reinit
>>    i2c: xiic: Fix the type check for xiic_wakeup
>>
>>   .../bindings/i2c/xlnx,xps-iic-2.00.a.yaml     |   4 +-
>>   drivers/i2c/busses/i2c-xiic.c                 | 593 ++++++++++++++----
>>   2 files changed, 487 insertions(+), 110 deletions(-)
>>
> 
> Acked-by: Michal Simek <michal.simek@xilinx.com>

I just tested this patchset on next-20210716 and the XIIC failures are 
still present, see:

xiic-i2c a0010000.i2c: mmio a0010000 irq 36
xiic-i2c a0120000.i2c: mmio a0120000 irq 38
atmel_mxt_ts 3-004a: supply vdda not found, using dummy regulator
atmel_mxt_ts 3-004a: supply vdd not found, using dummy regulator

xiic-i2c a0120000.i2c: Timeout waiting at Tx empty

atmel_mxt_ts 3-004a: __mxt_read_reg: i2c transfer failed (-5)
atmel_mxt_ts 3-004a: mxt_bootloader_read: i2c recv failed (-5)
atmel_mxt_ts 3-004a: Trying alternate bootloader address
atmel_mxt_ts 3-004a: mxt_bootloader_read: i2c recv failed (-5)
atmel_mxt_ts: probe of 3-004a failed with error -5

RE: [PATCH v2 00/10] i2c: xiic: Add features, bug fixes.

[Raviteja Narayanam]

> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Friday, July 16, 2021 9:31 PM
> To: Michal Simek <michals@xilinx.com>; Raviteja Narayanam
> <rna@xilinx.com>; linux-i2c@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; git
> <git@xilinx.com>; joe@perches.com
> Subject: Re: [PATCH v2 00/10] i2c: xiic: Add features, bug fixes.
> 
> On 6/28/21 9:23 AM, Michal Simek wrote:
> >
> >
> > On 6/26/21 12:27 PM, Raviteja Narayanam wrote:
> >> -Add 'standard mode' feature for reads > 255 bytes.
> >> -Add 'smbus block read' functionality.
> >> -Add 'xlnx,axi-iic-2.1' new IP version support.
> >> -Switch to 'AXI I2C standard mode' for i2c reads in affected IP versions.
> >> -Remove 'local_irq_save/restore' calls as discussed here:
> https://www.spinics.net/lists/linux-i2c/msg46483.html.
> >> -Some trivial fixes.
> >>
> >> Changes in v2:
> >> -Grouped the commits as fixes first and then features.
> >> -The first 4 commits fix the dynamic mode broken feature.
> >> -Corrected the indentation in coding style issues.
> >>
> >> Michal Simek (1):
> >>    i2c: xiic: Fix coding style issues
> >>
> >> Raviteja Narayanam (7):
> >>    i2c: xiic: Fix Tx Interrupt path for grouped messages
> >>    i2c: xiic: Add standard mode support for > 255 byte read transfers
> >>    i2c: xiic: Switch to Xiic standard mode for i2c-read
> >>    i2c: xiic: Remove interrupt enable/disable in Rx path
> >>    dt-bindings: i2c: xiic: Add 'xlnx,axi-iic-2.1' to compatible
> >>    i2c: xiic: Update compatible with new IP version
> >>    i2c: xiic: Add smbus_block_read functionality
> >>
> >> Shubhrajyoti Datta (2):
> >>    i2c: xiic: Return value of xiic_reinit
> >>    i2c: xiic: Fix the type check for xiic_wakeup
> >>
> >>   .../bindings/i2c/xlnx,xps-iic-2.00.a.yaml     |   4 +-
> >>   drivers/i2c/busses/i2c-xiic.c                 | 593 ++++++++++++++----
> >>   2 files changed, 487 insertions(+), 110 deletions(-)
> >>
> >
> > Acked-by: Michal Simek <michal.simek@xilinx.com>
> 
> I just tested this patchset on next-20210716 and the XIIC failures are still
> present, see:

The probe of ' atmel_mxt_ts' failed as per the error. May I know the details of
your test case if you tweaked any i2ctransfers/added delays. 
If it failed without adding anything, then please check whether the vivado design constraints
are correctly applied or not. 
Also check if the other devices on the bus are detected and i2ctransfer command is successful on them.
It would be helpful to know if the device ' atmel_mxt_ts' is successfully probed with next-20210716
without applying this patchset. 

I have tested this again on our boards with eeprom and other sensors, this is working fine for us.

Regards,
Raviteja N

> 
> xiic-i2c a0010000.i2c: mmio a0010000 irq 36 xiic-i2c a0120000.i2c: mmio
> a0120000 irq 38 atmel_mxt_ts 3-004a: supply vdda not found, using dummy
> regulator atmel_mxt_ts 3-004a: supply vdd not found, using dummy
> regulator
> 
> xiic-i2c a0120000.i2c: Timeout waiting at Tx empty
> 
> atmel_mxt_ts 3-004a: __mxt_read_reg: i2c transfer failed (-5) atmel_mxt_ts
> 3-004a: mxt_bootloader_read: i2c recv failed (-5) atmel_mxt_ts 3-004a:
> Trying alternate bootloader address atmel_mxt_ts 3-004a:
> mxt_bootloader_read: i2c recv failed (-5)
> atmel_mxt_ts: probe of 3-004a failed with error -5

Re: [PATCH v2 00/10] i2c: xiic: Add features, bug fixes.

[Marek Vasut]

On 7/19/21 12:09 PM, Raviteja Narayanam wrote:

Hi,

[...]

>>>> -Add 'standard mode' feature for reads > 255 bytes.
>>>> -Add 'smbus block read' functionality.
>>>> -Add 'xlnx,axi-iic-2.1' new IP version support.
>>>> -Switch to 'AXI I2C standard mode' for i2c reads in affected IP versions.
>>>> -Remove 'local_irq_save/restore' calls as discussed here:
>> https://www.spinics.net/lists/linux-i2c/msg46483.html.
>>>> -Some trivial fixes.
>>>>
>>>> Changes in v2:
>>>> -Grouped the commits as fixes first and then features.
>>>> -The first 4 commits fix the dynamic mode broken feature.
>>>> -Corrected the indentation in coding style issues.
>>>>
>>>> Michal Simek (1):
>>>>     i2c: xiic: Fix coding style issues
>>>>
>>>> Raviteja Narayanam (7):
>>>>     i2c: xiic: Fix Tx Interrupt path for grouped messages
>>>>     i2c: xiic: Add standard mode support for > 255 byte read transfers
>>>>     i2c: xiic: Switch to Xiic standard mode for i2c-read
>>>>     i2c: xiic: Remove interrupt enable/disable in Rx path
>>>>     dt-bindings: i2c: xiic: Add 'xlnx,axi-iic-2.1' to compatible
>>>>     i2c: xiic: Update compatible with new IP version
>>>>     i2c: xiic: Add smbus_block_read functionality
>>>>
>>>> Shubhrajyoti Datta (2):
>>>>     i2c: xiic: Return value of xiic_reinit
>>>>     i2c: xiic: Fix the type check for xiic_wakeup
>>>>
>>>>    .../bindings/i2c/xlnx,xps-iic-2.00.a.yaml     |   4 +-
>>>>    drivers/i2c/busses/i2c-xiic.c                 | 593 ++++++++++++++----
>>>>    2 files changed, 487 insertions(+), 110 deletions(-)
>>>>
>>>
>>> Acked-by: Michal Simek <michal.simek@xilinx.com>
>>
>> I just tested this patchset on next-20210716 and the XIIC failures are still
>> present, see:
> 
> The probe of ' atmel_mxt_ts' failed as per the error. May I know the details of
> your test case if you tweaked any i2ctransfers/added delays.

It is still the same test case from a year ago -- Atmel MXT touchscreen 
controller connected to XIIC I2C IP in ZynqMP FPGA, both drivers are 
compiled into the kernel. Also, it is not the "new" XIIC IP revision, 
but older one from Vivado 2019 or so.

> If it failed without adding anything, then please check whether the vivado design constraints
> are correctly applied or not.

They are, we already checked multiple times and the FPGA part is OK.

> Also check if the other devices on the bus are detected and i2ctransfer command is successful on them.

Note that this problem is very likely a race condition in the XIIC 
driver, so a trivial test like i2ctransfer on idle system from userspace 
is unlikely to trigger it. When the system is under heavy load e.g. 
during the kernel boot, that is when these corner cases start showing up.

> It would be helpful to know if the device ' atmel_mxt_ts' is successfully probed with next-20210716
> without applying this patchset.

Sometimes, the XIIC driver in current mainline Linux suffers from race 
conditions on SMP, so it depends.

The MXT driver also has to be patched to avoid longer than 255 byte 
transfers, because that is currently broken with XIIC.

> I have tested this again on our boards with eeprom and other sensors, this is working fine for us.

Can you share details of how those tests were performed ?

RE: [PATCH v2 00/10] i2c: xiic: Add features, bug fixes.

[Raviteja Narayanam]

> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Monday, July 19, 2021 11:30 PM
> To: Raviteja Narayanam <rna@xilinx.com>; Michal Simek
> <michals@xilinx.com>; linux-i2c@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; git
> <git@xilinx.com>; joe@perches.com
> Subject: Re: [PATCH v2 00/10] i2c: xiic: Add features, bug fixes.
> 
> On 7/19/21 12:09 PM, Raviteja Narayanam wrote:
> 
> Hi,
> 
> [...]
> 
> >>>> -Add 'standard mode' feature for reads > 255 bytes.
> >>>> -Add 'smbus block read' functionality.
> >>>> -Add 'xlnx,axi-iic-2.1' new IP version support.
> >>>> -Switch to 'AXI I2C standard mode' for i2c reads in affected IP versions.
> >>>> -Remove 'local_irq_save/restore' calls as discussed here:
> >> https://www.spinics.net/lists/linux-i2c/msg46483.html.
> >>>> -Some trivial fixes.
> >>>>
> >>>> Changes in v2:
> >>>> -Grouped the commits as fixes first and then features.
> >>>> -The first 4 commits fix the dynamic mode broken feature.
> >>>> -Corrected the indentation in coding style issues.
> >>>>
> >>>> Michal Simek (1):
> >>>>     i2c: xiic: Fix coding style issues
> >>>>
> >>>> Raviteja Narayanam (7):
> >>>>     i2c: xiic: Fix Tx Interrupt path for grouped messages
> >>>>     i2c: xiic: Add standard mode support for > 255 byte read transfers
> >>>>     i2c: xiic: Switch to Xiic standard mode for i2c-read
> >>>>     i2c: xiic: Remove interrupt enable/disable in Rx path
> >>>>     dt-bindings: i2c: xiic: Add 'xlnx,axi-iic-2.1' to compatible
> >>>>     i2c: xiic: Update compatible with new IP version
> >>>>     i2c: xiic: Add smbus_block_read functionality
> >>>>
> >>>> Shubhrajyoti Datta (2):
> >>>>     i2c: xiic: Return value of xiic_reinit
> >>>>     i2c: xiic: Fix the type check for xiic_wakeup
> >>>>
> >>>>    .../bindings/i2c/xlnx,xps-iic-2.00.a.yaml     |   4 +-
> >>>>    drivers/i2c/busses/i2c-xiic.c                 | 593 ++++++++++++++----
> >>>>    2 files changed, 487 insertions(+), 110 deletions(-)
> >>>>
> >>>
> >>> Acked-by: Michal Simek <michal.simek@xilinx.com>
> >>
> >> I just tested this patchset on next-20210716 and the XIIC failures
> >> are still present, see:
> >
> > The probe of ' atmel_mxt_ts' failed as per the error. May I know the
> > details of your test case if you tweaked any i2ctransfers/added delays.
> 
> It is still the same test case from a year ago -- Atmel MXT touchscreen
> controller connected to XIIC I2C IP in ZynqMP FPGA, both drivers are
> compiled into the kernel. Also, it is not the "new" XIIC IP revision, but older
> one from Vivado 2019 or so.
> 
> > If it failed without adding anything, then please check whether the
> > vivado design constraints are correctly applied or not.
> 
> They are, we already checked multiple times and the FPGA part is OK.
> 
> > Also check if the other devices on the bus are detected and i2ctransfer
> command is successful on them.
> 
> Note that this problem is very likely a race condition in the XIIC driver, so a
> trivial test like i2ctransfer on idle system from userspace is unlikely to trigger
> it. When the system is under heavy load e.g.
> during the kernel boot, that is when these corner cases start showing up.

Thanks for all the details, Marek. 

> 
> > It would be helpful to know if the device ' atmel_mxt_ts' is
> > successfully probed with next-20210716 without applying this patchset.
> 
> Sometimes, the XIIC driver in current mainline Linux suffers from race
> conditions on SMP, so it depends.
> 
> The MXT driver also has to be patched to avoid longer than 255 byte
> transfers, because that is currently broken with XIIC.
> 
> > I have tested this again on our boards with eeprom and other sensors, this
> is working fine for us.
> 
> Can you share details of how those tests were performed ?

Stress test - 1:
Heavy ethernet traffic running in the background. 
I2c commands script (like below) running. We can see visible stutter in the output as expected, but nothing failed.

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

Stress test - 2:
Two i2c scripts running in parallel with commands as shown above with different bus numbers (as a result of mux), but going into same XIIC adapter.
This is also working fine.

Stress test - 3:
Two i2c scripts running in parallel with same commands in separate terminals. This is also working fine.

From your log, the race condition is occurring at boot time during i2c clients registration. I am starting a similar test at my setup
to reproduce this issue at boot time.

Regards,
Raviteja N

Re: [PATCH v2 00/10] i2c: xiic: Add features, bug fixes.

[Marek Vasut]

On 7/20/21 4:19 PM, Raviteja Narayanam wrote:

Hi,

[...]

>>> I have tested this again on our boards with eeprom and other sensors, this
>> is working fine for us.
>>
>> Can you share details of how those tests were performed ?
> 
> Stress test - 1:
> Heavy ethernet traffic running in the background.
> I2c commands script (like below) running. We can see visible stutter in the output as expected, but nothing failed.
> 
> i=0
> while [ 1 ]
> do
> 		i2ctransfer -y -f 2 w1@0X54 0X00 r31@0X54
> 		i2ctransfer -y -f 2 w1@0X54 0X00 r32@0X54
> 		i2ctransfer -y -f 2 w1@0X54 0X00 r255@0X54
> 		i2ctransfer -y -f 2 w1@0X54 0X00 r273@0X54
>                               i2ctransfer -y -f 2 w1@0X54 0X00 r1@0X54

Could it be that you never see the problem because you always talk to 
one single device ?

Do you also test writes which are not 1 byte long ?

>          i=$(expr $i + 1)
>          echo "$i"
> done
> 
> Stress test - 2:
> Two i2c scripts running in parallel with commands as shown above with different bus numbers (as a result of mux), but going into same XIIC adapter.
> This is also working fine.

Could it be the i2c-dev serializes each of those transfers , so no race 
can be triggered ?

> Stress test - 3:
> Two i2c scripts running in parallel with same commands in separate terminals. This is also working fine.
> 
>  From your log, the race condition is occurring at boot time during i2c clients registration. I am starting a similar test at my setup
> to reproduce this issue at boot time.

Thank you

RE: [PATCH v2 00/10] i2c: xiic: Add features, bug fixes.

[Raviteja Narayanam]

> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Wednesday, July 21, 2021 3:14 AM
> To: Raviteja Narayanam <rna@xilinx.com>; Michal Simek
> <michals@xilinx.com>; linux-i2c@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; git
> <git@xilinx.com>; joe@perches.com
> Subject: Re: [PATCH v2 00/10] i2c: xiic: Add features, bug fixes.
> 
> On 7/20/21 4:19 PM, Raviteja Narayanam wrote:
> 
> Hi,
> 
> [...]
> 
> >>> I have tested this again on our boards with eeprom and other
> >>> sensors, this
> >> is working fine for us.
> >>
> >> Can you share details of how those tests were performed ?
> >
> > Stress test - 1:
> > Heavy ethernet traffic running in the background.
> > I2c commands script (like below) running. We can see visible stutter in the
> output as expected, but nothing failed.
> >
> > i=0
> > while [ 1 ]
> > do
> > 		i2ctransfer -y -f 2 w1@0X54 0X00 r31@0X54
> > 		i2ctransfer -y -f 2 w1@0X54 0X00 r32@0X54
> > 		i2ctransfer -y -f 2 w1@0X54 0X00 r255@0X54
> > 		i2ctransfer -y -f 2 w1@0X54 0X00 r273@0X54
> >                               i2ctransfer -y -f 2 w1@0X54 0X00 r1@0X54
> 
> Could it be that you never see the problem because you always talk to one
> single device ?

There are transfers to other devices as well. 
Our board has multiple power monitors, eeprom and other misc devices that
are accessed through the same driver and are working fine.

> 
> Do you also test writes which are not 1 byte long ?
>

Yes, like for eeprom 1 page (16 bytes)  is written.
 
> >          i=$(expr $i + 1)
> >          echo "$i"
> > done
> >
> > Stress test - 2:
> > Two i2c scripts running in parallel with commands as shown above with
> different bus numbers (as a result of mux), but going into same XIIC adapter.
> > This is also working fine.
> 
> Could it be the i2c-dev serializes each of those transfers , so no race can be
> triggered ?
> 

Yes, that is true because all our tests are going through the i2c-core only
and there is a lock at adapter level in the core.
It has to be reproducible through the i2c standard interface, which is not
happening at our setup.

I can take your patches that are targeted for this issue, rebase, test
and send them.

Regards,
Raviteja N

Re: [PATCH v2 00/10] i2c: xiic: Add features, bug fixes.

[Marek Vasut]

On 7/26/21 7:26 AM, Raviteja Narayanam wrote:

Hi,

[...]

>>>>> I have tested this again on our boards with eeprom and other
>>>>> sensors, this
>>>> is working fine for us.
>>>>
>>>> Can you share details of how those tests were performed ?
>>>
>>> Stress test - 1:
>>> Heavy ethernet traffic running in the background.
>>> I2c commands script (like below) running. We can see visible stutter in the
>> output as expected, but nothing failed.
>>>
>>> i=0
>>> while [ 1 ]
>>> do
>>> 		i2ctransfer -y -f 2 w1@0X54 0X00 r31@0X54
>>> 		i2ctransfer -y -f 2 w1@0X54 0X00 r32@0X54
>>> 		i2ctransfer -y -f 2 w1@0X54 0X00 r255@0X54
>>> 		i2ctransfer -y -f 2 w1@0X54 0X00 r273@0X54
>>>                                i2ctransfer -y -f 2 w1@0X54 0X00 r1@0X54
>>
>> Could it be that you never see the problem because you always talk to one
>> single device ?
> 
> There are transfers to other devices as well.

The above test only accesses device at address 0x54, right ?

> Our board has multiple power monitors, eeprom and other misc devices that
> are accessed through the same driver and are working fine.

That does not seem to be what the test above does .

>> Do you also test writes which are not 1 byte long ?
>>
> 
> Yes, like for eeprom 1 page (16 bytes)  is written.

I suspect the atmel mxt does much longer writes, try 255 bytes or so.

>>>           i=$(expr $i + 1)
>>>           echo "$i"
>>> done
>>>
>>> Stress test - 2:
>>> Two i2c scripts running in parallel with commands as shown above with
>> different bus numbers (as a result of mux), but going into same XIIC adapter.
>>> This is also working fine.
>>
>> Could it be the i2c-dev serializes each of those transfers , so no race can be
>> triggered ?
>>
> 
> Yes, that is true because all our tests are going through the i2c-core only
> and there is a lock at adapter level in the core.
> It has to be reproducible through the i2c standard interface, which is not
> happening at our setup.
> 
> I can take your patches that are targeted for this issue, rebase, test
> and send them.

I think you and Michal talked about getting the atmel mxt touchscreen, 
so you can test that yourself as well.

RE: [PATCH v2 00/10] i2c: xiic: Add features, bug fixes.

[Raviteja Narayanam]

> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Monday, July 26, 2021 6:43 PM
> To: Raviteja Narayanam <rna@xilinx.com>; Michal Simek
> <michals@xilinx.com>; linux-i2c@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; git
> <git@xilinx.com>; joe@perches.com
> Subject: Re: [PATCH v2 00/10] i2c: xiic: Add features, bug fixes.
> 
> On 7/26/21 7:26 AM, Raviteja Narayanam wrote:
> 
> Hi,
> 
> [...]
> 
> >>>>> I have tested this again on our boards with eeprom and other
> >>>>> sensors, this
> >>>> is working fine for us.
> >>>>
> >>>> Can you share details of how those tests were performed ?
> >>>
> >>> Stress test - 1:
> >>> Heavy ethernet traffic running in the background.
> >>> I2c commands script (like below) running. We can see visible stutter
> >>> in the
> >> output as expected, but nothing failed.
> >>>
> >>> i=0
> >>> while [ 1 ]
> >>> do
> >>> 		i2ctransfer -y -f 2 w1@0X54 0X00 r31@0X54
> >>> 		i2ctransfer -y -f 2 w1@0X54 0X00 r32@0X54
> >>> 		i2ctransfer -y -f 2 w1@0X54 0X00 r255@0X54
> >>> 		i2ctransfer -y -f 2 w1@0X54 0X00 r273@0X54
> >>>                                i2ctransfer -y -f 2 w1@0X54 0X00
> >>> r1@0X54
> >>
> >> Could it be that you never see the problem because you always talk to
> >> one single device ?
> >
> > There are transfers to other devices as well.
> 
> The above test only accesses device at address 0x54, right ?

Above code is just one part.
We are doing read/writes to all devices present on this board https://www.xilinx.com/support/documentation/boards_and_kits/zcu102/ug1182-zcu102-eval-bd.pdf 

> 
> > Our board has multiple power monitors, eeprom and other misc devices
> > that are accessed through the same driver and are working fine.
> 
> That does not seem to be what the test above does .
> 
> >> Do you also test writes which are not 1 byte long ?
> >>
> >
> > Yes, like for eeprom 1 page (16 bytes)  is written.
> 
> I suspect the atmel mxt does much longer writes, try 255 bytes or so.

Ok, I will do longer writes (in the range of 255) on supported slave devices.

> 
> >>>           i=$(expr $i + 1)
> >>>           echo "$i"
> >>> done
> >>>
> >>> Stress test - 2:
> >>> Two i2c scripts running in parallel with commands as shown above
> >>> with
> >> different bus numbers (as a result of mux), but going into same XIIC
> adapter.
> >>> This is also working fine.
> >>
> >> Could it be the i2c-dev serializes each of those transfers , so no
> >> race can be triggered ?
> >>
> >
> > Yes, that is true because all our tests are going through the i2c-core
> > only and there is a lock at adapter level in the core.
> > It has to be reproducible through the i2c standard interface, which is
> > not happening at our setup.
> >
> > I can take your patches that are targeted for this issue, rebase, test
> > and send them.
> 
> I think you and Michal talked about getting the atmel mxt touchscreen, so
> you can test that yourself as well.

Yes, that is the plan, we are trying to get the part. Only problem is it is subject to
availability and may take more time to get it delivered to our place.

Regards,
Raviteja N

Re: [PATCH v2 00/10] i2c: xiic: Add features, bug fixes.

[Marek Vasut]

On 7/28/21 12:11 PM, Raviteja Narayanam wrote:
[...]

>>>>>>> I have tested this again on our boards with eeprom and other
>>>>>>> sensors, this
>>>>>> is working fine for us.
>>>>>>
>>>>>> Can you share details of how those tests were performed ?
>>>>>
>>>>> Stress test - 1:
>>>>> Heavy ethernet traffic running in the background.
>>>>> I2c commands script (like below) running. We can see visible stutter
>>>>> in the
>>>> output as expected, but nothing failed.
>>>>>
>>>>> i=0
>>>>> while [ 1 ]
>>>>> do
>>>>> 		i2ctransfer -y -f 2 w1@0X54 0X00 r31@0X54
>>>>> 		i2ctransfer -y -f 2 w1@0X54 0X00 r32@0X54
>>>>> 		i2ctransfer -y -f 2 w1@0X54 0X00 r255@0X54
>>>>> 		i2ctransfer -y -f 2 w1@0X54 0X00 r273@0X54
>>>>>                                 i2ctransfer -y -f 2 w1@0X54 0X00
>>>>> r1@0X54
>>>>
>>>> Could it be that you never see the problem because you always talk to
>>>> one single device ?
>>>
>>> There are transfers to other devices as well.
>>
>> The above test only accesses device at address 0x54, right ?
> 
> Above code is just one part.
> We are doing read/writes to all devices present on this board https://www.xilinx.com/support/documentation/boards_and_kits/zcu102/ug1182-zcu102-eval-bd.pdf

Can you share details of how those tests were performed ?

>>> Our board has multiple power monitors, eeprom and other misc devices
>>> that are accessed through the same driver and are working fine.
>>
>> That does not seem to be what the test above does .
>>
>>>> Do you also test writes which are not 1 byte long ?
>>>>
>>>
>>> Yes, like for eeprom 1 page (16 bytes)  is written.
>>
>> I suspect the atmel mxt does much longer writes, try 255 bytes or so.
> 
> Ok, I will do longer writes (in the range of 255) on supported slave devices.

Thank you

RE: [PATCH v2 00/10] i2c: xiic: Add features, bug fixes.

[Guntupalli, Manikanta]

Hi Marek,

-----Original Message-----
From: Marek Vasut <marex@denx.de> 
Sent: Thursday, July 29, 2021 12:17 AM
To: Raviteja Narayanam <rna@xilinx.com>; Simek, Michal <michal.simek@amd.com>; linux-i2c@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; git <git@xilinx.com>; joe@perches.com
Subject: Re: [PATCH v2 00/10] i2c: xiic: Add features, bug fixes.

On 7/28/21 12:11 PM, Raviteja Narayanam wrote:
[...]

>>
>> I suspect the atmel mxt does much longer writes, try 255 bytes or so.
> 

I was able to probe the atmel successfully after applying the below series.
https://lore.kernel.org/linux-arm-kernel/1656072327-13628-4-git-send-email-manikanta.guntupalli@xilinx.com/T/

Could you please confirm, if it works for you.


Thanks,
Manikanta.