[PATCH v3 0/6] fix iproc driver to handle master read request

[PATCH v3 0/6] fix iproc driver to handle master read request

[Rayagonda Kokatanur]

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

This series of patches adds the following,
- Handle master abort error
- Fix support for single/multi byte master read request with/without
repeated start.
- Handle rx fifo full interrupt
- Fix typo

Changes from V2:
--Add missing Acked-by tag.

Changes from V1:
--Address review comments from Ray Jui,
  Remove fixes tag

Rayagonda Kokatanur (6):
  i2c: iproc: handle Master aborted error
  i2c: iproc: handle only slave interrupts which are enabled
  i2c: iproc: update slave isr mask (ISR_MASK_SLAVE)
  i2c: iproc: fix typo in slave_isr function
  i2c: iproc: handle master read request
  i2c: iproc: handle rx fifo full interrupt

 drivers/i2c/busses/i2c-bcm-iproc.c | 254 +++++++++++++++++++++++------
 1 file changed, 200 insertions(+), 54 deletions(-)

-- 
2.17.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4187 bytes --]

[PATCH v3 0/6] fix iproc driver to handle master read request

[Rayagonda Kokatanur]

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

This series of patches adds the following,
- Handle master abort error
- Fix support for single/multi byte master read request with/without
repeated start.
- Handle rx fifo full interrupt
- Fix typo

Changes from V2:
--Add missing Acked-by tag.

Changes from V1:
--Address review comments from Ray Jui,
  Remove fixes tag

Rayagonda Kokatanur (6):
  i2c: iproc: handle Master aborted error
  i2c: iproc: handle only slave interrupts which are enabled
  i2c: iproc: update slave isr mask (ISR_MASK_SLAVE)
  i2c: iproc: fix typo in slave_isr function
  i2c: iproc: handle master read request
  i2c: iproc: handle rx fifo full interrupt

 drivers/i2c/busses/i2c-bcm-iproc.c | 254 +++++++++++++++++++++++------
 1 file changed, 200 insertions(+), 54 deletions(-)

-- 
2.17.1


[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4187 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

[PATCH v3 1/6] i2c: iproc: handle Master aborted error

[Rayagonda Kokatanur]

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

Handle Master aborted error by flushing tx and rx fifo
and reinitializing the hw.

Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
Acked-by: Ray Jui <ray.jui@broadcom.com>
---
 drivers/i2c/busses/i2c-bcm-iproc.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index d8295b1c379d..834a98caeada 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -93,6 +93,7 @@
 #define S_CMD_STATUS_MASK            0x07
 #define S_CMD_STATUS_SUCCESS         0x0
 #define S_CMD_STATUS_TIMEOUT         0x5
+#define S_CMD_STATUS_MASTER_ABORT    0x7
 
 #define IE_OFFSET                    0x38
 #define IE_M_RX_FIFO_FULL_SHIFT      31
@@ -311,9 +312,10 @@ static void bcm_iproc_i2c_check_slave_status(
 		return;
 
 	val = (val >> S_CMD_STATUS_SHIFT) & S_CMD_STATUS_MASK;
-	if (val == S_CMD_STATUS_TIMEOUT) {
-		dev_err(iproc_i2c->device, "slave random stretch time timeout\n");
-
+	if (val == S_CMD_STATUS_TIMEOUT || val == S_CMD_STATUS_MASTER_ABORT) {
+		dev_err(iproc_i2c->device, (val == S_CMD_STATUS_TIMEOUT) ?
+			"slave random stretch time timeout\n" :
+			"Master aborted read transaction\n");
 		/* re-initialize i2c for recovery */
 		bcm_iproc_i2c_enable_disable(iproc_i2c, false);
 		bcm_iproc_i2c_slave_init(iproc_i2c, true);
-- 
2.17.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4187 bytes --]

[PATCH v3 1/6] i2c: iproc: handle Master aborted error

[Rayagonda Kokatanur]

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

Handle Master aborted error by flushing tx and rx fifo
and reinitializing the hw.

Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
Acked-by: Ray Jui <ray.jui@broadcom.com>
---
 drivers/i2c/busses/i2c-bcm-iproc.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index d8295b1c379d..834a98caeada 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -93,6 +93,7 @@
 #define S_CMD_STATUS_MASK            0x07
 #define S_CMD_STATUS_SUCCESS         0x0
 #define S_CMD_STATUS_TIMEOUT         0x5
+#define S_CMD_STATUS_MASTER_ABORT    0x7
 
 #define IE_OFFSET                    0x38
 #define IE_M_RX_FIFO_FULL_SHIFT      31
@@ -311,9 +312,10 @@ static void bcm_iproc_i2c_check_slave_status(
 		return;
 
 	val = (val >> S_CMD_STATUS_SHIFT) & S_CMD_STATUS_MASK;
-	if (val == S_CMD_STATUS_TIMEOUT) {
-		dev_err(iproc_i2c->device, "slave random stretch time timeout\n");
-
+	if (val == S_CMD_STATUS_TIMEOUT || val == S_CMD_STATUS_MASTER_ABORT) {
+		dev_err(iproc_i2c->device, (val == S_CMD_STATUS_TIMEOUT) ?
+			"slave random stretch time timeout\n" :
+			"Master aborted read transaction\n");
 		/* re-initialize i2c for recovery */
 		bcm_iproc_i2c_enable_disable(iproc_i2c, false);
 		bcm_iproc_i2c_slave_init(iproc_i2c, true);
-- 
2.17.1


[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4187 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

[PATCH v3 2/6] i2c: iproc: handle only slave interrupts which are enabled

[Rayagonda Kokatanur]

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

Handle only slave interrupts which are enabled.

The IS_OFFSET register contains the interrupt status bits which will be
set regardless of the enabling of the corresponding interrupt condition.
One must therefore look at both IS_OFFSET and IE_OFFSET to determine
whether an interrupt condition is set and enabled.

Fixes: c245d94ed106 ("i2c: iproc: Add multi byte read-write support for slave mode")
Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
Acked-by: Ray Jui <ray.jui@broadcom.com>
---
 drivers/i2c/busses/i2c-bcm-iproc.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index 834a98caeada..b54f5130d246 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -507,12 +507,17 @@ static void bcm_iproc_i2c_process_m_event(struct bcm_iproc_i2c_dev *iproc_i2c,
 static irqreturn_t bcm_iproc_i2c_isr(int irq, void *data)
 {
 	struct bcm_iproc_i2c_dev *iproc_i2c = data;
-	u32 status = iproc_i2c_rd_reg(iproc_i2c, IS_OFFSET);
+	u32 slave_status;
+	u32 status;
 	bool ret;
-	u32 sl_status = status & ISR_MASK_SLAVE;
 
-	if (sl_status) {
-		ret = bcm_iproc_i2c_slave_isr(iproc_i2c, sl_status);
+	status = iproc_i2c_rd_reg(iproc_i2c, IS_OFFSET);
+	/* process only slave interrupt which are enabled */
+	slave_status = status & iproc_i2c_rd_reg(iproc_i2c, IE_OFFSET) &
+		       ISR_MASK_SLAVE;
+
+	if (slave_status) {
+		ret = bcm_iproc_i2c_slave_isr(iproc_i2c, slave_status);
 		if (ret)
 			return IRQ_HANDLED;
 		else
-- 
2.17.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4187 bytes --]

[PATCH v3 2/6] i2c: iproc: handle only slave interrupts which are enabled

[Rayagonda Kokatanur]

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

Handle only slave interrupts which are enabled.

The IS_OFFSET register contains the interrupt status bits which will be
set regardless of the enabling of the corresponding interrupt condition.
One must therefore look at both IS_OFFSET and IE_OFFSET to determine
whether an interrupt condition is set and enabled.

Fixes: c245d94ed106 ("i2c: iproc: Add multi byte read-write support for slave mode")
Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
Acked-by: Ray Jui <ray.jui@broadcom.com>
---
 drivers/i2c/busses/i2c-bcm-iproc.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index 834a98caeada..b54f5130d246 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -507,12 +507,17 @@ static void bcm_iproc_i2c_process_m_event(struct bcm_iproc_i2c_dev *iproc_i2c,
 static irqreturn_t bcm_iproc_i2c_isr(int irq, void *data)
 {
 	struct bcm_iproc_i2c_dev *iproc_i2c = data;
-	u32 status = iproc_i2c_rd_reg(iproc_i2c, IS_OFFSET);
+	u32 slave_status;
+	u32 status;
 	bool ret;
-	u32 sl_status = status & ISR_MASK_SLAVE;
 
-	if (sl_status) {
-		ret = bcm_iproc_i2c_slave_isr(iproc_i2c, sl_status);
+	status = iproc_i2c_rd_reg(iproc_i2c, IS_OFFSET);
+	/* process only slave interrupt which are enabled */
+	slave_status = status & iproc_i2c_rd_reg(iproc_i2c, IE_OFFSET) &
+		       ISR_MASK_SLAVE;
+
+	if (slave_status) {
+		ret = bcm_iproc_i2c_slave_isr(iproc_i2c, slave_status);
 		if (ret)
 			return IRQ_HANDLED;
 		else
-- 
2.17.1


[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4187 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

[PATCH v3 3/6] i2c: iproc: update slave isr mask (ISR_MASK_SLAVE)

[Rayagonda Kokatanur]

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

Update slave isr mask (ISR_MASK_SLAVE) to include remaining
two slave interrupts.

Fixes: c245d94ed106 ("i2c: iproc: Add multi byte read-write support for slave mode")
Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
Acked-by: Ray Jui <ray.jui@broadcom.com>
---
 drivers/i2c/busses/i2c-bcm-iproc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index b54f5130d246..cd687696bf0b 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -216,7 +216,8 @@ struct bcm_iproc_i2c_dev {
 
 #define ISR_MASK_SLAVE (BIT(IS_S_START_BUSY_SHIFT)\
 		| BIT(IS_S_RX_EVENT_SHIFT) | BIT(IS_S_RD_EVENT_SHIFT)\
-		| BIT(IS_S_TX_UNDERRUN_SHIFT))
+		| BIT(IS_S_TX_UNDERRUN_SHIFT) | BIT(IS_S_RX_FIFO_FULL_SHIFT)\
+		| BIT(IS_S_RX_THLD_SHIFT))
 
 static int bcm_iproc_i2c_reg_slave(struct i2c_client *slave);
 static int bcm_iproc_i2c_unreg_slave(struct i2c_client *slave);
-- 
2.17.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4187 bytes --]

[PATCH v3 3/6] i2c: iproc: update slave isr mask (ISR_MASK_SLAVE)

[Rayagonda Kokatanur]

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

Update slave isr mask (ISR_MASK_SLAVE) to include remaining
two slave interrupts.

Fixes: c245d94ed106 ("i2c: iproc: Add multi byte read-write support for slave mode")
Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
Acked-by: Ray Jui <ray.jui@broadcom.com>
---
 drivers/i2c/busses/i2c-bcm-iproc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index b54f5130d246..cd687696bf0b 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -216,7 +216,8 @@ struct bcm_iproc_i2c_dev {
 
 #define ISR_MASK_SLAVE (BIT(IS_S_START_BUSY_SHIFT)\
 		| BIT(IS_S_RX_EVENT_SHIFT) | BIT(IS_S_RD_EVENT_SHIFT)\
-		| BIT(IS_S_TX_UNDERRUN_SHIFT))
+		| BIT(IS_S_TX_UNDERRUN_SHIFT) | BIT(IS_S_RX_FIFO_FULL_SHIFT)\
+		| BIT(IS_S_RX_THLD_SHIFT))
 
 static int bcm_iproc_i2c_reg_slave(struct i2c_client *slave);
 static int bcm_iproc_i2c_unreg_slave(struct i2c_client *slave);
-- 
2.17.1


[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4187 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

[PATCH v3 4/6] i2c: iproc: fix typo in slave_isr function

[Rayagonda Kokatanur]

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

Fix typo in bcm_iproc_i2c_slave_isr().

Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
---
 drivers/i2c/busses/i2c-bcm-iproc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index cd687696bf0b..7a235f9f5884 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -382,7 +382,7 @@ static bool bcm_iproc_i2c_slave_isr(struct bcm_iproc_i2c_dev *iproc_i2c,
 	if (status & BIT(IS_S_START_BUSY_SHIFT)) {
 		i2c_slave_event(iproc_i2c->slave, I2C_SLAVE_STOP, &value);
 		/*
-		 * Enable interrupt for TX FIFO becomes empty and
+		 * Disable interrupt for TX FIFO becomes empty and
 		 * less than PKT_LENGTH bytes were output on the SMBUS
 		 */
 		val = iproc_i2c_rd_reg(iproc_i2c, IE_OFFSET);
-- 
2.17.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4187 bytes --]

[PATCH v3 4/6] i2c: iproc: fix typo in slave_isr function

[Rayagonda Kokatanur]

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

Fix typo in bcm_iproc_i2c_slave_isr().

Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
---
 drivers/i2c/busses/i2c-bcm-iproc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index cd687696bf0b..7a235f9f5884 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -382,7 +382,7 @@ static bool bcm_iproc_i2c_slave_isr(struct bcm_iproc_i2c_dev *iproc_i2c,
 	if (status & BIT(IS_S_START_BUSY_SHIFT)) {
 		i2c_slave_event(iproc_i2c->slave, I2C_SLAVE_STOP, &value);
 		/*
-		 * Enable interrupt for TX FIFO becomes empty and
+		 * Disable interrupt for TX FIFO becomes empty and
 		 * less than PKT_LENGTH bytes were output on the SMBUS
 		 */
 		val = iproc_i2c_rd_reg(iproc_i2c, IE_OFFSET);
-- 
2.17.1


[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4187 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

[PATCH v3 5/6] i2c: iproc: handle master read request

[Rayagonda Kokatanur]

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

Handle single or multi byte master read request with or without
repeated start.

Fixes: c245d94ed106 ("i2c: iproc: Add multi byte read-write support for slave mode")
Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
---
 drivers/i2c/busses/i2c-bcm-iproc.c | 215 +++++++++++++++++++++++------
 1 file changed, 170 insertions(+), 45 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index 7a235f9f5884..22e04055b447 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -160,6 +160,11 @@
 
 #define IE_S_ALL_INTERRUPT_SHIFT     21
 #define IE_S_ALL_INTERRUPT_MASK      0x3f
+/*
+ * It takes ~18us to reading 10bytes of data, hence to keep tasklet
+ * running for less time, max slave read per tasklet is set to 10 bytes.
+ */
+#define MAX_SLAVE_RX_PER_INT         10
 
 enum i2c_slave_read_status {
 	I2C_SLAVE_RX_FIFO_EMPTY = 0,
@@ -206,8 +211,18 @@ struct bcm_iproc_i2c_dev {
 	/* bytes that have been read */
 	unsigned int rx_bytes;
 	unsigned int thld_bytes;
+
+	bool slave_rx_only;
+	bool rx_start_rcvd;
+	bool slave_read_complete;
+	u32 tx_underrun;
+	u32 slave_int_mask;
+	struct tasklet_struct slave_rx_tasklet;
 };
 
+/* tasklet to process slave rx data */
+static void slave_rx_tasklet_fn(unsigned long);
+
 /*
  * Can be expanded in the future if more interrupt status bits are utilized
  */
@@ -261,6 +276,7 @@ static void bcm_iproc_i2c_slave_init(
 {
 	u32 val;
 
+	iproc_i2c->tx_underrun = 0;
 	if (need_reset) {
 		/* put controller in reset */
 		val = iproc_i2c_rd_reg(iproc_i2c, CFG_OFFSET);
@@ -297,8 +313,11 @@ static void bcm_iproc_i2c_slave_init(
 
 	/* Enable interrupt register to indicate a valid byte in receive fifo */
 	val = BIT(IE_S_RX_EVENT_SHIFT);
+	/* Enable interrupt register to indicate a Master read transaction */
+	val |= BIT(IE_S_RD_EVENT_SHIFT);
 	/* Enable interrupt register for the Slave BUSY command */
 	val |= BIT(IE_S_START_BUSY_SHIFT);
+	iproc_i2c->slave_int_mask = val;
 	iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, val);
 }
 
@@ -324,76 +343,176 @@ static void bcm_iproc_i2c_check_slave_status(
 	}
 }
 
-static bool bcm_iproc_i2c_slave_isr(struct bcm_iproc_i2c_dev *iproc_i2c,
-				    u32 status)
+static void bcm_iproc_i2c_slave_read(struct bcm_iproc_i2c_dev *iproc_i2c)
 {
+	u8 rx_data, rx_status;
+	u32 rx_bytes = 0;
 	u32 val;
-	u8 value, rx_status;
 
-	/* Slave RX byte receive */
-	if (status & BIT(IS_S_RX_EVENT_SHIFT)) {
+	while (rx_bytes < MAX_SLAVE_RX_PER_INT) {
 		val = iproc_i2c_rd_reg(iproc_i2c, S_RX_OFFSET);
 		rx_status = (val >> S_RX_STATUS_SHIFT) & S_RX_STATUS_MASK;
-		if (rx_status == I2C_SLAVE_RX_START) {
-			/* Start of SMBUS for Master write */
-			i2c_slave_event(iproc_i2c->slave,
-					I2C_SLAVE_WRITE_REQUESTED, &value);
+		rx_data = ((val >> S_RX_DATA_SHIFT) & S_RX_DATA_MASK);
 
-			val = iproc_i2c_rd_reg(iproc_i2c, S_RX_OFFSET);
-			value = (u8)((val >> S_RX_DATA_SHIFT) & S_RX_DATA_MASK);
+		if (rx_status == I2C_SLAVE_RX_START) {
+			/* Start of SMBUS Master write */
 			i2c_slave_event(iproc_i2c->slave,
-					I2C_SLAVE_WRITE_RECEIVED, &value);
-		} else if (status & BIT(IS_S_RD_EVENT_SHIFT)) {
-			/* Start of SMBUS for Master Read */
+					I2C_SLAVE_WRITE_REQUESTED, &rx_data);
+			iproc_i2c->rx_start_rcvd = true;
+			iproc_i2c->slave_read_complete = false;
+		} else if (rx_status == I2C_SLAVE_RX_DATA &&
+			   iproc_i2c->rx_start_rcvd) {
+			/* Middle of SMBUS Master write */
 			i2c_slave_event(iproc_i2c->slave,
-					I2C_SLAVE_READ_REQUESTED, &value);
-			iproc_i2c_wr_reg(iproc_i2c, S_TX_OFFSET, value);
+					I2C_SLAVE_WRITE_RECEIVED, &rx_data);
+		} else if (rx_status == I2C_SLAVE_RX_END &&
+			   iproc_i2c->rx_start_rcvd) {
+			/* End of SMBUS Master write */
+			if (iproc_i2c->slave_rx_only)
+				i2c_slave_event(iproc_i2c->slave,
+						I2C_SLAVE_WRITE_RECEIVED,
+						&rx_data);
+
+			i2c_slave_event(iproc_i2c->slave, I2C_SLAVE_STOP,
+					&rx_data);
+		} else if (rx_status == I2C_SLAVE_RX_FIFO_EMPTY) {
+			iproc_i2c->rx_start_rcvd = false;
+			iproc_i2c->slave_read_complete = true;
+			break;
+		}
 
-			val = BIT(S_CMD_START_BUSY_SHIFT);
-			iproc_i2c_wr_reg(iproc_i2c, S_CMD_OFFSET, val);
+		rx_bytes++;
+	}
+}
 
-			/*
-			 * Enable interrupt for TX FIFO becomes empty and
-			 * less than PKT_LENGTH bytes were output on the SMBUS
-			 */
-			val = iproc_i2c_rd_reg(iproc_i2c, IE_OFFSET);
-			val |= BIT(IE_S_TX_UNDERRUN_SHIFT);
-			iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, val);
-		} else {
-			/* Master write other than start */
-			value = (u8)((val >> S_RX_DATA_SHIFT) & S_RX_DATA_MASK);
+static void slave_rx_tasklet_fn(unsigned long data)
+{
+	struct bcm_iproc_i2c_dev *iproc_i2c = (struct bcm_iproc_i2c_dev *)data;
+	u32 int_clr;
+
+	bcm_iproc_i2c_slave_read(iproc_i2c);
+
+	/* clear pending IS_S_RX_EVENT_SHIFT interrupt */
+	int_clr = BIT(IS_S_RX_EVENT_SHIFT);
+
+	if (!iproc_i2c->slave_rx_only && iproc_i2c->slave_read_complete) {
+		/*
+		 * In case of single byte master-read request,
+		 * IS_S_TX_UNDERRUN_SHIFT event is generated before
+		 * IS_S_START_BUSY_SHIFT event. Hence start slave data send
+		 * from first IS_S_TX_UNDERRUN_SHIFT event.
+		 *
+		 * This means don't send any data from slave when
+		 * IS_S_RD_EVENT_SHIFT event is generated else it will increment
+		 * eeprom or other backend slave driver read pointer twice.
+		 */
+		iproc_i2c->tx_underrun = 0;
+		iproc_i2c->slave_int_mask |= BIT(IE_S_TX_UNDERRUN_SHIFT);
+
+		/* clear IS_S_RD_EVENT_SHIFT interrupt */
+		int_clr |= BIT(IS_S_RD_EVENT_SHIFT);
+	}
+
+	/* clear slave interrupt */
+	iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET, int_clr);
+	/* enable slave interrupts */
+	iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, iproc_i2c->slave_int_mask);
+}
+
+static bool bcm_iproc_i2c_slave_isr(struct bcm_iproc_i2c_dev *iproc_i2c,
+				    u32 status)
+{
+	u32 val;
+	u8 value;
+
+	/*
+	 * Slave events in case of master-write, master-write-read and,
+	 * master-read
+	 *
+	 * Master-write     : only IS_S_RX_EVENT_SHIFT event
+	 * Master-write-read: both IS_S_RX_EVENT_SHIFT and IS_S_RD_EVENT_SHIFT
+	 *                    events
+	 * Master-read      : both IS_S_RX_EVENT_SHIFT and IS_S_RD_EVENT_SHIFT
+	 *                    events or only IS_S_RD_EVENT_SHIFT
+	 */
+	if (status & BIT(IS_S_RX_EVENT_SHIFT) ||
+	    status & BIT(IS_S_RD_EVENT_SHIFT)) {
+		/* disable slave interrupts */
+		val = iproc_i2c_rd_reg(iproc_i2c, IE_OFFSET);
+		val &= ~iproc_i2c->slave_int_mask;
+		iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, val);
+
+		if (status & BIT(IS_S_RD_EVENT_SHIFT))
+			/* Master-write-read request */
+			iproc_i2c->slave_rx_only = false;
+		else
+			/* Master-write request only */
+			iproc_i2c->slave_rx_only = true;
+
+		/* schedule tasklet to read data later */
+		tasklet_schedule(&iproc_i2c->slave_rx_tasklet);
+
+		/* clear only IS_S_RX_EVENT_SHIFT interrupt */
+		iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET,
+				 BIT(IS_S_RX_EVENT_SHIFT));
+	}
+
+	if (status & BIT(IS_S_TX_UNDERRUN_SHIFT)) {
+		iproc_i2c->tx_underrun++;
+		if (iproc_i2c->tx_underrun == 1)
+			/* Start of SMBUS for Master Read */
 			i2c_slave_event(iproc_i2c->slave,
-					I2C_SLAVE_WRITE_RECEIVED, &value);
-			if (rx_status == I2C_SLAVE_RX_END)
-				i2c_slave_event(iproc_i2c->slave,
-						I2C_SLAVE_STOP, &value);
-		}
-	} else if (status & BIT(IS_S_TX_UNDERRUN_SHIFT)) {
-		/* Master read other than start */
-		i2c_slave_event(iproc_i2c->slave,
-				I2C_SLAVE_READ_PROCESSED, &value);
+					I2C_SLAVE_READ_REQUESTED,
+					&value);
+		else
+			/* Master read other than start */
+			i2c_slave_event(iproc_i2c->slave,
+					I2C_SLAVE_READ_PROCESSED,
+					&value);
 
 		iproc_i2c_wr_reg(iproc_i2c, S_TX_OFFSET, value);
+		/* start transfer */
 		val = BIT(S_CMD_START_BUSY_SHIFT);
 		iproc_i2c_wr_reg(iproc_i2c, S_CMD_OFFSET, val);
+
+		/* clear interrupt */
+		iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET,
+				 BIT(IS_S_TX_UNDERRUN_SHIFT));
 	}
 
-	/* Stop */
+	/* Stop received from master in case of master read transaction */
 	if (status & BIT(IS_S_START_BUSY_SHIFT)) {
-		i2c_slave_event(iproc_i2c->slave, I2C_SLAVE_STOP, &value);
 		/*
 		 * Disable interrupt for TX FIFO becomes empty and
 		 * less than PKT_LENGTH bytes were output on the SMBUS
 		 */
-		val = iproc_i2c_rd_reg(iproc_i2c, IE_OFFSET);
-		val &= ~BIT(IE_S_TX_UNDERRUN_SHIFT);
-		iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, val);
+		iproc_i2c->slave_int_mask &= ~BIT(IE_S_TX_UNDERRUN_SHIFT);
+		iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET,
+				 iproc_i2c->slave_int_mask);
+
+		/* End of SMBUS for Master Read */
+		val = BIT(S_TX_WR_STATUS_SHIFT);
+		iproc_i2c_wr_reg(iproc_i2c, S_TX_OFFSET, val);
+
+		val = BIT(S_CMD_START_BUSY_SHIFT);
+		iproc_i2c_wr_reg(iproc_i2c, S_CMD_OFFSET, val);
+
+		/* flush TX FIFOs */
+		val = iproc_i2c_rd_reg(iproc_i2c, S_FIFO_CTRL_OFFSET);
+		val |= (BIT(S_FIFO_TX_FLUSH_SHIFT));
+		iproc_i2c_wr_reg(iproc_i2c, S_FIFO_CTRL_OFFSET, val);
+
+		i2c_slave_event(iproc_i2c->slave, I2C_SLAVE_STOP, &value);
+
+		/* clear interrupt */
+		iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET,
+				 BIT(IS_S_START_BUSY_SHIFT));
 	}
 
-	/* clear interrupt status */
-	iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET, status);
+	/* check slave transmit status only if slave is transmitting */
+	if (!iproc_i2c->slave_rx_only)
+		bcm_iproc_i2c_check_slave_status(iproc_i2c);
 
-	bcm_iproc_i2c_check_slave_status(iproc_i2c);
 	return true;
 }
 
@@ -1074,6 +1193,10 @@ static int bcm_iproc_i2c_reg_slave(struct i2c_client *slave)
 		return -EAFNOSUPPORT;
 
 	iproc_i2c->slave = slave;
+
+	tasklet_init(&iproc_i2c->slave_rx_tasklet, slave_rx_tasklet_fn,
+		     (unsigned long)iproc_i2c);
+
 	bcm_iproc_i2c_slave_init(iproc_i2c, false);
 	return 0;
 }
@@ -1094,6 +1217,8 @@ static int bcm_iproc_i2c_unreg_slave(struct i2c_client *slave)
 			IE_S_ALL_INTERRUPT_SHIFT);
 	iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, tmp);
 
+	tasklet_kill(&iproc_i2c->slave_rx_tasklet);
+
 	/* Erase the slave address programmed */
 	tmp = iproc_i2c_rd_reg(iproc_i2c, S_CFG_SMBUS_ADDR_OFFSET);
 	tmp &= ~BIT(S_CFG_EN_NIC_SMB_ADDR3_SHIFT);
-- 
2.17.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4187 bytes --]

[PATCH v3 5/6] i2c: iproc: handle master read request

[Rayagonda Kokatanur]

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

Handle single or multi byte master read request with or without
repeated start.

Fixes: c245d94ed106 ("i2c: iproc: Add multi byte read-write support for slave mode")
Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
---
 drivers/i2c/busses/i2c-bcm-iproc.c | 215 +++++++++++++++++++++++------
 1 file changed, 170 insertions(+), 45 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index 7a235f9f5884..22e04055b447 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -160,6 +160,11 @@
 
 #define IE_S_ALL_INTERRUPT_SHIFT     21
 #define IE_S_ALL_INTERRUPT_MASK      0x3f
+/*
+ * It takes ~18us to reading 10bytes of data, hence to keep tasklet
+ * running for less time, max slave read per tasklet is set to 10 bytes.
+ */
+#define MAX_SLAVE_RX_PER_INT         10
 
 enum i2c_slave_read_status {
 	I2C_SLAVE_RX_FIFO_EMPTY = 0,
@@ -206,8 +211,18 @@ struct bcm_iproc_i2c_dev {
 	/* bytes that have been read */
 	unsigned int rx_bytes;
 	unsigned int thld_bytes;
+
+	bool slave_rx_only;
+	bool rx_start_rcvd;
+	bool slave_read_complete;
+	u32 tx_underrun;
+	u32 slave_int_mask;
+	struct tasklet_struct slave_rx_tasklet;
 };
 
+/* tasklet to process slave rx data */
+static void slave_rx_tasklet_fn(unsigned long);
+
 /*
  * Can be expanded in the future if more interrupt status bits are utilized
  */
@@ -261,6 +276,7 @@ static void bcm_iproc_i2c_slave_init(
 {
 	u32 val;
 
+	iproc_i2c->tx_underrun = 0;
 	if (need_reset) {
 		/* put controller in reset */
 		val = iproc_i2c_rd_reg(iproc_i2c, CFG_OFFSET);
@@ -297,8 +313,11 @@ static void bcm_iproc_i2c_slave_init(
 
 	/* Enable interrupt register to indicate a valid byte in receive fifo */
 	val = BIT(IE_S_RX_EVENT_SHIFT);
+	/* Enable interrupt register to indicate a Master read transaction */
+	val |= BIT(IE_S_RD_EVENT_SHIFT);
 	/* Enable interrupt register for the Slave BUSY command */
 	val |= BIT(IE_S_START_BUSY_SHIFT);
+	iproc_i2c->slave_int_mask = val;
 	iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, val);
 }
 
@@ -324,76 +343,176 @@ static void bcm_iproc_i2c_check_slave_status(
 	}
 }
 
-static bool bcm_iproc_i2c_slave_isr(struct bcm_iproc_i2c_dev *iproc_i2c,
-				    u32 status)
+static void bcm_iproc_i2c_slave_read(struct bcm_iproc_i2c_dev *iproc_i2c)
 {
+	u8 rx_data, rx_status;
+	u32 rx_bytes = 0;
 	u32 val;
-	u8 value, rx_status;
 
-	/* Slave RX byte receive */
-	if (status & BIT(IS_S_RX_EVENT_SHIFT)) {
+	while (rx_bytes < MAX_SLAVE_RX_PER_INT) {
 		val = iproc_i2c_rd_reg(iproc_i2c, S_RX_OFFSET);
 		rx_status = (val >> S_RX_STATUS_SHIFT) & S_RX_STATUS_MASK;
-		if (rx_status == I2C_SLAVE_RX_START) {
-			/* Start of SMBUS for Master write */
-			i2c_slave_event(iproc_i2c->slave,
-					I2C_SLAVE_WRITE_REQUESTED, &value);
+		rx_data = ((val >> S_RX_DATA_SHIFT) & S_RX_DATA_MASK);
 
-			val = iproc_i2c_rd_reg(iproc_i2c, S_RX_OFFSET);
-			value = (u8)((val >> S_RX_DATA_SHIFT) & S_RX_DATA_MASK);
+		if (rx_status == I2C_SLAVE_RX_START) {
+			/* Start of SMBUS Master write */
 			i2c_slave_event(iproc_i2c->slave,
-					I2C_SLAVE_WRITE_RECEIVED, &value);
-		} else if (status & BIT(IS_S_RD_EVENT_SHIFT)) {
-			/* Start of SMBUS for Master Read */
+					I2C_SLAVE_WRITE_REQUESTED, &rx_data);
+			iproc_i2c->rx_start_rcvd = true;
+			iproc_i2c->slave_read_complete = false;
+		} else if (rx_status == I2C_SLAVE_RX_DATA &&
+			   iproc_i2c->rx_start_rcvd) {
+			/* Middle of SMBUS Master write */
 			i2c_slave_event(iproc_i2c->slave,
-					I2C_SLAVE_READ_REQUESTED, &value);
-			iproc_i2c_wr_reg(iproc_i2c, S_TX_OFFSET, value);
+					I2C_SLAVE_WRITE_RECEIVED, &rx_data);
+		} else if (rx_status == I2C_SLAVE_RX_END &&
+			   iproc_i2c->rx_start_rcvd) {
+			/* End of SMBUS Master write */
+			if (iproc_i2c->slave_rx_only)
+				i2c_slave_event(iproc_i2c->slave,
+						I2C_SLAVE_WRITE_RECEIVED,
+						&rx_data);
+
+			i2c_slave_event(iproc_i2c->slave, I2C_SLAVE_STOP,
+					&rx_data);
+		} else if (rx_status == I2C_SLAVE_RX_FIFO_EMPTY) {
+			iproc_i2c->rx_start_rcvd = false;
+			iproc_i2c->slave_read_complete = true;
+			break;
+		}
 
-			val = BIT(S_CMD_START_BUSY_SHIFT);
-			iproc_i2c_wr_reg(iproc_i2c, S_CMD_OFFSET, val);
+		rx_bytes++;
+	}
+}
 
-			/*
-			 * Enable interrupt for TX FIFO becomes empty and
-			 * less than PKT_LENGTH bytes were output on the SMBUS
-			 */
-			val = iproc_i2c_rd_reg(iproc_i2c, IE_OFFSET);
-			val |= BIT(IE_S_TX_UNDERRUN_SHIFT);
-			iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, val);
-		} else {
-			/* Master write other than start */
-			value = (u8)((val >> S_RX_DATA_SHIFT) & S_RX_DATA_MASK);
+static void slave_rx_tasklet_fn(unsigned long data)
+{
+	struct bcm_iproc_i2c_dev *iproc_i2c = (struct bcm_iproc_i2c_dev *)data;
+	u32 int_clr;
+
+	bcm_iproc_i2c_slave_read(iproc_i2c);
+
+	/* clear pending IS_S_RX_EVENT_SHIFT interrupt */
+	int_clr = BIT(IS_S_RX_EVENT_SHIFT);
+
+	if (!iproc_i2c->slave_rx_only && iproc_i2c->slave_read_complete) {
+		/*
+		 * In case of single byte master-read request,
+		 * IS_S_TX_UNDERRUN_SHIFT event is generated before
+		 * IS_S_START_BUSY_SHIFT event. Hence start slave data send
+		 * from first IS_S_TX_UNDERRUN_SHIFT event.
+		 *
+		 * This means don't send any data from slave when
+		 * IS_S_RD_EVENT_SHIFT event is generated else it will increment
+		 * eeprom or other backend slave driver read pointer twice.
+		 */
+		iproc_i2c->tx_underrun = 0;
+		iproc_i2c->slave_int_mask |= BIT(IE_S_TX_UNDERRUN_SHIFT);
+
+		/* clear IS_S_RD_EVENT_SHIFT interrupt */
+		int_clr |= BIT(IS_S_RD_EVENT_SHIFT);
+	}
+
+	/* clear slave interrupt */
+	iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET, int_clr);
+	/* enable slave interrupts */
+	iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, iproc_i2c->slave_int_mask);
+}
+
+static bool bcm_iproc_i2c_slave_isr(struct bcm_iproc_i2c_dev *iproc_i2c,
+				    u32 status)
+{
+	u32 val;
+	u8 value;
+
+	/*
+	 * Slave events in case of master-write, master-write-read and,
+	 * master-read
+	 *
+	 * Master-write     : only IS_S_RX_EVENT_SHIFT event
+	 * Master-write-read: both IS_S_RX_EVENT_SHIFT and IS_S_RD_EVENT_SHIFT
+	 *                    events
+	 * Master-read      : both IS_S_RX_EVENT_SHIFT and IS_S_RD_EVENT_SHIFT
+	 *                    events or only IS_S_RD_EVENT_SHIFT
+	 */
+	if (status & BIT(IS_S_RX_EVENT_SHIFT) ||
+	    status & BIT(IS_S_RD_EVENT_SHIFT)) {
+		/* disable slave interrupts */
+		val = iproc_i2c_rd_reg(iproc_i2c, IE_OFFSET);
+		val &= ~iproc_i2c->slave_int_mask;
+		iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, val);
+
+		if (status & BIT(IS_S_RD_EVENT_SHIFT))
+			/* Master-write-read request */
+			iproc_i2c->slave_rx_only = false;
+		else
+			/* Master-write request only */
+			iproc_i2c->slave_rx_only = true;
+
+		/* schedule tasklet to read data later */
+		tasklet_schedule(&iproc_i2c->slave_rx_tasklet);
+
+		/* clear only IS_S_RX_EVENT_SHIFT interrupt */
+		iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET,
+				 BIT(IS_S_RX_EVENT_SHIFT));
+	}
+
+	if (status & BIT(IS_S_TX_UNDERRUN_SHIFT)) {
+		iproc_i2c->tx_underrun++;
+		if (iproc_i2c->tx_underrun == 1)
+			/* Start of SMBUS for Master Read */
 			i2c_slave_event(iproc_i2c->slave,
-					I2C_SLAVE_WRITE_RECEIVED, &value);
-			if (rx_status == I2C_SLAVE_RX_END)
-				i2c_slave_event(iproc_i2c->slave,
-						I2C_SLAVE_STOP, &value);
-		}
-	} else if (status & BIT(IS_S_TX_UNDERRUN_SHIFT)) {
-		/* Master read other than start */
-		i2c_slave_event(iproc_i2c->slave,
-				I2C_SLAVE_READ_PROCESSED, &value);
+					I2C_SLAVE_READ_REQUESTED,
+					&value);
+		else
+			/* Master read other than start */
+			i2c_slave_event(iproc_i2c->slave,
+					I2C_SLAVE_READ_PROCESSED,
+					&value);
 
 		iproc_i2c_wr_reg(iproc_i2c, S_TX_OFFSET, value);
+		/* start transfer */
 		val = BIT(S_CMD_START_BUSY_SHIFT);
 		iproc_i2c_wr_reg(iproc_i2c, S_CMD_OFFSET, val);
+
+		/* clear interrupt */
+		iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET,
+				 BIT(IS_S_TX_UNDERRUN_SHIFT));
 	}
 
-	/* Stop */
+	/* Stop received from master in case of master read transaction */
 	if (status & BIT(IS_S_START_BUSY_SHIFT)) {
-		i2c_slave_event(iproc_i2c->slave, I2C_SLAVE_STOP, &value);
 		/*
 		 * Disable interrupt for TX FIFO becomes empty and
 		 * less than PKT_LENGTH bytes were output on the SMBUS
 		 */
-		val = iproc_i2c_rd_reg(iproc_i2c, IE_OFFSET);
-		val &= ~BIT(IE_S_TX_UNDERRUN_SHIFT);
-		iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, val);
+		iproc_i2c->slave_int_mask &= ~BIT(IE_S_TX_UNDERRUN_SHIFT);
+		iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET,
+				 iproc_i2c->slave_int_mask);
+
+		/* End of SMBUS for Master Read */
+		val = BIT(S_TX_WR_STATUS_SHIFT);
+		iproc_i2c_wr_reg(iproc_i2c, S_TX_OFFSET, val);
+
+		val = BIT(S_CMD_START_BUSY_SHIFT);
+		iproc_i2c_wr_reg(iproc_i2c, S_CMD_OFFSET, val);
+
+		/* flush TX FIFOs */
+		val = iproc_i2c_rd_reg(iproc_i2c, S_FIFO_CTRL_OFFSET);
+		val |= (BIT(S_FIFO_TX_FLUSH_SHIFT));
+		iproc_i2c_wr_reg(iproc_i2c, S_FIFO_CTRL_OFFSET, val);
+
+		i2c_slave_event(iproc_i2c->slave, I2C_SLAVE_STOP, &value);
+
+		/* clear interrupt */
+		iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET,
+				 BIT(IS_S_START_BUSY_SHIFT));
 	}
 
-	/* clear interrupt status */
-	iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET, status);
+	/* check slave transmit status only if slave is transmitting */
+	if (!iproc_i2c->slave_rx_only)
+		bcm_iproc_i2c_check_slave_status(iproc_i2c);
 
-	bcm_iproc_i2c_check_slave_status(iproc_i2c);
 	return true;
 }
 
@@ -1074,6 +1193,10 @@ static int bcm_iproc_i2c_reg_slave(struct i2c_client *slave)
 		return -EAFNOSUPPORT;
 
 	iproc_i2c->slave = slave;
+
+	tasklet_init(&iproc_i2c->slave_rx_tasklet, slave_rx_tasklet_fn,
+		     (unsigned long)iproc_i2c);
+
 	bcm_iproc_i2c_slave_init(iproc_i2c, false);
 	return 0;
 }
@@ -1094,6 +1217,8 @@ static int bcm_iproc_i2c_unreg_slave(struct i2c_client *slave)
 			IE_S_ALL_INTERRUPT_SHIFT);
 	iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, tmp);
 
+	tasklet_kill(&iproc_i2c->slave_rx_tasklet);
+
 	/* Erase the slave address programmed */
 	tmp = iproc_i2c_rd_reg(iproc_i2c, S_CFG_SMBUS_ADDR_OFFSET);
 	tmp &= ~BIT(S_CFG_EN_NIC_SMB_ADDR3_SHIFT);
-- 
2.17.1


[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4187 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

[PATCH v3 6/6] i2c: iproc: handle rx fifo full interrupt

[Rayagonda Kokatanur]

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

Add code to handle IS_S_RX_FIFO_FULL_SHIFT interrupt to support
master write request with >= 64 bytes.

Iproc has a slave rx fifo size of 64 bytes.
Rx fifo full interrupt (IS_S_RX_FIFO_FULL_SHIFT) will be generated
when RX fifo becomes full. This can happen if master issues write
request of more than 64 bytes.

Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
---
 drivers/i2c/busses/i2c-bcm-iproc.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index 22e04055b447..cceaf69279a9 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -313,6 +313,8 @@ static void bcm_iproc_i2c_slave_init(
 
 	/* Enable interrupt register to indicate a valid byte in receive fifo */
 	val = BIT(IE_S_RX_EVENT_SHIFT);
+	/* Enable interrupt register to indicate Slave Rx FIFO Full */
+	val |= BIT(IE_S_RX_FIFO_FULL_SHIFT);
 	/* Enable interrupt register to indicate a Master read transaction */
 	val |= BIT(IE_S_RD_EVENT_SHIFT);
 	/* Enable interrupt register for the Slave BUSY command */
@@ -434,9 +436,15 @@ static bool bcm_iproc_i2c_slave_isr(struct bcm_iproc_i2c_dev *iproc_i2c,
 	 *                    events
 	 * Master-read      : both IS_S_RX_EVENT_SHIFT and IS_S_RD_EVENT_SHIFT
 	 *                    events or only IS_S_RD_EVENT_SHIFT
+	 *
+	 * iproc has a slave rx fifo size of 64 bytes. Rx fifo full interrupt
+	 * (IS_S_RX_FIFO_FULL_SHIFT) will be generated when RX fifo becomes
+	 * full. This can happen if Master issues write requests of more than
+	 * 64 bytes.
 	 */
 	if (status & BIT(IS_S_RX_EVENT_SHIFT) ||
-	    status & BIT(IS_S_RD_EVENT_SHIFT)) {
+	    status & BIT(IS_S_RD_EVENT_SHIFT) ||
+	    status & BIT(IS_S_RX_FIFO_FULL_SHIFT)) {
 		/* disable slave interrupts */
 		val = iproc_i2c_rd_reg(iproc_i2c, IE_OFFSET);
 		val &= ~iproc_i2c->slave_int_mask;
@@ -452,9 +460,14 @@ static bool bcm_iproc_i2c_slave_isr(struct bcm_iproc_i2c_dev *iproc_i2c,
 		/* schedule tasklet to read data later */
 		tasklet_schedule(&iproc_i2c->slave_rx_tasklet);
 
-		/* clear only IS_S_RX_EVENT_SHIFT interrupt */
-		iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET,
-				 BIT(IS_S_RX_EVENT_SHIFT));
+		/*
+		 * clear only IS_S_RX_EVENT_SHIFT and
+		 * IS_S_RX_FIFO_FULL_SHIFT interrupt.
+		 */
+		val = BIT(IS_S_RX_EVENT_SHIFT);
+		if (status & BIT(IS_S_RX_FIFO_FULL_SHIFT))
+			val |= BIT(IS_S_RX_FIFO_FULL_SHIFT);
+		iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET, val);
 	}
 
 	if (status & BIT(IS_S_TX_UNDERRUN_SHIFT)) {
-- 
2.17.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4187 bytes --]

[PATCH v3 6/6] i2c: iproc: handle rx fifo full interrupt

[Rayagonda Kokatanur]

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

Add code to handle IS_S_RX_FIFO_FULL_SHIFT interrupt to support
master write request with >= 64 bytes.

Iproc has a slave rx fifo size of 64 bytes.
Rx fifo full interrupt (IS_S_RX_FIFO_FULL_SHIFT) will be generated
when RX fifo becomes full. This can happen if master issues write
request of more than 64 bytes.

Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
---
 drivers/i2c/busses/i2c-bcm-iproc.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index 22e04055b447..cceaf69279a9 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -313,6 +313,8 @@ static void bcm_iproc_i2c_slave_init(
 
 	/* Enable interrupt register to indicate a valid byte in receive fifo */
 	val = BIT(IE_S_RX_EVENT_SHIFT);
+	/* Enable interrupt register to indicate Slave Rx FIFO Full */
+	val |= BIT(IE_S_RX_FIFO_FULL_SHIFT);
 	/* Enable interrupt register to indicate a Master read transaction */
 	val |= BIT(IE_S_RD_EVENT_SHIFT);
 	/* Enable interrupt register for the Slave BUSY command */
@@ -434,9 +436,15 @@ static bool bcm_iproc_i2c_slave_isr(struct bcm_iproc_i2c_dev *iproc_i2c,
 	 *                    events
 	 * Master-read      : both IS_S_RX_EVENT_SHIFT and IS_S_RD_EVENT_SHIFT
 	 *                    events or only IS_S_RD_EVENT_SHIFT
+	 *
+	 * iproc has a slave rx fifo size of 64 bytes. Rx fifo full interrupt
+	 * (IS_S_RX_FIFO_FULL_SHIFT) will be generated when RX fifo becomes
+	 * full. This can happen if Master issues write requests of more than
+	 * 64 bytes.
 	 */
 	if (status & BIT(IS_S_RX_EVENT_SHIFT) ||
-	    status & BIT(IS_S_RD_EVENT_SHIFT)) {
+	    status & BIT(IS_S_RD_EVENT_SHIFT) ||
+	    status & BIT(IS_S_RX_FIFO_FULL_SHIFT)) {
 		/* disable slave interrupts */
 		val = iproc_i2c_rd_reg(iproc_i2c, IE_OFFSET);
 		val &= ~iproc_i2c->slave_int_mask;
@@ -452,9 +460,14 @@ static bool bcm_iproc_i2c_slave_isr(struct bcm_iproc_i2c_dev *iproc_i2c,
 		/* schedule tasklet to read data later */
 		tasklet_schedule(&iproc_i2c->slave_rx_tasklet);
 
-		/* clear only IS_S_RX_EVENT_SHIFT interrupt */
-		iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET,
-				 BIT(IS_S_RX_EVENT_SHIFT));
+		/*
+		 * clear only IS_S_RX_EVENT_SHIFT and
+		 * IS_S_RX_FIFO_FULL_SHIFT interrupt.
+		 */
+		val = BIT(IS_S_RX_EVENT_SHIFT);
+		if (status & BIT(IS_S_RX_FIFO_FULL_SHIFT))
+			val |= BIT(IS_S_RX_FIFO_FULL_SHIFT);
+		iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET, val);
 	}
 
 	if (status & BIT(IS_S_TX_UNDERRUN_SHIFT)) {
-- 
2.17.1


[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4187 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

[PATCH v3 5/6] i2c: iproc: handle master read request

[Dhananjay Phadke]

On Mon,  2 Nov 2020 09:24:32 +0530, Rayagonda Kokatanur wrote:

> Handle single or multi byte master read request with or without
> repeated start.
> 
> Fixes: c245d94ed106 ("i2c: iproc: Add multi byte read-write support for slave mode")
> Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
> ---
>  drivers/i2c/busses/i2c-bcm-iproc.c | 215 +++++++++++++++++++++++------
>  1 file changed, 170 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
> index 7a235f9f5884..22e04055b447 100644
> --- a/drivers/i2c/busses/i2c-bcm-iproc.c
> +++ b/drivers/i2c/busses/i2c-bcm-iproc.c
> @@ -160,6 +160,11 @@
>  
>  #define IE_S_ALL_INTERRUPT_SHIFT     21
>  #define IE_S_ALL_INTERRUPT_MASK      0x3f
> +/*
> + * It takes ~18us to reading 10bytes of data, hence to keep tasklet
> + * running for less time, max slave read per tasklet is set to 10 bytes.
> + */
> +#define MAX_SLAVE_RX_PER_INT         10
>  

In patch [3/6], you've enabled IS_S_RX_THLD_SHIFT in slave ISR bitmask,
however it's not actually used in processing rx events.

Instead of hardcoding this threshold here, it's better to add a
device-tree knob for rx threshold, program it in controller and handle
that RX_THLD interrupt. This will give more flexibility to drain the rx
fifo earlier than -
(1) waiting for FIFO_FULL interrupt for transactions > 64B.
(2) waiting for start of read transaction in case of master write-read.

Regards,
Dhananjay

[PATCH v3 5/6] i2c: iproc: handle master read request

[Dhananjay Phadke]

On Mon,  2 Nov 2020 09:24:32 +0530, Rayagonda Kokatanur wrote:

> Handle single or multi byte master read request with or without
> repeated start.
> 
> Fixes: c245d94ed106 ("i2c: iproc: Add multi byte read-write support for slave mode")
> Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
> ---
>  drivers/i2c/busses/i2c-bcm-iproc.c | 215 +++++++++++++++++++++++------
>  1 file changed, 170 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
> index 7a235f9f5884..22e04055b447 100644
> --- a/drivers/i2c/busses/i2c-bcm-iproc.c
> +++ b/drivers/i2c/busses/i2c-bcm-iproc.c
> @@ -160,6 +160,11 @@
>  
>  #define IE_S_ALL_INTERRUPT_SHIFT     21
>  #define IE_S_ALL_INTERRUPT_MASK      0x3f
> +/*
> + * It takes ~18us to reading 10bytes of data, hence to keep tasklet
> + * running for less time, max slave read per tasklet is set to 10 bytes.
> + */
> +#define MAX_SLAVE_RX_PER_INT         10
>  

In patch [3/6], you've enabled IS_S_RX_THLD_SHIFT in slave ISR bitmask,
however it's not actually used in processing rx events.

Instead of hardcoding this threshold here, it's better to add a
device-tree knob for rx threshold, program it in controller and handle
that RX_THLD interrupt. This will give more flexibility to drain the rx
fifo earlier than -
(1) waiting for FIFO_FULL interrupt for transactions > 64B.
(2) waiting for start of read transaction in case of master write-read.

Regards,
Dhananjay


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

Re: [PATCH v3 5/6] i2c: iproc: handle master read request

[Florian Fainelli]

On 11/2/2020 10:19 PM, Dhananjay Phadke wrote:
> On Mon,  2 Nov 2020 09:24:32 +0530, Rayagonda Kokatanur wrote:
> 
>> Handle single or multi byte master read request with or without
>> repeated start.
>>
>> Fixes: c245d94ed106 ("i2c: iproc: Add multi byte read-write support for slave mode")
>> Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
>> ---
>>  drivers/i2c/busses/i2c-bcm-iproc.c | 215 +++++++++++++++++++++++------
>>  1 file changed, 170 insertions(+), 45 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
>> index 7a235f9f5884..22e04055b447 100644
>> --- a/drivers/i2c/busses/i2c-bcm-iproc.c
>> +++ b/drivers/i2c/busses/i2c-bcm-iproc.c
>> @@ -160,6 +160,11 @@
>>  
>>  #define IE_S_ALL_INTERRUPT_SHIFT     21
>>  #define IE_S_ALL_INTERRUPT_MASK      0x3f
>> +/*
>> + * It takes ~18us to reading 10bytes of data, hence to keep tasklet
>> + * running for less time, max slave read per tasklet is set to 10 bytes.
>> + */
>> +#define MAX_SLAVE_RX_PER_INT         10
>>  
> 
> In patch [3/6], you've enabled IS_S_RX_THLD_SHIFT in slave ISR bitmask,
> however it's not actually used in processing rx events.
> 
> Instead of hardcoding this threshold here, it's better to add a
> device-tree knob for rx threshold, program it in controller and handle
> that RX_THLD interrupt. This will give more flexibility to drain the rx
> fifo earlier than -
> (1) waiting for FIFO_FULL interrupt for transactions > 64B.
> (2) waiting for start of read transaction in case of master write-read.

The Device Tree is really intended to describe the hardware FIFO size,
not watermarks, as those tend to be more of a policy/work load decision.
Maybe this is something that can be added as a module parameter, or
configurable via ioctl() at some point.
-- 
Florian

Re: [PATCH v3 5/6] i2c: iproc: handle master read request

[Florian Fainelli]

On 11/2/2020 10:19 PM, Dhananjay Phadke wrote:
> On Mon,  2 Nov 2020 09:24:32 +0530, Rayagonda Kokatanur wrote:
> 
>> Handle single or multi byte master read request with or without
>> repeated start.
>>
>> Fixes: c245d94ed106 ("i2c: iproc: Add multi byte read-write support for slave mode")
>> Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
>> ---
>>  drivers/i2c/busses/i2c-bcm-iproc.c | 215 +++++++++++++++++++++++------
>>  1 file changed, 170 insertions(+), 45 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
>> index 7a235f9f5884..22e04055b447 100644
>> --- a/drivers/i2c/busses/i2c-bcm-iproc.c
>> +++ b/drivers/i2c/busses/i2c-bcm-iproc.c
>> @@ -160,6 +160,11 @@
>>  
>>  #define IE_S_ALL_INTERRUPT_SHIFT     21
>>  #define IE_S_ALL_INTERRUPT_MASK      0x3f
>> +/*
>> + * It takes ~18us to reading 10bytes of data, hence to keep tasklet
>> + * running for less time, max slave read per tasklet is set to 10 bytes.
>> + */
>> +#define MAX_SLAVE_RX_PER_INT         10
>>  
> 
> In patch [3/6], you've enabled IS_S_RX_THLD_SHIFT in slave ISR bitmask,
> however it's not actually used in processing rx events.
> 
> Instead of hardcoding this threshold here, it's better to add a
> device-tree knob for rx threshold, program it in controller and handle
> that RX_THLD interrupt. This will give more flexibility to drain the rx
> fifo earlier than -
> (1) waiting for FIFO_FULL interrupt for transactions > 64B.
> (2) waiting for start of read transaction in case of master write-read.

The Device Tree is really intended to describe the hardware FIFO size,
not watermarks, as those tend to be more of a policy/work load decision.
Maybe this is something that can be added as a module parameter, or
configurable via ioctl() at some point.
-- 
Florian

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

Re: [PATCH v3 5/6] i2c: iproc: handle master read request

[Rayagonda Kokatanur]

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

On Wed, Nov 4, 2020 at 9:05 AM Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>
>
> On 11/2/2020 10:19 PM, Dhananjay Phadke wrote:
> > On Mon,  2 Nov 2020 09:24:32 +0530, Rayagonda Kokatanur wrote:
> >
> >> Handle single or multi byte master read request with or without
> >> repeated start.
> >>
> >> Fixes: c245d94ed106 ("i2c: iproc: Add multi byte read-write support for slave mode")
> >> Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
> >> ---
> >>  drivers/i2c/busses/i2c-bcm-iproc.c | 215 +++++++++++++++++++++++------
> >>  1 file changed, 170 insertions(+), 45 deletions(-)
> >>
> >> diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
> >> index 7a235f9f5884..22e04055b447 100644
> >> --- a/drivers/i2c/busses/i2c-bcm-iproc.c
> >> +++ b/drivers/i2c/busses/i2c-bcm-iproc.c
> >> @@ -160,6 +160,11 @@
> >>
> >>  #define IE_S_ALL_INTERRUPT_SHIFT     21
> >>  #define IE_S_ALL_INTERRUPT_MASK      0x3f
> >> +/*
> >> + * It takes ~18us to reading 10bytes of data, hence to keep tasklet
> >> + * running for less time, max slave read per tasklet is set to 10 bytes.
> >> + */
> >> +#define MAX_SLAVE_RX_PER_INT         10
> >>
> >
> > In patch [3/6], you've enabled IS_S_RX_THLD_SHIFT in slave ISR bitmask,
> > however it's not actually used in processing rx events.
> >
> > Instead of hardcoding this threshold here, it's better to add a
> > device-tree knob for rx threshold, program it in controller and handle
> > that RX_THLD interrupt. This will give more flexibility to drain the rx
> > fifo earlier than -
> > (1) waiting for FIFO_FULL interrupt for transactions > 64B.
> > (2) waiting for start of read transaction in case of master write-read.

Yes this is one way to implement.
But do you see any issue in batching 64 bytes at a time in case of
transaction > 64 Bytes.
I feel batching will be more efficient as it avoids more number of
interrupts and hence context switch.

>
> The Device Tree is really intended to describe the hardware FIFO size,
> not watermarks, as those tend to be more of a policy/work load decision.
> Maybe this is something that can be added as a module parameter, or
> configurable via ioctl() at some point.

#define MAX_SLAVE_RX_PER_INT         10

is not hw fifo threshold level, it is a kind of watermark for the
tasklet to process the max number of packets in single run.
The intention to add the macro is to make sure the tasklet does not
run more than 20us.
If we keep this as a module parameter or dt parameter then there is a
good possibility that the number can be set to higher value.
This will make the tasklet run more than 20us and defeat the purpose.

This number is constant and not variable to change

Please feel free to add your comments.

Best regards,
Rayagonda

> --
> Florian

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4187 bytes --]

Re: [PATCH v3 5/6] i2c: iproc: handle master read request

[Rayagonda Kokatanur]

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

On Wed, Nov 4, 2020 at 9:05 AM Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>
>
> On 11/2/2020 10:19 PM, Dhananjay Phadke wrote:
> > On Mon,  2 Nov 2020 09:24:32 +0530, Rayagonda Kokatanur wrote:
> >
> >> Handle single or multi byte master read request with or without
> >> repeated start.
> >>
> >> Fixes: c245d94ed106 ("i2c: iproc: Add multi byte read-write support for slave mode")
> >> Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
> >> ---
> >>  drivers/i2c/busses/i2c-bcm-iproc.c | 215 +++++++++++++++++++++++------
> >>  1 file changed, 170 insertions(+), 45 deletions(-)
> >>
> >> diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
> >> index 7a235f9f5884..22e04055b447 100644
> >> --- a/drivers/i2c/busses/i2c-bcm-iproc.c
> >> +++ b/drivers/i2c/busses/i2c-bcm-iproc.c
> >> @@ -160,6 +160,11 @@
> >>
> >>  #define IE_S_ALL_INTERRUPT_SHIFT     21
> >>  #define IE_S_ALL_INTERRUPT_MASK      0x3f
> >> +/*
> >> + * It takes ~18us to reading 10bytes of data, hence to keep tasklet
> >> + * running for less time, max slave read per tasklet is set to 10 bytes.
> >> + */
> >> +#define MAX_SLAVE_RX_PER_INT         10
> >>
> >
> > In patch [3/6], you've enabled IS_S_RX_THLD_SHIFT in slave ISR bitmask,
> > however it's not actually used in processing rx events.
> >
> > Instead of hardcoding this threshold here, it's better to add a
> > device-tree knob for rx threshold, program it in controller and handle
> > that RX_THLD interrupt. This will give more flexibility to drain the rx
> > fifo earlier than -
> > (1) waiting for FIFO_FULL interrupt for transactions > 64B.
> > (2) waiting for start of read transaction in case of master write-read.

Yes this is one way to implement.
But do you see any issue in batching 64 bytes at a time in case of
transaction > 64 Bytes.
I feel batching will be more efficient as it avoids more number of
interrupts and hence context switch.

>
> The Device Tree is really intended to describe the hardware FIFO size,
> not watermarks, as those tend to be more of a policy/work load decision.
> Maybe this is something that can be added as a module parameter, or
> configurable via ioctl() at some point.

#define MAX_SLAVE_RX_PER_INT         10

is not hw fifo threshold level, it is a kind of watermark for the
tasklet to process the max number of packets in single run.
The intention to add the macro is to make sure the tasklet does not
run more than 20us.
If we keep this as a module parameter or dt parameter then there is a
good possibility that the number can be set to higher value.
This will make the tasklet run more than 20us and defeat the purpose.

This number is constant and not variable to change

Please feel free to add your comments.

Best regards,
Rayagonda

> --
> Florian

[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4187 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH v3 5/6] i2c: iproc: handle master read request

[Ray Jui]

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



On 11/3/2020 7:57 PM, Rayagonda Kokatanur wrote:
> On Wed, Nov 4, 2020 at 9:05 AM Florian Fainelli <f.fainelli@gmail.com> wrote:
>>
>>
>>
>> On 11/2/2020 10:19 PM, Dhananjay Phadke wrote:
>>> On Mon,  2 Nov 2020 09:24:32 +0530, Rayagonda Kokatanur wrote:
>>>
>>>> Handle single or multi byte master read request with or without
>>>> repeated start.
>>>>
>>>> Fixes: c245d94ed106 ("i2c: iproc: Add multi byte read-write support for slave mode")
>>>> Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
>>>> ---
>>>>  drivers/i2c/busses/i2c-bcm-iproc.c | 215 +++++++++++++++++++++++------
>>>>  1 file changed, 170 insertions(+), 45 deletions(-)
>>>>
>>>> diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
>>>> index 7a235f9f5884..22e04055b447 100644
>>>> --- a/drivers/i2c/busses/i2c-bcm-iproc.c
>>>> +++ b/drivers/i2c/busses/i2c-bcm-iproc.c
>>>> @@ -160,6 +160,11 @@
>>>>
>>>>  #define IE_S_ALL_INTERRUPT_SHIFT     21
>>>>  #define IE_S_ALL_INTERRUPT_MASK      0x3f
>>>> +/*
>>>> + * It takes ~18us to reading 10bytes of data, hence to keep tasklet
>>>> + * running for less time, max slave read per tasklet is set to 10 bytes.
>>>> + */
>>>> +#define MAX_SLAVE_RX_PER_INT         10
>>>>
>>>
>>> In patch [3/6], you've enabled IS_S_RX_THLD_SHIFT in slave ISR bitmask,
>>> however it's not actually used in processing rx events.
>>>
>>> Instead of hardcoding this threshold here, it's better to add a
>>> device-tree knob for rx threshold, program it in controller and handle
>>> that RX_THLD interrupt. This will give more flexibility to drain the rx
>>> fifo earlier than -
>>> (1) waiting for FIFO_FULL interrupt for transactions > 64B.
>>> (2) waiting for start of read transaction in case of master write-read.
> 
> Yes this is one way to implement.
> But do you see any issue in batching 64 bytes at a time in case of
> transaction > 64 Bytes.
> I feel batching will be more efficient as it avoids more number of
> interrupts and hence context switch.
> 
>>
>> The Device Tree is really intended to describe the hardware FIFO size,
>> not watermarks, as those tend to be more of a policy/work load decision.
>> Maybe this is something that can be added as a module parameter, or
>> configurable via ioctl() at some point.
> 

Yes, DT can have properties to describe the FIFO size, if there happens
to be some variants in the HW blocks in different versions. But that is
not the case here. DT should not be used to control SW/use case specific
behavior.


> #define MAX_SLAVE_RX_PER_INT         10
> 
> is not hw fifo threshold level, it is a kind of watermark for the
> tasklet to process the max number of packets in single run.
> The intention to add the macro is to make sure the tasklet does not
> run more than 20us.
> If we keep this as a module parameter or dt parameter then there is a
> good possibility that the number can be set to higher value.
> This will make the tasklet run more than 20us and defeat the purpose.
> 
> This number is constant and not variable to change
> 
> Please feel free to add your comments.
> 
> Best regards,
> Rayagonda
> 
>> --
>> Florian

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4151 bytes --]

Re: [PATCH v3 5/6] i2c: iproc: handle master read request

[Ray Jui]

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



On 11/3/2020 7:57 PM, Rayagonda Kokatanur wrote:
> On Wed, Nov 4, 2020 at 9:05 AM Florian Fainelli <f.fainelli@gmail.com> wrote:
>>
>>
>>
>> On 11/2/2020 10:19 PM, Dhananjay Phadke wrote:
>>> On Mon,  2 Nov 2020 09:24:32 +0530, Rayagonda Kokatanur wrote:
>>>
>>>> Handle single or multi byte master read request with or without
>>>> repeated start.
>>>>
>>>> Fixes: c245d94ed106 ("i2c: iproc: Add multi byte read-write support for slave mode")
>>>> Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
>>>> ---
>>>>  drivers/i2c/busses/i2c-bcm-iproc.c | 215 +++++++++++++++++++++++------
>>>>  1 file changed, 170 insertions(+), 45 deletions(-)
>>>>
>>>> diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
>>>> index 7a235f9f5884..22e04055b447 100644
>>>> --- a/drivers/i2c/busses/i2c-bcm-iproc.c
>>>> +++ b/drivers/i2c/busses/i2c-bcm-iproc.c
>>>> @@ -160,6 +160,11 @@
>>>>
>>>>  #define IE_S_ALL_INTERRUPT_SHIFT     21
>>>>  #define IE_S_ALL_INTERRUPT_MASK      0x3f
>>>> +/*
>>>> + * It takes ~18us to reading 10bytes of data, hence to keep tasklet
>>>> + * running for less time, max slave read per tasklet is set to 10 bytes.
>>>> + */
>>>> +#define MAX_SLAVE_RX_PER_INT         10
>>>>
>>>
>>> In patch [3/6], you've enabled IS_S_RX_THLD_SHIFT in slave ISR bitmask,
>>> however it's not actually used in processing rx events.
>>>
>>> Instead of hardcoding this threshold here, it's better to add a
>>> device-tree knob for rx threshold, program it in controller and handle
>>> that RX_THLD interrupt. This will give more flexibility to drain the rx
>>> fifo earlier than -
>>> (1) waiting for FIFO_FULL interrupt for transactions > 64B.
>>> (2) waiting for start of read transaction in case of master write-read.
> 
> Yes this is one way to implement.
> But do you see any issue in batching 64 bytes at a time in case of
> transaction > 64 Bytes.
> I feel batching will be more efficient as it avoids more number of
> interrupts and hence context switch.
> 
>>
>> The Device Tree is really intended to describe the hardware FIFO size,
>> not watermarks, as those tend to be more of a policy/work load decision.
>> Maybe this is something that can be added as a module parameter, or
>> configurable via ioctl() at some point.
> 

Yes, DT can have properties to describe the FIFO size, if there happens
to be some variants in the HW blocks in different versions. But that is
not the case here. DT should not be used to control SW/use case specific
behavior.


> #define MAX_SLAVE_RX_PER_INT         10
> 
> is not hw fifo threshold level, it is a kind of watermark for the
> tasklet to process the max number of packets in single run.
> The intention to add the macro is to make sure the tasklet does not
> run more than 20us.
> If we keep this as a module parameter or dt parameter then there is a
> good possibility that the number can be set to higher value.
> This will make the tasklet run more than 20us and defeat the purpose.
> 
> This number is constant and not variable to change
> 
> Please feel free to add your comments.
> 
> Best regards,
> Rayagonda
> 
>> --
>> Florian

[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4151 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH v3 5/6] i2c: iproc: handle master read request

[Dhananjay Phadke]

On Wed, 4 Nov 2020 10:01:06 -0800, Ray Jui wrote:

>>>> +#define MAX_SLAVE_RX_PER_INT         10
>>>>
>>>
>>>> In patch [3/6], you've enabled IS_S_RX_THLD_SHIFT in slave ISR bitmask,
>>>> however it's not actually used in processing rx events.
>>>>
>>>> Instead of hardcoding this threshold here, it's better to add a
>>>> device-tree knob for rx threshold, program it in controller and handle
>>>> that RX_THLD interrupt. This will give more flexibility to drain the rx
>>>> fifo earlier than -
>>>> (1) waiting for FIFO_FULL interrupt for transactions > 64B.
>>>> (2) waiting for start of read transaction in case of master write-read.
>> 
>> Yes this is one way to implement.
>> But do you see any issue in batching 64 bytes at a time in case of
>> transaction > 64 Bytes.
>> I feel batching will be more efficient as it avoids more number of
>> interrupts and hence context switch.
>> 
>>>
>>> The Device Tree is really intended to describe the hardware FIFO size,
>>> not watermarks, as those tend to be more of a policy/work load decision.
>>> Maybe this is something that can be added as a module parameter, or
>>> configurable via ioctl() at some point.
>> 
>
>Yes, DT can have properties to describe the FIFO size, if there happens
>to be some variants in the HW blocks in different versions. But that is
>not the case here. DT should not be used to control SW/use case specific
>behavior.

So the suggestion was to set HW threshold for rx fifo interrupt, not
really a SW property. By setting it in DT, makes it easier to
customize for target system, module param needs or ioctl makes it
dependent on userpsace to configure it.

The need for tasklet seems to arise from the fact that many bytes are
left in the fifo. If there's a common problem here, such tasklet would be
needed in i2c subsys rather than controller specific tweak, akin to
how networking uses NAPI or adding block transactions to the interface?

For master write-read event, it seems both IS_S_RD_EVENT_SHIFT and
IS_S_RX_EVENT_SHIFT are detected, which implies that core is late to
drain rx fifo i.e. write is complete and the read has started on the bus?


Thanks,
Dhananjay

Re: [PATCH v3 5/6] i2c: iproc: handle master read request

[Dhananjay Phadke]

On Wed, 4 Nov 2020 10:01:06 -0800, Ray Jui wrote:

>>>> +#define MAX_SLAVE_RX_PER_INT         10
>>>>
>>>
>>>> In patch [3/6], you've enabled IS_S_RX_THLD_SHIFT in slave ISR bitmask,
>>>> however it's not actually used in processing rx events.
>>>>
>>>> Instead of hardcoding this threshold here, it's better to add a
>>>> device-tree knob for rx threshold, program it in controller and handle
>>>> that RX_THLD interrupt. This will give more flexibility to drain the rx
>>>> fifo earlier than -
>>>> (1) waiting for FIFO_FULL interrupt for transactions > 64B.
>>>> (2) waiting for start of read transaction in case of master write-read.
>> 
>> Yes this is one way to implement.
>> But do you see any issue in batching 64 bytes at a time in case of
>> transaction > 64 Bytes.
>> I feel batching will be more efficient as it avoids more number of
>> interrupts and hence context switch.
>> 
>>>
>>> The Device Tree is really intended to describe the hardware FIFO size,
>>> not watermarks, as those tend to be more of a policy/work load decision.
>>> Maybe this is something that can be added as a module parameter, or
>>> configurable via ioctl() at some point.
>> 
>
>Yes, DT can have properties to describe the FIFO size, if there happens
>to be some variants in the HW blocks in different versions. But that is
>not the case here. DT should not be used to control SW/use case specific
>behavior.

So the suggestion was to set HW threshold for rx fifo interrupt, not
really a SW property. By setting it in DT, makes it easier to
customize for target system, module param needs or ioctl makes it
dependent on userpsace to configure it.

The need for tasklet seems to arise from the fact that many bytes are
left in the fifo. If there's a common problem here, such tasklet would be
needed in i2c subsys rather than controller specific tweak, akin to
how networking uses NAPI or adding block transactions to the interface?

For master write-read event, it seems both IS_S_RD_EVENT_SHIFT and
IS_S_RX_EVENT_SHIFT are detected, which implies that core is late to
drain rx fifo i.e. write is complete and the read has started on the bus?


Thanks,
Dhananjay



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

Re: [PATCH v3 5/6] i2c: iproc: handle master read request

[Rayagonda Kokatanur]

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

On Thu, Nov 5, 2020 at 1:16 PM Dhananjay Phadke
<dphadke@linux.microsoft.com> wrote:
>
> On Wed, 4 Nov 2020 10:01:06 -0800, Ray Jui wrote:
>
> >>>> +#define MAX_SLAVE_RX_PER_INT         10
> >>>>
> >>>
> >>>> In patch [3/6], you've enabled IS_S_RX_THLD_SHIFT in slave ISR bitmask,
> >>>> however it's not actually used in processing rx events.
> >>>>
> >>>> Instead of hardcoding this threshold here, it's better to add a
> >>>> device-tree knob for rx threshold, program it in controller and handle
> >>>> that RX_THLD interrupt. This will give more flexibility to drain the rx
> >>>> fifo earlier than -
> >>>> (1) waiting for FIFO_FULL interrupt for transactions > 64B.
> >>>> (2) waiting for start of read transaction in case of master write-read.
> >>
> >> Yes this is one way to implement.
> >> But do you see any issue in batching 64 bytes at a time in case of
> >> transaction > 64 Bytes.
> >> I feel batching will be more efficient as it avoids more number of
> >> interrupts and hence context switch.
> >>
> >>>
> >>> The Device Tree is really intended to describe the hardware FIFO size,
> >>> not watermarks, as those tend to be more of a policy/work load decision.
> >>> Maybe this is something that can be added as a module parameter, or
> >>> configurable via ioctl() at some point.
> >>
> >
> >Yes, DT can have properties to describe the FIFO size, if there happens
> >to be some variants in the HW blocks in different versions. But that is
> >not the case here. DT should not be used to control SW/use case specific
> >behavior.
>
> So the suggestion was to set HW threshold for rx fifo interrupt, not
> really a SW property. By setting it in DT, makes it easier to
> customize for target system, module param needs or ioctl makes it
> dependent on userpsace to configure it.
>
> The need for tasklet seems to arise from the fact that many bytes are
> left in the fifo. If there's a common problem here, such tasklet would be
> needed in i2c subsys rather than controller specific tweak, akin to
> how networking uses NAPI or adding block transactions to the interface?
>
> For master write-read event, it seems both IS_S_RD_EVENT_SHIFT and
> IS_S_RX_EVENT_SHIFT are detected, which implies that core is late to
> drain rx fifo i.e. write is complete and the read has started on the bus?

Yes it's true that for master write-read events both
IS_S_RD_EVENT_SHIFT and IS_S_RX_EVENT_SHIFT  are coming together.
So before the slave starts transmitting data to the master, it should
first read all data from rx-fifo i.e. complete master write and then
process master read.

To minimise interrupt overhead, we are batching 64bytes.
To keep isr running for less time, we are using a tasklet.
Again to keep the tasklet not running for more than 20u, we have set
max of 10 bytes data read from rx-fifo per tasklet run.

If we start processing everything in isr and using rx threshold
interrupt, then isr will run for a longer time and this may hog the
system.
For example, to process 10 bytes it takes 20us, to process 30 bytes it
takes 60us and so on.
So is it okay to run isr for so long ?

Keeping all this in mind we thought a tasklet would be a good option
and kept max of 10 bytes read per tasklet.

Please let me know if you still feel we should not use a tasklet and
don't batch 64 bytes.

Thanks
Rayagonda
>
>
> Thanks,
> Dhananjay
>
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4187 bytes --]

Re: [PATCH v3 5/6] i2c: iproc: handle master read request

[Rayagonda Kokatanur]

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

On Thu, Nov 5, 2020 at 1:16 PM Dhananjay Phadke
<dphadke@linux.microsoft.com> wrote:
>
> On Wed, 4 Nov 2020 10:01:06 -0800, Ray Jui wrote:
>
> >>>> +#define MAX_SLAVE_RX_PER_INT         10
> >>>>
> >>>
> >>>> In patch [3/6], you've enabled IS_S_RX_THLD_SHIFT in slave ISR bitmask,
> >>>> however it's not actually used in processing rx events.
> >>>>
> >>>> Instead of hardcoding this threshold here, it's better to add a
> >>>> device-tree knob for rx threshold, program it in controller and handle
> >>>> that RX_THLD interrupt. This will give more flexibility to drain the rx
> >>>> fifo earlier than -
> >>>> (1) waiting for FIFO_FULL interrupt for transactions > 64B.
> >>>> (2) waiting for start of read transaction in case of master write-read.
> >>
> >> Yes this is one way to implement.
> >> But do you see any issue in batching 64 bytes at a time in case of
> >> transaction > 64 Bytes.
> >> I feel batching will be more efficient as it avoids more number of
> >> interrupts and hence context switch.
> >>
> >>>
> >>> The Device Tree is really intended to describe the hardware FIFO size,
> >>> not watermarks, as those tend to be more of a policy/work load decision.
> >>> Maybe this is something that can be added as a module parameter, or
> >>> configurable via ioctl() at some point.
> >>
> >
> >Yes, DT can have properties to describe the FIFO size, if there happens
> >to be some variants in the HW blocks in different versions. But that is
> >not the case here. DT should not be used to control SW/use case specific
> >behavior.
>
> So the suggestion was to set HW threshold for rx fifo interrupt, not
> really a SW property. By setting it in DT, makes it easier to
> customize for target system, module param needs or ioctl makes it
> dependent on userpsace to configure it.
>
> The need for tasklet seems to arise from the fact that many bytes are
> left in the fifo. If there's a common problem here, such tasklet would be
> needed in i2c subsys rather than controller specific tweak, akin to
> how networking uses NAPI or adding block transactions to the interface?
>
> For master write-read event, it seems both IS_S_RD_EVENT_SHIFT and
> IS_S_RX_EVENT_SHIFT are detected, which implies that core is late to
> drain rx fifo i.e. write is complete and the read has started on the bus?

Yes it's true that for master write-read events both
IS_S_RD_EVENT_SHIFT and IS_S_RX_EVENT_SHIFT  are coming together.
So before the slave starts transmitting data to the master, it should
first read all data from rx-fifo i.e. complete master write and then
process master read.

To minimise interrupt overhead, we are batching 64bytes.
To keep isr running for less time, we are using a tasklet.
Again to keep the tasklet not running for more than 20u, we have set
max of 10 bytes data read from rx-fifo per tasklet run.

If we start processing everything in isr and using rx threshold
interrupt, then isr will run for a longer time and this may hog the
system.
For example, to process 10 bytes it takes 20us, to process 30 bytes it
takes 60us and so on.
So is it okay to run isr for so long ?

Keeping all this in mind we thought a tasklet would be a good option
and kept max of 10 bytes read per tasklet.

Please let me know if you still feel we should not use a tasklet and
don't batch 64 bytes.

Thanks
Rayagonda
>
>
> Thanks,
> Dhananjay
>
>

[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4187 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH v3 5/6] i2c: iproc: handle master read request

[Dhananjay Phadke]

On Thu, 5 Nov 2020 15:13:04 +0530, Rayagonda Kokatanur wrote:
>> So the suggestion was to set HW threshold for rx fifo interrupt, not
>> really a SW property. By setting it in DT, makes it easier to
>> customize for target system, module param needs or ioctl makes it
>> dependent on userpsace to configure it.
>>
>> The need for tasklet seems to arise from the fact that many bytes are
>> left in the fifo. If there's a common problem here, such tasklet would be
>> needed in i2c subsys rather than controller specific tweak, akin to
>> how networking uses NAPI or adding block transactions to the interface?
>>
>> For master write-read event, it seems both IS_S_RD_EVENT_SHIFT and
>> IS_S_RX_EVENT_SHIFT are detected, which implies that core is late to
>> drain rx fifo i.e. write is complete and the read has started on the bus?
>
>Yes it's true that for master write-read events both
>IS_S_RD_EVENT_SHIFT and IS_S_RX_EVENT_SHIFT  are coming together.
>So before the slave starts transmitting data to the master, it should
>first read all data from rx-fifo i.e. complete master write and then
>process master read.
>
>To minimise interrupt overhead, we are batching 64bytes.
>To keep isr running for less time, we are using a tasklet.
>Again to keep the tasklet not running for more than 20u, we have set
>max of 10 bytes data read from rx-fifo per tasklet run.
>
>If we start processing everything in isr and using rx threshold
>interrupt, then isr will run for a longer time and this may hog the
>system.
>For example, to process 10 bytes it takes 20us, to process 30 bytes it
>takes 60us and so on.
>So is it okay to run isr for so long ?
>
>Keeping all this in mind we thought a tasklet would be a good option
>and kept max of 10 bytes read per tasklet.
>
>Please let me know if you still feel we should not use a tasklet and
>don't batch 64 bytes.

Deferring to tasklet is OK, could use a kernel thread (i.e. threaded_irq)
as i2c rate is quite low.

But do enable rx_threshold and read out early. This will avoid fifo full
or master write-read situation where lot of bytes must be drained from rx
fifo before serving tx fifo (avoid tx underrun).

Best would have been setting up DMA into mem (some controllers seem capable).
In absence of that, it's a trade off: if rx intr threshold is low,
there will be more interrupts, but less time spent in each. Default could
still be 64B or no-thresh (allow override in dtb).

Few other comments -

>+		/* schedule tasklet to read data later */
>+		tasklet_schedule(&iproc_i2c->slave_rx_tasklet);
>+
>+		/* clear only IS_S_RX_EVENT_SHIFT interrupt */
>+		iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET,
>+				 BIT(IS_S_RX_EVENT_SHIFT));
>+	}

Why clearing one rx interrupt bit here after scheduling tasklet? Should all that
be done by tasklet? Also should just return after scheduling tasklet?

Thanks,
Dhananjay

Re: [PATCH v3 5/6] i2c: iproc: handle master read request

[Dhananjay Phadke]

On Thu, 5 Nov 2020 15:13:04 +0530, Rayagonda Kokatanur wrote:
>> So the suggestion was to set HW threshold for rx fifo interrupt, not
>> really a SW property. By setting it in DT, makes it easier to
>> customize for target system, module param needs or ioctl makes it
>> dependent on userpsace to configure it.
>>
>> The need for tasklet seems to arise from the fact that many bytes are
>> left in the fifo. If there's a common problem here, such tasklet would be
>> needed in i2c subsys rather than controller specific tweak, akin to
>> how networking uses NAPI or adding block transactions to the interface?
>>
>> For master write-read event, it seems both IS_S_RD_EVENT_SHIFT and
>> IS_S_RX_EVENT_SHIFT are detected, which implies that core is late to
>> drain rx fifo i.e. write is complete and the read has started on the bus?
>
>Yes it's true that for master write-read events both
>IS_S_RD_EVENT_SHIFT and IS_S_RX_EVENT_SHIFT  are coming together.
>So before the slave starts transmitting data to the master, it should
>first read all data from rx-fifo i.e. complete master write and then
>process master read.
>
>To minimise interrupt overhead, we are batching 64bytes.
>To keep isr running for less time, we are using a tasklet.
>Again to keep the tasklet not running for more than 20u, we have set
>max of 10 bytes data read from rx-fifo per tasklet run.
>
>If we start processing everything in isr and using rx threshold
>interrupt, then isr will run for a longer time and this may hog the
>system.
>For example, to process 10 bytes it takes 20us, to process 30 bytes it
>takes 60us and so on.
>So is it okay to run isr for so long ?
>
>Keeping all this in mind we thought a tasklet would be a good option
>and kept max of 10 bytes read per tasklet.
>
>Please let me know if you still feel we should not use a tasklet and
>don't batch 64 bytes.

Deferring to tasklet is OK, could use a kernel thread (i.e. threaded_irq)
as i2c rate is quite low.

But do enable rx_threshold and read out early. This will avoid fifo full
or master write-read situation where lot of bytes must be drained from rx
fifo before serving tx fifo (avoid tx underrun).

Best would have been setting up DMA into mem (some controllers seem capable).
In absence of that, it's a trade off: if rx intr threshold is low,
there will be more interrupts, but less time spent in each. Default could
still be 64B or no-thresh (allow override in dtb).

Few other comments -

>+		/* schedule tasklet to read data later */
>+		tasklet_schedule(&iproc_i2c->slave_rx_tasklet);
>+
>+		/* clear only IS_S_RX_EVENT_SHIFT interrupt */
>+		iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET,
>+				 BIT(IS_S_RX_EVENT_SHIFT));
>+	}

Why clearing one rx interrupt bit here after scheduling tasklet? Should all that
be done by tasklet? Also should just return after scheduling tasklet?

Thanks,
Dhananjay

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

Re: [PATCH v3 5/6] i2c: iproc: handle master read request

[Rayagonda Kokatanur]

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

Hi Ray,

Could you please check Dhananjay comments and update your thoughts.

On Fri, Nov 6, 2020 at 11:11 PM Dhananjay Phadke
<dphadke@linux.microsoft.com> wrote:
>
> On Thu, 5 Nov 2020 15:13:04 +0530, Rayagonda Kokatanur wrote:
> >> So the suggestion was to set HW threshold for rx fifo interrupt, not
> >> really a SW property. By setting it in DT, makes it easier to
> >> customize for target system, module param needs or ioctl makes it
> >> dependent on userpsace to configure it.
> >>
> >> The need for tasklet seems to arise from the fact that many bytes are
> >> left in the fifo. If there's a common problem here, such tasklet would be
> >> needed in i2c subsys rather than controller specific tweak, akin to
> >> how networking uses NAPI or adding block transactions to the interface?
> >>
> >> For master write-read event, it seems both IS_S_RD_EVENT_SHIFT and
> >> IS_S_RX_EVENT_SHIFT are detected, which implies that core is late to
> >> drain rx fifo i.e. write is complete and the read has started on the bus?
> >
> >Yes it's true that for master write-read events both
> >IS_S_RD_EVENT_SHIFT and IS_S_RX_EVENT_SHIFT  are coming together.
> >So before the slave starts transmitting data to the master, it should
> >first read all data from rx-fifo i.e. complete master write and then
> >process master read.
> >
> >To minimise interrupt overhead, we are batching 64bytes.
> >To keep isr running for less time, we are using a tasklet.
> >Again to keep the tasklet not running for more than 20u, we have set
> >max of 10 bytes data read from rx-fifo per tasklet run.
> >
> >If we start processing everything in isr and using rx threshold
> >interrupt, then isr will run for a longer time and this may hog the
> >system.
> >For example, to process 10 bytes it takes 20us, to process 30 bytes it
> >takes 60us and so on.
> >So is it okay to run isr for so long ?
> >
> >Keeping all this in mind we thought a tasklet would be a good option
> >and kept max of 10 bytes read per tasklet.
> >
> >Please let me know if you still feel we should not use a tasklet and
> >don't batch 64 bytes.
>
> Deferring to tasklet is OK, could use a kernel thread (i.e. threaded_irq)
> as i2c rate is quite low.
>
> But do enable rx_threshold and read out early. This will avoid fifo full
> or master write-read situation where lot of bytes must be drained from rx
> fifo before serving tx fifo (avoid tx underrun).
>
> Best would have been setting up DMA into mem (some controllers seem capable).
> In absence of that, it's a trade off: if rx intr threshold is low,
> there will be more interrupts, but less time spent in each. Default could
> still be 64B or no-thresh (allow override in dtb).
>
> Few other comments -
>
> >+              /* schedule tasklet to read data later */
> >+              tasklet_schedule(&iproc_i2c->slave_rx_tasklet);
> >+
> >+              /* clear only IS_S_RX_EVENT_SHIFT interrupt */
> >+              iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET,
> >+                               BIT(IS_S_RX_EVENT_SHIFT));
> >+      }
>
> Why clearing one rx interrupt bit here after scheduling tasklet? Should all that
> be done by tasklet? Also should just return after scheduling tasklet?
>
> Thanks,
> Dhananjay

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4187 bytes --]

Re: [PATCH v3 5/6] i2c: iproc: handle master read request

[Rayagonda Kokatanur]

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

Hi Ray,

Could you please check Dhananjay comments and update your thoughts.

On Fri, Nov 6, 2020 at 11:11 PM Dhananjay Phadke
<dphadke@linux.microsoft.com> wrote:
>
> On Thu, 5 Nov 2020 15:13:04 +0530, Rayagonda Kokatanur wrote:
> >> So the suggestion was to set HW threshold for rx fifo interrupt, not
> >> really a SW property. By setting it in DT, makes it easier to
> >> customize for target system, module param needs or ioctl makes it
> >> dependent on userpsace to configure it.
> >>
> >> The need for tasklet seems to arise from the fact that many bytes are
> >> left in the fifo. If there's a common problem here, such tasklet would be
> >> needed in i2c subsys rather than controller specific tweak, akin to
> >> how networking uses NAPI or adding block transactions to the interface?
> >>
> >> For master write-read event, it seems both IS_S_RD_EVENT_SHIFT and
> >> IS_S_RX_EVENT_SHIFT are detected, which implies that core is late to
> >> drain rx fifo i.e. write is complete and the read has started on the bus?
> >
> >Yes it's true that for master write-read events both
> >IS_S_RD_EVENT_SHIFT and IS_S_RX_EVENT_SHIFT  are coming together.
> >So before the slave starts transmitting data to the master, it should
> >first read all data from rx-fifo i.e. complete master write and then
> >process master read.
> >
> >To minimise interrupt overhead, we are batching 64bytes.
> >To keep isr running for less time, we are using a tasklet.
> >Again to keep the tasklet not running for more than 20u, we have set
> >max of 10 bytes data read from rx-fifo per tasklet run.
> >
> >If we start processing everything in isr and using rx threshold
> >interrupt, then isr will run for a longer time and this may hog the
> >system.
> >For example, to process 10 bytes it takes 20us, to process 30 bytes it
> >takes 60us and so on.
> >So is it okay to run isr for so long ?
> >
> >Keeping all this in mind we thought a tasklet would be a good option
> >and kept max of 10 bytes read per tasklet.
> >
> >Please let me know if you still feel we should not use a tasklet and
> >don't batch 64 bytes.
>
> Deferring to tasklet is OK, could use a kernel thread (i.e. threaded_irq)
> as i2c rate is quite low.
>
> But do enable rx_threshold and read out early. This will avoid fifo full
> or master write-read situation where lot of bytes must be drained from rx
> fifo before serving tx fifo (avoid tx underrun).
>
> Best would have been setting up DMA into mem (some controllers seem capable).
> In absence of that, it's a trade off: if rx intr threshold is low,
> there will be more interrupts, but less time spent in each. Default could
> still be 64B or no-thresh (allow override in dtb).
>
> Few other comments -
>
> >+              /* schedule tasklet to read data later */
> >+              tasklet_schedule(&iproc_i2c->slave_rx_tasklet);
> >+
> >+              /* clear only IS_S_RX_EVENT_SHIFT interrupt */
> >+              iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET,
> >+                               BIT(IS_S_RX_EVENT_SHIFT));
> >+      }
>
> Why clearing one rx interrupt bit here after scheduling tasklet? Should all that
> be done by tasklet? Also should just return after scheduling tasklet?
>
> Thanks,
> Dhananjay

[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4187 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH v3 5/6] i2c: iproc: handle master read request

[Ray Jui]

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



On 11/9/2020 8:23 PM, Rayagonda Kokatanur wrote:
> Hi Ray,
> 
> Could you please check Dhananjay comments and update your thoughts.
> 
> On Fri, Nov 6, 2020 at 11:11 PM Dhananjay Phadke
> <dphadke@linux.microsoft.com> wrote:
>>
>> On Thu, 5 Nov 2020 15:13:04 +0530, Rayagonda Kokatanur wrote:
>>>> So the suggestion was to set HW threshold for rx fifo interrupt, not
>>>> really a SW property. By setting it in DT, makes it easier to
>>>> customize for target system, module param needs or ioctl makes it
>>>> dependent on userpsace to configure it.
>>>>
>>>> The need for tasklet seems to arise from the fact that many bytes are
>>>> left in the fifo. If there's a common problem here, such tasklet would be
>>>> needed in i2c subsys rather than controller specific tweak, akin to
>>>> how networking uses NAPI or adding block transactions to the interface?
>>>>
>>>> For master write-read event, it seems both IS_S_RD_EVENT_SHIFT and
>>>> IS_S_RX_EVENT_SHIFT are detected, which implies that core is late to
>>>> drain rx fifo i.e. write is complete and the read has started on the bus?
>>>
>>> Yes it's true that for master write-read events both
>>> IS_S_RD_EVENT_SHIFT and IS_S_RX_EVENT_SHIFT  are coming together.
>>> So before the slave starts transmitting data to the master, it should
>>> first read all data from rx-fifo i.e. complete master write and then
>>> process master read.
>>>
>>> To minimise interrupt overhead, we are batching 64bytes.
>>> To keep isr running for less time, we are using a tasklet.
>>> Again to keep the tasklet not running for more than 20u, we have set
>>> max of 10 bytes data read from rx-fifo per tasklet run.
>>>
>>> If we start processing everything in isr and using rx threshold
>>> interrupt, then isr will run for a longer time and this may hog the
>>> system.
>>> For example, to process 10 bytes it takes 20us, to process 30 bytes it
>>> takes 60us and so on.
>>> So is it okay to run isr for so long ?
>>>
>>> Keeping all this in mind we thought a tasklet would be a good option
>>> and kept max of 10 bytes read per tasklet.
>>>
>>> Please let me know if you still feel we should not use a tasklet and
>>> don't batch 64 bytes.
>>
>> Deferring to tasklet is OK, could use a kernel thread (i.e. threaded_irq)
>> as i2c rate is quite low.
>>

kernel thread was proposed in the internal review. I don't see much
benefit of using tasklet. If a thread is blocked from running for more
than several tenth of ms, that's really a system-level issue than an
issue with this driver.

IMO, it's an overkill to use tasklet here but we can probably leave it
as it is since it does not have a adverse effect and the code ran in
tasklet is short.

>> But do enable rx_threshold and read out early. This will avoid fifo full
>> or master write-read situation where lot of bytes must be drained from rx
>> fifo before serving tx fifo (avoid tx underrun).
>>
>> Best would have been setting up DMA into mem (some controllers seem capable).
>> In absence of that, it's a trade off: if rx intr threshold is low,
>> there will be more interrupts, but less time spent in each. Default could
>> still be 64B or no-thresh (allow override in dtb).

How much time is expected to read 64 bytes from an RX FIFO? Even with
APB bus each register read is expected to be in the tenth or hundreds of
nanosecond range. Reading the entire FIFO of 64 bytes should take less
than 10 us. The interrupt context switch overhead is probably longer
than that. It's much more effective to read all of them in a single
batch than breaking them into multiple batches.

Like Florian already suggested, DT property is meant to describe
variants in HW, it should not be used for this purpose. DT maintainer
Rob also mentioned this multiple times in other reviews.


>>
>> Few other comments -
>>
>>> +              /* schedule tasklet to read data later */
>>> +              tasklet_schedule(&iproc_i2c->slave_rx_tasklet);
>>> +
>>> +              /* clear only IS_S_RX_EVENT_SHIFT interrupt */
>>> +              iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET,
>>> +                               BIT(IS_S_RX_EVENT_SHIFT));
>>> +      }
>>
>> Why clearing one rx interrupt bit here after scheduling tasklet? Should all that
>> be done by tasklet? Also should just return after scheduling tasklet?
>>
>> Thanks,
>> Dhananjay

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4151 bytes --]

Re: [PATCH v3 5/6] i2c: iproc: handle master read request

[Ray Jui]

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



On 11/9/2020 8:23 PM, Rayagonda Kokatanur wrote:
> Hi Ray,
> 
> Could you please check Dhananjay comments and update your thoughts.
> 
> On Fri, Nov 6, 2020 at 11:11 PM Dhananjay Phadke
> <dphadke@linux.microsoft.com> wrote:
>>
>> On Thu, 5 Nov 2020 15:13:04 +0530, Rayagonda Kokatanur wrote:
>>>> So the suggestion was to set HW threshold for rx fifo interrupt, not
>>>> really a SW property. By setting it in DT, makes it easier to
>>>> customize for target system, module param needs or ioctl makes it
>>>> dependent on userpsace to configure it.
>>>>
>>>> The need for tasklet seems to arise from the fact that many bytes are
>>>> left in the fifo. If there's a common problem here, such tasklet would be
>>>> needed in i2c subsys rather than controller specific tweak, akin to
>>>> how networking uses NAPI or adding block transactions to the interface?
>>>>
>>>> For master write-read event, it seems both IS_S_RD_EVENT_SHIFT and
>>>> IS_S_RX_EVENT_SHIFT are detected, which implies that core is late to
>>>> drain rx fifo i.e. write is complete and the read has started on the bus?
>>>
>>> Yes it's true that for master write-read events both
>>> IS_S_RD_EVENT_SHIFT and IS_S_RX_EVENT_SHIFT  are coming together.
>>> So before the slave starts transmitting data to the master, it should
>>> first read all data from rx-fifo i.e. complete master write and then
>>> process master read.
>>>
>>> To minimise interrupt overhead, we are batching 64bytes.
>>> To keep isr running for less time, we are using a tasklet.
>>> Again to keep the tasklet not running for more than 20u, we have set
>>> max of 10 bytes data read from rx-fifo per tasklet run.
>>>
>>> If we start processing everything in isr and using rx threshold
>>> interrupt, then isr will run for a longer time and this may hog the
>>> system.
>>> For example, to process 10 bytes it takes 20us, to process 30 bytes it
>>> takes 60us and so on.
>>> So is it okay to run isr for so long ?
>>>
>>> Keeping all this in mind we thought a tasklet would be a good option
>>> and kept max of 10 bytes read per tasklet.
>>>
>>> Please let me know if you still feel we should not use a tasklet and
>>> don't batch 64 bytes.
>>
>> Deferring to tasklet is OK, could use a kernel thread (i.e. threaded_irq)
>> as i2c rate is quite low.
>>

kernel thread was proposed in the internal review. I don't see much
benefit of using tasklet. If a thread is blocked from running for more
than several tenth of ms, that's really a system-level issue than an
issue with this driver.

IMO, it's an overkill to use tasklet here but we can probably leave it
as it is since it does not have a adverse effect and the code ran in
tasklet is short.

>> But do enable rx_threshold and read out early. This will avoid fifo full
>> or master write-read situation where lot of bytes must be drained from rx
>> fifo before serving tx fifo (avoid tx underrun).
>>
>> Best would have been setting up DMA into mem (some controllers seem capable).
>> In absence of that, it's a trade off: if rx intr threshold is low,
>> there will be more interrupts, but less time spent in each. Default could
>> still be 64B or no-thresh (allow override in dtb).

How much time is expected to read 64 bytes from an RX FIFO? Even with
APB bus each register read is expected to be in the tenth or hundreds of
nanosecond range. Reading the entire FIFO of 64 bytes should take less
than 10 us. The interrupt context switch overhead is probably longer
than that. It's much more effective to read all of them in a single
batch than breaking them into multiple batches.

Like Florian already suggested, DT property is meant to describe
variants in HW, it should not be used for this purpose. DT maintainer
Rob also mentioned this multiple times in other reviews.


>>
>> Few other comments -
>>
>>> +              /* schedule tasklet to read data later */
>>> +              tasklet_schedule(&iproc_i2c->slave_rx_tasklet);
>>> +
>>> +              /* clear only IS_S_RX_EVENT_SHIFT interrupt */
>>> +              iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET,
>>> +                               BIT(IS_S_RX_EVENT_SHIFT));
>>> +      }
>>
>> Why clearing one rx interrupt bit here after scheduling tasklet? Should all that
>> be done by tasklet? Also should just return after scheduling tasklet?
>>
>> Thanks,
>> Dhananjay

[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4151 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH v3 5/6] i2c: iproc: handle master read request

[Dhananjay Phadke]

On Tue, 10 Nov 2020 11:24:36 -0800, Ray Jui wrote:

>>>> Yes it's true that for master write-read events both
>>>> IS_S_RD_EVENT_SHIFT and IS_S_RX_EVENT_SHIFT  are coming together.
>>>> So before the slave starts transmitting data to the master, it should
>>>> first read all data from rx-fifo i.e. complete master write and then
>>>> process master read.
>>>>
>>>> To minimise interrupt overhead, we are batching 64bytes.
>>>> To keep isr running for less time, we are using a tasklet.
>>>> Again to keep the tasklet not running for more than 20u, we have set
>>>> max of 10 bytes data read from rx-fifo per tasklet run.
>>>>
>>>> If we start processing everything in isr and using rx threshold
>>>> interrupt, then isr will run for a longer time and this may hog the
>>>> system.
>>>> For example, to process 10 bytes it takes 20us, to process 30 bytes it
>>>> takes 60us and so on.
>>>> So is it okay to run isr for so long ?
>>>>
>>>> Keeping all this in mind we thought a tasklet would be a good option
>>>> and kept max of 10 bytes read per tasklet.
>>>>
>>>> Please let me know if you still feel we should not use a tasklet and
>>>> don't batch 64 bytes.
>>>
>>> Deferring to tasklet is OK, could use a kernel thread (i.e. threaded_irq)
>>> as i2c rate is quite low.
>>>
>
>kernel thread was proposed in the internal review. I don't see much
>benefit of using tasklet. If a thread is blocked from running for more
>than several tenth of ms, that's really a system-level issue than an
>issue with this driver.
>
>IMO, it's an overkill to use tasklet here but we can probably leave it
>as it is since it does not have a adverse effect and the code ran in
>tasklet is short.
>
>How much time is expected to read 64 bytes from an RX FIFO? Even with
>APB bus each register read is expected to be in the tenth or hundreds of
>nanosecond range. Reading the entire FIFO of 64 bytes should take less
>than 10 us. The interrupt context switch overhead is probably longer
>than that. It's much more effective to read all of them in a single
>batch than breaking them into multiple batches.

OK, there's a general discussions towards removing tasklets, if this
fix works with threaded isr, strongly recommend that route.

This comment in the code suggested that register reads take long time to
drain 64 bytes.

>+/*
>+ * It takes ~18us to reading 10bytes of data, hence to keep tasklet
>+ * running for less time, max slave read per tasklet is set to 10
>bytes.

@Rayagonda, please take care of hand-off mentioned below, once the tasklet
is scheduled, isr should just return and clear status at the end of tasklet.

>>
>> Few other comments -
>>
>>> +              /* schedule tasklet to read data later */
>>> +              tasklet_schedule(&iproc_i2c->slave_rx_tasklet);
>>> +
>>> +              /* clear only IS_S_RX_EVENT_SHIFT interrupt */
>>> +              iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET,
>>> +                               BIT(IS_S_RX_EVENT_SHIFT));
>>> +      }
>>
>> Why clearing one rx interrupt bit here after scheduling tasklet? Should all that
>> be done by tasklet? Also should just return after scheduling tasklet?

Regards,
Dhananjay

Re: [PATCH v3 5/6] i2c: iproc: handle master read request

[Dhananjay Phadke]

On Tue, 10 Nov 2020 11:24:36 -0800, Ray Jui wrote:

>>>> Yes it's true that for master write-read events both
>>>> IS_S_RD_EVENT_SHIFT and IS_S_RX_EVENT_SHIFT  are coming together.
>>>> So before the slave starts transmitting data to the master, it should
>>>> first read all data from rx-fifo i.e. complete master write and then
>>>> process master read.
>>>>
>>>> To minimise interrupt overhead, we are batching 64bytes.
>>>> To keep isr running for less time, we are using a tasklet.
>>>> Again to keep the tasklet not running for more than 20u, we have set
>>>> max of 10 bytes data read from rx-fifo per tasklet run.
>>>>
>>>> If we start processing everything in isr and using rx threshold
>>>> interrupt, then isr will run for a longer time and this may hog the
>>>> system.
>>>> For example, to process 10 bytes it takes 20us, to process 30 bytes it
>>>> takes 60us and so on.
>>>> So is it okay to run isr for so long ?
>>>>
>>>> Keeping all this in mind we thought a tasklet would be a good option
>>>> and kept max of 10 bytes read per tasklet.
>>>>
>>>> Please let me know if you still feel we should not use a tasklet and
>>>> don't batch 64 bytes.
>>>
>>> Deferring to tasklet is OK, could use a kernel thread (i.e. threaded_irq)
>>> as i2c rate is quite low.
>>>
>
>kernel thread was proposed in the internal review. I don't see much
>benefit of using tasklet. If a thread is blocked from running for more
>than several tenth of ms, that's really a system-level issue than an
>issue with this driver.
>
>IMO, it's an overkill to use tasklet here but we can probably leave it
>as it is since it does not have a adverse effect and the code ran in
>tasklet is short.
>
>How much time is expected to read 64 bytes from an RX FIFO? Even with
>APB bus each register read is expected to be in the tenth or hundreds of
>nanosecond range. Reading the entire FIFO of 64 bytes should take less
>than 10 us. The interrupt context switch overhead is probably longer
>than that. It's much more effective to read all of them in a single
>batch than breaking them into multiple batches.

OK, there's a general discussions towards removing tasklets, if this
fix works with threaded isr, strongly recommend that route.

This comment in the code suggested that register reads take long time to
drain 64 bytes.

>+/*
>+ * It takes ~18us to reading 10bytes of data, hence to keep tasklet
>+ * running for less time, max slave read per tasklet is set to 10
>bytes.

@Rayagonda, please take care of hand-off mentioned below, once the tasklet
is scheduled, isr should just return and clear status at the end of tasklet.

>>
>> Few other comments -
>>
>>> +              /* schedule tasklet to read data later */
>>> +              tasklet_schedule(&iproc_i2c->slave_rx_tasklet);
>>> +
>>> +              /* clear only IS_S_RX_EVENT_SHIFT interrupt */
>>> +              iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET,
>>> +                               BIT(IS_S_RX_EVENT_SHIFT));
>>> +      }
>>
>> Why clearing one rx interrupt bit here after scheduling tasklet? Should all that
>> be done by tasklet? Also should just return after scheduling tasklet?

Regards,
Dhananjay

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

Re: [PATCH v3 5/6] i2c: iproc: handle master read request

[Wolfram Sang]

> All review comments are scattered now, please let me know what has to be
> done further,
> Are we going to change the tasklet to irq thread ?
> Are we going to remove batching 64 packets if transaction > 64B and use rx
> fifo threshold ?
> 
> I don't see any issue with current code but if it has to change we need a
> valid reason for the same.
> If nothing to be done, please acknowledge the patch.

Valid request. Has there been any news?

Re: [PATCH v3 5/6] i2c: iproc: handle master read request

[Wolfram Sang]

> All review comments are scattered now, please let me know what has to be
> done further,
> Are we going to change the tasklet to irq thread ?
> Are we going to remove batching 64 packets if transaction > 64B and use rx
> fifo threshold ?
> 
> I don't see any issue with current code but if it has to change we need a
> valid reason for the same.
> If nothing to be done, please acknowledge the patch.

Valid request. Has there been any news?


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

Re: [PATCH v3 5/6] i2c: iproc: handle master read request

[Ray Jui]

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



On 11/19/2020 9:59 PM, Rayagonda Kokatanur wrote:
> Hi Ray and Dhananjay,
> 
> All review comments are scattered now, please let me know what has to be
> done further,
> Are we going to change the tasklet to irq thread ?

It really depends on the time it takes to read data out of the FIFO.
Dhananjay pointed out that your comment indicates reading 10 bytes of
data takes 20 us, i.e., 2 us per byte read. Do you know why it took so
long? The APB bus should be a lot faster than that (in the hundreds of
ns range). I am making the assumption that by the time when you try to
read data out of the FIFO, the data is of course already in the FIFO, so
it's not like you are waiting for data from the I2C bus and I cannot
understand why it took this long.


> Are we going to remove batching 64 packets if transaction > 64B and use
> rx fifo threshold ?
> 

I don't see any issue with batching. It's more efficient and less
context switch overhead.

> I don't see any issue with current code but if it has to change we need
> a valid reason for the same.

I think we need to confirm the exact time it takes to fetch data from
FIFO. Once that's done, we can make a decision between keeping the
tasklet based approach vs irq thread.

Thanks.


> If nothing to be done, please acknowledge the patch.
>  
> Best regards,
> Raygonda
> 
> 
> On Sat, Nov 14, 2020 at 6:47 AM Dhananjay Phadke
> <dphadke@linux.microsoft.com <mailto:dphadke@linux.microsoft.com>> wrote:
> 
>     On Tue, 10 Nov 2020 11:24:36 -0800, Ray Jui wrote:
> 
>     >>>> Yes it's true that for master write-read events both
>     >>>> IS_S_RD_EVENT_SHIFT and IS_S_RX_EVENT_SHIFT  are coming together.
>     >>>> So before the slave starts transmitting data to the master, it
>     should
>     >>>> first read all data from rx-fifo i.e. complete master write and
>     then
>     >>>> process master read.
>     >>>>
>     >>>> To minimise interrupt overhead, we are batching 64bytes.
>     >>>> To keep isr running for less time, we are using a tasklet.
>     >>>> Again to keep the tasklet not running for more than 20u, we
>     have set
>     >>>> max of 10 bytes data read from rx-fifo per tasklet run.
>     >>>>
>     >>>> If we start processing everything in isr and using rx threshold
>     >>>> interrupt, then isr will run for a longer time and this may hog the
>     >>>> system.
>     >>>> For example, to process 10 bytes it takes 20us, to process 30
>     bytes it
>     >>>> takes 60us and so on.
>     >>>> So is it okay to run isr for so long ?
>     >>>>
>     >>>> Keeping all this in mind we thought a tasklet would be a good
>     option
>     >>>> and kept max of 10 bytes read per tasklet.
>     >>>>
>     >>>> Please let me know if you still feel we should not use a
>     tasklet and
>     >>>> don't batch 64 bytes.
>     >>>
>     >>> Deferring to tasklet is OK, could use a kernel thread (i.e.
>     threaded_irq)
>     >>> as i2c rate is quite low.
>     >>>
>     >
>     >kernel thread was proposed in the internal review. I don't see much
>     >benefit of using tasklet. If a thread is blocked from running for more
>     >than several tenth of ms, that's really a system-level issue than an
>     >issue with this driver.
>     >
>     >IMO, it's an overkill to use tasklet here but we can probably leave it
>     >as it is since it does not have a adverse effect and the code ran in
>     >tasklet is short.
>     >
>     >How much time is expected to read 64 bytes from an RX FIFO? Even with
>     >APB bus each register read is expected to be in the tenth or
>     hundreds of
>     >nanosecond range. Reading the entire FIFO of 64 bytes should take less
>     >than 10 us. The interrupt context switch overhead is probably longer
>     >than that. It's much more effective to read all of them in a single
>     >batch than breaking them into multiple batches.
> 
>     OK, there's a general discussions towards removing tasklets, if this
>     fix works with threaded isr, strongly recommend that route.
> 
>     This comment in the code suggested that register reads take long time to
>     drain 64 bytes.
> 
>     >+/*
>     >+ * It takes ~18us to reading 10bytes of data, hence to keep tasklet
>     >+ * running for less time, max slave read per tasklet is set to 10
>     >bytes.
> 
>     @Rayagonda, please take care of hand-off mentioned below, once the
>     tasklet
>     is scheduled, isr should just return and clear status at the end of
>     tasklet.
> 
>     >>
>     >> Few other comments -
>     >>
>     >>> +              /* schedule tasklet to read data later */
>     >>> +              tasklet_schedule(&iproc_i2c->slave_rx_tasklet);
>     >>> +
>     >>> +              /* clear only IS_S_RX_EVENT_SHIFT interrupt */
>     >>> +              iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET,
>     >>> +                               BIT(IS_S_RX_EVENT_SHIFT));
>     >>> +      }
>     >>
>     >> Why clearing one rx interrupt bit here after scheduling tasklet?
>     Should all that
>     >> be done by tasklet? Also should just return after scheduling tasklet?
> 
>     Regards,
>     Dhananjay
> 

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4151 bytes --]

Re: [PATCH v3 5/6] i2c: iproc: handle master read request

[Ray Jui]

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



On 11/19/2020 9:59 PM, Rayagonda Kokatanur wrote:
> Hi Ray and Dhananjay,
> 
> All review comments are scattered now, please let me know what has to be
> done further,
> Are we going to change the tasklet to irq thread ?

It really depends on the time it takes to read data out of the FIFO.
Dhananjay pointed out that your comment indicates reading 10 bytes of
data takes 20 us, i.e., 2 us per byte read. Do you know why it took so
long? The APB bus should be a lot faster than that (in the hundreds of
ns range). I am making the assumption that by the time when you try to
read data out of the FIFO, the data is of course already in the FIFO, so
it's not like you are waiting for data from the I2C bus and I cannot
understand why it took this long.


> Are we going to remove batching 64 packets if transaction > 64B and use
> rx fifo threshold ?
> 

I don't see any issue with batching. It's more efficient and less
context switch overhead.

> I don't see any issue with current code but if it has to change we need
> a valid reason for the same.

I think we need to confirm the exact time it takes to fetch data from
FIFO. Once that's done, we can make a decision between keeping the
tasklet based approach vs irq thread.

Thanks.


> If nothing to be done, please acknowledge the patch.
>  
> Best regards,
> Raygonda
> 
> 
> On Sat, Nov 14, 2020 at 6:47 AM Dhananjay Phadke
> <dphadke@linux.microsoft.com <mailto:dphadke@linux.microsoft.com>> wrote:
> 
>     On Tue, 10 Nov 2020 11:24:36 -0800, Ray Jui wrote:
> 
>     >>>> Yes it's true that for master write-read events both
>     >>>> IS_S_RD_EVENT_SHIFT and IS_S_RX_EVENT_SHIFT  are coming together.
>     >>>> So before the slave starts transmitting data to the master, it
>     should
>     >>>> first read all data from rx-fifo i.e. complete master write and
>     then
>     >>>> process master read.
>     >>>>
>     >>>> To minimise interrupt overhead, we are batching 64bytes.
>     >>>> To keep isr running for less time, we are using a tasklet.
>     >>>> Again to keep the tasklet not running for more than 20u, we
>     have set
>     >>>> max of 10 bytes data read from rx-fifo per tasklet run.
>     >>>>
>     >>>> If we start processing everything in isr and using rx threshold
>     >>>> interrupt, then isr will run for a longer time and this may hog the
>     >>>> system.
>     >>>> For example, to process 10 bytes it takes 20us, to process 30
>     bytes it
>     >>>> takes 60us and so on.
>     >>>> So is it okay to run isr for so long ?
>     >>>>
>     >>>> Keeping all this in mind we thought a tasklet would be a good
>     option
>     >>>> and kept max of 10 bytes read per tasklet.
>     >>>>
>     >>>> Please let me know if you still feel we should not use a
>     tasklet and
>     >>>> don't batch 64 bytes.
>     >>>
>     >>> Deferring to tasklet is OK, could use a kernel thread (i.e.
>     threaded_irq)
>     >>> as i2c rate is quite low.
>     >>>
>     >
>     >kernel thread was proposed in the internal review. I don't see much
>     >benefit of using tasklet. If a thread is blocked from running for more
>     >than several tenth of ms, that's really a system-level issue than an
>     >issue with this driver.
>     >
>     >IMO, it's an overkill to use tasklet here but we can probably leave it
>     >as it is since it does not have a adverse effect and the code ran in
>     >tasklet is short.
>     >
>     >How much time is expected to read 64 bytes from an RX FIFO? Even with
>     >APB bus each register read is expected to be in the tenth or
>     hundreds of
>     >nanosecond range. Reading the entire FIFO of 64 bytes should take less
>     >than 10 us. The interrupt context switch overhead is probably longer
>     >than that. It's much more effective to read all of them in a single
>     >batch than breaking them into multiple batches.
> 
>     OK, there's a general discussions towards removing tasklets, if this
>     fix works with threaded isr, strongly recommend that route.
> 
>     This comment in the code suggested that register reads take long time to
>     drain 64 bytes.
> 
>     >+/*
>     >+ * It takes ~18us to reading 10bytes of data, hence to keep tasklet
>     >+ * running for less time, max slave read per tasklet is set to 10
>     >bytes.
> 
>     @Rayagonda, please take care of hand-off mentioned below, once the
>     tasklet
>     is scheduled, isr should just return and clear status at the end of
>     tasklet.
> 
>     >>
>     >> Few other comments -
>     >>
>     >>> +              /* schedule tasklet to read data later */
>     >>> +              tasklet_schedule(&iproc_i2c->slave_rx_tasklet);
>     >>> +
>     >>> +              /* clear only IS_S_RX_EVENT_SHIFT interrupt */
>     >>> +              iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET,
>     >>> +                               BIT(IS_S_RX_EVENT_SHIFT));
>     >>> +      }
>     >>
>     >> Why clearing one rx interrupt bit here after scheduling tasklet?
>     Should all that
>     >> be done by tasklet? Also should just return after scheduling tasklet?
> 
>     Regards,
>     Dhananjay
> 

[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4151 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH v3 5/6] i2c: iproc: handle master read request

[Ray Jui]

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



On 12/2/2020 6:35 AM, Wolfram Sang wrote:
> 
>> All review comments are scattered now, please let me know what has to be
>> done further,
>> Are we going to change the tasklet to irq thread ?
>> Are we going to remove batching 64 packets if transaction > 64B and use rx
>> fifo threshold ?
>>
>> I don't see any issue with current code but if it has to change we need a
>> valid reason for the same.
>> If nothing to be done, please acknowledge the patch.
> 
> Valid request. Has there been any news?
> 

Sorry for the delay. I just replied.

Thanks,

Ray

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4151 bytes --]

Re: [PATCH v3 5/6] i2c: iproc: handle master read request

[Ray Jui]

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



On 12/2/2020 6:35 AM, Wolfram Sang wrote:
> 
>> All review comments are scattered now, please let me know what has to be
>> done further,
>> Are we going to change the tasklet to irq thread ?
>> Are we going to remove batching 64 packets if transaction > 64B and use rx
>> fifo threshold ?
>>
>> I don't see any issue with current code but if it has to change we need a
>> valid reason for the same.
>> If nothing to be done, please acknowledge the patch.
> 
> Valid request. Has there been any news?
> 

Sorry for the delay. I just replied.

Thanks,

Ray

[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4151 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH v3 5/6] i2c: iproc: handle master read request

[Rayagonda Kokatanur]

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

On Wed, Dec 2, 2020 at 11:14 PM Ray Jui <ray.jui@broadcom.com> wrote:
>
>
>
> On 12/2/2020 6:35 AM, Wolfram Sang wrote:
> >
> >> All review comments are scattered now, please let me know what has to be
> >> done further,
> >> Are we going to change the tasklet to irq thread ?
> >> Are we going to remove batching 64 packets if transaction > 64B and use rx
> >> fifo threshold ?
> >>
> >> I don't see any issue with current code but if it has to change we need a
> >> valid reason for the same.
> >> If nothing to be done, please acknowledge the patch.
> >
> > Valid request. Has there been any news?
> >
>
> Sorry for the delay. I just replied.

This patch is tested and validated with all corner cases and its working.
Can we merge this and take up any improvement as part of separate patch?

Thanks,
Rayagonda

>
>
> Thanks,
>
> Ray

-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4187 bytes --]

Re: [PATCH v3 5/6] i2c: iproc: handle master read request

[Rayagonda Kokatanur]

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

On Wed, Dec 2, 2020 at 11:14 PM Ray Jui <ray.jui@broadcom.com> wrote:
>
>
>
> On 12/2/2020 6:35 AM, Wolfram Sang wrote:
> >
> >> All review comments are scattered now, please let me know what has to be
> >> done further,
> >> Are we going to change the tasklet to irq thread ?
> >> Are we going to remove batching 64 packets if transaction > 64B and use rx
> >> fifo threshold ?
> >>
> >> I don't see any issue with current code but if it has to change we need a
> >> valid reason for the same.
> >> If nothing to be done, please acknowledge the patch.
> >
> > Valid request. Has there been any news?
> >
>
> Sorry for the delay. I just replied.

This patch is tested and validated with all corner cases and its working.
Can we merge this and take up any improvement as part of separate patch?

Thanks,
Rayagonda

>
>
> Thanks,
>
> Ray

-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.

[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4187 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH v3 5/6] i2c: iproc: handle master read request

[Ray Jui]

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



On 12/16/2020 8:08 PM, Rayagonda Kokatanur wrote:
> On Wed, Dec 2, 2020 at 11:14 PM Ray Jui <ray.jui@broadcom.com> wrote:
>>
>>
>>
>> On 12/2/2020 6:35 AM, Wolfram Sang wrote:
>>>
>>>> All review comments are scattered now, please let me know what has to be
>>>> done further,
>>>> Are we going to change the tasklet to irq thread ?
>>>> Are we going to remove batching 64 packets if transaction > 64B and use rx
>>>> fifo threshold ?
>>>>
>>>> I don't see any issue with current code but if it has to change we need a
>>>> valid reason for the same.
>>>> If nothing to be done, please acknowledge the patch.
>>>
>>> Valid request. Has there been any news?
>>>
>>
>> Sorry for the delay. I just replied.
> 
> This patch is tested and validated with all corner cases and its working.
> Can we merge this and take up any improvement as part of separate patch?
> 

I think that makes sense, and I'm okay with these patches going in as
they are now.

Acked-by: Ray Jui <ray.jui@broadcom.com>

But please help to collect precise FIFO access timing (later when you
have time), that would allow us to know if the current defer-to-tasklet
(instead of thread) based approach makes sense or not.

Thanks,

Ray

> Thanks,
> Rayagonda
> 
>>
>>
>> Thanks,
>>
>> Ray

-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4151 bytes --]

Re: [PATCH v3 5/6] i2c: iproc: handle master read request

[Ray Jui]

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



On 12/16/2020 8:08 PM, Rayagonda Kokatanur wrote:
> On Wed, Dec 2, 2020 at 11:14 PM Ray Jui <ray.jui@broadcom.com> wrote:
>>
>>
>>
>> On 12/2/2020 6:35 AM, Wolfram Sang wrote:
>>>
>>>> All review comments are scattered now, please let me know what has to be
>>>> done further,
>>>> Are we going to change the tasklet to irq thread ?
>>>> Are we going to remove batching 64 packets if transaction > 64B and use rx
>>>> fifo threshold ?
>>>>
>>>> I don't see any issue with current code but if it has to change we need a
>>>> valid reason for the same.
>>>> If nothing to be done, please acknowledge the patch.
>>>
>>> Valid request. Has there been any news?
>>>
>>
>> Sorry for the delay. I just replied.
> 
> This patch is tested and validated with all corner cases and its working.
> Can we merge this and take up any improvement as part of separate patch?
> 

I think that makes sense, and I'm okay with these patches going in as
they are now.

Acked-by: Ray Jui <ray.jui@broadcom.com>

But please help to collect precise FIFO access timing (later when you
have time), that would allow us to know if the current defer-to-tasklet
(instead of thread) based approach makes sense or not.

Thanks,

Ray

> Thanks,
> Rayagonda
> 
>>
>>
>> Thanks,
>>
>> Ray

-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.

[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4151 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH v3 5/6] i2c: iproc: handle master read request

[Rayagonda Kokatanur]

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

On Fri, Dec 18, 2020 at 12:41 AM Ray Jui <ray.jui@broadcom.com> wrote:
>
>
>
> On 12/16/2020 8:08 PM, Rayagonda Kokatanur wrote:
> > On Wed, Dec 2, 2020 at 11:14 PM Ray Jui <ray.jui@broadcom.com> wrote:
> >>
> >>
> >>
> >> On 12/2/2020 6:35 AM, Wolfram Sang wrote:
> >>>
> >>>> All review comments are scattered now, please let me know what has to be
> >>>> done further,
> >>>> Are we going to change the tasklet to irq thread ?
> >>>> Are we going to remove batching 64 packets if transaction > 64B and use rx
> >>>> fifo threshold ?
> >>>>
> >>>> I don't see any issue with current code but if it has to change we need a
> >>>> valid reason for the same.
> >>>> If nothing to be done, please acknowledge the patch.
> >>>
> >>> Valid request. Has there been any news?
> >>>
> >>
> >> Sorry for the delay. I just replied.
> >
> > This patch is tested and validated with all corner cases and its working.
> > Can we merge this and take up any improvement as part of separate patch?
> >
>
> I think that makes sense, and I'm okay with these patches going in as
> they are now.
>
> Acked-by: Ray Jui <ray.jui@broadcom.com>

Thank you.

>
> But please help to collect precise FIFO access timing (later when you
> have time), that would allow us to know if the current defer-to-tasklet
> (instead of thread) based approach makes sense or not.
>
> Thanks,
>
> Ray
>
> > Thanks,
> > Rayagonda
> >
> >>
> >>
> >> Thanks,
> >>
> >> Ray

-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4187 bytes --]

Re: [PATCH v3 5/6] i2c: iproc: handle master read request

[Rayagonda Kokatanur]

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

On Fri, Dec 18, 2020 at 12:41 AM Ray Jui <ray.jui@broadcom.com> wrote:
>
>
>
> On 12/16/2020 8:08 PM, Rayagonda Kokatanur wrote:
> > On Wed, Dec 2, 2020 at 11:14 PM Ray Jui <ray.jui@broadcom.com> wrote:
> >>
> >>
> >>
> >> On 12/2/2020 6:35 AM, Wolfram Sang wrote:
> >>>
> >>>> All review comments are scattered now, please let me know what has to be
> >>>> done further,
> >>>> Are we going to change the tasklet to irq thread ?
> >>>> Are we going to remove batching 64 packets if transaction > 64B and use rx
> >>>> fifo threshold ?
> >>>>
> >>>> I don't see any issue with current code but if it has to change we need a
> >>>> valid reason for the same.
> >>>> If nothing to be done, please acknowledge the patch.
> >>>
> >>> Valid request. Has there been any news?
> >>>
> >>
> >> Sorry for the delay. I just replied.
> >
> > This patch is tested and validated with all corner cases and its working.
> > Can we merge this and take up any improvement as part of separate patch?
> >
>
> I think that makes sense, and I'm okay with these patches going in as
> they are now.
>
> Acked-by: Ray Jui <ray.jui@broadcom.com>

Thank you.

>
> But please help to collect precise FIFO access timing (later when you
> have time), that would allow us to know if the current defer-to-tasklet
> (instead of thread) based approach makes sense or not.
>
> Thanks,
>
> Ray
>
> > Thanks,
> > Rayagonda
> >
> >>
> >>
> >> Thanks,
> >>
> >> Ray

-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.

[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4187 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH v3 5/6] i2c: iproc: handle master read request

[Wolfram Sang]

> > I think that makes sense, and I'm okay with these patches going in as
> > they are now.
> >
> > Acked-by: Ray Jui <ray.jui@broadcom.com>
> 
> Thank you.

Yes, thank you everyone.

All applied to for-next, thanks!

> -- 
> This electronic communication and the information and any files transmitted 
> with it, or attached to it, are confidential and are intended solely for 
...

Please remove this paragraph for mailing lists.

Re: [PATCH v3 5/6] i2c: iproc: handle master read request

[Wolfram Sang]

> > I think that makes sense, and I'm okay with these patches going in as
> > they are now.
> >
> > Acked-by: Ray Jui <ray.jui@broadcom.com>
> 
> Thank you.

Yes, thank you everyone.

All applied to for-next, thanks!

> -- 
> This electronic communication and the information and any files transmitted 
> with it, or attached to it, are confidential and are intended solely for 
...

Please remove this paragraph for mailing lists.


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

Re: [PATCH v3 5/6] i2c: iproc: handle master read request

[Florian Fainelli]

On 1/5/21 8:21 AM, Wolfram Sang wrote:
> 
>>> I think that makes sense, and I'm okay with these patches going in as
>>> they are now.
>>>
>>> Acked-by: Ray Jui <ray.jui@broadcom.com>
>>
>> Thank you.
> 
> Yes, thank you everyone.
> 
> All applied to for-next, thanks!
> 
>> -- 
>> This electronic communication and the information and any files transmitted 
>> with it, or attached to it, are confidential and are intended solely for 
> ...
> 
> Please remove this paragraph for mailing lists.

We are working on it, but as you may expect with any large corporations
it is "complicated".
-- 
Florian

Re: [PATCH v3 5/6] i2c: iproc: handle master read request

[Florian Fainelli]

On 1/5/21 8:21 AM, Wolfram Sang wrote:
> 
>>> I think that makes sense, and I'm okay with these patches going in as
>>> they are now.
>>>
>>> Acked-by: Ray Jui <ray.jui@broadcom.com>
>>
>> Thank you.
> 
> Yes, thank you everyone.
> 
> All applied to for-next, thanks!
> 
>> -- 
>> This electronic communication and the information and any files transmitted 
>> with it, or attached to it, are confidential and are intended solely for 
> ...
> 
> Please remove this paragraph for mailing lists.

We are working on it, but as you may expect with any large corporations
it is "complicated".
-- 
Florian

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

Re: [PATCH v3 5/6] i2c: iproc: handle master read request

[Wolfram Sang]

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


> We are working on it, but as you may expect with any large corporations
> it is "complicated".

I understand. Good luck, then!


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

Re: [PATCH v3 5/6] i2c: iproc: handle master read request

[Wolfram Sang]

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


> We are working on it, but as you may expect with any large corporations
> it is "complicated".

I understand. Good luck, then!


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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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