[PATCH 00/12] i2c: xiic: Added Standard mode and SMBus

[PATCH 00/12] i2c: xiic: Added Standard mode and SMBus

[Manikanta Guntupalli]

-Added standard mode for AXI I2C controller.
-Added Smbus block read support to xiic driver.
-Added 'xlnx,axi-iic-2.1' new IP version support.
-Added dynamic SCL frequency configuration support.
-Removed 'local_irq_save/restore' calls as discussed
 here: https://www.spinics.net/lists/linux-i2c/msg46483.html.
-Corrected the BNB interrupt enable sequence.

Raviteja Narayanam (11):
  i2c: xiic: Add standard mode support for > 255 byte read transfers
  i2c: xiic: Enter standard mode only for > 255 byte read transfers
  i2c: xiic: Fix Rx and Tx paths in standard mode repeated start
  i2c: xiic: Add wait for FIFO empty in send_tx
  i2c: xiic: Use 'nmsgs' variable instead of repeated_start
  i2c: xiic: Add smbus_block_read functionality
  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 SCL frequency configuration support

Srinivas Goud (1):
  i2c: xiic: Correct the BNB interrupt enable sequence

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

-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 01/12] i2c: xiic: Add standard mode support for > 255 byte read transfers

[Manikanta Guntupalli]

From: Raviteja Narayanam <raviteja.narayanam@xilinx.com>

Added standard mode for AXI I2C controller to enable read transfers
of size more than 255 bytes. The driver selects standard mode in the
following scenarios.

1. If a single message request comes from user space, requesting a
read of more than 255 bytes

2. If a message set request comes from user space consisting of many
messages and if any one of them is a read operation, irrespective
of the size of transfer. (This is done because it is observed that
repeated start operation is not happening in dynamic mode read as
expected in a message set request from user space.)

Signed-off-by: Raviteja Narayanam <raviteja.narayanam@xilinx.com>
Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@xilinx.com>
---
 drivers/i2c/busses/i2c-xiic.c | 326 +++++++++++++++++++++++++++++-----
 1 file changed, 280 insertions(+), 46 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 9a1c3f8b7048..fb2443623844 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
+ * @repeated_start: Repeated start operation
  */
 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 repeated_start;
 };
 
 #define XIIC_MSB_OFFSET 0
@@ -143,6 +147,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
@@ -316,13 +323,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));
@@ -330,13 +338,52 @@ 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);
+	}
+
+	if (i2c->dynamic) {
+		u8 bytes;
 
-	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);
+		/* 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)
@@ -367,6 +414,56 @@ 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->repeated_start))
+		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;
+
+			/* 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;
@@ -401,7 +498,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) ||
@@ -480,7 +579,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) {
@@ -579,31 +681,99 @@ static int xiic_busy(struct xiic_i2c *i2c)
 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;
 
-	/* 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));
 
-	/* 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, (u8)(rx_watermark - 1));
+	/* Disable Tx interrupts */
+	xiic_irq_dis(i2c, XIIC_INTR_TX_HALF_MASK | XIIC_INTR_TX_EMPTY_MASK);
 
-	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 (i2c->dynamic) {
+		u8 bytes;
+		u16 val;
 
-	xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET,
-		msg->len | ((i2c->nmsgs == 1) ? XIIC_TX_DYN_STOP_MASK : 0));
+		/* 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 {
+		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) {
+			i2c->repeated_start = true;
+			/* 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));
+
+		/* Write to Control Register,to start transaction in Rx mode */
+		if ((cr & XIIC_CR_MSMS_MASK) == 0) {
+			i2c->repeated_start = false;
+			xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, (cr |
+					XIIC_CR_MSMS_MASK)
+					& ~(XIIC_CR_DIR_IS_TX_MASK));
+		}
+		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 */
@@ -611,10 +781,15 @@ 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);
 }
 
 static void xiic_start_send(struct xiic_i2c *i2c)
 {
+	u8 cr = 0;
+	u16 data;
 	struct i2c_msg *msg = i2c->tx_msg;
 
 	dev_dbg(i2c->adap.dev.parent, "%s entry, msg: %p, len: %d",
@@ -623,24 +798,61 @@ 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);
+		}
+
+		/* 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->nmsgs > 1 || xiic_tx_space(i2c)) ?
+				XIIC_INTR_TX_HALF_MASK : 0));
+
+		xiic_fill_tx_fifo(i2c);
+	} else {
+		/* Check if RSTA should be set */
+		cr = xiic_getreg8(i2c, XIIC_CR_REG_OFFSET);
+		if (cr & XIIC_CR_MSMS_MASK) {
+			i2c->repeated_start = true;
+			/* 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);
-	}
 
-	/* 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->nmsgs > 1 || xiic_tx_space(i2c)) ?
-			XIIC_INTR_TX_HALF_MASK : 0));
+		/* Fill fifo */
+		xiic_std_fill_tx_fifo(i2c);
+
+		if ((cr & XIIC_CR_MSMS_MASK) == 0) {
+			i2c->repeated_start = false;
+
+			/* 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);
+		}
 
-	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);
+	}
 }
 
 static void __xiic_start_xfer(struct xiic_i2c *i2c)
@@ -701,6 +913,33 @@ static int xiic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 	if (err < 0)
 		return err;
 
+	/* Decide standard mode or Dynamic mode */
+	i2c->dynamic = true;
+
+	/*
+	 * If number of messages is 1 and read length is > 255 bytes,
+	 * enter standard mode
+	 */
+
+	if (i2c->nmsgs == 1 && (i2c->tx_msg->flags & I2C_M_RD) &&
+	    i2c->tx_msg->len > MAX_READ_LENGTH_DYNAMIC) {
+		i2c->dynamic = false;
+	} else if (i2c->nmsgs > 1) {
+		int count;
+
+		/*
+		 * If number of messages is more than 1 and one of them is
+		 * a read message, enter standard mode. Since repeated start
+		 * operation in dynamic mode read is not happenning
+		 */
+		for (count = 0; count < i2c->nmsgs; count++) {
+			if (i2c->tx_msg[count].flags & I2C_M_RD) {
+				i2c->dynamic = false;
+				break;
+			}
+		}
+	}
+
 	err = xiic_start_xfer(i2c, msgs, num);
 	if (err < 0) {
 		dev_err(adap->dev.parent, "Error xiic_start_xfer\n");
@@ -737,15 +976,10 @@ 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,
 	.class = I2C_CLASS_DEPRECATED,
 	.algo = &xiic_algorithm,
-	.quirks = &xiic_quirks,
 };
 
 static int xiic_i2c_probe(struct platform_device *pdev)
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 01/12] i2c: xiic: Add standard mode support for > 255 byte read transfers

[Krzysztof Adamski]

W dniu 24.06.2022 o 14:05, Manikanta Guntupalli pisze:
[...]
> +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->repeated_start))
> +		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.
[...]
> @@ -579,31 +681,99 @@ static int xiic_busy(struct xiic_i2c *i2c)
>   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;
>   
> -	/* 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));
>   
> -	/* 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, (u8)(rx_watermark - 1));

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.

> +	/* Disable Tx interrupts */
> +	xiic_irq_dis(i2c, XIIC_INTR_TX_HALF_MASK | XIIC_INTR_TX_EMPTY_MASK);
>   
> -	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 (i2c->dynamic) {
> +		u8 bytes;
> +		u16 val;
>   
> -	xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET,
> -		msg->len | ((i2c->nmsgs == 1) ? XIIC_TX_DYN_STOP_MASK : 0));
> +		/* 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--;
Again, do we really want to write 255 to RFD if msg->len == 0?

> +
> +		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 {
> +		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?
[...]

Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 01/12] i2c: xiic: Add standard mode support for > 255 byte read transfers

[Guntupalli, Manikanta]

Hi Krzysztof,

> -----Original Message-----
> From: Krzysztof Adamski <krzysztof.adamski@nokia.com>
> Sent: Wednesday, June 29, 2022 5:49 PM
> To: Manikanta Guntupalli <manikanta.guntupalli@xilinx.com>;
> michal.simek@xilinx.com; Simek, Michal <michal.simek@amd.com>; linux-
> arm-kernel@lists.infradead.org; linux-i2c@vger.kernel.org; linux-
> kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.com>
> Cc: Raviteja Narayanam <raviteja.narayanam@xilinx.com>
> Subject: Re: [PATCH 01/12] i2c: xiic: Add standard mode support for > 255
> byte read transfers
> 
> 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.
> 
> 
> W dniu 24.06.2022 o 14:05, Manikanta Guntupalli pisze:
> [...]
> > +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->repeated_start))
> > +             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.

Agreed with your comments, we will fix in V2.

> [...]
> > @@ -579,31 +681,99 @@ static int xiic_busy(struct xiic_i2c *i2c)
> >   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;
> >
> > -     /* 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));
> >
> > -     /* 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, (u8)(rx_watermark - 1));
> 
> 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.
> 

Here comparing and taking minimum value of msg->len and  IIC_RX_FIFO_DEPTH. The value of IIC_RX_FIFO_DEPTH is 16, while writing into register performed -1, so maximum value writing into RX_FIFO_PIRQ is 15.

> > +     /* Disable Tx interrupts */
> > +     xiic_irq_dis(i2c, XIIC_INTR_TX_HALF_MASK |
> > + XIIC_INTR_TX_EMPTY_MASK);
> >
> > -     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 (i2c->dynamic) {
> > +             u8 bytes;
> > +             u16 val;
> >
> > -     xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET,
> > -             msg->len | ((i2c->nmsgs == 1) ? XIIC_TX_DYN_STOP_MASK : 0));
> > +             /* 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--;
> Again, do we really want to write 255 to RFD if msg->len == 0?
> 

Here comparing and taking minimum value of msg->len and  IIC_RX_FIFO_DEPTH. The value of IIC_RX_FIFO_DEPTH is 16, before writing into register performed decrement, so maximum value writing into RX_FIFO_PIRQ is 15.

> > +
> > +             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.

For receive operation with byte count greater than 255, we will switch to standard mode else always in the dynamic mode only. 

> 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?

After deciding dynamic mode based on above logic, we need to  set the DYN_START bit along with address in the TX FIFO to start the transfer. 

> 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? 
>

I2C controller supports 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?
> 

Below notes mentioned in I2C_M_NOSTART section of kernel documentation(link mentioned below),
" If you set the I2C_M_NOSTART variable for the first partial message,
   we do not generate Addr, but we do generate the startbit S. This will
   probably confuse all other clients on your bus, so don't try this."
 
https://www.kernel.org/doc/Documentation/i2c/i2c-protocol

So I2C_M_NOSTART  flag need to use in second i2c_msg or in later i2c_msgs, but not in first i2c_msg. During first i2c_msg dynamic mode starts, so again no need to set start bit TX_FIFO.

> 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?

Since it supports I2C_M_NOSTART flag, need to keep code handling 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 {
> > +             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?

Here comparing and taking minimum value of msg->len and  IIC_RX_FIFO_DEPTH. The value of IIC_RX_FIFO_DEPTH is 16, before writing into register performed -1, so maximum value writing into RX_FIFO_PIRQ is 15.

> [...]
> 
> Krzysztof

Thanks,
Manikanta.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 01/12] i2c: xiic: Add standard mode support for > 255 byte read transfers

[Krzysztof Adamski]

Hi Manikanta,

W dniu 13.07.2022 o 09:54, Guntupalli, Manikanta pisze:
> Hi Krzysztof,
>> [...]
>>> @@ -579,31 +681,99 @@ static int xiic_busy(struct xiic_i2c *i2c)
>>>    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;
>>>
>>> -     /* 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));
>>>
>>> -     /* 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, (u8)(rx_watermark - 1));
>> 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.
>>
> Here comparing and taking minimum value of msg->len and  IIC_RX_FIFO_DEPTH. The value of IIC_RX_FIFO_DEPTH is 16, while writing into register performed -1, so maximum value writing into RX_FIFO_PIRQ is 15.
>
I'm not sure you got my point - my point was that if the provided 
msg->len is 0, then rx_watermark will also be 0 and thus we will set the 
XIIC_RFD_REG_OFFSET to value 255 which seems illegal.
>>> +     /* Disable Tx interrupts */
>>> +     xiic_irq_dis(i2c, XIIC_INTR_TX_HALF_MASK |
>>> + XIIC_INTR_TX_EMPTY_MASK);
>>>
>>> -     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 (i2c->dynamic) {
>>> +             u8 bytes;
>>> +             u16 val;
>>>
>>> -     xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET,
>>> -             msg->len | ((i2c->nmsgs == 1) ? XIIC_TX_DYN_STOP_MASK : 0));
>>> +             /* 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--;
>> Again, do we really want to write 255 to RFD if msg->len == 0?
>>
> Here comparing and taking minimum value of msg->len and  IIC_RX_FIFO_DEPTH. The value of IIC_RX_FIFO_DEPTH is 16, before writing into register performed decrement, so maximum value writing into RX_FIFO_PIRQ is 15.
You are not correct - in case the msg->len is 0, the value written into 
the register will be 255 (if you decrement 0, you will get 255 in case 
of u8). Unless there is some check making sure msg->len is never 0, that 
I do not see.
>
>>> +
>>> +             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.
> For receive operation with byte count greater than 255, we will switch to standard mode else always in the dynamic mode only.
>
>> 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?
> After deciding dynamic mode based on above logic, we need to  set the DYN_START bit along with address in the TX FIFO to start the transfer.
>
>> 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?
>>
> I2C controller supports 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?
>>
> Below notes mentioned in I2C_M_NOSTART section of kernel documentation(link mentioned below),
> " If you set the I2C_M_NOSTART variable for the first partial message,
>     we do not generate Addr, but we do generate the startbit S. This will
>     probably confuse all other clients on your bus, so don't try this."
>   
> https://www.kernel.org/doc/Documentation/i2c/i2c-protocol
>
> So I2C_M_NOSTART  flag need to use in second i2c_msg or in later i2c_msgs, but not in first i2c_msg. During first i2c_msg dynamic mode starts, so again no need to set start bit TX_FIFO.

But what happens if somebody passes the I2C_M_NOSTART bit in the first 
message? There seems to be no check for that. I know the documentation 
says that this should be avioded but it also states what should happen 
(S bit is generated by no address is sent) and as far as I understand 
this is not what will happen in i2c-xiic. What will happen is that the 
address *will* be generated and it will be the first byte from the buf.

>
>> 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?
> Since it supports I2C_M_NOSTART flag, need to keep code handling flag.
But if it supports it, that should be annouced in xiic_func.
>
>>> +
>>> +             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 {
>>> +             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?
> Here comparing and taking minimum value of msg->len and  IIC_RX_FIFO_DEPTH. The value of IIC_RX_FIFO_DEPTH is 16, before writing into register performed -1, so maximum value writing into RX_FIFO_PIRQ is 15.
>
Again, only if msg->len is not 0, which is not ensured.

Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 01/12] i2c: xiic: Add standard mode support for > 255 byte read transfers

[Guntupalli, Manikanta]

Hi Krzysztof,

> -----Original Message-----
> From: Krzysztof Adamski <krzysztof.adamski@nokia.com>
> Sent: Wednesday, July 20, 2022 3:56 PM
> To: Guntupalli, Manikanta <manikanta.guntupalli@amd.com>; Manikanta
> Guntupalli <manikanta.guntupalli@xilinx.com>; michal.simek@xilinx.com;
> Simek, Michal <michal.simek@amd.com>; linux-arm-
> kernel@lists.infradead.org; linux-i2c@vger.kernel.org; linux-
> kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.com>
> Cc: Raviteja Narayanam <raviteja.narayanam@xilinx.com>
> Subject: Re: [PATCH 01/12] i2c: xiic: Add standard mode support for > 255
> byte read transfers
> 
> Hi Manikanta,
> 
> W dniu 13.07.2022 o 09:54, Guntupalli, Manikanta pisze:
> > Hi Krzysztof,
> >> [...]
> >>> @@ -579,31 +681,99 @@ static int xiic_busy(struct xiic_i2c *i2c)
> >>>    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;
> >>>
> >>> -     /* 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));
> >>>
> >>> -     /* 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, (u8)(rx_watermark - 1));
> >> 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.
> >>
> > Here comparing and taking minimum value of msg->len and
> IIC_RX_FIFO_DEPTH. The value of IIC_RX_FIFO_DEPTH is 16, while writing
> into register performed -1, so maximum value writing into RX_FIFO_PIRQ is
> 15.
> >
> I'm not sure you got my point - my point was that if the provided
> msg->len is 0, then rx_watermark will also be 0 and thus we will set the
> XIIC_RFD_REG_OFFSET to value 255 which seems illegal.

We will fix in V2.

> >>> +     /* Disable Tx interrupts */
> >>> +     xiic_irq_dis(i2c, XIIC_INTR_TX_HALF_MASK |
> >>> + XIIC_INTR_TX_EMPTY_MASK);
> >>>
> >>> -     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 (i2c->dynamic) {
> >>> +             u8 bytes;
> >>> +             u16 val;
> >>>
> >>> -     xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET,
> >>> -             msg->len | ((i2c->nmsgs == 1) ? XIIC_TX_DYN_STOP_MASK : 0));
> >>> +             /* 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--;
> >> Again, do we really want to write 255 to RFD if msg->len == 0?
> >>
> > Here comparing and taking minimum value of msg->len and
> IIC_RX_FIFO_DEPTH. The value of IIC_RX_FIFO_DEPTH is 16, before writing
> into register performed decrement, so maximum value writing into
> RX_FIFO_PIRQ is 15.
> You are not correct - in case the msg->len is 0, the value written into the
> register will be 255 (if you decrement 0, you will get 255 in case of u8). Unless
> there is some check making sure msg->len is never 0, that I do not see.

Agreed, we will fix in V2.

> >
> >>> +
> >>> +             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.
> > For receive operation with byte count greater than 255, we will switch to
> standard mode else always in the dynamic mode only.
> >
> >> 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?
> > After deciding dynamic mode based on above logic, we need to  set the
> DYN_START bit along with address in the TX FIFO to start the transfer.
> >
> >> 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?
> >>
> > I2C controller supports 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?
> >>
> > Below notes mentioned in I2C_M_NOSTART section of kernel
> > documentation(link mentioned below), " If you set the I2C_M_NOSTART
> variable for the first partial message,
> >     we do not generate Addr, but we do generate the startbit S. This will
> >     probably confuse all other clients on your bus, so don't try this."
> >
> > https://www.kernel.org/doc/Documentation/i2c/i2c-protocol
> >
> > So I2C_M_NOSTART  flag need to use in second i2c_msg or in later
> i2c_msgs, but not in first i2c_msg. During first i2c_msg dynamic mode starts,
> so again no need to set start bit TX_FIFO.
> 
> But what happens if somebody passes the I2C_M_NOSTART bit in the first
> message? There seems to be no check for that. I know the documentation
> says that this should be avioded but it also states what should happen (S bit
> is generated by no address is sent) and as far as I understand this is not what
> will happen in i2c-xiic. What will happen is that the address *will* be
> generated and it will be the first byte from the buf.
> 

As per kernel documentation it's recommended to use I2C_M_NOSTART flag from second message onwards, but not mandatory. 
Dynamic mode won't start if someone passes I2C_M_NOSTART flag in first message.

> >
> >> 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?
> > Since it supports I2C_M_NOSTART flag, need to keep code handling flag.
> But if it supports it, that should be annouced in xiic_func.

Since we are not announcing I2C_FUNC_NOSTART, we will remove  I2C_M_NOSTART flag handling checks in V2.

> >
> >>> +
> >>> +             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 {
> >>> +             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?
> > Here comparing and taking minimum value of msg->len and
> IIC_RX_FIFO_DEPTH. The value of IIC_RX_FIFO_DEPTH is 16, before writing
> into register performed -1, so maximum value writing into RX_FIFO_PIRQ is
> 15.
> >
> Again, only if msg->len is not 0, which is not ensured.
> 

Agreed, we will fix in V2.

> Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 02/12] i2c: xiic: Enter standard mode only for > 255 byte read transfers

[Manikanta Guntupalli]

From: Raviteja Narayanam <raviteja.narayanam@xilinx.com>

To maintain backward compatibility the default transfer mode is dynamic
mode. Enter standard mode only when the size of read transfer is > 255
bytes.

Signed-off-by: Raviteja Narayanam <raviteja.narayanam@xilinx.com>
Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@xilinx.com>
---
 drivers/i2c/busses/i2c-xiic.c | 29 +++++++----------------------
 1 file changed, 7 insertions(+), 22 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index fb2443623844..c8b68176427c 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -904,7 +904,7 @@ static int xiic_start_xfer(struct xiic_i2c *i2c, struct i2c_msg *msgs, int num)
 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));
@@ -916,27 +916,12 @@ static int xiic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 	/* Decide standard mode or Dynamic mode */
 	i2c->dynamic = true;
 
-	/*
-	 * If number of messages is 1 and read length is > 255 bytes,
-	 * enter standard mode
-	 */
-
-	if (i2c->nmsgs == 1 && (i2c->tx_msg->flags & I2C_M_RD) &&
-	    i2c->tx_msg->len > MAX_READ_LENGTH_DYNAMIC) {
-		i2c->dynamic = false;
-	} else if (i2c->nmsgs > 1) {
-		int count;
-
-		/*
-		 * If number of messages is more than 1 and one of them is
-		 * a read message, enter standard mode. Since repeated start
-		 * operation in dynamic mode read is not happenning
-		 */
-		for (count = 0; count < i2c->nmsgs; count++) {
-			if (i2c->tx_msg[count].flags & I2C_M_RD) {
-				i2c->dynamic = false;
-				break;
-			}
+	/* 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;
 		}
 	}
 
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 02/12] i2c: xiic: Enter standard mode only for > 255 byte read transfers

[Krzysztof Adamski]

Hi,

W dniu 24.06.2022 o 14:05, Manikanta Guntupalli pisze:
> From: Raviteja Narayanam <raviteja.narayanam@xilinx.com>
>
> To maintain backward compatibility the default transfer mode is dynamic
> mode. Enter standard mode only when the size of read transfer is > 255
> bytes.
>
> Signed-off-by: Raviteja Narayanam <raviteja.narayanam@xilinx.com>
> Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@xilinx.com>

[...]

I don't really understand why this patch isn't squashed into previous 
one. The previous patch was only checking the first message which was 
wrong, this one fixes that.

Krzytof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 02/12] i2c: xiic: Enter standard mode only for > 255 byte read transfers

[Guntupalli, Manikanta]

> -----Original Message-----
> From: Krzysztof Adamski <krzysztof.adamski@nokia.com>
> Sent: Wednesday, June 29, 2022 5:52 PM
> To: Manikanta Guntupalli <manikanta.guntupalli@xilinx.com>;
> michal.simek@xilinx.com; Simek, Michal <michal.simek@amd.com>; linux-
> arm-kernel@lists.infradead.org; linux-i2c@vger.kernel.org; linux-
> kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.com>
> Cc: Raviteja Narayanam <raviteja.narayanam@xilinx.com>
> Subject: Re: [PATCH 02/12] i2c: xiic: Enter standard mode only for > 255 byte
> read transfers
> 
> 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,
> 
> W dniu 24.06.2022 o 14:05, Manikanta Guntupalli pisze:
> > From: Raviteja Narayanam <raviteja.narayanam@xilinx.com>
> >
> > To maintain backward compatibility the default transfer mode is
> > dynamic mode. Enter standard mode only when the size of read transfer
> > is > 255 bytes.
> >
> > Signed-off-by: Raviteja Narayanam <raviteja.narayanam@xilinx.com>
> > Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@xilinx.com>
> 
> [...]
> 
> I don't really understand why this patch isn't squashed into previous one. The
> previous patch was only checking the first message which was wrong, this
> one fixes that.

We will fix in  V2

Thanks,
Manikanta.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 03/12] i2c: xiic: Fix Rx and Tx paths in standard mode repeated start

[Manikanta Guntupalli]

From: Raviteja Narayanam <raviteja.narayanam@xilinx.com>

When a combined message request comes from user space, the
controller has to initiate repeated start sequence. In standard
mode, this repeated start sequence is corrupted if there is still
data in the Tx FIFO.

So, always make sure that all the bytes are completely transmitted
out of the FIFO by waiting for TXEMPTY, if the previous message is
of Tx type.

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

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index c8b68176427c..2d5214f1ba03 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -62,6 +62,7 @@ enum xiic_endian {
  * @singlemaster: Indicates bus is single master
  * @dynamic: Mode of controller
  * @repeated_start: Repeated start operation
+ * @prev_msg_tx: Previous message is Tx
  */
 struct xiic_i2c {
 	struct device *dev;
@@ -80,6 +81,7 @@ struct xiic_i2c {
 	bool singlemaster;
 	bool dynamic;
 	bool repeated_start;
+	bool prev_msg_tx;
 };
 
 #define XIIC_MSB_OFFSET 0
@@ -282,6 +284,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;
@@ -731,6 +751,20 @@ static void xiic_start_recv(struct xiic_i2c *i2c)
 		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 */
@@ -784,6 +818,8 @@ static void xiic_start_recv(struct xiic_i2c *i2c)
 
 	/* 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)
@@ -820,6 +856,20 @@ static void xiic_start_send(struct xiic_i2c *i2c)
 
 		xiic_fill_tx_fifo(i2c);
 	} 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) {
@@ -853,6 +903,7 @@ static void xiic_start_send(struct xiic_i2c *i2c)
 				XIIC_INTR_TX_ERROR_MASK |
 				XIIC_INTR_BNB_MASK);
 	}
+	i2c->prev_msg_tx = true;
 }
 
 static void __xiic_start_xfer(struct xiic_i2c *i2c)
@@ -916,6 +967,9 @@ static int xiic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int 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) &&
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 04/12] i2c: xiic: Add wait for FIFO empty in send_tx

[Manikanta Guntupalli]

From: Raviteja Narayanam <raviteja.narayanam@xilinx.com>

If the tx_half_empty interrupt comes first instead of tx_empty,
STOP bit is generated even before all the bytes are transmitted
out on the bus.
STOP bit should be sent only after all the bytes in the FIFO are
transmitted out of the FIFO. So wait until FIFO is empty before sending
the STOP bit.

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

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 2d5214f1ba03..9c3266ac209e 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -472,6 +472,12 @@ static void xiic_send_tx(struct xiic_i2c *i2c)
 
 		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);
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 05/12] i2c: xiic: Use 'nmsgs' variable instead of repeated_start

[Manikanta Guntupalli]

From: Raviteja Narayanam <raviteja.narayanam@xilinx.com>

Remove the redundant repeated_start variable as it is setup at
different places, and read at only one place which can be decided
by 'nmsgs' variable alone.

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

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 9c3266ac209e..3debb44608cf 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -61,7 +61,6 @@ enum xiic_endian {
  * @state: See STATE_
  * @singlemaster: Indicates bus is single master
  * @dynamic: Mode of controller
- * @repeated_start: Repeated start operation
  * @prev_msg_tx: Previous message is Tx
  */
 struct xiic_i2c {
@@ -80,7 +79,6 @@ struct xiic_i2c {
 	enum xilinx_i2c_state state;
 	bool singlemaster;
 	bool dynamic;
-	bool repeated_start;
 	bool prev_msg_tx;
 };
 
@@ -445,7 +443,7 @@ static void xiic_std_fill_tx_fifo(struct xiic_i2c *i2c)
 
 	if (len > fifo_space)
 		len = fifo_space;
-	else if (len && !(i2c->repeated_start))
+	else if (len && !(i2c->nmsgs > 1))
 		len--;
 
 	while (len--) {
@@ -786,7 +784,6 @@ static void xiic_start_recv(struct xiic_i2c *i2c)
 		}
 		/* Check if RSTA should be set */
 		if (cr & XIIC_CR_MSMS_MASK) {
-			i2c->repeated_start = true;
 			/* Already a master, RSTA should be set */
 			xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, (cr |
 					XIIC_CR_REPEATED_START_MASK) &
@@ -805,7 +802,6 @@ static void xiic_start_recv(struct xiic_i2c *i2c)
 
 		/* Write to Control Register,to start transaction in Rx mode */
 		if ((cr & XIIC_CR_MSMS_MASK) == 0) {
-			i2c->repeated_start = false;
 			xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, (cr |
 					XIIC_CR_MSMS_MASK)
 					& ~(XIIC_CR_DIR_IS_TX_MASK));
@@ -879,7 +875,6 @@ static void xiic_start_send(struct xiic_i2c *i2c)
 		/* Check if RSTA should be set */
 		cr = xiic_getreg8(i2c, XIIC_CR_REG_OFFSET);
 		if (cr & XIIC_CR_MSMS_MASK) {
-			i2c->repeated_start = true;
 			/* Already a master, RSTA should be set */
 			xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, (cr |
 					XIIC_CR_REPEATED_START_MASK |
@@ -895,7 +890,6 @@ static void xiic_start_send(struct xiic_i2c *i2c)
 		xiic_std_fill_tx_fifo(i2c);
 
 		if ((cr & XIIC_CR_MSMS_MASK) == 0) {
-			i2c->repeated_start = false;
 
 			/* Start Tx by writing to CR */
 			cr = xiic_getreg8(i2c, XIIC_CR_REG_OFFSET);
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 06/12] i2c: xiic: Add smbus_block_read functionality

[Manikanta Guntupalli]

From: Raviteja Narayanam <raviteja.narayanam@xilinx.com>

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>
Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@xilinx.com>
---
 drivers/i2c/busses/i2c-xiic.c | 82 ++++++++++++++++++++++++++++++++---
 1 file changed, 76 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 3debb44608cf..57ede62de67d 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -62,6 +62,7 @@ enum xiic_endian {
  * @singlemaster: Indicates bus is single master
  * @dynamic: Mode of controller
  * @prev_msg_tx: Previous message is Tx
+ * @smbus_block_read: Flag to handle block read
  */
 struct xiic_i2c {
 	struct device *dev;
@@ -80,6 +81,7 @@ struct xiic_i2c {
 	bool singlemaster;
 	bool dynamic;
 	bool prev_msg_tx;
+	bool smbus_block_read;
 };
 
 #define XIIC_MSB_OFFSET 0
@@ -339,6 +341,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;
@@ -361,6 +411,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;
 
@@ -777,8 +839,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;
 		}
@@ -970,10 +1036,14 @@ 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 */
+	/*
+	 * Enter standard mode only when read length is > 255 bytes or
+	 * for smbus_block_read transaction
+	 */
 	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) {
+		if (((i2c->tx_msg[count].flags & I2C_M_RD) &&
+		     i2c->tx_msg[count].len > MAX_READ_LENGTH_DYNAMIC) ||
+		    (i2c->tx_msg[count].flags & I2C_M_RECV_LEN)) {
 			i2c->dynamic = false;
 			break;
 		}
@@ -1007,7 +1077,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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 06/12] i2c: xiic: Add smbus_block_read functionality

[Krzysztof Adamski]

W dniu 24.06.2022 o 14:05, Manikanta Guntupalli pisze:
> From: Raviteja Narayanam <raviteja.narayanam@xilinx.com>
>
> 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>
> Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@xilinx.com>

I have successfully tested the patches from this series up until this 
one so:

Tested-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 07/12] i2c: xiic: Switch to Xiic standard mode for i2c-read

[Manikanta Guntupalli]

From: Raviteja Narayanam <raviteja.narayanam@xilinx.com>

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>
Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@xilinx.com>
---
 drivers/i2c/busses/i2c-xiic.c | 87 +++++++++++++++++++++++------------
 1 file changed, 58 insertions(+), 29 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 57ede62de67d..74cd50478694 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -32,6 +32,7 @@
 #include <linux/pm_runtime.h>
 
 #define DRIVER_NAME "xiic-i2c"
+#define DYNAMIC_MODE_READ_BROKEN_BIT	BIT(0)
 
 enum xilinx_i2c_state {
 	STATE_DONE,
@@ -63,6 +64,7 @@ enum xiic_endian {
  * @dynamic: Mode of controller
  * @prev_msg_tx: Previous message is Tx
  * @smbus_block_read: Flag to handle block read
+ * @quirks: To hold platform specific bug info
  */
 struct xiic_i2c {
 	struct device *dev;
@@ -82,6 +84,11 @@ struct xiic_i2c {
 	bool dynamic;
 	bool prev_msg_tx;
 	bool smbus_block_read;
+	u32 quirks;
+};
+
+struct xiic_version_data {
+	u32 quirks;
 };
 
 #define XIIC_MSB_OFFSET 0
@@ -995,7 +1002,8 @@ static void __xiic_start_xfer(struct xiic_i2c *i2c)
 
 static int xiic_start_xfer(struct xiic_i2c *i2c, struct i2c_msg *msgs, int num)
 {
-	int ret;
+	bool broken_read, max_read_len, smbus_blk_read;
+	int ret, count;
 
 	mutex_lock(&i2c->lock);
 
@@ -1008,6 +1016,34 @@ static int xiic_start_xfer(struct xiic_i2c *i2c, struct i2c_msg *msgs, int num)
 	i2c->nmsgs = num;
 	init_completion(&i2c->completion);
 
+	/* Decide standard mode or Dynamic mode */
+	i2c->dynamic = true;
+
+	/* Initialize prev message type */
+	i2c->prev_msg_tx = false;
+
+	/*
+	 * 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 || smbus_blk_read) {
+			i2c->dynamic = false;
+			break;
+		}
+	}
+
 	ret = xiic_reinit(i2c);
 	if (!ret)
 		__xiic_start_xfer(i2c);
@@ -1021,7 +1057,7 @@ static int xiic_start_xfer(struct xiic_i2c *i2c, struct i2c_msg *msgs, int num)
 static int xiic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 {
 	struct xiic_i2c *i2c = i2c_get_adapdata(adap);
-	int err, count;
+	int err;
 
 	dev_dbg(adap->dev.parent, "%s entry SR: 0x%x\n", __func__,
 		xiic_getreg8(i2c, XIIC_SR_REG_OFFSET));
@@ -1030,25 +1066,6 @@ static int xiic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 	if (err < 0)
 		return err;
 
-	/* 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 or
-	 * for smbus_block_read transaction
-	 */
-	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->tx_msg[count].flags & I2C_M_RECV_LEN)) {
-			i2c->dynamic = false;
-			break;
-		}
-	}
-
 	err = xiic_start_xfer(i2c, msgs, num);
 	if (err < 0) {
 		dev_err(adap->dev.parent, "Error xiic_start_xfer\n");
@@ -1091,10 +1108,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;
@@ -1104,6 +1134,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))
@@ -1215,14 +1252,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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 08/12] i2c: xiic: Remove interrupt enable/disable in Rx path

[Manikanta Guntupalli]

From: Raviteja Narayanam <raviteja.narayanam@xilinx.com>

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>
Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@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 74cd50478694..720aa671f74b 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -776,7 +776,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),
@@ -808,7 +807,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,
@@ -822,7 +820,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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 09/12] dt-bindings: i2c: xiic: Add 'xlnx,axi-iic-2.1' to compatible

[Manikanta Guntupalli]

From: Raviteja Narayanam <raviteja.narayanam@xilinx.com>

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 xilinx I2C new version 'xlnx,axi-iic-2.1' string to
compatible.

Signed-off-by: Raviteja Narayanam <raviteja.narayanam@xilinx.com>
Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Manikanta Guntupalli]

From: Raviteja Narayanam <raviteja.narayanam@xilinx.com>

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>
Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@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 720aa671f74b..f9492c39d3c7 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -1112,6 +1112,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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 11/12] i2c: xiic: Add SCL frequency configuration support

[Manikanta Guntupalli]

From: Raviteja Narayanam <raviteja.narayanam@xilinx.com>

From 'clock-frequency' device tree property, configure I2C SCL
frequency by calculating the timing register values according to
input clock.

After soft reset in reinit function, the timing registers are set
to default values (configured in vivado design). So, setting SCL
frequency is done inside reinit function after the soft reset.
This allows configuration of SCL frequency exclusively through
software via device tree property, overriding the vivado design.
If the clock-frequency parameter is not specified in DT, driver
doesn't configure frequency, making it backward compatible.

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

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index f9492c39d3c7..f29acabba5e7 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -65,6 +65,8 @@ enum xiic_endian {
  * @prev_msg_tx: Previous message is Tx
  * @smbus_block_read: Flag to handle block read
  * @quirks: To hold platform specific bug info
+ * @input_clk: Input clock to I2C controller
+ * @i2c_clk: I2C SCL frequency
  */
 struct xiic_i2c {
 	struct device *dev;
@@ -85,12 +87,30 @@ struct xiic_i2c {
 	bool prev_msg_tx;
 	bool smbus_block_read;
 	u32 quirks;
+	unsigned long input_clk;
+	unsigned int i2c_clk;
 };
 
 struct xiic_version_data {
 	u32 quirks;
 };
 
+/**
+ * struct timing_regs - AXI I2C timing registers that depend on I2C spec
+ * @tsusta: setup time for a repeated START condition
+ * @tsusto: setup time for a STOP condition
+ * @thdsta: hold time for a repeated START condition
+ * @tsudat: setup time for data
+ * @tbuf: bus free time between STOP and START
+ */
+struct timing_regs {
+	unsigned int tsusta;
+	unsigned int tsusto;
+	unsigned int thdsta;
+	unsigned int tsudat;
+	unsigned int tbuf;
+};
+
 #define XIIC_MSB_OFFSET 0
 #define XIIC_REG_OFFSET (0x100 + XIIC_MSB_OFFSET)
 
@@ -109,6 +129,19 @@ struct xiic_version_data {
 #define XIIC_RFD_REG_OFFSET  (0x20 + XIIC_REG_OFFSET)	/* Rx FIFO Depth reg  */
 #define XIIC_GPO_REG_OFFSET  (0x24 + XIIC_REG_OFFSET)	/* Output Register    */
 
+/*
+ * Timing register offsets from RegisterBase. These are used only for
+ * setting i2c clock frequency for the line.
+ */
+#define XIIC_TSUSTA_REG_OFFSET (0x28 + XIIC_REG_OFFSET) /* TSUSTA Register */
+#define XIIC_TSUSTO_REG_OFFSET (0x2C + XIIC_REG_OFFSET) /* TSUSTO Register */
+#define XIIC_THDSTA_REG_OFFSET (0x30 + XIIC_REG_OFFSET) /* THDSTA Register */
+#define XIIC_TSUDAT_REG_OFFSET (0x34 + XIIC_REG_OFFSET) /* TSUDAT Register */
+#define XIIC_TBUF_REG_OFFSET   (0x38 + XIIC_REG_OFFSET) /* TBUF Register */
+#define XIIC_THIGH_REG_OFFSET  (0x3C + XIIC_REG_OFFSET) /* THIGH Register */
+#define XIIC_TLOW_REG_OFFSET   (0x40 + XIIC_REG_OFFSET) /* TLOW Register */
+#define XIIC_THDDAT_REG_OFFSET (0x44 + XIIC_REG_OFFSET) /* THDDAT Register */
+
 /* Control Register masks */
 #define XIIC_CR_ENABLE_DEVICE_MASK        0x01	/* Device enable = 1      */
 #define XIIC_CR_TX_FIFO_RESET_MASK        0x02	/* Transmit FIFO reset=1  */
@@ -190,6 +223,7 @@ struct xiic_version_data {
 
 static int xiic_start_xfer(struct xiic_i2c *i2c, struct i2c_msg *msgs, int num);
 static void __xiic_start_xfer(struct xiic_i2c *i2c);
+static int xiic_setclk(struct xiic_i2c *i2c);
 
 /*
  * For the register read and write functions, a little-endian and big-endian
@@ -315,6 +349,10 @@ static int xiic_reinit(struct xiic_i2c *i2c)
 
 	xiic_setreg32(i2c, XIIC_RESETR_OFFSET, XIIC_RESET_MASK);
 
+	ret = xiic_setclk(i2c);
+	if (ret)
+		return ret;
+
 	/* Set receive Fifo depth to maximum (zero based). */
 	xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET, IIC_RX_FIFO_DEPTH - 1);
 
@@ -1118,6 +1156,105 @@ static const struct of_device_id xiic_of_match[] = {
 MODULE_DEVICE_TABLE(of, xiic_of_match);
 #endif
 
+enum i2c_scl_freq {
+	REG_VALUES_100KHZ = 0,
+	REG_VALUES_400KHZ = 1,
+	REG_VALUES_1MHZ = 2
+};
+
+/* Reg values in ns derived from I2C spec and AXI I2C PG for different frequencies */
+static const struct timing_regs timing_reg_values[] = {
+	{ 5700, 5000, 4300, 550, 5000 }, /* Reg values for 100KHz */
+	{ 900, 900, 900, 400, 1600 },    /* Reg values for 400KHz */
+	{ 380, 380, 380, 170, 620 },     /* Reg values for 1MHz   */
+};
+
+/**
+ * xiic_setclk - Sets the configured clock rate
+ * @i2c: Pointer to the xiic device structure
+ *
+ * The timing register values are calculated according to the input clock
+ * frequency and configured scl frequency. For details, please refer the
+ * AXI I2C PG and NXP I2C Spec.
+ * Supported frequencies are 100KHz, 400KHz and 1MHz.
+ *
+ * Return: 0 on success (Supported frequency selected or not configurable in SW)
+ *	   -EINVAL on failure (scl frequency not supported or THIGH is 0)
+ */
+static int xiic_setclk(struct xiic_i2c *i2c)
+{
+	unsigned int clk_in_mhz;
+	unsigned int index = 0;
+	u32 reg_val;
+
+	dev_dbg(i2c->adap.dev.parent,
+		"%s entry, i2c->input_clk: %ld, i2c->i2c_clk: %d\n",
+		__func__, i2c->input_clk, i2c->i2c_clk);
+
+	/* If not specified in DT, do not configure in SW. Rely only on Vivado design */
+	if (!i2c->i2c_clk)
+		return 0;
+
+	clk_in_mhz = DIV_ROUND_UP(i2c->input_clk, 1000000);
+
+	switch (i2c->i2c_clk) {
+	case I2C_MAX_FAST_MODE_PLUS_FREQ:
+		index = REG_VALUES_1MHZ;
+		break;
+	case I2C_MAX_FAST_MODE_FREQ:
+		index = REG_VALUES_400KHZ;
+		break;
+	case I2C_MAX_STANDARD_MODE_FREQ:
+		index = REG_VALUES_100KHZ;
+		break;
+	default:
+		dev_warn(i2c->adap.dev.parent, "Unsupported scl frequency\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * Value to be stored in a register is the number of clock cycles required
+	 * for the time duration. So the time is divided by the input clock time
+	 * period to get the number of clock cycles required. Refer Xilinx AXI I2C
+	 * PG document and I2C specification for further details.
+	 */
+
+	/* THIGH - Depends on SCL clock frequency(i2c_clk) as below */
+	reg_val = (DIV_ROUND_UP(i2c->input_clk, 2 * i2c->i2c_clk)) - 7;
+	if (reg_val == 0)
+		return -EINVAL;
+
+	xiic_setreg32(i2c, XIIC_THIGH_REG_OFFSET, reg_val - 1);
+
+	/* TLOW - Value same as THIGH */
+	xiic_setreg32(i2c, XIIC_TLOW_REG_OFFSET, reg_val - 1);
+
+	/* TSUSTA */
+	reg_val = (timing_reg_values[index].tsusta * clk_in_mhz) / 1000;
+	xiic_setreg32(i2c, XIIC_TSUSTA_REG_OFFSET, reg_val - 1);
+
+	/* TSUSTO */
+	reg_val = (timing_reg_values[index].tsusto * clk_in_mhz) / 1000;
+	xiic_setreg32(i2c, XIIC_TSUSTO_REG_OFFSET, reg_val - 1);
+
+	/* THDSTA */
+	reg_val = (timing_reg_values[index].thdsta * clk_in_mhz) / 1000;
+	xiic_setreg32(i2c, XIIC_THDSTA_REG_OFFSET, reg_val - 1);
+
+	/* TSUDAT */
+	reg_val = (timing_reg_values[index].tsudat * clk_in_mhz) / 1000;
+	xiic_setreg32(i2c, XIIC_TSUDAT_REG_OFFSET, reg_val - 1);
+
+	/* TBUF */
+	reg_val = (timing_reg_values[index].tbuf * clk_in_mhz) / 1000;
+	xiic_setreg32(i2c, XIIC_TBUF_REG_OFFSET, reg_val - 1);
+
+	/* THDDAT */
+	xiic_setreg32(i2c, XIIC_THDDAT_REG_OFFSET, 1);
+
+	return 0;
+}
+
 static int xiic_i2c_probe(struct platform_device *pdev)
 {
 	struct xiic_i2c *i2c;
@@ -1176,6 +1313,15 @@ static int xiic_i2c_probe(struct platform_device *pdev)
 	pm_runtime_use_autosuspend(i2c->dev);
 	pm_runtime_set_active(i2c->dev);
 	pm_runtime_enable(i2c->dev);
+
+	/* SCL frequency configuration */
+	i2c->input_clk = clk_get_rate(i2c->clk);
+	ret = of_property_read_u32(pdev->dev.of_node, "clock-frequency",
+				   &i2c->i2c_clk);
+	/* If clock-frequency not specified in DT, do not configure in SW */
+	if (ret || i2c->i2c_clk > I2C_MAX_FAST_MODE_PLUS_FREQ)
+		i2c->i2c_clk = 0;
+
 	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
 					xiic_process, IRQF_ONESHOT,
 					pdev->name, i2c);
@@ -1219,6 +1365,9 @@ static int xiic_i2c_probe(struct platform_device *pdev)
 			i2c_new_client_device(&i2c->adap, pdata->devices + i);
 	}
 
+	dev_dbg(&pdev->dev, "mmio %08lx irq %d scl clock frequency %d\n",
+		(unsigned long)res->start, irq, i2c->i2c_clk);
+
 	return 0;
 
 err_clk_dis:
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 12/12] i2c: xiic: Correct the BNB interrupt enable sequence

[Manikanta Guntupalli]

From: Srinivas Goud <srinivas.goud@xilinx.com>

With updated AXI IIC IP core(v2.1)there is change in IP behavior
in dynamic mode, where controller initiate read transfer on IIC
bus only after getting the value for the number of bytes to receive.

In the existing xiic_start_recv function Bus Not Busy(BNB)
interrupt is enabled just after "slave address + start"
write to FIFO and before the "count + stop"write to FIFO.
Since IIC controller drives the start address of a transaction
on the bus only after it has received the byte count information
the above sequence can lead to spurious BNB interrupt in case
there is any delay after "slave address + start" write to FIFO.

This is fixed by ensuring that BNB interrupt is enabled only
after "count + stop" has been written to FIFO.

Signed-off-by: Srinivas Goud <srinivas.goud@xilinx.com>
Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@xilinx.com>
---
 drivers/i2c/busses/i2c-xiic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index f29acabba5e7..0a34daa9e372 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -851,13 +851,13 @@ static void xiic_start_recv(struct xiic_i2c *i2c)
 				      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);
+
+		xiic_irq_clr_en(i2c, XIIC_INTR_BNB_MASK);
 	} else {
 		/*
 		 * If previous message is Tx, make sure that Tx FIFO is empty
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 12/12] i2c: xiic: Correct the BNB interrupt enable sequence

[Krzysztof Adamski]

W dniu 24.06.2022 o 14:05, Manikanta Guntupalli pisze:
> From: Srinivas Goud <srinivas.goud@xilinx.com>
>
> With updated AXI IIC IP core(v2.1)there is change in IP behavior
> in dynamic mode, where controller initiate read transfer on IIC
> bus only after getting the value for the number of bytes to receive.
>
> In the existing xiic_start_recv function Bus Not Busy(BNB)
> interrupt is enabled just after "slave address + start"
> write to FIFO and before the "count + stop"write to FIFO.
> Since IIC controller drives the start address of a transaction
> on the bus only after it has received the byte count information
> the above sequence can lead to spurious BNB interrupt in case
> there is any delay after "slave address + start" write to FIFO.
>
> This is fixed by ensuring that BNB interrupt is enabled only
> after "count + stop" has been written to FIFO.
>
> Signed-off-by: Srinivas Goud <srinivas.goud@xilinx.com>
> Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@xilinx.com>
> ---

[...]

Does this spurious interrupt cause any trouble or it is just ignored and 
the only problem is unneeded extra CPU load?

Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 12/12] i2c: xiic: Correct the BNB interrupt enable sequence

[Guntupalli, Manikanta]

Hi Krzysztof,

> -----Original Message-----
> From: Krzysztof Adamski <krzysztof.adamski@nokia.com>
> Sent: Wednesday, June 29, 2022 6:44 PM
> To: Manikanta Guntupalli <manikanta.guntupalli@xilinx.com>;
> michal.simek@xilinx.com; Simek, Michal <michal.simek@amd.com>; linux-
> arm-kernel@lists.infradead.org; linux-i2c@vger.kernel.org; linux-
> kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.com>
> Cc: Srinivas Goud <srinivas.goud@xilinx.com>
> Subject: Re: [PATCH 12/12] i2c: xiic: Correct the BNB interrupt enable
> sequence
> 
> 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.
> 
> 
> W dniu 24.06.2022 o 14:05, Manikanta Guntupalli pisze:
> > From: Srinivas Goud <srinivas.goud@xilinx.com>
> >
> > With updated AXI IIC IP core(v2.1)there is change in IP behavior in
> > dynamic mode, where controller initiate read transfer on IIC bus only
> > after getting the value for the number of bytes to receive.
> >
> > In the existing xiic_start_recv function Bus Not Busy(BNB) interrupt
> > is enabled just after "slave address + start"
> > write to FIFO and before the "count + stop"write to FIFO.
> > Since IIC controller drives the start address of a transaction on the
> > bus only after it has received the byte count information the above
> > sequence can lead to spurious BNB interrupt in case there is any delay
> > after "slave address + start" write to FIFO.
> >
> > This is fixed by ensuring that BNB interrupt is enabled only after
> > "count + stop" has been written to FIFO.
> >
> > Signed-off-by: Srinivas Goud <srinivas.goud@xilinx.com>
> > Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@xilinx.com>
> > ---
> 
> [...]
> 
> Does this spurious interrupt cause any trouble or it is just ignored and the
> only problem is unneeded extra CPU load?
> 

No functional issue we just corrected the sequence as per IP changes.

> Krzysztof


Thanks,
Manikanta.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel